Gilad Chaplik has posted comments on this change. Change subject: core, db: disk profile for disk image ......................................................................
Patch Set 3: (14 comments) new patch to follow. http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java: Line 559: Line 560: private Guid getDiskProfileId() { Line 561: if (getParameters().getDiskInfo() != null Line 562: && DiskStorageType.IMAGE == getParameters().getDiskInfo().getDiskStorageType()) { Line 563: return getDiskImageInfo().getDiskProfileId(); > what about canDoAction check to verify that this profile exists? taken care of in patch: http://gerrit.ovirt.org/#/c/29038/ Line 564: } Line 565: return null; Line 566: } Line 567: http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromTemplateCommand.java: Line 123: params.setEntityInfo(getParameters().getEntityInfo()); Line 124: params.setQuotaId(diskInfoDestinationMap.get(disk.getId()).getQuotaId() != null ? Line 125: diskInfoDestinationMap.get(disk.getId()).getQuotaId() : null); Line 126: params.setDiskProfileId(diskInfoDestinationMap.get(disk.getId()).getDiskProfileId() != null ? Line 127: diskInfoDestinationMap.get(disk.getId()).getDiskProfileId() : null); > it exactly the same as writing Done Line 128: return params; Line 129: } Line 130: Line 131: @Override http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CopyImageGroupCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CopyImageGroupCommand.java: Line 111: getImageStorageDomainMapDao().save Line 112: (new image_storage_domain_map(getParameters().getImageId(), Line 113: getParameters().getStorageDomainId(), Line 114: getParameters().getQuotaId(), Line 115: getParameters().getDiskProfileId())); > shouldn't be passed on the parameters, this info should be contained in the should be treated exactly as quota and SD. when you copy image you can change DiskProfile. Line 116: } Line 117: Line 118: setSucceeded(true); Line 119: } http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateCloneOfTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateCloneOfTemplateCommand.java: Line 30: protected DiskImage cloneDiskImage(Guid newImageGuid) { Line 31: DiskImage returnValue = super.cloneDiskImage(newImageGuid); Line 32: returnValue.setStorageIds(new ArrayList<Guid>(Arrays.asList(getDestinationStorageDomainId()))); Line 33: returnValue.setQuotaId(getParameters().getQuotaId()); Line 34: returnValue.setDiskProfileId(getParameters().getDiskProfileId()); > see related comments same. Line 35: // override to have no template Line 36: returnValue.setParentId(VmTemplateHandler.BLANK_VM_TEMPLATE_ID); Line 37: returnValue.setImageTemplateId(VmTemplateHandler.BLANK_VM_TEMPLATE_ID); Line 38: if (getParameters().getDiskImageBase() != null) { http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java: Line 277: image.getActive(), Line 278: new image_storage_domain_map(image.getImageId(), Line 279: image.getStorageIds().get(0), Line 280: image.getQuotaId(), Line 281: image.getDiskProfileIds().get(0))); > in the current code disk can have no disk profile, which will lead to NPE/I Done Line 282: } Line 283: Line 284: /** Line 285: * Adds disk to a VM without creating a VmDevice entry http://gerrit.ovirt.org/#/c/29036/3/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 822: params.setEntityInfo(new EntityInfo(VdcObjectType.VM, getVm().getId())); Line 823: params.setRevertDbOperationScope(ImageDbOperationScope.IMAGE); Line 824: params.setQuotaId(disk.getQuotaId() != null ? disk.getQuotaId() : getParameters().getQuotaId()); Line 825: params.setDiskProfileId(disk.getDiskProfileId() != null ? disk.getDiskProfileId() Line 826: : getParameters().getDiskProfileId()); > is disk profile is part of the ovf? good point, it doesn't :-) should be taken from parameters Line 827: if (getParameters().getVm().getDiskMap() != null Line 828: && getParameters().getVm().getDiskMap().containsKey(originalDiskId)) { Line 829: DiskImageBase diskImageBase = Line 830: (DiskImageBase) getParameters().getVm().getDiskMap().get(originalDiskId); http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportVmTemplateCommand.java: Line 368: tempVar.setRevertDbOperationScope(ImageDbOperationScope.IMAGE); Line 369: for (DiskImage diskImage : getParameters().getVmTemplate().getDiskList()) { Line 370: if (originalDiskId.equals(diskImage.getId())) { Line 371: tempVar.setQuotaId(diskImage.getQuotaId()); Line 372: tempVar.setDiskProfileId(diskImage.getDiskProfileId()); > see relevant comment on ImportVm afaik, diskImage comes from parameters, which is okay. Line 373: break; Line 374: } Line 375: } Line 376: http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageDomainCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddStorageDomainCommand.java: Line 9: import java.util.List; Line 10: import java.util.Set; Line 11: Line 12: import org.ovirt.engine.core.bll.Backend; Line 13: import org.ovirt.engine.core.bll.profiles.DiskProfileHelper; > mistake, should be in the next patch... Done Line 14: import org.ovirt.engine.core.bll.utils.PermissionSubject; Line 15: import org.ovirt.engine.core.common.AuditLogType; Line 16: import org.ovirt.engine.core.common.VdcObjectType; Line 17: import org.ovirt.engine.core.common.action.StorageDomainManagementParameter; http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LiveMigrateDiskParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/LiveMigrateDiskParameters.java: Line 14: Guid sourceDomainId, Line 15: Guid destDomainId, Line 16: Guid vmId, Line 17: Guid quotaId, Line 18: Guid diskProfileId, > what's the reason to pass it to parameters? we should have it from the disk as I said in previous comment, when you move a disk you need to specify the dest SD profile, same as quota. Line 19: Guid imageGroupId) { Line 20: super(imageId, sourceDomainId, destDomainId, ImageOperation.Move); Line 21: setVmId(vmId); Line 22: setQuotaId(quotaId); http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoTest.java File backend/manager/modules/dal/src/test/java/org/ovirt/engine/core/dao/ImageStorageDomainMapDaoTest.java: Line 107: public void testChangeDiksProfileForDisk() { Line 108: // fetch image Line 109: image_storage_domain_map image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); Line 110: // test that the current disk profile doesn't equal with the new disk profile Line 111: if (image_storage_domain_map.getDiskProfileId().equals(FixturesTool.DISK_PROFILE_2)) { > no need for a constant, let's just create a new guid here. Done Line 112: fail("Same source and dest disk profile id, cannot perform test"); Line 113: } Line 114: // change to DISK_PROFILE_2 Line 115: dao.updateDiskProfileForImageAndSnapshots(EXISTING_DISK_ID, EXISTING_DOMAIN_ID, FixturesTool.DISK_PROFILE_2); Line 109: image_storage_domain_map image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); Line 110: // test that the current disk profile doesn't equal with the new disk profile Line 111: if (image_storage_domain_map.getDiskProfileId().equals(FixturesTool.DISK_PROFILE_2)) { Line 112: fail("Same source and dest disk profile id, cannot perform test"); Line 113: } > please change to assertEquals.. Done. since there is no assertNotEquals, using assertThat. Line 114: // change to DISK_PROFILE_2 Line 115: dao.updateDiskProfileForImageAndSnapshots(EXISTING_DISK_ID, EXISTING_DOMAIN_ID, FixturesTool.DISK_PROFILE_2); Line 116: // fetch the image again Line 117: image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); http://gerrit.ovirt.org/#/c/29036/3/backend/manager/modules/dal/src/test/resources/fixtures.xml File backend/manager/modules/dal/src/test/resources/fixtures.xml: Line 5026: <value>c2211b56-8869-41cd-84e1-78d7cb96f31d</value> Line 5027: <null /> Line 5028: </row> Line 5029: <row> Line 5030: <value>fd81f1e1-785b-4579-ab75-1419ebb87053</value> > seems related to other patch this is a disk profile record used to enhance current testing. Line 5031: <value>engine4_profile</value> Line 5032: <value>c2211b56-8869-41cd-84e1-78d7cb96f31d</value> Line 5033: <value>ae956031-6be2-43d6-bb90-5191c9253314</value> Line 5034: </row> http://gerrit.ovirt.org/#/c/29036/3/packaging/dbscripts/image_storage_domain_map_sp.sql File packaging/dbscripts/image_storage_domain_map_sp.sql: Line 83: WHERE i.image_group_id = v_disk_id AND i.image_guid = isdm.image_id AND storage_domain_id = v_storage_domain_id; Line 84: END; $procedure$ Line 85: LANGUAGE plpgsql; Line 86: Line 87: Create or replace FUNCTION UpdateDiskProfileForImageAndSnapshots(v_disk_id UUID, v_storage_domain_id UUID, v_disk_profile_id UUID) > /s/UpdateDiskProfileForImageAndSnapshots/UpdateDiskProfileByImageGroupId Done Line 88: RETURNS VOID Line 89: AS $procedure$ Line 90: BEGIN Line 91: UPDATE image_storage_domain_map as isdm Line 90: BEGIN Line 91: UPDATE image_storage_domain_map as isdm Line 92: SET disk_profile_id = v_disk_profile_id Line 93: FROM images as i Line 94: WHERE i.image_group_id = v_disk_id AND i.image_guid = isdm.image_id AND storage_domain_id = v_storage_domain_id; > clearer IMO - will put a note to self to get back to it :-) don't have time to test it now :/ Line 95: END; $procedure$ Line 96: LANGUAGE plpgsql; -- To view, visit http://gerrit.ovirt.org/29036 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9488a1e31e85013a01ee088fdf71854f765059c9 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Doron Fediuck <dfedi...@redhat.com> Gerrit-Reviewer: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Tal Nisan <tni...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
