Liron Aravot has posted comments on this change. Change subject: core, db: disk profile for disk image ......................................................................
Patch Set 7: Code-Review-1 (9 comments) http://gerrit.ovirt.org/#/c/29036/7/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 127: params.setParentParameters(getParameters()); Line 128: params.setEntityInfo(getParameters().getEntityInfo()); Line 129: params.setQuotaId(diskInfoDestinationMap.get(disk.getId()).getQuotaId() != null ? Line 130: diskInfoDestinationMap.get(disk.getId()).getQuotaId() : null); Line 131: params.setDiskProfileId(diskInfoDestinationMap.get(disk.getId()).getDiskProfileId()); there's an issue here - if the vm disks are created on another domain, it shouldn't have the profile related to the domain the template disks reside on Line 132: return params; Line 133: } Line 134: Line 135: @Override http://gerrit.ovirt.org/#/c/29036/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmTemplateCommand.java: Line 205: .get(0)); Line 206: createParams.setDiskAlias(diskInfoDestinationMap.get(diskImage.getId()).getDiskAlias()); Line 207: createParams.setParentParameters(getParameters()); Line 208: createParams.setQuotaId(getQuotaIdForDisk(diskImage)); Line 209: createParams.setDiskProfileId(diskInfoDestinationMap.get(diskImage.getId()).getDiskProfileId()); same here, if the template disks are on a different domain than the vm disks, we can't have the same profile Line 210: return createParams; Line 211: } Line 212: Line 213: @Override http://gerrit.ovirt.org/#/c/29036/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportRepoImageCreateTaskHandler.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImportRepoImageCreateTaskHandler.java: Line 92: parameters.setStoragePoolId(enclosingCommand.getParameters().getStoragePoolId()); Line 93: parameters.setStorageDomainId(enclosingCommand.getParameters().getStorageDomainId()); Line 94: parameters.setImageGroupID(enclosingCommand.getParameters().getImageGroupID()); Line 95: parameters.setQuotaId(enclosingCommand.getParameters().getQuotaId()); Line 96: parameters.setDiskProfileId(enclosingCommand.getParameters().getDiskProfileId()); is the disk profile is set in the enclosing command on that case? Line 97: parameters.setParentCommand(VdcActionType.ImportRepoImage); Line 98: parameters.setParentParameters(enclosingCommand.getParameters()); Line 99: parameters.setDestinationImageId(enclosingCommand.getParameters().getDestinationImageId()); Line 100: return parameters; http://gerrit.ovirt.org/#/c/29036/7/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 825: params.setImportEntity(true); Line 826: params.setEntityInfo(new EntityInfo(VdcObjectType.VM, getVm().getId())); Line 827: params.setRevertDbOperationScope(ImageDbOperationScope.IMAGE); Line 828: params.setQuotaId(disk.getQuotaId() != null ? disk.getQuotaId() : getParameters().getQuotaId()); Line 829: params.setDiskProfileId(getParameters().getDiskProfileId()); where's the disk profile is being set in the parameters in that case? Line 830: if (getParameters().getVm().getDiskMap() != null Line 831: && getParameters().getVm().getDiskMap().containsKey(originalDiskId)) { Line 832: DiskImageBase diskImageBase = Line 833: (DiskImageBase) getParameters().getVm().getDiskMap().get(originalDiskId); http://gerrit.ovirt.org/#/c/29036/7/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 367: tempVar.setRevertDbOperationScope(ImageDbOperationScope.IMAGE); Line 368: for (DiskImage diskImage : getParameters().getVmTemplate().getDiskList()) { Line 369: if (originalDiskId.equals(diskImage.getId())) { Line 370: tempVar.setQuotaId(diskImage.getQuotaId()); Line 371: tempVar.setDiskProfileId(diskImage.getDiskProfileId()); when it's set to the disk list in the parameters? Line 372: break; Line 373: } Line 374: } Line 375: http://gerrit.ovirt.org/#/c/29036/7/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java: Line 351: getBaseDiskDao().update(disk); Line 352: if (disk.getDiskStorageType() == DiskStorageType.IMAGE) { Line 353: DiskImage diskImage = (DiskImage) disk; Line 354: diskImage.setQuotaId(getQuotaId()); Line 355: diskImage.setDiskProfileId(((DiskImage) getNewDisk()).getDiskProfileId()); this should move to the applyUserChanges method Line 356: if (unlockImage && diskImage.getImageStatus() == ImageStatus.LOCKED) { Line 357: diskImage.setImageStatus(ImageStatus.OK); Line 358: } Line 359: getImageDao().update(diskImage.getImage()); Line 386: DiskImage oldDisk = (DiskImage) getOldDisk(); Line 387: if (!Objects.equals(oldDisk.getDiskProfileId(), diskImage.getDiskProfileId())) { Line 388: getImageStorageDomainMapDao().updateDiskProfileByImageGroupId(diskImage.getId(), Line 389: diskImage.getStorageIds().get(0), Line 390: diskImage.getDiskProfileIds().get(0)); why not getDiskProfileId? Line 391: } Line 392: } Line 393: } Line 394: http://gerrit.ovirt.org/#/c/29036/7/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 111: // fetch image Line 112: image_storage_domain_map image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); Line 113: // test that the current disk profile doesn't equal with the new disk profile Line 114: assertThat("Same source and dest disk profile id, cannot perform test", Line 115: image_storage_domain_map.getDiskProfileId(), not(equalTo(Guid.newGuid()))); you use different guid here and on the update statement. Line 116: // change to newDiskProfileId Line 117: dao.updateDiskProfileByImageGroupId(EXISTING_DISK_ID, EXISTING_DOMAIN_ID, FixturesTool.DISK_PROFILE_2); Line 118: // fetch the image again Line 119: image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); Line 115: image_storage_domain_map.getDiskProfileId(), not(equalTo(Guid.newGuid()))); Line 116: // change to newDiskProfileId Line 117: dao.updateDiskProfileByImageGroupId(EXISTING_DISK_ID, EXISTING_DOMAIN_ID, FixturesTool.DISK_PROFILE_2); Line 118: // fetch the image again Line 119: image_storage_domain_map = dao.getAllByImageId(EXISTING_IMAGE_ID).get(0); this check might be wrong, if there are some records for that image on different domains..you might fail although the profile was updated. Line 120: // check that the new disk profile is the inserted one Line 121: assertEquals("disk profile wasn't changed", Line 122: image_storage_domain_map.getDiskProfileId(), Line 123: FixturesTool.DISK_PROFILE_2); -- 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: 7 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
