Maor Lipchuk has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 31:
(11 comments)
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/RestoreAllSnapshotsCommand.java
Line 370: if (!validate(storageValidator.allDomainsExistAndActive()) ||
Line 371:
!validate(storageValidator.allDomainsWithinThresholds()) ||
Line 372: !performImagesChecks() ||
Line 373: !validate(vmValidator.vmDown()) ||
Line 374: // if the user just previewed a snapshot and selected
"undo" - the vm can have snapshots attached to other vms.
I would consider to extract this to another method instead put comment in the
middle of the if condition
Line 375: (targetSnapshot.getType() != SnapshotType.PREVIEW &&
!validate(vmValidator.vmNotHavingDeviceSnapshotsAttachedToOtherVms()))) {
Line 376: return false;
Line 377: }
Line 378:
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java
Line 51: implements QuotaStorageDependent {
Line 52:
Line 53: private List<PermissionSubject> listPermissionSubjects;
Line 54: private Map<Guid, List<Disk>> otherVmDisks = new HashMap<Guid,
List<Disk>>();
Line 55: /* vms that the disk (active image) is plugged to */
I'm quite sure this is not a java doc.
Java doc should start with /**
Line 56: private List<VM> vmsDiskPluggedTo;
Line 57: /* vms that a snapshot of the disk is plugged to. */
Line 58: private List<VM> vmsDiskSnapshotPluggedTo;
Line 59: /* vms that a disk or it's sansphot is plugged to. */
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java
Line 43: drive.put(VdsProperties.Type, VmDeviceType.DISK.getName());
Line 44: addAddress(drive, getParameters().getVmDevice().getAddress());
Line 45: drive.put(VdsProperties.INTERFACE,
disk.getDiskInterface().getName());
Line 46: drive.put(VdsProperties.Shareable,
Line 47: (vmDevice.getSnapshotId() != null &&
FeatureSupported.hotPlugDiskSnapshot(getParameters().getVm()
At least re-extend to another method, this will be hard to debug
Line 48: .getVdsGroupCompatibilityVersion())) ?
VdsProperties.Transient
Line 49: : disk.isShareable());
Line 50: drive.put(VdsProperties.Optional, Boolean.FALSE.toString());
Line 51: drive.put(VdsProperties.ReadOnly,
String.valueOf(vmDevice.getIsReadOnly()));
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java
Line 240:
Line 241: @Override
Line 242: protected void buildVmDrives() {
Line 243:
Line 244: boolean bootDiskFound = false;
Suggestion: /s/bootDiskFound/isBootDiskFound
Line 245: List<Disk> disks = getSortedDisks();
Line 246: for (Disk disk : disks) {
Line 247: Map<String, Object> struct = new HashMap<String,
Object>();
Line 248: // get vm device for this disk from DB
Line 301:
Line 302: addBootOrder(vmDevice, struct);
Line 303: struct.put(VdsProperties.Shareable,
Line 304: (vmDevice.getSnapshotId() != null &&
FeatureSupported.hotPlugDiskSnapshot(vm.getVdsGroupCompatibilityVersion())) ?
VdsProperties.Transient
Line 305: : disk.isShareable());
I would much appreciate if you will re-consider, and at least extract all this
horrible logic to another method.
This is un-debugable
Line 306: struct.put(VdsProperties.Optional,
Boolean.FALSE.toString());
Line 307: struct.put(VdsProperties.ReadOnly,
String.valueOf(vmDevice.getIsReadOnly()));
Line 308: struct.put(VdsProperties.SpecParams,
vmDevice.getSpecParams());
Line 309: struct.put(VdsProperties.DeviceId,
String.valueOf(vmDevice.getId().getDeviceId()));
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 345:
Line 346: @DefaultStringValue("Cannot ${action} ${type}. The following VM
disks snapshots are attached to other VMs: ${disksInfo}. Please detach them
from those VMs and try again.")
Line 347: String
ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM();
Line 348:
Line 349: @DefaultStringValue("Cannot ${action} ${type}. The following VM
activated disks are disk snapshots: ${diskAliases}. Please deactivate them and
try again.")
See my comment at AppErrors.properties regarding changing to VM's Activated
disks
Line 350: String ACTION_TYPE_FAILED_VM_HAS_PLUGGED_DISK_SNAPSHOT();
Line 351:
Line 352: @DefaultStringValue("Cannot ${action} ${type}: The following
disks are locked: ${diskAliases}. Please try again in a few minutes.")
Line 353: String ACTION_TYPE_FAILED_DISKS_LOCKED();
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/EditDiskModel.java
Line 49: setVolumeFormat(diskImage.getVolumeFormat());
Line 50:
Line 51: boolean isExtendImageSizeEnabled = getVm() != null &&
!diskImage.isDiskSnapshot() &&
Line 52: VdcActionUtils.canExecute(Arrays.asList(getVm()),
VM.class, VdcActionType.ExtendImageSize);
Line 53:
* Please consider to re-extend this to a method, now it is way not readable
then before.
* new line is not needed here
Line 54: getSizeExtend().setIsChangable(isExtendImageSizeEnabled);
Line 55:
Line 56: Guid storageDomainId = diskImage.getStorageIds().get(0);
Line 57: AsyncDataProvider.getStorageDomainById(new
AsyncQuery(this, new INewAsyncCallback() {
....................................................
File
frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/VmDiskListModel.java
Line 408: diskModel.setVm(getEntity());
Line 409:
Line 410: items.add(diskModel);
Line 411:
Line 412: // A shared disk/disk snapshot can only be detached
disk/disk is confusing,
What about the following :
A shared disk or a disk snapshot can only be detached
Line 413: if (disk.getNumberOfVms() > 1) {
Line 414: model.getLatch().setIsChangable(false);
Line 415: }
Line 416: }
....................................................
File
frontend/webadmin/modules/userportal-gwtp/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 129: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM=Cannot
remove Directory Group. Detach Directory Group from VM first.
Line 130: VM_NOT_FOUND=VM not found
Line 131: ACTION_TYPE_FAILED_VM_IN_PREVIEW=Cannot ${action} ${type}. VM is
previewing a Snapshot.
Line 132: ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM=Cannot
${action} ${type}. The following VM disks snapshots are attached to other VMs:
${disksInfo}. Please detach them from those VMs and try again.
Line 133: ACTION_TYPE_FAILED_VM_HAS_PLUGGED_DISK_SNAPSHOT=Cannot ${action}
${type}. The following VM activated disks are disk snapshots: ${diskAliases}.
Please deactivate them and try again.
See comment in other AppErrors file : The following VM's activate d disks
Line 134: ACTION_TYPE_FAILED_DISKS_LOCKED=Cannot ${action} ${type}: The
following disks are locked: ${diskAliases}. Please try again in a few minutes.
Line 135: ACTION_TYPE_FAILED_DISKS_ILLEGAL=Cannot ${action} ${type}. The
following attached disks are in ILLEGAL status: ${diskAliases} - please remove
them and try again.
Line 136: ACTION_TYPE_FAILED_MOVE_DISKS_MIXED_PLUGGED_STATUS=Cannot ${action}
${type}. The following disks could not be moved: ${diskAliases}. Please make
sure that all disks are active or inactive in the VM.
Line 137: ACTION_TYPE_FAILED_IMPORT_DISKS_ALREADY_EXIST=Cannot ${action}
${type}. The following disks already exist: ${diskAliases}. Please import as a
clone.
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 133: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM=Cannot
remove Directory Group. Detach Directory Group from VM first.
Line 134: VM_NOT_FOUND=VM not found
Line 135: ACTION_TYPE_FAILED_VM_IN_PREVIEW=Cannot ${action} ${type}. VM is
previewing a Snapshot.
Line 136: ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM=Cannot
${action} ${type}. The following VM disks snapshots are attached to other VMs:
${disksInfo}. Please detach them from those VMs and try again.
Line 137: ACTION_TYPE_FAILED_VM_HAS_PLUGGED_DISK_SNAPSHOT=Cannot ${action}
${type}. The following VM activated disks are disk snapshots: ${diskAliases}.
Please deactivate them and try again.
Missed that one, but It should be : The following VM's activated disks...
instead of The following VM activated disks....
Line 138: ACTION_TYPE_FAILED_DISKS_LOCKED=Cannot ${action} ${type}: The
following disks are locked: ${diskAliases}. Please try again in a few minutes.
Line 139: ACTION_TYPE_FAILED_DISKS_ILLEGAL=Cannot ${action} ${type}. The
following attached disks are in ILLEGAL status: ${diskAliases} - please remove
them and try again.
Line 140: ACTION_TYPE_FAILED_MOVE_DISKS_MIXED_PLUGGED_STATUS=Cannot ${action}
${type}. The following disks could not be moved: ${diskAliases}. Please make
sure that all disks are active or inactive in the VM.
Line 141: ACTION_TYPE_FAILED_IMPORT_DISKS_ALREADY_EXIST=Cannot ${action}
${type}. The following disks already exist: ${diskAliases}. Please import as a
clone.
Line 222: ACTION_TYPE_FAILED_NO_VDS_IN_POOL=Cannot ${action} ${type}. There is
no active Host in the Data Center.
Line 223: ACTION_TYPE_FAILED_REQUESTED_DISK_SIZE_IS_TOO_SMALL=Cannot ${action}
${type}. New disk size must be larger than current disk size.
Line 224: ACTION_TYPE_FAILED_CLOUD_INIT_IS_NOT_SUPPORTED=Cannot ${action}
${type}. Cloud-Init is only supported on cluster compatibility version 3.3 and
higher.
Line 225: ACTION_TYPE_FAILED_CANNOT_RESIZE_DISK_SNAPSHOT=Cannot ${action}
${type}. Disk snapshot cannot be resized.
Line 226: >>>>>>> core, restapi: Introducing support for attaching disk snapshot
Wrong
Line 227: VAR__TYPE__HOST=$type Host
Line 228: VAR__ENTITIES__HOSTS=$entities hosts
Line 229: VAR__TYPE__NETWORK=$type Network
Line 230: VAR__TYPE__NETWORKS=$type Networks
--
To view, visit http://gerrit.ovirt.org/17679
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I02579bf1a91cd294a5040acf432f1fdb87eb18c1
Gerrit-PatchSet: 31
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Liron Ar <lara...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Ayal Baron <aba...@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: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Sergey Gotliv <sgot...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
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
