mds: require config list to be sorted by batrick · Pull Request #59088 · ceph/ceph

batrick · 2024-08-08T00:55:33Z

Fun little programming project for the obsessive mind. I find it easier to have the configs grouped by component (part of the config name usually) so keeping the alphabetized is helpful. It also helps to avoid conflicts if everyone would otherwise be appending to the end of the array.

Contribution Guidelines

To sign and title your commits, please refer to Submitting Patches to Ceph.
If you are submitting a fix for a stable branch (e.g. "quincy"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.
When filling out the below checklist, you may click boxes directly in the GitHub web UI. When entering or editing the entire PR message in the GitHub web UI editor, you may also select a checklist item by adding an x between the brackets: [x]. Spaces and capitalization matter when checking off items this way.

Checklist

Tracker (select at least one)
- References tracker ticket
- Very recent bug; references commit where it was introduced
- New feature (ticket optional)
- Doc update (no ticket needed)
- Code cleanup (no ticket needed)
Component impact
- Affects Dashboard, opened tracker ticket
- Affects Orchestrator, opened tracker ticket
- No impact that needs to be tracked
Documentation (select at least one)
- Updates relevant documentation
- No doc update is appropriate
Tests (select at least one)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

Show available Jenkins commands

jenkins retest this please
jenkins test classic perf
jenkins test crimson perf
jenkins test signed
jenkins test make check
jenkins test make check arm64
jenkins test submodules
jenkins test dashboard
jenkins test dashboard cephadm
jenkins test api
jenkins test docs
jenkins render docs
jenkins test ceph-volume all
jenkins test ceph-volume tox
jenkins test windows
jenkins test rook e2e

batrick · 2024-08-08T02:24:46Z

jenkins test api

batrick · 2024-08-09T16:04:18Z

jenkins test api

vshankar · 2024-08-09T18:00:39Z

jenkins test api

batrick · 2024-08-10T13:59:45Z

ninja: build stopped: interrupted by user.
Build step 'Execute shell' marked build as failure

😮

batrick · 2024-08-10T13:59:57Z

jenkins test api

batrick · 2024-08-13T12:59:10Z

This PR is under test in https://tracker.ceph.com/issues/67516.

dparmar18 · 2024-08-13T18:36:15Z

src/common/make_array.h

+template<typename CT, typename... Args>
+constexpr auto make_array(Args&&... args)
+{
+  return std::array<CT, sizeof...(Args)>{std::forward<CT>(args)...};


Shouldn't we be forwarding args with type Args?

Suggested change

return std::array<CT, sizeof...(Args)>{std::forward<CT>(args)...};

return std::array<CT, sizeof...(Args)>{std::forward<Args>(args)...};

I've since found std::to_array so this code no longer exists.

But to answer your question: I think so.

vshankar · 2024-08-14T09:30:29Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)

Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

Signed-off-by: Patrick Donnelly <[email protected]>

This helps keeps related options near each other. Signed-off-by: Patrick Donnelly <[email protected]>

batrick · 2024-08-14T13:41:22Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)

Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

vshankar · 2024-08-14T14:12:17Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)
Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

Oh, I meant something in compile/build time that creates a cc source with keys from mds.yaml.in.

dparmar18

LGTM

batrick · 2024-08-14T14:45:21Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)
Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

Oh, I meant something in compile/build time that creates a cc source with keys from mds.yaml.in.

I'm not sure I follow what you're trying to accomplish by doing so?

vshankar · 2024-08-14T15:11:49Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)
Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

Oh, I meant something in compile/build time that creates a cc source with keys from mds.yaml.in.

I'm not sure I follow what you're trying to accomplish by doing so?

Trying to have a single place where we specify config options and then automagically have the list of keys in MDS source.

Never mind - that's my obsessive mind working late evenings.

batrick · 2024-08-14T15:15:39Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)
Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

Oh, I meant something in compile/build time that creates a cc source with keys from mds.yaml.in.

I'm not sure I follow what you're trying to accomplish by doing so?

Trying to have a single place where we specify config options and then automagically have the list of keys in MDS source.

Never mind - that's my obsessive mind working late evenings.

ahhh, that would be interesting. Actually, you could just pull the list of configs with the RUNTIME flag.

Edit: and you'd want to indicate which RUNTIME configs are watched for which daemon.

vshankar · 2024-08-14T15:24:44Z

Fun little programming project for the obsessive mind

Keeps the neurons firing :)
Anyway, I'm fine with having this constraint in place. Follow up question unrelated to this change: Is it possible to auto-generate this list of keys from mds.yaml..in by any chance?

I think the idea of this interface was to prevent unnecessary "config change" deliveries to the daemon. I personally don't see the issue unless you are acquiring expensive locks but that can be avoided usually. So to answer your question: I think that adding all the keys from mds.yaml.in would be abusing the config API.

Oh, I meant something in compile/build time that creates a cc source with keys from mds.yaml.in.

I'm not sure I follow what you're trying to accomplish by doing so?

Trying to have a single place where we specify config options and then automagically have the list of keys in MDS source.
Never mind - that's my obsessive mind working late evenings.

ahhh, that would be interesting. Actually, you could just pull the list of configs with the RUNTIME flag.

Edit: and you'd want to indicate which RUNTIME configs are watched for which daemon.

Agree. You don't have to do that in this PR though. I'll create a ticket for our enthusiasts :)

batrick · 2024-08-14T15:34:51Z

jenkins test make check arm64

batrick · 2024-08-14T17:54:19Z

jenkins test make check arm64

batrick · 2024-08-15T00:52:15Z

jenkins test make check arm64

batrick · 2024-08-15T00:52:45Z

This PR is under test in https://tracker.ceph.com/issues/67563.

batrick · 2024-08-15T12:41:44Z

jenkins test make check arm64

batrick · 2024-08-27T17:13:45Z

https://tracker.ceph.com/issues/67563#note-1

* refs/pull/59088/head: mds: add compile time checks for sortedness mds: sort conf keys Reviewed-by: Dhairya Parmar <[email protected]>

github-actions bot added cephfs Ceph File System common labels Aug 8, 2024

batrick requested a review from a team August 9, 2024 16:07

batrick added the wip-pdonnell-testing label Aug 9, 2024

dparmar18 reviewed Aug 13, 2024

View reviewed changes

batrick added 2 commits August 14, 2024 09:33

mds: sort conf keys

6e257b5

Signed-off-by: Patrick Donnelly <[email protected]>

mds: add compile time checks for sortedness

5c3dddf

This helps keeps related options near each other. Signed-off-by: Patrick Donnelly <[email protected]>

batrick force-pushed the mds-conf-sorted branch from 40c45da to 5c3dddf Compare August 14, 2024 13:33

dparmar18 approved these changes Aug 14, 2024

View reviewed changes

hkadam134 added needs-qa wip-hemanth-testing labels Aug 19, 2024

batrick merged commit 086910c into ceph:main Aug 27, 2024

batrick deleted the mds-conf-sorted branch August 27, 2024 17:09

pritha-srivastava pushed a commit to pritha-srivastava/ceph that referenced this pull request Aug 28, 2024

Merge PR ceph#59088 into main

e23d9ed

* refs/pull/59088/head: mds: add compile time checks for sortedness mds: sort conf keys Reviewed-by: Dhairya Parmar <[email protected]>

	return std::array<CT, sizeof...(Args)>{std::forward<CT>(args)...};
	return std::array<CT, sizeof...(Args)>{std::forward<Args>(args)...};

Conversation

batrick commented Aug 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

batrick commented Aug 8, 2024

Uh oh!

batrick commented Aug 9, 2024

Uh oh!

vshankar commented Aug 9, 2024

Uh oh!

batrick commented Aug 10, 2024

Uh oh!

batrick commented Aug 10, 2024

Uh oh!

batrick commented Aug 13, 2024

Uh oh!

dparmar18 Aug 13, 2024

Choose a reason for hiding this comment

Uh oh!

batrick Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

batrick commented Aug 14, 2024

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

dparmar18 left a comment

Choose a reason for hiding this comment

Uh oh!

batrick commented Aug 14, 2024

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

batrick commented Aug 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vshankar commented Aug 14, 2024

Uh oh!

batrick commented Aug 14, 2024

Uh oh!

batrick commented Aug 14, 2024

Uh oh!

batrick commented Aug 15, 2024

Uh oh!

batrick commented Aug 15, 2024

Uh oh!

batrick commented Aug 15, 2024

Uh oh!

batrick commented Aug 27, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

batrick commented Aug 8, 2024 •

edited

Loading

batrick Aug 14, 2024 •

edited

Loading

batrick commented Aug 14, 2024 •

edited

Loading