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
_______________________________________________
Lsr mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/lsr
