Subject: [Lsr] Opsdir last call review of draft-ietf-ospf-ospfv3-segment-routing-extensions-16

From: Joe Clarke <[email-masked]>
Date: Fri, 26 Oct 2018 12:42:13 -0700

Reviewer: Joe Clarke
Review result: Has Nits

I have been assigned to review 
draft-ietf-ospf-ospfv3-segment-routing-extensions  on behalf of the ops
directorate.  This document defines OSPFv3 extensions needed for segment
routing (SR).  And therein lies my first nit.  While the document begins to set
forth this overarching scope, a small paragraph in section 1 further limits it
to MPLS dataplanes only.  I think perhaps the abstract should be updated to
clarify that.

Other items I found are listed below.

Overall, there are a lot of terminology used like RSVP, LDP, LSP, SID, etc.  I
think this document would benefit from a terminology section.

With respect to TLV types 8, 9, 14, and 15, they are defined in
draft-ietf-ospf-segment-routing-extensions, and it took me a while to figure
out where you were getting those values and why they weren't spelled out in the
IANA considerations.  You have a normative reference to this, which is good,
but you only mention it with respect to the algorithm parameters.  I think
another mention is required.

I'm going to be pedantic here.  According to RFC7770, when a new OSPF Router
Information LSA TLV is defined, the spec needs to explicitly state if it's
applicable to OSPFv2, v3, or both.  While you reference the TLVs from
draft-ietf-ospf-segment-routing-extensions, I didn't see that either document
_explicitly_ states that they are applicable to both.

===

Section 2.1

s/length is other then 3 or 4/length is other than 3 or 4/

===

Section 3.2

s/If more then one SID/Label/If more than one SID/label/

===

Section 3.2

"When a router receives multiple overlapping ranges, it MUST
      conform to the procedures defined in
      [I-D.ietf-spring-segment-routing-mpls]."

It would be useful to include a section pointer here.  I think your referring
to Section 2.3 where the router ignores the range?   Is it likely that will
change to something other than "ignore?"  If not, maybe it's just worth
mentioning that here.

===

Section 3.3

s/If more then one SID/Label/If more than one SID/Label/

===

Section 3.3

"The originating router MUST NOT advertise overlapping ranges."

You specify what a router should do if it receives overlapping ranges above.  I
think the same text should be used here, too.

===

Section 5

"Other bits: Reserved.  These MUST be zero when sent and are
         ignored when received."

The normative language changes.  In other places you say the bits SHOULD be 0. 
I suggest:

Other bits: Reserved.  These SHOULD be set to 0 when sent and MUST be ignored
when received.

===

Section 7.4.1

s/state lower then 2-Way/state lower than 2-Way/

===

---

From: Peter Psenak <[email-masked]>
Date: Tue, 30 Oct 2018 13:05:19 +0100

Hi Joe,

thanks for your review, please see inline (##PP):

> to MPLS dataplanes only.  I think perhaps the abstract should be updated to

##PP
Done

> think this document would benefit from a terminology section.

##PP
added

> _explicitly_ states that they are applicable to both.

##PP
added the following to each of the values:

Type: X as defined in [I-D.ietf-ospf-segment-routing-extensions] and 
aplicable to OSPFv3.

> s/length is other then 3 or 4/length is other than 3 or 4/

##PP
fixed

> s/If more then one SID/Label/If more than one SID/label/

##PP
fixed

> change to something other than "ignore?"  If not, maybe it's just worth

##PP
I don't think it is good to specify the behavior which is described 
somewhere else. Regarding the section, the 
ietf-spring-segment-routing-mpls is still being worked on and the 
section may changes. We used the same text in OSPFv2 and ISIS SR drafts. 
I would like to be consistent here.

> s/If more then one SID/Label/If more than one SID/Label/

##PP
fixed.

> think the same text should be used here, too.

##PP
Here we say that the originating router MUST NOT advertise overlapping 
ranges. We can not specify what it should do when it breaks the MUST.

We specify what other routers should do when they receive overlapping 
ranges and we refer it to spring-segment-routing-mpls draft. Again this 
is the same as we used in OSPFv3 and ISIS SR extensions. I would like to 
keep the consistency here.

> The normative language changes.  In other places you say the bits SHOULD be 0.

##PP
Whenever we refer to "other bits" in the flag fields we use the same 
language.

> Other bits: Reserved.  These SHOULD be set to 0 when sent and MUST be ignored

##PP
this refers to Reserved fields in the TLV (not the bits in a flag field) 
and again is used consistently across document.

> s/state lower then 2-Way/state lower than 2-Way/

##PP
fixed.

thanks,
Peter
> .

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 30 Oct 2018 14:46:41 +0000

Hi Peter, Joe, et al, 

On 10/30/18, 8:05 AM, "Peter Psenak (ppsenak)" <[email-masked]> wrote:

    Hi Joe,

    
    ##PP
    Done

    ##PP
    added

    ##PP

    
    ##PP
    fixed

    ##PP
    fixed

    ##PP

Given that this is a normative reference, I don't think it would create too much of a dependency to include the section in the reference. We've had a protracted discussion (1-2 years) on the whole SID overlap topic in SPRING and I believe we've finally come up with behavior and the specification of such behavior with which everyone agree (or at least doesn't strongly disagree). 

    ##PP
    fixed.

    ##PP

    
    ##PP

    ##PP

I agree. Use "These SHOULD be set to 0 when sent and MUST be ignored when received." For all reserved bits. 

Thanks,
Acee

    ##PP
    fixed.
    
    thanks,
    Peter

---

From: "Les Ginsberg (ginsberg)" <[email-masked]>
Date: Tue, 30 Oct 2018 15:15:05 +0000

Acee -

> Given that this is a normative reference, I don't think it would create too
> much of a dependency to include the section in the reference. We've had a
> protracted discussion (1-2 years) on the whole SID overlap topic in SPRING
> and I believe we've finally come up with behavior and the specification of
> such behavior with which everyone agree (or at least doesn't strongly
> disagree).
> 
[Les:] I strongly agree with Peter (and disagree with you).
Why would we want to risk having an incorrect section reference to a document which is still being revised? This needlessly introduces a dependency such that if the section numbering changes in the SR-MPLS draft we would then have to update the dependent document(s).
Note this has nothing to do with the SID overlap discussion itself. The compelling reason to NOT discuss this in the IGP documents but simply refer to the document that defines the solution is so that whatever the outcome in SPRING the IGP documents do not also have to be changed.

   Les

---

From: "Acee Lindem (acee)" <[email-masked]>
Date: Tue, 30 Oct 2018 15:33:21 +0000

Hi Les,

On 10/30/18, 11:15 AM, "Les Ginsberg (ginsberg)" <[email-masked]> wrote:

    Acee -

While I don't feel as strongly as either of you, this could improve the readability. For example, if you read RFC 8362 you'll see that I have referred extensively to sections in RFC 5340. I may be overoptimistic but I'm hoping we are finally done with the SR-MPLS draft as it is blocking all our LSR SR documents. 

Thanks,
Acee
    
       Les

---

From: Benjamin Kaduk <[email-masked]>
Date: Tue, 30 Oct 2018 20:16:32 -0500

> While I don't feel as strongly as either of you, this could improve the readability. For example, if you read RFC 8362 you'll see that I have referred extensively to sections in RFC 5340. I may be…

I also agree that specific section references can (in general) aid
readability.  And there's always "[RFC Editor: please check with authors
during AUTH48 that the section reference remains correct]"; we've done
essentially that on a document I was shepherding in the past.

-Benjamin

---

From: Joe Clarke <[email-masked]>
Date: Tue, 30 Oct 2018 23:15:27 -0400

> Type: X as defined in [I-D.ietf-ospf-segment-routing-extensions] and

Thanks.  But s/aplicable/applicable/ :-)

> I would like to be consistent here.

I can agree that copying might be problematic.  But I think a section
ref is good here.  Finding the specific part about "overlapping" in this
document is kind of like a needle in a haystack.  I think it will add to
overall readability.

> ranges. We can not specify what it should do when it breaks the MUST.

I meant you have used text as to what happens when a router receives
data it should ignore in other parts.  I was asking to use similar text
here.

> keep the consistency here.

Right.  But you don't re-reference that text here.  Again, I'm just
asking for consistent text that references the
spring-segment-routing-mpls drafts.

Joe

---

From: Peter Psenak <[email-masked]>
Date: Wed, 31 Oct 2018 08:38:29 +0100

Hi Ben, Acee,

> essentially that on a document I was shepherding in the past.

ok, added the section reference.

thanks,
Peter

---

From: Peter Psenak <[email-masked]>
Date: Wed, 31 Oct 2018 08:49:34 +0100

Hi Joe,

> Thanks.  But s/aplicable/applicable/ :-)

fixed :)

> document is kind of like a needle in a haystack.  I think it will add to

added the section reference.

> spring-segment-routing-mpls drafts.

got it and updated the text.

thanks,
Peter