Re: [Lsr] A review of draft-zhu-lsr-isis-sr-vtn-flexalgo-04

[Adrian Farrel <[e-mail]>]

All looks good.
I'll watch for the updated draft.
Best,
Adrian

-----Original Message-----
From: Dongjie (Jimmy) <[e-mail]> 
Sent: 17 May 2022 10:59
To: [e-mail]; [e-mail]
Cc: [e-mail]
Subject: RE: A review of draft-zhu-lsr-isis-sr-vtn-flexalgo-04

Hi Adrian, 

Thanks a lot for your detailed review. All your comments and suggestions
look good and we will produce a new revision to incorporate them. 

And please see replies to some points inline:

Best regards,
Jie

> -----Original Message-----
> From: Adrian Farrel [mailto:[e-mail]]
> Sent: Monday, May 16, 2022 7:22 PM
> To: [e-mail]
> Cc: [e-mail]
> Subject: FW: A review of draft-zhu-lsr-isis-sr-vtn-flexalgo-04
> 
> Hi LSR and draft authors,
> 
> I read this draft, and it seems to me that it provides a useful
transitional
> mechanism. It can obviously only support a relatively small number of VTNs
> (128 due to the limited number of Flex-Algos the network devices can
> support), but it looks to be a worthwhile first step because it can be
achieved
> with a very minor control plane extension.
> 
> Perhaps this document is a good first step while we work on a longer term
> and more scalable control plane solution. It would also give us the chance
to
> determine the level of interest in VTNs.

Indeed, this is exactly the purpose of this document.

> 
> My comments, below, are mainly editorial, but there are a couple of issues
> around the use of the E flag.
> 
> Best,
> Adrian
> 
> (PS. For those of you saying "What's a VTN?" the "Virtual Transport
> Network"
> is a network construct described in the TEAS WG to support Enhanced VPNs
> (https://datatracker.ietf.org/doc/draft-ietf-teas-enhanced-vpn/) and
network
> slicing
> (https://datatracker.ietf.org/doc/draft-ietf-teas-ietf-network-slices/)
> where it maps to a "Network Resource Partition".)
> 
> ===
> 
> As currently defined, I think this document needs to update RFC 8668. This
is
> because that RFC says that other flags in the flag field of the Parent L3
> Neighbor Descriptor in the L2 Bundle Member Attributes TLV "MUST be
> ignored".
> 
> That's easy enough to handle:
> - You add the "updates 8668" element to the XML
> - You add a final paragraph to the Abstract to say
>     This document updates RFC 8668 by defining a new flag in the Parent
>     L3 Neighbor Descriptor in the L2 Bundle Member Attributes TLV.
> - You add a final paragraph to the Introduction to say
>     This document updates [RFC8668] by defining a new flag in the Parent
>     L3 Neighbor Descriptor in the L2 Bundle Member Attributes TLV.
>     [RFC8668] states that all bit fields not defined in that document
>     "MUST be set to zero on transmission and ignored on receipt".
>     Section 3 of this document defines a new flag and specifies both
>     when it is set and how it should be processed.
> 
> However, a purist might point out that RFC 8668 should be fixed so that:
> 
> - The unused bits are defined as reserved for future use
> - The text should be updated to describe how the bits are set and handled
>   by implementations that don't understand them
> - There should be an IANA registry for the bits
> 
> You're not responsible for fixing RFC 8668, but you could if you want to.
> 
> Making this document an "update" is also important because of the absence
> of an IANA registry -- it is important to provide a way for people to
track the
> bits so that there is no collision when another bit is defined.
> 
> You could use definitely use this document to create the necessary IANA
> registry, even if you are not fixing the other parts of 8668.

Thanks for your suggestion, we will make this document an update of RFC 8668
to help track the new flag.


> 
> ---
> 
> Would be worth expanding "VTN" in the title to read...
>   Using Flex-Algo for Segment Routing based Virtual Transport Networks
> 
> ---
> 
> Abstract
> 
> The first mention of "Flex-Algo" needs to have some explanation of the
> concept. Not many words, but something like...
> 
> OLD
>    The topological constraints of a VTN can be defined using Flex-Algo.
> NEW
>    The topological constraints of a VTN can be defined using Flex-Algo,
>    a mechanism to provide distributed constraint-path computation.
> END

We will add some description about Flex-Algo. 


> ---
> 
> Abstract
> 
> "SR" should be spelled out as "Segment Routing (SR)"
> 
> ---
> 
> Abstract
> 
> s/L2 bundle/L2 bundles/
> 
> ---
> 
> 1.
> 
> The word "traditional" has other meanings in American English and we are
> now asked to avoid using it.
> 
> OLD
>    than that can be provided with traditional overlay VPNs.
> NEW
>    than that could be provided with existing overlay VPNs techniques.
> END

OK, will make the change accordingly.

> 
> ---
> 
> 1.
> 
> s/resource-aware SIDs/resource-aware segment identifiers (SIDs)/ s/With
> segment routing/With a segment routing/ s/Segment Identifiers (SIDs) can
> be used/SIDs can be used/ s/using control plane/using the control plane/
> s/scalable Segment Routing (SR)/scalable SR/ s/a unique Flex-Algo/a unique
> Flex-Algo [I-D.ietf-lsr-flex-algo]/
> 
> ---
> 
> Section 1 has just one sentence on the purpose and content of this
> document.
> 
>    This document
>    describes a mechanism to build the SR based VTNs using SR Flex-Algo
>    and IGP L2 bundle with minor extensions.
> 
> This text is fine, but rather limited.
> I suggest:
> - Make it a separate paragraph so that it stands out.
> - Add at least two more sentences describing what is found in this
>   document. Probably you can just summarise what the mechanism is.
> - Describe the purpose and intended use of the mechanism.
> 

We will expand this with a few more sentences.


> ---
> 
> 1.1
> 
> The boilerplate here is slightly wrong. Should read...
> 
>    The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL
> NOT",
>    "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED",
> "MAY", and
>    "OPTIONAL" in this document are to be interpreted as described in BCP
>    14 [RFC2119] [RFC8174] when, and only when, they appear in all
>    capitals, as shown here.
> 
> ---
> 
> 3.
> 
> s/can be allocated with a set/can be allocated a set/
> 
> ---
> 
> 3.
> 
> OLD
>    In order to perform constraint
>    based path computation for each VTN on network controller and the
>    ingress nodes, the resource attribute of each VTN also needs to be
>    advertised.
> NEW
>    In order for a network controller or the ingress nodes to perform
>    constraint based path computation for each VTN, the resource
>    attributes of each VTN need to be advertised.
> END
> 
> ---
> 
> 3.
> 
> s/resource attribute of the VTN/resource attributes of the VTN/
> 
> ---
> 
> 3.
> 
> OLD
>    The layer-3 link may or may not be a bundle of layer-2 links, as long
>    as it has the capability of partitioning the link resources into
>    different subsets for different VTNs it participates in.
> NEW
>    The layer-3 link must have the capability of partitioning the link
>    resources into different subsets for the different VTNs it
>    participates in.  It may or may not be a bundle of layer-2 links to
>    achieve this.
> END
> 
> ---
> 
> 3.
> 
> s/set of link resource allocated/set of link resources allocated/ s/the
Parent
> L3 link are used/the Parent L3 link is used/
> 
> ---
> 
> 3.
> 
> Add to the paragraph that begins "E flag:" ...
> 
>    Note that legacy implementations of [RFC8668] will set the E flag to
>    zero (clear) meaning that load balancing among component links is the
>    default behavior. Further, when a legacy implementation receives an
>    E flag that is set, it will ignore the flag and so will assume that
>    load balancing among component links is allowed even when the sender
>    has requested it to not be used.
> 
> NOTE WELL! If this is not the behaviour you want to see, you need to do
> something different with the E flag.

Yes, a legacy node will ignore this Flag and perform load balancing among
the component links. While since Flex-Algo is used to control the set of
nodes involved in a VTN, only the nodes which support the extension will
participate in the Flex-Algo for VTN.


> 
> ---
> 
> 3.
> 
>    For each virtual or physical layer-2 member link, the TE attributes
>    defined in [RFC5305] such as the Maximum Link Bandwidth and Admin
>    Groups SHOULD be advertised using the mechanism as defined in
>    [RFC8668].
> 
> a. You need to say why an implementation might choose to not do this
>    (to explain your use of SHOULD), what the consequences would be, and
>    what it might do instead.
> 
> b. s/[RFC5305]/[RFC5305],/
> 
> c. s/Groups/Groups,/

In RFC 8668, the TE attributes of the layer-2 member link are optional
attributes. In this VTN scenario, the admin groups (color) is required for
the correlation between the Flex-Algo specific forwarding entries and the
member link. The bandwidth attribute is optional and may be needed in the
constraint based path computation performed by the network controller or the
ingress nodes. We will correct the requirement language usage. 


> 
> ---
> 
> 3.
> 
>    The SR-MPLS Adj-SIDs or SRv6 End.X SIDs associated with
>    each of the virtual or physical Layer-2 member links SHOULD also be
>    advertised according to [RFC8668] and [I-D.dong-lsr-l2bundle-srv6].
> 
> You need to say why an implementation might choose to not do this (to
> explain your use of SHOULD), what the consequences would be, and what it
> might do instead.

The SR SIDs associated with the layer-2 member links are required in the
mechanism. We will replace the "SHOULD" with "MUST". 


> 
> ---
> 
> 3.
> 
>    In order to correlate the virtual or physical layer-2 member links
>    with the Flex-Algo ID which is used to identify the VTN, each VTN
>    SHOULD be assigned with a unique Admin Group (AG) or Extended Admin
>    Group (EAG), and the virtual or physical layer-2 member links
>    associated with this VTN SHOULD be configured with the AG or EAG
>    assigned to the VTN.  The AG or EAG of the parent layer-3 link SHOULD
>    be set to the union of all the AGs or EAGs of its virtual or physical
>    layer-2 member links.
> 
> I think the three instances of "SHOULD" here can be:
> s/SHOULD be/is/
> s/SHOULD be/are/
> s/SHOULD be/is/
> 
> ---
> 
> 3.
> 
> s/VTN, It/VTN, it/
> 
> ---
> 
> 4.
> 
> s/For SR-MPLS data plane/For the SR-MPLS data plane/
> 
> ---
> 
> 4.
> 
>    The Adj-SIDs associated
>    with the virtual or physical member links of a VTN MAY be used with
>    the prefix-SIDs of the same VTN together to build SR-MPLS TE paths
>    with the topological and resource constraints of the VTN.
> 
> I recommend s/MAY/can/
> 
> Similarly in
> 
>    The
>    End.XU SIDs associated with the virtual or physical member links of a
>    VTN MAY be used with the SRv6 Locator prefix of the same VTN together
>    to build SRv6 paths with the topological and resource constraints of
>    the VTN.
> 
> ---
> 
> 4.
> 
> s/For SRv6 data plane/For the SRv6 data plane/
> 
> ---
> 
> 5.
> 
> OLD
>    which is related to the number of Flex-Algos defined NEW
>    which is related to the maximum number of Flex-Algos supported END
> 
> OLD
>    described in [I-D.dong-teas-nrp-scalability].
> NEW
>    found in [I-D.dong-teas-nrp-scalability].
> END

Thanks for catching this, we will update the reference in next revision.