KVM: fix leaked RBD exclusive-lock breaking volume snapshot revert on Ceph [4.22]

[calvix]

Description

This PR fixes a leaked RBD exclusive-lock on KVM + Ceph/RBD that makes revertSnapshot fail, and adds regression tests.

Actual behaviour: on a KVM cluster with Ceph/RBD primary storage, reverting a volume snapshot fails, seemingly at random:

com.ceph.rbd.RbdException: Failed to rollback snapshot <snapshot-uuid>
  at com.ceph.rbd.RbdImage.snapRollBack(RbdImage.java:139)
  at LibvirtRevertSnapshotCommandWrapper.execute(LibvirtRevertSnapshotCommandWrapper.java:111)

Expected behaviour: the snapshot is rolled back.

Root cause. KVMStorageProcessor.takeRbdVolumeSnapshotOfStoppedVm() calls image.snapCreate(snapshotName) twice:

logger.debug("Attempting to create RBD snapshot {}@{}", disk.getName(), snapshotName);
image.snapCreate(snapshotName);      // creates the snapshot

image.snapCreate(snapshotName);      // always throws: the snapshot already exists
long rbdSnapshotSize = getRbdSnapshotSize(...);   // unreachable
...
rbd.close(image);                    // unreachable
r.ioCtxDestroy(io);                  // unreachable
} catch (final Exception e) {
    logger.error("A RBD snapshot operation on [{}] failed. ...");   // swallowed
}

The duplicate looks like a merge-conflict resolution that kept the call from both sides — 4.20 grew a snapCreate next to the new getRbdSnapshotSize() while 4.22 already had one:

commit 30d306622a90ac43f2a6c35ee999110ad1bc5194  "Merge branch '4.20' into 4.22"
  parent ef60aa5601  -> image.snapCreate(snapshotName) x1
  parent 6bed3d4e64  -> image.snapCreate(snapshotName) x1
  result             -> image.snapCreate(snapshotName) x2

4.19 and 4.20 have one call; 4.22 and main have two.

Because there is no finally, the exception from the second call skips rbd.close(image) and r.ioCtxDestroy(io), so the agent keeps the image open and holds its RBD exclusive-lock indefinitely. The exception is only logged, so the snapshot job still reports success and nothing looks wrong.

Note this method also runs for running VMs: createSnapshot() branches on RUNNING && !primaryPool.isExternalSnapshot(), and RBD is an external-snapshot pool, so every RBD volume snapshot takes this path.

Resulting symptoms:

1. revertSnapshot fails with EROFS. A live peer holds the exclusive-lock, so librbd refuses snap_rollback. It only succeeds once that client dies and librbd can break the lock, which is why it looks intermittent.
2. Snapshots report physicalsize: 0 when snapshot.backup.to.secondary=false, because getRbdSnapshotSize() is never reached.
3. Volumes get stuck in state Destroy. The leaked watchers keep the image busy, so rbd rm fails and the volume can never be expunged.

The agent log shows the swallowed exception on every snapshot:

ERROR [kvm.storage.KVMStorageProcessor] A RBD snapshot operation on [<volume-uuid>] failed.
The error was: Failed to create snapshot <snapshot-uuid>
  at com.ceph.rbd.RbdImage.snapCreate(RbdImage.java:111)
  at KVMStorageProcessor.takeRbdVolumeSnapshotOfStoppedVm(KVMStorageProcessor.java:2392)
  at KVMStorageProcessor.createSnapshot(KVMStorageProcessor.java:1907)

<snapshot-uuid> there is exactly the snapshot the next revertSnapshot then failed to roll back.

The fix

1. takeRbdVolumeSnapshotOfStoppedVm() — remove the duplicated snapCreate, and move rbd.close(image) / r.ioCtxDestroy(io) into a finally so the exclusive-lock is released even when the snapshot itself fails.

2. createRBDvolumeFromRBDSnapshot() — the same class of defect. It released its handles only on the success path, so the early Could not find snapshot ... on RBD return and any exception from clone() / resize() / flatten() leaked the same lock. Worse, the failure paths after snapProtect() left the snapshot protected, and a protected snapshot can be deleted neither on its own nor with its volume. Cleanup moved into a finally, tracking whether the snapshot was actually protected so snapUnprotect() runs exactly when it should. Cleanup failures are logged and never mask the original outcome; a failed snapUnprotect is logged at ERROR because it needs manual intervention.

3. Regression tests in KVMStorageProcessorTest (see below).

No behaviour change on the success path in either method.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

Screenshots (if appropriate):

N/A

How Has This Been Tested?

Unit tests — two new tests in KVMStorageProcessorTest, using the MockedConstruction pattern already used in that class (the Rbd instance is constructed inside the method under test):

• takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce — asserts snapCreate is invoked exactly once, and that the image and IO context are released. This is the direct guard against the duplicated call reappearing.
• takeRbdVolumeSnapshotOfStoppedVmTestReleasesHandlesWhenSnapshotFails — makes snapCreate throw RbdException and asserts rbd.close(image) and ioCtxDestroy(io) still run, so a future failure cannot leak the lock again.

takeRbdVolumeSnapshotOfStoppedVm, radosConnect and getRbdSnapshotSize were widened from private to protected to make this testable.

mvn -pl plugins/hypervisors/kvm -Dtest=KVMStorageProcessorTest test on this branch:

Tests run: 39, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

The first test was also verified to actually catch the defect: reintroducing the duplicated snapCreate on top of this branch makes it fail, and only it —

[ERROR] KVMStorageProcessorTest.takeRbdVolumeSnapshotOfStoppedVmTestCreatesSnapshotExactlyOnce
rbdImage.snapCreate(
Wanted 1 time:
[ERROR] Tests run: 39, Failures: 1, Errors: 0, Skipped: 0

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.secondary both true and false):

• Reproduced end to end before the fix: createSnapshot on a DATADISK, stop the VM, revertSnapshot → RbdException: Failed to rollback snapshot.

• Verified the RBD snapshot itself exists and is correctly named, so the rollback target was never the problem — rbd snap ls returned exactly the name passed to snapRollBack.

• Correlated success/failure with the lock owner: when the lock holder still appears in rbd status (live client) the revert fails; when it does not (dead client, librbd breaks the lock) it succeeds.

• Confirmed the mechanism by clearing the lock by hand:

  # rbd snap rollback <pool>/<vol>@<snap>
  Rolling back to snapshot: 0% complete...failed.
  rbd: rollback failed: (30) Read-only file system
  
  # rbd lock rm <pool>/<vol> "auto <id>" client.<id>
  # rbd snap rollback <pool>/<vol>@<snap>
  Rolling back to snapshot: 100% complete...done.
  

  After clearing the lock the CloudStack revertSnapshot API job also returns success.

• Confirmed the duplicated call is reached on every snapshot via the agent log stack trace quoted above.

How did you try to break this feature and the system with this change?

• Ran the snapshot/revert cycle repeatedly (snapshot on a running VM → stop VM → revert) and correlated every outcome with rbd lock ls / rbd status, including predicting failures in advance from the lock state.
• Exercised both snapshot.backup.to.secondary=true (snapshot backed up to secondary storage) and false (kept on primary), since they take different snapshot paths and only the latter reaches getRbdSnapshotSize().
• Exercised snapshots of both an unencrypted and an encrypted RBD volume, and of volumes attached to running and stopped VMs.
• Checked the alternative restore path (createVolume from a snapshot), which is what exercises createRBDvolumeFromRBDSnapshot() — including its "snapshot not found" path, which previously returned without releasing anything.
• Reviewed the remaining RBD paths in KVMStorageProcessor for the same pattern: deleteSnapshot() already releases its handles in a finally and is unchanged.