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

["Ralph Droms (rdroms)" <[e-mail]>]

(originally sent 10/16)

Hi, Himanshu - responses in line...

> On Oct 15, 2015, at 7:44 PM 10/15/15, Shah, Himanshu <[e-mail]> wrote:
> 
> 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.

I agree that, in general, duplicating specifications from other documents increases the possibility that the respective documents unintentionally conflict with each other or are not updated in parallel.
> 
> To that end, I would rather prefer, trimming by removing conflicting/confusing text.

I wasn't clear in my review - I think making the references to other documents concise and clear, along with trimming unnecessary text from draft-ietf-pals-mpls-tp-mac-wd, will result in the best document.

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

This example is sort of an interesting one to consider, as I was thinking more of the examples in which the format and semantics of the MAC TLVs are exactly the same as in the cited defining documents.

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

Noted.

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

Again, I wasn't clear in my review - my paging back and forth was both within draft-ietf-pals-mpls-tp-mac-wd and between draft-ietf-pals-mpls-tp-mac-wd and cited documents.  What I think would improve this specification is clarification that trims redundant specification details from draft-ietf-pals-mpls-tp-mac-wd and cites, concisely and exactly, the other documents from which the specifications are copied.

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

It wasn't obvious to me what is intended as a protocol specification and what is intended as a description of the protocol.  I see that RFC 4762 includes text that describes how to process an empty MAC List TLV, so perhaps removal of the text altogether would be best.

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

I think it would be helpful to state explicitly that the Sequence Number TLV MUST be the first TLV in the message.

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

I apologize if I appear to be finicky, again, but the sentence I quoted simply wasn't clear to me.  Common sense interpretation of specifications is, of course, expected, but in my experience a simple sentence like:

 A MAC Address Withdraw OAM message with the A-bit set is sent by a receiver
 to acknowledge receipt and processing of a MAC Address Withdraw OAM message.
 The acknowledgment message contains a Sequence Number TLV to indicate which
 message is being acknowledged.

...which raises a question about error handling: does the acknowledgment indicate simple receipt of the MAC Address Withdraw OAM message or does it also include successful processing?  Are there any error conditions that should be signaled?  If not, perhaps that should be made explicit in the specification.

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

Again not meaning to be finicky, the term "overflow" is not used anywhere in RFC 4385, so RFC 4385 doesn't define "overflow detection".  I think what you mean is that the sequence number mechanism is treated as a 31-bit number space, instead of a 16-bit number space as in RFC 4385, which means:

* the expected sequence number that follows 2^31-1 (maximum unsigned 31-bit number) is one, and
* if the packet sequence number is greater than the expected sequence number, and the packet sequence number minus the expected sequence number is less than 2^30

In my opinion, it would be clearer to just call out those specific changes and otherwise refer to the text in section 4.2 of RFC 4385.

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

OK.  Might be helpful to add a phrase "and the sequence number of the first transmitted message is therefore 2"

(Himanshu - your e-mail to me is truncated at this point in my mail reader)

- Ralph