Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

file: Add support for named MDS metadata pool names #15056

Merged

Conversation

NotTheEvilOne
Copy link
Contributor

This commit adds optional support to specify the MDS metadata pool name. It defaults to <fsName>-metadata as current implementation expects but allows customization if needed e.g. if exisiting naming conventions used <fsName>_metadata.

Checklist:

  • Commit Message Formatting: Commit titles and messages follow guidelines in the developer guide.
  • Reviewed the developer guide on Submitting a Pull Request
  • Pending release notes updated with breaking and/or notable changes for the next minor release.
  • Documentation has been updated, if necessary.
  • Unit tests have been added, if necessary.
  • Integration tests have been added, if necessary.

@NotTheEvilOne NotTheEvilOne force-pushed the prs/add-support-for-named-mds-metadata-pools branch from dcb071e to ec854ba Compare November 25, 2024 16:01
metadataPool.Application = cephfsApplication
if metadataPool.Name == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently, the data pool names always have the filesystem name prefix and the customization of the pool name is only after the prefix (see generateDataPoolNames()). For consistency, seems like the metadata pool name should also have the filesystem name prefix. Or is there any need to not use the filesystem name as a prefix?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You are right here. The documentation has resulted in a misunderstanding as it says [...] a pool name can be specified with the name field to override the default generated name; see more below.. But further below it describes that the pool name is generated anyway of what is specified. It only appends it to the filesystem name combined with an -.

Do you see any possibility and does it sound reasonable for your to add a flag to really be able to specify the pool names manually? I could add a flag like generatePoolNames default to true to pools [1].

[1] https://rook.github.io/docs/rook/latest-release/CRDs/Shared-Filesystem/ceph-filesystem-crd/#pools

Copy link
Contributor Author

@NotTheEvilOne NotTheEvilOne Nov 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea behind this request is that I come with migration of exisiting resources in mind. Ceph documents that MDS uses pool names like cephfs_metadata and cephfs_data. Currently there is no way in Rook to use these kinds of names as they always get prefixed including - in it's name.

See https://docs.ceph.com/en/latest/cephfs/createfs/#creating-pools

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you say more about your migration? Rook has never had the goal to take over a ceph cluster. There are too many different assumptions in the deployments of the components such as mons, osds, the naming convention of pools, etc.

If we need to ability to set any pool name for cephfs that will be easy enough if it's needed, I'm more concerned about the attempt to migrate which is a bigger goal.

Copy link
Contributor Author

@NotTheEvilOne NotTheEvilOne Nov 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The request is coming from #14971. A third-party tool provides support on doing an migration with minimal downtime just touching Ceph daemons and replacing them with Rook based ones. A blocker we had is that most traditional Ceph clusters use e.g. cephfs_metadata as documented upstream.

Ceph configuration already stored at e.g. "ceph-mon" daemons are not touched and do work after migration as long as Rook does not try to change these settings. We haven't encountered severe issues in the migration process but our migration scenario required had been pretty straight forward so far.

I've added an boolean now to decide if metadata and data pool names are used as is or default behavior is used.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting to hear the migration is actually working. At some point, more comments on #14971 would be helpful to understand these steps more, or perhaps it's already documented in that rookify project.

@NotTheEvilOne NotTheEvilOne force-pushed the prs/add-support-for-named-mds-metadata-pools branch from ec854ba to ca3ad08 Compare November 27, 2024 18:47
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good, I just reran one of the CI tests to confirm if it was intermittent before merging...

metadataPool.Application = cephfsApplication
if metadataPool.Name == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting to hear the migration is actually working. At some point, more comments on #14971 would be helpful to understand these steps more, or perhaps it's already documented in that rookify project.

@NotTheEvilOne NotTheEvilOne force-pushed the prs/add-support-for-named-mds-metadata-pools branch from 8c9341f to e93a7cc Compare December 4, 2024 13:40
Copy link
Member

@travisn travisn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One more thing...Please also add mention of the new setting in ceph-filesystem-crd.md

@NotTheEvilOne NotTheEvilOne force-pushed the prs/add-support-for-named-mds-metadata-pools branch 3 times, most recently from cc0894f to 33bb7e1 Compare December 5, 2024 09:14
This commit adds optional support to specify the MDS metadata pool name. It defaults to `<fsName>-metadata` as current implementation expects but allows customization if needed e.g. if exisiting naming conventions used `<fsName>_metadata`. Additionally an "preservePoolNames" boolean has been added to indicate that no generated pool names should be used.

Signed-off-by: Tobias Wolf <wolf@b1-systems.de>
@NotTheEvilOne NotTheEvilOne force-pushed the prs/add-support-for-named-mds-metadata-pools branch from 33bb7e1 to 675678f Compare December 6, 2024 08:16
@travisn travisn merged commit a8b7dbf into rook:master Dec 9, 2024
55 checks passed
travisn added a commit that referenced this pull request Dec 9, 2024
file: Add support for named MDS metadata pool names (backport #15056)
travisn added a commit that referenced this pull request Dec 9, 2024
file: Add support for named MDS metadata pool names (backport #15056)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants