Maor Lipchuk has posted comments on this change. Change subject: core: Add alias and description for disk meta data ......................................................................
Patch Set 13: (11 comments) http://gerrit.ovirt.org/#/c/34163/13/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); > see comment in patch 3, the auditlog used is incorrect ( we are not updatin removed Line 131: return StringUtils.EMPTY; Line 132: } Line 133: } Line 134: http://gerrit.ovirt.org/#/c/34163/13/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/GetUnregisteredDiskQuery.java: Line 74: } Line 75: Line 76: DiskImage newDiskImage = (DiskImage) imageInfoReturn.getReturnValue(); Line 77: if (StringUtils.isNotEmpty(newDiskImage.getDescription())) { Line 78: Map<String, Object> diskDescriptionMap; > you can define it in line 80 done Line 79: try { Line 80: diskDescriptionMap = JsonHelper.jsonToMap(newDiskImage.getDescription()); Line 81: newDiskImage.setDiskAlias((String) diskDescriptionMap.get(ImagesHandler.DISK_ALIAS)); Line 82: newDiskImage.setDiskDescription((String) diskDescriptionMap.get(ImagesHandler.DISK_DESCRIPTION)); http://gerrit.ovirt.org/#/c/34163/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 354: Line 355: private void performDiskUpdate(final boolean unlockImage) { Line 356: if (isMetaDataPropertiesChangedForDiskImage()) { Line 357: updateMetaDataDescription((DiskImage) getNewDisk()); Line 358: } > now this command executes vdsm call, the disk should be locked in the db We should not add a lock in the DB since this is a synchronize operation Line 359: final Disk disk = getDiskDao().get(getParameters().getDiskId()); Line 360: applyUserChanges(disk); Line 361: Line 362: TransactionSupport.executeInNewTransaction(new TransactionMethod<Object>() { Line 395: } Line 396: }); Line 397: } Line 398: Line 399: private boolean isMetaDataPropertiesChangedForDiskImage() { > /s/isMetaDataPropertiesChangedForDiskImage/shouldPerformMetadataUpdate done Line 400: if ((getNewDisk().getDiskStorageType() == DiskStorageType.IMAGE) Line 401: && (!getOldDisk().getDiskAlias().equals(getNewDisk().getDiskAlias()) || !getOldDisk().getDiskDescription() Line 402: .equals(getNewDisk().getDiskDescription()))) { Line 403: return true; Line 397: } Line 398: Line 399: private boolean isMetaDataPropertiesChangedForDiskImage() { Line 400: if ((getNewDisk().getDiskStorageType() == DiskStorageType.IMAGE) Line 401: && (!getOldDisk().getDiskAlias().equals(getNewDisk().getDiskAlias()) || !getOldDisk().getDiskDescription() > can't the DiskDescription be null? what about NPE? use ObjectUtils.isEqual. done Line 402: .equals(getNewDisk().getDiskDescription()))) { Line 403: return true; Line 404: } Line 405: return false; Line 398: Line 399: private boolean isMetaDataPropertiesChangedForDiskImage() { Line 400: if ((getNewDisk().getDiskStorageType() == DiskStorageType.IMAGE) Line 401: && (!getOldDisk().getDiskAlias().equals(getNewDisk().getDiskAlias()) || !getOldDisk().getDiskDescription() Line 402: .equals(getNewDisk().getDiskDescription()))) { > please move this check (alias/description changed) to a ImagesHandler next adding comment in ImagesHandler Line 403: return true; Line 404: } Line 405: return false; Line 406: } Line 401: && (!getOldDisk().getDiskAlias().equals(getNewDisk().getDiskAlias()) || !getOldDisk().getDiskDescription() Line 402: .equals(getNewDisk().getDiskDescription()))) { Line 403: return true; Line 404: } Line 405: return false; > Agree with the above comments. We should not move the if condition to the ImagesHandler, I think that the updateVmDisk is more appropriate place since this is the only point which we use this and it is related specifically to the updateDisk operation. Once we will use this in other places then maybe we should move this to the ImagesHandler Line 406: } Line 407: Line 408: private void updateMetaDataDescription(DiskImage diskImage) { Line 409: try { Line 412: diskImage.getStorageIds().get(0), Line 413: diskImage.getId(), Line 414: diskImage.getImageId(), Line 415: getJsonDiskDescription()); Line 416: runVdsCommand(VDSCommandType.SetVolumeDescription, vdsCommandParameters); > ack. I disagree, first the user can still encounter a failure when calling the VdsCommnand, second, the user should still have an audit log message that he failed. Line 417: } catch (Exception e) { Line 418: StorageDomain storageDomain = Line 419: getStorageDomainDAO().getForStoragePool(diskImage.getStorageIds().get(0), Line 420: getVm().getStoragePoolId()); Line 430: try { Line 431: return ImagesHandler.getJsonDiskDescription(getParameters().getDiskInfo().getDiskAlias(), Line 432: getParameters().getDiskInfo().getDiskDescription()); Line 433: } catch (IOException e) { Line 434: throw new RuntimeException("Exception while generating json for disk", e); > ack I disagree, I think that this exception indicates that we failed on the json description, in that way we can see in the log whether this is a json problem or another problem. Line 435: } Line 436: } Line 437: Line 438: protected void updateDiskProfile() { http://gerrit.ovirt.org/#/c/34163/13/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/ImagesHandlerTest.java: Line 125: String diskAlias = null; Line 126: try { Line 127: diskAlias = ImagesHandler.getJsonDiskDescription("DiskAlias", "DiskDescription"); Line 128: } catch (IOException e) { Line 129: // Do nothing. Test should fail. > yup done Line 130: } Line 131: assertTrue("Should be map of disk alias and disk description", Line 132: diskAlias.equals("{\"DiskDescription\":\"DiskDescription\",\"DiskAlias\":\"DiskAlias\"}")); Line 133: } Line 129: // Do nothing. Test should fail. Line 130: } Line 131: assertTrue("Should be map of disk alias and disk description", Line 132: diskAlias.equals("{\"DiskDescription\":\"DiskDescription\",\"DiskAlias\":\"DiskAlias\"}")); Line 133: } > missing: a test for an empty ("") description and a null description. done -- 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: 13 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
