Liron Aravot has posted comments on this change. Change subject: core: disk profile commands and queries ......................................................................
Patch Set 10: Code-Review-1 (17 comments) http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/AddDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/AddDiskProfileCommand.java: Line 8: import org.ovirt.engine.core.common.VdcObjectType; Line 9: import org.ovirt.engine.core.common.action.DiskProfileParameters; Line 10: import org.ovirt.engine.core.common.errors.VdcBllMessages; Line 11: import org.ovirt.engine.core.compat.Guid; Line 12: please make non transactive. when we'll solve the bug of the "default non transactive" commands it'll be removed from here as well. Line 13: public class AddDiskProfileCommand extends DiskProfileCommandBase { Line 14: Line 15: public AddDiskProfileCommand(DiskProfileParameters parameters) { Line 16: super(parameters); Line 27: Line 28: @Override Line 29: protected void executeCommand() { Line 30: getParameters().getProfile().setId(Guid.newGuid()); Line 31: getDiskProfileDao().save(getParameters().getProfile()); how do we handle synchronization here? for example - what if we add a profile and on the same time remove the chosen qos?same goes for the name.. locks should be added. Line 32: getReturnValue().setActionReturnValue(getParameters().getProfile().getId()); Line 33: setSucceeded(true); Line 34: } Line 35: Line 34: } Line 35: Line 36: @Override Line 37: public List<PermissionSubject> getPermissionCheckSubjects() { Line 38: return Collections.singletonList(new PermissionSubject(getParameters().getProfile().getStorageDomainId(), if the profile isn't set you'll have NPE here. Line 39: VdcObjectType.Storage, getActionType().getActionGroup())); Line 40: } Line 41: Line 42: @Override http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileCommandBase.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileCommandBase.java: Line 18: if (profile == null) { Line 19: if (getParameters().getProfile() != null) { Line 20: profile = getParameters().getProfile(); Line 21: } Line 22: else if (getParameters().getProfileId() != null) { please move the else a line up Line 23: profile = getProfileDao().get(getParameters().getProfileId()); Line 24: } else { Line 25: log.error("couldn't find disk profile"); Line 26: } Line 21: } Line 22: else if (getParameters().getProfileId() != null) { Line 23: profile = getProfileDao().get(getParameters().getProfileId()); Line 24: } else { Line 25: log.error("couldn't find disk profile"); - no need for that, same as we do not log if it's not found in the db. - perhaps just init it on the begining, otherwise if it doesn't exists each call to getProfile will attempt to load it again. Line 26: } Line 27: } Line 28: return profile; Line 29: } Line 34: profileId = getParameters().getProfileId(); Line 35: } else if (getParameters().getProfile() != null) { Line 36: profileId = getParameters().getProfile().getId(); Line 37: } else { Line 38: log.error("couldn't find disk profile id"); same.. Line 39: } Line 40: } Line 41: return profileId; Line 42: } http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/DiskProfileValidator.java: Line 15: Line 16: public class DiskProfileValidator { Line 17: Line 18: private final DiskProfile diskProfile; Line 19: private DiskProfile oldDiskProfile; please rename to diskProfileFromDb Line 20: private StorageDomain storageDomain; Line 21: private List<DiskProfile> diskProfiles; Line 22: Line 23: // private List<DiskImage> diskImages; Line 19: private DiskProfile oldDiskProfile; Line 20: private StorageDomain storageDomain; Line 21: private List<DiskProfile> diskProfiles; Line 22: Line 23: // private List<DiskImage> diskImages; ? Line 24: Line 25: public DiskProfileValidator(DiskProfile diskProfile) { Line 26: this.diskProfile = diskProfile; Line 27: } Line 72: } Line 73: Line 74: // public ValidationResult diskProfileNotUsedByDisks() { Line 75: // return diskProfileNotUsed(getDisksUsingProfile(), VdcBllMessages.VAR__ENTITIES__VMS); Line 76: // } ? Line 77: Line 78: protected ValidationResult diskProfileNotUsed(List<? extends Nameable> entities, VdcBllMessages entitiesReplacement) { Line 79: if (entities.isEmpty()) { Line 80: return ValidationResult.VALID; Line 107: } Line 108: Line 109: return oldDiskProfile; Line 110: } Line 111: ? Line 112: // protected List<VM> getDisksUsingProfile() { Line 113: // if (diskImages == null) { Line 114: // diskImages = getDbFacade().getDiskImageDao().getAllForDiskProfile(diskProfile.getId()); Line 115: // } http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/RemoveDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/RemoveDiskProfileCommand.java: Line 19: protected boolean canDoAction() { Line 20: DiskProfileValidator validator = new DiskProfileValidator(getProfile()); Line 21: return validate(validator.diskProfileIsSet()) Line 22: && validate(validator.diskProfileExists()); Line 23: // && validate(validator.diskProfileNotUsedBydisks()) why don't we check if it's used or not? what about synchronization with other flows that uses the profile? Line 24: } Line 25: Line 26: @Override Line 27: protected void executeCommand() { Line 30: } Line 31: Line 32: @Override Line 33: public List<PermissionSubject> getPermissionCheckSubjects() { Line 34: return Collections.singletonList(new PermissionSubject(getParameters().getProfile().getStorageDomainId(), NPE if the profile isn't set (checked on CDA) Line 35: VdcObjectType.Storage, getActionType().getActionGroup())); Line 36: } Line 37: Line 38: @Override http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/UpdateDiskProfileCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/profiles/UpdateDiskProfileCommand.java: Line 21: return validate(validator.diskProfileIsSet()) Line 22: && validate(validator.diskProfileExists()) Line 23: && validate(validator.diskProfileNameNotUsed()) Line 24: && validate(validator.storageDomainNotChanged()) Line 25: && validate(validator.qosExistsOrNull()); what if there are running vms whose disks use that profile? we should atleast have this mentioned in the audit log (change will take affect on next run..something like that) Line 26: } Line 27: Line 28: @Override Line 29: protected void executeCommand() { http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/StorageDomainValidator.java: Line 37: return ValidationResult.VALID; Line 38: } Line 39: Line 40: public ValidationResult isDomainExistAndActive() { Line 41: if (storageDomain == null) { please replace those lines with call to the method you added.. Line 42: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_NOT_EXIST); Line 43: } Line 44: if (storageDomain.getStatus() != StorageDomainStatus.Active) { Line 45: return new ValidationResult(VdcBllMessages.ACTION_TYPE_FAILED_STORAGE_DOMAIN_STATUS_ILLEGAL2, http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/AuditLogType.java: Line 995: USER_UPDATED_QOS(10114), Line 996: USER_FAILED_TO_UPDATE_QOS(10115, AuditLogSeverity.ERROR), Line 997: Line 998: // Disk Profile Line 999: USER_ADDED_DISK_PROFILE(10120), AuditLogMessages.properties is missing in this patch Line 1000: USER_FAILED_TO_ADD_DISK_PROFILE(10121, AuditLogSeverity.ERROR), Line 1001: USER_REMOVED_DISK_PROFILE(10122), Line 1002: USER_FAILED_TO_REMOVE_DISK_PROFILE(10123, AuditLogSeverity.ERROR), Line 1003: USER_UPDATED_DISK_PROFILE(10124), http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/DiskProfileParameters.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/action/DiskProfileParameters.java: Line 9: Line 10: public DiskProfileParameters() { Line 11: } Line 12: Line 13: public DiskProfileParameters(DiskProfile diskProfile, Guid diskProfileId) { can't we use the id within the diskProfile object? Line 14: super(diskProfile, diskProfileId); Line 15: } http://gerrit.ovirt.org/#/c/28732/10/backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java File backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/errors/VdcBllMessages.java: Line 1002: // profiles Line 1003: ACTION_TYPE_FAILED_PROFILE_NOT_EXISTS(ErrorType.BAD_PARAMETERS), Line 1004: ACTION_TYPE_FAILED_PROFILE_NAME_IN_USE(ErrorType.BAD_PARAMETERS), Line 1005: ACTION_TYPE_FAILED_CANNOT_CHANGE_PROFILE(ErrorType.BAD_PARAMETERS), Line 1006: ACTION_TYPE_FAILED_PROFILE_IN_USE(ErrorType.BAD_PARAMETERS), messages files are missing in this patch Line 1007: Line 1008: // Affinity Groups Line 1009: AFFINITY_GROUP_NAME_TOO_LONG(ErrorType.BAD_PARAMETERS), Line 1010: AFFINITY_GROUP_NAME_INVALID(ErrorType.BAD_PARAMETERS), -- To view, visit http://gerrit.ovirt.org/28732 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I0d871f1603671bc14ce88c68f85638b1af67f5e1 Gerrit-PatchSet: 10 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Arik Hadas <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Kobi Ianko <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: [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
