Andr0human commented on code in PR #13700:
URL: https://github.com/apache/cloudstack/pull/13700#discussion_r3782720239
##########
engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java:
##########
@@ -5938,14 +5938,25 @@ public Outcome<VirtualMachine>
removeNicFromVmThroughJobQueue(
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 {
Review Comment:
Thanks @DaanHoogland, the nitpicks were fair and they made the PR better.
Agreed on the extraction too, both paths have pretty much the same shape
now. Since this is going into 4.22 I'd rather keep the diff small and not
invalidate the green test run, so I'll pick it up as a follow-up on main if
that works for you.
--
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]
