Hello, I have been selected as the Routing Directorate reviewer for this draft. The Routing Directorate seeks to review all routing or routing-related drafts as they pass through IETF last call and IESG review, and sometimes on special request. The purpose of the review is to provide assistance to the Routing ADs. For more information about the Routing Directorate, please see http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir Although these comments are primarily for the use of the Routing ADs, it would be helpful if you could consider them along with any other IETF Last Call comments that you receive, and strive to resolve them through discussion or by updating the draft. Document: draft-ietf-pals-status-reduction-01.txt Reviewer: Adrian Farrel Review Date: 27 September 2016 IETF LC End Date: Not yet started Intended Status: Standards Track ==Summary:== I have some minor concerns about this document that I think should be resolved before publication. The volume of minor concerns add up to a significant concern although no one issue is large. I recommend that the Routing ADs discuss these issues further with the authors and consider whether an updated I-D would need to be re-reviewed by the WG. ==Comments:== This document defines a mechanism to "bundle" status reports on the PWs carried on an MPLS tunnel. The effect is to significantly reduce the number of messages need in the (normal) stable state. I don't see any problems with the mechanism defined and I am sure it would work. The writing style is mainly clear. However, there is a host of minor issues and holes in the documentation that puts the specification below the level necessary to guarantee interoperable implementations. My review is not as an implementer, and it worries me that if I find so many questions and issues, an implementer would find far more. I checked to see whether anyone had commented in WGLC that they had read the document and I didn't see anything (perhaps my list subscription is broken?) ==Minor Issues:== Section 1.3 I think you need to give a reference for the base BNF you are using. You could choose from RFC 5234 or RFC 5511. But it seems peculiar that you have set out to define your own notation for things that can be expressed in existing BNF. Section 4 The figure makes it look like MPLS labels are 32 bits long. I guess you could fix this by s/label/label stack entry/ Or you could show all of the LSE fields. Section 4 You have not described all of the fields in the figure. Section 4 You have... Channel type 0xZZ pending IANA allocation. ...but no mention in the IANA section The figure shows "0xZZ PW OAM Message", but surely this is "PW Status Reduction Message" Section 4 In the description of the Session ID CRC16 seed really "recommended"? I mean, is this "RECOMMENDED", and if so, why? You *do* need a locally selected, locally unique value. It does not need to be unique over time, but you may need suggest a damping mechanism on re-use of session ID. However, recommending this mechanism for generating session IDs seems to me to be over-weaning. What is the reason? Section 4 There are a number of rules expressed here about the setting of the message fields (such as the Refresh Timer that is a "non zero unsigned 16 bit integer value greater or equal to 10"). What happens if a message is received that violates one of these rules. Section 4 OLD 7 bits of flags reserved for future use, they MUST be set to 0 on transmission, and ignored on reception. NEW 6 bits of flags reserved for future use, they MUST be set to 0 on transmission, and ignored on reception. END Section 4 Do you want IANA to track the flags field? Section 4 You have... It should be noted that the Checksum, Message Sequence Number, Last Received Message Sequence Number, Message Type, Flags, and control message body are OPTIONAL. This *sounds* like each field is individually optional. But that is not the case, I think (because the resulting message would be impossible to parse). So you need to clarify. Probably that the length field indicates where the sequence stops, or that the whole set may be omitted or included en mass. Section 5 opening paragraphs are confusing. Either you have a "control message" or you have a "control construct carried on a status reduction message". - It is possible you mean the former. That is "There are two status reduction messages defined in this document and they are both control messages: the Notification message and the PW Configuration message. - It is possible you mean have the latter. Thus, when you talk about the "message sequence number of the control message" you really mean the "message sequence number of the status reduction message that carries the control construct". You can probably note that the control construct can be carried on a Notification message or a PW Configuration message. You should probably also say whether the control construct can be carried on other (future) messages. Section 5 In 8.3 you list a number of notification codes. A little can be deduced from their names, but you do not describe anywhere when or why most of them are used (5.0.2.3 and 6 are the exceptions). You should, and section 5 or 6 is probably the place. Section 5.0.1 What is the setting of the C flag on the Notification message? Section 5.0.2 The message has no preset length limit, however its total length will be limited by the transport network Maximum Transmit Unit (MTU). This is fine, however: - You probably want to add "Status Reduction messages MUST NOT be fragmented." - You probably also want "If a sender has more configuration information to send than will fit into one PW Configuration Message it may send further messages carrying further TLVs." Section 5.0.2 I think you need to define a generic TLV format so that new TLVs can be added and parsed over. Since you use a common format for your TLVs, this should not be hard. However, you also need to describe how "unknown" TLVs are handled. Section 5.0.2.1 Is the MPLS-TP Tunnel ID TLV optional? 5.0.2.2 shows the PW ID configured List TLV as optional, so it looks like the MPLS-TP Tunnel ID might be mandatory TLV. Furthermore, you seem to have said that the order of TLVs is arbitrary but wouldn't it be best to have this TLV show up first? What happens if there are multiple of this TLV present? Section 5.0.2.2 (and 5.0.2.3) The number of PW Path IDs in the TLV will be inferred by the length of the TLV up to a maximum of 8. Now, "The PW Path ID is a 32 octet pseudowire path identifier" 8*32=256 The Length field has 8 bits and so encodes a maximum of 255 octets. So you can actually only carry 7 PW Path IDs in this TLV. Furthermore, Length values that are not a multiple of 32 are an encoding error, but you haven't stated how to handle encoding errors. What happens if the same PW Path ID appears twice in a list? Section 6 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. I think that probably hidden in here is "MUST NOT include a PW configuration message on a status reduction message unless the local protocol state is ACTIVE." Section 6 the following action SHOULD be taken: -i. The local PW MUST be considered in "Not Forwarding" State. That creates a composite "SHOULD-MUST" which is not very clear. Suggest you change to... the following actions are taken: -i. MUST -ii. SHOULD -iii. SHOULD Section 7 Why does this section not discuss the Checksum field? Doesn't this add a measure of security (at least against simple, random attacks)? Should you also give guidance (here or in section 4) about when it is acceptable to use a zero checksum. Section 8.1 - meta-point to be addressed before the following issue with this section... You seem to have an overly-complex assignment policy for this registry. All of the types seem to have equal weight (i.e., there are no "special purpose" values yet you cover the range with: - Expert review - IETF review - FCFS Since FCFS guarantees an assignment without any further review, why would anyone bother with Expert Review. For that matter, why would anyone bother with IETF Review? So, why not make life easier and say all code points are FCFS? Section 8.1 s/IETF consensus/IETF review/ Section 8.1 You have a range assigned by Expert Review. It is a requirement that you give guidance to the experts about how they should review requests. Section 8.1 The values 128 through 254 are assigned using FCFS. You cannot then attempt to further constrain how the values are used (e.g., "for vendor proprietary extensions"), and you should avoid the word "reserved" since it has special meaning for IANA. Section 8.2 I have essentially an identical set of points about 8.2 carried from 8.1 Section 8.3 Again the same set of comments. Additionally, you need to tell IANA what the "Error?" column means. This is probably... For each value assigned IANA should also track whether the value constitutes an error as described in Section 5.0.1. When values are assigned by IETF Review, the setting of this column must be documented in the RFC that requests the allocation. For Expert Review and FCFS assignments, the setting of this column must be made clear by the requested at the time of assignment. ==Nits:== I believe Luca may want to change his reported affiliation. Abstract OLD This document describes a method for generating an aggregated pseudowire status message on Multi-Protocol Label Switching (MPLS) network Label Switched Path (LSP). NEW This document describes a method for generating an aggregated pseudowire status message transmitted on a Multi-Protocol Label Switching (MPLS) Label Switched Path (LSP) to indicate the status of one or more pseudowires carried on the LSP. END Introduction Expand "PW" on first use. s/they are setup/they are set up/ Section 2 "MPLS Generic Associated Channel" needs a reference to 5586 Throughout the document you need to be consistent (and with prior art) about capitalisation ("generic associated channel" or "Generic Associated Channel" etc.) and abbreviations ("GACH" or "G-ACh" etc.) Section 4 Message Sequence Number s/A unsigned/An unsigned/ Section 4 (from the figure) OLD | Last Received Seq Number | Message Type |U C Flags | NEW | Last Received Seq Number | Message Type |U|C|Flags | END Section 4 Unknown flag bit. s/MUST be acknowledge/MUST be acknowledged/ The sub-section numbering in Section 5 is broken. You can have, for example, "5.0.1". In a number of places you have "sub-TLVs" and in other places "TLVs". I think they are all "TLVs". In 5.0.2.1 "the address of the MPLS-TP tunnel ID" is confused. It is "the identifier of the MPLS tunnel". Indeed, where did MPLS-TP suddenly come from since you want this to work for all LSPs (see also 8.2). 2.1.1, 2.1.3, and 5.0.2.3 "unprovisioned" I think the word is "deprovisioned" Section 6 If a PE receives such a notification it should stop sending PW configuration control messages for the duration of the PW refresh reduction keepalive session. s/should/SHOULD/ Section 8 All the registries in this section are to be created or updated as apropriate in the PW Name Spaces. No such name space error. I think you mean "Pseudowire Name Spaces (PWE3)" s/apropriate/appropriate/ s/10. Author's Addresses/10. Authors' Addresses/