Maor Lipchuk has posted comments on this change. Change subject: core: introduce add Cinder disk support ......................................................................
Patch Set 3: (5 comments) https://gerrit.ovirt.org/#/c/39024/3/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 536: @Override Line 537: public AuditLogType getAuditLogTypeValue() { Line 538: switch (getActionState()) { Line 539: case EXECUTE: Line 540: if (getParameters().getDiskInfo().getDiskStorageType() == DiskStorageType.IMAGE || Consider to re-extrract this method to something more meaningless like 'isDiskStorageTypeSupportExecute()' Line 541: getParameters().getDiskInfo().getDiskStorageType() == DiskStorageType.CINDER) { Line 542: return getExecuteAuditLogTypeValue(getSucceeded()); Line 543: } else { Line 544: return getEndSuccessAuditLogTypeValue(getSucceeded()); https://gerrit.ovirt.org/#/c/39024/3/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddCinderDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/AddCinderDiskCommand.java: Line 77: null, Line 78: shouldDiskBePlugged(), Line 79: Boolean.TRUE.equals(cinderDisk.getReadOnly()), Line 80: null); Line 81: } I would re-factor this in AddDiskCommand at createDiskBasedOnImage for example Line 82: return null; Line 83: } Line 84: }); Line 85: } Line 116: @Override Line 117: protected void endWithFailure() { Line 118: super.endWithFailure(); Line 119: ImagesHandler.updateImageStatus(getDiskId(), ImageStatus.ILLEGAL); Line 120: auditLogDirector.log(this, getEndSuccessAuditLogTypeValue(false)); Worth to add a comment here to mention why we initialize the audit log with false and why it was initialize with true at endSuccessfully Line 121: } Line 122: Line 123: private Guid getDiskId() { Line 124: return getParameters().getDiskInfo().getId(); Line 135: } Line 136: Line 137: @Override Line 138: protected Map<String, Pair<String, String>> getExclusiveLocks() { Line 139: return Collections.emptyMap(); What about boot disk? we will need to lock the VM so we won't create two of them Line 140: } Line 141: Line 142: @Override Line 143: protected Map<String, Pair<String, String>> getSharedLocks() { Line 140: } Line 141: Line 142: @Override Line 143: protected Map<String, Pair<String, String>> getSharedLocks() { Line 144: return Collections.emptyMap(); We should add a shared lock on the disk Line 145: } -- To view, visit https://gerrit.ovirt.org/39024 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ie20f31afc59801e951b52053ba8bfef55e13e323 Gerrit-PatchSet: 3 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Maor Lipchuk <mlipc...@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
