Andr0human opened a new pull request, #13700:
URL: https://github.com/apache/cloudstack/pull/13700
### Description
This PR makes the NIC uuid part of the pending work-job lookup key in
`VirtualMachineManagerImpl.removeNicFromVmThroughJobQueue`, so that two
concurrent `removeNicFromVirtualMachine` requests for **different NICs on the
same VM** no longer collapse into a single work job.
Before this change the lookup was NIC-agnostic —
`retrievePendingWorkJob(vmId, commandName)`, i.e. the 3-arg
`VmWorkJobDao.listPendingWorkJobs(type, vmId, cmd)`. If a second remove-NIC
request arrived while the first `VmWorkRemoveNicFromVm` job was still pending,
it matched that job and **joined** it instead of submitting its own. The job
removes only the NIC it was created for, but both callers wait on the same job
id and both receive its success — so the second NIC stays attached while its
API call reports success.
**Functional change in behaviour:**
* **Before:** concurrent removal of NIC-A and NIC-B produces **one** work
job. NIC-A is removed; NIC-B is silently left attached; **both** callers are
told SUCCEEDED.
* **After:** each request gets its **own** work job, keyed on its own NIC
uuid. Both NICs are removed and both callers are told the truth. Genuine
duplicates — two requests for the *same* NIC — still dedup exactly as before.
This is the mirror of an already-merged fix on the add path.
`addVmToNetworkThroughJobQueue` was fixed in #5658 (issue #5541, 4.16.1.0) by
keying on the object uuid; the symmetric remove path never got the same
treatment. The change here follows that established pattern line for line:
* look up pending jobs with the 4-arg `listPendingWorkJobs(Instance, vmId,
VmWorkRemoveNicFromVm.class.getName(), nic.getUuid())`;
* if more than one pending job matches, fail fast with a
`CloudRuntimeException` (same guard as the add path);
* stamp newly created jobs with
`workJob.setSecondaryObjectIdentifier(nic.getUuid())` before submitting.
`Nic extends Identity`, so `getUuid()` is available — no interface or schema
change is required.
Scope is deliberately tight: only `removeNicFromVmThroughJobQueue` is
touched. The sibling `removeVmFromNetworkThroughJobQueue` still uses the
network-agnostic `retrievePendingWorkJob(vmId, commandName)` lookup and appears
to have the same class of race; that is left out of this PR and noted in the
issue as a follow-up.
Fixes: #13699
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing
functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [x] 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
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [x] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
N/A — management-server logic change, no UI surface.
### How Has This Been Tested?
**1. Unit tests — three regression tests added to
`VirtualMachineManagerImplTest`**
| test | asserts |
| --- | --- |
| `removeNicFromVmThroughJobQueueDoesNotJoinAnotherNicsPendingJob` | with
another NIC's job already pending, a **new** job is created, stamped with this
NIC's uuid, submitted and joined |
| `removeNicFromVmThroughJobQueueJoinsExistingJobForSameNic` | per-NIC dedup
still works — an existing same-NIC job is joined, nothing new is submitted |
| `removeNicFromVmThroughJobQueueThrowsWhenMultiplePendingJobsForSameNic` |
the `size() > 1` guard throws |
```
mvn -pl engine/orchestration test -Dtest=VirtualMachineManagerImplTest
Tests run: 96, Failures: 0, Errors: 0, Skipped: 0
```
Red→green was confirmed by reverting the production change and re-running
against this branch: `Tests run: 96, Failures: 1, Errors: 2` — all three new
tests and only those, with the regression test failing at the
`setCmdInfoAndSubmitAsyncJob` verification, i.e. precisely because the second
request joined the first NIC's job instead of submitting its own.
`checkstyle:check` on the module reports 0 violations.
**2. End-to-end on the CloudStack simulator**
Advanced-zone simulator (`mvn -Pdeveloper -Dsimulator`, `tools/docker`), VM
with three NICs, two of them removed concurrently. `UnPlugNicCommand` was
stalled with `configureSimulator name=UnPlugNicCommand value=wait:20000` so the
first work job is guaranteed to still be pending when the second request
arrives — that is the window the bug lives in. Both halves were built through
the same two-module pipeline (`-pl engine/orchestration` then `-pl client`), so
the only difference between the runs is the fix itself. (This end-to-end run
was done on a simulator build of `main`; the diff proposed here is identical on
both branches, and the unit tests above were run on this 4.22 branch.)
| | before (unfixed) | after (this PR) |
| --- | --- | --- |
| `vm_work_job` rows for `VmWorkRemoveNicFromVm` | **1** | **2** |
| `secondary_object` (the per-NIC key) | `NULL` | two distinct NIC uuids |
| call A | SUCCEEDED — NIC removed | SUCCEEDED — NIC removed |
| call B | **SUCCEEDED — NIC still attached** | SUCCEEDED — NIC removed |
| NICs remaining on the VM | **2** (default + orphan) | 1 (default) |
The unfixed run reports the contradiction directly — the caller is told the
removal succeeded while `listVirtualMachines` still shows the NIC attached:
```
call B: asked to remove nic 6a005f6b-f852-4231-8dc7-938f9b7c1800
jobstatus = 1 (SUCCEEDED)
jobresultcode = 0
nics on the returned VM = ['98179dff-...-d08ea0dc6014',
'6a005f6b-...-938f9b7c1800']
*** caller reported SUCCEEDED but the nic is STILL ATTACHED ***
```
and the same query after the fix shows the two per-NIC jobs the dedup key
now produces:
```
+----+----------------+--------------------------------------+-----------------------+------------+
| id | vm_instance_id | secondary_object | cmd
| job_status |
+----+----------------+--------------------------------------+-----------------------+------------+
| 70 | 19 | fa062278-26e3-4286-a534-cdb4ebd994c7 |
VmWorkRemoveNicFromVm | 1 |
| 72 | 19 | a49f67b8-a9b9-4e4b-bb10-dc7d048a9b11 |
VmWorkRemoveNicFromVm | 1 |
+----+----------------+--------------------------------------+-----------------------+------------+
```
#### How did you try to break this feature and the system with this change?
* **Genuine duplicate requests.** Two removals of the *same* NIC still
collapse into one job — the narrower key must not disable dedup, only scope it.
Covered by `removeNicFromVmThroughJobQueueJoinsExistingJobForSameNic`.
* **More than one pending job for the same NIC.** Should never happen, but
the add path treats it as a hard error rather than silently picking one, so
this does too. Covered by
`removeNicFromVmThroughJobQueueThrowsWhenMultiplePendingJobsForSameNic`.
* **Sequential (non-concurrent) removals.** Unaffected: with no pending job
the lookup returns empty and a new job is created exactly as before.
* **Persistence of the new key.** `secondary_object` is an existing
`vm_work_job` column already written by the add path since #5658 — no schema
change, and it is confirmed populated in the simulator run above.
* **Blast radius.** `setSecondaryObjectIdentifier` is only ever set here for
jobs whose `cmd` is `VmWorkRemoveNicFromVm`, and the 4-arg
`listPendingWorkJobs` overload already exists and is already used by
`addVmToNetworkThroughJobQueue`. No other work-job path changes behaviour; the
full `VirtualMachineManagerImplTest` suite (96 tests) passes.
--
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]
