Liron Aravot has posted comments on this change. Change subject: core: Add alias and description for disk meta data ......................................................................
Patch Set 3: (6 comments) http://gerrit.ovirt.org/#/c/34163/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddImageFromScratchCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddImageFromScratchCommand.java: Line 126: AuditLogableBase auditLogableBase = new AuditLogableBase(); Line 127: auditLogableBase.addCustomValue("DataCenterName", getStoragePool().getName()); Line 128: auditLogableBase.addCustomValue("StorageDomainName", getStorageDomain().getName()); Line 129: auditLogableBase.addCustomValue("DiskName", getParameters().getDiskInfo().getDiskAlias()); Line 130: AuditLogDirector.log(auditLogableBase, AuditLogType.UPDATE_DESCRIPTION_FOR_DISK_FAILED); there error here is in generating the description and not in updating it (why would it ever happen? log should be enough). Line 131: return StringUtils.EMPTY; Line 132: } Line 133: } Line 134: Line 127: auditLogableBase.addCustomValue("DataCenterName", getStoragePool().getName()); Line 128: auditLogableBase.addCustomValue("StorageDomainName", getStorageDomain().getName()); Line 129: auditLogableBase.addCustomValue("DiskName", getParameters().getDiskInfo().getDiskAlias()); Line 130: AuditLogDirector.log(auditLogableBase, AuditLogType.UPDATE_DESCRIPTION_FOR_DISK_FAILED); Line 131: return StringUtils.EMPTY; you can replace StringUtils.EMPTY with "", it's shorter. Line 132: } Line 133: } Line 134: Line 135: @Override http://gerrit.ovirt.org/#/c/34163/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/DetachDiskFromVmCommand.java: Line 104: Line 105: setSucceeded(true); Line 106: } Line 107: Line 108: private void updateMetaDataDescription() { I don't think that's correct - *there's no need to perform the update on each detach of a disk- what if nothing was updated? *There are further problems, today you can detach a disk also when the domain is inactive..etc, which is obviously not possible now. Detach is an engine only operations and should remain so. *The needed fields should be updated when a disk is updated, the bug isn't only about import storage domain scenario, but also generally when you to the storage and want to locate a disk (for some reason), it's incorrect to update it here imo. Line 109: DiskImage diskImage = (DiskImage) disk; Line 110: SetVolumeDescriptionVDSCommandParameters vdsCommandParameters = Line 111: new SetVolumeDescriptionVDSCommandParameters(getVm().getStoragePoolId(), Line 112: diskImage.getStorageIds().get(0), Line 126: AuditLogDirector.log(auditLogableBase, AuditLogType.UPDATE_DESCRIPTION_FOR_DISK_FAILED); Line 127: } Line 128: } Line 129: Line 130: private String getJsonDiskDescription() { why the behavior here is different than the AddImageFromScratch command? Line 131: try { Line 132: return ImagesHandler.getJsonDiskDescription(getParameters().getDiskInfo().getDiskAlias(), Line 133: getParameters().getDiskInfo().getDiskDescription()); Line 134: } catch (IOException e) { http://gerrit.ovirt.org/#/c/34163/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 843: dummy.getSnapshots().addAll(ImagesHandler.getAllImageSnapshots(dummy.getImageId())); Line 844: return dummy; Line 845: } Line 846: Line 847: public static String getJsonDiskDescription(String diskAlias, String diskDescription) throws IOException { Not sure that i agree with this approach though I understand the motivation for choosing it. I think that description is description and alias is alias, those fields are relevant for any disk so seems to me like they should be separated in the metadata and not combined into one field or we can save only one of them. Line 848: Map<String, Object> description = new HashMap<>(); Line 849: description.put(ImagesHandler.DISK_ALIAS, diskAlias); Line 850: description.put(ImagesHandler.DISK_DESCRIPTION, diskDescription); Line 851: return JsonHelper.mapToJson(description, false); http://gerrit.ovirt.org/#/c/34163/3/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 853: CREATE_OVF_STORE_FOR_STORAGE_DOMAIN_FAILED(191, AuditLogSeverity.WARNING), Line 854: CREATE_OVF_STORE_FOR_STORAGE_DOMAIN_INITIATE_FAILED(192), Line 855: DELETE_OVF_STORE_FOR_STORAGE_DOMAIN_FAILED(193), Line 856: UPDATE_DESCRIPTION_FOR_OVF_STORE_FAILED(1016), Line 857: UPDATE_DESCRIPTION_FOR_DISK_FAILED(1018, AuditLogSeverity.WARNING), if you added it as 1018, please move it a line below after 1017 to try and keep some order in this class Line 858: RETRIEVE_OVF_STORE_FAILED(1017, AuditLogSeverity.WARNING), Line 859: Line 860: // Authentication Line 861: USER_ACCOUNT_DISABLED_OR_LOCKED(160, AuditLogSeverity.ERROR, -- To view, visit http://gerrit.ovirt.org/34163 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie2642ae7016579ead699509426e01ac2010bd374 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Allon Mureinik <[email protected]> Gerrit-Reviewer: Daniel Erez <[email protected]> Gerrit-Reviewer: Liron Aravot <[email protected]> Gerrit-Reviewer: Maor Lipchuk <[email protected]> Gerrit-Reviewer: Tal Nisan <[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
