Subject: [sfc] Opsdir last call review of draft-ietf-sfc-nsh-18

From: Jürgen Schönwälder <[email-masked]>
Date: Fri, 04 Aug 2017 07:44:45 -0700

Reviewer: Jürgen Schönwälder
Review result: Has Issues

I am the OPS-DIR reviewer and in general the document seems to be in
good shape. I understand that the WG will work on management
specifications later and so I will not ask for them now. ;-)

The only section that worries me a bit is the fragmentation
considerations section, which points to a not helpful section of an
expired I-D. I understand that MSH assumes that the underlay will
provide a large enough MTU. Still, I think it would help if the spec
would spell out what happens if on a mis-configured network the MTU
does not fit and I think the document should require that such events
(not all of them but sufficiently many) are logged so that such
problems can be identified. (Hunting MTU black-holes is no fun.)

The rest is mostly editorial, consider the comments as you see fit.

- Introduction

  I was not sure what a 'service plane protocol' is and I think this
  term is not needed here either. Here is a possible rewrite:

  OLD

    Network Service Header (NSH) defines a new service plane protocol
    specifically for the creation of dynamic service chains and is
    composed of the following elements:

  NEW

    Network Service Header (NSH) defines a new protocol for the
    creation of dynamic service chains and is composed of the
    following elements:

  That said, it is somewhat odd that a header defines a
  protocol. Perhaps more accurate:

  NEWER

    Network Service Header (NSH) is used by a new protocol for the
    creation of dynamic service chains and is composed of the
    following elements:

  I figured out that 'service plane' is defined later informally but
  as a first time reader I think it is preferable to avoid this term
  in the introduction.

- Definition of Terms

    Network Node/Element:  Device that forwards packets or frames based
       on an outer header (i.e., transport) information.

  I got confused by '(i.e., transport)' likely because transport can
  have multiple meanings. Perhaps simply remove '(i.e., transport)'?

    NSH-aware  SFC-encapsulation-aware, or SFC-aware [RFC7665].

  Does this mean all three terms mean the same thing and they are
  interchangeable? I guess not. I assume NSH-aware means SFC-aware
  when using an NSH, i.e., SFC-aware is the general concept and
  NFC-aware is the concrete case when NSH is used. It would be nice
  to   not have to guess...

- NSH-based Service Chaining

  Back to the term 'service plane' - should this be a defined term?
  It is defined informally here but it seems to be used in several
  places, perhaps it deserves a definition in the terminology section.

  What exactly is a 'network transport protocol'?

- NSH Base Header

  I tried to search for the explanation of U in Fig. 2 and did not
  find it. Perhaps change

    All other flag fields are unassigned [...]

  to

    All fields marked U are unassigned [...]

- Optional Variable Length Metadata

  s/order their appear/order they appear/

  s/must process first instance/must process the first instance/

- NSH Actions

  s/un-encapsulated packet/un-encapsulated packet./

  s/selection a different/selection of a different/

- Fragmentation Consideration

  The document refers to draft-ietf-rtgwg-dt-encap-02.txt, more
  specifically section 6. However, this I-D has expired. Section 6 of
  this document is a terminology section. Perhaps the pointer should
  have been to section 9? But even then, does the referenced text
  really help? Would it make sense to inline the text that is supposed
  to help here?

  I think this is a real management and operations concern. I assume
  (it is not state explicitly) that packets are dropped if
  fragmentation is needed but there is no way to fragment. Is there
  also an error reporting mechanism? Or a logging mechanism? Or are
  these then silently black-holed packets?

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Sat, 5 Aug 2017 02:41:26 +0000

Hi, Jürgen,

Many thanks for the review!

Please see inline.

> On Aug 4, 2017, at 10:44 AM, Jürgen Schönwälder <[email-masked]> wrote:
> 
> good shape.

Thanks!

> I understand that the WG will work on management

I think it is safe to use “service plane” here since it is used in the Normative RFC 7665.
However, I agree that “service plane protocol” too early might be confusing.

>    creation of dynamic service chains and is composed of the

This is a good point. However, here’s another suggestion:

"The Network Service Header (NSH) specification defines a new protocol for the creation  […], operating at the service plane. NSH is composed […]"

>  as a first time reader I think it is preferable to avoid this term

Final text:

   The Network Service Header (NSH) specification defines a new protocol
   for the creation of dynamic service chains, operating at the service
   plane.  NSH is composed of the following elements:

>  have multiple meanings. Perhaps simply remove '(i.e., transport)’?

Agreed. Instead of removing it, I’ll replace with “encapsulation"

>  to   not have to guess…

Good guess :-) We will disambiguate as follows:

   NSH-aware:  NSH-aware means SFC-encapsulation-aware.  This
      specification uses NSH-aware as a more specific term from the more
      generic term SFC-aware [RFC7665].

>  places, perhaps it deserves a definition in the terminology section.

The term comes from RFC 7665.

If you believe it will help, we can add the definition:

        <t hangText="Service Plane:">
                    The collection of SFFs and associated SFs creates a service-plane
   overlay in which all SFs reside <xref target="RFC7665"></xref>.
        </t>

>  What exactly is a 'network transport protocol’?

Means “encapsulation” Change:

OLD:

       NSH is transport-independent. An appropriate
       (for a given deployment) network transport protocol can be used
       to transport NSH-encapsulated traffic.  

OLD:

       NSH is encapsulation-independent, meaning it can be transported
       by a variety of protocols. An appropriate
       (for a given deployment) encapsulation protocol can be used
       to carry NSH-encapsulated traffic.  

>    All fields marked U are unassigned […]

Good point. Combo:

NEW:

   All other flag fields, marked U, are unassigned [...]

>  s/order their appear/order they appear/

Done.

>  s/must process first instance/must process the first instance/

Ack. Done.

>  s/un-encapsulated packet/un-encapsulated packet./

OK.

>  s/selection a different/selection of a different/

Ack. Done.

>  have been to section 9?

Good point. Should indeed be Section 9.

> But even then, does the referenced text

I do not believe that inlining text will help as it creates a source of desynchronization.

Instead, I added:

OLD:

NEW:

<t>
For example, when NSH is encapsulated in IP, IP-level fragmentation coupled with 
Path MTU Discovery (PMTUD) is used. 
When, on the other hand, the underlay does not support fragmentation procedures,
an error message SHOULD be logged when dropping a packet too big.
Lastly, NSH-specific fragmentation and reassembly methods
may be defined as well, but
these methods are outside the scope of this document.
</t>

Thoughts?

>  these then silently black-holed packets?

See above. 

Concrete Text improvement suggestions most welcome!

Thanks!

—
Carlos Pignataro, [email-masked]

“Sometimes I use big words that I do not fully understand, to make myself sound more photosynthesis."

---

From: Juergen Schoenwaelder <[email-masked]>
Date: Sun, 6 Aug 2017 09:24:22 +0200

>    plane.  NSH is composed of the following elements:

OK
 
> Agreed. Instead of removing it, I’ll replace with “encapsulation"

OK 

>       generic term SFC-aware [RFC7665].

Should it be "NSH-aware means SFC-encapsulation-aware using the
NSH protocol."?
 
>    overlay in which all SFs reside <xref target="RFC7665"></xref>.

I think it is useful to have an easy to find clear definition of this
term. It should probably have been defined as a term in RFC 7665...

>        to carry NSH-encapsulated traffic.  

OK 
 
>    All other flag fields, marked U, are unassigned [...]

OK 
 
> these methods are outside the scope of this document.

Works for me. 
 
> Concrete Text improvement suggestions most welcome!

I think the above proposed text is what I was looking for.

Thanks for your quick response.

/js

-- 
Juergen Schoenwaelder           Jacobs University Bremen gGmbH
Phone: +49 421 200 3587         Campus Ring 1 | 28759 Bremen | Germany
Fax:   +49 421 200 3103         <http://www.jacobs-university.de/>

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Sun, 6 Aug 2017 17:24:08 +0000

Thanks, Jürgen. I believe we have converged on all items. There's one more follow up inline for completeness. 

Thank you for your review, our working copy incorporated these changes and is ready for when Joel/Alia want us to hit Submit. 

Sent from my iPad

> On Aug 6, 2017, at 3:24 AM, Juergen Schoenwaelder <[email-masked]> wrote:
> 

Yes, and I can add that. It is quite implicit given the last sentence of the Abstract. 

Thanks!

C. 

> _______________________________________________
> sfc mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/sfc