From 09bed9d91e4df4786d9bd61dc63fc13ab95aae22 Mon Sep 17 00:00:00 2001 From: Ayush Sinha Date: Sat, 25 Jul 2026 17:24:51 +0530 Subject: [PATCH] engine: key removeNic work job lookup on nic uuid to fix concurrent removal race 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 --- .../cloud/vm/VirtualMachineManagerImpl.java | 17 +++- .../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 b69491b64668..be7087c92b47 100755 --- a/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java +++ b/engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java @@ -5938,14 +5938,25 @@ public Outcome removeNicFromVmThroughJobQueue( final VirtualMachine vm, final Nic nic) { Long vmId = vm.getId(); String commandName = VmWorkRemoveNicFromVm.class.getName(); - Pair 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 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 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 a07870d09af2..fd9a92fc6e50 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.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; @@ -1865,6 +1866,97 @@ public void testUnmanagePendingWorkJobs() { 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(newJob, mock(VmWork.class))) + .when(virtualMachineManagerImpl).createWorkJobAndWorkInfo(commandName, vmInstanceVoMockId); + doNothing().when(virtualMachineManagerImpl).setCmdInfoAndSubmitAsyncJob(any(), any(), anyLong()); + + AsyncJobExecutionContext execContext = mock(AsyncJobExecutionContext.class); + try (MockedStatic 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 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);