Repository navigation
Conversation
|
@blueorangutan package |
|
@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 Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #14401 +/- ##
============================================
+ Coverage 20.21% 20.67% +0.45%
- Complexity 20749 21727 +978
============================================
Files 6426 6492 +66
Lines 580181 590736 +10555
Branches 71033 72449 +1416
============================================
+ Hits 117292 122132 +4840
- Misses 450174 455329 +5155
- Partials 12715 13275 +560
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:
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19553 |
| // A queued detach can run after the volume was already detached and attached elsewhere; leave that attachment alone | ||
| if (volume.getInstanceId() == null || volume.getInstanceId() != vmId) { | ||
| logger.warn("Volume {} is no longer attached to VM {}, skipping detach", volume, vm); | ||
| return volume; | ||
| } | ||
|
|
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
done, moved it into shouldDetachBeSkipped with a javadoc
|
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. |
|
thanks @dloszewski-cni, the check at the start doesnt cover the window while the host replies. can you open your branch as a PR against mine so your tests come with it? |
…hing VM The execution-time skip added in this branch reads volume.getInstanceId() at the top of orchestrateDetachVolumeFromVM, but the attachment row is not cleared until after the agent round-trip returns. On a busy host that gap was 2-3 minutes in the environment reported in apache#14214, and an attach that lands inside it is still cleared by the straggler, so the race is narrowed rather than closed. Make the final clear conditional as well: - VolumeDao.detachVolume(volumeId, vmId) puts both id and instance_id in the search criteria and reports whether a row actually changed, so a volume that moved while the detach was in flight is left alone. This is the same shape as VolumeDaoImpl.updateState and VMInstanceDaoImpl.updateState, which already guard their updates on the state they expect to find. - The caller logs which VM the volume now belongs to, re-reads it, and skips the rest of the success block, since revoking access or publishing a detach event would assert something that is no longer true. This closes the database-write half of the race. It does not close hypervisor-side divergence: the DettachCommand has already succeeded by this point, so on a losing update the disk may be unplugged while the row says attached. Leaving the newer attachment intact is still strictly better than clearing it, and the warning names the volume and both VMs so the inconsistency can be reconciled. Tests cover both halves: the DAO update asserting that instance_id is in the where clause and that both ids are bound, and the service path on the branch that actually performs the agent round-trip, asserting that a skipped stale detach performs no disk statistics collection and publishes no usage event. Refs apache#14214, apache#14401 Signed-off-by: Dave Loszewski <dloszewski@centripetalnetworks.com>
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.