Omer Frenkel has posted comments on this change. Change subject: Engine: Vm Init - new Feature ......................................................................
Patch Set 16: (10 comments) partially reviewed, some comments for now 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 doing the same, please verify (this is called when creating vm pool) 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 and not in canDoAction 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 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? 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 keep it to be on the safe side 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? 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.. 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 exist) and then add the new one? or just update? Line 308: } else if (oldVm != null) { Line 309: // make sure we have the correct vm init Line 310: VmHandler.updateVmInitFromDB(oldVm, true); Line 311: Line 315: } Line 316: } Line 317: } Line 318: Line 319: public static void removeVmInitFromDB(VmBase vm) { i think you can remove without checking if exist.. but need to verify this Line 320: VmInitDAO db = DbFacade.getInstance().getVmInitDao(); Line 321: if (db.get(vm.getId()) != null) { Line 322: db.remove(vm.getId()); Line 323: } 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 Line 258: private VmInit vmInit; Line 259: Line 260: public VmBase(VmBase vmBase) { Line 261: this(vmBase.getName(), -- 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 <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Omer Frenkel <[email protected]> Gerrit-Reviewer: Roy Golan <[email protected]> Gerrit-Reviewer: Shahar Havivi <[email protected]> Gerrit-Reviewer: Tomas Jelinek <[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
