Skip to content

mgr/vol: count number of ongoing clones in CloneProgressReporter...#61212

Merged
rishabh-d-dave merged 5 commits intoceph:mainfrom
rishabh-d-dave:mgr-vol-count-clones
Apr 22, 2025
Merged

mgr/vol: count number of ongoing clones in CloneProgressReporter...#61212
rishabh-d-dave merged 5 commits intoceph:mainfrom
rishabh-d-dave:mgr-vol-count-clones

Conversation

@rishabh-d-dave
Copy link
Contributor

@rishabh-d-dave rishabh-d-dave commented Jan 2, 2025

instead of relying on the config value "mgr/volumes/max_concurrent_clones".
This reliance is not really needed since CloneProgressReporter goes
through all clone entries and collect info about every clone anyways. If
it takes one more step to get clone state it wouldn't need to reply on
this config option.

Fixes: https://tracker.ceph.com/issues/67987
Signed-off-by: Rishabh Dave ridave@redhat.com

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 requested a review from a team January 2, 2025 15:27
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jan 14, 2025

@vshankar This is one of the follow up PRs that we had discussed before merging of "clone stats" PR. cc @ceph/cephfs

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Jan 14, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Feb 9, 2025

@vshankar gentle reminder, PTAL. Note: this is a follow up of the idea we discussed during "clone stats" PR.

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Feb 18, 2025

@vshankar gentle reminder, PTAL. Note: this is a follow up of the idea we discussed during "clone stats" PR.

@vshankar pinging again

@vshankar
Copy link
Contributor

vshankar commented Feb 18, 2025

@vshankar gentle reminder, PTAL. Note: this is a follow up of the idea we discussed during "clone stats" PR.

@vshankar pinging again

This will have to wait a bit due to downstream work that I have been involved with.

@rishabh-d-dave rishabh-d-dave added wip-rishabh-testing Rishabh's testing label and removed wip-rishabh-testing Rishabh's testing label labels Feb 28, 2025
vshankar
vshankar reviewed Mar 10, 2025
View reviewed changes
@rishabh-d-dave rishabh-d-dave force-pushed the mgr-vol-count-clones branch 4 times, most recently from d57f8a6 to 5b2400d Compare March 17, 2025 12:05
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

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

I tested this PR with fs:volumes sub-suite over last couple of weeks -

https://pulpito.ceph.com/rishabh-2025-03-15_19:38:51-fs:volumes-rishabh-count-clones-testing-default-smithi/
https://pulpito.ceph.com/rishabh-2025-03-15_14:26:59-fs:volumes-rishabh-count-clones-testing-default-smithi/
https://pulpito.ceph.com/rishabh-2025-03-04_06:20:43-fs:volumes-rishabh-count-clones-testing-default-smithi/

And tests are running successfully now - https://pulpito.ceph.com/rishabh-2025-03-17_12:06:02-fs:volumes-rishabh-count-clones-testing-default-smithi/

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

Did one more run for fs:volumes after today's change, it ran successfully - https://pulpito.ceph.com/rishabh-2025-03-18_12:33:45-fs:volumes-rishabh-count-clones-testing-default-smithi/

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Mar 18, 2025

jenkins test api

@rishabh-d-dave
mgr/vol: count number of ongoing clones in CloneProgressReporter...
e04f527 
instead of relying on the config value "mgr/volumes/max_concurrent_clones".
This reliance is not really needed since CloneProgressReporter goes
through all clone entries and collect info about every clone anyways. If
it takes one more step to get clone state it wouldn't need to reply on
this config option.

Fixes: https://tracker.ceph.com/issues/67987
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
qa/cephfs: move helpers to a new class for resuability
edda358 
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
qa/cephfs: move tests related to CloneProgressReporter to a new file
62d29c6 
So that there is a separate file where several different groups of tests
for CloneProgressReporter can reside together.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
mchangir
mchangir reviewed Apr 1, 2025
View reviewed changes
mchangir
mchangir reviewed Apr 1, 2025
View reviewed changes
@rishabh-d-dave rishabh-d-dave force-pushed the mgr-vol-count-clones branch 2 times, most recently from f0fdacf to 8128d75 Compare April 3, 2025 14:26
@rishabh-d-dave
qa/cephfs: test ongoing clones counter in CloneProgressReporter
9f74f85 
Test that CloneProgressReporter counts number of ongoing clones works
fine.

Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave rishabh-d-dave requested a review from mchangir April 3, 2025 14:46
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 3, 2025

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 3, 2025

jenkins test make check arm64

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 3, 2025

jenkins test windows

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 3, 2025

jenkins test api

mchangir
mchangir approved these changes Apr 4, 2025
View reviewed changes
Copy link
Contributor

@mchangir mchangir left a comment

Choose a reason for hiding this comment

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

LGTM

@rishabh-d-dave rishabh-d-dave added the wip-rishabh-testing Rishabh's testing label label Apr 11, 2025
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 11, 2025

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

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 13, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 16, 2025

jenkins test make check arm64

@rishabh-d-dave
mgr/vol: handle exception when path for lstat() goes missing
e14b8bb 
It might happen that clone index entry goes missing because a clone job
was completed or cancelled. In such a case, lstat() to clone entry's
path would fail. Catch the exception in such a case and handle it so
that clone progress reporter thread doesn't crash.

Crashing of clone progress reporter thread causes clone progress bars to
not be removed from "ceph status" output when they should, resulting in
these bars to being in stuck in the output forever.

Fixes: https://tracker.ceph.com/issues/70941
Signed-off-by: Rishabh Dave <ridave@redhat.com>
@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 16, 2025 •
edited
Loading

small change in commit title, that's it.

mchangir
mchangir approved these changes Apr 16, 2025
View reviewed changes
Copy link
Contributor

@mchangir mchangir left a comment

Choose a reason for hiding this comment

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

LGTM

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 17, 2025

jenkins test api

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 17, 2025

jenkins test make check

@rishabh-d-dave
Copy link
Contributor Author

rishabh-d-dave commented Apr 17, 2025

jenkins test make check arm64

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

rishabh-d-dave commented Apr 22, 2025

jenkins test make check arm64

rishabh-d-dave
rishabh-d-dave commented Apr 22, 2025
View reviewed changes
Copy link
Contributor Author

@rishabh-d-dave rishabh-d-dave left a comment

Choose a reason for hiding this comment

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

QA run was successful - https://tracker.ceph.com/projects/cephfs/wiki/QA_main_2025#wip-rishabh-testing-20250414181222-debug

Waiting for "make check arm64" CI job to pass before merging.

@rishabh-d-dave rishabh-d-dave added ready-to-merge passed-qa for PRs that have passed QA but there's an on-going conversation, so merging would be premature labels Apr 22, 2025
@rishabh-d-dave rishabh-d-dave merged commit 089cd8e into ceph:main Apr 22, 2025
12 checks passed
@rishabh-d-dave rishabh-d-dave deleted the mgr-vol-count-clones branch April 22, 2025 15:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vshankar vshankar vshankar left review comments

@mchangir mchangir mchangir approved these changes

Assignees

No one assigned

Labels

cephfs Ceph File System needs-review passed-qa for PRs that have passed QA but there's an on-going conversation, so merging would be premature 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.

3 participants

@rishabh-d-dave @vshankar @mchangir