Last Call Review of draft-ietf-mpls-tp-psc-itu-02
review-ietf-mpls-tp-psc-itu-02-genart-lc-davies-2014-02-24-00

I am the assigned Gen-ART reviewer for this draft. For background on
Gen-ART, please see the FAQ at

<

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

Please resolve these comments along with any other Last Call comments
you may receive.

Document: draft-ietf-mpls-tp-psc-itu-02.txt
Reviewer: Elwyn Davies
Review Date: 24 Feb 2014
IETF LC End Date: 24 Feb 2014
IESG Telechat date: (if known) -

Summary:
Almost Ready for IESG.  There is a statement in s1 that could indicate
that the consequences of turning on a subset of the new capabilities has
not been fully thought through.  I am also not clear whether EXER
fulfils the intentions of R84 in RFC 5654 as stated.  The repeated
transmission of the advertised capabilities adds to the scope for random
bit errors to halt protection operation.  A few minor nits noted below.

Major issues:
None

Minor issues:
s1, next to last para:

>    This document describes the behavior of the PSC protocol including
>    the priority logic and the state machine when all the capabilities
>    associated with the APS mode are enabled.  The PSC protocol behavior
>    for the PSC mode is as defined in RFC 6378.

APS mode involves five capabilities which can, apparently, be
implemented and advertised indpendently, so presumably there might be
reasons for either implementing or turning on just a subset.  Are there
any implications for the PSC protocol if only some subset of the the
capabilities are available in a given node?  (This may be a dumb
question and I haven't tried to work out what might go wrong if you did
have any of the available subsets.) 

s9.1.1/s9.2: Following on from the previous point, s9.1.1 implies that
the system can operate happily with some subset of the five capabilities
turned on provided that both ends concur.  However there is no mode name
that would cover such a subset.  Does this actually mean that turning on
a subset doesn't really work? And if it doesn't work why bother with
five flag bits?  Also the phraseology used here could lead to future
problems if more capabilities are defined:  suppose some future new mode
(FOO) adds <n> more capabilities but the two 'modes' can be turned on
independently - as currently expressed you would have to define three
mode names for APS only, FOO only and APS + FOO.. and so on with more
capability sets.  Unintended consequence? Oh, and what if a capability
is used in more than one mode?

s8: EXER appears to test the PSC channel but nothing else.  I am not
clear that this fully satisfies R84 in RFC 5654.. It may be that I don't
know what information would go back in the RR response, but should the
response say anything about the state in the remote node (like the
remote end of the working path is not working in some way and/or what is
the PSC state - RFC 6378 s3.6)?

s13: The addition of the Capabilities TLV and the requirement that it is
carried accurately and repeatedly in every PSC message introduces a new
aspect to the DoS/random corruption problem mentioned in s6 of RFC 6378.
A single bit corruption in a PSC message will lead to disablement of
protection switching and requirement of operator intervention.  I am not
sure if this is a serious issue, but it probably merits a comment in s13
and verification that it does match with the words in RFC 6378 as
regards 'converging on a stable state'.

Nits/editorial comments:
s4.4, title: s/Capability 1/Priority Modifications/ (Ok, its accurate
but bald)

s7.3 et seq: The term "selector bridge" is introduced without
definition.  I suspect it is a piece of jargon I am supposed to know but
I think a reference would help.

s7.4, para 1: 

> the rules to resolve the equal priority requests are
>    required.
Should this be s/the rules/some rules/?

s8, definition of Exercise (EXER): Describing EXER as 'replacing' one of
NR, RR or DNR seems a bit odd.  Mentioning RR is particularly
problematic since it makes the definitions sort of circular as RR is the
response to EXER.  I guess what is probably appropriate is to say that
it is built in the same way as an NR message would be built. 

s9.1: RFC 6378 doesn't define the encoding of the TLV type and TLV
length fields, so it needs to be done here (Unsigned integers). (It also
doesn't define encoding of the (unnecessary) overall TLV length field in
the PSC header.

s9.1: What happens if the receiver gets a TLV with some a flags length
that isn't a multiple of 4 (or indeed a TLV it deosn't recognize?)  It
might be cleaner to define the length in terms of units of 32 bits.

s10.3, para 2: s/they SHALL be disappeared./they SHALL be discarded./