Maor Lipchuk has posted comments on this change. Change subject: core: support Cinder disk update ......................................................................
Patch Set 5: (5 comments) https://gerrit.ovirt.org/#/c/39201/5/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 534: Future<VdcReturnValueBase> future = CommandCoordinatorUtil.executeAsyncCommand( Line 535: VdcActionType.ExtendCinderDisk, Line 536: new UpdateVmDiskParameters(getVmId(), newCinderDisk.getId(), newCinderDisk), Line 537: cloneContextAndDetachFromParent()); Line 538: addCustomValue("NewSize", String.valueOf(getNewDiskSizeInGB())); Why do we add this? Line 539: try { Line 540: setReturnValue(future.get()); Line 541: setSucceeded(getReturnValue().getSucceeded()); Line 542: } catch (InterruptedException | ExecutionException e) { Line 621: } Line 622: Line 623: public long getNewDiskSizeInGB() { Line 624: CinderDisk cinderDisk = (CinderDisk) getNewDisk(); Line 625: return cinderDisk.getSize() / (1024 * 1024 * 1024); Please use BYTES_IN_GB Line 626: } Line 627: Line 628: private boolean isDiskImage() { Line 629: return getOldDisk() != null && getNewDisk() != null && DiskStorageType.IMAGE == getOldDisk().getDiskStorageType(); Line 625: return cinderDisk.getSize() / (1024 * 1024 * 1024); Line 626: } Line 627: Line 628: private boolean isDiskImage() { Line 629: return getOldDisk() != null && getNewDisk() != null && DiskStorageType.IMAGE == getOldDisk().getDiskStorageType(); Suggestion: Please change this method to be called isDiskType(DiskStorageType diskStorageType) and use it also for Cinder Line 630: } Line 631: Line 632: private boolean isCinderDisk() { Line 633: return getOldDisk() != null && getNewDisk() != null && DiskStorageType.CINDER == getOldDisk().getDiskStorageType(); Line 695: switch (getNewDisk().getDiskStorageType()) { Line 696: case IMAGE: Line 697: return sizeChanged && vmDeviceForVm.getSnapshotId() == null; Line 698: case CINDER: Line 699: return sizeChanged; Why don't we need to check here also if the snapshot id is null? Line 700: } Line 701: return false; Line 702: } Line 703: https://gerrit.ovirt.org/#/c/39201/5/backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/StorageDomainValidatorFreeSpaceTest.java File backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/validator/storage/StorageDomainValidatorFreeSpaceTest.java: Line 43: public static Collection<Object[]> createParams() { Line 44: List<Object[]> params = new ArrayList<>(); Line 45: Line 46: for (StorageType storageType : StorageType.values()) { Line 47: if (storageType.isConcreteStorageType() && !storageType.isCinderDomain()) { How difficult it would be to fix this test? or do we want to add a different test in the future for Cinder? Line 48: List<VolumeType> volumeTypes = Line 49: storageType.isFileDomain() ? Arrays.asList(VolumeType.Preallocated, VolumeType.Sparse) Line 50: : Collections.singletonList(VolumeType.Preallocated); Line 51: for (VolumeType volumeType : volumeTypes) { -- To view, visit https://gerrit.ovirt.org/39201 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I16d4ce9f53569ddfe1984b29dcbe1cdda4f7b444 Gerrit-PatchSet: 5 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
