cephfs_mirror: avoid latest changes on the source fs to enable mirroring by joscollin · Pull Request #61539 · ceph/ceph

joscollin · 2025-01-27T12:51:46Z

This avoids considering latest changes from the source filesystem for
the mirroring of already existing snapshots. Thus the destination filesystem
and snapshots would be created based only on the source snapshots.
The destination fs would be a replica of the last snapshot taken.

To check this, do:

$ sudo ./bin/mount.ceph admin@.a=/ /mnt/cephfs1 -o conf=$(pwd)/ceph.conf;
$ sudo ./bin/mount.ceph admin@.remotefs=/ /mnt/cephfs2 -o conf=$(pwd)/ceph.conf;

Make some changes and take a snapshot:

$ mkdir -p /mnt/cephfs1/dir/d1
$ touch /mnt/cephfs1/dir/d1/file1
$ mkdir -p /mnt/cephfs1/dir/.snap/snap1;

Make some more changes & take another snapshot:

$ touch /mnt/cephfs1/dir/d1/file2
$ mkdir -p /mnt/cephfs1/dir/.snap/snap2;

Make a change and don't take snapshot:
$ rm /mnt/cephfs1/dir/d1/file2

Add dir for mirroring:
$ ./bin/ceph fs snapshot mirror add a /dir

Results:

$ tree /mnt/cephfs1/dir/.snap
/mnt/cephfs1/dir/.snap
├── snap1
│   └── d1
│       └── file1
└── snap2
    └── d1
        ├── file1
        └── file2

$ tree /mnt/cephfs2/dir/.snap
/mnt/cephfs2/dir/.snap
├── snap1
│   └── d1
│       └── file1
└── snap2
    └── d1
        ├── file1
        └── file2

$ tree /mnt/cephfs1/dir
/mnt/cephfs1/dir
└── d1
    └── file1

$ tree /mnt/cephfs2/dir
/mnt/cephfs2/dir
└── d1
    ├── file1
    └── file2

Fixes: https://tracker.ceph.com/issues/68567
Fixes: https://tracker.ceph.com/issues/69671

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)
- 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
jenkins test rook e2e

joscollin · 2025-01-27T12:59:28Z

jenkins test api

joscollin · 2025-01-28T03:23:07Z

jenkins test api

joscollin · 2025-01-28T04:06:51Z

jenkins test api

joscollin · 2025-01-28T04:07:06Z

Tests passed:
https://pulpito.ceph.com/jcollin-2025-01-28_01:15:02-fs:mirror-wip-jcollin-testing-2701-distro-default-smithi/
https://pulpito.ceph.com/jcollin-2025-01-28_05:16:12-fs:mirror-wip-jcollin-testing-2701-distro-default-smithi/
https://pulpito.ceph.com/jcollin-2025-01-28_10:29:33-fs:mirror-wip-jcollin-testing-2701-distro-default-smithi/

joscollin · 2025-01-28T11:08:52Z

QA test case in progress.

joscollin · 2025-01-29T04:18:05Z

jenkins test make check

joscollin · 2025-01-29T04:18:18Z

jenkins test windows

joscollin · 2025-01-29T04:34:40Z

QA test passed:
https://pulpito.ceph.com/jcollin-2025-01-29_03:18:32-fs:mirror-wip-jcollin-testing-2701-distro-default-smithi/

joscollin · 2025-01-29T09:52:02Z

jenkins test make check

joscollin · 2025-01-30T12:50:17Z

Putting DNM, as this needs more fix for purged_snapshot failure.

joscollin · 2025-02-01T13:26:57Z

Passes the entire fs:mirror and fs:valgrind tests:
https://pulpito.ceph.com/jcollin-2025-02-01_01:45:29-fs:mirror-wip-B68567_B69671-distro-default-smithi/
https://pulpito.ceph.com/jcollin-2025-02-01_01:48:47-fs:valgrind-wip-B68567_B69671-distro-default-smithi/

joscollin · 2025-02-01T13:35:39Z

@vshankar
This PR is ready for review.
Please note that PeerReplayer::validate_source is more or less a copy-paste of PeerReplayer::pre_sync_check_and_open_handles. I would prefer using PeerReplayer::pre_sync_check_and_open_handles instead as it does the switching of local prev snapshot to remote dir_root. WDYT?

joscollin · 2025-02-07T04:09:05Z

The new qa test code updated and fs:mirror and fs:valgrind passed:
https://pulpito.ceph.com/jcollin-2025-02-06_13:57:56-fs:mirror-wip-B68567_B69671-distro-default-smithi/
https://pulpito.ceph.com/jcollin-2025-02-06_13:59:33-fs:valgrind-wip-B68567_B69671-distro-default-smithi/

@vshankar This PR is ready for review.

vshankar · 2025-02-07T16:51:46Z

The new qa test code updated and fs:mirror and fs:valgrind passed:
https://pulpito.ceph.com/jcollin-2025-02-06_13:57:56-fs:mirror-wip-B68567_B69671-distro-default-smithi/
https://pulpito.ceph.com/jcollin-2025-02-06_13:59:33-fs:valgrind-wip-B68567_B69671-distro-default-smithi/

@vshankar This PR is ready for review.

Thanks. I will have a look and put it to test.

vshankar · 2025-02-10T09:35:20Z

src/tools/cephfs_mirror/PeerReplayer.cc

-                     CEPH_STATX_MODE | CEPH_STATX_UID | CEPH_STATX_GID |
-                     CEPH_STATX_SIZE | CEPH_STATX_ATIME | CEPH_STATX_MTIME,
-                     AT_STATX_DONT_SYNC | AT_SYMLINK_NOFOLLOW);
+      npath = entry_path(epath, nname);


@joscollin Could you share a sample log snippet involving these paths without and with the fix?

I don't have it handy. I need to reproduce it with old code then.

@joscollin - if possible please share this, otherwise I will check it when I pull in this change to test the file block diff integration with the mirror daemon.

@vshankar Let me see, if I could reproduce it. I don't have a build now. So please don't wait for me. If you are testing block diff, please pull this PR.

vshankar · 2025-02-10T09:36:59Z

qa/tasks/cephfs/test_mirroring.py

                    break
        self.disable_mirroring(self.primary_fs_name, self.primary_fs_id)
+
+    def test_cephfs_mirror_sync_already_existing_snapshots(self):


Isn't this a redundant test? I believe there is at least one test that checks if snapshots are synchronized as expected.

No, this should be the only test that does add_directory after snapshot-ting. Is there any function does that already?

But, how is this test relevant to this fix?

@vshankar
This test checks if the changes are taken from the previous snapshot or not, while adding a directory.
There are multiple things gets fixed by this PR.

Incremental sync after adding a directory for mirroring -> We have test for this.

Incremental sync of the already existing snapshots while adding a directory for mirroring -> We don't have test for this.

switch to remote dir_root when local prev snapshot is unavailable -> We have test for this.

Hope that clarifies.

Isn't (2) pretty much covered with (1)? Do we really need a separate test for it?

@vshankar I think it's better to have a separate test for it (add_directory after snapshot-ting), as the same thing is reported here: #54633 (comment).

vshankar · 2025-02-10T09:40:08Z

src/tools/cephfs_mirror/PeerReplayer.cc

 }

+// Determines if the source is the local (previous) snapshot or the remote dir_root
+int PeerReplayer::validate_source(const std::string &dir_root,


Sorry - I don't understand why is this required. PeerReplayer would use local snapshot comparison (and hence local mount handles) when it's sure that the data on the remote file system (live) exactly matches the last snapshot transferred. This is determined using an xattr (from the remote file system directory).

Right. But this change is made to resolve the test failure https://tracker.ceph.com/issues/69671. This happens only when the source snapshot is deleted during an incremental sync. Then the incremental sync has to check and make the switch to remote dir_root as per test_cephfs_mirror_sync_with_purged_snapshot.

Previously, before snapdiff, this was taken care of as everything was a full sync. But with incremental sync, as per the test (test_cephfs_mirror_sync_with_purged_snapshot), the source has to be validated each time. Hope this clarifies.

So, the bug was that the mirror daemon was (incorrectly) using incremental sync with remote directory snapshot comparison?

@vshankar
There's no issue with incremental sync with remote directory snapshot comparison. But the snapdiff code (the new do_synchronize function) wasn't making the switch to the remote dir_root. There was no such mechanism in the new do_synchronize. I think I've missed it back then. That's the reason for this failure https://tracker.ceph.com/issues/69671. With this PR, the switch happens, as per the original design.

OK. So, if a snapshot is deleted when it was used for incremental sync, the mirror daemon code was still incorrectly using incremental sync in the next iteration, yes? If that's the case, then can you put this up in the commit message please?

Yes it was.
Updated the commit message @vshankar.

@vshankar There's no issue with incremental sync with remote directory snapshot comparison. But the snapdiff code (the new do_synchronize function) wasn't making the switch to the remote dir_root. There was no such mechanism in the new do_synchronize. I think I've missed it back then. That's the reason for this failure https://tracker.ceph.com/issues/69671. With this PR, the switch happens, as per the original design.

Sorry for the wrong explanation. I didn't miss to implement the switching. This is a regression caused by 7a747bc. That's the reason test_cephfs_mirror_sync_with_purged_snapshot fails now. It was passing earlier, but now it fails.

Updated the commit message with the details @vshankar

joscollin · 2025-02-13T14:36:24Z

@vshankar ping

joscollin · 2025-02-24T04:26:56Z

@vshankar ping :)
This is needed for other PRs to test.

vshankar · 2025-02-24T04:34:11Z

@vshankar ping :)
This is needed for other PRs to test.

I am planning to test this with #61937 w/ mirror daemon change to use file block diff. I still have to push that bit of code to the PR.

vshankar · 2025-02-24T04:40:19Z

@joscollin a suggestion - please do not mark a comment as resolved until the participants in a comment have agreed to either incorporate the fix or leave the change as status-quo.

joscollin · 2025-02-24T04:57:56Z

@joscollin a suggestion - please do not mark a comment as resolved until the participants in a comment have agreed to either incorporate the fix or leave the change as status-quo.

@vshankar I was following what you've suggested earlier. You wanted it to mark the comment resolved after replying. Anyway, I'll stop clicking resolve button.

vshankar · 2025-02-24T05:04:19Z

@joscollin a suggestion - please do not mark a comment as resolved until the participants in a comment have agreed to either incorporate the fix or leave the change as status-quo.

@vshankar I was following what you've suggested earlier. You wanted it to mark the comment resolved after replying. Anyway, I'll stop clicking resolve button.

That's not what I meant - If there is agreement on the comment then its fine to mark the comment as resolved.

This avoids considering latest changes from the source filesystem for the mirroring of already existing snapshots. Thus the destination filesystem and snapshots would be created based only on the source snapshots. The destination fs would be a replica of the last snapshot taken. Fixes: https://tracker.ceph.com/issues/68567 Signed-off-by: Jos Collin <jcollin@redhat.com>

…napshot is unavailable Fixes the incremental sync not switching the source to remote dir_root, when the local previous snapshot is deleted. This is a regression caused by ceph@7a747bc. Fixes: https://tracker.ceph.com/issues/69671 Signed-off-by: Jos Collin <jcollin@redhat.com>

Fixes: https://tracker.ceph.com/issues/68567 Signed-off-by: Jos Collin <jcollin@redhat.com>

joscollin · 2025-03-17T06:17:23Z

jenkins test make check arm64

vshankar · 2025-03-28T04:24:41Z

Relevant commits pulled into PR #62250

github-actions bot added the cephfs Ceph File System label Jan 27, 2025

joscollin added the bug-fix label Jan 27, 2025

joscollin requested a review from a team January 28, 2025 05:35

joscollin mentioned this pull request Jan 28, 2025

reef: cephfs_mirror: use snapdiff api for incremental syncing #58985

Open

joscollin force-pushed the wip-B68567_B69671 branch from 61492b3 to f0b52c3 Compare January 28, 2025 06:08

joscollin changed the title ~~cephfs_mirror: avoid latest changes on the source fs for mirroring~~ cephfs_mirror: avoid latest changes on the source fs to enable mirroring Jan 28, 2025

joscollin force-pushed the wip-B68567_B69671 branch from f0b52c3 to fbd1e7b Compare January 28, 2025 12:03

github-actions bot added the tests label Jan 28, 2025

joscollin force-pushed the wip-B68567_B69671 branch from fbd1e7b to 767b9f0 Compare January 29, 2025 03:48

joscollin force-pushed the wip-B68567_B69671 branch from 767b9f0 to 11be04e Compare January 30, 2025 09:32

joscollin added the DNM label Jan 30, 2025

joscollin force-pushed the wip-B68567_B69671 branch from 11be04e to 09ef1b4 Compare February 1, 2025 13:24

joscollin removed the DNM label Feb 1, 2025

joscollin requested a review from vshankar February 4, 2025 16:29

joscollin force-pushed the wip-B68567_B69671 branch from 09ef1b4 to f36485f Compare February 6, 2025 15:23

vshankar reviewed Feb 10, 2025

View reviewed changes

joscollin requested a review from vshankar February 12, 2025 13:55

joscollin added wip-jcollin-testing and removed wip-jcollin-testing labels Feb 20, 2025

joscollin force-pushed the wip-B68567_B69671 branch from f36485f to e7aff00 Compare February 24, 2025 10:21

joscollin added 3 commits February 25, 2025 11:34

qa: add test for syncing already existing snapshots

956642d

Fixes: https://tracker.ceph.com/issues/68567 Signed-off-by: Jos Collin <jcollin@redhat.com>

joscollin force-pushed the wip-B68567_B69671 branch from e7aff00 to 956642d Compare February 25, 2025 06:07

vshankar mentioned this pull request Feb 28, 2025

mds/client: file blockdiff support #61937

Merged

14 tasks

joscollin mentioned this pull request Mar 4, 2025

qa: fix test_mirroring_init_failure_with_recovery failed to get state: "failed" #60052

Closed

14 tasks

joscollin added wip-jcollin-testing and removed wip-jcollin-testing labels Mar 7, 2025

joscollin mentioned this pull request Mar 26, 2025

cephfs-mirror: integrate blockdiff with mirror daemon #62250

Merged

14 tasks

vshankar closed this Mar 28, 2025

joscollin deleted the wip-B68567_B69671 branch March 28, 2025 04:28

Conversation

joscollin commented Jan 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution Guidelines

Checklist

Uh oh!

joscollin commented Jan 27, 2025

Uh oh!

joscollin commented Jan 28, 2025

Uh oh!

joscollin commented Jan 28, 2025

Uh oh!

joscollin commented Jan 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

joscollin commented Jan 28, 2025

Uh oh!

joscollin commented Jan 29, 2025

Uh oh!

joscollin commented Jan 29, 2025

Uh oh!

joscollin commented Jan 29, 2025

Uh oh!

joscollin commented Jan 29, 2025

Uh oh!

joscollin commented Jan 30, 2025

Uh oh!

joscollin commented Feb 1, 2025

Uh oh!

joscollin commented Feb 1, 2025

Uh oh!

joscollin commented Feb 7, 2025

Uh oh!

vshankar commented Feb 7, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

joscollin commented Feb 13, 2025

Uh oh!

joscollin commented Feb 24, 2025

Uh oh!

vshankar commented Feb 24, 2025

Uh oh!

vshankar commented Feb 24, 2025

Uh oh!

joscollin commented Feb 24, 2025

joscollin commented Jan 27, 2025 •

edited

Loading

joscollin commented Jan 28, 2025 •

edited

Loading