tools/cephfs_mirror: enable multiple peers #52456

dparmar18 · 2023-07-14T13:51:32Z

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

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. "pacific"), please refer to Submitting Patches to Ceph - Backports for the proper workflow.

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)
- Includes unit test(s)
- Includes integration test(s)
- Includes bug reproducer
- No tests

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 · 2023-07-15T21:14:04Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2023-07-16T13:19:42Z

teuthology job passed http://pulpito.front.sepia.ceph.com/dparmar-2023-07-16_08:15:59-fs:mirror-enable_multipeer-distro-default-smithi/

dparmar18 · 2023-07-16T13:32:48Z

@vshankar There's just one test case added currently and I'm sure more needs to be added; can you help identify what all should I test, some test cases that I identified based on existing tests could be:

Test disabling mirroring with multiple peers
Blocklist mirror daemon; restart and make sure peers are intact
Test mirror daemon status with multiple peers
Incremental sync
With parent snapshot/purged snapshot

Do let me know if the above list needs any addition/removal

vshankar · 2023-07-17T10:15:12Z

qa/tasks/cephfs/test_mirroring.py

@@ -1305,3 +1319,23 @@ def test_local_and_remote_dir_root_mode(self):
        self.disable_mirroring(self.primary_fs_name, self.primary_fs_id)
        self.mount_a.run_shell(["rmdir", "l1/.snap/snap0"])
        self.mount_a.run_shell(["rmdir", "l1"])
+
+    def test_mirror_multiple_peers(self):


This should also check if the snapshots have been synchronized to all the peers.

I suggest, creating a separate class for

fan-out peer setup (site-a->site-b, site-a->site-c, site-a->site-d)

cascaded setup (site-a->site-b->site-c->site-d)

Mostly, you'd need to verify if the snapshots get synchronized to all peers - this would involve checks via peer status command.

dparmar18 · 2023-07-20T13:02:33Z

@vshankar local test case failure(unable to run admin-daemon cmd) https://termbin.com/9j8zf

dparmar18 · 2023-07-20T13:05:57Z

other admin-daemon cmds not working https://termbin.com/wuz5

I know these cmds are incomplete and ought to fail but it should've not said this right, should've threw some suggestions(yesterday worked fine)

vshankar · 2023-07-24T12:55:25Z

@dparmar18 In my list. On it soon.

dparmar18 · 2023-07-24T13:13:23Z

@dparmar18 In my list. On it soon.

@vshankar ah i was talking about this one #52524. This PR is still wip

vshankar · 2023-07-24T13:47:14Z

@dparmar18 In my list. On it soon.

@vshankar ah i was talking about this one #52524. This PR is still wip

Ah ok. Best is to tag me on the PR mentioning that it's ready for review.

dparmar18 · 2023-07-27T07:30:06Z

@vshankar the test_mirroring.py needs further more changes to accommodate the cascaded setup, it includes touching class TestMirroing's tests too. I'll need to add those changes here I'll modify the helper methods and doing this will require those test cases to be modified, i hope thats fine right?

vshankar · 2023-07-27T09:32:09Z

@vshankar the test_mirroring.py needs further more changes to accommodate the cascaded setup, it includes touching class TestMirroing's tests too. I'll need to add those changes here I'll modify the helper methods and doing this will require those test cases to be modified, i hope thats fine right?

Touching which test cases? The one's in TestMirroring that exists now? You can add the fan-out/cascaded tests in a separate class, since the only test that would make sense there is to validate that the snapshots synchronize to all the peers.

dparmar18 · 2023-07-27T09:39:48Z

@vshankar the test_mirroring.py needs further more changes to accommodate the cascaded setup, it includes touching class TestMirroing's tests too. I'll need to add those changes here I'll modify the helper methods and doing this will require those test cases to be modified, i hope thats fine right?

Touching which test cases? The one's in TestMirroring that exists now? You can add the fan-out/cascaded tests in a separate class, since the only test that would make sense there is to validate that the snapshots synchronize to all the peers.

i have added a new class for helpers(because the classes i wrote had to use helpers present in TestMirroring); TestMirroring and other two classes(that i added for testing fan out and cascaded setup) now use those helpers. These helpers have some values hardcoded like fs_map = status.get_fsmap_byname(self.primary_fs_name) in get_peer_uuid() which doesn't work in cascaded setup as i might want to get the fsmap of fs other than primary fs. This would mean I need to make the helper get_peer_uuid() accept a fs, this would require changes to all the test cases and other helpers using get_peer_uuid()

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2023-07-31T09:19:09Z

class TestMirroring passed with my modifications - http://pulpito.front.sepia.ceph.com/dparmar-2023-07-28_10:58:32-fs:mirror-enable_multipeer-distro-default-smithi/

Please nudge when ready for review.

test classes are ready but you mentioned on IRC last week that we need more YAMLs for these classes to only run a subset of tests? Should I put them in qa/suites/fs/mirror/tasks/ as mirror_multiple_peers_fanout.yaml and mirror_multiple_peers_cascaded.yaml ?

vshankar · 2023-07-31T09:42:00Z

class TestMirroring passed with my modifications - http://pulpito.front.sepia.ceph.com/dparmar-2023-07-28_10:58:32-fs:mirror-enable_multipeer-distro-default-smithi/

Please nudge when ready for review.

test classes are ready but you mentioned on IRC last week that we need more YAMLs for these classes to only run a subset of tests? Should I put them in qa/suites/fs/mirror/tasks/ as mirror_multiple_peers_fanout.yaml and mirror_multiple_peers_cascaded.yaml ?

Yeh - mirror_fanout.yaml and mirror_cascaded.yaml sounds a bit more appropriate.

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 · 2023-08-02T03:25:58Z

all passed http://pulpito.front.sepia.ceph.com/dparmar-2023-08-01_18:44:35-fs:mirror-enable_multipeer-distro-default-smithi/

dparmar18 · 2023-08-03T06:06:30Z

@vshankar there are a lot of changes made to the existing code (and there are some more plans in future to the qa code), if you need help, we can go through the changes together. Do let me know

vshankar · 2023-08-16T07:59:32Z

qa/tasks/cephfs/test_mirroring.py


+class MirroringHelpers(CephFSTestCase):


If this is an helper class, I believe this need not be subclassed from CephFSTestCase, yes?

it'a a bit complicated, the actual plan is, YES make this helper class an actual helper class but it requires a lot of changes that are unrelated to this PR(making changes to all other test cases). It's a bit hard to explain you in comment, if possible do let know if/when you can join for a call or maybe discuss this post standup tomorrow?

Sorry but hard to explain why this needs to be subclassed from CephFSTestCase? Could you explain a bit and I'll try my best to understand.

Note that it's not just me who is required to review PRs - so taking time to explain here is going to benefit everyone.

So the thing is that the change to enable multiple peers required me to create a helper class since the existing helpers in the class TestMirroring has hard coded values that only applies to single peer but just adding those helpers into a class and use it into test classes directly is not enough because the helpers that are used in the existing class TestMirroring (that are also used in the classes I wrote) now need modifications. This forced me to make changes in this PR that are not related (to the tracker). So in order to make minimal unrelated changes, some helpers still depends on the CephFSTestCase, thus you see it being inherited.

I'm planning to make it a proper helper class but I'm not sure which way to go with; either I can make the changes here in this PR altogether or in a new PR. I'm open to suggestions.

So the thing is that the change to enable multiple peers required me to create a helper class since the existing helpers in the class TestMirroring has hard coded values that only applies to single peer but just adding those helpers into a class and use it into test classes directly is not enough because the helpers that are used in the existing class TestMirroring (that are also used in the classes I wrote) now need modifications. This forced me to make changes in this PR that are not related (to the tracker). So in order to make minimal unrelated changes, some helpers still depends on the CephFSTestCase, thus you see it being inherited.

I'm planning to make it a proper helper class but I'm not sure which way to go with; either I can make the changes here in this PR altogether or in a new PR. I'm open to suggestions.

IMO having is as a separate (standalone) helper would be the better. I understand that its more work that's the current implementation, but it way better in maintenance in the longer run.

so you want me to do it in this PR itself right?

vshankar · 2023-08-16T08:00:04Z

qa/tasks/cephfs/test_mirroring.py

@@ -242,6 +258,24 @@ def get_mirror_daemon_status(self):
        log.debug(f'status: {status}')
        return status

+
+class TestMirroring(MirroringHelpers):


And, then this can be subclassed from MirroringHelpers and CephFSTestCase.

vshankar · 2023-08-17T07:14:05Z

jenkins test make check

github-actions · 2023-11-07T18:17:43Z

This pull request can no longer be automatically merged: a rebase is needed and changes have to be manually resolved

github-actions · 2024-01-16T09:01:37Z

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.

github-actions · 2024-02-15T10:01:36Z

This pull request has been automatically closed because there has been no activity for 90 days. Please feel free to reopen this pull request (or open a new one) if the proposed change is still appropriate. Thank you for your contribution!

vshankar · 2024-03-19T06:08:48Z

As discussed, @dparmar18 will revisit this once other higher priority items are taken care.

github-actions bot added cephfs Ceph File System tests labels Jul 14, 2023

dparmar18 force-pushed the enable_multipeer branch from 37ff527 to 8b44f47 Compare July 14, 2023 18:16

dparmar18 changed the title ~~qa: test multiple peers~~ tools/cephfs_mirror: enable multiple peers Jul 15, 2023

github-actions bot added the needs-rebase label Jul 15, 2023

dparmar18 force-pushed the enable_multipeer branch from 2ed4b18 to f68827a Compare July 16, 2023 08:14

github-actions bot removed the needs-rebase label Jul 16, 2023

dparmar18 force-pushed the enable_multipeer branch from f68827a to d1b2c1b Compare July 16, 2023 13:05

tools/cephfs_mirror: enable multiple peers

700e654

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the enable_multipeer branch from d1b2c1b to fb33952 Compare July 16, 2023 13:12

dparmar18 marked this pull request as ready for review July 16, 2023 13:19

dparmar18 requested review from a team and vshankar July 16, 2023 13:19

vshankar reviewed Jul 17, 2023

View reviewed changes

dparmar18 force-pushed the enable_multipeer branch 2 times, most recently from 9c61fc0 to 2e0d510 Compare July 24, 2023 11:39

dparmar18 mentioned this pull request Jul 25, 2023

tools/cephfs_mirror: only perform actions if init succeed #52524

Merged

14 tasks

dparmar18 force-pushed the enable_multipeer branch 2 times, most recently from a493198 to 8587d46 Compare July 25, 2023 12:08

qa: test multiple peers in fan out setup

b456726

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the enable_multipeer branch 2 times, most recently from c32dc5e to eab5762 Compare July 31, 2023 09:06

vshankar requested a review from a team August 1, 2023 06:10

dparmar18 force-pushed the enable_multipeer branch from 3b2e417 to e1f6ec6 Compare August 1, 2023 16:18

dparmar18 added 3 commits August 1, 2023 23:20

qa: test multiple peers in cascaded setup

7b4e3c5

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

qa: add yamls for new classes

9a6863e

Fixes: https://tracker.ceph.com/issues/61978 Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

qa: add two more clients

5d589d6

Signed-off-by: Dhairya Parmar <dparmar@redhat.com>

dparmar18 force-pushed the enable_multipeer branch from e1f6ec6 to 5d589d6 Compare August 1, 2023 17:51

dparmar18 marked this pull request as ready for review August 2, 2023 02:54

dparmar18 requested a review from vshankar August 2, 2023 05:15

dparmar18 added the needs-review label Aug 3, 2023

vshankar requested a review from joscollin August 16, 2023 07:51

vshankar requested changes Aug 16, 2023

View reviewed changes

dparmar18 requested review from vshankar and a team August 28, 2023 11:11

github-actions bot added the needs-rebase label Nov 7, 2023

github-actions bot added the stale label Jan 16, 2024

github-actions bot closed this Feb 15, 2024

dparmar18 reopened this Feb 15, 2024

github-actions bot removed the stale label Feb 15, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tools/cephfs_mirror: enable multiple peers #52456

tools/cephfs_mirror: enable multiple peers #52456

dparmar18 commented Jul 14, 2023 •

edited

github-actions bot commented Jul 15, 2023

dparmar18 commented Jul 16, 2023

dparmar18 commented Jul 16, 2023

vshankar Jul 17, 2023 •

edited

dparmar18 commented Jul 20, 2023

dparmar18 commented Jul 20, 2023

vshankar commented Jul 24, 2023

dparmar18 commented Jul 24, 2023

vshankar commented Jul 24, 2023

dparmar18 commented Jul 27, 2023

vshankar commented Jul 27, 2023

dparmar18 commented Jul 27, 2023 •

edited

dparmar18 commented Jul 31, 2023

vshankar commented Jul 31, 2023

dparmar18 commented Aug 2, 2023

dparmar18 commented Aug 3, 2023

vshankar Aug 16, 2023

dparmar18 Aug 16, 2023

vshankar Aug 17, 2023

vshankar Aug 17, 2023

dparmar18 Aug 28, 2023

vshankar Nov 13, 2023

dparmar18 Nov 17, 2023

vshankar Nov 17, 2023

vshankar Aug 16, 2023

vshankar commented Aug 17, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Jan 16, 2024

github-actions bot commented Feb 15, 2024

vshankar commented Mar 19, 2024

tools/cephfs_mirror: enable multiple peers #52456

Are you sure you want to change the base?

tools/cephfs_mirror: enable multiple peers #52456

Conversation

dparmar18 commented Jul 14, 2023 • edited

Contribution Guidelines

Checklist

github-actions bot commented Jul 15, 2023

dparmar18 commented Jul 16, 2023

dparmar18 commented Jul 16, 2023

vshankar Jul 17, 2023 • edited

Choose a reason for hiding this comment

dparmar18 commented Jul 20, 2023

dparmar18 commented Jul 20, 2023

vshankar commented Jul 24, 2023

dparmar18 commented Jul 24, 2023

vshankar commented Jul 24, 2023

dparmar18 commented Jul 27, 2023

vshankar commented Jul 27, 2023

dparmar18 commented Jul 27, 2023 • edited

dparmar18 commented Jul 31, 2023

vshankar commented Jul 31, 2023

dparmar18 commented Aug 2, 2023

dparmar18 commented Aug 3, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vshankar commented Aug 17, 2023

github-actions bot commented Nov 7, 2023

github-actions bot commented Jan 16, 2024

github-actions bot commented Feb 15, 2024

vshankar commented Mar 19, 2024

dparmar18 commented Jul 14, 2023 •

edited

vshankar Jul 17, 2023 •

edited

dparmar18 commented Jul 27, 2023 •

edited