Re: [OPSAWG] AD review of draft-ietf-opsawg-sap-09

["Rob Wilton (rwilton)" <[e-mail]>]

Hi Med,

Comments inline below ...

I've snipped out anything that we have already reached agreement on.


> -----Original Message-----
> From: [e-mail]
> <[e-mail]>
> Sent: 23 September 2022 14:04
> To: Rob Wilton (rwilton) <[e-mail]>; draft-ietf-opsawg-
> [e-mail]; [e-mail]
> Subject: RE: AD review of draft-ietf-opsawg-sap-09
> 
> Hi Rob,
> 
> Thank you for the review. The changes can be tracked at:
> https://tinyurl.com/sap-latest
> 
> Please note that I made a change to better allow for reuse of the SAP
> information in other modules (this can be tracked here:
> https://github.com/IETF-OPSAWG-
> WG/lxnm/commit/e8b406d7fad5225dd84ad7ff03f6da446eae6736).

Okay.


> 
> Please see inline for more context.
> 
> Cheers,
> Med
> 
> > -----Message d'origine-----
> > De : Rob Wilton (rwilton) <[e-mail]>
> > Envoyé : vendredi 23 septembre 2022 14:01
> > À : [e-mail]; [e-mail]
> > Objet : AD review of draft-ietf-opsawg-sap-09
> >
> > Hi authors, WG,
> >
> > Thank you for this document.  I also think that this document is
> > well written and in good shape, and I mostly found the
> > explanations and examples clear.  There were two specific points
> > that I found slightly confusing related to differentiating between
> > SAPs in use, and those that are not, and also parent interfaces
> > also be listed as SAPs, details below.
> 
> [Med] Thanks
> 
> >
> > Moderate level comments:
> >
> > (1) p 10, sec 5.  SAP Module Tree Structure
> >
> >       SAPs that are associated with the interfaces that are
> > directly
> >       hosting services, interfaces that are ready to host per-
> > service
> >       sub-interfaces (but not yet activated), or service that are
> >       already instantiated on sub-interfaces are listed as SAPs.
> >
> > >From the model, is isn't clear to me how different differentiates
> > between a SAP that has been pre-provisioned to potentially be used
> > for a service in future, v.s., one is that is actively
> > provisioned.
> 
> [Med] This is inferred from the service-status=down for these SAPs.

So, thinking of this at device level configuration there is effectively 3 levels of configuration/activation (at least for L2VPNs):

(1) A sub-interface is configured, but without any IP address or VRF (for L3VPN), or without being attached to an L2VPN or Bridge Domain (for L2VPNs).  I.e., the sub-interface isn't connected anyway.
(2) The sub-interface is configured and connected into a bridge domain or P2P L2VPN but that service is down (e.g., perhaps because the AC at the remote end of the L2VPN is physically down).
(3) The sub-interface is configured and connected into a bridge domain or P2P L2VPN and that service is up.

I think that you are saying that you regard that both (1) and (2) would be indicated by service-status=down?  Would it be worth expanding the description at all to make this more explicit?  But I'm still not convinced this will be sufficient (e.g., see my following response below related to the example for selecting a service).

> 
> 
>   I think that it would be helpful if these two cases
> > can be clearly distinguished.  Note, I have made a similar comment
> > related to appendix D about identifying a "free" SAP.
> 
> [Med] Added this NEW to the appendix:
> 
> "SAPs that are ready to host per-service (but not yet activated) are identified
> by the "service-status" set to "ietf-vpn-common:op-down"."

But how do you distinguish between a SAP that hasn't been provisioned yet to a service vs a SAP that has been provisioned but the service is down?  E.g., trying to find a free SAP just by looking for a SAP with a service-status of op-down doesn't appear to be sufficient on its own.



> 
> >
> >
> > (2) p 28, sec Appendix B.  A Simple Example of SAP Network Model:
> > Node Filter
> >
> 
> [Med] Please note "Simple" in the title :-)

OK, noted. ;-)


> 
> >    A service orchestrator can query what services are provided on
> > which
> >    SAPs of PE1 from the network controller by sending, e.g., a GET
> >    RESTCONF request.  Figure 9 shows the body of the RESTCONF
> > response
> >    that is received from the network controller.
> >
> > In this example, you have GE0/6/4 as being ready to host SAPs, but
> > I could equally imagine that given GE0/6/4 is just a UNI, then you
> > may not want the parent interface to be available to host SAPs
> > directly at all.  I.e., it may always the case that sub-interfaces
> > are used as SAPs.  Hence, did you consider whether it makes sense
> > for there to be a different category of SAP service-types for
> > UNI's and NNI's that don't host services directly on the
> > interface, but always on sub-interfaces?
> 
> [Med] Yes, we do need this for the LxVPN case.

It's still not clear to me.  E.g., in the example, GE0/6/4 is listed as an VPLS SAP and as an L3VPN SAP, and it isn't obvious that it is actually either of those.  I.e., really it is just a parent to sub-interfaces that represent the VPLS and L3VPN SAPs.

Hence, I was asking whether you considered putting GE0/6/1 into a different service/sap list (e.g., of service-type "sap-parent"), or similar.  This would mean that the interface would be listed only once rather than under both VPLS and L3VPN.


> 
>   Related to this, it
> > feels slightly strange to me to have GE0/6/4 listed under both
> > l3vpn and vpls SAP lists.
> 
> [Med] Actually there are attached to distinct sub-interfaces:

So, my question is really whether, in this case, GE0/6/4 is actually a SAP at all, or whether really it is only the child sub-interfaces of GE0/6/4 that are actually SAPs?

Possibly adding the "ready-child-sap" resolves this ambiguity.  Possibly, I would have a chosen a different name and description for this leaf (e.g., "sap-parent") that indicates that this node can act as a parent to other SAPs.


> 
>    Also, let us assume that, for
>    the SAPs that are associated with the physical interface "GE0/6/4",
>    VPLS and L3VPN services are activated on the two sub-interfaces
>    "GE0/6/4.1" and "GE0/6/4.2", respectively.
> 
> Updated the example to align with this text.
> 
>   Having the same information twice in
> > the data model introduces the risk that a device could export
> > inconsistent information and hence it hard for the customer to
> > know which data is accurate.  I can understand why having it twice
> > might be convenient, but having an indication that it is actually
> > just a container node for SAPs rather than a SAP itself might be
> > better.
> >
> >
> >
> > Minor level comments:
> >
> 
> >
> > (5) p 10, sec 5.  SAP Module Tree Structure
> >
> >    These service types build on the types that are already defined
> > in
> >    [RFC9181] and additional types that are defined in this
> > document.
> >    Other service types can be defined in future YANG modules, if
> > needed.
> >
> > In future YANG modules, or perhaps also future versions of the
> > YANG model defined in this document?
> >
> 
> [Med] Changed to "future YANG modules (including future revisions of the
> YANG model defined in this document)"

Okay.

> 
> >
> > (6) p 11, sec 5.  SAP Module Tree Structure
> >
> >       This data node can be used, for example, to decide whether
> > an
> >       existing SAP can be (re)used to host a service or if a new
> > sub-
> >       interface has to be instantiated.
> >
> > So, is this field effectively also being used to determine if the
> > SAP is in use?
> >
> 
> [Med] No, this is determined by 'sap-status' (completed with 'service-
> status').
> 
> 
> >
> > (7) p 12, sec 5.  SAP Module Tree Structure
> >
> >       When both a sub-interface and its parent interface are
> > present,
> >       the status of the parent interface takes precedence over the
> >       status indicated for the sub-interface.
> >
> > This seems strange to me.  E.g., if the parent-interface was up,
> > but the sub-interface was administratively down then would you be
> > expected to report the sap-status as up?  I would think that this
> > should always be reporting the sub-interface state, but that the
> > sub-interface state should inherit
> >
> 
> [Med] Good catch. "but the parent interface is disabled" was missing from
> the OLD text.

Okay.


> 
> >
> > (8) p 16, sec 6.  SAP YANG Module
> >
> >      identity logical {
> >        base interface-type;
> >        description
> >          "Refers to a logical sub-interface that is typically
> >           used to bind a service. This type is used only
> >           if none of the other logical types can be used.";
> >      }
> >
> > Perhaps clarify what you mean by "other logical types".  E.g., do
> > you just mean loopback or irb, or does this include local-bridge
> > as well?
> >
> >
> 
> [Med] We meant the other non-PHY types already defined. Changed to:
> 
>       "Refers to a logical sub-interface that is typically
>        used to bind a service. This type is used only
>        if none of the other types (i.e., loopback, lag,
>        irb, or local-bridge) can be used.";

Okay.  As a minor suggestion, you could possibly change "types" to "other more specific types".


> 
> 
> > (9) p 17, sec 6.  SAP YANG Module
> >
> >          leaf peer-sap-id {
> >            type string;
> >            description
> >              "Indicates an identifier of the peer's termination
> >               identifier (e.g., Customer Edge (CE)). This
> >               information can be used for correlation purposes,
> >               such as identifying the SAP that is attached to
> >               an endpoint that is provided in a service request.";
> >          }
> >
> > Would it be helpful to indicate that the "peer-sap-id" is not
> > necessarily the same as the peer's sap_id for that same attachment
> > circuit endpoint?  E.g., it appears to differ in your example in
> > Appendix C.
> >
> 
> [Med] Given that this is used for correlation purposes, the value enclosed in
> peer-sap-id is useful when it is known to the peer. Typically, that value will be
> indicated as the service delivery point. I tend to leave the description as it is.

Okay, the reason that I queried this was that in your appendix C, you don't appear to follow this.  E.g., the peer_sap_id for "sap#12" is given as "asbr-b1", but that is probably not what the peer sap id would actually be (or otherwise all the "peer_sap_id" names would collide).  Perhaps this example is an exception to the rule, but then that would be worth highlighting in the text that describes the example.



> 
> >
> > (10) p 19, sec 8.  Security Considerations
> >
> >    *  /nw:networks/nw:network/nw:node/sap:service-type/sap:sap
> >
> > Should this be:
> > /nw:networks/nw:network/nw:node/sap:service/sap:sap ?
> >
> 
> [Med] This should be
> "/nw:networks/nw:network/nw:node/sap:service/sap:service-
> type/sap:sap"

According to the tree diagram, I don't think this path can be right because service-type is a terminal leaf node. I.e., "sap" is augmented onto "service" not "service-type".



> 
> >
> > (11) p 20, sec 8.  Security Considerations
> >
> >    *  /nw:networks/nw:network/nw:node/sap:service-type/sap:sap
> >
> > Should this be:
> > /nw:networks/nw:network/nw:node/sap:service/sap:sap ?

This will need to be similarly fixed (if we agree).


> >
> >
> > (12) p 25, sec Appendix A.  A Simplified SAP Network Example
> >
> >    An example of a SAP topology that is reported by a network
> > controller
> >    is depicted in Figure 7.  This example echoes the topology
> > shown in
> >    Figure 4.  Only a minimum set of information is provided for
> > each
> >    SAP.
> >
> > I'm surprised that service-status isn't reported for the saps that
> > only have names.  Perhaps that information is elided to make the
> > examples shorter, but it may be helpful to clarify that.  E.g.,
> > perhaps expand "Only a minimum set of information is provided for
> > each
> >    SAP." to be explicit about SAP information that has been elided
> > (e.g., attachment interface, interface type, role).  Or perhaps a
> > bit more of an explanation about how different SAPS are being
> > modelled here would be helpful
> 
> [Med] That was the intent of having "Only a minimum set of information is
> provided for each SAP.", but I hear this is no that clear. Added the status and
> a mention about the nodes that are listed for the sake of illustration.

Thanks.  The updated text that you have in your latest looks better.

Thanks,
Rob