Andr0human opened a new issue, #13699:
URL: https://github.com/apache/cloudstack/issues/13699

   ### problem
   
   When two `removeNicFromVirtualMachine` requests are issued for **different 
NICs on the same VM**, close enough in time that the first work job is still 
pending, the second request **joins the first job instead of creating its 
own**. That single job removes only the first NIC, then **both** API callers 
wait on it and **both receive its success**. The second NIC is silently left 
attached even though its removal call returned success — reconciling desired 
vs. actual state then requires manual intervention.
   
   **Root cause**
   
   `VirtualMachineManagerImpl.removeNicFromVmThroughJobQueue(...)` deduplicates 
pending work jobs by `(vmType, vmId, commandName)` only — via 
`retrievePendingWorkJob(vmId, commandName)` → 
`VmWorkJobDao.listPendingWorkJobs(type, vmId, cmd)` (the 3-arg overload). **The 
NIC is not part of the lookup key.** So a second concurrent remove-NIC request 
for a *different* NIC matches the first still-pending `VmWorkRemoveNicFromVm` 
job and joins it. The job body removes only the NIC it was created for; both 
callers join the same job id and both get its result.
   
   **This is the exact mirror of an already-fixed bug on the add path.** 
`addVmToNetworkThroughJobQueue` was fixed in PR #5658 (issue #5541, merged for 
4.16.1.0) by making the object UUID part of the key: it uses the 4-arg 
`listPendingWorkJobs(type, vmId, cmd, network.getUuid())` and stamps new jobs 
with `setSecondaryObjectIdentifier(network.getUuid())`. The symmetric remove 
path never got the same treatment — @DaanHoogland acknowledged at the time that 
the remove side was left unfixed.
   
   ### versions
   
   ```
   4.20, 4.22, main (4.23.0.0-SNAPSHOT)
   ```
   
   The logic is unchanged since 4.16.x, so this affects every release that 
carries the `addVmToNetwork` fix (PR #5658) but not its remove-side 
counterpart. Verified present on the current release branch (4.22) and on main.
   
   Environment-independent — this is a logic bug in the management server, not 
hypervisor-, storage- or network-specific. Reproducible on any VM with two or 
more NICs.
   
   ### The steps to reproduce the bug
   
   1. Create a VM attached to two network tiers, so it has two NICs — NIC-A and 
NIC-B.
   2. Issue `removeNicFromVirtualMachine` for NIC-A, then for NIC-B, close 
enough together that the second call arrives while the first work job is still 
pending.
   3. Observe both API calls return **success**.
   4. Query the VM's NICs: NIC-B is still attached. Only NIC-A was removed.
   5. Inspect `vm_work_job` for the VM: there is a **single** row for 
`VmWorkRemoveNicFromVm`, not two — the second request joined the first job 
rather than submitting its own.
   
   **Expected:** both NICs are removed, each request tracked by its own per-NIC 
work job; success is reported only for NICs that were actually removed.
   
   **Actual:** only the first NIC is removed, the second is silently left 
attached, and both callers receive success.
   
   ### What to do about it?
   
   Mirror the add path in `removeNicFromVmThroughJobQueue`:
   
   * look up pending jobs with the 4-arg `listPendingWorkJobs(Instance, vmId, 
VmWorkRemoveNicFromVm.class.getName(), nic.getUuid())` instead of the 
NIC-agnostic 3-arg `retrievePendingWorkJob(vmId, commandName)`;
   * guard against more than one match (`size() > 1` → fail fast, exactly as 
the add path does);
   * stamp new jobs with `workJob.setSecondaryObjectIdentifier(nic.getUuid())` 
before submitting.
   
   `Nic extends Identity`, so `getUuid()` is available.
   
   I have a patch ready for this and will open a PR — it keeps the scope tight 
to `removeNicFromVmThroughJobQueue` and adds three regression unit tests to 
`VirtualMachineManagerImplTest` (verified failing on current code, passing with 
the fix).
   
   **Related**
   
   * PR #5658 / issue #5541 — the symmetric fix on the **add** path 
(`addVmToNetworkThroughJobQueue`).
   * **Sibling issue, out of scope here and worth a follow-up:** 
`removeVmFromNetworkThroughJobQueue` still uses the same network-agnostic 
`retrievePendingWorkJob(vmId, commandName)` lookup and looks affected by the 
same class of race.


-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to