Michael Pasternak has posted comments on this change.
Change subject: restapi: disk entity export to glance
......................................................................
Patch Set 2: Code-Review-1
(12 comments)
....................................................
Commit Message
Line 4: Commit: Federico Simoncelli <fsimo...@redhat.com>
Line 5: CommitDate: 2013-08-22 23:55:10 +0200
Line 6:
Line 7: restapi: disk entity export to glance
Line 8:
you didn't address comments from the previous patch-set
Line 9: Change-Id: If351d209d7828f0d59926bb98da9317567976d41
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml
Line 538: urlparams: {}
Line 539: headers:
Line 540: Content-Type: {value: application/xml|json, required: true}
Line 541: Correlation-Id: {value: 'any string', required: false}
Line 542: Filter: {value: true|false, required: false}
afaics it also exposed at
BackendStorageDomainDiskResource
and
BackendDiskResource
Line 543: - name: /api/vms/{vm:id}/disks/{disk:id}/export|rel=export
Line 544: request:
Line 545: body:
Line 546: parameterType: Action
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendDiskResource.java
Line 50: public Response doExport(Action action) {
Line 51: validateParameters(action, "storageDomain.id|name");
Line 52:
Line 53: Disk disk = get();
Line 54: Guid sourceStorageDomainId = new
Guid(disk.getStorageDomains().getStorageDomains().get(0).getId());
please use asGuid()
Line 55: Guid sourceStoragePoolId =
StorageDomainHelper.getStoragePoolIdForDomain(this, sourceStorageDomainId);
Line 56:
Line 57: ExportRepoImageParameters exportParameters = new
ExportRepoImageParameters(new Guid(disk.getImageId()));
Line 58: exportParameters.setStoragePoolId(sourceStoragePoolId);
Line 53: Disk disk = get();
Line 54: Guid sourceStorageDomainId = new
Guid(disk.getStorageDomains().getStorageDomains().get(0).getId());
Line 55: Guid sourceStoragePoolId =
StorageDomainHelper.getStoragePoolIdForDomain(this, sourceStorageDomainId);
Line 56:
Line 57: ExportRepoImageParameters exportParameters = new
ExportRepoImageParameters(new Guid(disk.getImageId()));
please use asGuid()
Line 58: exportParameters.setStoragePoolId(sourceStoragePoolId);
Line 59: exportParameters.setStorageDomainId(sourceStorageDomainId);
Line 60: exportParameters.setImageGroupID(new Guid(disk.getId()));
Line 61:
exportParameters.setDestinationDomainId(getStorageDomainId(action));
Line 55: Guid sourceStoragePoolId =
StorageDomainHelper.getStoragePoolIdForDomain(this, sourceStorageDomainId);
Line 56:
Line 57: ExportRepoImageParameters exportParameters = new
ExportRepoImageParameters(new Guid(disk.getImageId()));
Line 58: exportParameters.setStoragePoolId(sourceStoragePoolId);
Line 59: exportParameters.setStorageDomainId(sourceStorageDomainId);
in my view get_source logic should reside at the engine, this way you can
change at one place when you need (for load balancing for instance, etc.)
Line 60: exportParameters.setImageGroupID(new Guid(disk.getId()));
Line 61:
exportParameters.setDestinationDomainId(getStorageDomainId(action));
Line 62:
Line 63: return doAction(VdcActionType.ExportRepoImage,
exportParameters, action);
Line 56:
Line 57: ExportRepoImageParameters exportParameters = new
ExportRepoImageParameters(new Guid(disk.getImageId()));
Line 58: exportParameters.setStoragePoolId(sourceStoragePoolId);
Line 59: exportParameters.setStorageDomainId(sourceStorageDomainId);
Line 60: exportParameters.setImageGroupID(new Guid(disk.getId()));
please use asGuid()
Line 61:
exportParameters.setDestinationDomainId(getStorageDomainId(action));
Line 62:
Line 63: return doAction(VdcActionType.ExportRepoImage,
exportParameters, action);
Line 64: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmDiskResource.java
Line 119:
VdcObjectType.Disk));
Line 120: }
Line 121:
Line 122: @Override
Line 123: public Response doExport(Action action) {
1. same comments as in BackendDiskResource.java
2. it would be good if you can share this method with BackendDiskResource (this
is a same code after all)
Line 124: validateParameters(action, "storageDomain.id|name");
Line 125:
Line 126: Disk disk = getDisk();
Line 127: Guid sourceStorageDomainId = new
Guid(disk.getStorageDomains().getStorageDomains().get(0).getId());
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/util/DiskHelper.java
Line 4: import org.ovirt.engine.api.model.StorageDomain;
Line 5: import org.ovirt.engine.core.common.action.ExportRepoImageParameters;
Line 6: import org.ovirt.engine.core.compat.Guid;
Line 7:
Line 8: public class DiskHelper {
you mean removing entire class?, if no i've comments
Line 9: public static ExportRepoImageParameters
getExportRepoImageParameters(Disk disk, Guid destinationStorageDomainId) {
Line 10: if (disk.isSetLunStorage()) {
Line 11: throw new UnsupportedOperationException("Direct LUN disks
are unsupported");
Line 12: }
....................................................
File
backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/DiskMapper.java
Line 181: }
Line 182: for (Guid id : entity.getStorageIds()){
Line 183: StorageDomain storageDomain = new StorageDomain();
Line 184: storageDomain.setId(id.toString());
Line 185: storageDomain.setDataCenter(dataCenter);
comment from the previous patch-set
Line 186:
model.getStorageDomains().getStorageDomains().add(storageDomain);
Line 187: }
Line 188: }
Line 189: if (entity.getQuotaId()!=null) {
Line 304: return null;
Line 305: }
Line 306: }
Line 307:
Line 308: public static ExportRepoImageParameters
getExportRepoImageParameters(Disk disk, Guid destinationStorageDomainId) {
1. i didn't saw you suing this mapper at doExport() methods
2. it should be regular map() method with appropriate annotations
Line 309: if (disk.isSetLunStorage()) {
Line 310: throw new UnsupportedOperationException("Direct LUN disks
are unsupported");
Line 311: }
Line 312:
Line 311: }
Line 312:
Line 313: if (disk.getStorageDomains() == null ||
disk.getStorageDomains().getStorageDomains().size() < 1) {
Line 314: throw new IllegalStateException("Cannot find disk storage
domain");
Line 315: }
these are command CDA errors, they should not reside in api
Line 316:
Line 317: ExportRepoImageParameters exportParameters = new
ExportRepoImageParameters(new Guid(disk.getImageId()));
Line 318: StorageDomain storageDomain =
disk.getStorageDomains().getStorageDomains().get(0);
Line 319:
Line 318: StorageDomain storageDomain =
disk.getStorageDomains().getStorageDomains().get(0);
Line 319:
Line 320: exportParameters.setStoragePoolId(new
Guid(storageDomain.getDataCenter().getId()));
Line 321: exportParameters.setStorageDomainId(new
Guid(storageDomain.getId()));
Line 322: exportParameters.setImageGroupID(new Guid(disk.getId()));
same comments as in doExport() methods
Line 323:
Line 324:
exportParameters.setDestinationDomainId(destinationStorageDomainId);
Line 325:
Line 326: return exportParameters;
--
To view, visit http://gerrit.ovirt.org/17541
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If351d209d7828f0d59926bb98da9317567976d41
Gerrit-PatchSet: 2
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimo...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@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
