Maor Lipchuk 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 through the proxy, but that should be in another, separate validation. 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 AddCinderDiskCommand. Are you sure that we have to override this even if we don't use the coco infrastructure here? 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 fetch from the DB is enough? Also we should check if the disk is indeed exists through the proxy, but that should be in another, separate validation. 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 the "diskalias" in the validation messages (not your problem, it's just make me sad) 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: 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
