Subject: [Pce] Genart last call review of draft-ietf-pce-stateful-path-protection-08

From: Pete Resnick via Datatracker <[email-masked]>
Date: Wed, 28 Aug 2019 20:04:52 -0700

Reviewer: Pete Resnick
Review result: Ready

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

<https://trac.ietf.org/trac/gen/wiki/GenArtfaq>.

Document: draft-ietf-pce-stateful-path-protection-08
Reviewer: Pete Resnick
Review Date: 2019-08-28
IETF LC End Date: 2019-08-28
IESG Telechat date: Not scheduled for a telechat

Summary: Ready

No issues of substance that I can see. A few editorial suggestions below, but
nothing earth-shattering.

Major issues: None.

Minor issues: None.

Nits/editorial comments:

Purely editorial suggestions:

Section 3.1:

Delete:
   This document defines a new Association type, the "Path Protection
   Association Type", value will be assigned by IANA (TBD1).

You already say this in the first paragraph.

Section 3.2:

OLD
   The type (16 bits) of the TLV is to be assigned by IANA.  The length
   field (16 bit) has a fixed value of 4.
NEW
   The type (16 bits) of the TLV is TBD2.  The length field (16 bit)
   has a fixed value of 4.

It would probably be caught by the RFC Editor the way you had it, but this way
IANA and the RFC Editor can search and replace for anything with "TBD".

OLD
     |         Type = TBD2         |              Length             |
NEW
     |         Type = TBD2         |          Length = 4             |

OLD
     |   PT      |     Path Protection Association Flags         |S|P|
NEW
     |   PT      |                Unassigned                     |S|P|

Section 6:

At the top of the section, I suggest putting in the following:

[Note to RFC Editor and IANA: Sections 3.1, 3.2, and 4.5 contain "TBD1" through
"TBD5" those should be replaced by the values that IANA assigns. Also, Section
4.5 includes several occurrences of the phrase "(Early allocation by IANA)";
please confirm that the value mentioned there is correct and delete that phrase
from the document before publication.]

---

From: Dhruv Dhody <[email-masked]>
Date: Thu, 29 Aug 2019 15:47:08 +0530

Hi Pete,

Thanks for your review and nits. Just snipping to two points...

>      |   PT      |     Path Protection Association Flags         |S|P|

I feel it is important to keep the name "flags" in the figure to match
with the text following the figure. Also this seems to be our usual
practice in past documents as well. We can change to just "flags" if
you would prefer that?

For context ->

   The format of the Path Protection Association TLV (Figure 1) 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
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |         Type = TBD2         |              Length             |
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
     |   PT      |     Path Protection Association Flags         |S|P|
     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

             Figure 1: Path Protection Association TLV format

   Path Protection Association Flags (32 bits) - The following flags are
   currently defined -

      Protecting (P): 1 bit - This bit is as defined in Section 14.1 of
      [RFC4872] to indicate if the LSP is working or protection LSP.

      Secondary (S): 1 bit - This bit is as defined in Section 14.1 of
      [RFC4872] to indicate if the LSP is primary or secondary LSP.  The
      S flag is ignored if the P flag is not set.

      Protection Type (PT): 6 bits - This field is as defined in
      Section 14.1 of [RFC4872] to indicate the LSP protection type in
      use.

      Unassigned bits are considered reserved.  They MUST be set to 0 on
      transmission and MUST be ignored on receipt.

> from the document before publication.]

I would suggest the authors to remove the phrase "(Early allocation by
IANA)" in the document now as the referenced draft is in RFC-EDITOR
queue and the early allocation tag is removed in the IANA page -
https://www.iana.org/assignments/pcep/pcep.xhtml#pcep-objects

Thanks!
Dhruv

---

From: "Pete Resnick" <[email-masked]>
Date: Thu, 29 Aug 2019 10:55:38 -0500

Hi Dhruv,

On 29 Aug 2019, at 5:17, Dhruv Dhody wrote:

>>      |   PT      |                Unassigned                     
>> |S|P|

So this is what confused me about the diagram: "Path Protection 
Association Flags" is the entire 32-bit field, which includes PT, S, and 
P. But in the diagram, you have the unassigned 24 bits labeled "Path 
Protection Association Flags", which seems incorrect. Perhaps 
"Unassigned Flags" would be best.

> https://www.iana.org/assignments/pcep/pcep.xhtml#pcep-objects

That's fine too.

pr
-- 
Pete Resnick http://www.episteme.net/
All connections to the world are tenuous at best

---

From: Mahend Negi <[email-masked]>
Date: Sat, 31 Aug 2019 15:01:09 +0530

Hi Dhruv/Pete,

Thanks for the review, new version handles the comments.

FYI:
New Version:
https://datatracker.ietf.org/doc/html/draft-ietf-pce-stateful-path-protection
Diff             :
https://www.ietf.org/rfcdiff?url2=draft-ietf-pce-stateful-path-protection-09

Thanks,
Mahendra