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);