Sergey Gotliv has posted comments on this change.
Change subject: core, restapi: Introducing ImportVmFromConfigurationCommand
......................................................................
Patch Set 9: Looks good to me, but someone else must approve
(8 inline comments)
Ack for the Backend part.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetVmFromConfigurationQuery.java
Line 15: try {
Line 16:
getQueryReturnValue().setReturnValue(ovfHelper.readVmFromOvf(getParameters().getVmConfiguration()));
Line 17: getQueryReturnValue().setSucceeded(true);
Line 18: } catch (Exception e) {
Line 19: log.debug("failed to parse a given ovf configuration:
\n" + getParameters().getVmConfiguration(), e);
It looks like that only exception that can be thrown here is
OvfReaderException. Consider to catch it only
Line 20: getQueryReturnValue().setExceptionString("failed to
parse a given ovf configuration " + e.getMessage());
Line 21: }
Line 22: }
Line 23: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmFromConfigurationCommand.java
Line 42: vmDisksToAttach =
vmFromConfiguration.getDiskMap().values();
Line 43: clearVmDisks(vmFromConfiguration);
Line 44: getParameters().setCopyCollapse(true);
Line 45: getParameters().setVm(vmFromConfiguration);
Line 46:
getParameters().setContainerId(vmFromConfiguration.getId());
Why do you need to complete parameters here? Why the caller doesn't do that?
Line 47: }
Line 48:
Line 49: setVdsGroupId(getParameters().getVdsGroupId());
Line 50:
getParameters().setStoragePoolId(getVdsGroup().getStoragePoolId());
Line 83: return
AuditLogType.VM_IMPORT_FROM_CONFIGURATION_ATTACH_DISKS_FAILED;
Line 84: }
Line 85:
Line 86: return
AuditLogType.VM_IMPORT_FROM_CONFIGURATION_EXECUTED_SUCCESFULLY;
Line 87: }
SUCCESSFULLY
Line 88:
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java
Line 376: VM_IMPORT_INFO(144),
Line 377: VM_PAUSED_EIO(145),
Line 378: VM_PAUSED_EPERM(146),
Line 379: VM_POWER_DOWN_FAILED(147),
Line 380: VM_IMPORT_FROM_CONFIGURATION_EXECUTED_SUCCESFULLY(148),
SUCCESSFULLY
Line 381: VM_IMPORT_FROM_CONFIGURATION_ATTACH_DISKS_FAILED(149),
Line 382:
Line 383: VM_MEMORY_UNDER_GUARANTEED_VALUE(148,
AuditLogTimeInterval.MINUTE.getValue() * 15),
Line 384:
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties
Line 305: VM_PAUSED_EPERM=VM ${VmName} has paused due to storage permissions
problem.
Line 306: VM_POWER_DOWN_FAILED=Shutdown of VM ${VmName} failed.
Line 307: VM_MEMORY_UNDER_GUARANTEED_VALUE=VM ${VmName} on host ${VdsName} was
guaranteed ${MemGuaranteed} MB but currently has ${MemActual} MB
Line 308: VM_IMPORT_FROM_CONFIGURATION_EXECUTED_SUCCESFULLY=VM ${VmName} has
been successfully imported from the given configuration.
Line 309: VM_IMPORT_FROM_CONFIGURATION_ATTACH_DISKS_FAILED=VM ${VmName} has
been imported from the given configuration, failed to attach to it following
disk(s): ${DiskAliases}.
Personally, I like when all names placeholders surrounded by '', but I leave it
for A. Burden.
Line 310: VDS_INSTALL=Host ${VdsName} installed
Line 311: VDS_INSTALL_FAILED=Host ${VdsName} installation failed.
${FailedInstallMessage}.
Line 312: VDS_INITIALIZING=Host ${VdsName} is initializing. Message:
${ErrorMessage}
Line 313: VDS_INITIATED_RUN_VM=VM ${VmName} was restarted on Host ${VdsName}
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 2270: <xs:element name="custom_properties"
type="CustomProperties" minOccurs="0"/>
Line 2271: <xs:element name="payloads" type="Payloads" minOccurs="0"/>
Line 2272: <xs:element name="statistics" type="Statistics"
minOccurs="0" maxOccurs="1"/>
Line 2273: <xs:element name="disks" type="Disks" minOccurs="0"
maxOccurs="1"/>
Line 2274: <xs:element ref="initialization" minOccurs="0"/>
How many occurrences you allow?
Line 2275: <xs:element name="nics" type="Nics" minOccurs="0"
maxOccurs="1"/>
Line 2276: <xs:element name="tags" type="Tags" minOccurs="0"
maxOccurs="1"/>
Line 2277: <xs:element name="snapshots" type="Snapshots"
minOccurs="0" maxOccurs="1"/>
Line 2278: <xs:element name="placement_policy"
type="VmPlacementPolicy" minOccurs="0" maxOccurs="1"/>
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmsResource.java
Line 123: } else if (Guid.Empty.equals(templateId)) {
Line 124: response = addVmFromScratch(staticVm, vm,
storageDomainId);
Line 125: } else {
Line 126: response = addVm(staticVm, vm, storageDomainId,
templateId);
Line 127: }
I see 3 levels of if/else here. Consider to change it in the future.
Line 128: }
Line 129: }
Line 130: return removeRestrictedInfoFromResponse(response);
Line 131: }
Line 197: if (namedCluster(vm)) {
Line 198: clusterId = getClusterId(vm);
Line 199: } else {
Line 200: clusterId =
Guid.createGuidFromString(vm.getCluster().getId());
Line 201: }
5 rows can be changed by 1 with '?'
Line 202:
Line 203: ImportVmParameters parameters = new ImportVmParameters();
Line 204: parameters.setVm(vmConfiguration);
Line 205: parameters.setVdsGroupId(clusterId);
--
To view, visit http://gerrit.ovirt.org/15894
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I9390bcc11e5f3cb8c8c89ce791aabe63fe0ca341
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <[email protected]>
Gerrit-Reviewer: A Burden <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
