IETF conflict review for draft-browne-sfc-nsh-kpi-stamp
conflict-review-browne-sfc-nsh-kpi-stamp-00

IESG,


here are some comments I'm about to send to the Authors/ISE.
sorry they come after the conflict review itself, but they don't change my proposed position.

---
Authors,

please find some comments that I have on your document as part of the conflict review.

Overall, I am under the impressions that, while bits on the wire are well specified, there are missing parts/ambiguities which would make the life of an implementer very hard. I understand this is the independent submission track and this is Informational but if you publish this document I guess this is with the hope that others can take it up and implement it.

Abstract
"Experimental" carries a special meaning in publication streams. So I'd like the use of this term to be avoided in the doc because you target an Informational status.

Terminology
LSN is a forwarding element. I believe this needs to be detailed more than what is found in the terminology and do so elsewhere than in a terminology section.

The SC instructs the classifier on how to build the NSH. I believe this again is worth a longer description. I guess you mean the kpi specific part of the NSH metadata. Otherwise I feel that would conflict with the architecture which does not require the existence of an SC.

The existence of the two very similar acronyms SFN and FSN is a bit unfortunate.

NSH KPI Stamping: An Overview
   The Stamping Controller (SC) will most probably be part of the SFC
   controller
What is an SFC controller?

   The SC is responsible for initiating start/stop stamp requests to the
   SCL or First Stamp Node (FSN), and also for distributing NSH stamping
   policy into the service chain via the Stamping Control Plane (SCP)
   interface.
I can't find any specification of that interface, is that on purpose?

   The FSN is responsible for marking NSH MD fields which tells upstream
If we are talking about flow direction, wouldn't it be downstream instead?

   The Last Stamp Node (LSN) should strip the entire NSH header and
   forward the raw packet to the IP next hop as per [RFC8300]. 
Maybe I misunderstood but this seem not fully aligned with what is described in the Terminology section.

   By synchronizing the network in this way, the timestamping operation
   is independent of the current Rendered Service Path (RSP). Indeed the
   timestamp metadata can indicate where a chain has been moved due to a
   resource starvation event as indicated in Figure 2, between VNF 3 and
   VNF 4 at time B.
Could it be that, because clocks are not synchronized accurately that a time stamp of a 'N+1' VNF is in the past of a 'N' VNF?

   KPI-stamping detection mode uses MD type 2 defined in [RFC8300]. This
   involves the SFC classifier stamping the flow at chain ingress, and
   no subsequent stamps being applied, rather each SF upstream can
   compare its local condition with the ingress value and take
   appropriate action. Therefore detection mode is very efficient in
   terms of header size that does not grow after the classification.
   This is further explained in Section 4.1.
This left me doubtful because, unless I'm wrong, I did not see the detection/extended modes being introduced or described before. Why mention detection mode here, and why only mention detection mode.

4.1. KPI-stamping Extended Encapsulation
references to figures' numbers are wrong, in the text.
s/6/5/
s/8/7/

   The Reference Time is represented in 64-bit NTP format [RFC5905]
But I read in 5905:
   A timestamp T(t) represents either the UTC date or time offset from
   UTC at running time t.  Which meaning is intended should be clear
   from the context. 
Does that imply that you should specify what the timestamp represents?

   The Ingress Timestamp and Egress Timestamp are represented in 64-bit
   NTP format. The corresponding bits (I and E) reported in the Stamping
   Reporting Header of the node's metadata.
   Timestamps are represented in 64-bit NTP [RFC5905] format, which is
   one of the recommended formats of [TS].
Duplicate text

Why has 'U':
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |         Metadata Class        |     Type      |U|    Length   |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
became 'R' (for TS2 and TS3)?
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |          Metadata Class       |  Type=TS(2)   |R|     Len     |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |I|E|T|U|U|U|SSI|  Stamping SI  |           Flow ID             |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-|
I guess U means Unassigned/Unused. It might be worth clarifying this.


4.1.2. NSH QoS-stamping Encapsulation (Extended Mode)
How do you deal with MPLS stacks deeper than 2?

Also, what is the 'C' bit here?
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
    |         Metadata Class        |C| Type=QoS(3) |R|     Len     |
    +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+


Typos:
s/among an order set of Service/among an ordered set of Service/
s/5-tuple coordiante/5-tuple coordinate/
s/Primary Reference Clock (PRC)/Primary Reference Time Clock (PRTC)/
s/whist/whilst/

I note this text in the Introduction: 

   This
   document differs from to [I-D.ippm.ioam] in the sense that it is
   specifically tied to NSH operation and not generic in nature.

I don't think this document *conflicts* with IPPM (which Mirja is now responsible AD for), and in fact think it's useful to get experience with protocol mechanisms like this one while the IETF is working on iOAM, but I do think that naming IPPM in the conflict review as related work that does not conflict would be correct.