Daniel Erez has posted comments on this change. Change subject: core: introduce RegisterCinderDiskCommand ......................................................................
Patch Set 2: (4 comments) https://gerrit.ovirt.org/#/c/39652/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RegisterCinderDiskCommand.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/storage/RegisterCinderDiskCommand.java: Line 32: public boolean canDoAction() { Line 33: CinderDisk cinderDisk = getCinderDisk(); Line 34: cinderDisk.setStorageIds(new ArrayList<>(Arrays.asList(getParameters().getStorageDomainId()))); Line 35: CinderDisksValidator cinderDiskValidator = getCinderDisksValidator(cinderDisk); Line 36: return validate(cinderDiskValidator.validateCinderDisksAlreadyRegistered()); > Why not adding a validation for checking if the disk is indeed exists throu hmm, thought about it.. but why should we validate it instead of simply fail on execute? (i mean, like we don't call vdsm on canDo..) Line 37: } Line 38: Line 39: @Override Line 40: public void executeCommand() { Line 50: } Line 51: Line 52: @Override Line 53: public CommandCallback getCallback() { Line 54: return null; > I assume you implemented this, to override the method in AddCinderDiskComma yeah, since we need to return null to make sure it won't use CoCo. Line 55: } Line 56: Line 57: @Override Line 58: public AuditLogType getAuditLogTypeValue() { https://gerrit.ovirt.org/#/c/39652/2/backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/CinderDisksValidator.java File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/storage/CinderDisksValidator.java: Line 67: @Override Line 68: public ValidationResult call() { Line 69: for (CinderDisk disk : cinderDisks) { Line 70: OpenStackVolumeProviderProxy proxy = diskProxyMap.get(disk.getId()); Line 71: Volume volume = proxy.getVolumeById(disk.getId().toString()); > I'm not sure why do we need to get the volume from the proxy, isn't the fet Yeah, fetch from DB should be enough, done. Not sure about validating through proxy - set previous comment. Line 72: if (getDiskDao().get(disk.getId()) != null) { Line 73: return new ValidationResult(VdcBllMessages.CINDER_DISK_ALREADY_REGISTERED, Line 74: String.format("$diskAlias %s", volume.getName())); Line 75: } https://gerrit.ovirt.org/#/c/39652/2/backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties File backend/manager/modules/dal/src/main/resources/bundles/AuditLogMessages.properties: Line 57: USER_FAILED_REMOVE_DISK_SNAPSHOT=Failed to delete Disk '${DiskAlias}' from Snapshot(s) ${Snapshots} of VM ${VmName} (User: ${UserName}). Line 58: USER_REMOVE_DISK_SNAPSHOT_FINISHED_SUCCESS=Disk '${DiskAlias}' from Snapshot(s) '${Snapshots}' of VM '${VmName}' deletion has been completed (User: ${UserName}). Line 59: USER_REMOVE_DISK_SNAPSHOT_FINISHED_FAILURE=Failed to complete deletion of Disk '${DiskAlias}' from snapshot(s) '${Snapshots}' of VM '${VmName}' (User: ${UserName}). Line 60: USER_REGISTER_DISK_FINISHED_SUCCESS=Disk '${DiskAlias}' has been successfully registered as a floating disk. Line 61: USER_REGISTER_DISK_FINISHED_FAILURE=Failed to register Disk '${DiskAlias}'. > sadly, "DiskAlias" in the audit log is not case sensitive compatible with t :)) Line 62: USER_DETACH_USER_FROM_POOL=User ${AdUserName} was detached from VM Pool ${VmPoolName} by ${UserName}. Line 63: USER_DETACH_USER_FROM_POOL_FAILED=Failed to detach User ${AdUserName} from VM Pool ${VmPoolName} (User: ${UserName}). Line 64: USER_DETACH_USER_FROM_VM=User ${AdUserName} was detached from VM ${VmName} by ${UserName}. Line 65: USER_FAILED_DETACH_USER_FROM_VM=Failed to detach User ${AdUserName} from VM ${VmName} (User: ${UserName}). -- To view, visit https://gerrit.ovirt.org/39652 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iefb1ecf11d66039bbc4f5080b021b7393f27a0be Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <[email protected]> Gerrit-Reviewer: Daniel Erez <[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
