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 
<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-bess-nsh-bgp-control-plane-13.txt
Reviewer: Ravi Singh
Review Date: 01-26-2020
Intended Status: Standards Track

Summary:
I have some minor concerns about this document that I think should be resolved 
before publication.


Comments: 
0.  Having finished reading through the entire doc, the whole picture comes 
together clearly. 
However, given the size of this document…..as one is reading this document it 
takes a high level of effort to stay with the picture.
I feel the readability could be somewhat improved.  See #1 under “minor issues” 
for a suggestion.

Major Issues:
No major issues found.

Minor issues:

1.
Section2: During an initial reading, the terminology comes across as overly 
repetitive and a bit pedantic….which takes away from the readability a bit when 
one is reading the sections in the order listed. Content of RFC7665/8300 are 
also contributors to this.
Eg: "In fact, each SI is mapped to one or more SFs that are implemented by one 
or more Service Function Instances (SFIs) that support those specified SFs. "  
: almost sounds like legalese when someone who has not grasped the complete 
picture of the draft.
 
Wrote up the above as I was reading that section for the first time.
However, after finishing the review…started to appreciate this.
 
In having read the sections in order, I think the readability would have been 
greatly enhanced if I had read section 8 first, else once just gets lost in all 
the details.
It would be worthwhile suggesting in the text that once the reader has read 
through section 2, it would help to read section 8 before reviewing the 
intervening sections.
 
2. Section 3.2.1: page 16:
"[RFC7606]
   revises BGP error handling specifically for the for UPDATE message,"
 
->
"[RFC7606]
   revises BGP error handling specifically for the UPDATE message,”

3. 
Page 17:
  a. Was the intention for treating error 4 any different from errors, 
[1,2,6,7]? If not, why the need to call out 4 separately?
  b. "Unknown SFIR-RD found in a Hop TLV." :
The format of the Hop TLV in section 3.2.1.2 contains no reference to an RD. 
So, was the intention instead to refer to the SFIR-RD list of one of the SFT 
TLVs inside the Hop TLV?

4.
Section 3.2.1.2: "At least one sub-TLV MUST be present. "
Where are these sub-TLVs defined?
In this regard,
  a. Section 3.2.1.3 says "The SFT TLV MAY be included in the list of sub-TLVs 
of the Hop TLV.":
  b. Section 3.2.1.4 says "The MPLS Swapping/Stacking TLV (Type value 4) is a 
zero length sub-TLV that is OPTIONAL in the Hop TLV "

In the absence of specific mention of details of sub-TLVs in section 3.2.1.2, 
is a reader to assume that SFT TLVs & the MPLS swap/stack TVLs are the only 
possible subTLVs under a hop-TLV (as intended in this revision of the ID)?
This aspect becomes clear later, when one gets to reading the description of 
the subsequently mentioned TLVs. Might be worth alluding to this in 3.2.1.2.

5.
"In the normal case the SPI remains unchanged and the SI will have been 
decremented to indicate the next SF along the path.": will SI really be 
decremented instead of just setting it to the appropriate value? As per this 
draft, set to an appropriate lower value…

6.
What exactly does the following mean? "Also, as described in [RFC8300], an SFF 
receiving an SI that is unknown in the context of the SPI can reduce the value 
to the next meaningful SI value in the SFP indicated by the SPI. If no such 
value exists or if the SFF does not support this function, the SFF drops the 
packet and should log the event: such logs are also subject to rate limits."

7. 
Figure 1: showing the SFIs hosted on SFF2 and SFF3 in the same conceptual block 
(just because they share the same type) is a bit confusing when the diagram is 
showing a logical view of the physical layout of the elements of the solution.

8.
Section 3.1:the following is an ambiguous sentence & I suggest rewording to 
clarify:
"Note that it is assumed that each SFF has one or more globally unique SFC 
Context Labels and that the context label space and the SPI address space are 
disjoint." 
This sounds like that the "context label space" and the "SPI address space" are 
disjoint w.r.t. each other. What appears to be intended is that the SFC context 
labels and SPI-address-space should be disjoint across the different SFFs.
However, is it not sufficient to just have the SFC-context label spaces be 
disjoint across SFFs?

9.
Section 3.2: Why is "If two SFPRs are originated from different Controllers 
they MUST have different RDs" needed when "All SFPs MUST be associated with 
different RDs." is already stated?

10.
Section 3.2.1: "The Extended Length bit is set according to the length of the 
SFP attribute as defined in [RFC4271].": minor quibble: but this sentence needs 
rewording for correctness of intended meaning.

11.
Pg26: Typo in "This makes the bevahior "

12.
Section 5: pg 29: suggest rewording "Thus, at any point in time when an SFF 
selects its next hop, it chooses from the intersection of the set of next hop 
RDs contained in the SFPR and the RDs contained in its local set of SFIRs." to
"Thus, at any point in time when an SFF selects its next hop, it chooses from 
the intersection of the set of next hop RDs contained in the SFPR and the RDs 
contained in the SFPR's local set of SFIRs."

13.
Section 5 pg 29: Not clear "Similarly, when this condition obtains " what is 
intended here?

14.
Section 6: typo in "If the SPI indicates anther path,"

15.
Section 6.1: SI (As defined in SFIR format in section 3.1) is a 1 octet 
quantity. So, want to make the SI field 1 byte long and the reserved field 4 
bytes long?

16.
Section 6.1: "Note that Special Purpose SFTs MUST NOT be advertised in SFIRs.": 
what is the intended behavior if these were so advertised?

17.
Section 7.4: "that can be used to disposition those packets" and "it MUST NOT 
be used for dispositioning the packets of the specified" feels a little 
strange: perhaps the RFC-editor will have more to say about this, if this 
really is strange

18.
Section 7.6: "with that SFP’ last hop." -> "with that SFP’s last hop."

19.
Section 8.4: "path selecting between all SFF2 that support an SF of type 43 and 
SFF3 that supports" could use some rewording.

20.
Section 8.7: "[SI = 245, SFT = 42, RD = 192.0.2.3:7]" could be made more in 
line with the encoding, by showing it as [SFT = 42, RD = 192.0.2.3:7] with the 
SI=245 being nested under [SI=245, …] on the lines of as is done in section 8.4.

21.

Section 8.9.2: what is gained by having a given SFI be identified using 
multiple different SI s?
Eg:
[SI = 255, SFT = 41, RD = 192.0.2.1:11],
And
[SI = 253, SFT = 41, RD = 192.0.2.1:11]
 
The above 2 representations are for the same SFI: i.e. SFT & RD are the same. 
So, why represent them using different SI s?
This ties to the question about: why worry about the numeric ordering of the SI 
values in a given SFP?

Regards
Ravi




_______________________________________________
BESS mailing list
BESS@ietf.org
https://www.ietf.org/mailman/listinfo/bess

Reply via email to