[Gen-art] Gen-art LC review of draft-ietf-mpls-mldp-node-protection-05

[Elwyn Davies <[e-mail]>]

I am the assigned Gen-ART reviewer for this draft. The General Area
Review Team (Gen-ART) reviews all IETF documents being processed
by the IESG for the IETF Chair.  Please treat these comments just
like any other last call comments.

For more information, please see the FAQ at

<http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-mpls-mldp-node-protection-05
Reviewer: Elwyn Davies
Review Date: 2015/08/28
IETF LC End Date: 2015/09/08
IESG Telechat date: (if known) -

Summary:
Almost ready.  The distinction that (I believe) is the case, between the 
need for unidirectional bypass LDPs in the non-root case and 
bidirectional (or two unidirectional) ones in the root case appears to 
merit some additional explanation.  Otherwise, there are a couple of 
minor issues  that are only just above the level of nits.  In 
particular, there doesn't seem to be any explanation of why the PLRs and 
MPTs have to be directly connected to the protected node (RFC 6388 is 
happy to find non-directly connected nodes) and I cannot see why direct 
connection is vital to this specification.  This may of course because I 
am not an MPLS afficionado!  There are, however, a considerable number 
of language nits which I have documented - another editorial pass by a 
reviewer with English mother tongue after fixing the nits I found would 
probably help.

Major issues:
None.

Minor issues:
s1:  This specification uses LDP capability negotiation (RFC 5561). The 
introduction should be clear that nodes involved in the node protection 
scheme MUST support RFC 5561 negotiations.

s1, para 2:  There is a very dangerous slang phrase here: "most ... 
should just work"!   Does this have the literal meaning that there are 
some circumstances in which they might NOT work and/or there are other 
capabilities that aren't enabled - if so, what are they? Or, I guess 
more likely, the slang meaning that using this method will allow these 
capabilities to work without additional effort.  Please use a non-slang 
phrase and be more precise as to what will/won't work.

s2.1:
>     The upstream LSR in figure 1 is LSR1
>     because it is the ***first hop*** along the shortest path to reach the root
>     address.
Does it have to be the 'first hop' (I read this as the first LSR on the 
path from N towards the root)?
RFC 6388 s2.4.1.1 allows the upstream to be any LSR along the path 
according to some local selection policy.  Can't the PLR be any such 
upstream LSR?

s2.2:  Following on from the previous comment, do LSR1..3 *have* to be 
'directly connected'?  Can they not be just one from the set of LSRs 
along each separate LSR emanating from the root?

s2.2:  If I understand correctly, the bypass LSPs have to be 
bidirectional (or they could be two unidirectional ones) unlike those in 
s2.1 which will be unidirectional.  I think this ought to be mentioned, 
assuming I am right - and presumably one could do a bit of optimisation 
in setup.  This has some knock-on effects as regards what happens when 
the node fails.  I wonder if there should be some explanation of what 
happens in an extra sub-section in s4 - just that the various LSRs need 
to think about what role they are playing depending on where the 
incoming packets are coming from, I guess.

s2.3, next to last para:
> To remove all PLR addresses belonging to
>     the encoded Address Family, an LSR N MUST encode PLR Status Value
>     Element with no PLR entry and "Num PLR entry" field MUST be set to
>     zero.
  This is inconsistent with the statement in the "PLR Status Value 
Element" figure:
             PLR entry (1 or more)
I guess this last should be 'zero or more'.

Nits/editorial comments:
======================
Abstract: Need to expand LSR on first use.

Abstract and  s1: s/that has been built by/that have been built by the/ 
(2 places)

s1, para 1: Need to expand RSVP-TE and LFA on first use, and give 
references for them - probably RFC 5420 for RSVP-TE and RFC 5286 for LDP 
LFA.

s1, para 2: s/Targetted/Targeted/

s1.2: mLDP probably needs a reference - RFC 6388, I guess

s1, para 2: the concept 'Target(t)ed LDP session' comes from RFC 7060 - 
please add a reference.

s2: It might be helpful to mention that the tLDP sessions are 
established over the bypass LSPs from PLR to MPT and reiterate the 
comments in s1 about how these LSPs might be established to avoid the 
protected node.

s2: The term 'root node' (presumably from RFC 6388) should be in the 
terminology section.

s2: s/e.g./e.g.,/

s2.1, s2.2, s5: The use of phrases such as 'we are ...ing' is deprecated 
for RFCs - please rephrase using, for example, 'this document <does 
something>'.

s2.1, Fig 1 caption: s/to the LSR2/to LSR2/

s2.1: 'which is feature enabled':  Although it is probably obvious it 
would be clearer to say 'which has the node protection feature enabled'

s2.1: s/i.e./i.e.,/

s2.1: s/for Capability negotiation/during Capability negotiation/

s2.2, last para: s/don't/doesn't/, s/will help/will help in/

s2.3, para 1: s/with MP status TLV/with an MP status TLV (see Section 5 
of [RFC6388])/

s2.3: 'Length' and 'Num PLR entry' fields need a type (presumably 
'unsigned integer')

s2.3, "Address Family": s/Address Family/Addr Family/ (for consistency 
with Figure); also add a reference for the correct IANA registry 
(http://www.iana.org/assignments/address-family-numbers/address-family-numbers.xhtml)

s2.3, "Num PLR entry":
OLD:
       Element, followed by "Num PLR entry" field (please see format of a
       PLR entry below).
NEW:
       Element as an unsigned, non-zero integer followed by that number 
of  "PLR entry" fields in the format specified below.
END
(but see issue above, regarding non-zero)

s2.3, "PLR Entry", s/must be zero/MUST be zero/ (I assume). (also 
applies to Reserved fields in s5.4)

s2.3, 3rd last para: s/is depending on/is dependent on/

s2.3, next to last para:
OLD:
by setting "A bit"
    to 1 or 0 respectively in corresponding PLR entry.
NEW:
by setting the "A bit"
    to 1 or 0 respectively in the corresponding PLR entry.
END

s2.3, last para:  The term "MP FEC TLV" is not defined explicitly 
anywhere.  I take it from RFC 6388 that it is (probably) a FEC TLV 
[RFC5036] with a P2MP FEC Element in it.  Please give references and 
make it clear what is implied.

s3, para 1: s/PLR MP Status/PLR Status/ (there is no such thing as a PLR 
MP Status.)

s3, "Length": Needs the type defined (unsigned integer presumably); also 
s/(for Address/for Address/

s3, "Address Family": s/Address Family/Addr Family/ (for consistency 
with Figure); also add a reference for the correct IANA registry 
(http://www.iana.org/assignments/address-family-numbers/address-family-numbers.xhtml)

s3, last para: s/Typical/Typically/

s4, para after Fig 3:
The English in this para needs considerable attention. Suggestion:
OLD:

    Assume that LSR1 is the PLR for protected node N, LSR2 and LSR3 are
    MPTs for node N. When LSR1 discovered that node N is unreachable, it
    can't determine whether it is the 'LSR1 - N' link or node N that
    failed.  In Figure 3, the link between LSR1 and N is also protected
    using Fast ReRoute (FRR) [RFC4090 <https://tools.ietf.org/html/rfc4090>] link protection via node M. LSR1
    MAY potentially invoke 2 protection mechanisms at the same time,
    redirection the traffic due to link protection via node M to N, and
    for node protection directly to LSR1 and LSR2.  If only the link
    failed, LSR2 and LSR3 will receive the packets twice due to the two
    protection mechanisms.  To prevent duplicate packets to be forwarded
    to the receivers on the tree, LSR2 and LSR3 need to determin which
    upstream node to accept the packets from.  So, either from the
    primary upstream LSR N or from the secondary upstream LSR1, but never
    both at the same time.  The selection between the primary upstream
    LSR or (one or more) secondary upstream LSRs (on LSR2 and LSR3) is
    based on the reachability of the protected node N. As long as N is
    reachable, N is the primary upstream LSR who is accepting the MPLS
    packets and forwarding them.  Once N becomes unreachable, the
    secondary upstream LSRs (LSR1 in our example) are activated.  Note
    that detecting if N is unreachable is a local decision and not
    spelled out in this draft.  Typical link failure or Bidirectional
    Forwarding Detection (BFD) can be used to determine and detect node
    unreachability.
NEW:
    Assume that LSR1 is the PLR for protected node N, LSR2 and LSR3 are
    MPTs for node N. When LSR1 discovers that node N is unreachable, it
    cannot immediately determine whether it is the link from LSR1 to N or the actual node N that
    has failed.  In Figure 3, the link between LSR1 and N is also protected
    using Fast ReRoute (FRR) [RFC4090 <https://tools.ietf.org/html/rfc4090>] link protection via node M. LSR1
    MAY potentially invoke both protection mechanisms at the same time, that is
    redirection of the traffic using link protection via node M to N, and
    for node protection directly to LSR1 and LSR2.  If only the link
    failed, LSR2 and LSR3 will receive the packets twice due to the two
    protection mechanisms.  To prevent duplicate packets being forwarded
    to the receivers on the tree, LSR2 and LSR3 need to determine from which
    upstream node they should accept the packets.  This can be either from the
    primary upstream LSR N or from the secondary upstream LSR1, but never
    both at the same time.  The selection between the primary upstream
    LSR or (one or more) secondary upstream LSRs (on LSR2 and LSR3) is
    based on the reachability of the protected node N. As long as N is
    reachable from an MPT, the MPT should accept and forward the MPLS
    packets from N.  Once N becomes unreachable, the LSPs frpm
    secondary upstream PLR LSRs (LSR1 in our example) are activated.  Note
    that detecting if N is unreachable is a local decision and not
    spelled out in this draft.  Typically link failure or Bidirectional
    Forwarding Detection (BFD) can be used to determine and detect node
    unreachability.
END

s4.1, caption of Fig 4: s/after N failure/after failure of node N/

s4.1, last para:
OLD:
    Assume that LSR1 has detected that Node N is unreachable and invoked
    both the Link Protection and Node Protection procedures as described
    in this draft.  LSR1 is acting as PLR and sending traffic over both
    the backup P2P LSP to node N (via M) and the P2P LSPs directly to
    LSR2 and LSR3, acting as MPT LSRs.  The sequence of events are
    depending on whether the link 'LSR1 - N' has failed or node N itself.
    The node's downsteam from the protected node (and participating in
    node protection) MUST have the capability to determin that the
    protected node became unreachable.  Otherwise the procedures below
    can not be applied.
NEW:
    Assume that LSR1 has detected that Node N is unreachable and invoked
    both the Link Protection and Node Protection procedures as described
    in this example.  LSR1 is acting as PLR and sending traffic over both
    the backup P2P LSP to node N (via M) and the P2P LSPs directly to
    LSR2 and LSR3, acting as MPT LSRs.  The sequence of events is
    dependent on whether the link from LSR1 to N has failed or node N itself.
    The nodes downstream from the protected node (and participating in
    node protection) MUST have the capability to determine that the
    protected node has become unreachable.  Otherwise the procedures below
    can not be applied.
END

s4.1.1: s/over the/over to the/; s/that where/that were/

s4.1.2: s/MUST sent a Label/MUST sen a Label/

s5.2, last para: s/the MPT capable/MPT capable/

s5.4, "Length": - unsigned integer again.

s5.4, P and M bits: Add 'Set to 1 to indicate...'