kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph - #13835
Open
calvix wants to merge 3 commits into
Open
kvm: fix RBD exclusive-lock leak that breaks revertSnapshot on Ceph#13835calvix wants to merge 3 commits into
calvix wants to merge 3 commits into
Conversation
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.
Comment on lines
+2351
to
+2353
| io = r.ioCtxCreate(primaryPool.getSourceDir()); | ||
| rbd = new Rbd(io); | ||
| image = rbd.open(disk.getName()); |
Contributor
There was a problem hiding this comment.
io and image are torn down in nested try-catch constructions in a finally clause of the parent try-catch clause. I think this should be re-structured to be in called methods.
In addition the same pattern happens below. a lot of re-use can be gained,
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13835 +/- ##
============================================
- Coverage 17.69% 17.69% -0.01%
+ Complexity 15835 15833 -2
============================================
Files 5925 5925
Lines 533539 533578 +39
Branches 65274 65280 +6
============================================
- Hits 94427 94424 -3
- Misses 428435 428476 +41
- Partials 10677 10678 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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.
Description
This PR fixes snapshot operations for KVM + ceph/rbd
takeRbdVolumeSnapshotOfStoppedVm() calls image.snapCreate(snapshotName)
twice. The first call creates the RBD snapshot, the second one always
throws RbdException ("Failed to create snapshot ") because the
snapshot already exists.
The duplicate happened in a merge artifact: 30d3066 ("Merge branch '4.20' into
4.22")
30d3066#diff-8a24835eeac038da0df1229615ce6ce564f2e49273abb55f4c9a90dedd29c222L2318-R2333
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:
snapshot ". 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.
physical size 0 when snapshot.backup.to.secondary is false.
cannot be expunged - it stays stuck in state Destroy.
The fix removes the duplicated call and move the image cleanup into a
finally block so the lock is released even if the snapshot itself fails.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
How Has This Been Tested?
Manual testing on a KVM + Ceph/RBD cluster (CloudStack 4.22.1.0, Ceph 20.2.2, single RBD pool, tested with
snapshot.backup.to.secondarybothtrueandfalse):createSnapshoton a DATADISK, stop the VM,revertSnapshot→RbdException: Failed to rollback snapshot.rbd snap lsreturned exactly the name passed tosnapRollBack.rbd status(live client) the revert fails; when it does not (dead client, librbd breaks the lock), it succeeds.revertSnapshotAPI job also returns success.