Re: [Gen-art] Review: draft-ietf-pals-mpls-tp-mac-wd

["Shah, Himanshu" <[e-mail]>]

Hi Ralph -
Thanks for your thorough review.

Let me first address your major concern.

As you point out, this draft builds on existing standards.
We (authors and WG) had to carefully balance between the right amount of information
and wanting to describe details of methods described in other RFCs.

This is frustrating to implementer (including myself) having to go back and forth
between the documents. So I share that concern.

But we would like to refrain from indulging in beefing up the doc and risk deviating
from other base standards. However, for certain, if there is any conflict or lack of
clarity, we would prefer to rectify.

To that end, I would rather prefer, trimming by removing conflicting/confusing text.
For example, sequence number processing - I rather would ask reader to get all details
from relevant RFC, and point to only delta (which is to apply same logic that is used
for 16-bit sequence number field to 32-bit field sequence number field that is used in
this document).

More comments in line.. and I look forward to your further guidance so we can wrap this 
up in time.

As a data point, there are two implementations of this draft deployed in a Telco network 
in Asia.


Thanks,
Himanshu


-----Original Message-----
From: Ralph Droms (rdroms) [mailto:[e-mail]] 
Sent: Wednesday, October 14, 2015 4:02 PM
To: A. Jean Mahoney; General Area Review Team; [e-mail]
Subject: [Gen-art] Review: draft-ietf-pals-mpls-tp-mac-wd

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-pals-mpls-tp-mac-wd-02, "MAC Address Withdrawal over Static Pseudowire"
Reviewer: Ralph Droms
Review Date: 2015-10-14
IETF LC End Date: 2015-10-19
IESG Telechat date: (if known) N/A

Summary:

This draft is on the right track but has open issues, described in the review.


Major issues:

While this document is describing a straightforward adaptation of previously defined standards to statically provisioned PWs, in my opinion an implementor would not necessarily be able to construct a fully interoperable implementation from this document.  There are several sections of the document that are not clear in their description of how to use mechanisms from referenced documents in this standard.

If it appears that some of my comments are overly finicky, I'll agree that the correct implementation could probably be deduced from the text in most cases.  That is, I didn't find many outright errors or inconsistencies.  Many of my comments took a lot of paging back and forth, reading of referenced documents and head-scratching, which, in my experience, can lead to implementations that don't interoperate.

[Himanshu>] Please see above for the justification of this approach.

Section 1:

  When the number of MAC addresses to be
  removed is large, the empty MAC List TLV may be used.  The empty MAC
  List TLV signifies wildcard MAC Address withdrawl.

This text seems to be the only reference to the processing of an empty MAC List TLV.  Specification of how the protocol works doesn't belong in the Introduction, and "wildcard MAC Address withdrawal" could certainly use some more explanation.

[Himanshu>] I would prefer taking the text out if its mention in "Introduction" section is not desirable.
Wildcard MAC withdraw is a very well-known concept in VPLS architecture and needs no more description to L2VPNers, IMO.
So absence of its reference in subsequent sections does not dilute the purpose of this document. 

Section 3:

  The PW OAM message header is exactly the same as what is
  defined in [RFC6478].

Is this statement really true?  The MAC Address Withdraw header seems to replace the "Refresh Timer" field with a "Reserved" field, and adds a new "R" flag.  The statement might be misleading to an implementor.

[Himanshu>] I agree with you. This is statement is used loosely.
The PW OAM message header is meant to apply only to first 4 bytes.

Perhaps -
"The MAC withdraw PW OAM message follows the same guidelines used in [RFC6478],
whereby first 4-bytes of OAM message header is followed by message specific
field and a set of TLVs relevant for the message"



  a MAC address withdraw OAM message MUST contain a
  "Sequence Number TLV" otherwise the entire message is dropped.

Is the "Sequence Number TLV" required to be the first TLV in the message?  Are the TLVs required to appear in any particular order?

[Himanshu>] Yes. It is important to determine the "newness" of the message first for processing eligibility and should not require
hunting through entire message to find that TLV. My hope is that this is obvious to the reader who 'follows' the concepts in the draft.

If you feel, that such an explicit mention is necessary, I do not mind.





  A single bit (called A-bit) is set to indicate if a MAC withdraw
  message is for ACK.  Also, ACK does not include MAC TLV(s).

Does this mean that the message is an ACK if the A-bit is set?  Can an ACK contain a "Sequence Number" TLV?

[Himanshu>] I do not quite follow. ACK HAS TO INCLUDE THE SEQ NO TLV, how else receiver know what is ACK of 
what seq# message is of? I think this falls into commonsense category BUT, the text explicitly says that ONLY
MAC TLVs are not required.


  Only half of the sequence number space is used.  Modular arithmetic
  is used to detect wrapping of sequence number.  When sequence number
  wraps, all MAC addresses are flushed and the sequence number is
  reset.  The 16-bit sequence number handling is described in
  [RFC4385]. This document uses 32-bits sequence numbers and hence
  sequence number in half the number space (i.e. 31-bits or ~2billion)
  is considered in the valid receive range.

This paragraph is not at all clear to me. Reading section 4 of RFC 4385 helped but left me unsure about how my understanding of how to extend the sequence number mechanism to 32 bits corresponds to the expectations of this document.

[Himanshu>] OK.
[Himanshu>] How about this paragraph -

The lack of reliable transport protocol for the in-band OAM necessitates a presence of 
sequencing scheme so that the receiver can recognize newer message from retransmitted
older messages. The [RFC4385] describes details of sequence number handling which include
overflow detection. This document leverages the same scheme, with the exception of overflow
detection which is simplified here such that sequence number exceed 2,147,483,647 (0x7fffffff) is 
considered overflow and reset to 1.





Section 4.1:

  Each PW is associated with a counter to keep track of the sequence
  number of the transmitted MAC withdrawal messages.  Whenever a node
  sends a new set of MAC TLVs, it increments the transmitted sequence
  number counter, and include the new sequence number in the message.
  The transmit sequence number is initialized to 1 at the onset.

I'm pretty sure this is supposed to mean that the sequence number in the first message is 1.  The text could be interpreted to mean that the counter is initialized to 1 and then incremented to 2 when sending the first message

[Himanshu>] 
[Himanshu>] No. The text is correct. Each endpoint keeps the record of send and receive sequence number. These values are initialized to 1.
If the sender sends the first MAC withdraw with sequence number 1, then receiver will ignore because it does not think that received MAC withdraw
is new. So starting MAC withdraw send seq# is in fact 2. Do note that the purpose of the seq# logic is ONLY to detect the received message as NEWER than what was ack'ed
before or re-transmits due to in-transits.