Shmuel Leib Melamud has posted comments on this change. Change subject: engine: Removal of VM pool with VMs ......................................................................
Patch Set 6: (11 comments) https://gerrit.ovirt.org/#/c/40557/6/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmPoolCommand.java: Line 35: import org.ovirt.engine.core.utils.transaction.TransactionSupport; Line 36: import org.slf4j.Logger; Line 37: import org.slf4j.LoggerFactory; Line 38: Line 39: public class RemoveVmPoolCommand<T extends VmPoolParametersBase> extends VmPoolCommandBase<T> { > is there a reason to keep it transactional? Done Line 40: Line 41: private static final Logger log = LoggerFactory.getLogger(RemoveVmPoolCommand.class); Line 42: Line 43: private List<VM> vmsInPool = null; Line 39: public class RemoveVmPoolCommand<T extends VmPoolParametersBase> extends VmPoolCommandBase<T> { Line 40: Line 41: private static final Logger log = LoggerFactory.getLogger(RemoveVmPoolCommand.class); Line 42: Line 43: private List<VM> vmsInPool = null; > 1. no need to explicitly initialize it with null Done Line 44: Line 45: public RemoveVmPoolCommand(T parameters) { Line 46: this(parameters, null); Line 47: } Line 90: LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM, getVmIsBeingRemovedMessage(vm))); Line 91: if (!StringUtils.isBlank(vm.getName())) { Line 92: locks.put(vm.getName(), Line 93: LockMessagesMatchUtil.makeLockingPair(LockingGroup.VM_NAME, getVmIsBeingRemovedMessage(vm))); Line 94: } > no need to lock VM_NAME. this lock is used to lock the name before the VM i This is intentional. In the pool there may be VMs that are in the process of creation (see bug https://bugzilla.redhat.com/show_bug.cgi?id=1168249). This makes their removal impossible. After the pool is removed, these VMs will still exist. Locking VM_NAME will block RemoveVmPool if AddVm for some of the VMs in the pool is still in progress. Line 95: } Line 96: Line 97: private String getVmPoolIsBeingRemovedMessage() { Line 98: StringBuilder builder = new StringBuilder( Line 136: if (vmPool.getPrestartedVms() > 0) { Line 137: vmPool.setPrestartedVms(0); Line 138: getVmPoolDAO().update(vmPool); Line 139: } Line 140: } > don't you need to do that in new transaction as well? Done Line 141: Line 142: private boolean stopVms() { Line 143: for (VM vm : getVmsInPool()) { Line 144: if (!vm.isDown()) { Line 154: return true; Line 155: } Line 156: Line 157: private boolean removeVmsInPool() { Line 158: VdcReturnValueBase result; > please declare it in #163 Done Line 159: Line 160: for (VM vm : getVmsInPool()) { Line 161: RemoveVmFromPoolParameters removeVmFromPoolParameters = new RemoveVmFromPoolParameters(vm.getId()); Line 162: removeVmFromPoolParameters.setTransactionScopeOption(TransactionScopeOption.Suppress); Line 187: if (getVmPoolId() != null && canRemoveVmPool(getVmPoolId())) { Line 188: getVmPoolDAO().remove(getVmPoolId()); Line 189: return true; Line 190: } else { Line 191: return false; > please add a log message Done Line 192: } Line 193: } Line 194: Line 195: private CommandContext createRemoveVmStepContext(VM vm) { https://gerrit.ovirt.org/#/c/40557/6/backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties: Line 758: ACTION_TYPE_FAILED_TEMPLATE_IS_BEING_REMOVED=Cannot ${action} ${type}. Template ${TemplateName} is being removed. Line 759: ACTION_TYPE_FAILED_VM_IS_BEING_REMOVED_FROM_EXPORT_DOMAIN=Cannot ${action} ${type}. VM ${VmName} is being removed from export domain. Line 760: ACTION_TYPE_FAILED_DISK_IS_USED_BY_GET_ALIGNMENT=Cannot ${action} ${type}. Disk ${DiskAlias} alignment is currently being determined. Line 761: ACTION_TYPE_FAILED_DISK_IS_BEING_EXPORTED=Cannot ${action} ${type}. Disk ${DiskAlias} is being exported. Line 762: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed. > s/VM pool/VM Pool Done Line 763: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_UPDATED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being updated. Line 764: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED_WITH_VM=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed with VM ${VmName}. Line 765: ACTION_TYPE_FAILED_VM_IS_BEING_CREATED_AND_ATTACHED_TO_POOL=Cannot ${action} ${type}. A VM is being created and attached to pool ${VmPoolName}. Line 766: NETWORK_BOND_HAVE_ATTACHED_VLANS=Bond attached to vlan, remove bonds vlan first Line 759: ACTION_TYPE_FAILED_VM_IS_BEING_REMOVED_FROM_EXPORT_DOMAIN=Cannot ${action} ${type}. VM ${VmName} is being removed from export domain. Line 760: ACTION_TYPE_FAILED_DISK_IS_USED_BY_GET_ALIGNMENT=Cannot ${action} ${type}. Disk ${DiskAlias} alignment is currently being determined. Line 761: ACTION_TYPE_FAILED_DISK_IS_BEING_EXPORTED=Cannot ${action} ${type}. Disk ${DiskAlias} is being exported. Line 762: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed. Line 763: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_UPDATED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being updated. > same Done Line 764: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED_WITH_VM=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed with VM ${VmName}. Line 765: ACTION_TYPE_FAILED_VM_IS_BEING_CREATED_AND_ATTACHED_TO_POOL=Cannot ${action} ${type}. A VM is being created and attached to pool ${VmPoolName}. Line 766: NETWORK_BOND_HAVE_ATTACHED_VLANS=Bond attached to vlan, remove bonds vlan first Line 767: NETWORK_INTERFACE_CONNECT_TO_VLAN=Cannot attach non vlan network to vlan interface Line 760: ACTION_TYPE_FAILED_DISK_IS_USED_BY_GET_ALIGNMENT=Cannot ${action} ${type}. Disk ${DiskAlias} alignment is currently being determined. Line 761: ACTION_TYPE_FAILED_DISK_IS_BEING_EXPORTED=Cannot ${action} ${type}. Disk ${DiskAlias} is being exported. Line 762: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed. Line 763: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_UPDATED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being updated. Line 764: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED_WITH_VM=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed with VM ${VmName}. > same Done Line 765: ACTION_TYPE_FAILED_VM_IS_BEING_CREATED_AND_ATTACHED_TO_POOL=Cannot ${action} ${type}. A VM is being created and attached to pool ${VmPoolName}. Line 766: NETWORK_BOND_HAVE_ATTACHED_VLANS=Bond attached to vlan, remove bonds vlan first Line 767: NETWORK_INTERFACE_CONNECT_TO_VLAN=Cannot attach non vlan network to vlan interface Line 768: NETWORK_CANNOT_REMOVE_NETWORK_IN_USE_BY_VM=Cannot remove network '${NetworkName}', it's in use by a VM Line 761: ACTION_TYPE_FAILED_DISK_IS_BEING_EXPORTED=Cannot ${action} ${type}. Disk ${DiskAlias} is being exported. Line 762: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed. Line 763: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_UPDATED=Cannot ${action} ${type}. VM pool ${VmPoolName} is being updated. Line 764: ACTION_TYPE_FAILED_VM_POOL_IS_BEING_REMOVED_WITH_VM=Cannot ${action} ${type}. VM pool ${VmPoolName} is being removed with VM ${VmName}. Line 765: ACTION_TYPE_FAILED_VM_IS_BEING_CREATED_AND_ATTACHED_TO_POOL=Cannot ${action} ${type}. A VM is being created and attached to pool ${VmPoolName}. > s/pool/VM Pool Done Line 766: NETWORK_BOND_HAVE_ATTACHED_VLANS=Bond attached to vlan, remove bonds vlan first Line 767: NETWORK_INTERFACE_CONNECT_TO_VLAN=Cannot attach non vlan network to vlan interface Line 768: NETWORK_CANNOT_REMOVE_NETWORK_IN_USE_BY_VM=Cannot remove network '${NetworkName}', it's in use by a VM Line 769: ACTION_TYPE_FAILED_DISK_MAX_SIZE_EXCEEDED=Cannot create disk more than ${max}_disk_size GB https://gerrit.ovirt.org/#/c/40557/6/backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/DestroyVmVDSCommand.java File backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/DestroyVmVDSCommand.java: Line 68: // if using stop then call to ProcessOnVmStop because Line 69: // will not be called from UpdateRunTimeInfo Line 70: if (!parameters.getGracefully()) { Line 71: ResourceManager.getInstance().getEventListener() Line 72: .processOnVmStop(Collections.singleton(curVm.getId()), getParameters().getVdsId(), false); > I'm not sure if we don't need to do it in the same thread only when it is c Done Line 73: } Line 74: Line 75: getVDSReturnValue().setReturnValue(curVm.getStatus()); Line 76: } else if (vdsBrokerCommand.getVDSReturnValue().getExceptionObject() != null) { -- To view, visit https://gerrit.ovirt.org/40557 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I58b0f37903447b49edf74bea5a51108818701698 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud <smela...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Shmuel Leib Melamud <smela...@redhat.com> Gerrit-Reviewer: Shmuel Melamud <smela...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
