Re: [Pals] Routing Directorate review of draft-ietf-pals-rfc4447bis

["Andrew G. Malis" <[e-mail]>]

Adrian,

Thanks for the review!

Cheers,
Andy

On Sun, Feb 21, 2016 at 10:16 AM, Adrian Farrel <[e-mail]> wrote:

> 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. 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 as the draft advances, and
> strive to resolve them through discussion or by updating the draft.
>
> Thanks,
> Adrian
>
> ===
>
> Document: draft-ietf-pals-rfc4447bis-03.txt
> Reviewer: Adrian Farrel
> Review Date: 21 February 2016
> IETF LC End Date: Not yet started
> Intended Status: Internet Standard
>
> = Summary =
>
> I have some minor concerns about this document that I think should be
> resolved before publication.
>
> = Comments =
>
> I'm OK to see 4447 being advanced to full standard, although I'm also a
> little surprised that there was energy to do it.
>
> The work is largely mechanical. My comments are a mix of minor process
> concerns (that your AD should be able to resolve), minor text
> clarifications, and nits.
>
> It's been a while since I was concerned with LDP, but I have no reason to
> doubt the integrity of this document.
>
> = Major Issues =
>
> No major issues found.
>
> = Minor Issues =
>
> I believe that this document should be marked as obsoleting 4447 and
> 6723. It will result in a new RFC with a new number, so 4447 will no
> longer be relevant. And, as Section 1 says: a new section (6.3) on
> control-word renegotiation by label request message has been added,
> obsoleting RFC 6723.
>
> By the way, it is section 7.3 (not 6.3) that you have added!
>
> [NB: Q16 of the Shepherd write-up seems a bit garbled on this front.
> Especially "Will publication of this document change the status of any
> existing RFCs?" seems to need a much clearer answer!]
>
> ---
>
> I'm not up to speed on the new process for advancing a specification to
> Internet-Standard when it has normative references that are not
> similarly advanced/advancing. I do know that the process changed
> recently and I am sure that your AD can look it up.
>
> ---
>
> It might be a reasonable question to ask why some of the
> (informatively) referenced related PW RFCs (such as encapsulations)
> are not being advanced at the same time, especially those that are
> described as "companion documents".
>
> ---
>
> It seems unnecessary petty-fogging process, but the inclusion of
> section 7.3 seems to me to defeat the stability requirement for
> advancement to Internet Standard unless your claim here is that you
> are advancing 4447 and 6723 together in this single document. That
> might be fine.
>
> ---
>
> The Errata that have been deliberately disregarded need to be marked as
> Rejected. I think the ones that have been actioned could usefully be
> marked as Verified or have a note added saying "fixed in RFCabcd".
>
> As far as I can tell:
>
> 3554 was not actioned
> 938 was done
> 86 was done
> 3555 was done
> 3115 no longer applies
> 3114 was done
> 3112 was done
> 1530 was done
>
> ---
>
> Because you (incorrectly) said that section 6.3 was new text, I gave it
> careful review. I think the text is in rather poor state:
>
> For example, in 6.3.1...
>
>    Using the procedures
>    outlined in this section, a simple label withdraw method MAY also be
>    supported as a legacy means of signaling PW status and AC status.
>
> ...Since this is *the* specification now, what does "legacy" mean?
>
> Indeed, you end 6.3.1 with...
>    The PW status
>    signaling procedures described in this section MUST be fully
>    implemented.
> ...which kind of implies ... hmm.
>
> I think you could rework 6.3.1 to handle this more gracefully and avoid
> the "clunk" of the 2nd para...
>
>    Once the PW status negotiation procedures are completed and if they
>
> ...where we are surprised to hear about status negotiation.
>
> For example:
>
> OLD
>    The PEs MUST send Label Mapping Messages to their peers as soon as
>    the PW is configured and administratively enabled, regardless of the
>    attachment circuit state.  The PW label should not be withdrawn
>    unless the operator administratively configures the pseudowire down
>    (or the PW configuration is deleted entirely).  Using the procedures
>    outlined in this section, a simple label withdraw method MAY also be
>    supported as a legacy means of signaling PW status and AC status.  In
>    any case, if the label-to-PW binding is not available the PW MUST be
>    considered in the down state.
>
>    Once the PW status negotiation procedures are completed and if they
>    result in the use of the label withdraw method for PW status
>    communication, and this method is not supported by one of the PEs,
>    then that PE must send a Label Release Message to its peer with the
>    following error:
>
>    "Label Withdraw PW Status Method Not Supported"
>
>    If the label withdraw method for PW status communication is selected
>    for the PW, it will result in the Label Mapping Message being
>    advertised only if the attachment circuit is active.  The PW status
>    signaling procedures described in this section MUST be fully
>    implemented.
> NEW
>    The PEs MUST send Label Mapping Messages to their peers as soon as
>    the PW is configured and administratively enabled, regardless of the
>    attachment circuit state.  The PW label should not be withdrawn
>    unless the operator administratively configures the pseudowire down
>    (or the PW configuration is deleted entirely).
>
>    Section 6.3.2 describes a mechanism for signaling the status of the
>    PW and AC.  Additionally, a simple label withdraw method MAY be used
>    to signal the PW status and AC status.  In any case, if the label-to-
>    PW binding is not available the PW MUST be considered to be in the
>    down state.
>
>    The PEs negotiate which PW and AC status signaling mechanism to use
>    by following the procedures in Section 6.3.3.  If the PW negotiation
>    procedures are completed and if they result in the use of the simple
>    label withdraw method for PW status communication, and if this method
>    is not supported by one of the PEs then that PE must send a Label
>    Release Message to its peer with the following error:
>
>    "Label Withdraw PW Status Method Not Supported"
>
>    If the label withdraw method for PW status communication is selected
>    for the PW, it will result in the Label Mapping Message being
>    advertised only if the AC is active.
>
>    The PW status signaling procedures described in Section 6.3.2 MUST be
>    fully implemented.
> END
>
> ---
>
> Section 6.3.2 achieves ambiguity by stating...
>    The general format of the Notification Message is:
> ...and then showing the specific case of "PW status".
>
> You can fix this by re-ordering and tweaking slightly, as follows.
>
> Note that the old text says:
>    Since this notification does not
>    refer to any particular message, the Message Id and Message Type
>    fields are set to 0.
> ...but I think the Message Type is set to 0x0001 :-)
>
>
> OLD
>    The Status TLV is transported to the remote PW peer via the LDP
>    Notification message.  The general format of the Notification Message
>    is:
>
>     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
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |0|   Notification (0x0001)     |      Message Length           |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                       Message ID                              |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                       Status (TLV)                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                      PW Status TLV                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |           PWId FEC TLV or Generalized ID FEC TLV              |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |               PW Grouping ID TLV (Optional)                   |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>
>
>    The Status TLV status code is set to 0x00000028, "PW status", to
>    indicate that PW status follows.  Since this notification does not
>    refer to any particular message, the Message Id and Message Type
>    fields are set to 0.
> NEW
>    The Status TLV is transported to the remote PW peer via the LDP
>    Notification message as described in [RFC5036].
>
>    The Status TLV status code is set to 0x00000028, "PW status", to
>    indicate that PW status follows.  The format of the Notification
>    Message for carrying the PW Status is 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
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |0|   Notification (0x0001)     |      Message Length           |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                       Message ID                              |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                       Status (TLV)                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |                      PW Status TLV                            |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |           PWId FEC TLV or Generalized ID FEC TLV              |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>    |               PW Grouping ID TLV (Optional)                   |
>    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
>
>    Since this notification does not refer to any particular message,
>    the Message Id field is set to 0.
> END
>
> ---
>
> Section 8 could be tidied a little.
>
> The use of "in general" gives the IANA no way to know whether they
> should or should not update a reference.
>
> Are you sure that you want to remove the nice, concise listing of code
> points that are present in RFC 4447?
>
> 8.1, 8.2, and 8.3 all say "new". I suggest deleting that word.
>
> And lastly, "IANA needs to..." is not what we say :-) "IANA is requested
> to..." is nicer.
>
> [NB: The Shepherd write-up seems to report all this wrongly at Q17]
>
> ---
>
> Shouldn't the whole of Section 10 be moved to an interop report
> somewhere? It's presence within the document seems somewhat temporally
> unstable. I understand that you want to write it down and record it
> somewhere, also that you probably developed it in the I-D as a useful
> place for working text. For advice about how and where to do this you
> can look at RFC 5657.
>
> You seem to have answered the four points from 6410, but may be missing
> a little corroboration.
>
> >  The pseudowire technology was first deployed in 2001 and has been
> >  widely deployed by many carriers. [RFC7079] documents the results of
> >  a survey of PW implementations, with specific emphasis on Control
> >  Word usage.  [EANTC] documents a public multi-vendor interoperability
> >  test of MPLS and Carrier Ethernet equipment, which included testing
> >  of Ethernet, ATM and TDM pseudowires.
>
> 7079 is a slightly related piece of survey work, but the only mention of
> LDP (which is the subject of *this* document) is to point out an issue
> with non-implementation.
>
> The EANTC reference really needs something that helps us find the report
> of the interop event. Possibly this is
>
> http://www.eantc.de/fileadmin/eantc/downloads/events/2007-2010/MPLSEWC09/EANTC-MPLSEWC2009-WhitePaper-v1.0.pdf
> Looking at that paper, it definitely supports the claim of multiple
> interoperating implementations of 4447.
>
> We lack, however, any statements about "widespread deployment and
> successful operational experience."
>
> >  The errata against [RFC4447] are all editorial in nature and have
> >  been addressed in this document.
>
> Good (except for those not addressed as above, and except that some of
> those errata are incorrectly marked as "Technical").
>
> >  All features in this specification have been implemented by multiple
> >  vendors.
>
> How do we know this?
> Further, are all the features used? One point of this step in the
> process is to prune out features that are unnecessary (and so complicate
> implementation and operation, and cost money).
>
> >  There is no IPR filed against this document or its predecessor.
>
> This may be overly vague (you probably mean "No IPR disclosures have
> been made to the IETF related to this document, to RFC 4447 or RFC 6723,
> or to the Internet-Drafts that resulted in RFC 4447 and RFC 6723.").
>
> ---
>
> = Nits =
>
> In the Abstract, probably...
>
> OLD
>    This document has been written to address errata in a previous
>    version of this standard.
> NEW
>    This document has been written to address errata in a previous
>    version of this specification.
> END
>
> But, actually, I think you are missing the main purpose which is to
> advance the spec to full standard.
>
> On reflection, I think that this paragraph probably served well in the
> draft as it was being processed, but can be removed now. The equivalent
> text obviously needs to be in the shepherd write-up.
>
> [ditto the paragraph in the introduction]
>
> ---
>
> Thank you for including Section 1. It's important and helpful.
>
> Three tiny points:
>
> Second para should probably s/However/Additionally/
>
> The RFC Editor requires that section 1 is the Introduction. A simple
> solution is to swap sections 1 and 2, or to move the current section 2
> to be the new section 1 and the current section 1 to be section 1.1.
>
> The text says "A note was added to clarify that the C-bit is part of
> the FEC." I think you should s/was/has been/ and you could also usefully
> give a little hint as to where the note was added.
>
> ---
>
> Shouldn't you mention the inclusion of text referencing 7358? This is
> clearly a change to the original text. I wonder, however, whether what
> really want to do is take that part of 7358 that updated 4447 and simply
> write it here so that this spec has no need of that reference.
>
> Either way, you also need to take care about the stability requirement
> for advancement.
>
> ---
>
> In 6.2.2.1
> please note that IANA has "PW Interface Parameters TLV" not "Interface
> Parameters TLV".
>
> The is a good chance to make this consistent.
>
> ---
>
> In 6.2.2.2 and the rest of the document, please note that the IANA has
> "PW Group ID TLV" not "PW Grouping ID TLV".
>
> The is a good chance to make this consistent.
>
> ---
>
> 6.3.2 has a figure which claims to include a "PWId FEC TLV" or a
> "Generalized ID FEC TLV". However, 6.1 and 6.2 describe these as
> "elements" not TLVs and they show up in the Forwarding Equivalence Class
> (FEC) Type Name Space.
>
> I think you can clarify 6.3.2 by explaining that what is included here
> is "a FEC TLV containing either a PWId FEC element or a Generalized FEC
> element.
>
> Then, just after the figure you have "The PW FEC TLV" but it is just a
> "FEC TLV"
>
> ---
>
> In 7.3 you have "re-provisioned". I wonder whether you mean
> "re-configured". "Provisioning" has a context initial set-up, so re-
> provisioning suggests (to me?) re-initialization.
>
> ---
>
> In the first para of 7.3 you have "PW C-bit negotiation procedure" and
> "Control-Word negotiation procedures".
>
> Could you decide on a single name, and on whether it is singular or
> plural.
>
> ---
>
> 7.3
> OLD
>         -i. If local PE has previously sent a Label Mapping message, it
>             MUST send a Label Withdraw message to remote PE and wait
> NEW
>         -i. If the local PE has previously sent a Label Mapping message, it
>             MUST send a Label Withdraw message to the remote PE and wait
> END
>
> ---
>
> 7.3
> OLD
>        -ii. the local PE MUST send a label release message to the remote
> NEW
>        -ii. The local PE MUST send a Label Release message to the remote
> END
>
> ---
>
> 7.3
> s/negotaiation/negotiation/
> s/renegotation/renegotiation/
>
> ---
>
> 7.3
>
>    The above C-bit renegotiation process SHOULD NOT be interrupted until
>    it is completed, or unpredictable results might occur.
>
> This is a bit odd. Normally the "or" that follows a "SHOULD" or "SHOULD
> NOT" explains the conditions under which you might want to vary the
> "SHOULD".
>
> But I'm struggling: interrupted by whom and by doing what?
>
> ---
>
> Some of the formatting in the references seems to have been hit with a
> global change of /. /.  / You might want to polish.
>
> ---
>
> The latest version of G.707 is dated January 2007. Is there any reason
> to not update the reference?
>
> ---
>
> Have a look at Section 8 of RFC 5036 for an example of a nice way to
> handle "legacy authors" and credits for past work.
>
> In any case, the RFC Editor will require that Section 15 is renamed
> "Contributors".
>
> ---
>
> There are two trivial nits from idnits.
>
>