Re: [Detnet] WG Last Call: draft-ietf-detnet-yang-12

[t petch <[e-mail]>]

----- Original Message -----
From: "Janos Farkas" <[e-mail]>
Sent: Thursday, May 27, 2021 10:34 PM

> This starts Working Group Last Call on draft-ietf-detnet-yang-12
>
> https://datatracker.ietf.org/doc/draft-ietf-detnet-yang/
>
> The Working Group Last Call ends on June 11th.
>
> Please send your comments to the working group mailing list.

My previoius comments were largely  editorial, looking to bring the I-D
in line with YANG Guidelines which I would expect to be relatively
straightforward.  These comments are a bit more techinical and some may
be easier to progress in a separate thread.

References.  Could do with more in the YANG module, one per leaf/list
etc is not too many - I identify some specific ones I struggle with
below.

Address Families.  Need explaining in the body of the I-D. I know of two 
different meanings in other WG and recently encountered a third, strange 
one.

Augments.  Need explaining in the body of the I-D.  YANG that is valid
for a standalone module may be invalid for a model of several modules
(as the TEAS WG recently found).  Some authors include such an augment
as an Appendix to guide developers.  At the least there should be
explanations as to what kind of augments are anticipated where as 
opposed to reverse engineering the YANG.

YANG Prefix.  Good practice is to have prefix for relatedmodules with an
obvious connection, such as the same 3-4-5 letters with a suffix to
separate them.  This reinforces my earlier comment about the chosen
prefix being long as and when there are augmenting modules.

      typedef app-flow-ref {
What is an application in this context?  Could do with a reference and a
paragraph in the body of the I-D.

      typedef ipsec-spi {
Others model spi as uint64.  I see nothing in RFC6071 to tell me what
the valid range of values is.

      typedef service-operation-type {
and most of the other typedef
Context is usually enough to know the class of an identifier; suggest
removing '-type'

      typedef forwarding-operations (-type { )
Could do with a reference or explanation for the enum which is not in 
RFC8960

           MPLS enum.  The first 4 enums are the same as A YANG Data
           Model for MPLS Base.  draft-ietf-mpls-base-yang.";
And the other enum not in RFC8960?

      grouping ip-header {
          type inet:ip-address-no-zone;
A wise choice but why is the tree diagram inet:ip-address?  Is the tree
diagram really of this version of the module?

        choice detnet-flow-type {
          case ip-detnet-flow {
Elsewhere it is 'case  ip' and 'case mpls'; I would think 'detnet-' is
wholy redundant and probably '-flow' as well.

        choice header-type {
          case detnet-mpls-header {
As above, ditch the 'detnet-' and probably '-header' as well

            leaf outgoing-interface {
...
                "The outgoing interface, if this is a whole interface.";
What is an interface when it is not whole? A reference perhaps.  This
occurs elsewhere.

      grouping detnet-forwarding-next-hop-content {
This I am confused by.
           case simple-next-hop {
...
               Modules for address families MUST augment this case with a
               leaf containing a next-hop address of that address
               family.";
This sort of implies that
            leaf outgoing-interface {
is an address family (not that I know what a detnet address family is)
whereas
          case next-hop-list {
...
                                                 Modules for address
                 families MUST augment this list with a leaf containing a
                 next-hop address of that address family.";
sort of suggests that
               leaf hop-index {
is an address family.  I am unsure what is meant to be augmented; YANG
choice?.

          leaf profile-name {
            type string;
            description
              "An Aggregation group ID. Zero means the service is not
               part of a group.";
How is zero encoded?

            leaf min-bandwidth {
              type uint64;
              units "bps";
..
                 guaranteed for the DetNet service.  MinBandwidth is
                 specified in octets per second.";
byte per second?

            leaf max-latency {
              type uint32;
              units "nanoseconds";
Has the max value a special meaning, such as too big for a uint32?

                "The period of time in which the traffic
                 specification cannot be exceeded.";
Mmm.. should not?

            leaf max-payload-size {
I would like a reference.  MTU and such like are a source of confusion
as to what is included; I want to know the name of the protocol field
that starts the payload and where that field is defined.

          list app-flow {
            leaf name {
              type string;
RFC9016 would appear to label this as 'flowid'

                  "The DetNet service protection type such as PRF, PEF,
                   PEOF,PERF, and PEORF.";
Abbreviations need expanding.

                reference
                [RFC9016]  "draft-ietf-detnet-data-plane-framework
Section 4.3";
I see no section 4.3 in RFC9016

              choice outgoing-type {
Probably better without '-type'

                  "The out-going type may be a forwarding Sub-layer or a
With or with-out a hyphen?  I like consistency.

                   service sub-layer or ? types need to be named.";
??

                     Control-Word and S-label when the operation type is
I need a reference; they are not in RFC9016 although I might guess where
they are.

                     decapsulates the DetNet Control-Word and A-label for
ditto

            leaf forwarding-operation-type {
              type forwarding-operations-type;
The typedef list six options; what about the sixth?

                      "This is reference to the forwarding sub-layer.";
Not quite English.

Tom Petch

> Positive comments, e.g., "I've reviewed this document and believe it
is ready for publication", are welcome!
>
> This is useful and important, even from authors.
>
> Thank you,
>
> János (DetNet Co-Chair & doc Shepherd)
>
>
>