Alissa Bonas has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 6:
(7 comments)
partial review..
....................................................
Commit Message
Line 36:
Line 37: OPEN ISSUES:
Line 38: 1. UI - would be handled in a following patch (the
Line 39: information is accessible through REST)
Line 40: 2. REST issue with multiple parents of resource, as a temp soltuion -
this item is not very clear - what exact issue? what's the symptom and is it a
bug? and how temporary is the solution introduced in this patch?
Line 41: i've added the snapshot id info as a new tag "snapshot_id"
Line 42: 3. Permissions/Tests.
Line 43:
Line 44: Change-Id: I02579bf1a91cd294a5040acf432f1fdb87eb18c1
....................................................
File
backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AttachDiskToVmCommand.java
Line 76: }
Line 77:
Line 78: updateDisksFromDb();
Line 79: if ((!isOperationPerformedOnDiskSnapshot() &&
!isDiskCanBeAddedToVm(disk, getVm())) || !isDiskPassPciAndIdeLimit(disk)) {
Line 80: return false;
is there an error message set somewhere so user can understand what's wrong?
Line 81: }
Line 82:
Line 83: if (getVmDeviceDao().exists(new VmDeviceId(disk.getId(),
getVmId()))) {
Line 84: return
failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_DISK_ALREADY_ATTACHED);
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/VmDeviceDAODbFacadeImpl.java
Line 48: .addValue("is_readonly", entity.getIsReadOnly())
Line 49: .addValue("alias", entity.getAlias())
Line 50: .addValue("custom_properties",
Line 51:
SerializationFactory.getSerializer().serialize(entity.getCustomProperties()))
Line 52: .addValue("snapshot_id", entity.getSnapshotId());
is it possible this property is empty? what is the default value? (especially
for entities that exist in db since previous versions).
Line 53: }
Line 54:
Line 55: @Override
Line 56: protected RowMapper<VmDevice> createEntityRowMapper() {
Line 143: vmDevice.setIsReadOnly(rs.getBoolean("is_readonly"));
Line 144: vmDevice.setAlias(rs.getString("alias"));
Line 145:
vmDevice.setCustomProperties(SerializationFactory.getDeserializer()
Line 146:
.deserializeOrCreateNew(rs.getString("custom_properties"),
LinkedHashMap.class));
Line 147: vmDevice.setSnapshotId(getGuid(rs, "snapshot_id"));
same comment as above - won't this fail from older entities?
Line 148: return vmDevice;
Line 149: }
Line 150: }
Line 151:
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 2661: <xs:element ref="quota" minOccurs="0" maxOccurs="1"/>
Line 2662: <xs:element name="lun_storage" type="Storage"
minOccurs="0"/>
Line 2663: <xs:element name="sgio" type="xs:string" minOccurs="0"/>
Line 2664: <xs:element ref="snapshot" minOccurs="0" maxOccurs="1"/>
Line 2665: <xs:element name="snapshot_id" type="xs:string"
minOccurs="0" maxOccurs="1"/>
why is the snapshot id next to the snapshot? I mean - why isn't it
hierarchically contained in the snapshot object?
What happens in the currently proposed structure if you have a snapshot
element, but not snapshot_id?
Line 2666: </xs:sequence>
Line 2667: </xs:extension>
Line 2668: </xs:complexContent>
Line 2669: </xs:complexType>
....................................................
File
frontend/webadmin/modules/frontend/src/main/java/org/ovirt/engine/ui/frontend/AppErrors.java
Line 337: @DefaultStringValue("Cannot ${action} ${type}. VM is previewing a
Snapshot.")
Line 338: String ACTION_TYPE_FAILED_VM_IN_PREVIEW();
Line 339:
Line 340: @DefaultStringValue("Cannot ${action} ${type}. The following VM
activated disks are snapshots: ${diskAliases}, please deactivate them and try
again.")
Line 341: String ACTION_TYPE_FAILED_VM_HAS_PLUGGED_DISK_SNAPSHOT();
in which command is this error used? I didn't see it in any of the commands in
this patch (perhaps missed it?)
Line 342:
Line 343: @DefaultStringValue("Cannot ${action} ${type}: The following
disks are locked: ${diskAliases}. Please try again in a few minutes.")
Line 344: String ACTION_TYPE_FAILED_DISKS_LOCKED();
Line 345:
....................................................
File packaging/dbscripts/disk_images_sp.sql
Line 131: RETURNS SETOF images_storage_domain_view
Line 132: AS $procedure$
Line 133: BEGIN
Line 134: RETURN QUERY SELECT *
Line 135: FROM images_storage_domain_view images_storage_domain_view
no need to write the name of the view twice. it's meaningless.
Line 136: WHERE image_group_id = v_image_group_id
Line 137: AND vm_snapshot_id = v_vm_snapshot_id;
Line 138: END; $procedure$
Line 139: LANGUAGE plpgsql;
--
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: 6
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: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
