Juan Hernandez has posted comments on this change. Change subject: restapi: support unregistered Cinder disks ......................................................................
Patch Set 2: (3 comments) https://gerrit.ovirt.org/#/c/40664/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDiskResource.java: Line 29: Disk disk; Line 30: if (QueryHelper.hasMatrixParam(getUriInfo(), UNREGISTERED_CONSTRAINT_PARAMETER)) { Line 31: if (getStorageDomain().getStorageType().isCinderDomain()) { Line 32: disk = super.performGet(VdcQueryType.GetUnregisteredCinderDiskByIdAndStorageDomainId, Line 33: new GetCinderEntityByStorageDomainIdParameters(guid, parent.getStorageDomainId())); Why does the RESTAPI need to handle differently Cinder? I'd say that this logic belongs in the backend. It would be better if the existing "GetUnregisteredDisk" query supported Cinder transparently. Line 34: } else { Line 35: VdcQueryReturnValue result = runQuery(VdcQueryType.GetDiskByDiskId, new IdQueryParameters(guid)); Line 36: if (!result.getSucceeded() || result.getReturnValue() == null) { Line 37: Guid storageDomainGuid = asGuid(storageDomainId); Line 38: disk = super.performGet(VdcQueryType.GetUnregisteredDisk, new GetUnregisteredDiskQueryParameters(guid, storageDomainGuid, parent.getStoragePoolIdForDomain(storageDomainGuid))); Line 39: } else { Line 40: // The disk was found in the first get which means it is already registered. We must return nothing since the unregistered Line 41: // parameter was passed. Line 42: return notFound(); Doesn't this "notFound" logic apply to Cinder disks? Line 43: } Line 44: } Line 45: } else { Line 46: disk = super.performGet(VdcQueryType.GetDiskByDiskId, new IdQueryParameters(guid)); https://gerrit.ovirt.org/#/c/40664/2/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDisksResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageDomainDisksResource.java: Line 68: CinderDisk unregisteredDisk = (CinderDisk) getMapper( Line 69: Disk.class, org.ovirt.engine.core.common.businessentities.storage.Disk.class).map(disk, null); Line 70: RegisterCinderDiskParameters registerDiskParams = Line 71: new RegisterCinderDiskParameters(unregisteredDisk, storageDomainId); Line 72: return performCreate(VdcActionType.RegisterCinderDisk, registerDiskParams, ID_RESOLVER); Can we make the "RegisterDisk" command work with Cinder disks as well? Otherwise we are putting a lot of business logic in the RESTAPI, which we should avoid. Line 73: } else { Line 74: // First we need to query the backend to fill in all the information about the disk from the VDSM. Line 75: // We don't just use the information from the Disk object because it's missing a few things like creation Line 76: // date and last modified date. -- To view, visit https://gerrit.ovirt.org/40664 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ia4cdf428535141caef382444aa634783044664ca Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Daniel Erez <de...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list Engine-patches@ovirt.org http://lists.ovirt.org/mailman/listinfo/engine-patches
