Maor Lipchuk has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 27:
(25 comments)
Almost finished all the review,looks good for now, good job.
please separate the patch for Rest, engine and GUI to ease the review.
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AbstractDiskVmCommand.java
Line 124: return true;
Line 125: }
Line 126:
Line 127: protected Disk loadDisk(Guid diskId) {
Line 128: if (getParameters().getSnapshotId() == null) {
just to make it more clear, I would re-factor this condition to a method and
call it isDiskSnapshot()
Line 129: return getDiskDao().get(diskId);
Line 130: } else {
Line 131: return
getDiskImageDao().getDiskSnapshotForVmSnapshot(diskId,
getParameters().getSnapshotId());
Line 132: }
Line 191: return false;
Line 192: }
Line 193:
Line 194: protected boolean isHotPlugOperationSupported() {
Line 195: if (getParameters().getSnapshotId() == null) {
same here
Line 196: return isHotPlugSupported();
Line 197: }
Line 198:
Line 199: return isHotPlugDiskSnapshotSupported();
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/HotPlugDiskToVmCommand.java
Line 74: protected void performDbLoads() {
Line 75: oldVmDevice =
Line 76: getVmDeviceDao().get(new
VmDeviceId(getParameters().getDiskId(), getVmId()));
Line 77: if (oldVmDevice != null) {
Line 78: if (oldVmDevice.getSnapshotId() != null) {
I would refactor this condition to a method called isDiskSnapshot()
Line 79: disk =
getDiskImageDao().getDiskSnapshotForVmSnapshot(getParameters().getDiskId(),
oldVmDevice.getSnapshotId());
Line 80: } else {
Line 81: disk = getDiskDao().get(getParameters().getDiskId());
Line 82: }
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/ImagesHandler.java
Line 452: }
Line 453: return result;
Line 454: }
Line 455:
Line 456: public static List<DiskImage> getPluggedDisksForVm(Guid vmId) {
Why changing images to disks, we are using only filtering for images here.
Line 457: return
filterImageDisks(DbFacade.getInstance().getDiskDao().getAllForVm(vmId, true),
true, false, true);
Line 458: }
Line 459:
Line 460: /**
Line 480: }
Line 481: }
Line 482:
Line 483: /**
Line 484: * Filter image disks by attributes, disk snapshots are always
filtered out.
Why disk snapshots are always filtered out, there is a boolean which decides it.
Line 485: *
Line 486: *
Line 487: * @param listOfDisks
Line 488: * - The list of disks to be filtered.
Line 489: * @param allowOnlyNotShareableDisks
Line 490: * - Indication whether to allow only disks that are
not shareable
Line 491: * @param allowOnlySnapableDisks
Line 492: * - Indication whether to allow only disks which are
allowed to be snapshoted.
Line 493: * @param allowOnlyActiveDisks
Please elaborate
Line 494: * @return - List filtered of disk images according to the given
filters excluding disk snapshots.
Line 495: */
Line 496: public static List<DiskImage> filterImageDisks(Collection<?
extends Disk> listOfDisks,
Line 497: boolean
allowOnlyNotShareableDisks,
Line 490: * - Indication whether to allow only disks that are
not shareable
Line 491: * @param allowOnlySnapableDisks
Line 492: * - Indication whether to allow only disks which are
allowed to be snapshoted.
Line 493: * @param allowOnlyActiveDisks
Line 494: * @return - List filtered of disk images according to the given
filters excluding disk snapshots.
why "excluding disk snaoshots"? it is dependent on the boolean.
Line 495: */
Line 496: public static List<DiskImage> filterImageDisks(Collection<?
extends Disk> listOfDisks,
Line 497: boolean
allowOnlyNotShareableDisks,
Line 498: boolean
allowOnlySnapableDisks,
Line 495: */
Line 496: public static List<DiskImage> filterImageDisks(Collection<?
extends Disk> listOfDisks,
Line 497: boolean
allowOnlyNotShareableDisks,
Line 498: boolean
allowOnlySnapableDisks,
Line 499: boolean
allowOnlyActiveDisks) {
Is this indentation is by the formatter?
Line 500: List<DiskImage> diskImages = new ArrayList<DiskImage>();
Line 501: for (Disk disk : listOfDisks) {
Line 502: if (disk.getDiskStorageType() == DiskStorageType.IMAGE &&
Line 503: (!allowOnlyNotShareableDisks ||
!disk.isShareable()) &&
Line 501: for (Disk disk : listOfDisks) {
Line 502: if (disk.getDiskStorageType() == DiskStorageType.IMAGE &&
Line 503: (!allowOnlyNotShareableDisks ||
!disk.isShareable()) &&
Line 504: (!allowOnlySnapableDisks ||
disk.isAllowSnapshot()) &&
Line 505: (!allowOnlyActiveDisks ||
Boolean.TRUE.equals(((DiskImage)disk).getActive()))) {
I would simply use ((DiskImage)disk).getActive()
instead of Boolean.TRUE.equals(((DiskImage)disk).getActive()), it much shorter
and nicer IMO
Also getActive is already reflects a primitive boolean so there is no risk of
null value.
Line 506: diskImages.add((DiskImage) disk);
Line 507: }
Line 508: }
Line 509: return diskImages;
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/config/ConfigValues.java
Line 1506: NetworkQosSupported(536),
Line 1507:
Line 1508: @TypeConverterAttribute(Boolean.class)
Line 1509: @DefaultValueAttribute("false")
Line 1510: HotPlugDiskSnapshotSupported(525),
crucial: The is here is already in use, please find another one
Line 1511:
Line 1512: Invalid(65535);
Line 1513:
Line 1514: private int intValue;
....................................................
File backend/manager/modules/dal/src/main/resources/bundles/AppErrors.properties
Line 136: VM_NOT_FOUND=VM not found
Line 137: ACTION_TYPE_FAILED_MISSED_STORAGES_FOR_SOME_DISKS=Cannot ${action}
${type}. One or more provided storage domains are in
maintenance/non-operational status.
Line 138: ACTION_TYPE_FAILED_STOARGE_DOMAIN_IS_WRONG=Cannot ${action} ${type}.
Provided wrong storage domain, which is not related to disk.
Line 139: ACTION_TYPE_FAILED_VM_IN_PREVIEW=Cannot ${action} ${type}. VM is
previewing a Snapshot.
Line 140: ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM=Cannot
${action} ${type}. The following VM disks snapshots are attached to another
VMs: ${disksInfo}. Please detach them from those VMs and try again.
Please see comments in the other appErrors.properties file
Line 141: 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.
Line 142: ACTION_TYPE_FAILED_DISKS_LOCKED=Cannot ${action} ${type}: The
following disks are locked: ${diskAliases}. Please try again in a few minutes.
Line 143: ACTION_TYPE_FAILED_DISKS_ILLEGAL=Cannot ${action} ${type}. The
following attached disks are in ILLEGAL status: ${diskAliases} - please remove
them and try again.
Line 144: ACTION_TYPE_FAILED_IMPORT_DISKS_ALREADY_EXIST=Cannot ${action}
${type}. The following disks already exist: ${diskAliases}. Please import as a
clone.
....................................................
File
backend/manager/modules/utils/src/main/java/org/ovirt/engine/core/utils/ovf/OvfReader.java
Line 249: if (node.SelectSingleNode(OvfProperties.VMD_SNAPSHOT_PROP,
_xmlNS) != null
Line 250: &&
StringUtils.isNotEmpty(node.SelectSingleNode(OvfProperties.VMD_SNAPSHOT_PROP,
_xmlNS).InnerText)) {
Line 251: vmDevice.setSnapshotId(new
Guid(String.valueOf(node.SelectSingleNode(OvfProperties.VMD_CUSTOM_PROP,
_xmlNS).InnerText)));
Line 252: } else {
Line 253: vmDevice.setSnapshotId(null);
Isn't the snapshotId will be null be default?
Line 254: }
Line 255:
Line 256: if (isManaged) {
Line 257: vmDevice.setIsManaged(true);
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/HotPlugDiskVDSCommand.java
Line 42:
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,
It will be more elegant to use as so:
property = disk.isShareable();
if ((vmDevice.getSnapshotId() != null && ....) {
property = VdsProperties.Transient
}
drive.put(VdsProperties.Shareable, VdsProperties.Transient)
Line 47: (vmDevice.getSnapshotId() != null &&
FeatureSupported.hotPlugDiskSnapshot(getParameters().getVm()
Line 48: .getVdsGroupCompatibilityVersion())) ?
VdsProperties.Transient
Line 49: : disk.isShareable());
Line 50: drive.put(VdsProperties.Optional, Boolean.FALSE.toString());
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VdsProperties.java
Line 239: public static final String ImageId = "imageID";
Line 240: public static final String VolumeId = "volumeID";
Line 241: public static final String Format = "format";
Line 242: public static final String Shareable = "shared";
Line 243: public static final String None = "none";
None is too general, I would use something more specific for vm device
Line 244: public static final String Exclusive = "exclusive";
Line 245: public static final String Shared = "shared";
Line 246: public static final String Transient = "transient";
Line 247: public static final String SpecParams = "specParams";
Line 241: public static final String Format = "format";
Line 242: public static final String Shareable = "shared";
Line 243: public static final String None = "none";
Line 244: public static final String Exclusive = "exclusive";
Line 245: public static final String Shared = "shared";
There is already Shareable property , why not using it?
Line 246: public static final String Transient = "transient";
Line 247: public static final String SpecParams = "specParams";
Line 248: public static final String Address = "address";
Line 249: public static final String Alias = "alias";
....................................................
File
backend/manager/modules/vdsbroker/src/main/java/org/ovirt/engine/core/vdsbroker/vdsbroker/VmInfoBuilder.java
Line 269
Line 270
Line 271
Line 272
Line 273
No need to change this
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 think it will be more elegant to use :
if ( (vmDevice.getSnapshotId() != null &&...) {
struct.put(VdsProperties.Shareable,VdsProperties.Transient)
else
struct.put(VdsProperties.Shareable,disk.isShareable())
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}. VM is previewing a
Snapshot.")
Line 347: String ACTION_TYPE_FAILED_VM_IN_PREVIEW();
Line 348:
Line 349: @DefaultStringValue("Cannot ${action} ${type}. The following VM
disks snapshots are attached to another VMs: ${disksInfo}. Please detach them
from those VMs and try again.")
See comments in appErrors.properties
Line 350: String
ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM();
Line 351:
Line 352: @DefaultStringValue("Cannot ${action} ${type}. The following VM
activated disks are disk snapshots: ${diskAliases}, please deactivate them and
try again.")
Line 353: String ACTION_TYPE_FAILED_VM_HAS_PLUGGED_DISK_SNAPSHOT();
....................................................
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 can only be detached
I think the comment should be changed here.
Line 413: if (disk.getNumberOfVms() > 1 &&
Line 414:
!(DiskStorageType.IMAGE.equals(disk.getDiskStorageType()) &&
((DiskImage)disk).isDiskSnapshot())) {
Line 415: model.getLatch().setIsChangable(false);
Line 416: }
....................................................
File
frontend/webadmin/modules/webadmin/src/main/resources/org/ovirt/engine/ui/frontend/AppErrors.properties
Line 133: VMT_CANNOT_UPDATE_ILLEGAL_FIELD=Failed updating the properties of the
VM template.
Line 134: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM=Cannot
remove Directory Group. Detach Directory Group from VM first.
Line 135: VM_NOT_FOUND=VM not found
Line 136: ACTION_TYPE_FAILED_VM_IN_PREVIEW=Cannot ${action} ${type}. VM is
previewing a Snapshot.
Line 137: ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM=Cannot
${action} ${type}. The following VM disks snapshots are attached to another
VMs: ${disksInfo}. Please detach them from those VMs and try again.
1. /s/attached to another VMs/already attached to other VMs
2. Use new line after $(disksInfo}.
Line 138: 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.
Line 139: ACTION_TYPE_FAILED_DISKS_LOCKED=Cannot ${action} ${type}: The
following disks are locked: ${diskAliases}. Please try again in a few minutes.
Line 140: ACTION_TYPE_FAILED_DISKS_ILLEGAL=Cannot ${action} ${type}. The
following attached disks are in ILLEGAL status: ${diskAliases} - please remove
them and try again.
Line 141: ACTION_TYPE_FAILED_IMPORT_DISKS_ALREADY_EXIST=Cannot ${action}
${type}. The following disks already exist: ${diskAliases}. Please import as a
clone.
Line 134: DIRECTORY_GROUP_CANNOT_REMOVE_DIRECTORY_GROUP_ATTACHED_TO_VM=Cannot
remove Directory Group. Detach Directory Group from VM first.
Line 135: VM_NOT_FOUND=VM not found
Line 136: ACTION_TYPE_FAILED_VM_IN_PREVIEW=Cannot ${action} ${type}. VM is
previewing a Snapshot.
Line 137: ACTION_TYPE_FAILED_VM_DISK_SNAPSHOT_IS_PLUGGED_TO_ANOTHER_VM=Cannot
${action} ${type}. The following VM disks snapshots are attached to another
VMs: ${disksInfo}. Please detach them from those VMs and try again.
Line 138: 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.
/s/snapshots: ${diskAliases}, please/snapshots: ${diskAliases}. please (Use dot
instead of comma)
Line 139: ACTION_TYPE_FAILED_DISKS_LOCKED=Cannot ${action} ${type}: The
following disks are locked: ${diskAliases}. Please try again in a few minutes.
Line 140: ACTION_TYPE_FAILED_DISKS_ILLEGAL=Cannot ${action} ${type}. The
following attached disks are in ILLEGAL status: ${diskAliases} - please remove
them and try again.
Line 141: ACTION_TYPE_FAILED_IMPORT_DISKS_ALREADY_EXIST=Cannot ${action}
${type}. The following disks already exist: ${diskAliases}. Please import as a
clone.
Line 142: ACTION_TYPE_FAILED_VM_IS_LOCKED=Cannot ${action} ${type}: VM is
locked. Please try again in a few minutes.
....................................................
File packaging/dbscripts/all_disks_sp.sql
Line 44: RETURN QUERY SELECT all_disks_including_snapshots.*
Line 45: FROM all_disks_including_snapshots
Line 46: LEFT JOIN vm_device ON vm_device.device_id =
all_disks_including_snapshots.image_group_id AND (NOT v_only_plugged OR
is_plugged)
Line 47: WHERE vm_device.vm_id = v_vm_guid
Line 48: AND ((vm_device.snapshot_id IS NULL AND
(all_disks_including_snapshots.active != FALSE OR
all_disks_including_snapshots.active IS NULL))
1. please switch : all_disks_including_snapshots.active != FALSE OR all_
disks_including_snapshots.active IS NULL
to all_disks_including_snapshots.active IS NOT false
2. When active can be null?
Line 49: OR vm_device.snapshot_id =
all_disks_including_snapshots.vm_snapshot_id)
Line 50: AND (NOT v_is_filtered OR EXISTS (SELECT 1
Line 51: FROM
user_disk_permissions_view
Line 52: WHERE user_id = v_user_id AND
entity_id = all_disks_including_snapshots.disk_id));
....................................................
File packaging/dbscripts/create_views.sql
Line 250: CREATE OR REPLACE VIEW all_disks
Line 251: AS
Line 252: SELECT *
Line 253: FROM all_disks_including_snapshots
Line 254: WHERE active IS NULL OR active = TRUE;
Please use indentation here
Line 255:
Line 256:
Line 257: CREATE OR REPLACE VIEW storage_domains
Line 258: AS
....................................................
File packaging/dbscripts/disk_images_sp.sql
Line 112: LANGUAGE plpgsql;
Line 113:
Line 114:
Line 115:
Line 116: Create or replace FUNCTION GetVmAttachedDiskSnapshots(v_vm_guid UUID,
v_is_plugged BOOLEAN)
Suggestion: The name of the query is misleading, we don't get a VM, we should
name it GetAttachedDiskSnapshotsToVM
Line 117: RETURNS SETOF images_storage_domain_view
Line 118: AS $procedure$
Line 119: BEGIN
Line 120: RETURN QUERY SELECT images_storage_domain_view.*
....................................................
File
packaging/dbscripts/upgrade/03_03_0800_add_snapshot_id_column_to_vm_device.sql
Line 1: select fn_db_add_column('vm_device', 'snapshot_id', 'UUID REFERENCES
SNAPSHOTS(snapshot_id) ON DELETE CASCADE DEFAULT NULL');
crucial: The sequential number is already in use, it should be updated, please
don't forget to change it before merging
--
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: 27
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: 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
