Alissa Bonas has posted comments on this change.
Change subject: restapi: add storage server connections resource
......................................................................
Patch Set 25: (13 inline comments)
....................................................
File
backend/manager/modules/common/src/main/java/org/ovirt/engine/core/common/queries/StorageServerConnectionQueryParametersBase.java
Line 18: public StorageServerConnectionQueryParametersBase(String
serverConnectionId) {
Line 19: setServerConnectionId(serverConnectionId);
Line 20: }
Line 21:
Line 22: /**used by REST because AbstractBackendResource has id member that
is always assumed to be guid
Done
Line 23: *
Line 24: * @param serverConnectionId
Line 25: */
Line 26: public StorageServerConnectionQueryParametersBase(Guid
serverConnectionId) {
....................................................
File
backend/manager/modules/dal/src/main/java/org/ovirt/engine/core/dao/StorageServerConnectionDAODbFacadeImpl.java
Line 23:
Line 24: @Override
Line 25: public List<StorageServerConnections> getAll() {
Line 26: MapSqlParameterSource parameterSource =
getCustomMapSqlParameterSource();
Line 27: return
getCallsHandler().executeReadList("GetAllstorage_server_connections",
mapper,parameterSource) ;
Done
Line 28: }
Line 29:
Line 30: @Override
Line 31: public StorageServerConnections get(String id) {
....................................................
File
backend/manager/modules/restapi/interface/common/jaxrs/src/main/java/org/ovirt/engine/api/common/util/LinkHelper.java
Line 271: map = new ParentToCollectionMap(SnapshotResource.class,
SnapshotsResource.class, VM.class);
Line 272: TYPES.put(Snapshot.class, map);
Line 273:
Line 274: map = new
ParentToCollectionMap(StorageServerConnectionResource.class,
StorageServerConnectionsResource.class);
Line 275: map.add(StorageResource.class, HostStorageResource.class,
Host.class);
I am adding a new relation - just doing it first at the creation of the map,
and then adding the existing one to keep backwards compatibility. (the order is
per Allon's suggestion)
Line 276: TYPES.put(Storage.class, map);
Line 277:
Line 278: map = new ParentToCollectionMap(StorageDomainResource.class,
StorageDomainsResource.class);
Line 279: map.add(AttachedStorageDomainResource.class,
AttachedStorageDomainsResource.class, DataCenter.class);
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/java/org/ovirt/engine/api/resource/StorageServerConnectionsResource.java
Line 24: /**
Line 25: * Adds a storage connection entity
Line 26: *
Line 27: * @param storageConnection the storage connection to add
Line 28: * @return the new newly added storage connection
Done
Line 29: */
Line 30: @POST
Line 31: @Formatted
Line 32: @Consumes({ApiMediaType.APPLICATION_XML,
ApiMediaType.APPLICATION_JSON, ApiMediaType.APPLICATION_X_YAML})
....................................................
File
backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd
Line 1974: <xs:complexType name="Storage">
Line 1975: <xs:complexContent>
Line 1976: <xs:extension base="BaseResource">
Line 1977: <xs:sequence>
Line 1978: <xs:group ref="BaseStorageConnection"/>
BaseResource contains fields that are irrelevant for storage connection - name,
description, comment - there are no such fields there.
Thus the separation, so restructuring as per your suggestion is not the best
solution.
The usage of BaseResource and Storage objects remained because of the need in
backwards compatibility using that structure as part of storage domains
resource, especially in iscsi case.
Line 1979: <xs:choice>
Line 1980: <xs:group ref="NfsStorage"/>
Line 1981: <xs:group ref="IscsiStorage"/>
Line 1982: <xs:group ref="IscsiStorageConnection"/>
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionResource.java
Line 23: }
Line 24:
Line 25: @Override
Line 26: public Storage get() {
Line 27: return performGet(VdcQueryType.GetStorageServerConnectionById,
new StorageServerConnectionQueryParametersBase(guid.toString()));
StorageServerConnectoin id is of format Guid, however for some historical
reason is represented in backend by String - that is the reason for it.
Line 28: }
Line 29:
Line 30: @Override
Line 31: public Storage update(Storage connection) {
Line 28: }
Line 29:
Line 30: @Override
Line 31: public Storage update(Storage connection) {
Line 32: validateParameters(connection, "address", "type", "host");
In order to update something in storage connection, currently ( in the future
it will change) user is required to enter the vdsm host details with which the
validations of the connection to storage will be performed -because update of
storage connection not just updates it in db, but it requires some work on vdsm
side as well.
Also, there is no description field in connection. Users mostly want to update
things related to the connection itself - address/port etc.
I can remove validation of address and type, however host is currently required.
Line 33: validateEnums(Storage.class, connection);
Line 34: return performUpdate(connection,
Line 35: new
QueryIdResolver<String>(VdcQueryType.GetStorageServerConnectionById,
StorageServerConnectionQueryParametersBase.class),
Line 36:
VdcActionType.UpdateStorageServerConnection,
Line 56: return new
StorageServerConnectionParametersBase(connection,hostId);
Line 57: }
Line 58:
Line 59: private Guid getHostId(Host host) {
Line 60: // presence of host ID or name already validated
1. see my comment above regarding required host.
2. the connections resource it not inheriting from actionableResource.
Also there are other resources that also not reusing it - see domainsresource,
vmsresource. I agree it would be nicer, however this is a cross change to
change hierarchy/make that method available for all inheritance, and not
something to be done in this patch.
Line 61: return host.isSetId()
Line 62: ? new Guid(host.getId())
Line 63: : host.isSetName()
Line 64: ? getEntity(VDS.class,
Line 64: ? getEntity(VDS.class,
Line 65: SearchType.VDS,
Line 66: "Hosts: name=" +
host.getName()).getId()
Line 67: : null;
Line 68: }
Done
Line 69: }
....................................................
File
backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionsResource.java
Line 45: StorageConnections collection = new StorageConnections();
Line 46: for
(org.ovirt.engine.core.common.businessentities.StorageServerConnections entity
: entities) {
Line 47: Storage connection = map(entity);
Line 48: if (connection != null) {
Line 49: collection.getStorageConnections().add(connection);
I understand your comment regarding addLinks missing, added it.
However what populate method are you referring to?
I checked BackendStorageDomainsResource - mapCollection method, it has
addlinks, but no populate method.
Please explain.
Line 50: }
Line 51: }
Line 52: return collection;
Line 53: }
Line 53: }
Line 54:
Line 55: @Override
Line 56: public Response add(Storage storage) {
Line 57: validateParameters(storage, "address", "type", "host");
1. agree.
2. yes, address field is relevant for all storage types.
Line 58: // map to backend object
Line 59: StorageServerConnections storageConnection =
Line 60: getMapper(Storage.class,
StorageServerConnections.class).map(storage, null);
Line 61: Guid hostId = getHostId(storage.getHost());
Line 57: validateParameters(storage, "address", "type", "host");
Line 58: // map to backend object
Line 59: StorageServerConnections storageConnection =
Line 60: getMapper(Storage.class,
StorageServerConnections.class).map(storage, null);
Line 61: Guid hostId = getHostId(storage.getHost());
will solve it with validation of host id/name
Line 62: switch (storageConnection.getstorage_type()) {
Line 63: case ISCSI:
Line 64: validateParameters(storage, "iqn", "port");
Line 65: break;
....................................................
File
backend/manager/modules/restapi/jaxrs/src/test/java/org/ovirt/engine/api/restapi/resource/BackendStorageServerConnectionResourceTest.java
Line 101: protected void update(boolean canDo, boolean
executeCommandResult, int getConnectionExecTimes) throws Exception {
Line 102: // the below method is called several times because
Line 103: // the mocked behavior must be recorded twice
Line 104: // since the getConnectionById query is executed
Line 105: // twice during a successful update operation.
Yes, I know WHY it is called twice, however in the current test infrastructure
mocking behavior it must be taken care of "manually" as I did below.
Line 106: // not calling it twice causes the test to fail with NPE.
Line 107: for (int i = 0; i < getConnectionExecTimes; i++) {
Line 108: setUpGetEntityExpectations();
Line 109: }
--
To view, visit http://gerrit.ovirt.org/16617
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: If6bc32ead098390723825872f6fb292097d52835
Gerrit-PatchSet: 25
Gerrit-Project: ovirt-engine
Gerrit-Branch: master
Gerrit-Owner: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Alissa Bonas <abo...@redhat.com>
Gerrit-Reviewer: Allon Mureinik <amure...@redhat.com>
Gerrit-Reviewer: Daniel Erez <de...@redhat.com>
Gerrit-Reviewer: Michael Pasternak <mpast...@redhat.com>
Gerrit-Reviewer: Ori Liel <ol...@redhat.com>
Gerrit-Reviewer: Tal Nisan <tni...@redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
_______________________________________________
Engine-patches mailing list
Engine-patches@ovirt.org
http://lists.ovirt.org/mailman/listinfo/engine-patches
