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

mgr/volumes: support to reject CephFS clones if cloner threads are not available #52670

Merged
merged 3 commits into from
Feb 20, 2024

Conversation

neesingh-rh
Copy link
Contributor

@neesingh-rh neesingh-rh commented Jul 27, 2023

Allow the cloning only when (pending_clones + in-progress_clones) < max_concurrent_clones, otherwise return the error EAGAIN. This way we solves the issue of stale cephfs clones.

Fixes: https://tracker.ceph.com/issues/59714
Signed-off-by: Neeraj Pratap Singh neesingh@redhat.com

Contribution Guidelines

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

@vshankar vshankar requested a review from kotreshhr August 2, 2023 02:55
@vshankar vshankar marked this pull request as ready for review August 2, 2023 02:55
@neesingh-rh neesingh-rh force-pushed the wip-59714 branch 4 times, most recently from b96184f to b969f0a Compare August 2, 2023 18:48
src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/volume.py Outdated Show resolved Hide resolved
@kotreshhr
Copy link
Contributor

@neesingh-rh I think this should be sent on top of #52765 ?

@neesingh-rh
Copy link
Contributor Author

@neesingh-rh I think this should be sent on top of #52765 ?

Yup, I will do as soon as it merges or should I add those commits in this PR?

@kotreshhr
Copy link
Contributor

@neesingh-rh I think this should be sent on top of #52765 ?

Yup, I will do as soon as it merges or should I add those commits in this PR?

Let's wait for it to merge. I think it's in @vshankar 's testing branch.

@vshankar
Copy link
Contributor

vshankar commented Aug 8, 2023

@neesingh-rh I think this should be sent on top of #52765 ?

Yup, I will do as soon as it merges or should I add those commits in this PR?

Let's wait for it to merge. I think it's in @vshankar 's testing branch.

I'll run PR #52765 through fs:volumes sub-suite to faster the merge process.

@vshankar
Copy link
Contributor

vshankar commented Aug 8, 2023

jenkins retest this please

@neesingh-rh neesingh-rh force-pushed the wip-59714 branch 2 times, most recently from 3fa0299 to 43555ca Compare August 11, 2023 06:21
Copy link
Contributor

@kotreshhr kotreshhr left a comment

Choose a reason for hiding this comment

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

@vshankar I think this feature needs to be optional and enabled by default. It restricts other users to create a clone only when the thread is free. What do you think?

src/pybind/mgr/volumes/fs/volume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved
qa/tasks/cephfs/test_volumes.py Outdated Show resolved Hide resolved
qa/tasks/cephfs/test_volumes.py Show resolved Hide resolved
@vshankar
Copy link
Contributor

@vshankar I think this feature needs to be optional and enabled by default. It restricts other users to create a clone only when the thread is free. What do you think?

I think this feature needs to be enabled by the operator.

Copy link
Contributor

@kotreshhr kotreshhr left a comment

Choose a reason for hiding this comment

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

Looks good to me otherwise. Let's wait for the input whether we should make this configurable.

src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/operations/volume.py Outdated Show resolved Hide resolved
src/pybind/mgr/volumes/fs/volume.py Show resolved Hide resolved
@neesingh-rh
Copy link
Contributor Author

Looks good to me otherwise. Let's wait for the input whether we should make this configurable.

I was working on addition of this functionality ( taking Venky's suggestion #52670 (comment) in mind for now) and have updated the PR with those changes too, Will do the amendments as per the inputs.

@neesingh-rh
Copy link
Contributor Author

jenkins retest this please

Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

@neesingh-rh PTAL at the failure: /a/vshankar-2024-01-22_07:03:31-fs-wip-vshankar-testing-20240119.075157-1-testing-default-smithi/7525715

qa/tasks/cephfs/test_volumes.py Show resolved Hide resolved
vshankar added a commit to vshankar/ceph that referenced this pull request Jan 25, 2024
* refs/pull/52670/head:
	doc: add the reject the clone when threads are not available feature in the doc
	qa: add test cases for the support to reject clones feature
	mgr/volumes: support to reject CephFS clones if cloner threads are not available

Reviewed-by: Venky Shankar <vshankar@redhat.com>
Reviewed-by: Kotresh Hiremath Ravishankar <khiremat@redhat.com>
…t available

CephFS clone creation have a limit of 4 parallel clones by default at a time and rest
of the clone create requests are queued. This makes CephFS cloning very slow when
there is large amount of clones being created.After this patch clone requests won't be accepeted
when the requests exceed the `max_concurrent_clones` config value.

Fixes:  https://tracker.ceph.com/issues/59714
Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Copy link
Contributor

@anthonyeleven anthonyeleven left a comment

Choose a reason for hiding this comment

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

wording requests

PendingReleaseNotes Outdated Show resolved Hide resolved
PendingReleaseNotes Outdated Show resolved Hide resolved
doc/cephfs/fs-volumes.rst Outdated Show resolved Hide resolved
neeraj pratap singh added 2 commits February 13, 2024 14:45
Fixes: https://tracker.ceph.com/issues/59714
Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
…in the document

Fixes: https://tracker.ceph.com/issues/59714
Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
vshankar added a commit to vshankar/ceph that referenced this pull request Feb 19, 2024
* refs/pull/52670/head:
	doc: add the reject the clone when threads are not available feature in the document
	qa: add test cases for the support to reject clones feature
	mgr/volumes: support to reject CephFS clones if cloner threads are not available

Reviewed-by: Kotresh Hiremath Ravishankar <khiremat@redhat.com>
Reviewed-by: Venky Shankar <vshankar@redhat.com>
@vshankar
Copy link
Contributor

jenkins test windows

@vshankar
Copy link
Contributor

@neesingh-rh I had to include an additional commit to gixup failures in the fs:volumes. No need to refresh - I'll merge this and the additional commit tomorrow.

Copy link
Contributor

@vshankar vshankar left a comment

Choose a reason for hiding this comment

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants