Martin Mucha has posted comments on this change. Change subject: restapi: Add support for Network Attachements. ......................................................................
Patch Set 23: (34 comments) I think bonds and nics can be specified via names, I verify if (alter code that) same is true for networks. https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd File backend/manager/modules/restapi/interface/definition/src/main/resources/api.xsd: Line 3985: </xs:extension> Line 3986: </xs:complexContent> Line 3987: </xs:complexType> Line 3988: Line 3989: <xs:element name="ip_configuration" type="IpConfiguration"/> > We already have "ip" and "ips" elements. Reuse them instead of adding new o needinfo: I don't know how to do that. In 'IPv4' complex type there's 'primary' element. So I altered this complex type to extend ip adding new 'primary' attribute. But don't know how to reuse 'ips' to contain both collection of 'ip's and collections of 'IPv4's. Line 3990: Line 3991: <xs:complexType name="IpConfiguration"> Line 3992: <xs:sequence> Line 3993: <xs:element ref="ipv4s" minOccurs="0" maxOccurs="1"> Line 3997: </xs:appinfo> Line 3998: </xs:annotation> Line 3999: </xs:element> Line 4000: </xs:sequence> Line 4001: </xs:complexType> > Remember to fix indentation before merging: two spaces per level. Done Line 4002: Line 4003: <xs:element name="ipv4s" type="IPv4s"/> Line 4004: Line 4005: <xs:complexType name="IPv4s"> Line 4018: </xs:extension> Line 4019: </xs:complexContent> Line 4020: </xs:complexType> Line 4021: Line 4022: <xs:element name="ipv4" type="IPv4"/> > Reuse the existing "ip" element. Done Line 4023: Line 4024: <xs:complexType name="IPv4"> Line 4025: <xs:sequence> Line 4026: <xs:element name="primary" type="xs:boolean" minOccurs="0" maxOccurs="1"/> Line 4027: <xs:element name="address" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 4028: <xs:element name="netmask" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 4029: <xs:element name="gateway" type="xs:string" minOccurs="0" maxOccurs="1"/> Line 4030: </xs:sequence> Line 4031: </xs:complexType> > Remember to fix indentation before merging. Done Line 4032: Line 4033: <!-- Host Storage --> Line 4034: Line 4035: <xs:element name="host_storage" type="HostStorage"/> https://gerrit.ovirt.org/#/c/32775/23/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 2634: description: get the list of network attachments of the network interface Line 2635: request: Line 2636: body: Line 2637: parameterType: null Line 2638: signatures: [] > Don't add these three lines ^ . Done Line 2639: urlparams: Line 2640: max: {context: matrix, type: 'xs:int', value: 'max results', required: false} Line 2641: headers: {} Line 2642: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments/{networkattachment:id}|rel=get Line 2637: parameterType: null Line 2638: signatures: [] Line 2639: urlparams: Line 2640: max: {context: matrix, type: 'xs:int', value: 'max results', required: false} Line 2641: headers: {} > Remove this empty list of headers. Done Line 2642: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments/{networkattachment:id}|rel=get Line 2643: description: get the details of the specified network attachment of the network interface Line 2644: request: Line 2645: body: Line 2645: body: Line 2646: parameterType: null Line 2647: signatures: [] Line 2648: urlparams: {} Line 2649: headers: {} > Don't add these 5 lines ^ . Done Line 2650: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments/{networkattachment:id}|rel=delete Line 2651: description: delete the specified network attachment from the network interface Line 2652: request: Line 2653: body: Line 2651: description: delete the specified network attachment from the network interface Line 2652: request: Line 2653: body: Line 2654: parameterType: null Line 2655: signatures: [] > Don't add these three lines ^ . Done Line 2656: urlparams: Line 2657: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} Line 2658: headers: Line 2659: Correlation-Id: {value: 'any string', required: false} Line 2655: signatures: [] Line 2656: urlparams: Line 2657: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} Line 2658: headers: Line 2659: Correlation-Id: {value: 'any string', required: false} > The "Correlation-Id" header is added implicitly, remove completely these tw Done Line 2660: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments/{networkattachment:id}|rel=update Line 2661: description: update the specified network attachment on the network interface Line 2662: request: Line 2663: body: Line 2663: body: Line 2664: parameterType: NetworkAttachment Line 2665: signatures: Line 2666: - mandatoryArguments: {} Line 2667: optionalArguments: { > Don't use the curly brackets, use indentation instead (two spaces per level Done Line 2668: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2669: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2670: ipv4.address: 'xs:string', Line 2671: ipv4.netmask: 'xs:string', Line 2676: property.value: 'xs:string' Line 2677: }, Line 2678: network_attachment.override_configuration: 'xs:boolean' Line 2679: } Line 2680: urlparams: {} > Don't add this ^ empty list of URL parameters. Done Line 2681: headers: Line 2682: Content-Type: {value: application/xml|json, required: true} Line 2683: Correlation-Id: {value: 'any string', required: false} Line 2684: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments|rel=add Line 2679: } Line 2680: urlparams: {} Line 2681: headers: Line 2682: Content-Type: {value: application/xml|json, required: true} Line 2683: Correlation-Id: {value: 'any string', required: false} > These two headers are added implicitly, remove them from this file. Done Line 2684: - name: /hosts/{host:id}/nics/{nic:id}/networkattachments|rel=add Line 2685: description: add a new network attachment to the network interface Line 2686: request: Line 2687: body: Line 2686: request: Line 2687: body: Line 2688: parameterType: NetworkAttachment Line 2689: signatures: Line 2690: - mandatoryArguments: {network_attachment.network.id: 'xs:string'} > Use indentation instead of curly brackets. Done Line 2691: optionalArguments: { Line 2692: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2693: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2694: ipv4.address: 'xs:string', Line 2687: body: Line 2688: parameterType: NetworkAttachment Line 2689: signatures: Line 2690: - mandatoryArguments: {network_attachment.network.id: 'xs:string'} Line 2691: optionalArguments: { > Same, indentation instead of curly brackets. Done Line 2692: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2693: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2694: ipv4.address: 'xs:string', Line 2695: ipv4.netmask: 'xs:string', Line 2700: property.value: 'xs:string' Line 2701: }, Line 2702: network_attachment.override_configuration: 'xs:boolean' Line 2703: } Line 2704: urlparams: {} > No empty lists. Done Line 2705: headers: Line 2706: Content-Type: {value: application/xml|json, required: true} Line 2707: Expect: {value: 201-created, required: false} Line 2708: Correlation-Id: {value: 'any string', required: false} Line 2704: urlparams: {} Line 2705: headers: Line 2706: Content-Type: {value: application/xml|json, required: true} Line 2707: Expect: {value: 201-created, required: false} Line 2708: Correlation-Id: {value: 'any string', required: false} > These three headers are added implicitly, remove them from this file. needinfo: even 201-created is added automatically to all rel=add methods? Line 2709: - name: /hosts/{host:id}/networkattachments|rel=get Line 2710: description: get the list of network attachments of the host Line 2711: request: Line 2712: body: Line 2710: description: get the list of network attachments of the host Line 2711: request: Line 2712: body: Line 2713: parameterType: null Line 2714: signatures: [] > Remove these ^ three lines. Done Line 2715: urlparams: Line 2716: max: {context: matrix, type: 'xs:int', value: 'max results', required: false} Line 2717: headers: {} Line 2718: - name: /hosts/{host:id}/networkattachments/{networkattachment:id}|rel=get Line 2713: parameterType: null Line 2714: signatures: [] Line 2715: urlparams: Line 2716: max: {context: matrix, type: 'xs:int', value: 'max results', required: false} Line 2717: headers: {} > Remove this empty list of headers, it is the default already. Done Line 2718: - name: /hosts/{host:id}/networkattachments/{networkattachment:id}|rel=get Line 2719: description: get the details of the specified network attachment of the host Line 2720: request: Line 2721: body: Line 2721: body: Line 2722: parameterType: null Line 2723: signatures: [] Line 2724: urlparams: {} Line 2725: headers: {} > Remove these ^ five lines. needinfo: empty request should stay? if so, why? Line 2726: - name: /hosts/{host:id}/networkattachments/{networkattachment:id}|rel=delete Line 2727: description: delete the specified network attachment from the host Line 2728: request: Line 2729: body: Line 2727: description: delete the specified network attachment from the host Line 2728: request: Line 2729: body: Line 2730: parameterType: null Line 2731: signatures: [] > Remove these three lines. Done Line 2732: urlparams: Line 2733: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} Line 2734: headers: Line 2735: Correlation-Id: {value: 'any string', required: false} Line 2731: signatures: [] Line 2732: urlparams: Line 2733: async: {context: matrix, type: 'xs:boolean', value: true|false, required: false} Line 2734: headers: Line 2735: Correlation-Id: {value: 'any string', required: false} > Remove this header. Done Line 2736: - name: /hosts/{host:id}/networkattachments/{networkattachment:id}|rel=update Line 2737: description: update the specified network attachment on the host Line 2738: request: Line 2739: body: Line 2739: body: Line 2740: parameterType: NetworkAttachment Line 2741: signatures: Line 2742: - mandatoryArguments: {} Line 2743: optionalArguments: { > Use indentation instead of curly brackets. Done Line 2744: network_attachment.host_nic.id: 'xs:string', Line 2745: network_attachment.host_nic.name: 'xs:string', Line 2746: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2747: network_attachment.ip_configuration.ipv4s.ipv4--COLLECTION: { Line 2757: } Line 2758: urlparams: {} Line 2759: headers: Line 2760: Content-Type: {value: application/xml|json, required: true} Line 2761: Correlation-Id: {value: 'any string', required: false} > Remove the empty list of URL parameters and the headers. Done Line 2762: - name: /hosts/{host:id}/networkattachments|rel=add Line 2763: description: add a new network attachment to the host Line 2764: request: Line 2765: body: Line 2764: request: Line 2765: body: Line 2766: parameterType: NetworkAttachment Line 2767: signatures: Line 2768: - mandatoryArguments: {network_attachment.network.id: 'xs:string'} > Use indentation instead of curly brackets. Done Line 2769: optionalArguments: { Line 2770: network_attachment.host_nic.id: 'xs:string', Line 2771: network_attachment.host_nic.name: 'xs:string', Line 2772: network_attachment.ip_configuration.ipv4s.boot_protocol: 'xs:string', Line 2784: urlparams: {} Line 2785: headers: Line 2786: Content-Type: {value: application/xml|json, required: true} Line 2787: Expect: {value: 201-created, required: false} Line 2788: Correlation-Id: {value: 'any string', required: false} > Remove the empty list of URL parameters and the headers. Done Line 2789: - name: /hosts/{host:id}/permissions|rel=get Line 2790: description: get the list of permissions for the host Line 2791: request: Line 2792: body: https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendNetworkAttachmentResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendNetworkAttachmentResource.java: Line 3: import org.ovirt.engine.api.resource.NetworkAttachmentResource; Line 4: import org.ovirt.engine.core.common.action.NetworkAttachmentParameters; Line 5: import org.ovirt.engine.core.common.action.VdcActionParametersBase; Line 6: import org.ovirt.engine.core.common.action.VdcActionType; Line 7: import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment; > When there are name clashes import the api.model class and use the fully qu Done 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 23: @Override Line 24: public org.ovirt.engine.api.model.NetworkAttachment get() { Line 25: org.ovirt.engine.api.model.NetworkAttachment model = Line 26: performGet(VdcQueryType.GetNetworkAttachmentById, new IdQueryParameters(guid), parent.getParentClass()); Line 27: parent.addParents(model); > The result of the "addParents" method may not be the same object that you p Done Line 28: return addLinks(model); Line 29: } Line 30: Line 31: @Override Line 47: } Line 48: Line 49: @Override Line 50: protected org.ovirt.engine.api.model.NetworkAttachment addParents(org.ovirt.engine.api.model.NetworkAttachment model) { Line 51: return getParent().addParents(model); > Why not "parent.addParents(model)"? Done Line 52: } Line 53: Line 54: protected class UpdateParametersProvider Line 55: implements ParametersProvider<org.ovirt.engine.api.model.NetworkAttachment, NetworkAttachment> { https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendNetworkAttachmentsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/AbstractBackendNetworkAttachmentsResource.java: Line 8: import org.ovirt.engine.api.model.NetworkAttachments; Line 9: import org.ovirt.engine.api.resource.NetworkAttachmentsResource; Line 10: import org.ovirt.engine.core.common.action.NetworkAttachmentParameters; Line 11: import org.ovirt.engine.core.common.action.VdcActionType; Line 12: import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment; > Import the api.model class and use the fully qualified name of the backend Done Line 13: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 14: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 15: import org.ovirt.engine.core.compat.Guid; Line 16: Line 76: Line 77: private NetworkAttachments mapCollection(List<NetworkAttachment> networkAttachments) { Line 78: NetworkAttachments collection = new NetworkAttachments(); Line 79: for (NetworkAttachment attachment : networkAttachments) { Line 80: org.ovirt.engine.api.model.NetworkAttachment entity = populate(map(attachment), attachment); > The name "entity" here may be misleading, one may think that it is a backen Done Line 81: collection.getNetworkAttachments().add(addLinks(entity, getParentClass())); Line 82: } Line 83: Line 84: return collection; https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNetworkAttachmentsResource.java: Line 4: Line 5: import org.ovirt.engine.api.model.BaseResource; Line 6: import org.ovirt.engine.api.model.Host; Line 7: import org.ovirt.engine.api.resource.NetworkAttachmentResource; Line 8: import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment; > Import the api.model class and use the fully qualified name of the backend Done Line 9: import org.ovirt.engine.core.common.queries.IdQueryParameters; Line 10: import org.ovirt.engine.core.common.queries.VdcQueryType; Line 11: import org.ovirt.engine.core.compat.Guid; Line 12: https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNicNetworkAttachmentsResource.java File backend/manager/modules/restapi/jaxrs/src/main/java/org/ovirt/engine/api/restapi/resource/BackendHostNicNetworkAttachmentsResource.java: Line 53: Line 54: if (!nicId.equals(hostNicGuid)) { Line 55: //TODO MM: add message. Line 56: return Response.status(Response.Status.BAD_REQUEST).build(); Line 57: } > I think I don't understand this. We are just checking that the given host N POSTing to …/networkattachments creates new binding of network to nic. There are two ways how to do that: …/api/hosts/{host:id}/networkattachments and …/api/hosts/{host:id}/nics/{nic:id}/networkattachments 'id' a a part of request is needed for first path, since it's not specifiable elsewhere. While in second it's optional in request. But if specified in request, it better be the same id as one in path. It can be solved properly via removing nicId from request for second resource, but it will probably require more coding in api.xsd and *Resource classes. Line 58: } else { Line 59: HostNIC hostNIC = new HostNIC(); Line 60: hostNIC.setId(nicId.toString()); Line 61: attachment.setHostNic(hostNIC); https://gerrit.ovirt.org/#/c/32775/23/backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/NetworkAttachmentMapper.java File backend/manager/modules/restapi/types/src/main/java/org/ovirt/engine/api/restapi/types/NetworkAttachmentMapper.java: Line 8: import org.ovirt.engine.api.restapi.utils.CustomPropertiesParser; Line 9: import org.ovirt.engine.api.restapi.utils.GuidUtils; Line 10: import org.ovirt.engine.core.common.businessentities.network.IpConfiguration; Line 11: import org.ovirt.engine.core.common.businessentities.network.NetworkAttachment; Line 12: import org.ovirt.engine.core.common.businessentities.network.NetworkBootProtocol; > Import the api.model classes and use the fully qualified names of the backe Done Line 13: Line 14: public class NetworkAttachmentMapper { Line 15: Line 16: @Mapping(from = org.ovirt.engine.api.model.NetworkAttachment.class, to = NetworkAttachment.class) Line 41: } Line 42: Line 43: if (model.isSetIpConfiguration() && model.getIpConfiguration().isSetIPv4Configuration()) { Line 44: IPv4S ipv4configuration = model.getIpConfiguration().getIPv4Configuration(); Line 45: if (ipv4configuration != null) { > This will never be null, you already checked in the "if". Done Line 46: entity.setIpConfiguration(new org.ovirt.engine.core.common.businessentities.network.IpConfiguration()); Line 47: Line 48: if (ipv4configuration.isSetBootProtocol()) { Line 49: NetworkBootProtocol networkBootProtocol = -- To view, visit https://gerrit.ovirt.org/32775 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Id6ac91eea1000f7fdf6105777343a1ac1c77368d Gerrit-PatchSet: 23 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Moti Asayag <masa...@redhat.com> Gerrit-Reviewer: Juan Hernandez <juan.hernan...@redhat.com> Gerrit-Reviewer: Martin Mucha <mmu...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org 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
