Re: [Lsr] AD Review of draft-ietf-ospf-mpls-elc-12

[Peter Psenak <[e-mail]>]

Hi Alvaro,

thanks for your comments, please see inline:

On 29/02/2020 06:03, Alvaro Retana wrote:
> Dear authors:
> 
> This is my review of draft-ietf-ospf-mpls-elc-12.  I reviewed this
> document alongside draft-ietf-isis-mpls-elc-10, so many comments are
> the same/similar.  Thank you for the work in both of them!
> 
> I will progress both documents together, so I will wait for both of
> them to address my comments before starting their IETF LC.
> 
> Thanks!!
> 
> Alvaro.

[Line numbers from idnits.]

...
14	  Signaling Entropy Label Capability and Entropy Readable Label-stack
15	                            Depth Using OSPF

[minor] s/Label-stack/Label

##PP
done

16	                      draft-ietf-ospf-mpls-elc-12

18	Abstract

20	   Multiprotocol Label Switching (MPLS) has defined a mechanism to load-
21	   balance traffic flows using Entropy Labels (EL).  An ingress Label
22	   Switching Router (LSR) cannot insert ELs for packets going into a
23	   given tunnel unless an egress LSR has indicated via signaling that it
24	   has the capability to process ELs, referred to as Entropy Label
25	   Capability (ELC), on that tunnel.  In addition, it would be useful
26	   for ingress LSRs to know each LSR's capability of reading the maximum
27	   label stack depth and performing EL-based load-balancing, referred to
28	   as Entropy Readable Label Depth (ERLD).  This document defines a
29	   mechanism to signal these two capabilities using OSPF and OSPFv3.
30	   These mechanism is particularly useful in the environment where
31	   Segment Routing (SR) is used, where label advertisements are done via
32	   protocols like OSPF and OSPFv3.

[nit] s/given tunnel/given Label Switched Path (LSP)

[nit] s/as Entropy Label Capability/as the Entropy Label Capability

[nit] s/capability of reading/capability for reading

[minor] s/OSPF and OSPFv3/OSPFv2 and OSPFv3/g


##PP
all done

[minor] Add some text in the Introduction pointing at the generic use
of "OSPF" to mean both OSPFv2 and OSPFv3.

##PP
added it to the Terminology section.

[minor] "protocols like OSPF..."  That last sentence sounds as if
there were other options; for example advertise labels with IS-IS and
then use the extensions here.  It's just a minor point, but I think
that maybe that last sentence is not needed.

##PP
removed


...
83	1.  Introduction
...
96	   In addition, in the cases where stacked LSPs are used for whatever
97	   reasons (e.g., SR-MPLS [I-D.ietf-spring-segment-routing-mpls]), it
98	   would be useful for ingress LSRs to know each intermediate LSR's
99	   capability of reading the maximum label stack depth and performing
100	   EL-based load-balancing.  This capability, referred to as Entropy
101	   Readable Label Depth (ERLD) as defined in
102	   [I-D.ietf-mpls-spring-entropy-label] may be used by ingress LSRs to
103	   determine the position of the EL label in the stack, and whether it's
104	   necessary to insert multiple ELs at different positions in the label
105	   stack.

[minor] s/stacked LSPs/LSPs
I'm assuming there's no special significance of "stacked" in this
case, as this is the only place in the document that it is used.

##PP
removed it

[nit] s/in the cases where LSPs are used for whatever reasons/in cases
where LSPs are used

##PP
done


107	2.  Terminology

109	   This document makes use of the terms defined in [RFC6790], [RFC7770]
110	   and [I-D.ietf-mpls-spring-entropy-label].

[minor] I'm not sure why rfc7770 is referenced here; what terminology
is needed from it?

##removed


112	   The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
113	   "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and
114	   "OPTIONAL" in this document are to be interpreted as described in
115	   [BCP14] [RFC2119] [RFC8174] when, and only when, they appear in all
116	   capitals, as shown here.

[major] s/[BCP14]/BCP 14

##PP
done


118	3.  Advertising ELC Using OSPF

120	   Even though ELC is a property of the node, in some cases it is
121	   advantageous to associate and advertise the ELC with the prefix.  In
122	   multi-area networks, routers may not know the identity of the prefix
123	   originator in a remote area, or may not know the capabilities of such
124	   originator.  Similarly, in a multi domain network, the identity of
125	   the prefix originator and its capabilities may not be known to the
126	   ingress LSR.

[nit] s/with the prefix/with a prefix

##PP
fixed

[minor] Is there a difference that are you trying to highlight between
multi-area and multi-domain?  The last two sentences seem redundant to
me; using "domain" should be enough.


##PP
Multi-area and multi-domain are two different cases. I believe it is
important to keep both in the text. (Agreed during the ISIS draft review)


128	   If a router has multiple line cards, the router MUST NOT announce ELC
129	   unless all of its line-cards are capable of processing ELs.

131	   If the router supports ELs on all of its line cards, it SHOULD
132	   advertise the ELC with every local host prefix it advertises in OSPF.

[major] From a general router architecture point of view, I understand
what you mean by line-card.  But, strictly speaking from a
specification point of view, what is a line-card?  Would using
"interface" instead be an acceptable generalization?

##PP
changed to "interface"


[major] "it SHOULD advertise the ELC for every local host prefix"  If
ELs are supported in all the interfaces, when would a router not set
the ELC?  IOW, why is "MUST" not used instead of "SHOULD"?


##PP
advertising ELC is not a MUST. It's an optional information that the
originator should advertise, but if it is not, it is not going to break
anything really. (agreed during the ISIS draft review)


[major/related] The two paragraphs seem to be redundant -- I think
that only the second one is needed; suggestion (assuming my
interpretation of the questions above):


##PP
if we only keep the second sentence, the reader may not know what to do
in case where some interfaces do not support ELC. I believe it is good
to keep both. (agreed during the ISIS draft review)


    If a router supports ELs on all of its interfaces, it MUST set the 
E-flag
    (ELC Flag) for every attached prefix it advertises.


134	   When an OSPF Area Border Router (ABR) advertises the prefix to the
135	   connected area based on the intra-area or inter-area prefix that is
136	   reachable in some other area, it MUST preserve the ELC signalling for
137	   such prefix.

[] Suggestion>

    When an OSPF Area Border Router (ABR) distributes information between
    connected areas it MUST preserve the ELC setting

##PP
done


139	   When an OSPF Autonomous System Boundary Router (ASBR) redistributes
140	   the prefix from another instance of the OSPF or from some other
141	   protocol, it SHOULD preserve the ELC signaling for the prefix.  The
142	   exact mechanism used to exchange ELC between protocol instances on
143	   the ASBR is outside of the scope of this document and is
144	   implementation specific.

[nit] s/redistributes the prefix/redistributes a prefix

##PP
done

[minor] s/ELC signaling/ELC setting

[nit] s/ and is implementation specific./.

##PP
done


...
156	3.2.  Advertising ELC Using OSPFv3

158	   [RFC5340] defines the OSPFv3 PrefixOptions that are advertised along
159	   with the prefix.  A new bit in the OSPFV3 PrefixOptions is used to
160	   signal the ELC for the prefix:

[minor] s/[RFC5340] defines the OSPFv3 PrefixOptions that are
advertised along with the prefix./[RFC5340] defines the OSPFv3
PrefixOptions field to indicate capabilities associated with a prefix.

##PP
done

[nit] s/OSPFV3 PrefixOptions/OSPFv3 PrefixOptions field

##PP
done


...
165	4.  Advertising ERLD Using OSPF

[major] draft-ietf-mpls-spring-entropy-label says that "To advertise
an ERLD value, a SPRING router:  MUST be entropy label capable".  This
requirement must be translated to this document so that the ERLD is
only advertised if the ELC is also advertised.  I'm assuming that the
ERLD should be ignored if the ELC is not advertised -- but that should
be explicitly defined as well.  If the ELC is advertised, but the ERLD
isn't, what value should be assumed, 0?

##PP
I have resolved this the same way we did during the ISIS draft review.


167	   A new MSD (Maximum SID Depth) type of the Node MSD sub-TLV [RFC8476],
168	   called ERLD is defined to advertise the ERLD of a given router.  The
169	   scope of the advertisement depends on the application.

171	   Assignment of a MSD-Type for ERLD is defined in
172	   [I-D.ietf-isis-mpls-elc].

[minor] Instead of the last two paragraphs:

    The ERLD is advertised in a Node MSD sub-TLV [RFC8476] using the 
ERLD-MSD
    type defined in [I-D.ietf-isis-mpls-elc].

##PP
done


...
181	5.  Signaling ELC and ERLD in BGP-LS
...
186	   The ELC Flag included in the OSPFv2 Extended Prefix TLV and the
187	   OSPFv3 PrefixOptions, as defined in Section 3, is advertised using
188	   the Prefix Attribute Flags TLV (TLV 1170) of the BGP-LS IPv4/IPv6
189	   Prefix NLRI Attribute as defined in section 2.3.2 of
190	   [I-D.ietf-idr-bgp-ls-segment-routing-ext].

[nit] Suggestion>

    The ELC is advertised using the Prefix Attribute Flags TLV as defined in
    [I-D.ietf-idr-bgp-ls-segment-routing-ext].

##PP
done


192	   The ERLD MSD-type introduced for OSPF in Section 4 is advertised
193	   using the Node MSD TLV (TLV 266) of the BGP-LS Node NLRI Attribute as
194	   defined in section 3 of [I-D.ietf-idr-bgp-ls-segment-routing-msd].

[nit] Suggestion>

    The ERLD-MSD is advertised using the Node MSD TLV as defined in
    [I-D.ietf-idr-bgp-ls-segment-routing-msd].

##PP
done


196	6.  Acknowledgements

[major] Move this section to just before the References.


##PP
done

...
202	7.  IANA Considerations

204	   This document requests IANA to allocate one flag from the OSPFv2
205	   Extended Prefix TLV Flags registry:

207	      0x20 - E-Flag (ELC Flag)

209	   This document requests IANA to allocate one flag from the OSPFv3
210	   Prefix Options registry:

212	      0x04 - E-Flag (ELC Flag)

[major] IANA has already assigned the values.  Suggestion>

    Early allocation has been done by IANA for this document as follows:

    - Flag 0x20 in the OSPFv2 Extended Prefix TLV Flags registry has been
    assigned to the E-Flag (ELC Flag).  IANA is asked to update the 
registry to
    reflect the name used in this document: E-Flag (ELC Flag).

    - Bit 0x04 in the "OSPFv3 Prefix Options (8 bits)" registry has been
    assigned to the E-Flag (ELC Flag).  IANA is asked to update the 
registry to
    reflect the name used in this document: E-Flag (ELC Flag).

##PP
done


214	8.  Security Considerations

216	   The security considerations as described in [RFC7770] and
217	   [I-D.ietf-mpls-spring-entropy-label] are applicable to this document.

[minor] Why?  Also, I think that some of the other references should
be added here.  Suggestion:

    This document specifies the ability to advertise additional node
    capabilities using IS-IS and BGP-LS.  As such, the security 
considerations
    as described in [RFC5340], [RFC7770], [RFC7752], [RFC7684], [RFC8476],
    [I-D.ietf-idr-bgp-ls-segment-routing-ext],
    [I-D.ietf-idr-bgp-ls-segment-routing-msd] and
    [I-D.ietf-mpls-spring-entropy-label] are applicable to this document.

##PP
done


219	   Incorrectly setting the E flag (ELC capable) (during origination,
220	   inter-area advertisement or redistribution) may lead to black-holing
221	   of the traffic on the egress node.

[minor] s/E flag (ELC capable)/E flag

[major] "...may lead to black-holing of the traffic on the egress
node."  I'm not sure I understand how, but the ELC advertisement
should be accompanied by the ERLD-MSD -- see my questions at the
beginning of §4.

##PP
resolved as agreed for ISIS draft


223	   Incorrectly setting of the ERLD value may lead to poor load-balancing
224	   of the traffic.

[minor] "may lead to poor load-balancing"  If the ERLD is low, then
the traffic may not be load balanced at all...that is not "poor", it
is "0".

##PP
changed as agreed for ISIS draft

...
252	10.1.  Normative References

254	   [BCP14]    , <https://tools.ietf.org/html/bcp14>.

[major] A reference to BCP14 is not needed.  The ones to [RFC2119] and
[RFC8174] are enough.

#PP
removed

...
275	   [I-D.ietf-mpls-spring-entropy-label]
276	              Kini, S., Kompella, K., Sivabalan, S., Litkowski, S.,
277	              Shakir, R., and J. Tantsura, "Entropy label for SPRING
278	              tunnels", draft-ietf-mpls-spring-entropy-label-12 (work in
279	              progress), July 2018.

== Outdated reference: draft-ietf-mpls-spring-entropy-label has been
    published as RFC 8662\

##PP
done


281	   [I-D.ietf-spring-segment-routing-mpls]
282	              Bashandy, A., Filsfils, C., Previdi, S., Decraene, B.,
283	              Litkowski, S., and R. Shakir, "Segment Routing with MPLS
284	              data plane", draft-ietf-spring-segment-routing-mpls-22
285	              (work in progress), May 2019.

== Outdated reference: draft-ietf-spring-segment-routing-mpls has been
    published as RFC 8660

[minor] This reference can be Informative.

##PP
done

...
326	10.2.  Informative References

328	   [I-D.ietf-ospf-segment-routing-extensions]
329	              Psenak, P., Previdi, S., Filsfils, C., Gredler, H.,
330	              Shakir, R., Henderickx, W., and J. Tantsura, "OSPF
331	              Extensions for Segment Routing", draft-ietf-ospf-segment-
332	              routing-extensions-27 (work in progress), December 2018.

== Outdated reference: draft-ietf-ospf-segment-routing-extensions has been
    published as RFC 8665

##PP
done

thanks,
Peter

     [Lsr] AD Review of draft-ietf-ospf-mpls-elc-12  Alvaro Retana