Allon Mureinik has posted comments on this change.
Change subject: backend,webadmin: Enable virtual drive size extension
......................................................................
Patch Set 9: (21 inline comments)
This patch is really hard to review - it combines both (good!) refactors and
new logic.
Is there any chance of separating 'em?
Also, see inline.
....................................................
File backend/manager/dbscripts/images_sp.sql
Line 96: AS $procedure$
Line 97: BEGIN
Line 98: UPDATE images
Line 99: SET size = v_size,
Line 100: lastModified = v_lastModified
_update_date should probably also be set, see UpdateImage.
Line 101: WHERE image_group_id = v_image_group_id;
Line 102: END; $procedure$
Line 103: LANGUAGE plpgsql;
Line 104:
....................................................
File backend/manager/dbscripts/upgrade/pre_upgrade/0000_config.sql
Line 180: select
fn_db_add_config_value('MultipleGatewaysSupported','true','3.3');
Line 181: select fn_db_add_config_value('ExtendImageSize','false','3.0');
Line 182: select fn_db_add_config_value('ExtendImageSize','false','3.1');
Line 183: select fn_db_add_config_value('ExtendImageSize','false','3.2');
Line 184: select fn_db_add_config_value('ExtendImageSize','true','3.3');
why do we need this as well as the action map entry?
Line 185:
Line 186: -- by default use no proxy
Line 187: select fn_db_add_config_value('SpiceProxyDefault','','general');
Line 188:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java
Line 46: }
Line 47:
Line 48: @Override
Line 49: protected void endWithFailure() {
Line 50: super.endWithFailure(); //To change body of overridden
methods use File | Settings | File Templates.
why do you need this?
Line 51: }
Line 52:
Line 53: protected void performPlugCommand(VDSCommandType commandType,
Line 54: Disk disk, VmDevice vmDevice) {
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddDiskCommand.java
Line 357:
Line 358: @Override
Line 359: protected void endWithFailure() {
Line 360: super.endWithFailure(); //To change body of overridden
methods use File | Settings | File Templates.
Line 361: }
why do you need this?
Line 362:
Line 363: @Override
Line 364: protected void endSuccessfully() {
Line 365: super.endSuccessfully(); //To change body of overridden
methods use File | Settings | File Templates.
Line 362:
Line 363: @Override
Line 364: protected void endSuccessfully() {
Line 365: super.endSuccessfully(); //To change body of overridden
methods use File | Settings | File Templates.
Line 366: }
why do you need this?
Line 367:
Line 368: private void createDiskBasedOnImage() {
Line 369: if(!getParameters().getDiskInfo().isWipeAfterDeleteSet()) {
Line 370: StorageType storageType =
getStorageDomain().getStorageType();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 44: implements QuotaStorageDependent {
Line 45:
Line 46: private List<PermissionSubject> listPermissionSubjects;
Line 47: private Map<Guid, List<Disk>> otherVmDisks = new HashMap<Guid,
List<Disk>>();
Line 48: private List<VM> vmsDiskPluggedTo;
The name sounds a bit funny.
How about vmsPluggedToDisk? (and in the getter too)
Line 49: private Disk oldDisk;
Line 50:
Line 51: public UpdateVmDiskCommand(T parameters) {
Line 52: super(parameters);
Line 76: }
Line 77: }
Line 78: if (shouldResizeDiskImage()) {
Line 79: exclusiveLock.put(getOldDisk().getId().toString(),
Line 80:
LockMessagesMatchUtil.makeLockingPair(LockingGroup.DISK,
VdcBllMessages.ACTION_TYPE_FAILED_DISKS_LOCKED));
If you're editing a bootable disk, you'll lock it both as shared and exclusive.
Is this intentional?
Line 81: }
Line 82: return exclusiveLock.isEmpty() ? null : exclusiveLock;
Line 83: }
Line 84:
Line 424: }
Line 425:
Line 426: private boolean shouldResizeDiskImage() {
Line 427: return getNewDisk().getDiskStorageType() ==
DiskStorageType.IMAGE &&
Line 428: getNewDisk().getSize() != getOldDisk().getSize();
what about failing if you try to resize a direct LUN?
Line 429: }
Line 430:
Line 431: private boolean shouldUpdatePropertiesOtherThanSize() {
Line 432: return shouldUpdateDiskProperties() ||
shouldUpdateImageProperties();
Line 438: getOldDisk().getPropagateErrors().getValue() !=
getNewDisk().getPropagateErrors().getValue() ||
Line 439: getOldDisk().isWipeAfterDelete() !=
getNewDisk().isWipeAfterDelete() ||
Line 440: getOldDisk().isShareable() !=
getNewDisk().isShareable() ||
Line 441:
!StringUtils.equalsIgnoreCase(getOldDisk().getDiskDescription(),
getNewDisk().getDiskDescription()) ||
Line 442:
!StringUtils.equalsIgnoreCase(getOldDisk().getDiskAlias(),
getNewDisk().getDiskAlias());
why ignore case?
Line 443: }
Line 444:
Line 445: private boolean shouldUpdateImageProperties() {
Line 446: return (getOldDisk().getDiskStorageType() ==
DiskStorageType.IMAGE) &&
....................................................
File
backend/manager/modules/bll/src/test/java/org/ovirt/engine/core/bll/UpdateVmDiskCommandTest.java
Line 51: import static org.mockito.Mockito.doReturn;
Line 52: import static org.mockito.Mockito.mock;
Line 53: import static org.mockito.Mockito.spy;
Line 54: import static org.mockito.Mockito.when;
Line 55: import static org.ovirt.engine.core.utils.MockConfigRule.mockConfig;
Please fix your auto-formatter - the order was OK beforehand...
Line 56:
Line 57: @RunWith(MockitoJUnitRunner.class)
Line 58: public class UpdateVmDiskCommandTest {
Line 59:
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/ImageDao.java
Line 8: /**
Line 9: * <code>ImageDAO</code> defines a type for performing CRUD operations
on instances of {@link Image}.
Line 10: */
Line 11: public interface ImageDao extends GenericDao<Image, Guid>,
StatusAwareDao<Guid, ImageStatus> {
Line 12: public void updateQuotaForImageAndSnapshots(Guid imageGroupId,
NGuid quotaId);
why is this related to the patch?
Line 13:
Line 14: public void updateImageVmSnapshotId(Guid id, Guid vmSnapshotId);
....................................................
File
backend/manager/modules/dal/src/main/resources/bundles/VdsmErrors.properties
Line 8: ExportError=Error exporting VM
Line 9: GeneralException=General Exception
Line 10: HostIdMismatch=Host id not found or does not match manager host id
Line 11: ImageDeleteError=Could not remove all files
Line 12: ImageDoesNotExistInDomainError=Image does not exist in domain
what's the deal with adding all the \r ?
Line 13: ImageIsNotEmpty=Image is not empty
Line 14: ImageMissingFromVm=Image missing from VM
Line 15: ImagePathError=Image path does not exist or cannot be accessed/created
Line 16: ImagesActionError=Error images action
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/ExtendVmDiskSizeVDSCommand.java
Line 19: );
Line 20:
Line 21: ProceedProxyReturnValue();
Line 22:
Line 23: if(getVDSReturnValue().getSucceeded()) {
formatting
Line 24: setReturnValue(result.getImageSize());
Line 25: }
Line 26: }
Line 27:
....................................................
Commit Message
Line 5: CommitDate: 2013-07-07 17:32:08 +0300
Line 6:
Line 7: backend,webadmin: Enable virtual drive size extension
Line 8:
Line 9: This patch provides the user the ability to extend virtual
s/virtual/a virtual/
Line 10: disk size online (when VM is running or paused) and offline
Line 11: (when VM is down or suspended).
Line 12:
Line 13: This functionality is exposed through the REST API and UI:
Line 6:
Line 7: backend,webadmin: Enable virtual drive size extension
Line 8:
Line 9: This patch provides the user the ability to extend virtual
Line 10: disk size online (when VM is running or paused) and offline
are you sure qemu supports extending when the VM is paused?
Line 11: (when VM is down or suspended).
Line 12:
Line 13: This functionality is exposed through the REST API and UI:
Line 14: in UI -
Line 11: (when VM is down or suspended).
Line 12:
Line 13: This functionality is exposed through the REST API and UI:
Line 14: in UI -
Line 15: go to "Virtual Machines" tab and pickup VM
s/pickup/choose/
Line 16: go to the "Disks" sub tab and pickup the disk
Line 17: click on "Edit" and insert value into "Extend size by(GB)" input
Line 18: field
Line 19:
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/AbstractDiskModel.java
Line 224: }
Line 225:
Line 226: public AbstractDiskModel() {
Line 227: setSizeExtend(new EntityModel());
Line 228: getSizeExtend().setEntity(String.valueOf(0));
why not just "0" ?
Line 229:
Line 230: setIsAttachDisk(new EntityModel());
Line 231: getIsAttachDisk().setEntity(false);
Line 232: getIsAttachDisk().getEntityChangedEvent().addListener(this);
Line 785: }
Line 786: }
Line 787:
Line 788: protected boolean isVmDown() {
Line 789: return getVm() != null && getVm().getStatus() ==
VMStatus.Down;
don't you mean to use getVm().getStatus().isDownOrHibernating()?
Line 790: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/EditDiskModel.java
Line 152: }
Line 153:
Line 154: private boolean isExtendImageSizeAllowed() {
Line 155: return (Boolean) AsyncDataProvider.getConfigValuePreConverted(
Line 156: ConfigurationValues.ExtendImageSize,
getVm().getVdsGroupCompatibilityVersion().toString()) &&
can't this be done with the action map?
Line 157: VdcActionUtils.CanExecute(Arrays.asList(getVm()), VM.class,
VdcActionType.ExtendImageSize);
Line 158: }
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmDiskListModel.java
Line 724:
Line 725: protected void updateExtendImageSizeEnabled() {
Line 726: isExtendImageSizeEnabled = (Boolean)
AsyncDataProvider.getConfigValuePreConverted(
Line 727: ConfigurationValues.ExtendImageSize,
getEntity().getVdsGroupCompatibilityVersion().toString()) &&
Line 728: VdcActionUtils.CanExecute(Arrays.asList(getEntity()),
VM.class, VdcActionType.ExtendImageSize);
here too - can't the action map be used?
Line 729: }
Line 730:
Line 731: @Override
Line 732: protected String getListName() {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/validation/NonNegativeLongNumberValidation.java
Line 1: package org.ovirt.engine.ui.uicommonweb.validation;
Line 2:
Line 3: import org.ovirt.engine.ui.uicompat.ConstantsManager;
Line 4:
Line 5: public class NonNegativeLongNumberValidation implements IValidation {
Can this be added as a @Valid annotation on the command's parameter class
too/instead?
Line 6:
Line 7: @Override
Line 8: public ValidationResult validate(Object value) {
Line 9: Long longValue = null;
--
To view, visit http://gerrit.ovirt.org/14975
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ie702348a68a26ac02a01f66aaa1ea42c2c675ebb
Gerrit-PatchSet: 9
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: A Burden <abur...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Cheryn Tan <cheryn...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Maor Lipchuk <mlipc...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
