Michael Pasternak has posted comments on this change.
Change subject: core, restapi: Introducing support for attaching disk snapshot
......................................................................
Patch Set 31: Code-Review-1
(12 comments)
....................................................
File
backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/LinkHelper.java
Line 693: public static <R extends BaseResource> R addLinks(UriInfo
uriInfo, R model, Class<? extends BaseResource> suggestedParentType, boolean
addActions) {
Line 694: setHref(uriInfo, model, suggestedParentType);
Line 695: if (addActions) {
Line 696: setActions(uriInfo, model, suggestedParentType);
Line 697: }
can you please explain why this change is needed?
Line 698:
Line 699: for (BaseResource inline : getInlineResources(model)) {
Line 700: if (inline.getId() != null) {
Line 701: setHref(uriInfo, inline);
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 534: disk.size: xs:int #deprecated, replaced by
'provisioned_size'
Line 535: disk.sparse: xs:boolean
Line 536: disk.bootable: xs:boolean
Line 537: disk.shareable: xs:boolean
Line 538: disk.snapshot.id: xs:string
i'd expect it to be in own signature (this one for create-disk-from-params)
Line 539: disk.propagate_errors: xs:boolean
Line 540: disk.wipe_after_delete: xs:boolean
Line 541: disk.storage_domains.storage_domain--COLLECTION:
{storage_domain.id|name: 'xs:string'}
Line 542: description: add a new disk to the virtual machine allocating
space from the storage domain
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotDisksResource.java
Line 23: for (DiskImage disk : vm.getDiskList()) {
Line 24: Disk d = DiskMapper.map(disk, null);
Line 25: d.setSnapshot(new Snapshot());
Line 26: d.getSnapshot().setId(parent.id);
Line 27: disks.getDisks().add(d);
if BE disk has snapshot-id it belongs to, move this code to DiskMapper.map,
otherwise add new (local) method map() and move it there
Line 28: }
Line 29: }
Line 30: return disks;
Line 31: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotsResource.java
Line 138:
Line 139: if (queryReturnValue.getSucceeded() &&
queryReturnValue.getReturnValue() != null) {
Line 140: org.ovirt.engine.core.common.businessentities.Snapshot
snapshot = queryReturnValue.getReturnValue();
Line 141: if (snapshot.getVmConfiguration() != null) {
Line 142: return
SnapshotMapper.mapSnapshotConfiguration(snapshot.getVmConfiguration(),
please rename this method to map()
Line 143: ConfigurationType.OVF,
Line 144: model);
Line 145: }
Line 146: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDisksResource.java
Line 120: super.addLinks(model, subCollectionMembersToExclude);
Line 121: if (snapshotInfo != null) {
Line 122: VdcQueryReturnValue queryReturnValue =
Line 123: runQuery(VdcQueryType.GetSnapshotBySnapshotId,
Line 124: new
IdQueryParameters(Guid.createGuidFromString(snapshotInfo.getId())));
please use asGuid(), it creates appropriate error/response when string is not
convertible to UUID
Line 125: if (queryReturnValue.getSucceeded() &&
queryReturnValue.getReturnValue() != null) {
Line 126:
org.ovirt.engine.core.common.businessentities.Snapshot snapshot =
queryReturnValue.getReturnValue();
Line 127: VM vm = new VM();
Line 128: vm.setId(snapshot.getVmId().toString());
Line 124: new
IdQueryParameters(Guid.createGuidFromString(snapshotInfo.getId())));
Line 125: if (queryReturnValue.getSucceeded() &&
queryReturnValue.getReturnValue() != null) {
Line 126:
org.ovirt.engine.core.common.businessentities.Snapshot snapshot =
queryReturnValue.getReturnValue();
Line 127: VM vm = new VM();
Line 128: vm.setId(snapshot.getVmId().toString());
i'm not follow, why to run VdcQueryType.GetSnapshotBySnapshotId? just to fetch
the VmId? you have it in the context already, it's called parentId
Line 129: snapshotInfo.setVm(vm);
Line 130: model.setSnapshot(snapshotInfo);
Line 131: }
Line 132: LinkHelper.addLinks(getUriInfo(), snapshotInfo, null,
false);
Line 211: new AttachDettachVmDiskParameters(parentId,
Guid.createGuidFromStringDefaultEmpty(disk.getId()), isDiskActive);
Line 212:
Line 213: if (disk.isSetSnapshot()) {
Line 214: validateParameters(disk, "snapshot.id");
Line 215:
params.setSnapshotId(Guid.createGuidFromString(disk.getSnapshot().getId()));
please use asGuid() method instead, it throws appropriate error/response when
string is not convertible to UUID
Line 216: }
Line 217:
Line 218: return performAction(VdcActionType.AttachDiskToVm, params);
Line 219: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotResourceTest.java
Line 62: populates.add("true");
Line 63: String ovfData = "data";
Line 64: org.ovirt.engine.core.common.businessentities.Snapshot
resultSnapshot = new org.ovirt.engine.core.common.businessentities.Snapshot();
Line 65: resultSnapshot.setVmConfiguration(ovfData);
Line 66: resultSnapshot.setId(SNAPSHOT_ID);
please do this in own/dedicated test, this one should pass as is.
Line 67:
expect(httpHeaders.getRequestHeader(BackendResource.POPULATE)).andReturn(populates).anyTimes();
Line 68: setUriInfo(setUpBasicUriExpectations());
Line 69: setUpGetEntityExpectations(asList(getEntity(1)));
Line 70:
setUpEntityQueryExpectations(VdcQueryType.GetSnapshotBySnapshotId,
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendSnapshotsResourceTest.java
Line 131: resultSnapshot0.setVmConfiguration(ovfData);
Line 132: resultSnapshot0.setId(SNAPSHOT_IDS[0]);
Line 133: org.ovirt.engine.core.common.businessentities.Snapshot
resultSnapshot1 = new org.ovirt.engine.core.common.businessentities.Snapshot();
Line 134: resultSnapshot1.setVmConfiguration(ovfData);
Line 135: resultSnapshot1.setId(SNAPSHOT_IDS[1]);
please do this in own/dedicated test, this one should pass as is
Line 136:
expect(httpHeaders.getRequestHeader(BackendResource.POPULATE)).andReturn(populates).anyTimes();
Line 137: UriInfo uriInfo = setUpUriExpectations(null);
Line 138: setUriInfo(setUpBasicUriExpectations());
Line 139: setUpGetEntityExpectations(1);
Line 186: setUriInfo(setUpBasicUriExpectations());
Line 187: String ovfData = "data";
Line 188: org.ovirt.engine.core.common.businessentities.Snapshot
resultSnapshot0 = new org.ovirt.engine.core.common.businessentities.Snapshot();
Line 189: resultSnapshot0.setVmConfiguration(ovfData);
Line 190: resultSnapshot0.setId(SNAPSHOT_IDS[0]);
please do this in own/dedicated test, this one should pass as is
Line 191:
setUpEntityQueryExpectations(VdcQueryType.GetSnapshotBySnapshotId,
Line 192: IdQueryParameters.class,
Line 193: new String[]{"Id"},
Line 194: new Object[]{SNAPSHOT_IDS[0]},
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendVmDisksResourceTest.java
Line 150: Disk model = getModel(0);
Line 151: model.setSnapshot(new Snapshot());
Line 152: model.getSnapshot().setId(snapshotId.toString());
Line 153: model.setId(DISK_ID.toString()); //means this is an existing
disk --> attach
Line 154: model.setSize(1024 * 1024L);
shouldn't size come from snapshot?
Line 155: setUpEntityQueryExpectations(VdcQueryType.GetAllDisksByVmId,
Line 156: IdQueryParameters.class,
Line 157: new String[] { "Id" },
Line 158: new Object[] { PARENT_ID },
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/DiskMapper.java
Line 112: if (disk.isSetStatus()) {
Line 113:
diskImage.setImageStatus(map(DiskStatus.fromValue(disk.getStatus().getState())));
Line 114: }
Line 115: if (disk.isSetSnapshot() && disk.getSnapshot().isSetId()) {
Line 116:
diskImage.setVmSnapshotId(GuidUtils.asGuid(disk.getSnapshot().getId()));
please add test for this field
Line 117: }
Line 118: if (disk.isSetSparse()) {
Line 119: diskImage.setVolumeType(disk.isSparse() ?
VolumeType.Sparse : VolumeType.Preallocated);
Line 120: }
--
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 <[email protected]>
Gerrit-Reviewer: Alissa Bonas <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Ayal Baron <[email protected]>
Gerrit-Reviewer: Daniel Erez <[email protected]>
Gerrit-Reviewer: Federico Simoncelli <[email protected]>
Gerrit-Reviewer: Liron Ar <[email protected]>
Gerrit-Reviewer: Maor Lipchuk <[email protected]>
Gerrit-Reviewer: Michael Pasternak <[email protected]>
Gerrit-Reviewer: Sergey Gotliv <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
_______________________________________________
Engine-patches mailing list
[email protected]
http://lists.ovirt.org/mailman/listinfo/engine-patches
