Shahar Havivi has posted comments on this change. Change subject: Engine: Vm Init - new Feature ......................................................................
Patch Set 16: (10 comments) http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndAttachToPoolCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmAndAttachToPoolCommand.java: Line 32: } else { Line 33: returnValueFromAddVm = addVm(vmStatic); Line 34: } Line 35: Line 36: VmHandler.addVmInitToDB(vmStatic); > not sure this is needed, since addVm() method calls addVmCommand which is d checked and you right. Line 37: Line 38: if (returnValueFromAddVm.getSucceeded()) { Line 39: getTaskIdList().addAll(returnValueFromAddVm.getInternalVdsmTaskIdList()); Line 40: addVmToPool(vmStatic); http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmCommand.java: Line 106: // update vm snapshots for storage free space check Line 107: ImagesHandler.fillImagesBySnapshots(getVm()); Line 108: Line 109: // update vm init Line 110: VmHandler.updateVmInitFromDB(getVm().getStaticData(), true); > is this used in canDoAction? if not its better to load this data in execute Done Line 111: Line 112: // check that the target and source domain are in the same storage_pool Line 113: if (getDbFacade().getStoragePoolIsoMapDao() Line 114: .get(new StoragePoolIsoMapId(getStorageDomain().getId(), http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ExportVmTemplateCommand.java: Line 147: } Line 148: Line 149: if (retVal) { Line 150: // update vm init Line 151: VmHandler.updateVmInitFromDB(getVmTemplate(), true); > also here, better in execute Done Line 152: } Line 153: return retVal; Line 154: } Line 155: http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveVmCommand.java: Line 74: // check that images are ok Line 75: // not checking storage domain, there is a check in Line 76: // checkTemplateInStorageDomain later Line 77: VmHandler.updateDisksFromDb(getVm()); Line 78: VmHandler.updateVmInitFromDB(getVm().getStaticData(), true); > why this is needed when moving disks? no need indeed... Line 79: List<DiskImage> diskImages = ImagesHandler.filterImageDisks(getVm().getDiskMap().values(), false, false, true); Line 80: List<DiskImage> diskImagesToValidate = ImagesHandler.filterImageDisks(diskImages, true, false, true); Line 81: DiskImagesValidator diskImagesValidator = new DiskImagesValidator(diskImagesToValidate); Line 82: retValue = retValue && http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RemoveVmCommand.java: Line 110: for (DiskImage image : diskImages) { Line 111: getImageDao().updateImageVmSnapshotId(image.getImageId(), null); Line 112: } Line 113: } Line 114: VmHandler.removeVmInitFromDB(getVm().getStaticData()); > this is probably redundant because there is delete on cascade, no? we can k yes its obsolete when adding on cascade... Line 115: return null; Line 116: } Line 117: }); Line 118: http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmOnceCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RunVmOnceCommand.java: Line 37: if (getParameters().getSysPrepUserName() == null ^ getParameters().getSysPrepPassword() == null) { Line 38: return failCanDoAction(VdcBllMessages.VM_CANNOT_RUN_ONCE_WITH_ILLEGAL_SYSPREP_PARAM); Line 39: } Line 40: Line 41: if (getParameters().getVmInit() != null > this wouldn't block sysprep on run once with older versions? is checking isWindows() to this condition is sufficient? Line 42: && !FeatureSupported.cloudInit(getVm().getVdsGroupCompatibilityVersion())) { Line 43: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CLOUD_INIT_IS_NOT_SUPPORTED); Line 44: } Line 45: http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmTemplateCommand.java: Line 73: } else { Line 74: if (getVdsGroup() == null) { Line 75: addCanDoActionMessage(VdcBllMessages.VMT_CLUSTER_IS_NOT_VALID); Line 76: } else if (isVmPriorityValueLegal(getParameters().getVmTemplateData().getPriority(), getReturnValue() Line 77: .getCanDoActionMessages())) { > we still need to check that the domain is legal.. Done Line 78: returnValue = VmTemplateHandler.isUpdateValid(mOldTemplate, getVmTemplate()); Line 79: if (!returnValue) { Line 80: addCanDoActionMessage(VdcBllMessages.VMT_CANNOT_UPDATE_ILLEGAL_FIELD); Line 81: } http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/VmHandler.java: Line 303: } Line 304: Line 305: public static void updateVmInitToDB(VmBase vm, VmBase oldVm) { Line 306: if (vm.getVmInit() != null) { Line 307: VmHandler.addVmInitToDB(vm); > i think something is wrong here, shouldnt we first remove the old one (if e Yes, no need for oldVm as parameter, just add/edit if there is vmInit or delete if not exist in the new VM Line 308: } else if (oldVm != null) { Line 309: // make sure we have the correct vm init Line 310: VmHandler.updateVmInitFromDB(oldVm, true); Line 311: http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/businessentities/VmBase.java: Line 253: Line 254: private Guid createdByUserId; Line 255: Line 256: @EditableField Line 257: @EditableOnTemplate > if you put editable, no need for editableOnTemplate, its good for both Done Line 258: private VmInit vmInit; Line 259: Line 260: public VmBase(VmBase vmBase) { Line 261: this(vmBase.getName(), http://gerrit.ovirt.org/#/c/23020/16/backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java File backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java: Line 331: if (node != null) { Line 332: vmBase.setDescription(node.innerText); Line 333: } Line 334: Line 335: if (vmInitEnabled()) { > shouldnt this be if not enabled? or im missing something? Actually it should be in any case if domain is present. Line 336: node = content.SelectSingleNode("Domain"); Line 337: if (node != null) { Line 338: vmBase.getVmInit().setDomain(node.innerText); Line 339: } -- To view, visit http://gerrit.ovirt.org/23020 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9383727c42713a0fda6d21782e2de708bfb57e47 Gerrit-PatchSet: 16 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Omer Frenkel <ofren...@redhat.com> Gerrit-Reviewer: Roy Golan <rgo...@redhat.com> Gerrit-Reviewer: Shahar Havivi <shav...@redhat.com> Gerrit-Reviewer: Tomas Jelinek <tjeli...@redhat.com> Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
