[Damans227]

Fixes #14214

A detach that waits in the queue can run after the volume was already detached and attached to another vm. When it finally runs it still clears the volume's attachment in the database, so CloudStack thinks the volume is free while it is still plugged in. Every later attach to that vm then gets the same device slot and fails with target 'vdX' duplicated. This makes the detach check that the volume is still attached to the vm it was asked to leave. If it isnt, it logs a warning and leaves the volume alone. The attach side case in the issue (occurrence 4) has no known cause yet and is not covered here.

Test:

Unit tests for a detach that runs after the volume moved to another vm, and after it was already detached.

On a KVM lab with 2 hosts, deploy vm A and vm B and attach data volume V to A. While V is on A, send back to back: detach V (job 1), stop A, detach V again (job 2). Job 2 waits behind the stop. When job 1 is done, attach V to B. Job 2 runs after the stop finishes.

Before: job 2 clears V's attachment while V is still plugged into B, and the next attach to B fails with target 'vdX' duplicated.

After: job 2 succeeds and leaves V on B. A later attach to B gets the next slot.

job-82 detach V        15:52:23 -> 15:52:26 SUCCEEDED
job-84 stop A          15:52:23 -> 15:53:01 SUCCEEDED
job-88 attach V to B   15:52:28 -> 15:52:31 SUCCEEDED
job-86 detach V        15:52:23 -> 15:53:04 SUCCEEDED

cmk list volumes id=<V>
  "name": "volV", "vmname": "vmB", "deviceid": 1

virsh domblklist i-2-8-VM   (vm B)
 vda  .../e5cf4917-49af-4adc-9b89-a3d77d7a562c
 vdb  .../e8a48991-edef-4020-9a44-38ba9ebe3283   (V)
 vdc  .../c77d109e-7e27-4e91-87a7-766d3edeb1e5   (next attach)

[Damans227]

@blueorangutan package

[blueorangutan]

@Damans227 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[codecov]

Codecov Report

❌ Patch coverage is 66.66667% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 20.21%. Comparing base (a945a7c) to head (9e4cee9).
⚠️ Report is 43 commits behind head on main.

Files with missing lines	Patch %	Lines
...n/java/com/cloud/storage/VolumeApiServiceImpl.java	66.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               main   #14401   +/-   ##
=========================================
  Coverage     20.21%   20.21%           
- Complexity    20749    20753    +4     
=========================================
  Files          6426     6426           
  Lines        580181   580184    +3     
  Branches      71033    71034    +1     
=========================================
+ Hits         117292   117301    +9     
+ Misses       450174   450170    -4     
+ Partials      12715    12713    -2     

Flag	Coverage Δ
uitests	3.69% <ø> (ø)
unittests	21.51% <66.66%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19553

[DaanHoogland]

does what it should, just one cleanup suggestion; add a shouldDetachBeSkipped(Volume voume, VMInstanceVO vm) and make the comment a javadoc. Then return the volume on true.

[DaanHoogland]

clgtm

[dloszewski-cni]

Thanks @Damans227 for picking this up, and @DaanHoogland for the ping — sorry for the slow reply.

On the automation question (Oct 7): no hand-written harness. The race is driven by the CloudStack CSI driver in a Kubernetes cluster — when a detach exceeds the driver’s ~300s timeout it re-submits, so several DetachVolume jobs for the same volume stack up on that VM’s work queue. Pods rescheduling across nodes then supply the attach-elsewhere step. We hit it four times in 20 days on 4.22.1.0 with KVM + Ceph RBD. The prerequisite is a slow per-VM work queue; in our case agent-side libvirt stats calls were pushing individual detaches to 3–6 minutes.

On #14401: the execution-time re-check is right, and it matches what we wrote. I think it needs a second piece to close the race rather than narrow it.

The check reads volume.getInstanceId() at the top of detachVolumeFromVm, but the row is cleared after the agent round-trip returns. In our environment that round-trip ran 2–3 minutes. A newer attach landing inside that window still gets its attachment cleared — which is the exact sequence behind the occurrences we reported.

We made the final clear conditional as well: a VolumeDao.detachVolume(volumeId, vmId) that puts both id and instance_id in the search criteria and returns whether a row actually changed, with the caller logging and re-reading when it didn’t. That’s a compare-and-swap, and it mirrors what the attach path already does with checkAndSetAttaching.

I have that as a branch against 4.22 with unit tests for both halves — the DAO-level conditional update, and the service-level skip asserting no agent round-trip or stats collection happens. Happy to open it as a PR against this one, or hand the DAO change to @Damans227 to fold in, whichever is less friction.

Still open: the attach-side case (occurrence 4 in the issue) isn’t addressed by either patch, and we never root-caused it.