Gilad Chaplik has posted comments on this change. Change subject: restapi: implement Quota REST-API ......................................................................
Patch Set 1: (34 comments) https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ClusterQuotaLimitsResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/ClusterQuotaLimitsResource.java: Line 6: Line 7: import org.ovirt.engine.api.model.ClusterQuotaLimit; Line 8: import org.ovirt.engine.api.model.ClusterQuotaLimits; Line 9: Line 10: @Path("/clusterquotalimits") > Remove this line, only necessary for root collections. Done Line 11: @Produces({ ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML }) Line 12: public interface ClusterQuotaLimitsResource extends QuotaLimitsResource<ClusterQuotaLimits, ClusterQuotaLimit> { Line 13: @Override Line 14: @Path("{id}") https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/QuotaResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/QuotaResource.java: Line 7: Line 8: @Produces({ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML}) Line 9: public interface QuotaResource extends UpdatableResource<Quota> { Line 10: Line 11: @Path("storagequotalimits") > If you do decide to change the names to quotastoragelimit and clusterstorag Done Line 12: public StorageQuotaLimitsResource getStorageQuotaLimitsResource(); Line 13: Line 14: @Path("clusterquotalimits") Line 15: public ClusterQuotaLimitsResource getClusterQuotaLimitsResource(); https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StorageQuotaLimitsResource.java File backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StorageQuotaLimitsResource.java: Line 6: Line 7: import org.ovirt.engine.api.model.StorageQuotaLimit; Line 8: import org.ovirt.engine.api.model.StorageQuotaLimits; Line 9: Line 10: @Path("/storagequotalimits") > Remove this, necessary only for root collections (collections which are dir Done Line 11: @Produces({ ApiMediaType.APPLICATION_XML, ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML }) Line 12: public interface StorageQuotaLimitsResource extends QuotaLimitsResource<StorageQuotaLimits, StorageQuotaLimit> { Line 13: @Override Line 14: @Path("{id}") https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 4060: <xs:element ref="users" minOccurs="0" maxOccurs="1"/> Line 4061: <xs:element name="cluster_soft_limit_pct" type="xs:int" minOccurs="0" maxOccurs="1"/> Line 4062: <xs:element name="cluster_hard_limit_pct" type="xs:int" minOccurs="0" maxOccurs="1"/> Line 4063: <xs:element name="storage_soft_limit_pct" type="xs:int" minOccurs="0" maxOccurs="1"/> Line 4064: <xs:element name="storage_hard_limit_pct" type="xs:int" minOccurs="0" maxOccurs="1"/> > Is "xs:int" a good type for a percentage in this case? In a quota of 1 TiB, this feature isn't used that much, also implemented as 'int' in backend, I prefer with you permission to leave it as simple as possible. Line 4065: </xs:sequence> Line 4066: </xs:extension> Line 4067: </xs:complexContent> Line 4068: </xs:complexType> Line 4083: </xs:extension> Line 4084: </xs:complexContent> Line 4085: </xs:complexType> Line 4086: Line 4087: <xs:element name="storage_quota_limit" type="StorageQuotaLimit" /> > Maybe QuotaLimitStorage, QuotaLimitCluster are clearer names? By having the Done Line 4088: Line 4089: <xs:complexType name="StorageQuotaLimit"> Line 4090: <xs:complexContent> Line 4091: <xs:extension base="BaseResource"> https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml File backend/manager/modules/restapi/interface/definition/src/main/resources/rsdl_metadata.yaml: Line 5465: body: Line 5466: parameterType: null Line 5467: signatures: [] Line 5468: - name: /datacenters/{datacenter:id}/quotas|rel=get Line 5469: description: get a list of Quotas for specified Data Center > Remove from here ... Done Line 5470: request: Line 5471: body: Line 5472: parameterType: null Line 5473: signatures: [] Line 5469: description: get a list of Quotas for specified Data Center Line 5470: request: Line 5471: body: Line 5472: parameterType: null Line 5473: signatures: [] > ... to here. Done Line 5474: - name: /datacenters/{datacenter:id}/quotas/{quota:id}|rel=get Line 5475: description: get the details of the specified Quota Line 5476: request: Line 5477: body: Line 5475: description: get the details of the specified Quota Line 5476: request: Line 5477: body: Line 5478: parameterType: null Line 5479: signatures: [] > Same. Done Line 5480: - name: /datacenters/{datacenter:id}/quotas/{quota:id}|rel=delete Line 5481: description: delete the specified user defined scheduling policy in the system Line 5482: request: Line 5483: body: Line 5477: body: Line 5478: parameterType: null Line 5479: signatures: [] Line 5480: - name: /datacenters/{datacenter:id}/quotas/{quota:id}|rel=delete Line 5481: description: delete the specified user defined scheduling policy in the system > left over from copy-paste? ;) Done Line 5482: request: Line 5483: body: Line 5484: parameterType: null Line 5485: signatures: [] Line 5483: body: Line 5484: parameterType: null Line 5485: signatures: [] Line 5486: urlparams: Line 5487: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} > A recent patch by Juan adds 'async' url param implicitly to all delete and Done Line 5488: - name: /datacenters/{datacenter:id}/quotas|rel=add Line 5489: description: add a new Quota to a Data Center Line 5490: request: Line 5491: body: Line 5490: request: Line 5491: body: Line 5492: parameterType: Quota Line 5493: signatures: Line 5494: - mandatoryArguments: {quota.name: 'xs:string'} > Use indentation instead of curly brackets: Done Line 5495: optionalArguments: Line 5496: quota.description: xs:string Line 5497: quota.cluster_soft_limit_pct: xs:int Line 5498: quota.cluster_hard_limit_pct: xs:int Line 5514: quota.storage_soft_limit_pct: xs:int Line 5515: quota.storage_hard_limit_pct: xs:int Line 5516: description: update the specified Quota Line 5517: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/storagequotalimits|rel=get Line 5518: description: get a list of specified storage limits of a Quota > Remove the word 'specified' Done Line 5519: request: Line 5520: body: Line 5521: parameterType: null Line 5522: signatures: [] Line 5518: description: get a list of specified storage limits of a Quota Line 5519: request: Line 5520: body: Line 5521: parameterType: null Line 5522: signatures: [] > Remove empty things. Done Line 5523: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/storagequotalimits/{storagequotalimit:id}|rel=get Line 5524: description: get a specified storage limit of a Quota Line 5525: request: Line 5526: body: Line 5524: description: get a specified storage limit of a Quota Line 5525: request: Line 5526: body: Line 5527: parameterType: null Line 5528: signatures: [] > Remove empty things. Done Line 5529: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/storagequotalimits/{storagequotalimit:id}|rel=delete Line 5530: description: delete a storage limit from a Quota Line 5531: request: Line 5532: body: Line 5541: parameterType: StorageQuotaLimit Line 5542: signatures: Line 5543: - mandatoryArguments: {} Line 5544: optionalArguments: Line 5545: storage_domain.id: 'xs:string' > I believe these should by mandatory arguments? a global limit is set without specifying the storage domain. Line 5546: limit: 'xs:long' Line 5547: description: add a storage limit to a specified Quota Line 5548: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/clusterquotalimits/{clusterquotalimit:id}|rel=get Line 5549: description: get a specified cluster limit of a Quota Line 5542: signatures: Line 5543: - mandatoryArguments: {} Line 5544: optionalArguments: Line 5545: storage_domain.id: 'xs:string' Line 5546: limit: 'xs:long' > Don't use quotes around "xs:...". Done Line 5547: description: add a storage limit to a specified Quota Line 5548: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/clusterquotalimits/{clusterquotalimit:id}|rel=get Line 5549: description: get a specified cluster limit of a Quota Line 5550: request: Line 5549: description: get a specified cluster limit of a Quota Line 5550: request: Line 5551: body: Line 5552: parameterType: null Line 5553: signatures: [] > Remove empty things. Done Line 5554: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/clusterquotalimits/{clusterquotalimit:id}|rel=delete Line 5555: description: delete a cluster limit from a Quota Line 5556: request: Line 5557: body: Line 5557: body: Line 5558: parameterType: null Line 5559: signatures: [] Line 5560: urlparams: Line 5561: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} > A recent patch by Juan adds 'async' url param implicitly to all delete and Done Line 5562: - name: /datacenters/{datacenter:id}/quotas/{quota:id}/clusterquotalimits|rel=add Line 5563: description: add a cluster limit to a specified Quota Line 5564: request: Line 5565: body: Line 5567: signatures: Line 5568: - mandatoryArguments: {} Line 5569: optionalArguments: Line 5570: cluster_domain.id: 'xs:string' Line 5571: limit: 'xs:long' > Don't use quotes around "xs:...". Done https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendClusterQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendClusterQuotaLimitsResource.java: Line 22: if (quota.getGlobalQuotaVdsGroup() != null) { Line 23: addLimit(quota.getGlobalQuotaVdsGroup(), quotaId.toString(), limits, quota); Line 24: } else if (quota.getQuotaVdsGroups() != null) { Line 25: for (QuotaVdsGroup quotaCluster : quota.getQuotaVdsGroups()) { Line 26: addLimit(quotaCluster, quotaCluster.getVdsGroupId().toString(), limits, quota); > 1) Using cluster-ID as the limitation ID implies that no more than 1 cluste 1) quota element is bound to a datacenter, therefore it may have more than a single cluster limit, i.e each limit will have its cluster id. 2) imo, using md5 here is an over head. we can take it off line and decide. Line 27: } Line 28: } Line 29: return limits; Line 30: } Line 54: } Line 55: // specific cluster (has same id as cluster) Line 56: } else { Line 57: if (entity.getQuotaVdsGroups() != null) { Line 58: for (int i = 0; i < entity.getQuotaVdsGroups().size(); i++) { > I think this isn't the right way to delete an item from a collection while Done (I'm interested to know why) Line 59: if (entity.getQuotaVdsGroups().get(i).getVdsGroupId().equals(id)) { Line 60: entity.getQuotaVdsGroups().remove(i); Line 61: return; Line 62: } https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitResource.java: Line 8: import org.ovirt.engine.core.compat.Guid; Line 9: Line 10: public abstract class BackendQuotaLimitResource<T extends BaseResource> extends AbstractBackendSubResource<T, Quota> implements Line 11: QuotaLimitResource<T> { Line 12: private static final String[] SUB_COLLECTIONS = {}; > No need if it's empty Done Line 13: private final Guid parentId; Line 14: Line 15: protected BackendQuotaLimitResource(String id, Line 16: Guid parentId, Line 29: return super.map(entity, createQuotaLimit()); Line 30: } Line 31: Line 32: @Override Line 33: protected T doPopulate(T model, Quota entity) { > No need to override this, enough that it's overriden in ancestor Done Line 34: return model; Line 35: } Line 36: Line 37: protected abstract T createQuotaLimit(); https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotaLimitsResource.java: Line 16: Line 17: public abstract class BackendQuotaLimitsResource<M extends BaseResources, N extends BaseResource> extends AbstractBackendCollectionResource<N, Quota> implements QuotaLimitsResource<M, N> { Line 18: Line 19: protected final Guid quotaId; Line 20: private static final String[] SUB_COLLECTIONS = {}; > No need for this variable. Done Line 21: Line 22: protected BackendQuotaLimitsResource(Guid quotaId, Line 23: Class<N> baseResourcesClass) { Line 24: super(baseResourcesClass, Quota.class, SUB_COLLECTIONS); Line 20: private static final String[] SUB_COLLECTIONS = {}; Line 21: Line 22: protected BackendQuotaLimitsResource(Guid quotaId, Line 23: Class<N> baseResourcesClass) { Line 24: super(baseResourcesClass, Quota.class, SUB_COLLECTIONS); > The constructor will work without SUB_COLLECTIONS Done Line 25: this.quotaId = quotaId; Line 26: } Line 27: Line 28: protected abstract void updateEntityForRemove(Quota entity, Guid id); Line 46: new QuotaCRUDParameters(entity)); Line 47: } Line 48: Line 49: @Override Line 50: public N add(N incoming) { > Consider improving the API of the engine. It's a burden on clients to have much needed change, unfortunatly, not in the scope of this feature Line 51: Quota entity = getQuota(); Line 52: performAction(VdcActionType.UpdateQuota, getAddParametersProvider().getParameters(incoming, entity)); Line 53: entity = getQuota(); Line 54: updateIncomingId(incoming, entity); https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotasResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendQuotasResource.java: Line 39: Line 40: @Override Line 41: protected Response performRemove(String id) { Line 42: QuotaCRUDParameters prms = new QuotaCRUDParameters(); Line 43: prms.setQuotaId(GuidUtils.asGuid(id)); > No need for GuidUtils, there's a asGuid(String guid) method in ancestor cla Done Line 44: return performAction(VdcActionType.RemoveQuota, prms); Line 45: } Line 46: Line 47: @Override Line 44: return performAction(VdcActionType.RemoveQuota, prms); Line 45: } Line 46: Line 47: @Override Line 48: public Response add(Quota quota) { > add call to validateParameters() method and list the mandatory parameters. Done Line 49: org.ovirt.engine.core.common.businessentities.Quota entity = map(quota); Line 50: entity.setStoragePoolId(dataCenterId); Line 51: return performCreate(VdcActionType.AddQuota, Line 52: new QuotaCRUDParameters(entity), https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageQuotaLimitsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageQuotaLimitsResource.java: Line 22: if (quota.getGlobalQuotaStorage() != null) { Line 23: addLimit(quota.getGlobalQuotaStorage(), quotaId.toString(), limits, quota); Line 24: } else if (quota.getQuotaStorages() != null) { Line 25: for (QuotaStorage quotaStorage : quota.getQuotaStorages()) { Line 26: addLimit(quotaStorage, quotaStorage.getStorageId().toString(), limits, quota); > I think there can be more than one storage-limit. Giving the storage limit same answer as before, lets take it off-line. Line 27: } Line 28: } Line 29: return limits; Line 30: } https://gerrit.ovirt.org/#/c/39904/1/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/QuotaMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/QuotaMapper.java: Line 74: StorageQuotaLimit template) { Line 75: if (template == null) { Line 76: assert (false) : "StorageQuotaLimit cannot be null"; Line 77: return null; Line 78: } > This, like all mappers, should create the model if no template is given: Done Line 79: Guid guid = GuidUtils.asGuid(template.getId()); Line 80: // global Line 81: if (guid.equals(entity.getId())) { Line 82: map(template, entity.getGlobalQuotaStorage(), null, entity.getStoragePoolId().toString(), entity.getId() Line 75: if (template == null) { Line 76: assert (false) : "StorageQuotaLimit cannot be null"; Line 77: return null; Line 78: } Line 79: Guid guid = GuidUtils.asGuid(template.getId()); > Don't assume that the incoming "template" object has an id, it may come fro mapping engine object to rest object. it cannot be null. I can add a null check but I think it's redundant. Line 80: // global Line 81: if (guid.equals(entity.getId())) { Line 82: map(template, entity.getGlobalQuotaStorage(), null, entity.getStoragePoolId().toString(), entity.getId() Line 83: .toString()); Line 81: if (guid.equals(entity.getId())) { Line 82: map(template, entity.getGlobalQuotaStorage(), null, entity.getStoragePoolId().toString(), entity.getId() Line 83: .toString()); Line 84: } else { // specific Line 85: if (entity.getQuotaStorages() != null) { > I think it's unnecessary to map storage limitations, because they are displ In order to create the link, datacenter ref is needed (/datacenter/<id>/quotas/<id>/quotastoragelimits/<id>/...), QuotaStorage entity is missing that -> byproduct of adding another layer of modeling. Line 86: for (QuotaStorage quotaStorage : entity.getQuotaStorages()) { Line 87: if (quotaStorage.getStorageId() != null && quotaStorage.getStorageId().equals(guid)) { Line 88: map(template, quotaStorage, quotaStorage.getStorageId().toString(), entity.getStoragePoolId() Line 89: .toString(), entity.getId().toString()); Line 93: } Line 94: return template; Line 95: } Line 96: Line 97: private static void map(StorageQuotaLimit template, > The name for this ^ parameter should probably be "model", not "template" as Done Line 98: QuotaStorage quotaStorage, Line 99: String storageDomainId, Line 100: String dataCenterId, Line 101: String quotaId) { Line 140: if (template == null) { Line 141: assert (false) : "ClusterQuotaLimit cannot be null"; Line 142: return null; Line 143: } Line 144: Guid guid = GuidUtils.asGuid(template.getId()); > Create the model object and check the id, see the previous comments. Done Line 145: // global Line 146: if (guid.equals(entity.getId())) { Line 147: map(template, entity.getGlobalQuotaVdsGroup(), null, entity.getStoragePoolId().toString(), entity.getId() Line 148: .toString()); -- To view, visit https://gerrit.ovirt.org/39904 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Ifaead35839c2c5f89f1551e67e58a58aa253cab5 Gerrit-PatchSet: 1 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Gilad Chaplik <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: Ori Liel <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
