Re: [Detnet] AD review of draft-ietf-detnet-mpls-oam-12

[Greg Mirsky <[e-mail]>]

Hi John,
many thanks for your detailed review and thought-inspiring comments. We,
the authors, discussed your questions and suggestions. You can find our
notes in the attached copy of -12 version following your notes under the
GIM>> tag. Also, please find the working version of the draft with all the
updates and the diff highlighting them. Please let us know if you have any
further questions and whether you find your concerns sufficiently addressed.

Regards,
Greg

On Wed, May 31, 2023 at 3:10 AM John Scudder <[e-mail]> wrote:

> Hi Authors, WG,
>
> Thanks for this document. I struggled with it more than most of my
> reviews, quite possibly because I am still quite inexpert at DetNet. I look
> forward to working with you to educate myself or improve the document,
> whichever is needed (or both).
>
> I’ve supplied my questions and comments in the form of an edited copy of
> the draft. Minor editorial suggestions I’ve made in place without further
> comment, more substantive questions and comments are done in-line and
> prefixed with “jgs:”. You can use your favorite diff tool to review them;
> I’ve attached the iddiff output for your convenience if you’d like to use
> it. I’ve also pasted a traditional diff below in case you want to use it
> for in-line reply.
>
> Thanks,
>
> —John
>
> --- draft-ietf-detnet-mpls-oam-12.txt   2023-05-30 17:56:11.000000000 -0400
> +++ draft-ietf-detnet-mpls-oam-12-jgs-comments.txt      2023-05-30
> 21:03:16.000000000 -0400
> @@ -17,7 +17,7 @@
>
>  Abstract
>
> -   This document defines format and use principles of the Deterministic
> +   This document defines the format and usage principles of the
> Deterministic
>     Network (DetNet) service Associated Channel (ACH) over a DetNet
>     network with the MPLS data plane.  The DetNet service ACH can be used
>     to carry test packets of active Operations, Administration, and
> @@ -100,7 +100,7 @@
>     can be achieved through a combination of active and hybrid, as
>     defined in [RFC7799], OAM methods.
>
> -   Also, this document defines format and use principles of the DetNet
> +   This document defines the format and usage principles of the DetNet
>     service Associated Channel over a DetNet network with the MPLS data
>     plane [RFC8964].
>
> @@ -116,17 +116,24 @@
>
>  2.1.  Terminology and Acronyms
>
> -   The term "DetNet OAM" used in this document interchangeably with
> +   The term "DetNet OAM" is used in this document interchangeably with
>     longer version "set of OAM protocols, methods and tools for
>     Deterministic Networks".
>
>     CW Control Word
> +--
> +jgs: you use this acronym once (twice if you count "d-CW"). Consider just
> +spelling it out where used? But OK to keep it this way if you prefer.
> +--
>
>     DetNet Deterministic Network
>
>     d-ACH DetNet Associated Channel Header
>
>     d-CW DetNet Control Word
> +--
> +jgs: as above.
> +--
>
>     GAL Generic Associated Channel Label
>
> @@ -147,17 +154,23 @@
>     LSR Label Switching Router
>
>     F-Label A Detnet "forwarding" label.  The F-Label identifies the LSP
> -   used to forward a DetNet flow across an MPLS PSN, e.g., a hop-by-hop
> +   used to forward a DetNet flow across an MPLS packet-switched network,
> e.g., a hop-by-hop
>     label used between label switching routers (LSR).
> +--
> +jgs: is that really "e.g." (for example) and not "i.e." (in other words)?
> +--
>
>     S-Label A DetNet "service" label.  An S-Label is used between DetNet
> -   nodes that implement also the DetNet service sub-layer functions.  An
> +   nodes that also implement the DetNet service sub-layer functions.  An
>     S-Label is also used to identify a DetNet flow at DetNet service sub-
>     layer.
> +--
> +jgs: I'm not sure you need the first "also" (the one I moved) at all.
> +--
>
>     Underlay Network or Underlay Layer: The network that provides
>     connectivity between the DetNet nodes.  One example of an underlay
> -   layer is an MPLS network that provides LSP connectivity between
> +   layer is an MPLS network that provides Label Switched Path (LSP)
> connectivity between
>     DetNet nodes.
>
>
> @@ -173,6 +186,12 @@
>     DetNet Node - a node that is an actor in the DetNet domain.  Examples
>     of DetNet nodes include DetNet domain Edge nodes, and DetNet nodes
>     that perform PREOF within the DetNet domain.
> +--
> +jgs: almost all the definitions need a separator after the defined term.
> +The first one doesn't, and the last two have them already -- although you
> +use a colon in one case and a hyphen in the other, please pick one or the
> +other.
> +--
>
>  2.2.  Keywords
>
> @@ -193,6 +212,10 @@
>        DetNet OAM packets MUST be in-band, i.e., follow precisely the
>        same path as DetNet data plane traffic both for unidirectional and
>        bi-directional DetNet paths.
> +--
> +jgs: Please update to cite the correct requirement # and requirement
> +text.
> +--
>
>     Operation of a DetNet data plane with an MPLS underlay network is
>     specified in [RFC8964].  Within the MPLS underlay network, DetNet
> @@ -234,15 +257,24 @@
>                            333333333333333333333333
>
>                    Figure 2: DetNet Data Plane Based on PW
> +--
> +jgs: Without labeling and explanation, this diagram is probably not going
> +to be very helpful to the people who need it -- so either eliminate the
> +diagram, or explain it.
> +--
>
>  3.1.  DetNet Active OAM Encapsulation
>
> -   DetNet OAM, like PW OAM, uses PW Associated Channel Header defined in
> +   DetNet OAM, like PW OAM, uses the PW Associated Channel Header defined
> in
>     [RFC4385].  At the same time, a DetNet PW can be viewed as a Multi-
> +--
> +jgs: would it be OK to eliminate the words "At the same time" and just
> start
> +the sentence as "A DetNet PW can be..."? If so I would suggest doing that.
> +--
>     Segment PW, where DetNet service sub-layer functions are at the
>     segment endpoints.  However, DetNet service sub-layer functions
> -   operate per packet level (not per segment level).  These per-packet
> -   level characteristics of PREOF require additional fields for proper
> +   operate per packet (not per segment).  These per-packet
> +   characteristics of PREOF require additional fields for proper
>     OAM packet processing.  Encapsulation of a DetNet MPLS [RFC8964]
>     active OAM packet is shown in Figure 3.
>
> @@ -324,12 +356,45 @@
>        Bits 0..3 MUST be 0b0001.  This value of the first nibble
>        distinguishes an IP packet [RFC4928] from a DetNet data packet
>        [RFC8964].
> +--
> +jgs: Shouldn't this say, "This allows the packet to be distinguished
> +from an IP packet [RFC4928] and from a DetNet data packet [RFC8964]."
> +That's just my copy-and-paste from RFC 4385 with s/PW/DetNet. The key
> +point is that you aren't trying to distinguish between IP and DetNet
> +data as your text says, you're trying to distinguish DetNet OAM from
> +*either* IP *or* DetNet data, right? In which case, sticking with the
> +RFC 4385 wording seems indicated.
> +--
>
>        Version - is a 4-bit field, and the value is the version number of
>        the d-ACH.  Version field is needed if the update to d-ACH can not
>        be introduced in a backward-compatible way.  This specification
>        defines version 0x1 to further differentiate d-ACH from PW ACH
>        defined in [RFC4385].
> +--
> +jgs: I have two concerns here.
> +
> +First, I don't think you need the sentence about what a version field is
> +needed for. It's a general statement about how protocols use version
> +fields, it clutters the spec and IMO is not required.
> +
> +Second and of greater concern, your selection of version 0x1 "to further
> +differentiate d-ACH from PW ACH". Are the version number spaces between
> +d-ACH and PW ACH disjoint, or are they the same number space? If they're
> +disjoint, there should be no need to "further differentiate" them, and
> +you should probably pick version 0. If they aren't disjoint, then you
> +should say more about that, and this spec would need to update RFC 4385
> +to indicate you're allocating a new value in the ACH version field. In
> +that case establishment of an IANA registry for the field would also be
> +indicated. And come to think of it, if the spaces are not disjoint, it's
> +not a version field any more, it's a type field, so you'd need to make
> +that change too.
> +
> +My guess is the spaces are disjoint and you should proceed
> +accordingly, as in
> +
> +   Version - a 4-bit field. This document specifies version 0.
> +--
>
>
>
> @@ -350,21 +415,41 @@
>        the IANA MPLS Generalized Associated Channel (G-ACh) Types
>        (including Pseudowire Associated Channel Types) registry
>        [IANA-G-ACh-Types].  New values can be defined in the future.
> +--
> +jgs: Probably you mean it MUST be one of the values from the G-ACh
> +registry? As written you've left the door open for it to be anything
> +else, too (for example my birthday encoded in BCD).
> +
> +The "New values can be defined in the future" sentence can be
> +deleted IMO. As with the "here is what version fields are for" above,
> +this is just a general statement about registries.
> +--
>
>        Node ID - is an unsigned 20-bit field.  The value of the Node ID
>        field identifies the DetNet node that originated the packet.
>        Methods of distributing Node ID are outside the scope of this
>        specification.
> +--
> +jgs: What spec *are* they in scope of? It seems a little weird to just
> +drop this DetNet Node ID concept in here without saying anything further
> +about it. It doesn't seem to be a concept that exists in the DetNet
> +Architecture document, for example.
> +--
>
> -      Level - is a 3-bit field.  Level field is used to cope with the
> +      Level - is a 3-bit field.  The Level field is used to cope with the
>        "all active path forwarding" characteristics of the PREOF concept.
>        A hierarchical relationship between OAM domains can be created
>        using the Level field value.
> +--
> +jgs: I'm excited about the hierarchical relationship that can be created
> +using this field, but sad that the spec doesn't tell me how to do it.
> +That seems... underspecified.
> +--
>
> -      Flags - is a 5-bit field.  Flags field contains five 1-bit flags.
> +      Flags - is a 5-bit field.  The Flags field contains five 1-bit
> flags.
>        Section 5.1 creates the IANA DetNet Associated Channel Header
>        Flags registry for new flags to be defined.  The flags defined in
> -      this specification presented in Figure 6.
> +      this specification are presented in Figure 6.
>
>
>               0 1 2 3 4
> @@ -377,12 +462,12 @@
>     U: Unused and for future use.  MUST be 0 on transmission and ignored
>     on receipt.
>
> -      Session ID is a 4-bits field.  Session field is used to
> +      Session ID is a 4-bit field.  The Session field is used to
>        distinguish OAM sessions originated from the same node (a given
>        Maintenance End Point may have multiple simultaneously active OAM
>        sessions).
>
> -   The DetNet flow, according to [RFC8964], is identified by the S-label
> +   A DetNet flow, according to [RFC8964], is identified by the S-label
>     that MUST be at the bottom of the stack.  An Active OAM packet MUST
>     include d-ACH immediately following the S-label.
>
> @@ -398,7 +483,7 @@
>        Interaction with Active OAM
>
>     At the DetNet service sub-layer, special functions (notably PREOF)
> -   MAY be applied to the particular DetNet flow to potentially lower
> +   MAY be applied to the particular DetNet flow to potentially reduce
>     packet loss, improve the probability of on-time packet delivery, and
>     ensure in-order packet delivery.  PREOF relies on sequencing
>     information in the DetNet service sub-layer.  For a DetNet active OAM
> @@ -406,6 +491,21 @@
>     inclusive of the first 32-bit word of the d-ACH, i.e., the
>     concatenation of Version, Sequence Number, and Channel Type fields,
>     as the source of this sequencing information.
> +--
> +jgs: It's unclear to me in what spec PREOF operation is defined :-(
> +for example, RFC 8964 tells me that "Implementation details of PREOF
> +are out of scope for this document". But in any case, it's surprising
> +to me that PREOF operation is being (sort of) defined here, and that
> +it's just being defined for OAM packets, which I thought were supposed
> +to share fate with the associated DetNet data packets -- so I would
> +have thought they'd need to experience identical PREOF treatment, not
> +define their own.
> +
> +Also, is there any special reason you go to the trouble to exclude bits
> +0 through 3 from being used for PREOF sequencing? 0 through 3 are fixed
> +as 0b0001 anyway, so I would have thought there'd be no harm in using
> +bits 0..31, which ought to be slightly less hassle for the implementor.
> +--
>
>  4.  OAM Interworking Models
>
> @@ -418,7 +518,7 @@
>     sent to a central controller.  In the tunneling model of OAM
>     interworking, usually, only one active OAM protocol is used.  Its
>     test packets are tunneled through another domain along with the data
> -   flow, thus ensuring the fate sharing among test and data packets.
> +   flow, thus ensuring fate sharing among test and data packets.
>
>  4.1.  OAM of DetNet MPLS Interworking with OAM of TSN
>
> @@ -429,6 +529,13 @@
>     [RFC9037].
>
>     When the peering model is used in CFM OAM, then the node that borders
> +--
> +jgs: This paragraph leads with a clause I don't understand, "When the
> +peering model is used in CFM OAM". Maybe this would make sense if you
> +had introduced CFM at all, but even if I peek down to learn that it
> +means "Connectivity Fault Management" I don't see what you're doing
> +here.
> +--
>     both TSN and DetNet MPLS domains MUST support [RFC7023].  [RFC7023]
>     specifies the mapping of defect states between Ethernet Attachment
>     Circuits and associated Ethernet PWs that are part of an E2E emulated
> @@ -453,10 +560,21 @@
>     MPLS and [ITU.Y1731] in the TSN domains, respectively.  Performance
>     objectives for each domain should refer to metrics that additive or
>     be defined for each domain separately.
> +--
> +jgs: The rest of the paragraph is difficult for me as well, to the
> +extent that I'm afraid I can't identify point problems to fix. I do have
> +some guesses, but not with sufficient confidence to suggest text. I have
> +a feeling that you're trying to do too much with too few words here and
> +that it would be helpful to spend a paragraph or two more fully
> +introducing the problem space, maybe even a diagram to refer that
> +illustrates the TSN and DetNet MPLS domains peering. But perhaps I should
> +have a call with one of the authors who can help me understand what
> +you're trying to accomplish.
> +--
>
>     The following considerations apply when using the tunneling model of
>     OAM interworking between DetNet MPLS and TSN domains based on general
> -   principles described in Section 4 [RFC9037]:
> +   principles described in Section 4 of [RFC9037]:
>
>     *  Active OAM test packets MUST be mapped to the same TSN Stream ID
>        as the monitored DetNet flow.
> @@ -464,6 +582,14 @@
>     *  Active OAM test packets MUST be treated in the TSN domain based on
>        its S-label and Class of Service marking (the Traffic Class field
>        value).
> +--
> +jgs: As I read it, this text requires IEEE 802.1 TSN devices to inspect
> +the S-Label, which doesn't make sense. The Traffic Class field makes sense
> +as far as it goes, but also runs aground when we consider that this looks
> +like an IETF document trying to mandate something of an IEEE layer.
> +I tried referring to Section 4 of RFC 9037, but that didn't enlighten
> +me. Can you help me understand what you're trying to do here?
> +--
>
>     Note that the tunneling model of the OAM interworking requires that
>     the remote peer of the E2E OAM domain supports the active OAM
>
>
>