Subject: [Gen-art] Gen-ART Last Call review of draft-ietf-ospf-segment-routing-msd-17

From: Paul Kyzivat <[email-masked]>
Date: Mon, 27 Aug 2018 13:33:01 -0400

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 
<​http://wiki.tools.ietf.org/area/gen/trac/wiki/GenArtfaq>.

Document: draft-ietf-ospf-segment-routing-msd-17
Reviewer: Paul Kyzivat
Review Date: 2018-08-27
IETF LC End Date: 2018-08-29
IESG Telechat date: ?

Summary:

This draft is on the right track but has open issues, described in the 
review.

Issues:

Major: 0
Minor: 3
Nits:  1

1) MINOR:

The abstract says:

    This
    document defines only one type of MSD, but defines an encoding that
    can support other MSD types.

and later section 1 says:

    It also defines
    the Base MPLS Imposition MSD type.

I can find nothing in the document that does this definition. It seems 
that this definition is actually done by 
draft-ietf-isis-segment-routing-msd, which is referenced. The text needs 
to agree with the structuring of the documents.

2) MINOR:

Section 1 also says:

    Although MSD
    advertisements are associated with Segment Routing, the
    advertisements MAY be present even if Segment Routing itself is not
    enabled.

I found nothing else in the document that elaborated on this. Further 
explanation is needed, in a section other than the introduction.

3) MINOR:

The first paragraph of section 4 says:

    When Link MSD is present for a given MSD type, the value of the Link
    MSD MUST take preference over the Node MSD.  When a Link MSD type is
    not signalled but the Node MSD type is, then the value of that *Link*
    MSD type MUST be considered as the corresponding *Node* MSD type
    value.

This appears to have scrambled the use of Link and Node a bit. I think 
it is intended to say:

    When Link MSD is present for a given MSD type, the value of the Link
    MSD MUST take preference over the Node MSD.  When a Link MSD type is
    not signalled but the Node MSD type is, then the value of that Node
    MSD type MUST be considered as the corresponding Link MSD type value.

4) NIT:

Section 2 uses: 'The Type:' when documenting the "Type" field of the 
TLV,  while sections 3 uses: 'Type:'. These ought to be consistent.

---

From: Jeff Tantsura <[email-masked]>
Date: Mon, 27 Aug 2018 11:18:50 -0700

Hi Paul,

Many thanks for your review!
Please see inline

Cheers,
Jeff

On 8/27/18, 10:33, "Paul Kyzivat" <[email-masked]> wrote:

    
    Summary:

    Issues:
    
    Major: 0
    Minor: 3
    Nits:  1
    
    1) MINOR:
    
    The abstract says:
    
        This

        It also defines
    
[jeff] ack, in the previous version BMI was defined on both, ospf and isis, removed
    
    2) MINOR:
    
    Section 1 also says:
    
        Although MSD
    
    I found nothing else in the document that elaborated on this. Further 
    explanation is needed, in a section other than the introduction.
[jeff] the meaning of the statement is that MSD MAY be advertised even if Segment Routing is not enabled, ie MSD(s) advertisement could be used by any other application that would find the data provided of value. I would appreciate, if you could be more specific about what's missing. 
    
    3) MINOR:

    
    
[jeff]ack, changed
    
    4) NIT:
    
[jeff]ack, changed

---

From: Paul Kyzivat <[email-masked]>
Date: Mon, 27 Aug 2018 15:50:28 -0400

Jeff,

> [jeff] the meaning of the statement is that MSD MAY be advertised even if Segment Routing is not enabled, ie MSD(s) advertisement could be used by any other application that would find the data pro…

Perhaps this is obvious to the likely readers, but not to me...

(As someone with absolutely no knowledge of OSPF I found the specifics 
of the document to be nearly impenetrable. But I'm not part of the 
target audience.)

I thought this information was specific to segment routing (whatever 
that is), so what is the point of having this information if segment 
routing isn't enabled? How would it be used? For me some discussion of 
that would be helpful.

In any case, AFAIK an "introduction" serves to introduce the material 
that is covered in more depth later in the document. In this case it 
isn't introducing anything that comes later. If it is to only appear in 
one place, then I would expect it to be later in the document.

	Thanks,
	Paul

---

From: Jeff Tantsura <[email-masked]>
Date: Mon, 27 Aug 2018 15:18:52 -0700

Hi Paul,

Yes, I completely agree with you, outside of routing context the document is quite difficult to parse, but so are most routing protocol extensions documents, they built on top of "well known" (to routing community) data. 

MSD (Type) is a particular capability as supported and configured by a node or a link on a node.
While the most obvious use case is SR indeed (SR stacks MPLS labels that define characteristics of a path thought the network), number of labels a system can impose is finite and that is the number  advertised by BMI-MSD  (as defined in isis draft) . There are other types, some of them already defined (max readable depth for entropy), some of them are going to be defined on the future.
So there could be another application (not SR) that could use the data(MSDs) distributed thru OSPF/ISIS.

Please note - there's. text in isis draft, that is a response to the comment similar to your 

"Note that in a non-SR MPLS network, label depth is what is defined by the MSD advertisements."

Would this comment work for you?
Many thanks!

Cheers,
Jeff

On 8/27/18, 12:50, "Paul Kyzivat" <[email-masked]> wrote:

    Jeff,

    

    
    
    	Thanks,
    	Paul

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 28 Aug 2018 10:59:56 -0400

> Yes, I completely agree with you, outside of routing context the document is quite difficult to parse, but so are most routing protocol extensions documents, they built on top of "well known" (to r…

Yes. I've also reviewed a couple of others, and it is difficult to get 
to the point that I can make any sense of the docs, even without trying 
to understand the subtleties that are being addressed. But that isn't 
necessarily a problem as long as the docs make sense to the intended 
audience.

> Would this comment work for you?

I've raised the issue. I'll be satisfied with whatever you think is the 
right fix, if any.

	Thanks,
	Paul

---

From: Jeff Tantsura <[email-masked]>
Date: Tue, 28 Aug 2018 10:30:39 -0700

Hi Paul,

Many thanks!
I have uploaded .18 version that should address your comments.
Please let me know.

Thanks! 

Cheers,
Jeff

On 8/28/18, 07:59, "Paul Kyzivat" <[email-masked]> wrote:

    

    	Thanks,
    	Paul

---

From: Paul Kyzivat <[email-masked]>
Date: Tue, 28 Aug 2018 16:25:01 -0400

> I have uploaded .18 version that should address your comments.

Looks good.

I forgot to run IdNits before, so I ran it now on -18. It is reporting 
one warning:

   == Outdated reference: A later version (-14) exists of
      draft-ietf-isis-segment-routing-msd-13

	Thanks,
	Paul

---

From: Jeff Tantsura <[email-masked]>
Date: Tue, 28 Aug 2018 14:12:18 -0700

Hi Paul,

Thanks!
This is a glitch in the system (resolution to an incorrect draft version), there are no references to a particular version of isis draft. Not sure why though...

tantsura$ grep isis draft-ietf-ospf-segment-routing-msd-18.xml
         
defined in <xref target="I-D.ietf-isis-segment-routing-msd"/>.</t>
defined in <xref target="I-D.ietf-isis-segment-routing-msd"/>.</t>
 <?rfc include="reference.I-D.ietf-isis-segment-routing-msd"?>

Hope this clarifies. 

Cheers,
Jeff

On 8/28/18, 13:25, "Paul Kyzivat" <[email-masked]> wrote:

    Looks good.

    
    	Thanks,
    	Paul