Maor Lipchuk 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 isUnr done 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 done 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 :) will be addressed in later submit 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 this is an extracted code, it is already being used in other command. I can change this, but it actualy doesn't really have any difference 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 will be done in a later patch 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? parameters that will be sent from REST and will be wrong 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? it is an error format, it's better to print as much as we can. 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 done 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() ? done, added it. good catch 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? done 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 :) done. 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: Maor Lipchuk <[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
