Omer Frenkel has posted comments on this change. Change subject: core: introduce import from external provider ......................................................................
Patch Set 60: (9 comments) https://gerrit.ovirt.org/#/c/33712/60/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromExternalProviderCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromExternalProviderCommand.java: Line 38: import org.ovirt.engine.core.compat.Guid; Line 39: Line 40: @DisableInPrepareMode Line 41: @NonTransactiveCommandAttribute(forceCompensation = true) Line 42: public class ImportVmFromExternalProviderCommand<T extends ImportVmFromExternalProviderParameters> extends ImportVmCommandBase<T> maybe you wanted to extend ImportVmCommand? Line 43: implements QuotaStorageDependent { Line 44: Line 45: private static final Pattern VMWARE_DISK_NAME_PATTERN = Pattern.compile("\\[.*?\\] .*/(.*).vmdk"); Line 46: Line 54: Line 55: @Override Line 56: protected void init() { Line 57: super.init(); Line 58: setVm(getParameters().getVm()); probably not needed as happens already in super Line 59: setVdsId(getParameters().getProxyHostId()); Line 60: setVdsGroupId(getParameters().getVdsGroupId()); Line 61: setStorageDomainId(getParameters().getDestDomainId()); Line 62: setStoragePoolId(getVdsGroup() != null ? getVdsGroup().getStoragePoolId() : null); Line 56: protected void init() { Line 57: super.init(); Line 58: setVm(getParameters().getVm()); Line 59: setVdsId(getParameters().getProxyHostId()); Line 60: setVdsGroupId(getParameters().getVdsGroupId()); also Line 61: setStorageDomainId(getParameters().getDestDomainId()); Line 62: setStoragePoolId(getVdsGroup() != null ? getVdsGroup().getStoragePoolId() : null); Line 63: } Line 64: Line 75: if (!getStorageDomain().getStoragePoolId().equals(getStoragePoolId())) { Line 76: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_AND_CLUSTER_IN_DIFFERENT_POOL); Line 77: } Line 78: Line 79: if (getStoragePool().getStatus() != StoragePoolStatus.Up) { no need to verify storage domain is up as well? and has enough space? and more checks are missing (look at importVmCommand) Line 80: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_POOL_STATUS_ILLEGAL); Line 81: } Line 82: Line 83: if (getVdsId() != null && !validate(validateRequestedProxyHost())) { Line 107: return ValidationResult.VALID; Line 108: } Line 109: Line 110: @Override Line 111: protected List<DiskImage> createDiskDummiesForSpaceValidations(List<DiskImage> disksList) { i dont think this is called currently Line 112: List<DiskImage> dummies = new ArrayList<>(disksList.size()); Line 113: for (DiskImage image : disksList) { Line 114: dummies.add(ImagesHandler.createDiskImageWithExcessData(image, getStorageDomainId())); Line 115: } Line 158: Line 159: @Override Line 160: protected void addVmInterfaces() { Line 161: super.addVmInterfaces(); Line 162: for (VmNetworkInterface iface : getVm().getInterfaces()) { this means the nics are not in the vm.devices list? Line 163: VmDeviceUtils.addNetworkInterfaceDevice( Line 164: new VmDeviceId(iface.getId(), getVmId()), Line 165: iface.isPlugged(), Line 166: false); Line 237: setSucceeded(true); Line 238: } Line 239: Line 240: private void removeVm() { Line 241: runInternalActionWithTasksContext( no lock on the vm here? Line 242: VdcActionType.RemoveVm, Line 243: new RemoveVmParameters(getVmId(), true)); Line 244: } Line 245: Line 270: @Override Line 271: public AuditLogType getAuditLogTypeValue() { Line 272: switch (getActionState()) { Line 273: case EXECUTE: Line 274: return AuditLogType.IMPORTEXPORT_STARTING_IMPORT_VM; what about failure? Line 275: case END_FAILURE: Line 276: return AuditLogType.IMPORTEXPORT_IMPORT_VM_FAILED; Line 277: case END_SUCCESS: Line 278: default: https://gerrit.ovirt.org/#/c/33712/60/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 653: ACTION_TYPE_FAILED_LUNS_ALREADY_USED_BY_DISKS(ErrorType.CONFLICT), Line 654: ACTION_TYPE_FAILED_STORAGE_POOL_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 655: ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 656: ACTION_TYPE_FAILED_STORAGE_DOMAIN_AND_CLUSTER_IN_DIFFERENT_POOL(ErrorType.BAD_PARAMETERS), Line 657: ACTION_TYPE_FAILED_VDS_NOT_IN_DEST_STORAGE_POOL(ErrorType.BAD_PARAMETERS), what about messages? Line 658: ACTION_TYPE_FAILED_STORAGE_CONNECTION_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 659: ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_NOT_EXIST(ErrorType.BAD_PARAMETERS), Line 660: ACTION_TYPE_FAILED_STORAGE_CONNECTION_ALREADY_EXISTS(ErrorType.CONFLICT), Line 661: ACTION_TYPE_FAILED_STORAGE_CONNECTION_FOR_DOMAIN_ALREADY_EXISTS(ErrorType.CONFLICT), -- To view, visit https://gerrit.ovirt.org/33712 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9328edf7b8f49aa7975e28f651183b1449030dd6 Gerrit-PatchSet: 60 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Arik Hadas <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
