[Detnet] AD review of draft-ietf-detnet-yang-17

[John Scudder <[e-mail]>]

Hi Authors, WG,

I apologize for the length of time this document has sat in AD Review! I found it a relatively easy review until I got to the examples, where I bogged down. I’m sending you my review without finishing past example B.5, in hopes that the issues identified up to that point help you do any necessary rework on the later examples as well — the issues identified do generally fall into a pattern that on brief review I do see repeating later.

I appreciate the inclusion of the detailed examples, they’re potentially extremely helpful to the non-expert reader (such as myself). I’m hoping that we can make the additional (I think relatively small) effort needed to make them truly usable by the audience. What I’ve identified so far largely involves providing better labeling of the diagrams, and prose introduction and separation of the various configurations. More detail is in the attached review.

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-yang-17.txt	2023-04-17 14:49:20.000000000 -0400
+++ draft-ietf-detnet-yang-17-jgs-comments.txt	2023-04-18 16:18:21.000000000 -0400
@@ -217,7 +217,14 @@
 
    At the egress the YANG model is handing off IP or MPLS to a next hop
    and a routing next hop structure is used.
-
+  
+--- 
+jgs: I found it a little difficult to understand what you meant by
+the above sentence. In the end, I guess what you're saying is that 
+forwarding at the egress isn't determined by anything in the DetNet
+YANG module, but rather non-DetNet forwarding is used? Maybe there 
+is some clearer way to say this? 
+---
 
 
 
@@ -245,10 +252,10 @@
 
    *  Service function indication, indicates which service function will
       be invoked at a DetNet edge, relay node or end station.  (DetNet
-      tunnel initialization or termination are default functions in
+      tunnel initialization or termination are default functions in the
       DetNet service layer, so there is no need for explicit
       indication).  The corresponding arguments for service functions
-      also needs to be defined.
+      also need to be defined.
 
 4.3.  DetNet Forwarding Sub-layer YANG Attributes
 
@@ -263,6 +270,14 @@
    transport layer related attributes are necessary:
 
    *  Flow Specification and Traffic Requirements, refers to [RFC9016].
+   
+---
+jgs: Would it be correct to elaborate the above something like
+
+   *  Flow Specification and Traffic Requirements, as described
+      in the information model in [RFC9016].
+---
+
       These may be used for resource reservation, flow shaping,
       filtering and policing by a control plane or other network
       management and control mechanisms.
@@ -286,7 +301,7 @@
 
    DetNet provides the capability of flow aggregation to improve
    scalability of DetNet data, management and control planes.
-   Aggregated flows can be viewed by the some DetNet nodes as individual
+   Aggregated flows can be viewed by some DetNet nodes as individual
    DetNet flows.  When aggregating DetNet flows, the flows should be
    compatible: if bandwidth reservations are used, the reservation
    should be a reasonable representation of the individual reservations;
@@ -297,6 +312,12 @@
    aggregation with the following functions:
 
    *  Aggregation flow encapsulation/decapsulation/identification
+   
+---
+jgs: I think the above should probably be rewritten as "Aggregated" flow,
+not Aggregation flow, but I'm flagging rather than changing inline in case
+you meant a different thing (if so, what?).
+---
 
    *  Mapping individual DetNet flows to an aggregated flow
 
@@ -345,7 +366,7 @@
 6.  DetNet YANG Structure Considerations
 
 
-   The picture shows that the general structure of the DetNet YANG
+   The picture shows the general structure of the DetNet YANG
    Model:
 
 
@@ -365,7 +386,7 @@
       | Profile    | | Profile    | | Profile      |   |Traffic Profile|
       +------------+ +------------+ +--------------+   +---------------+
 
-   There are three layer types in DetNet YANG Model: App-flow data
+   There are three layer types in the DetNet YANG Model: App-flow data
    layer, service sub-layer and forwarding sub-layer.  Additionally, the
    Traffic parameters are captured in a Traffic profile that can be
    referenced by any of the layers.
@@ -373,13 +394,19 @@
    Below is a a summary YANG tree showing the major items.  A complete
    YANG tree is in section Appendix A.
 
-   A traffic profile can be created for application, service sub-layer
-   or forwarding sub-layer.  A single profile may be shared by multiple
-   applications/sub-layer.  Each profile indicates the members that
+   A traffic profile can be created for an application, a service sub-layer
+   or a forwarding sub-layer.  A single profile may be shared by multiple
+   applications/sub-layers.  Each profile indicates the members that
    currently are using a profile.
+   
+---
+jgs: I wasn't sure if service sub-layer or forwarding sub-layer take a
+definite, or indefinite article in this context. I guessed "a" but if 
+it should be "the" please correct my correction.
+---
 
    Depending on which DetNet layers and functions are required, some or
-   all of the components may configured.  Examples are shown in
+   all of the components may be configured.  Examples are shown in
    Appendix B.
 
 
@@ -600,7 +627,7 @@
      identity failed {
        base app-status;
        description
-         "Application ingres/egresss failed.";
+         "Application ingres/egress failed.";
        reference
          "RFC 9016 Section 5.8";
      }
@@ -730,7 +757,7 @@
 Internet-Draft          draft-ietf-detnet-yang-17           October 2022
 
 
-             "This operation impose outgoing label(s) and forward to
+             "This operation imposes outgoing label(s) and forwards to the
               next-hop.";
            reference
              " A YANG Data Model for MPLS Base RFC 8960.";
@@ -751,18 +778,18 @@
          }
          enum swap-and-forward {
            description
-             "This operation swaps incoming label, with an outgoing
+             "This operation swaps an incoming label with an outgoing
               label and forwards to the next-hop.";
            reference
              " A YANG Data Model for MPLS Base RFC 8960.";
          }
          enum forward {
            description
-             "This operation forward to next-hop.";
+             "This operation forwards to the next-hop.";
          }
          enum pop-and-lookup {
            description
-             "This operation pops incoming label and performs a
+             "This operation pops an incoming label and performs a
               lookup.";
          }
        }
@@ -791,7 +818,7 @@
              "A Packet Replication Function (PRF) replicates DetNet
               flow packets and forwards them to one or more next hops in
               the DetNet domain.  The number of packet copies sent to
-              each next hop is a DetNet flow specific parameter at the
+              each next hop is a DetNet flow-specific parameter at the
               node doing the replication.  PRF can be implemented by an
               edge node, a relay node, or an end system.";
          }
@@ -851,6 +878,11 @@
              "This enum value means generate the sequence number by the
               DetNet flow.";
          }
+---
+jgs: a more descriptive description would be nice. The one above is
+essentially just a transcription of the name. I'd suggest text if I 
+were sure what to say.
+---
        }
        description
          "An enumeration for the sequence number behaviors supported.";
@@ -991,6 +1023,11 @@
        description
          "The TCP/UDP port(source/destination) identification
           information.";
+---
+jgs: What's "(source/destination) doing there? Surely this is just
+for the destination port, so maybe 'The TCP/UDP destination port' 
+would be appropriate?"
+---
        container destination-port {
          uses packet-fields:port-range-or-operator;
          description
@@ -1002,7 +1039,9 @@
        description
          "The TCP/UDP port(source/destination) identification
           information.";
-
+---
+jgs: Similarly to the above, this is the source port, only, right?
+---
 
 
 Geng, et al.              Expires 7 April 2023                 [Page 18]
@@ -1447,6 +1486,13 @@
                 MaxLatency is specified as an integer number of
                 nanoseconds. Any value above the MAX 4,294,967,295
                 is displayed as MAX";
+---
+jgs: I don't understand the last sentence. If it's a uint32, there *is*
+no value above 2^32-1, that's the highest representable value. So there
+is nothing to be "displayed as MAX". Furthermore, even if greater values
+were representable, is it the place of the YANG model to say how to 
+display them?
+---
              reference
                "RFC 9016 Section 4.2";
            }
@@ -1489,7 +1535,7 @@
              type uint32;
              units "packets";
              description
-               "Some applications have special loss requirement, such
+               "Some applications have special loss requirements, such
                 as MaxConsecutiveLossTolerance.  The maximum consecutive
                 loss tolerance parameter describes the maximum number of
                 consecutive packets whose loss can be tolerated.  The
@@ -1514,7 +1560,7 @@
 Internet-Draft          draft-ietf-detnet-yang-17           October 2022
 
 
-                maximum allowed misordering indicates that in order
+                maximum allowed misordering indicates that in-order
                 delivery is required, misordering cannot be tolerated.";
              reference
                "RFC 9016 Section 4.2";
@@ -1641,9 +1687,9 @@
              type boolean;
              default false;
              description
-               "Defines the data path requirement of the App-flow
+               "Defines the data path requirement of the App-flow,
                 whether it must share the same data path and physical
-                path for both directions through the network, e.g., to
+                path for both directions through the network, i.e., to
                 provide congruent paths in the two directions.";
              reference
                "RFC 9016
@@ -1653,14 +1699,14 @@
              type service-sub-layer-ref;
              config false;
              description
-               "Binding to this applications outgoing
+               "Binding to this application's outgoing
                 service.";
            }
            leaf incoming-service {
              type service-sub-layer-ref;
              config false;
              description
-               "Binding to this applications incoming service.";
+               "Binding to this application's incoming service.";
            }
            leaf traffic-profile {
              type traffic-profile-ref;
@@ -1706,7 +1752,7 @@
                "Route's next-hop attribute.";
              choice application-type {
                description
-                 "This is the application type choices.";
+                 "This is the application type choice.";
                container ethernet {
                  description
                    "This is TSN unaware traffic that maps to an
@@ -1823,7 +1869,7 @@
                container forwarding-aggregation {
                  description
                    "This service sub-layer is related to the forwarding
-                    sub-layer of the upper layer and provide
+                    sub-layer of the upper layer and provides
                     forwarding-to-service aggregation at the ingress
                     node or relay node.";
                  uses forwarding-sub-layer-group;
@@ -1831,7 +1877,7 @@
                container service-id {
                  description
                    "This service sub-layer is related to the service or
-                    forwarding sub-layer of the lower layer and provide
+                    forwarding sub-layer of the lower layer and provides
                     DetNet service relay or termination at the relay
                     node or egress node.";
                  uses detnet-flow-spec;
@@ -1860,7 +1906,7 @@
                     sub-layers of the lower layer for DetNet service
                     forwarding or service-to-forwarding aggregation at
                     the ingress node or relay node.  When the operation
-                    type is service-initiation, The service sub-layer
+                    type is service-initiation, the service sub-layer
                     encapsulates the DetNet Control-Word and services
                     label, which are for individual DetNet flow when the
                     incoming type is app-flow and for aggregated DetNet
@@ -2004,7 +2050,7 @@
                container service-sub-layer {
                  description
                    "This forwarding sub-layer is related to the service
-                    sub-layers of the upper layer and provide DetNet
+                    sub-layers of the upper layer and provides DetNet
                     forwarding or service-to-forwarding aggregation at
                     the ingress node or relay node.";
                  uses service-sub-layer-group;
@@ -2020,7 +2066,7 @@
 
                  description
                    "This forwarding sub-layer is related to the
-                    forwarding sub-layer of the upper layer and provide
+                    forwarding sub-layer of the upper layer and provides
                     forwarding-to-forwarding aggregation at the ingress
                     node or relay node or transit node.";
                  uses forwarding-sub-layer-group;
@@ -2028,7 +2074,7 @@
                container forwarding-id  {
                  description
                    "This forwarding sub-layer is related to all of the
-                    lower layer and provide DetNet forwarding swap or
+                    lower layer and provides DetNet forwarding swap or
                     termination at the transit node or relay node or
                     egress node.";
                  leaf interface {
@@ -2048,7 +2094,7 @@
              choice outgoing {
                mandatory true;
                description
-                 "This is when a service connected directly to an
+                 "This is when a service is connected directly to an
                  interface with no forwarding sub-layer.";
                container
                  interface {
@@ -2110,6 +2156,12 @@
                     disaggregation at the relay node or egress node.
                     This outgoing type only can be chosen when the
                     operation type is pop-and-lookup.";
+---
+jgs: the term "F-label" appears here without explanation, and in the following
+description, and figure 7, and nowhere else. I assume it means "forwarding 
+label" but if so, can you either use the term "forwarding label" which is
+self-explanatory, or gloss "f-label" somewhere?
+---
                  uses service-sub-layer-group;
                  reference
                    "RFC 8964 Section 4.2.3";
@@ -2122,6 +2174,9 @@
                     at the transit node or relay node or egress node.
                     This outgoing type only can be chosen when the
                     operation type is pop-and-lookup.";
+---
+jgs: same comment about F-label above.
+---
 
 
 
@@ -2206,8 +2261,8 @@
    be considered sensitive.
 
    /detnet/traffic-profile/member-app: This links traffic profiles to
-   applications, o this also could be considered more sensitive.  The
-   traffic profiles liked to service sub-layer and forwarding sub-layer
+   applications, so this also could be considered more sensitive.  The
+   traffic profiles linked to service sub-layer and forwarding sub-layer
    are less sensitive.
 
    /detnet/service/sub-layer/incoming/app-flow: This links applications
@@ -2219,7 +2274,7 @@
 11.  Contributors
 
    The editors of this document wish to thank and acknowledge the
-   following people who contributed substantially to the content of this
+   following person who contributed substantially to the content of this
    document and should be considered coauthors:
 
    Mach(Guoyi) Chen
@@ -2891,7 +2946,7 @@
 
 Appendix B.  Examples
 
-   The following examples are provided.  These examples are tested with
+   The following examples are provided.  These examples have been tested with
    Yanglint and use operational output to exercise both config true and
    config false objects.  Note that IPv4 and IPv6 addresses are
    supported but for clarity in the examples and diagrams IPv4 has been
@@ -2899,7 +2954,7 @@
    these support both IPv4 and IPv6.
 
    The following are examples of aggregation and disaggregation at
-   various points in Detnet.  Figures are provided in the PDF version of
+   various points in Detnet.  Figures are provided in the HTML and PDF versions of
    this document.
 
 
@@ -2923,6 +2978,19 @@
    address format.
 
    Please consult the PDF or HTML versions for the Case A-1 Diagram.
+   
+---
+jgs: In the diagram, some of the labels aren't self-explanatory, it seems
+like a legend is needed. I noted "DN-1", "R" in a circle, and "E" in a circle, 
+as needing to be glossed. (I presume "R" is "replication" and "E" is 
+"elimination", but please spell this out in a legend.)
+
+This comment applies to the later diagrams as well.
+
+In all of the examples, I presume the shown configuration applies to one or
+in some cases two of the boxes shown on the figure. Please specify, for each
+example, what configuration corresponds to what box.
+---
 
          Figure 2: Case A-1 Example JSON Operational/Configuration
 
@@ -3176,7 +3244,7 @@
 
 B.2.  Example B-1 XML Config: Aggregation using a Forwarding Sub-layer
 
-   This illustrates aggrgation in the service sub-layers of DetNet.
+   This illustrates aggregation in the service sub-layers of DetNet.
    Flows 1 and 2 are aggregated into a forwarding sub-layer.  A diagram
    illustrating this case is shown and then the corresponding XML
    operational data follows.
@@ -3439,9 +3507,22 @@
    This illustrates the service sub-layers of DetNet.  Flows 1 and 2 are
    aggregated into a service sub-layer of aggregated DetNet flow 1.  A
    diagram illustrating this case is shown and then the corresponding
-   JSON operational data follows.
+   JSON operational data for node Ingress 1 follows.
+---
+jgs: Would it be correct to interpolate "for node Ingress 1" as I've done?
+---
 
    Please consult the PDF or HTML versions for the Case B-2 Diagram.
+   
+---
+jgs: More items needing a legend appear on this diagram. S and A are
+identifiable by the diligent reader but an explanation would still be
+helpful, I assume they mean "source" and "aggregation". F-label is also
+a little problematic, as previously noted. 
+
+What is the meaning of the dashed boxes around "aggregation" and
+"disaggregation"?
+---
 
             Figure 7: Case B-2 Example JSON Service Aggregation
 
@@ -3700,6 +3781,13 @@
 
        }
      },
+---
+jgs: I found myself wondering why we need to have these interface
+stanzas spelled out in detail -- they don't help with illustrating the
+use case, in particular. I guess it's because these examples were 
+scraped directly from working labs, and are tested to work, and you
+don't want to alter them?
+---
      "ietf-interfaces:interfaces": {
        "interface": [
          {
@@ -3766,6 +3854,18 @@
    Please consult the PDF or HTML versions for the Case C-1 Diagram.
 
      Figure 9: Case C-1 Example JSON Service Aggregation/Disaggregation
+     
+---
+jgs: I see there is a de minimis identification of what this configuration
+applies to, in the "figure 10" legend that appears below it. This comes too
+late, and is also too subtle, to help the reader. Please consider introducing
+each distinct configuration with a short paragraph, as in,
+
+   Following is the JSON configuration for aggregation node Relay 1.
+   
+Making this configuration its own subsection would also help the reader to
+not get lost.
+---
 
    {
      "ietf-detnet:detnet": {
@@ -4231,6 +4331,14 @@
 
         Figure 10: Example C-1 DetNet JSON Relay Service Aggregation
 
+---
+jgs: and similarly, it would be helpful to introduce this one with something
+like 
+
+   Following is the JSON configuration for etc etc.
+
+and similarly, it might be helpful to put it in its own subsection.
+---
    {
      "ietf-detnet:detnet": {
        "traffic-profile": [
@@ -4687,10 +4795,16 @@
 
 B.5.  Example C-2 JSON Relay Aggregation Service Sub-Layer
 
-   This illustrates the Relay node 1 aggregating the service sub-layers
-   of DetNet flows 1 and 2 into a forwarding sub-layer A diagram
+   This illustrates the node Relay 1 aggregating the service sub-layers
+   of DetNet flows 1 and 2 into a forwarding sub-layer. A diagram
    illustrating both aggregation and disaggregation is shown and then
    the corresponding JSON operational data follows.
+---
+jgs: Shouldn't you also mention that it shows Relay 2 disaggregating it, 
+the way you do in B.6?
+
+A similar comment applies to B.4.
+---
 
    Please consult the PDF or HTML versions for the Case C-2 Diagram.