Gilad Chaplik has posted comments on this change. Change subject: core: add disk profile to disk's commands ......................................................................
Patch Set 13: (19 comments) new patch to follow http://gerrit.ovirt.org/#/c/29038/13/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 598: } Line 599: Line 600: protected boolean setAndValidateDiskProfiles() { Line 601: if (getDiskImageInfo() != null Line 602: && DiskStorageType.IMAGE == getDiskImageInfo().getDiskStorageType()) { > the if clause can be removed, see from where this method is called Done Line 603: Map<DiskImage, Guid> map = new HashMap<>(); Line 604: map.put(getDiskImageInfo(), getStorageDomainId()); Line 605: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); Line 606: } Line 600: protected boolean setAndValidateDiskProfiles() { Line 601: if (getDiskImageInfo() != null Line 602: && DiskStorageType.IMAGE == getDiskImageInfo().getDiskStorageType()) { Line 603: Map<DiskImage, Guid> map = new HashMap<>(); Line 604: map.put(getDiskImageInfo(), getStorageDomainId()); > replace with Collections.singletonMap Done Line 605: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); Line 606: } Line 607: return true; Line 608: } http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmCommand.java: Line 595: } Line 596: Line 597: protected boolean setAndValidateDiskProfiles() { Line 598: Map<DiskImage, Guid> map = new HashMap<>(); Line 599: if (diskInfoDestinationMap != null) { > create the map here only when needed Done Line 600: for (DiskImage diskImage : diskInfoDestinationMap.values()) { Line 601: map.put(diskImage, diskImage.getStorageIds().get(0)); Line 602: } Line 603: } Line 600: for (DiskImage diskImage : diskInfoDestinationMap.values()) { Line 601: map.put(diskImage, diskImage.getStorageIds().get(0)); Line 602: } Line 603: } Line 604: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); > move this to the if caluse, here you can return true. Done Line 605: } Line 606: Line 607: protected boolean checkTemplateImages(List<String> reasons) { Line 608: if (getParameters().getParentCommand() == VdcActionType.AddVmPoolWithVms) { http://gerrit.ovirt.org/#/c/29038/13/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 408: return doClusterRelatedChecks(); Line 409: } Line 410: } Line 411: Line 412: protected boolean setAndValidateDiskProfiles() { > same as previous file Done Line 413: Map<DiskImage, Guid> map = new HashMap<>(); Line 414: if (diskInfoDestinationMap != null) { Line 415: for (DiskImage diskImage : diskInfoDestinationMap.values()) { Line 416: map.put(diskImage, diskImage.getStorageIds().get(0)); http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/CreateAllSnapshotsFromVmCommand.java: Line 624: Line 625: protected boolean setAndValidateDiskProfiles() { Line 626: if (getDisksList() != null) { Line 627: Map<DiskImage, Guid> map = new HashMap<>(); Line 628: for (DiskImage diskImage : getDisksList()) { > when we create a snapshot, all the disks already have a profile that will happen by default, we're handling disks and not snapshots. Line 629: map.put(diskImage, diskImage.getStorageIds().get(0)); Line 630: } Line 631: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); Line 632: } http://gerrit.ovirt.org/#/c/29038/13/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 1291: protected boolean setAndValidateDiskProfiles() { Line 1292: if (getParameters().getVm().getDiskMap() != null) { Line 1293: Map<DiskImage, Guid> map = new HashMap<>(); Line 1294: for (Disk disk : getParameters().getVm().getDiskMap().values()) { Line 1295: if (disk instanceof DiskImage) { > check it like this - disk,getDiskStorageType() == DiskStorageType.IMAGE Done Line 1296: DiskImage diskImage = (DiskImage) disk; Line 1297: map.put(diskImage, imageToDestinationDomainMap.get(diskImage.getId())); Line 1298: } Line 1299: } http://gerrit.ovirt.org/#/c/29038/13/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 550: Line 551: protected boolean setAndValidateDiskProfiles() { Line 552: if (getParameters().getVmTemplate().getDiskList() != null) { Line 553: Map<DiskImage, Guid> map = new HashMap<>(); Line 554: for (Disk disk : getParameters().getVmTemplate().getDiskList()) { > /s/Disk/DiskImage Done Line 555: if (disk instanceof DiskImage) { Line 556: DiskImage diskImage = (DiskImage) disk; Line 557: map.put(diskImage, imageToDestinationDomainMap.get(diskImage.getId())); Line 558: } Line 551: protected boolean setAndValidateDiskProfiles() { Line 552: if (getParameters().getVmTemplate().getDiskList() != null) { Line 553: Map<DiskImage, Guid> map = new HashMap<>(); Line 554: for (Disk disk : getParameters().getVmTemplate().getDiskList()) { Line 555: if (disk instanceof DiskImage) { > getDiskList() returns a list of disk image, no need for the instance check. Done Line 556: DiskImage diskImage = (DiskImage) disk; Line 557: map.put(diskImage, imageToDestinationDomainMap.get(diskImage.getId())); Line 558: } Line 559: } http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/MoveOrCopyDiskCommand.java: Line 388: public String getDiskAlias() { Line 389: return getImage().getDiskAlias(); Line 390: } Line 391: Line 392: protected boolean setAndValidateDiskProfiles() { > this shoud be executed here only if the operation is "copy" (which is possi can you please elaborate why it has to be separated ? it's implemented here for both. similar code is already tested for at least 3 versions, we can do a refactoring later on. Line 393: if (getImage() != null Line 394: && DiskStorageType.IMAGE == getImage().getDiskStorageType()) { Line 395: // created a dummy disk image wrapper to allow Disk Profile check. Line 396: DiskImage diskImage = new DiskImage(); Line 390: } Line 391: Line 392: protected boolean setAndValidateDiskProfiles() { Line 393: if (getImage() != null Line 394: && DiskStorageType.IMAGE == getImage().getDiskStorageType()) { > getImage() won't return something that isn't image :) Done Line 395: // created a dummy disk image wrapper to allow Disk Profile check. Line 396: DiskImage diskImage = new DiskImage(); Line 397: diskImage.setDiskProfileId(getParameters().getDiskProfileId()); Line 398: Map<DiskImage, Guid> map = new HashMap<>(); Line 391: Line 392: protected boolean setAndValidateDiskProfiles() { Line 393: if (getImage() != null Line 394: && DiskStorageType.IMAGE == getImage().getDiskStorageType()) { Line 395: // created a dummy disk image wrapper to allow Disk Profile check. > why? actually that's wrong, we're using parameter in execute and not the image (so I'm changing it in parent exectue, since diskProfileId isn't mandatory). Line 396: DiskImage diskImage = new DiskImage(); Line 397: diskImage.setDiskProfileId(getParameters().getDiskProfileId()); Line 398: Map<DiskImage, Guid> map = new HashMap<>(); Line 399: map.put(diskImage, getParameters().getStorageDomainId()); http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RegisterDiskCommand.java: Line 88: } Line 89: Line 90: protected boolean setAndValidateDiskProfiles() { Line 91: if (getParameters().getDiskImage() != null Line 92: && DiskStorageType.IMAGE == getParameters().getDiskImage().getDiskStorageType()) { > unneeded checks Done Line 93: Map<DiskImage, Guid> map = new HashMap<>(); Line 94: map.put(getParameters().getDiskImage(), getStorageDomainId()); Line 95: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); Line 96: } Line 89: Line 90: protected boolean setAndValidateDiskProfiles() { Line 91: if (getParameters().getDiskImage() != null Line 92: && DiskStorageType.IMAGE == getParameters().getDiskImage().getDiskStorageType()) { Line 93: Map<DiskImage, Guid> map = new HashMap<>(); > Collections.singletonMap Done Line 94: map.put(getParameters().getDiskImage(), getStorageDomainId()); Line 95: return validate(DiskProfileHelper.setAndValidateDiskProfiles(map)); Line 96: } Line 97: return true; http://gerrit.ovirt.org/#/c/29038/13/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 157: && getVmDynamicDao().get(getVmId()).getStatus() == VMStatus.Paused; Line 158: ExecutionHandler.endJob(executionContext, runAndPausedSucceeded); Line 159: } Line 160: Line 161: protected boolean setAndValidateDiskProfiles() { > can you elaborate why it's needed here? you'd like to have qos defined for run once state. Line 162: if (isRunAsStateless() && getVm().getDiskList() != null) { Line 163: Map<DiskImage, Guid> map = new HashMap<>(); Line 164: for (DiskImage diskImage : getVm().getDiskList()) { Line 165: map.put(diskImage, diskImage.getStorageIds().get(0)); http://gerrit.ovirt.org/#/c/29038/13/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 523: } Line 524: return null; Line 525: } Line 526: Line 527: protected boolean setAndValidateDiskProfiles() { > why it's needed here? the disk already exists. can the profile be updated? * yes, we need to check disk profiles for update disk. * yes the profile can be updated. * it's a user choice, but if he doesn't provide it (null) and there's only one DP for SD we set it. Line 528: if (isDiskImage()) { Line 529: DiskImage diskImage = (DiskImage) getNewDisk(); Line 530: Map<DiskImage, Guid> map = new HashMap<>(); Line 531: map.put(diskImage, diskImage.getStorageIds().get(0)); http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/lsm/LiveMigrateVmDisksCommand.java: Line 257: addCanDoActionMessage(VdcBllMessages.VAR__ACTION__MOVE); Line 258: addCanDoActionMessage(VdcBllMessages.VAR__TYPE__VM_DISK); Line 259: } Line 260: Line 261: protected boolean setAndValidateDiskProfiles() { > seems to me that this validation should be on MoveDisksCommand instead of h same as previous, can be later refactored. Line 262: Map<DiskImage, Guid> map = new HashMap<>(); Line 263: for (LiveMigrateDiskParameters parameters : getParameters().getParametersList()) { Line 264: DiskImage diskImage = getDiskImageByImageId(parameters.getImageId()); Line 265: if (diskImage != null && diskImage.getDiskStorageType() == DiskStorageType.IMAGE) { http://gerrit.ovirt.org/#/c/29038/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileHelper.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileHelper.java: Line 34: for (Entry<DiskImage, Guid> entry : map.entrySet()) { Line 35: DiskImage diskImage = entry.getKey(); Line 36: Guid storageDomainId = entry.getValue(); Line 37: DiskProfile diskProfile = null; Line 38: if (diskImage.getDiskProfileId() == null && storageDomainId != null) { // set disk profile if there's only 1 for SD. > move the code from line 43 to here and check if it's null instead of "conta please read the commit message. iff the DiskProfileId is null I set it. Line 39: if (!storageDiskProfilesMap.containsKey(storageDomainId)) { Line 40: storageDiskProfilesMap.put(storageDomainId, Line 41: getDiskProfileDao().getAllForStorageDomain(storageDomainId)); Line 42: } Line 48: } else { Line 49: diskProfile = getDiskProfileDao().get(diskImage.getDiskProfileId()); Line 50: } Line 51: Line 52: ValidationResult result = new DiskProfileValidator(diskProfile).isStorageDomainValid(storageDomainId); > please file a bug to eliminate those unneeded checks when we set the profil didn't get you. Line 53: if (result != ValidationResult.VALID) { Line 54: return result; Line 55: } Line 56: } -- To view, visit http://gerrit.ovirt.org/29038 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie7b6d977243cffc0bde772665ebaca47340075c6 Gerrit-PatchSet: 13 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <gchap...@redhat.com> Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com> Gerrit-Reviewer: Arik Hadas <aha...@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
