rbd-mirror: prevent image deletion if remote image is not primary#63270
Merged
rbd-mirror: prevent image deletion if remote image is not primary#63270
Conversation
34098e4 to
50a6025
Compare
idryomov
reviewed
May 27, 2025
src/tools/rbd_mirror/image_replayer/snapshot/PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
50a6025 to
1760682
Compare
idryomov
reviewed
May 29, 2025
src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
4cc46e3 to
358ebc1
Compare
Contributor
Author
|
jenkins test make check |
idryomov
reviewed
Jun 2, 2025
Contributor
idryomov
left a comment
There was a problem hiding this comment.
It would be nice to add corresponding tests where is_remote_primary() would return false. For example, ResyncRequestedRemoteNotPrimary to src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc, InitResyncRequestedRemoteNotPrimary to src/test/rbd_mirror/image_replayer/journal/test_mock_Replayer.cc, etc. Place them after *ResyncRequested tests.
src/tools/rbd_mirror/image_replayer/journal/PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
358ebc1 to
6381169
Compare
Config Diff Tool Output- removed: rgw_bucket_eexist_override
- removed: breakpad
! changed: rgw_sts_key: old: Key used for encrypting/ decrypting role session tokens. This key must consist of 16 hexadecimal characters, which can be generated by the command 'openssl rand -hex 16'. All radosgw instances in a zone should use the same key. In multisite configurations, all zones in a realm should use the same key.
! changed: rgw_sts_key: new: Key used for encrypting/ decrypting session token.
! changed: osd_scrub_min_interval: old: The desired interval between scrubs of a specific PG. Note that this option must be set at ``global`` scope, or for both ``mgr`` and``osd``.
! changed: osd_scrub_min_interval: new: The desired interval between scrubs of a specific PG.
! changed: osd_deep_scrub_interval_cv: old: The coefficient of variation for the deep scrub interval, specified as a ratio. On average, the next deep scrub for a PG is scheduled osd_deep_scrub_interval after the last deep scrub . The actual time is randomized to a normal distribution with a standard deviation of osd_deep_scrub_interval * osd_deep_scrub_interval_cv (clamped to within 2 standard deviations). The default value guarantees that 95% of deep scrubs will be scheduled in the range [0.8 * osd_deep_scrub_interval, 1.2 * osd_deep_scrub_interval].
! changed: osd_deep_scrub_interval_cv: new: The coefficient of variation for the deep scrub interval, specified as a ratio. On average, the next deep scrub for a PG is scheduled osd_deep_scrub_interval after the last deep scrub . The actual time is randomized to a normal distribution with a standard deviation of osd_deep_scrub_interval * osd_deep_scrub_interval_cv (clamped to within 2 standard deviations). The default value guarantees that 95% of the deep scrubs will be scheduled in the range [0.8 * osd_deep_scrub_interval, 1.2 * osd_deep_scrub_interval].
! changed: osd_deep_scrub_interval_cv: old: deep scrub intervals are varied by a random amount to prevent stampedes. This parameter determines the amount of variation. Technically ``osd_deep_scrub_interval_cv`` is the coefficient of variation for the deep scrub interval.
! changed: osd_deep_scrub_interval_cv: new: deep scrub intervals are varied by a random amount to prevent stampedes. This parameter determines the amount of variation. Technically - osd_deep_scrub_interval_cv is the coefficient of variation for the deep scrub interval.
! changed: osd_deep_scrub_interval: old: Deep scrub each PG (i.e., verify data checksums) at least this often. Note that this option must be set at ``global`` scope, or for both ``mgr`` and``osd``.
! changed: osd_deep_scrub_interval: new: Deep scrub each PG (i.e., verify data checksums) at least this often
! changed: osd_scrub_max_interval: old: Scrub each PG no less often than this interval. Note that this option must be set at ``global`` scope, or for both ``mgr`` and``osd``.
! changed: osd_scrub_max_interval: new: Scrub each PG no less often than this interval
The above configuration changes are found in the PR. Please update the relevant release documentation if necessary. |
sunilangadi2
reviewed
Jun 5, 2025
qa/workunits/rbd/rbd_mirror.sh
Outdated
| wait_for_status_in_pool_dir ${CLUSTER1} ${POOL} ${test_resync_image} 'up+unknown' 'remote image is not primary' | ||
| promote_image ${CLUSTER1} ${POOL} ${test_resync_image} | ||
| compare_images ${CLUSTER1} ${CLUSTER2} ${POOL} ${POOL} ${test_resync_image} | ||
| remove_image_retry ${CLUSTER1} ${POOL} ${test_resync_image} |
There was a problem hiding this comment.
This can be fixed by ensuring that image deletion during a resync is only allowed when the remote image is confirmed to be primary.
If the primary image is deleted during a resync, what guarantees do we have that the secondary gets cleaned up properly and doesn't remain orphaned?
Does this logic also works against deleting a parent image of a clone during resync (especially with deep cloning support) ?
If a primary image is deleted without any resync operation involved, does it automatically trigger deletion of the corresponding secondary image as well?
make sure it shouldn't affect the existing workflow
idryomov
reviewed
Jun 6, 2025
src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/journal/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
6381169 to
01ced3a
Compare
idryomov
reviewed
Jun 8, 2025
src/test/rbd_mirror/image_replayer/journal/test_mock_PrepareReplayRequest.cc
Outdated
Show resolved
Hide resolved
01ced3a to
5aa5b86
Compare
idryomov
approved these changes
Jun 9, 2025
Contributor
|
We appear to have an issue with OMAP cleanup on The image in question is the one created by the new test: This happens both for snapshot-based and journal-based mirroring. Example jobs: https://pulpito.ceph.com/dis-2025-06-09_10:16:26-rbd-wip-dis-testing-distro-default-smithi/8316587 (snapshot-based) @VinayBhaskar-V This should be easy to reproduce, could you please investigate? |
Contributor
Author
I checked it and i think this is happening because in the current test case i had written i am not waiting for the promotion to complete before removing image on cluster1, which results in deleting the image on only on cluster1, and the image on cluster2 remains in an up+error state. |
a1beec1 to
c55abef
Compare
Contributor
|
The rerun with the amended test looks better, but I still got two related failures (and some seemingly unrelated, will have to look at them separately): https://pulpito.ceph.com/dis-2025-06-11_18:54:43-rbd-wip-dis-testing-distro-default-smithi/8322945 Here are excerpts from the first job: From the respective rbd-mirror daemon on cluster2: From the respective rbd-mirror daemon on cluster1: So the image was deleted by rbd-mirror daemon on cluster1 at 2025-06-11T20:22:52.731+0000. But according to the test logic, all rbd-mirror daemons on cluster1 should have been stopped at that time: @VinayBhaskar-V I see two issues here:
(1) should be fairly easy to track down -- it's likely a problem with how the test is set up or the helper. (2) may require a change in the approach... I'd probably start with putting together a similar test case where rbd-mirror daemon doesn't get stopped to gauge the extent of the problem. |
Contributor
Yup, the test just needs to be conditioned on |
Contributor
Author
While the rbd-mirror deamon continues : If the resync is triggered after completion of demotion on primary then everything is going as expected. |
c55abef to
5636df5
Compare
Contributor
Author
|
@idryomov I have made changes in snapshot replayer and it resolves the issue of deletion of image on secondary when resync is requested even when the rbd-mirror deamon is running. I have also tested the rbd_mirror.sh for snapshot mode, all tests passed. |
Contributor
I see a problem there. Consider a scenario where the remote image is primary and the local image is non-primary but also somehow corrupted such that rbd-mirror daemon can't make sense of it -- one example would be I'd try dealing with the resync marker after the remote image is refreshed (i.e. after |
Contributor
Author
Got it.Thanks for the clarification.
I think we can do like this but i have a small doubt is there a guarantee that the most recent mirror snapshot always contains the remote_mirror_peer_uuid? Like not in the current case of 2 clusters, if there are multiple peers, will the case still be same? So i am thinking to add something like this in handle_refresh_remote_image after reordering the refreshes. Please correct me if i am wrong. |
Contributor
I don't think it should be considered. The disposition (promotion state) of the image is global and is determined by the most recent mirror snapshot -- the image can't be primary for one peer and non-primary for another peer in case of multiple peers. When the user runs Note that |
719ded7 to
382c531
Compare
78bf3d3 to
d7d384f
Compare
ajarr
requested changes
Jul 14, 2025
Contributor
ajarr
left a comment
There was a problem hiding this comment.
I have a question in a test, and have requested a change in state diagram.
Otherwise, looks very good
d7d384f to
a9e7ac9
Compare
Contributor
|
The gate job failures look unrelated |
Contributor
Author
|
jenkins test api |
Contributor
Author
|
jenkins test make check |
idryomov
reviewed
Jul 22, 2025
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
src/test/rbd_mirror/image_replayer/snapshot/test_mock_Replayer.cc
Outdated
Show resolved
Hide resolved
A resync on a mirrored image may incorrectly results in the local image being deleted even when the remote image is no longer primary. This issue can occur under the following conditions: * if resync is requested on the secondary before the remote image has been fully demoted * if the demotion of the primary image is not mirrored due to the rbd-mirror daemon being offline. This can be fixed by ensuring that image deletion during a resync is only allowed when the remote image is confirmed to be primary. This commit fixes the issue only for snapshot based mirroring mode Fixes: https://tracker.ceph.com/issues/70948 Signed-off-by: VinayBhaskar-V <vvarada@redhat.com>
a9e7ac9 to
e14afbc
Compare
Contributor
|
jenkins test make check |
Contributor
|
jenkins test make check arm64 |
1 similar comment
Contributor
|
jenkins test make check arm64 |
idryomov
approved these changes
Jul 29, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A resync on a mirrored image may incorrectly results in the local image being deleted even when the remote image is no longer primary.
This issue can occur under the following conditions:
been fully demoted
due to the rbd-mirror daemon being offline.
This can be fixed by ensuring that image deletion during a resync is only allowed when the remote image is confirmed to be primary.
This PR fixes the issue only for snapshot based mirroring mode.
Fixes: https://tracker.ceph.com/issues/70948
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
xbetween the brackets:[x]. Spaces and capitalization matter when checking off items this way.Checklist
Show available Jenkins commands
jenkins test classic perfJenkins Job | Jenkins Job Definitionjenkins test crimson perfJenkins Job | Jenkins Job Definitionjenkins test signedJenkins Job | Jenkins Job Definitionjenkins test make checkJenkins Job | Jenkins Job Definitionjenkins test make check arm64Jenkins Job | Jenkins Job Definitionjenkins test submodulesJenkins Job | Jenkins Job Definitionjenkins test dashboardJenkins Job | Jenkins Job Definitionjenkins test dashboard cephadmJenkins Job | Jenkins Job Definitionjenkins test apiJenkins Job | Jenkins Job Definitionjenkins test docsReadTheDocs | Github Workflow Definitionjenkins test ceph-volume allJenkins Jobs | Jenkins Jobs Definitionjenkins test windowsJenkins Job | Jenkins Job Definitionjenkins test rook e2eJenkins Job | Jenkins Job Definition