Re: [Pals] Shepherds review of draft-ietf-pals-status-reduction-00

Hi Luca

We pretty much agree on everything. Please see inline for some notes and 
a couple of typos.

On 17/06/2016 17:40, Luca Martini wrote:
>
> On 06/09/16 12:54, Stewart Bryant wrote:
>> Hi Authors
>>
>> I have done a shepherd's review of this draft and have a few issues
>> that need to be looked. It would be timely if you could fix these during the
>> IPR poll I am about to start so that we can have cleaner text to
>> take to the WG for last call.
>>
>> Please see below.
>>
>> - Stewart
>>
>>   Abstract
>>
>>     This document describes a method for generating an aggregated
>>     pseudowire status message on Multi-Protocol Label Switching (MPLS)
>>     network Label Switched Path (LSP).
>>
>>     The method for transmitting the pseudowire (PW) status information is
>>     not new, however this protocol extension allows a Service Provider
>>     (SP) to reliably monitor the individual PW status while not
>>     overwhelming the network of multiple periodic status messages. This
>> SB> s/of/with/
> ok.
>>     is achieved by sending a single cumulative summary status
>>     verification message for all the PWs grouped in the same LSP.
>> SB> grouped in or grouped on?
> pw are inside an LSP . I think it's in.

Genuinely not sure - will leave it to the RFC editor.

>
>> ============
>>
>>
>>
>>       * Session ID
>>
>>         A non-zero, locally selected session number that is not preserved
>>         if the local PE restarts.
>>
>>         In order to get a locally unique session ID, the recommended
>>         choice is to perform a CRC-16 giving as input the following data
>>
>>         |Y|Y|M|M|D|D|H|H|M|M|S|S|L|L|L|
>>
>>         Where:  YY: are the decimal two last digit of the current year
>>         MM: are the decimal two digit of the current month DD: are the
>>         decimal two digit of the current day HHMMSSLLL: are the decimal
>>         digits of the current time expressed in (hour, minutes, seconds,
>>         milliseconds)
>>
>> SB> What happens if there is a hash collision?
> ok, I'll take care of this by just adding 1 to the number until it is
> locally unique.
> ( unless you have a better suggestion ? )
> I will add the following:
>
> If the calculation results in an already existing session ID, a unique
> session ID can be generated by adding 1 to the result until the session
> ID is unique.
> Any other method to generate a locally unique session ID is also acceptable.

That would work, or you could say any use any method you like so long as you
search for  value that is unique amongst the currently known Session IDs.

Is there any reason no to preserve the number? Surely it make n/w mgt 
easier?

>> ============
>>   
>>       * Refresh Timer.
>>
>>         A non zero unsigned 16 bit integer value greater or equal to 10,
>>         in milliseconds, that indicates the desired refresh interval. The
>>         default value of 30000 is RECOMENDED.
>>
>> SB> Do you need to specify a min of 10ms, or would it be adequate to
>> SB> to recommend this lower value?
> I'm not clear on your question. We are recommending a default, the value
> can be set to anything.
> anything below 10ms would be a bit silly.

With hindsight, yes you are right, leave it.

>> ===========
>>
>>
>> 5. PW status refresh reduction Control Messages
>>
>>
>>     There can only be one control message construct per PW status refresh
>>     reduction Message. If the U bit is set, and a PE receiving the PW
>>     status refresh reduction Message does not understand the control
>>     message, the control message MUST be silently ignored. However the
>>     control message sequence number MUST still be acknowledged by sending
>>     a null message back with the appropriate value in the Last Message
>>     Received Field.
>>
>> SB> The above is a little odd. On the one hand you are saying that
>> SB> that the message is not understood, but on the other hand that you
>> SB> expect the node to execute an element of the protocol procedure.
>> SB>
>> SB> Presumably it is a particular message type that is not understood?
> not quite. this is standard LDP like behavior. The idea is that a new
> implementation sending something additional that is not yet implemented
> in an old implementation should just work.
> say for example that someone starts sending some new extra information,
> and some peers are yet to be upgraded. We want that to just be ignored.
> We have to update the sequence number otherwise the message queue would
> just stop.
>
OK, I understand.

>> ==============
>>
>> 5.0.1. Notification message
>>
>>     The most common use of the Notification Message is to acknowledge the
>>     reception of a message by indicating the received message sequence
>>     number in the "Last Received Sequence Number" field. The notification
>>     message is encoded as follows:
>>
>>      0                   1                   2                   3
>>      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>     |         Checksum              |    Message Sequence Number    |
>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>     |  Last Received Seq Number     |  Type=0x01    |U   Flags      |
>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>     |                      Notification Code                        |
>>     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>>
>>
>>     The message type is set to 0x01, and the U bit is treated as
>>     described in the above section.
>>
>> SB> It might be better to give the explicit section number.
> I hate explicit section numbers , as they always end up wrong ... Is the
> section not Above ?
> As you remember we have errata on other documents on this point....
Ah, but not if you automatically generate them as I (and RFCed) do :)

The problem can arise if you add a section in and forget about the implicit
pointer.

I will not press the point.

>
>> ==========
>>
>> 5.0.2. PW Configuration Message
>>
>>     The PW status refresh reduction TLVs are informational TLVs, that
>>     allow the remote PE to verify certain provisioning information. This
>>     message contain a series of sub-TLVs in no particular order, that
>>     contain PW and LSP configuration information. The message has no
>>     preset length limit, however its total length will be limited by the
>>     transport network Maximum Transmit Unit (MTU).
>>
>> SB> How do you know the MTU?
> The physical link is attached to the PE , so the PE does know the MTU of
> the link. Remember that this is only going to the neighbor, the path
> only has one hop.
OK.
>
>> ========
>>
>> 5.0.2.2. PW ID configured List
>>
>>
>>     The number of PW Path IDs in the TLV will be inferred by the length
>>     of the TLV up to a maximum of 8.  The procedure for processing this
>>     TLV will be described in a section below.
>>
>> SB> It would be kind to the reader to give the section number.
> same comment - anytime i have done this sections numbers got out of
> order ...
Same reply :)
>> ===========
>>
>>
>>   5.0.2.3. PW ID unconfigured List
>>
>>     This OPTIONAL TLV contains a list of the PWs that have been
>>     unprovisioned on the LSP. Note that it is a fatal session error to
>>     send the same PW address in both the configured list TLV , and the
>>     unconfigured list TLV in the same configuration message.
>>
>> SB> What action is taken on a fatal session error?
> It is explained in section "5.0.1    Notification message"
> However, this text should explain which error code to send .....
> the following text will be added:
> If the this error occurs , an error notification message is returned
> with the error code of "PW Configuration TLV conflict" and the session
> is terminated by entering STARTUP state.
Good.
>
>> =========
>>
>> 6. PW provisioning verification procedure
>>
>>     This procedure and the advertisement of the PW configuration message
>>     are OPTIONAL.
>>
>>     A PE that desires to use the PW configuration message to verify the
>>     configuration of PWs on a particular LSP, should advertise its PW
>>     configuration to the remote PE on LSPs that have active keepalive
>>     sessions.  When a PE receives PW configuration information using this
>>     protocol and it not supporting or not willing to use the information,
>>     it MUST acknowledge all the PW configuration messages with a
>>     notification of "PW configuration not supported".
>>
>> SB> How does a PE acknowledge a protocol it does not support?
> ok, i see that the text is not clear. the notification is meant to
> indicate that this procedure is not supported.
> I will change the top text to be :
> The advertisement of the PW configuration message is OPTIONAL.
Thanks
>
>> ===========
>>
>>        -iii. An Alarm MAY be raised to a network management system.
>>
>> SB> Shouldn't that be SHOULD be raised?
> This really depends on local PE implementation and policy. It is not
> necessary for the protocol implementation.
> I can think of examples where an alarm is not desirable. For example of
> a PW is just being configured  manually, and the remote end is not yet
> added.
> I would rather leave this to a local implementation preference then
> force it with a SHOULD.
OK, maybe you need to put in a little text along the lines you just said 
to me.
If you want to leave it OK, but I anticipate a bunch of AD/Dir comments.

>
>> ===========
>>   6.1. PW ID List advertising and processing
>>
>>
>>     If a new PW is added to a particular LSP, the PE MUST place the
>>     configuration verification of this PW on hold for a period of at
>>     least 10 seconds. This is necessary to prevent false positive events
>>     of mis-configuration due to the ends of the PW being slightly out of
>>     sync.
>>
>> SB> Prevent or minimise?
> Yes minimize is a better term. Will fix. I will also move the number to
> 30s as it seem s a bit more appropriate.
>> ================
>>
>> 7. Security Considerations
>>
>>     Section to be completed in a later version of the document.
>>
>> SB> Sorry author's we can't that this to WG LC with this security
>> section!
> oops i missed this .....
> I will write it in.
> The security considerations of [RFC6478] are adequate for the proposed
> mechanism since the operating environment is almost identical to to the
s/to to/to /
> one where this protocol would be deployed. It should also be noted that
> since this protocol is defined  to be deployed between two adjacent PEs
defined or designed?
> connected by a physical link, it is not possible to misdirect or inject
> traffic without compromising the PW transport link itself, all these
> situations are covered in the security considerations of [RFC6478].
>> 8. IANA Considerations
>>
>> 8.1. PW Status Refresh Reduction Message Types
>>
>>     IANA needs to set up a registry of "PW status refresh reduction
>>     Control Messages".
>>
>> SB> This is presumably a registry contained in the PW Name Spaces
> yes , the names has PW in it , i would assume that IANA would put it
> there .Do i need to add something to the IANA instructions ?
It is common to specify the namespace. IANA are always nice to people, so
it is always good to make the instruction clear to help them out.

>
>> ===========
>>
>> 8.2. PW Configuration Message Sub-TLVs
>>
>>     IANA needs to set up a registry of "PW status refresh reduction
>>     Configuration Message Sub-TLVs".
>>
>> SB> This is presumably a registry contained in the PW Name Spaces
>>
>> SB> Actually do you need to create a sub registry to hold all three
>> SB> of these new registries. I would think you did to keep them
>> SB> together.
> Since this is really a trivial IANA organization issue, let's discuss
> this in person, and I will add the necessary IANA instructions as you
> seem fit.
OK.
>
>> =============
>>
>> 8.3. PW Status Refresh Reduction Notification Codes
>>
>>     IANA needs to set up a registry of "PW status refresh reduction
>>     Notification Codes".
>>
>> SB> This is presumably a registry contained in the PW Name Spaces
>> ==========
>>
>>     These are 32-bit values. Type value 1 through 7
>>     are defined in this document.
>>
>> SB> Actually 0 through 7 are defined aren't they?
> yes -fixed.
>> =========
>>
>> SB> What is the interaction between this protocol and PW protection?
> There is no real interaction other then this can be another source of
> information to set the PW state.
OK

If you do the edits we look good to go.

Stewart
>
>   =========================== ===========================
> Luca
>>
>> _______________________________________________
>> Pals mailing list
>> Pals@ietf.org
>> https://www.ietf.org/mailman/listinfo/pals