This is an automated email from the ASF dual-hosted git repository.
harikrishna-patnala pushed a commit to branch 4.22
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.22 by this push:
new cdbda0ae158 engine: key removeNic work job lookup on nic uuid to fix
concurrent removal race (#13700)
cdbda0ae158 is described below
commit cdbda0ae158fd9cdee6850f61f5b943bccdb1f26
Author: Ayush Sinha <[email protected]>
AuthorDate: Tue Sep 8 16:32:05 2026 +0530
engine: key removeNic work job lookup on nic uuid to fix concurrent removal
race (#13700)
removeNicFromVmThroughJobQueue looked up pending work jobs by
(vmType, vmId, commandName) only, so the nic was not part of the dedup
key. A second removeNicFromVirtualMachine request for a different nic on
the same vm matched the first still-pending VmWorkRemoveNicFromVm job and
joined it instead of submitting its own. That job removes only the nic it
was created for, yet both callers wait on the same job id and both receive
its success, leaving the second nic silently attached while its API call
reports success.
Make the nic uuid part of the lookup key, mirroring
addVmToNetworkThroughJobQueue which was fixed the same way in #5658:
- look up pending jobs with the 4-arg
listPendingWorkJobs(Instance, vmId, cmd, nic.getUuid())
- fail fast with CloudRuntimeException if more than one job matches
- stamp new jobs with setSecondaryObjectIdentifier(nic.getUuid())
before submitting
Genuine duplicates, two requests for the same nic, still dedup as before.
Adds three regression tests to VirtualMachineManagerImplTest covering the
cross-nic race, same-nic dedup, and the multiple-pending-jobs guard.
Fixes: #13699
Generated-by: Claude Code (Anthropic)
Signed-off-by: Ayush Sinha <[email protected]>
---
.../com/cloud/vm/VirtualMachineManagerImpl.java | 17 +++-
.../cloud/vm/VirtualMachineManagerImplTest.java | 92 ++++++++++++++++++++++
2 files changed, 106 insertions(+), 3 deletions(-)
diff --git
a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
index d322507fb4c..c98391a654d 100755
---
a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
+++
b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
@@ -5957,14 +5957,25 @@ public class VirtualMachineManagerImpl extends
ManagerBase implements VirtualMac
final VirtualMachine vm, final Nic nic) {
Long vmId = vm.getId();
String commandName = VmWorkRemoveNicFromVm.class.getName();
- Pair<VmWorkJobVO, Long> pendingWorkJob = retrievePendingWorkJob(vmId,
commandName);
- VmWorkJobVO workJob = pendingWorkJob.first();
+ // The nic uuid must be part of the pending-job lookup key. Without
it, a concurrent request
+ // to remove a different nic from the same vm matches this
still-pending job and joins it
+ // instead of submitting its own, so only one nic is removed while
both callers wait on the
+ // single job and both receive its success. Mirrors the symmetric
addVmToNetworkThroughJobQueue.
+ final List<VmWorkJobVO> pendingWorkJobs =
_workJobDao.listPendingWorkJobs(
+ VirtualMachine.Type.Instance, vmId, commandName,
nic.getUuid());
- if (workJob == null) {
+ VmWorkJobVO workJob;
+ if (pendingWorkJobs != null && pendingWorkJobs.size() > 0) {
+ if (pendingWorkJobs.size() > 1) {
+ throw new CloudRuntimeException(String.format("The number of
jobs to remove nic %s from vm %s are %d", nic.getUuid(), vm.getInstanceName(),
pendingWorkJobs.size()));
+ }
+ workJob = pendingWorkJobs.get(0);
+ } else {
Pair<VmWorkJobVO, VmWork> newVmWorkJobAndInfo =
createWorkJobAndWorkInfo(commandName, vmId);
workJob = newVmWorkJobAndInfo.first();
+ workJob.setSecondaryObjectIdentifier(nic.getUuid());
VmWorkRemoveNicFromVm workInfo = new
VmWorkRemoveNicFromVm(newVmWorkJobAndInfo.second(), nic.getId());
setCmdInfoAndSubmitAsyncJob(workJob, workInfo, vmId);
diff --git
a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
index 7329b67b4be..c9a404f9c89 100644
---
a/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
+++
b/engine/orchestration/src/test/java/com/cloud/vm/VirtualMachineManagerImplTest.java
@@ -70,6 +70,7 @@ import
org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
import org.apache.cloudstack.framework.extensions.dao.ExtensionDetailsDao;
import org.apache.cloudstack.framework.extensions.manager.ExtensionsManager;
import org.apache.cloudstack.framework.extensions.vo.ExtensionDetailsVO;
+import org.apache.cloudstack.framework.jobs.AsyncJobExecutionContext;
import org.apache.cloudstack.framework.jobs.dao.VmWorkJobDao;
import org.apache.cloudstack.framework.jobs.impl.VmWorkJobVO;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
@@ -1868,6 +1869,97 @@ public class VirtualMachineManagerImplTest {
virtualMachineManagerImpl.unmanage(vmMockUuid, null);
}
+ /**
+ * A pending remove-nic job for a different nic on the same vm must not
swallow the removal of
+ * this nic: the pending-job lookup has to be keyed on the nic uuid so
that a new job is
+ * submitted for this nic instead of joining the other nic's job.
Regression test for the
+ * concurrent-removeNic collapse.
+ */
+ @Test
+ public void
removeNicFromVmThroughJobQueueDoesNotJoinAnotherNicsPendingJob() {
+ String commandName = VmWorkRemoveNicFromVm.class.getName();
+ String nicUuid = UUID.randomUUID().toString();
+
+ Nic nic = mock(Nic.class);
+ when(nic.getId()).thenReturn(42L);
+ when(nic.getUuid()).thenReturn(nicUuid);
+
+ // A pending remove-nic job exists for the vm (e.g. for another nic);
the nic-agnostic
+ // lookup would return it, but the nic-keyed lookup finds nothing for
this nic. This stub is
+ // lenient because the fixed code never consults the nic-agnostic
3-arg lookup - only the
+ // buggy code does, where this job is what it wrongly joins.
+
Mockito.lenient().when(_workJobDao.listPendingWorkJobs(VirtualMachine.Type.Instance,
vmInstanceVoMockId, commandName))
+
.thenReturn(Collections.singletonList(mock(VmWorkJobVO.class)));
+ when(_workJobDao.listPendingWorkJobs(VirtualMachine.Type.Instance,
vmInstanceVoMockId, commandName, nicUuid))
+ .thenReturn(Collections.emptyList());
+
+ VmWorkJobVO newJob = mock(VmWorkJobVO.class);
+ when(newJob.getId()).thenReturn(100L);
+ doReturn(new Pair<VmWorkJobVO, VmWork>(newJob, mock(VmWork.class)))
+
.when(virtualMachineManagerImpl).createWorkJobAndWorkInfo(commandName,
vmInstanceVoMockId);
+
doNothing().when(virtualMachineManagerImpl).setCmdInfoAndSubmitAsyncJob(any(),
any(), anyLong());
+
+ AsyncJobExecutionContext execContext =
mock(AsyncJobExecutionContext.class);
+ try (MockedStatic<AsyncJobExecutionContext> ignored =
Mockito.mockStatic(AsyncJobExecutionContext.class)) {
+
when(AsyncJobExecutionContext.getCurrentExecutionContext()).thenReturn(execContext);
+
+
virtualMachineManagerImpl.removeNicFromVmThroughJobQueue(vmInstanceMock, nic);
+
+ // A new job must be created for this nic, stamped with the nic
uuid, submitted and joined.
+ verify(virtualMachineManagerImpl,
times(1)).createWorkJobAndWorkInfo(commandName, vmInstanceVoMockId);
+ verify(newJob, times(1)).setSecondaryObjectIdentifier(nicUuid);
+ verify(virtualMachineManagerImpl,
times(1)).setCmdInfoAndSubmitAsyncJob(eq(newJob), any(),
eq(vmInstanceVoMockId));
+ verify(execContext, times(1)).joinJob(100L);
+ }
+ }
+
+ /**
+ * When a pending remove-nic job already exists for this same nic, the
request must join it
+ * rather than submit a duplicate: per-nic deduplication still holds.
+ */
+ @Test
+ public void removeNicFromVmThroughJobQueueJoinsExistingJobForSameNic() {
+ String commandName = VmWorkRemoveNicFromVm.class.getName();
+ String nicUuid = UUID.randomUUID().toString();
+
+ Nic nic = mock(Nic.class);
+ when(nic.getUuid()).thenReturn(nicUuid);
+
+ VmWorkJobVO existingJob = mock(VmWorkJobVO.class);
+ when(existingJob.getId()).thenReturn(77L);
+ when(_workJobDao.listPendingWorkJobs(VirtualMachine.Type.Instance,
vmInstanceVoMockId, commandName, nicUuid))
+ .thenReturn(Collections.singletonList(existingJob));
+
+ AsyncJobExecutionContext execContext =
mock(AsyncJobExecutionContext.class);
+ try (MockedStatic<AsyncJobExecutionContext> ignored =
Mockito.mockStatic(AsyncJobExecutionContext.class)) {
+
when(AsyncJobExecutionContext.getCurrentExecutionContext()).thenReturn(execContext);
+
+
virtualMachineManagerImpl.removeNicFromVmThroughJobQueue(vmInstanceMock, nic);
+
+ verify(virtualMachineManagerImpl,
never()).createWorkJobAndWorkInfo(anyString(), anyLong());
+ verify(virtualMachineManagerImpl,
never()).setCmdInfoAndSubmitAsyncJob(any(), any(), anyLong());
+ verify(execContext, times(1)).joinJob(77L);
+ }
+ }
+
+ /**
+ * More than one pending remove-nic job for the same nic is an
inconsistent state and must fail
+ * fast rather than pick one arbitrarily. Mirrors the guard in
addVmToNetworkThroughJobQueue.
+ */
+ @Test(expected = CloudRuntimeException.class)
+ public void
removeNicFromVmThroughJobQueueThrowsWhenMultiplePendingJobsForSameNic() {
+ String commandName = VmWorkRemoveNicFromVm.class.getName();
+ String nicUuid = UUID.randomUUID().toString();
+
+ Nic nic = mock(Nic.class);
+ when(nic.getUuid()).thenReturn(nicUuid);
+
+ when(_workJobDao.listPendingWorkJobs(VirtualMachine.Type.Instance,
vmInstanceVoMockId, commandName, nicUuid))
+ .thenReturn(Arrays.asList(mock(VmWorkJobVO.class),
mock(VmWorkJobVO.class)));
+
+
virtualMachineManagerImpl.removeNicFromVmThroughJobQueue(vmInstanceMock, nic);
+ }
+
@Test
public void testUnmanageHostNotFoundAfterTransaction() {
when(vmInstanceMock.getHostId()).thenReturn(hostMockId);
