Daniel Erez has posted comments on this change. Change subject: core: Use import command to register a VM ......................................................................
Patch Set 14: (11 comments) http://gerrit.ovirt.org/#/c/27247/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmCommand.java: Line 581: return false; Line 582: } Line 583: Line 584: protected boolean checkTemplateInStorageDomain() { Line 585: boolean retValue = getParameters().isImportAsNewEntity() || (!isUnregisteredVM() && checkIfDisksExist(imageList)) || isUnregisteredVM(); the expression should be simplified :) - extract to variables/reorder isUnregisteredVM condition/etc Line 586: if (retValue && !VmTemplateHandler.BLANK_VM_TEMPLATE_ID.equals(getVm().getVmtGuid()) Line 587: && !getParameters().getCopyCollapse()) { Line 588: List<StorageDomain> domains = Backend.getInstance() Line 589: .runInternalQuery(VdcQueryType.GetStorageDomainsByVmTemplateId, http://gerrit.ovirt.org/#/c/27247/14/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java: Line 27: public class ImportVmFromConfigurationCommand<T extends ImportVmParameters> extends ImportVmCommand<T> { Line 28: Line 29: private static final Log log = LogFactory.getLog(ImportVmFromConfigurationCommand.class); Line 30: private Collection<Disk> vmDisksToAttach; Line 31: OvfEntityData ovfEntityData; private Line 32: Line 33: protected ImportVmFromConfigurationCommand(Guid commandId) { Line 34: super(commandId); Line 35: } Line 42: @Override Line 43: protected boolean canDoAction() { Line 44: if (isUnregisteredVM()) { Line 45: if (ovfEntityData == null && !getParameters().isImportAsNewEntity()) { Line 46: // TODO: Add CDA message. indeed :) Line 47: return false; Line 48: } Line 49: Line 50: setStorageDomainId(ovfEntityData.getStorageDomainId()); Line 52: return false; Line 53: } Line 54: Line 55: if (!getStorageDomain().getStorageDomainType().isDataDomain()) { Line 56: addCanDoActionMessage("$domainId " + getParameters().getStorageDomainId()); nit - we usually use 'String.format' in such cases Line 57: addCanDoActionMessage("$domainType " + getStorageDomain().getStorageDomainType()); Line 58: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_UNSUPPORTED); Line 59: return false; Line 60: } Line 54: Line 55: if (!getStorageDomain().getStorageDomainType().isDataDomain()) { Line 56: addCanDoActionMessage("$domainId " + getParameters().getStorageDomainId()); Line 57: addCanDoActionMessage("$domainType " + getStorageDomain().getStorageDomainType()); Line 58: addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_TYPE_UNSUPPORTED); probably should be extracted to a validator - same check is already used in other command Line 59: return false; Line 60: } Line 61: } Line 62: return super.canDoAction(); Line 82: Line 83: private void initUnregisteredVM() { Line 84: VM vmFromConfiguration; Line 85: ovfEntityData = getUnregisteredOVFDataDao().getByVmId(getParameters().getContainerId()); Line 86: if (ovfEntityData != null) { in which scenario it could be null? Line 87: try { Line 88: OvfHelper ovfHelper = new OvfHelper(); Line 89: vmFromConfiguration = ovfHelper.readVmFromOvf(ovfEntityData.getOvfData()); Line 90: vmFromConfiguration.setVdsGroupId(getParameters().getVdsGroupId()); Line 91: getParameters().setVm(vmFromConfiguration); Line 92: getParameters().setDestDomainId(ovfEntityData.getStorageDomainId()); Line 93: getParameters().setSourceDomainId(ovfEntityData.getStorageDomainId()); Line 94: } catch (OvfReaderException e) { Line 95: log.errorFormat("failed to parse a given ovf configuration: \n" + ovfEntityData.getOvfData(), e); is it really useful to print the entire configuration? isn't id enough? Line 96: // addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_OVF); Line 97: } Line 98: } Line 99: } Line 92: getParameters().setDestDomainId(ovfEntityData.getStorageDomainId()); Line 93: getParameters().setSourceDomainId(ovfEntityData.getStorageDomainId()); Line 94: } catch (OvfReaderException e) { Line 95: log.errorFormat("failed to parse a given ovf configuration: \n" + ovfEntityData.getOvfData(), e); Line 96: // addCanDoActionMessage(VdcBllMessages.ACTION_TYPE_FAILED_UNSUPPORTED_OVF); remove Line 97: } Line 98: } Line 99: } Line 100: Line 106: Line 107: @Override Line 108: public void executeCommand() { Line 109: super.executeImportVm(!isUnregisteredVM()); Line 110: if (isUnregisteredVM()) { shouldn't check getSucceeded() ? Line 111: getUnregisteredOVFDataDao().removeEntity(ovfEntityData.getVmId(), ovfEntityData.getStorageDomainId()); Line 112: } else if (!vmDisksToAttach.isEmpty()) { Line 113: AuditLogDirector.log(this, attemptToAttachDisksToImportedVm(vmDisksToAttach)); Line 114: } http://gerrit.ovirt.org/#/c/27247/14/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ImportVmParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/ImportVmParameters.java: Line 16: private Guid destDomainId; Line 17: private Guid vdsGroupId; Line 18: Line 19: public ImportVmParameters() { Line 20: setSourceDomainId(Guid.Empty); why using setters? Line 21: setDestDomainId(Guid.Empty); Line 22: } Line 23: Line 24: public ImportVmParameters(VM vm, Guid sourceStorageDomainId, Guid destStorageDomainId, Guid storagePoolId, Line 23: Line 24: public ImportVmParameters(VM vm, Guid sourceStorageDomainId, Guid destStorageDomainId, Guid storagePoolId, Line 25: Guid vdsGroupId) { Line 26: super(vm.getId(), destStorageDomainId); Line 27: this.setSourceDomainId(sourceStorageDomainId); * no need for 'this' in this case :) * imo, don't use setters here Line 28: this.setDestDomainId(destStorageDomainId); Line 29: setVm(vm); Line 30: setStorageDomainId(destStorageDomainId); Line 31: setStoragePoolId(storagePoolId); -- To view, visit http://gerrit.ovirt.org/27247 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5ee64651eb614feb6ac9d7fde88a4ee6348aff06 Gerrit-PatchSet: 14 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
