[preview] kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph - #5
Closed
calvix wants to merge 7 commits into
Closed
[preview] kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph#5calvix wants to merge 7 commits into
calvix wants to merge 7 commits into
Conversation
…pache#13579) Co-authored-by: mprokopchuk <mprokopchuk@apple.com>
takeRbdVolumeSnapshotOfStoppedVm() called image.snapCreate(snapshotName)
twice. The first call creates the RBD snapshot, the second one always
throws RbdException ("Failed to create snapshot <uuid>") because the
snapshot already exists.
The duplicate is a merge artifact: 30d3066 ("Merge branch '4.20' into
4.22") resolved a conflict by keeping the call from both sides - each
parent had exactly one.
Because there was no finally block, that exception skipped rbd.close(image)
and r.ioCtxDestroy(io), so the agent kept the image open and held its RBD
exclusive-lock indefinitely. The exception is only logged, so the snapshot
job still reported success and the fault stayed invisible.
Consequences observed on a KVM + Ceph/RBD cluster:
- revertSnapshot fails with "com.ceph.rbd.RbdException: Failed to rollback
snapshot <uuid>". librbd returns EROFS because a live peer holds the
exclusive-lock; 'rbd snap rollback' only succeeds once that client dies
and librbd can break the lock, which makes the failure look intermittent.
- getRbdSnapshotSize() is never reached, so every snapshot is reported with
physical size 0 when snapshot.backup.to.secondary is false.
- The leaked watchers keep the image busy, so 'rbd rm' fails and the volume
cannot be expunged - it stays stuck in state Destroy.
Note the method also runs for RUNNING VMs: createSnapshot() branches on
"RUNNING && !primaryPool.isExternalSnapshot()", and RBD is an
external-snapshot pool, so every RBD volume snapshot took this path.
Remove the duplicated call and move the image/IO-context cleanup into a
finally block so the lock is released even if the snapshot itself fails.
…napshot createRBDvolumeFromRBDSnapshot() closed the source image, the cloned image and the RADOS IO context only on the success path, and called snapUnprotect() only there too. Two paths escaped that cleanup: - the early "Could not find snapshot ... on RBD" return, and - any RadosException/RbdException from clone(), resize() or flatten(), which is caught and turned into a null disk. Both leave the images open, so this client keeps the RBD exclusive-lock. That later makes 'rbd snap rollback' (revertSnapshot) fail with EROFS from another host, and keeps the image busy so 'rbd rm' cannot remove it - the volume then stays stuck in state Destroy. The failure paths after snapProtect() are worse: the snapshot stays protected, and a protected snapshot can be deleted neither on its own nor together with its volume. Move the cleanup into a finally block, tracking whether the snapshot was actually protected so it is unprotected exactly when it needs to be. Failures during cleanup are logged and never mask the original outcome; a failed snapUnprotect is logged at ERROR since it needs manual intervention. This is the same class of defect as the leak fixed in takeRbdVolumeSnapshotOfStoppedVm(); no behaviour changes on the success path.
Two tests around takeRbdVolumeSnapshotOfStoppedVm, using the MockedConstruction pattern already used in this test class (the Rbd instance is created inside the method under test, so it cannot be injected): - createsSnapshotExactlyOnce guards the duplicated snapCreate call from coming back, and checks the image and IO context are released. - releasesHandlesWhenSnapshotFails makes snapCreate throw and asserts the image is still closed and the IO context destroyed, so a future failure cannot leak the RBD exclusive-lock again. takeRbdVolumeSnapshotOfStoppedVm, radosConnect and getRbdSnapshotSize widened from private to protected so the test can stub the Ceph interactions.
Address review feedback: the nested try-catch teardown blocks in the finally clauses of takeRbdVolumeSnapshotOfStoppedVm and createRBDvolumeFromRBDSnapshot are extracted into two reusable, null-safe helpers that log but never throw: - closeRbdImage (3 call sites) - destroyRadosIoCtx (2 call sites) No behavior change.
…k-refactor kvm: extract RBD handle teardown into null-safe helper methods
Follow-up to the teardown helpers: the snapUnprotect block in the finally clause of createRBDvolumeFromRBDSnapshot moves into a never-throwing unprotectRbdSnapshot helper, so the finally clause is now free of inline try-catch constructions entirely. No behavior change.
|
This pull request has merge conflicts. Dear author, please fix the conflicts and sync your branch with the base branch. |
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
2 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.
Fork-internal preview PR: full diff of
fix/rbd-snapshot-exclusive-lock-leakagainstmain, i.e. what upstream apache#13835 shows after the teardown-refactor merge (forkmainis even with upstreammain).Contains:
Do not merge — preview only.