Subject: [Gen-art] Gen-art LC review of draft-ietf-mpls-tp-psc-itu-02.txt

From: Elwyn Davies <[email-masked]>
Date: Mon, 24 Feb 2014 18:11:33 +0000

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./

---

From: Huub Van Helvoort <[email-masked]>
Date: Mon, 24 Feb 2014 21:50:39 +0000

Hello Elwyn,

Thank you for your review.

See my response in-line [Huub]

Summary:

[Huub] I have addressed these below.

Major issues:
None

Minor issues:

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.

[Huub] There were originally separate drafts for each of the capabilities.

However, it was very complex to define the effect on the state

transition tables for each capability or combination of capabilities.

[Huub] to resolve this issue it was deceided that for any permitation

of the possible combinations of capabilities a separate draft should be

developed. RFC6378 is the one where none of the capabilities is

used, and this draft is the one where all capabiliies are used. So far

none of the other possible drafts is written.

[Huub] This draft provides the behavior similar to APS used in ITU-T

for other technologies.

Are there

[Huub] both nodes MUST support the same subset, if they don't the

result will be unpredictable because the state machines are different.

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.

[Huub] as indicated above, the mode name does not exist because the

draft that describes that mode does not (yet) exist.

Does this actually mean that turning on
a subset doesn't really work?

[Huub] indeed, not until there is a draft that describes it.

And if it doesn't work why bother with
five flag bits?

[Huub] to give others the opportunity to write the missing draft(s), this

was the agreed by the WG chairs.

Also the phraseology used here could lead to future

[Huub] it is up to the authors of additional drafts to come up with a name

for the mode described in their draft.

s8: EXER appears to test the PSC channel but nothing else.  I am not
clear that this fully satisfies R84 in RFC 5654.

[Huub] EXER verifies that the PSC engine at the far end is still running

properly and not stuck at sending the same PSC message continuously.

The PSC channel is tested by sending PSC messages regularly.

(see RFC6378 s4.1)

It may be that I don't

[Huub] the fact that RR is received after sending EXER is proof that the

PSC engine is healthy. If it was not healty it would not have sent the RR.

Because EXER has the lowest priority is can only be used in the IDLE

state, so the far end could only send one state.

[Huub] a bit error will occur in one message, RFC6378 s4.1 addresses

that issue.

Nits/editorial comments:

[Huub] OK.

[Huub] RFC6378 uses "selector", "selector bridge" is the equivalent ITU term.

[Huub] or s/the rules/rules/

[Huub] the "replacing" means that in case an operator wants to verify

the operation of PSC, the excersise commend is issued and the

transmission of RR (or NR, or DNR) is stopped and EXER is sent instead.

[Huub] IANA asigned the TLV type value. By not giving exact values

we leave the possibility open that there are more than 32 flags.

[Huub] this can be treated the same as a bit error in a PSC message.

[Huub] OK.

Cheers, Huub.

--
Huub van Helvoort, 海高明
Senior Consultant, 高级顾问
Huawei Technologies Ltd, 华为技术有限公司 - European Research Center, 欧洲研究所
Karspeldreef 4, 1101CJ Amsterdam, The Netherlands
Tel: +31 20 4300 936   Mob: +31 6 49248936

---

From: Elwyn Davies <[email-masked]>
Date: Tue, 25 Feb 2014 11:18:59 +0000

> for the mode described in their draft.

To be absolutely honest, my initial reaction to the above (i.e., right
back to the beginning of Minor Issues) went something like 
"******** ARRRGGGGHHH! Choke!! Splutter!!!".

Be that as it may, if that is what is agreed then please could there be
some explanation in the document. Please!

> PSC engine is healthy. If it was not healty it would not have sent the
Agreed.
> state, so the far end could only send one state.
OK.. I rather thought that the requirement was looking for a bit more
information to be returned.

> [Huub] a bit error will occur in one message, RFC6378 s4.1 addresses

I don't think this covers the problem:
The last para of s6 of RFC 6378 says:

>    However, a new concern for this document is the accidental corruption
>    of messages (through faulty implementations or random corruption).
>    The main concern is around the Request, FPath, and Path fields as a
>    change to these fields would change the behavior of the peer end
>    point.  Although this document does not define a way to avoid a
>    change in network behavior upon receipt of a message indicating a
>    change in protection status, the transition between states will
>    converge on a known and stable behavior in the face of messages that
>    do not match reality.

This appears to imply that random bit errors can percolate up to the 
protocol engine.  If the bit error affects the capability flag bits in the 
Capability TLV then the behaviour specified in s9.1.1 would come into play:

>    When a node receives a Capabilities TLV it MUST compare it to its
>    most recent transmitted Capabilities TLV.  If the two are equal, the
>    protected domain is said to be running in the mode indicated by that
>    set of capabilities (see Section 9.2).  If the sent and received
>    Capabilities TLVs are not equal, this indicates a capabilities
>    mismatch.  When this happens, the node MUST alert the operator and
>    MUST NOT perform any protection switching until the operator resolves
>    the mismatch in the Capabilities TLV.

The result of such a corrupted bit would a.s. be a mismatch that should
be followed by a turn off of protection switching and operator alerting.

> [Huub] RFC6378 uses "selector", "selector bridge" is the equivalent
Can we tie them together then please?

> [Huub] or s/the rules/rules/
Either would help.

> response to EXER.  

Any thoughts here?...

> I guess what is probably appropriate is to say that

> transmission of RR (or NR, or DNR) is stopped and EXER is sent
I think I see what is going on noew 
Perhaps adding something like the phrase "that would normally be sent
periodically in the IDLE state" 
> we leave the possibility open that there are more than 32 flags.
That's not the issue.  The problem is that the protocol does not specify
how the type and length numbers should be represented on the wire.  The
standard answer is unsigned integer in network bit order.
> [Huub] this can be treated the same as a bit error in a PSC message.
This should be stated.

---

From: Huub Van Helvoort <[email-masked]>
Date: Wed, 26 Feb 2014 16:39:01 +0000

Hello Elwyn,

Please find my response in-line [Huub2]

To avoid any confusion I have snipped the parts that do not need
further expanation.

> [Huub] to resolve this issue it was decided that for any permitation
> question and I haven't tried to work out what might go wrong if you
> did have any of the available subsets.)
>
> turned on provided that both ends concur.  However there is no mode
> name that would cover such a subset.
>
> [Huub] to give others the opportunity to write the missing draft(s),
> this was the agreed by the WG chairs.
>
> [Huub] it is up to the authors of additional drafts to come up with a
> name for the mode described in their draft.

[Huub2] the probability that there may be several "solutions" is IMHO
very small.
Personally I do not expect any other solution because RFC6378 is
what IETF wants, and this draft is what ITU-T wants and will 
document in recommendation G.8131.
Any other "solution" would be non-standard. 
Adding an explanation to this draft will give the impression that
other solutions will be acceptable. Something neither IETF nor
ITU-t would admit to.

[Huub2]
A received PSC message with a bit error is a not valid PSC message.

If a received PSC message is unexpected (due to a bit error, or a corrupt
far end protocol engone) in a particular state it is considered a not valid 
message and is ignored

The capability of a protected link is provisioned at initiation and cannot
be changed during operation, so a PSC message with a capability bit
changed (e.g. due to a bit error)  is considered not valid PSC message
and is ignored.

I think this is covered by the last paragraph in s4.1 of RFC6378.
====
    If no valid PSC message is received, over a period of several
    continual messages intervals, the last valid received message remains
    applicable.
 ===

> s4.4, title: s/Capability 1/Priority Modifications/ (Ok, its accurate

[Huub2] will be changed as you propose.

[Huub2] for consistency I propose to use "selector" in the whole
document. Selector (without bridge) is already used in a few places.

> >    required. Should this be s/the rules/some rules/?
>

Either would help.

[Huub2] OK.

> problematic since it makes the definitions sort of circular as RR is
> the response to EXER.

Any thoughts here?...

[Huub2] see below.

[Huub2] if it helps readability we will do as suggested.

> doesn't define encoding of the (unnecessary) overall TLV length field
> in the PSC header.
>

[Huub2] OK, I now understand your point. This can be addressed.

This should be stated.

[Huub2] OK, we will address this.

Best regards, Huub.

---

From: Elwyn Davies <[email-masked]>
Date: Wed, 26 Feb 2014 18:23:35 +0000

Hi, Huub.

Thanks for the responses: Agreed bits snipped again.

> other solutions will be acceptable. Something neither IETF nor

Politics! OK - How about we add to either s9.1.1 or s9.2 something like:

   Only combinations of capabilities specified by a mode are allowed.   
   Capability TLVs with other combinations will be treated as invalid 
   PSC messages. 
>     continual messages intervals, the last valid received message remains

Sorry to harp on about this... I think we are trying to distinguish
between invalid messages and protocol errors.  If a PSC message with the
wrong set of Capability bits set is (always?) considered not a valid PSC
message then you need to change the wording of the piece of s9.1.1
quoted above.  The text in [Huub2} implies:
either:
   that there is a distinction between the first PSC message and   
   subsequent ones, in which case the comparison is against the 
   internally configured value of the capability bits, since nothing 
   has been sent or received yet, and that means that the 
   specified action only happens on the first PSC message - it 
   isn't clear to me that the specified action makes sense in that case.
or:
   that *any* PSC message with the wrong set of capability bits set 
   (again tested against the internally configured value) is 
   flagged up as invalid and discarded *without any action being 
   taken*. In this case the text in s9.1.1 is never actioned.

For my peace of mind.. Can you confirm whether my interprestation of RFC
6378 s6 is correct, i.e., bit errors in transmission are *not* detected
by the layers below the PSC protocol and corrupted PSC messages will be
passed to the PSC state machine.

---

From: "Adrian Farrel" <[email-masked]>
Date: Wed, 26 Feb 2014 19:10:25 -0000

Sut mae, Elwyn-gwas,

[snip]

> Politics! OK - How about we add to either s9.1.1 or s9.2 something like:

I don't think it is politics at all, and I don't really like that Huub
referenced this as IETF and ITU-T since both documents are IETF standards track
work.

There are some people who want to deploy one set of options (the empty set) and
they have an RFC.
There is another set of people who want to deploy a different set of options
(all 5 of them) and they will have this RFC.

>    Capability TLVs with other combinations will be treated as invalid

But this is not true.

What we have is:
A node knows which options it supports, and those options are expressed as
bundles (modes).
A node that tries to communicate with you must offer a set of options (a mode).
Either you support the mode or you don't.
If you support it, off you go.
If you don't support it, you reject the message saying "unsupported" as
described in this document.

The message that offers a different set of options (i.e. a different mode) is
not invalid, it is just that you don't understand it. You can't accept it
because you don't know how to behave in that mode.

OTOH, when someone writes the new RFC describing that mode, you might enhance
your implementation to support it as well.

(Hint: it really helps to think in modes. A mode is expressed as a bit-flag with
plenty-bits of which five currently have meaning.)

I agree with you that there is some distinction to be made between "invalid
message" and "protocol error".
But "no valid message" does not mean "protocol error" or "invalid message" was
received. It is a composite of possibilities that reflects the negation of
receiving a message that was small and perfectly formed.

Ciao,
Adrian

---

From: Huub Van Helvoort <[email-masked]>
Date: Wed, 26 Feb 2014 19:57:43 +0000

Hello Elwyn,

Thanks for your reply.
See my responses in-line [Huub3].

=====

[Huub3] that works for me. Eric, Jeong-dong can you address this?

>     continual messages intervals, the last valid received message remains

Sorry to harp on about this...

[Huub3] no problem, clarity is key

I think we are trying to distinguish
between invalid messages and protocol errors.  If a PSC message with the
wrong set of Capability bits set is (always?) considered not a valid PSC
message then you need to change the wording of the piece of s9.1.1
quoted above. 

[Huub3] I agree, I will check with the other authors if they agree too.
If they disagree I will inform you.

The text in [Huub2} implies:
either:

[Huub3] it is a bit different. When a new/changed PSC message
is sent, it is sent three times very fast. A majority decision is taken
to determine the correct value, so it is possible that the first is
discarded because it is not the same as the second and third.
The capability bits are always compared to the provisioned mode.

or:

[Huub3] according to s9.1.1 the capability TLV MUST be added
always to every PSC message sent. The capability bits are set to
the provisioned mode. There is one exception: the original PSC as 
described in RFC6378, because that does not use capability TLV.

[Huub3] If the (lower) server layer is Ethernet, the Ethernet frame can
use the FCS to detect bit errors and discard the Ethernet/MPLS frame.
If FCS is not used bit errors are undetected.
If the (lower) server layer is SDH/SONET/OTN the FEC (forward
error correction) will correct bit errors that occur in the physical layer.
If FEC is not used bit errors are counted, if the error rate is excessive
AIS will be inserted, in a packet (higher) client layer this will result in
discarding all frames.

Cheers, Huub.

---

From: Huub van Helvoort <[email-masked]>
Date: Wed, 26 Feb 2014 21:21:49 +0100

Shwmae Adrian,

> referenced this as IETF and ITU-T since both documents are IETF standards track

Please excuse me for giving the impression that this draft is an ITU-T
document. You are absolutely right that both are IETF documents.
I should have used "PSC in ITU-T mode" to refer to this draft.

> (all 5 of them) and they will have this RFC.

Indeed.

> described in this document.

Again: correct. I tried to simplify too much and used the wrong word
(invalid, instead of unsupported).

> because you don't know how to behave in that mode.

Indeed (not supported).

> your implementation to support it as well.

Yes, and initially advertise your own capabilities, compare to the
capabilities received from the far-end and attempt to converge.

> receiving a message that was small and perfectly formed.

OK.

Cheers, Huub.

-- 
*****************************************************************
               请记住，你是独一无二的，就像其他每一个人一样