Skip to content

mgr/vol: allow passing pool names to "fs volume create" cmd#61732

Merged
rishabh-d-dave merged 5 commits intoceph:mainfrom
rishabh-d-dave:mgr-vol-pools
Mar 21, 2025
Merged

mgr/vol: allow passing pool names to "fs volume create" cmd#61732
rishabh-d-dave merged 5 commits intoceph:mainfrom
rishabh-d-dave:mgr-vol-pools

Conversation

@rishabh-d-dave
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave commented Feb 8, 2025 •
edited
Loading

Allow passing names of existing data pool and metadata pool to ceph fs volume create command so that the command will use these pools for creating volume/FS instead of creating new pools. Passing name of a non-empty pool will cause the command to abort.

Fixes: https://tracker.ceph.com/issues/69878

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)
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

@rishabh-d-dave rishabh-d-dave added cephfs Ceph File System tests labels Feb 8, 2025
@rishabh-d-dave rishabh-d-dave requested review from a team, batrick and vshankar February 8, 2025 18:37
@github-actions github-actions bot added the pybind label Feb 8, 2025
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Feb 9, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Feb 9, 2025

jenkins test make check arm64

@vshankar
Copy link
Contributor

vshankar commented Feb 10, 2025

c.f.: https://tracker.ceph.com/issues/69878#note-3

@vshankar
Copy link
Contributor

vshankar commented Feb 28, 2025

@nizamial09 reached out to me regarding this. I had discussed with @rishabh-d-dave earlier this week and the conclusion is that this PR requires some additional work before it can be reviewed and put to test.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 3, 2025 •
edited
Loading

I had few quetsions about last few bits of this PR, I had emailed Patrick and Venky last to last regarding it but since they were busy with some urgent work, the conversation didn't happen. Anyways, the conversation has happened and I'll update the PR shortly.

@rishabh-d-dave rishabh-d-dave requested a review from a team as a code owner March 3, 2025 16:50
vshankar
vshankar reviewed Mar 3, 2025
View reviewed changes
@rishabh-d-dave rishabh-d-dave force-pushed the mgr-vol-pools branch 2 times, most recently from 6c7135f to 7027a1f Compare March 5, 2025 15:09
@rishabh-d-dave rishabh-d-dave requested a review from vshankar March 5, 2025 15:10
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 6, 2025

@vshankar PTAL

@rishabh-d-dave rishabh-d-dave mentioned this pull request Mar 7, 2025
Open
14 tasks
@rishabh-d-dave rishabh-d-dave force-pushed the mgr-vol-pools branch 3 times, most recently from 19d9d3b to 00661c0 Compare March 14, 2025 12:22
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 14, 2025

@vshankar PTAL

ping @vshankar

vshankar
vshankar requested changes Mar 14, 2025
View reviewed changes
@nizamial09
Copy link
Member

nizamial09 commented Mar 18, 2025

We can introduce flag --dont-rm-pools

What about introducing a flag for removing the pool once the volume is deleted. something like --force-remove-pools. Because IMO pools are where data's are stored and removing them by default might not be good and an unexpected behavior. So probably letting the user to choose whether to remove or keep the pools would be a nice thing and that could be something we can document. This way it doesn't clash with the existing behavior as well and we don't need to store any states to indicate whether this is a user created pool or the other.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

@vshankar

What's even not nicer is deleting pools that were pre-existing and which were supplied by the user to volume creation time. Maybe it's fine or maybe not. It's not entirely incorrect to delete pools on volume deletion, but I'm trying to get a nicer user experience and not have angry users. If this means documenting it for now, then I'm fine with that.

Agreed. That's why I proposed providing option to enable/disable pool deletion so that users could pick whatever they want.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

@nizamial09

This way it doesn't clash with the existing behavior as well and we don't need to store any states to indicate whether this is a user created pool or the other.

That's a good idea: leaving the current default behaviour undisturbed and providing an option to alter that default behaviour.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

QA run was successful for this PR - https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20250315

@vshankar
Copy link
Contributor

vshankar commented Mar 18, 2025

QA run was successful for this PR - https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20250315

This is mostly mergeable in it current form with status-quo functionality during volume removal. Let's wait till tomorrow in case we want to change that bit.

@vshankar
Copy link
Contributor

vshankar commented Mar 19, 2025

What about introducing a flag for removing the pool once the volume is deleted. something like --force-remove-pools. Because IMO pools are where data's are stored and removing them by default might not be good and an unexpected behavior. So probably letting the user to choose whether to remove or keep the pools would be a nice thing and that could be something we can document. This way it doesn't clash with the existing behavior as well and we don't need to store any states to indicate whether this is a user created pool or the other.

Removing pools has always been the default and also for the reason that reusing existing pools with data to create a file system is risky.

@rishabh-d-dave
mgr/vol: allow passing pool names to "fs volume create" cmd
e794a19 
Fixes: https://tracker.ceph.com/issues/69878
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
qa/cephfs: separate the tests for "ceph fs volume create" cmd
058769c 
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
qa/cephfs: test passing pool names to "fs volume create" cmd
3ae1930 
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 19, 2025

about last push: fixed the merge conflict.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 19, 2025

jenkins test make check arm64

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 19, 2025

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 19, 2025

jenkins test make check arm64

anthonyeleven
anthonyeleven reviewed Mar 19, 2025
View reviewed changes
@rishabh-d-dave
doc/cephfs: mention new options for "fs volume create" cmd
3044bf7 
Command "ceph fs volume create" accepts 2 new options to allow users to
pass data and metadata pool name. Update docs to include mention of both
the options.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
PendingReleaseNote: add note that "volume create" accepts pool names...
a74dbbe 
in args.

Add a release note that "ceph fs volume create" command allows users to
pass pool names to "ceph fs volume create" command.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 20, 2025

jenkins test windows

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 20, 2025

jenkins test api

1 similar comment
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 20, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 21, 2025

Link to QA result comment again - #61732 (comment)

@rishabh-d-dave rishabh-d-dave merged commit 6fc1a6d into ceph:main Mar 21, 2025
7 checks passed
This was referenced Mar 24, 2025
Open
Closed
@rishabh-d-dave rishabh-d-dave deleted the mgr-vol-pools branch April 13, 2025 12:18
@rishabh-d-dave rishabh-d-dave mentioned this pull request Sep 24, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anthonyeleven anthonyeleven anthonyeleven left review comments

@vshankar vshankar vshankar approved these changes

@batrick batrick Awaiting requested review from batrick

Assignees

No one assigned

Labels

cephfs Ceph File System documentation pybind ready-to-merge tests wip-rishabh-testing Rishabh's testing label

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rishabh-d-dave @vshankar @nizamial09 @anthonyeleven