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

Stewart,

Answers in line - thanks for the prompt response.
Luca

On 06/17/16 11:56, Stewart Bryant wrote:
> 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.
>
I agree.
>>
>>> ============
>>>
>>>
>>>
>>>       * 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?
>
I did not see a reason to force any implementation to preserve the
number. It certainly is not necessary for inter-operation of PEs.
Since the number is only unique to a PE , I would expect it to not be
used to identify links on it's own.

>>> ============
>>>         * 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.
>
yes, I am not sure why ... but in the past it was the rfc editor that
caused the issue ... ( AD sending a delete section message , and section
numbers not getting updated in the text)

>>
>>> ==========
>>>
>>> 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.
>
ok- I will change to SHOULD
>>
>>> ===========
>>>   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 /
fixed.
>> 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?
Designed.
Also reading the security section on those references , they are exactly
what is needed here. So i'm pretty happy that this section is adequate.

>> 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.
>
ok will add the reference to "PW Name Spaces".

>>
>>> ===========
>>>
>>> 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
>