Subject: [sfc] Review of draft-ietf-sc-nsh-14 (sic)

From: "Adrian Farrel" <[email-masked]>
Date: Thu, 20 Jul 2017 16:58:22 +0100

Hi,

Thanks for the recent set of revisions. I started reviewing -14 to
look for completion of my comments that I made on -12. I stuck with
that when -15 and -16 came out because otherwise I'd have to go
back and re-review.

You seem to have caught most points I raised, but not acted on a
few. I have discarded editorial nits and trivial points that you 
didn't include - they are not important. But there are a few missed
points listed below for attention. Sorry if I missed the discussion
on the list about any of these points: I went back to the archive, 
but didn't find anything.

As I went through I also picked up a few new (but small) points I have
included at the end of this email.

Thanks again for the focus and energy to complete this work.

Adrian

========

Why do you point to 7665 for the definition of "Classifier" but then
define "Service Classifier" to mean (AFAICS) exactly the same thing?

---

Wondering whether "Context Headers" should be "Context Header"
throughout.

---

OLD
3.2

   Version: The version field is used to ensure backward compatibility
   going forward with future NSH updates.  It MUST be set to 0x0 by the
   sender, in this first revision of NSH.  Given the widespread
   implementation of existing hardware that uses the first nibble after
   an MPLS label stack for ECMP decision processing, this document
   reserves version 01 and this value MUST NOT be used in future
   versions of the protocol.  Please see [RFC7325] for further
   discussion of MPLS-related forwarding requirements.
YOURS
2.2
   Version: The version field is used to ensure backward compatibility
   going forward with future NSH specification updates.  It MUST be set
   to 0x0 by the sender, in this first revision of NSH.  Given the
   widespread implementation of existing hardware that uses the first
   nibble after an MPLS label stack for ECMP decision processing, this
   document reserves version 01 and this value MUST NOT be used in
   future versions of the protocol.  Please see [RFC7325] for further
   discussion of MPLS-related forwarding requirements.

I believe this text is presuming the NSH-in-MPLS encapsulation. I don't
think it should do that, but if it does we have to look at the first
nibble not just the first two bits.

Now, as 4385 points out, if the first nibble is 4 or 6, IPv4 or IPv6
will be assumed. So 0b0100 or 0b0110 accounts for you reserving version
1 (just in case the C bit is 0 and for any value of the O bit). So far,
so good!

But 4385 defines a first nibble of 0000 to mean "here is a control word"
and that would mean you needed to reserve version 0 as well :-(

So my advice here would be that encapsulation in MPLS is not in scope
for this document, and you should not play with reserving version
numbers.

---

2.3

   In addition to indicating
   the location within a Service Function Path, SI can be used for
   service plane loop detection.

I think that, although this might still be true, the TTL will be more
helpful for loop detection and we should strike this text.

---

3.

   NSH-aware nodes are the only nodes that may alter the content of the
   NSH headers.

Is there a difference between an "SFC-aware node" and an "NSH-aware
node"? If not, you should retain the original terminology. If so, the
difference needs to be explained.

Isn't this sentence silly? "Only nodes that can understand the content
of the NSH can change the content of the NSH"

---

3.
Bullet 1

       Multiple logical classifiers may exist within a given service
       path.

There are several mentions of a "logical classifier" without discussion
or explanation. I looked in 7665 and did not find a description either.
I note that the "service classifier" in section 1.2 is described as 
being "logical" and that is also really unclear not just because of
the previous note on "service classifier" and "classifier" but because 
but because the term "logical" is possibly meant to men something.

Why can't you just say "classifier"?

---

In Figure 7

- Might an SFC proxy also select service policy for an NSH-unaware SF?

---

4.

   The presence of NSH is
   indicated via protocol type or other indicator in the outer
   encapsulation.

You are prejudging the "encapsulation of NSH" work that is to be done.
You may be largely correct, but not always. We should omit this text.

=============

Additional points on reading now...

---

In 2.2 I see

   0xF - this value is reserved for experimentation and testing, as per
   [RFC3692].  Implementations not explicitly configured to be part of
   an experiment SHOULD silently discard packets with MD Type 0x15.

<smile>

s/x15/xF/

But isn't this controversial? Should an SFF discard a packet with 
experimental metadata? 

Why can't an SF pass experimental metadata on to be used by a remote SF?
That would allow the experiment to be more flexible and more widely 
applied.

Finally, I wonder whether we shouldn't describe the processing when 
handling an unknown MD type. (Just like the unknown next protocol.)

---

Additionally, I don't see why 11.2.1 is a registry of "reserved bit" 
with each marked as Reserved (presumably in a 8126 sense).  Aren't these
actually Unassigned, and shouldn't a registry of bits include the O bit?

===========

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Fri, 21 Jul 2017 00:37:05 +0000

Hi, Adrian,

An interesting idea just occurred to me! We could start working on a set of Performance Metrics for Internet-Draft Reviews — we could define metrics (e.g., time to initial review, mean time between re-reviews), units (e.g., hours, days, months, IETF meetings), and specify mechanics for measurement (distribution of reviews after deadline) :-)

I’ll be happy to contribute (or better to review! :-), we could shoot for publishing on April 1st, not specifying the year of course :-)

Slightly more seriously, a few of these are already fixed in the currently posted revision.

Fully seriously now:

Thank you very much again Adrian for the newly discovered nits, editorials, and questions, and apologies on my behalf, editors, and WG for having missed some of the earlier comments. For real, thanks for the time and insight.

I appreciate these and we should (and will) address them.

*** However… ***

Hi, Chairs,

I very strongly recommend you ship the current revision to the AD. Nothing below is a show-stopper. We can continue integrating these in successive approximations towards perfection in /main, while publication is requested and the AD takes her/his time.

We have to draw the line somewhere, so, here it goes, the line before the comments below:

______________________________________

Thanks!

— Carlos.

On Jul 20, 2017, at 11:58 AM, Adrian Farrel <[email-masked]<mailto:[email-masked]>> wrote:

Hi,

Adrian

========

---

---

OLD
3.2

---

2.3

---

3.

---

3.
Bullet 1

---

In Figure 7

---

4.

  The presence of NSH is

=============

---

In 2.2 I see

<smile>

s/x15/xF/

---

===========

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

—
Carlos Pignataro, [email-masked]<mailto:[email-masked]>

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

---

From: "Adrian Farrel" <[email-masked]>
Date: Fri, 21 Jul 2017 06:23:46 +0100

I see no reasons for the April 1st RFC you mention: we already have RFC 7282.

From: Carlos Pignataro (cpignata) [mailto:[email-masked]] 
Sent: 21 July 2017 01:37
To: Adrian Farrel
Cc: Service Function Chaining IETF list
Subject: Re: [sfc] Review of draft-ietf-sc-nsh-14 (sic)
 
Hi, Adrian, 

 
 
Fully seriously now:

 
*** However… ***
 
Hi, Chairs,

 
 
Thanks!
 
— Carlos.
 
On Jul 20, 2017, at 11:58 AM, Adrian Farrel <[email-masked]> wrote:
 
Hi,

Adrian

========

---

---

OLD
3.2

---

2.3

---

3.

---

3.
Bullet 1

---

In Figure 7

---

4.

  The presence of NSH is

=============

---

In 2.2 I see

<smile>

s/x15/xF/

---

===========

[email-masked]
 
—
Carlos Pignataro, [email-masked]

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Wed, 26 Jul 2017 06:26:54 +0000

Hi Adrian,

Thank you for checking in and reviewing that these earlier comments were addressed.

Looking at the list archives, your initial email was responded to, with no additional follow-up. See:
https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e

Please see inline for additional responses.

> On Jul 20, 2017, at 8:58 AM, Adrian Farrel <[email-masked]> wrote:
> 
> Hi,
> 

See https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e.

> included at the end of this email.

Thanks! See below, as those are already addressed in a subsequently posted revision.

> Thanks again for the focus and energy to complete this work.

Thank you, Adrian, for the detailed review improving the document.

> define "Service Classifier" to mean (AFAICS) exactly the same thing?

The previous response includes:
PQ>  The definition in NSH provides important NSH-centric details that aren't in 7665.  

I’ll add that the RFC 7665 definition is fairly minimalistic, so this addition makes sense.

> Wondering whether "Context Headers" should be "Context Header"

Looking at the document and various instances of “context headers”, it is really referring to elements of context (i.e., the field contains multiple context headers). As such, it makes sense to me to leave it as-is.

However, if there’s a specific editorial proposal that would make things more clear, happy to edit it in. I believe this is an editorial, though.

> for this document, and you should not play with reserving version

The initial response on the list to this item was:
PQ>  That is where we started, but there was a lot of discussion on the list about the potential for conflict.  I have to go dig up the threads but I'm hoping that some of the folks who expressed concern here will weigh in.

And there has been no follow-up.

The previous discussion on the list regarding this topic made it clear that the intent was not not alias with multipath, hence the reference to 7325 and not to 4385.

The challenge is: we need to define the values in the base NSH spec or otherwise we cannot future proof it. This is without defining or taking position on an MPLS encapsulation.

> helpful for loop detection and we should strike this text.

Interestingly, this is the only presence of the word “loop” in the document. That text is neither incorrect nor harmful (SI can still be used for it). That said:

Section 2.2., paragraph 9:
EXPLANATION: add sentence about loops for TTL definition.

OLD:

|   TTL: Indicates the maximum SFF hops for an SFP.  The initial TTL
|   value SHOULD be configurable via the control plane; the configured
|   initial value can be specific to one or more SFPs.  If no initial
|   value is explicitly provided, the default initial TTL value 63 MUST
|   be used.  Each SFF involved in forwarding an NSH packet MUST
|   decrement the TTL value by 1 prior to NSH forwarding lookup.
|   Decrementing by 1 from an incoming value of 0 shall result in a TTL
|   value of 63.  The packet MUST NOT be forwarded if TTL is, after
|   decrement, 0.

NEW:

|   TTL: Indicates the maximum SFF hops for an SFP.  This field is used
|   for service plane loop detection.  The initial TTL value SHOULD be
|   configurable via the control plane; the configured initial value can
|   be specific to one or more SFPs.  If no initial value is explicitly
|   provided, the default initial TTL value 63 MUST be used.  Each SFF
|   involved in forwarding an NSH packet MUST decrement the TTL value by
|   1 prior to NSH forwarding lookup.  Decrementing by 1 from an incoming
|   value of 0 shall result in a TTL value of 63.  The packet MUST NOT be
|   forwarded if TTL is, after decrement, 0.

------------------------------------------------------------------------

Section 2.3., paragraph 8:
EXPLANATION: point to TTL as the main loop detection mechanism,.

OLD:

    The SI is used in conjunction with Service Path Identifier for
    Service Function Path Selection and for determining the next SFF/SF
    in the path.  The SI is also valuable when troubleshooting/ reporting
|   service paths.  In addition to indicating the location within a
|   Service Function Path, SI can be used for service plane loop
|   detection.

NEW:

    The SI is used in conjunction with Service Path Identifier for
    Service Function Path Selection and for determining the next SFF/SF
    in the path.  The SI is also valuable when troubleshooting/ reporting
|   service paths.  Additionally, while the TTL field is the main
|   mechanism for service plane loop detection, the SI can also be used
|   for detecting service plane loops.

> of the NSH can change the content of the NSH”

Your previous comment was s/MAY/may/, which was acted upon.

Good point about NSH-aware vs. SFC-aware. Added:
         <t hangText="NSH-aware">
                    SFC-encapsulation-aware, or SFC-aware <xref target="RFC7665"></xref>.
        </t>

Since NSH is the SFC Encapsulation, and SFC-aware means SFC encapsulation aware.

> Why can't you just say "classifier”?

This was answered at https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e

> - Might an SFC proxy also select service policy for an NSH-unaware SF?

This was answered at https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e

> You may be largely correct, but not always. We should omit this text.

This was answered at https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e

:-) Thanks, already fixed.

> That would allow the experiment to be more flexible and more widely 

In that case, those SFs would be part of the experiment :-)

> handling an unknown MD type. (Just like the unknown next protocol.)

Already addressed in a revision after your review.

> actually Unassigned, and shouldn't a registry of bits include the O bit?

Already addressed in a revision after your review.

I will submit a new revision momentarily with the edits as-per above.

Thanks!

— Carlos.

—

---

From: "Carlos Pignataro (cpignata)" <[email-masked]>
Date: Wed, 26 Jul 2017 06:41:45 +0000

Hi, Adrian,

In regards to your review comments based on -12, they were addressed item-by-item on a response posted on 20 March 2017, at:
https://mailarchive.ietf.org/arch/msg/sfc/ByFQ2ICtAPtVr_HYDQ837xAD1wo/?qid=87622f6f2cb03d8946dce8c80f43ee6e
The list does not show follow-up from you. The only pending item that I could identify was your comment regarding TTL, which was deferred (“PQ>  Once we finalize the TTL details, I agree this'll be cleaned up.”)
Revision -17, which posted moments ago, addresses this.

In regards to RFC 7282, that’s for the chairs to respond.
I will note that the list shows the chairs’ view of whether commends had been addressed and their respective consensus calls and timeline goals:
https://mailarchive.ietf.org/arch/msg/sfc/XL2JT1xXoh4HfXr-4xP_611C2kA
https://mailarchive.ietf.org/arch/msg/sfc/oPp8iWRBUb3Z111i3ahaVqp9Mx0
https://mailarchive.ietf.org/arch/msg/sfc/XoGxeX2Wfl-23eZTeE1EgKeyE6I

Thanks!

— Carlos.

On Jul 20, 2017, at 10:23 PM, Adrian Farrel <[email-masked]<mailto:[email-masked]>> wrote:

Hi, Adrian,

Fully seriously now:

*** However… ***

Hi, Chairs,

Thanks!

— Carlos.

Hi,

Adrian

========

---

---

OLD
3.2

---

2.3

---

3.

---

3.
Bullet 1

---

In Figure 7

---

4.

  The presence of NSH is

=============

---

In 2.2 I see

<smile>

s/x15/xF/

---

===========

—

—