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

mds: prevent scrubbing for standby-replay MDS #53301

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

neesingh-rh
Copy link
Contributor

@neesingh-rh neesingh-rh commented Sep 6, 2023

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

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

@github-actions github-actions bot added the cephfs Ceph File System label Sep 6, 2023
src/mds/MDSRank.cc Outdated Show resolved Hide resolved
@neesingh-rh neesingh-rh force-pushed the wip-62537 branch 2 times, most recently from 631bb8b to c181dd8 Compare September 13, 2023 09:46
@vshankar
Copy link
Contributor

@vshankar
Copy link
Contributor

vshankar commented Oct 4, 2023

@neesingh-rh please ping when this ready for review again.

@neesingh-rh
Copy link
Contributor Author

@neesingh-rh please ping when this ready for review again.

I have updated the PR, ready for re-review.

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

vshankar added a commit to vshankar/ceph that referenced this pull request Oct 7, 2023
* refs/pull/53301/head:
	mds: prevent scrub start for standby-replay MDS

Reviewed-by: Venky Shankar <vshankar@redhat.com>
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 LGTM -- please add a test.

@github-actions github-actions bot added the tests label Oct 10, 2023
@neesingh-rh
Copy link
Contributor Author

@neesingh-rh LGTM -- please add a test.

Added the test. PTAL

qa/tasks/cephfs/test_scrub_checks.py Outdated Show resolved Hide resolved
# start the scrub and verify
with self.assertRaises(CommandFailedError) as ce:
self.fs.run_scrub(["start", abs_test_path, "recursive"])
self.assertEqual(ce.exception.exitstatus, errno.EINVAL)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
self.assertEqual(ce.exception.exitstatus, errno.EINVAL)
self.assertEqual(ce.exception.exitstatus, errno.EINVAL)

Copy link
Member

Choose a reason for hiding this comment

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

and others below

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We should be checking for the error status only after getting the command fail exit status. I guess it should be this way only. https://github.com/ceph/ceph/blob/quincy/qa/tasks/cephfs/test_scrub_checks.py#L354

Copy link
Contributor

Choose a reason for hiding this comment

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

ce would be available inside with .., isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with.. is an alternative for try-catch, how can the statement be executed after the exception is encountered at the run_scrub only. Pls correct me if you mean something else.

Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is fine too


# start the scrub and verify
with self.assertRaises(CommandFailedError) as ce:
self.fs.run_scrub(["start", abs_test_path, "recursive"])
Copy link
Member

Choose a reason for hiding this comment

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

I'm puzzled. Is this test not racing with the MDS becoming active?

I also expected to see a scrub command directed at the standby-replay daemon.

Copy link
Contributor

@vshankar vshankar Apr 2, 2024

Choose a reason for hiding this comment

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

Spoke to @neesingh-rh - this needs fixing. Fetch the s-r mds daemons id and do a ceph tell mds.<> scrub start to the s-r daemon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the PR with the proposed changes.

@vshankar
Copy link
Contributor

@neesingh-rh ping?

Copy link

This pull request has been automatically marked as stale because it has not had any activity for 60 days. It will be closed if no further activity occurs for another 30 days.
If you are a maintainer or core committer, please follow-up on this pull request to identify what steps should be taken by the author to move this proposed change forward.
If you are the author of this pull request, thank you for your proposed contribution. If you believe this change is still appropriate, please ensure that any feedback has been addressed and ask for a code review.

@neesingh-rh
Copy link
Contributor Author

@neesingh-rh status of this?

In progress!!

@neesingh-rh
Copy link
Contributor Author

@vshankar Here's the run link with the latest changes: https://pulpito.ceph.com/neesingh-2024-04-25_09:36:50-fs:functional-wip-neesingh-testing-240416-distro-default-smithi/

@neesingh-rh
Copy link
Contributor Author

jenkins test make check

@neesingh-rh
Copy link
Contributor Author

@neesingh-rh
Copy link
Contributor Author

jenkins retest this please

@vshankar
Copy link
Contributor

@vshankar ping?

In my queue for reviews.

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.

LGTM

@vshankar
Copy link
Contributor

vshankar commented Jun 3, 2024

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

@neesingh-rh
Copy link
Contributor Author

jenkins test make check

@vshankar
Copy link
Contributor

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

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.

@vshankar
Copy link
Contributor

jenkins test make check

2 similar comments
@vshankar
Copy link
Contributor

jenkins test make check

@vshankar
Copy link
Contributor

jenkins test make check

@vshankar
Copy link
Contributor

vshankar commented Jul 1, 2024

@neesingh-rh could you rebase please? Jenkins tests aren't passing - maybe a rebase would kick it up.

neeraj pratap singh added 2 commits July 1, 2024 12:36
Fixes: https://tracker.ceph.com/issues/62537
Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Fixes: https://tracker.ceph.com/issues/62537
Signed-off-by: Neeraj Pratap Singh <neesingh@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
5 participants