From: "Ralph Droms (rdroms)" <rdroms@cisco.com>
To: "Shah, Himanshu" <hshah@ciena.com>
Thread-Topic: [Gen-art] Review: draft-ietf-pals-mpls-tp-mac-wd
Thread-Index: AQHRBrtB0K0DtUmA2kyW/x0Nq3/Sjg==
Date: Thu, 22 Oct 2015 15:29:04 +0000
Message-ID: <D98030BB-32EB-4859-A71B-8D54BDBF031C@cisco.com>
References: <095AB7C0-C1B7-4963-852C-A5A613AD261A@cisco.com>
 <40746B2300A8FC4AB04EE722A593182B9ACC4AF3@ONWVEXCHMB04.ciena.com>
In-Reply-To: <40746B2300A8FC4AB04EE722A593182B9ACC4AF3@ONWVEXCHMB04.ciena.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.131.118.62]
Content-Type: text/plain; charset="us-ascii"
Content-ID: <182B959B03E3C84C94A5C34632570769@emea.cisco.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
Archived-At: <http://mailarchive.ietf.org/arch/msg/gen-art/f97TYiQ8qDCrKKRgOAPHLZFwMzg>
Cc: General Area Review Team <gen-art@ietf.org>,
 "draft-ietf-pals-mpls-tp-mac-wd.all@ietf.org"
 <draft-ietf-pals-mpls-tp-mac-wd.all@ietf.org>, The IESG <iesg@ietf.org>
Subject: Re: [Gen-art] Review: draft-ietf-pals-mpls-tp-mac-wd
X-BeenThere: gen-art@ietf.org
X-Mailman-Version: 2.1.15
Precedence: list
List-Id: "GEN-ART: General Area Review Team" <gen-art.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/gen-art>,
 <mailto:gen-art-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/gen-art/>
List-Post: <mailto:gen-art@ietf.org>
List-Help: <mailto:gen-art-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/gen-art>,
 <mailto:gen-art-request@ietf.org?subject=subscribe>
X-List-Received-Date: Thu, 22 Oct 2015 15:29:33 -0000

(originally sent 10/16; second try)

Hi, Himanshu - responses in line...

> On Oct 15, 2015, at 7:44 PM 10/15/15, Shah, Himanshu <hshah@ciena.com> wr=
ote:
>=20
> Hi Ralph -
> Thanks for your thorough review.
>=20
> Let me first address your major concern.
>=20
> 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.
>=20
> This is frustrating to implementer (including myself) having to go back a=
nd forth
> between the documents. So I share that concern.
>=20
> But we would like to refrain from indulging in beefing up the doc and ris=
k 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 i=
ncreases the possibility that the respective documents unintentionally conf=
lict with each other or are not updated in parallel.
>=20
> To that end, I would rather prefer, trimming by removing conflicting/conf=
using text.

I wasn't clear in my review - I think making the references to other docume=
nts 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 ge=
t 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 th=
at is used in
> this document).

This example is sort of an interesting one to consider, as I was thinking m=
ore of the examples in which the format and semantics of the MAC TLVs are e=
xactly the same as in the cited defining documents.

>=20
> More comments in line.. and I look forward to your further guidance so we=
 can wrap this
> up in time.
>=20
> As a data point, there are two implementations of this draft deployed in =
a Telco network
> in Asia.

Noted.

>=20
>=20
> Thanks,
> Himanshu
>=20
>=20
> -----Original Message-----
> From: Ralph Droms (rdroms) [mailto:rdroms@cisco.com]
> Sent: Wednesday, October 14, 2015 4:02 PM
> To: A. Jean Mahoney; General Area Review Team; draft-ietf-pals-mpls-tp-ma=
c-wd.all@ietf.org
> Subject: [Gen-art] Review: draft-ietf-pals-mpls-tp-mac-wd
>=20
> I am the assigned Gen-ART reviewer for this draft. The General Area Revie=
w 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.
>=20
> For more information, please see the FAQ at
>=20
> <http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.
>=20
> 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
>=20
> Summary:
>=20
> This draft is on the right track but has open issues, described in the re=
view.
>=20
>=20
> Major issues:
>=20
> While this document is describing a straightforward adaptation of previou=
sly defined standards to statically provisioned PWs, in my opinion an imple=
mentor would not necessarily be able to construct a fully interoperable imp=
lementation from this document.  There are several sections of the document=
 that are not clear in their description of how to use mechanisms from refe=
renced documents in this standard.
>=20
> If it appears that some of my comments are overly finicky, I'll agree tha=
t the correct implementation could probably be deduced from the text in mos=
t 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 referen=
ced documents and head-scratching, which, in my experience, can lead to imp=
lementations that don't interoperate.
>=20
> [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 with=
in draft-ietf-pals-mpls-tp-mac-wd and between draft-ietf-pals-mpls-tp-mac-w=
d and cited documents.  What I think would improve this specification is cl=
arification 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.

>=20
> Section 1:
>=20
> 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.
>=20
> This text seems to be the only reference to the processing of an empty MA=
C List TLV. Specification of how the protocol works doesn't belong in the I=
ntroduction, and "wildcard MAC Address withdrawal" could certainly use some=
 more explanation.
>=20
> [Himanshu>] I would prefer taking the text out if its mention in "Introdu=
ction" section is not desirable.
> Wildcard MAC withdraw is a very well-known concept in VPLS architecture a=
nd needs no more description to L2VPNers, IMO.
> So absence of its reference in subsequent sections does not dilute the pu=
rpose of this document.

It wasn't obvious to me what is intended as a protocol specification and wh=
at is intended as a description of the protocol.  I see that RFC 4762 inclu=
des text that describes how to process an empty MAC List TLV, so perhaps re=
moval of the text altogether would be best.

>=20
> Section 3:
>=20
> The PW OAM message header is exactly the same as what is
> defined in [RFC6478].
>=20
> 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.
>=20
> [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.
>=20
> Perhaps -
> "The MAC withdraw PW OAM message follows the same guidelines used in [RFC=
6478],
> whereby first 4-bytes of OAM message header is followed by message specif=
ic
> field and a set of TLVs relevant for the message"
>=20
>=20
>=20
> a MAC address withdraw OAM message MUST contain a
> "Sequence Number TLV" otherwise the entire message is dropped.
>=20
> Is the "Sequence Number TLV" required to be the first TLV in the message?=
  Are the TLVs required to appear in any particular order?
>=20
> [Himanshu>] Yes. It is important to determine the "newness" of the messag=
e 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.
>=20
> 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 TL=
V 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).
>=20
> Does this mean that the message is an ACK if the A-bit is set?  Can an AC=
K contain a "Sequence Number" TLV?
>=20
> [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 sim=
ply 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 in=
dicate simple receipt of the MAC Address Withdraw OAM message or does it al=
so include successful processing?  Are there any error conditions that shou=
ld be signaled?  If not, perhaps that should be made explicit in the specif=
ication.

> 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.
>=20
> This paragraph is not at all clear to me. Reading section 4 of RFC 4385 h=
elped but left me unsure about how my understanding of how to extend the se=
quence number mechanism to 32 bits corresponds to the expectations of this =
document.
>=20
> [Himanshu>] OK.
> [Himanshu>] How about this paragraph -
>=20
> 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 r=
etransmitted
> older messages. The [RFC4385] describes details of sequence number handli=
ng which include
> overflow detection. This document leverages the same scheme, with the exc=
eption 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 i=
n 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 numbe=
r 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 numbe=
r, and the packet sequence number minus the expected sequence number is les=
s 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.

>=20
> Section 4.1:
>=20
> 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.
>=20
> 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 messa=
ge
>=20
> [Himanshu>]
> [Himanshu>] No. The text is correct. Each endpoint keeps the record of se=
nd and receive sequence number. These values are initialized to 1.
> If the sender sends the first MAC withdraw with sequence number 1, then r=
eceiver 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