Liron Ar has posted comments on this change. Change subject: core, restapi: Introducing support for attaching disk snapshot ......................................................................
Patch Set 31: (18 comments) @Sergey 1. It's already disabled :) take a look : http://gerrit.ovirt.org/#/c/17679/31/frontend/webadmin/modules/uicommonweb/src/main/java/org/ovirt/engine/ui/uicommonweb/models/vms/EditDiskModel.java 2. Answered in file .................................................... Commit Message Line 14: was used, if the user wanted to see the disk content at some snapshot he Line 15: had to preview that snapshot. Line 16: After this change, a snapshot of a disk can be attached to another vm, Line 17: regardless of the disk not being marked as shareable - when doing so, Line 18: VDSM should create a temp snapshot allowing read/write access above the selected snapshot, the above should happend when hotplugging a disk/ running a vm. In case of hot unplug of the disk snapshot vdsm should delete the temp snapshot. Done Line 19: Line 20: The following limitations currently applies on that ability: Line 21: 1. The created temp snapshots is stored on the host local storage (the host that the vm is running on) and not on the shared storage (domains) therefore the vm can't be migrated. Line 22: 2. A disk snapshot can be attached to a different VM than the one of Line 17: regardless of the disk not being marked as shareable - when doing so, Line 18: VDSM should create a temp snapshot allowing read/write access above the selected snapshot, the above should happend when hotplugging a disk/ running a vm. In case of hot unplug of the disk snapshot vdsm should delete the temp snapshot. Line 19: Line 20: The following limitations currently applies on that ability: Line 21: 1. The created temp snapshots is stored on the host local storage (the host that the vm is running on) and not on the shared storage (domains) therefore the vm can't be migrated. Done Line 22: 2. A disk snapshot can be attached to a different VM than the one of Line 23: which the snapshot (VM snapshot) was taken of. Line 24: Line 25: Usage: .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/AddVmFromSnapshotCommand.java Line 268: devices, Line 269: getSrcDeviceIdToTargetDeviceIdMapping(), Line 270: getParameters().isSoundDeviceEnabled(), Line 271: getParameters().isConsoleEnabled(), Line 272: isVirtioScsiEnabled(), false); Done Line 273: } Line 274: Line 275: @Override Line 276: protected VdcActionType getChildActionType() { .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/UpdateVmDiskCommand.java Line 249: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_RESIZE_DISK_SNAPSHOT); Line 250: } Line 251: } Line 252: Line 253: if (shouldResizeDiskImage()) { 1. Correct, done. 2. I prefer to not duplicate the logic, if we have a method called (shouldResize..) i prefer to call it although it will check again for the snapshotId, i see this as non issue. Line 254: DiskImage oldDiskImage = (DiskImage) getOldDisk(); Line 255: Line 256: if (oldDiskImage.getSize() > newDiskImage.getSize()) { Line 257: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REQUESTED_DISK_SIZE_IS_TOO_SMALL); Line 249: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_CANNOT_RESIZE_DISK_SNAPSHOT); Line 250: } Line 251: } Line 252: Line 253: if (shouldResizeDiskImage()) { Done Line 254: DiskImage oldDiskImage = (DiskImage) getOldDisk(); Line 255: Line 256: if (oldDiskImage.getSize() > newDiskImage.getSize()) { Line 257: return failCanDoAction(VdcBllMessages.ACTION_TYPE_FAILED_REQUESTED_DISK_SIZE_IS_TOO_SMALL); .................................................... File backend/manager/modules/bll/src/main/java/org/ovirt/engine/core/bll/validator/RunVmValidator.java Line 308: } Line 309: Line 310: /** Line 311: * A general method for run vm validations. used in runVmCommand and in VmPoolCommandBase Line 312: * seems like it is..but not related to this change :) Line 313: * @param vm Line 314: * @param messages Line 315: * @param vmDisks Line 316: * @param bootSequence .................................................... 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: } yes, because when adding the the vm/snapshot links for the disk snapshot when issueing GET per VM we don't want to actions to be displayed as they aren't related there. example of current output: <vm> <disks> <disk> ... <active>true</active> <snapshot href= "/api/vms/94a3548b-a3b9-422f-b971-ff3ea411ed93/snapshots/0f3525d1-6bdb-4231-907c-f0b6a7b2cc12" id="0f3525d1-6bdb-4231-907c-f0b6a7b2cc12"> <vm href= "/api/vms/94a3548b-a3b9-422f-b971-ff3ea411ed93" id="94a3548b-a3b9-422f-b971-ff3ea411ed93"/> </snapshot> ... </disk> no links for operations on the "origin" vm/snapshot. 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 can you elaborate? when adding a disk to the vm you could specify the snapshot of the disk that you want to attach. 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); Done. 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(), Done 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()))); Done 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()); The recieved snapshot is of another vm then the parent (we attached disk snapshot from another vm to "this" vm) - therefore it's taken from the query. 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())); Done 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); The query was changed in the backend in this patch, therefore this test has been changed otherwise it won't work - it's was just adjusted to work with the updated query which returns the snapshot (including configuration) and not just the configuration 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]); The query was changed in the backend in this patch, therefore this test has been changed otherwise it won't work - it's was just adjusted to work with the updated query which returns the snapshot (including configuration) and not just the configuration 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]); The query was changed in the backend in this patch, therefore this test has been changed otherwise it won't work - it's was just adjusted to work with the updated query which returns the snapshot (including configuration) and not just the configuration 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); Removed 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())); It's being tested on DiskMapperTest.testRoundtrip(), what further test would you like me to add? 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 <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
