Juan Hernandez has posted comments on this change. Change subject: restapi: Phase 5: Add graphics console representation ......................................................................
Patch Set 6: (17 comments) The general approach of this patch is correct, in my opinion, but there are some details that need to be taken care of. https://gerrit.ovirt.org/#/c/39057/6//COMMIT_MSG Commit Message: Line 17: n Note that the identifiers corresponding to each graphics console should be opaque for the users, they shouldn't count on these values. Instead they should iterate the available consoles and find the one with the protocol they want. If we want to make this easier then we should make the collection searchable, using the protocol as a search criteria: GET /vms/{vm:id}/graphicsconsoles?search=protocol%3Dspice https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3013: </xs:complexType> Line 3014: Line 3015: <xs:element name="graphics_consoles" type="GraphicsConsoles" /> Line 3016: Line 3017: <xs:complexType name="GraphicsConsoles"> This type should extend "BaseResources". Line 3018: <xs:sequence> Line 3019: <xs:element ref="graphics_console" minOccurs="0" maxOccurs="unbounded" /> Line 3020: </xs:sequence> Line 3021: </xs:complexType> Line 3015: <xs:element name="graphics_consoles" type="GraphicsConsoles" /> Line 3016: Line 3017: <xs:complexType name="GraphicsConsoles"> Line 3018: <xs:sequence> Line 3019: <xs:element ref="graphics_console" minOccurs="0" maxOccurs="unbounded" /> There should be an annotation for this element to specify that the corresponding Java name is plural, "GraphicsConsoles". Line 3020: </xs:sequence> Line 3021: </xs:complexType> Line 3022: Line 3023: <xs:element name="graphics_console" type="GraphicsConsole" /> Line 3021: </xs:complexType> Line 3022: Line 3023: <xs:element name="graphics_console" type="GraphicsConsole" /> Line 3024: Line 3025: <xs:complexType name="GraphicsConsole"> This type should extends "BaseResource", and then we don't need to explicitly add the "id" attribute. Line 3026: <xs:sequence> Line 3027: <xs:element name="protocol" type="xs:string" /> Line 3028: <xs:element name="port" type="xs:int" /> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3027: <xs:element name="protocol" type="xs:string" /> Line 3028: <xs:element name="port" type="xs:int" /> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3030: <xs:element name="address" type="xs:string" /> Line 3031: </xs:sequence> Be specific with the "minOccurs" and "maxOccurs" of the elements inside the sequence, should be "minOccurs=0" and "maxOccurs=1". Line 3032: <xs:attribute name="id" type="xs:string" /> Line 3033: </xs:complexType> Line 3034: Line 3035: <xs:complexType name="Ticket"> Line 3029: <xs:element name="tls_port" type="xs:int" /> Line 3030: <xs:element name="address" type="xs:string" /> Line 3031: </xs:sequence> Line 3032: <xs:attribute name="id" type="xs:string" /> Line 3033: </xs:complexType> Before merging remember to use two spaces per level. Line 3034: Line 3035: <xs:complexType name="Ticket"> Line 3036: <xs:sequence> Line 3037: <xs:element name="value" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3289: <xs:element ref="migration" minOccurs="0" maxOccurs="1" /> Line 3290: <xs:element name="custom_properties" type="CustomProperties" minOccurs="0" maxOccurs="1"/> Line 3291: <xs:element name="custom_emulated_machine" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3292: <xs:element name="custom_cpu_model" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 3293: <xs:element name="graphics_consoles" type="GraphicsConsoles"/> Be explicit with minOccurs and maxOccurs. Line 3294: </xs:sequence> Line 3295: </xs:extension> Line 3296: </xs:complexContent> Line 3297: </xs:complexType> https://gerrit.ovirt.org/#/c/39057/6/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 556: body: Line 557: parameterType: null Line 558: signatures: [] Line 559: urlparams: {} Line 560: # str: {context: matrix, type: 'xs:string', value: 'todo', required: true} Don't specify the "get" link twice, as that will break the generators of the SDKs. We don't currently have a good way to specify that this link supports multiple content types, so just skip that. Line 561: headers: Line 562: Content-Type: {value: application/x-virt-viewer, required: true} Line 563: Filter: {value: true|false, required: false} Line 564: - name: /vms/{vm:id}/logon|rel=logon https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsoleResource.java: Line 33: if (consoleId.equals(graphicsConsole.getId())) { Line 34: return graphicsConsole; Line 35: } Line 36: } Line 37: return null; Returning null will likely generate an ugly stack trace in the result. Check that please. Line 38: } Line 39: Line 40: @Override Line 41: public Response getConsoleDescriptor() { Line 41: public Response getConsoleDescriptor() { Line 42: GraphicsType graphicsType = GraphicsTypeHelper.fromGraphicsTypeHexa(consoleId); Line 43: Line 44: if (graphicsType == null) { Line 45: throw new NullPointerException("something is wrong"); I'd suggest to use a more descriptive message, like "can't find graphics type for graphics console ${id}". Did you check what is the result when this happens? It should be a HTTP 500 error and not a page with a stack trace. Line 46: } Line 47: Line 48: ConsoleOptions consoleOptions = new ConsoleOptions(graphicsType); Line 49: consoleOptions.setVmId(vmGuid); Line 48: ConsoleOptions consoleOptions = new ConsoleOptions(graphicsType); Line 49: consoleOptions.setVmId(vmGuid); Line 50: ConsoleOptions configuredOptions = runQuery(VdcQueryType.ConfigureConsoleOptions, Line 51: new ConfigureConsoleOptionsParams(consoleOptions, true)).getReturnValue(); // fill required data from BE Line 52: // todo adjust console opts based on query/matrix/whatever params Will that be part of a different patch? Line 53: Line 54: String descriptor = runQuery(VdcQueryType.GetConsoleDescriptorFile, new ConsoleOptionsParams(configuredOptions)) Line 55: .getReturnValue(); Line 56: https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendVmGraphicsConsolesResource.java: Line 8: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 9: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... What is the meaning of this comment? Line 13: extends BackendResource Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; Line 9: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 10: import org.ovirt.engine.core.compat.Guid; Line 11: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... Line 13: extends BackendResource This should extend AbstractBackendCollectionResource<GraphicsConsoles, VM> Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; Line 17: Line 12: public class BackendVmGraphicsConsolesResource // backendVmgraphics... Line 13: extends BackendResource Line 14: implements VmGraphicsConsolesResource { Line 15: Line 16: private final Guid guid; That base class provides a "parentId" attribute, so this won't be needed. Line 17: Line 18: public BackendVmGraphicsConsolesResource(Guid guid) { Line 19: this.guid = guid; Line 20: } Line 22: @Override Line 23: public GraphicsConsoles list() { Line 24: VM vm = getEntity(VM.class, VdcQueryType.GetVmByVmId, Line 25: new IdQueryParameters(guid), guid.toString(), true); Line 26: return VmMapper.map(vm, new GraphicsConsoles()); This doesn't populate links, you need to call the "addLinks" and "populate" methods. Look at "BackendStorageDomainImagesResource.mapCollection", for example. Line 27: } Line 28: Line 29: @Override Line 30: public VmGraphicsConsoleResource getVmConsoleResource(String id) { https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GraphicsTypeHelper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/GraphicsTypeHelper.java: Line 5: Line 6: public class GraphicsTypeHelper { Line 7: Line 8: private static final String SPICE_STRING = "SPICE"; Line 9: private static final String VNC_STRING = "VNC"; The usual way to do this in the RESTAPI is to define a "GraphicsType" enum, document it in the capabilities resource, and create a mapper that does this translations. Look at "DiskFormat", "BackendCapabilitiesResource.addDiskFormats", "DiskValidator" and "DiskMapper". Line 10: Line 11: Line 12: public static String graphicsTypeToString(GraphicsType graphicsType) { Line 13: if (graphicsType != null) { https://gerrit.ovirt.org/#/c/39057/6/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/VmMapper.java: Line 565: } Line 566: Line 567: @Mapping(from = org.ovirt.engine.core.common.businessentities.VM.class, to = GraphicsConsoles.class) Line 568: public static GraphicsConsoles map(org.ovirt.engine.core.common.businessentities.VM vm, GraphicsConsoles template) { Line 569: template = template != null ? template : new GraphicsConsoles(); Avoid returning "template" here, it is confusing. Create a "model" variable like in the other mappers: GraphicsConsoles model = ...; ... return model; Line 570: Line 571: for (Map.Entry<GraphicsType, GraphicsInfo> graphicsInfo : vm.getGraphicsInfos().entrySet()) { Line 572: GraphicsConsole graphics = new GraphicsConsole(); Line 573: String graphicsTypeString = GraphicsTypeHelper.graphicsTypeToString(graphicsInfo.getKey()); -- To view, visit https://gerrit.ovirt.org/39057 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I7dbc63ac49bfba5637c38b84b373d78f348c7872 Gerrit-PatchSet: 6 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Frank Kobzik <[email protected]> Gerrit-Reviewer: Frank Kobzik <[email protected]> Gerrit-Reviewer: Juan Hernandez <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
