Subject: [Detnet] Yangdoctors early review of draft-ietf-detnet-yang-12

From: Xufeng Liu via Datatracker <[email-masked]>
Date: Sun, 27 Jun 2021 15:10:49 -0700

Reviewer: Xufeng Liu
Review result: On the Right Track

This is a review of the YANG module in draft-ietf-detnet-yang-12.

1) Missing diagrams in the PDF and HTML versions of the document.
Sec A.1. says “Please consult the PDF or HTML versions for the Case A-1
Diagram”, but the this diagram and other diagrams are not present. Because of
the missing diagrams, it is unclear how these examples work, so the following
comments are based on some assumptions that may or may not be what the authors
intend to hold.

2) Implementation locations and the users of the model.
    Is this a network model implemented on a controller or a device model
    implemented on a network element? The abstract section states “The model
    allows for provisioning of end-to-end DetNet service”, but the YANG module
    and all examples use “ietf-interfaces”, which is a device model. The
    interface name such as “eth0” is unique within the scope of a device. There
    is no explanation in the YANG module and any of these examples.
   If the model is intended to be used on a network-wide controller, any device
   module should not be imported. A network-wide module may be used on a
   device, and a device module may then be imported.

3) In Sec 5., in the diagram, “TR” is used, but not defined. Amusing that “Ref”
means “referencing”, the abbreviation also needs to be defined, especially if
this is using the YANG leafref mechanism.

4) In Sec 5., the last paragraph states “four parts of DetNet functions defined
in section 3”, but “section 3” does not define the “four” parts.

5) In Sec 5., the last paragraph describes “three instances in DetNet YANG
Model”. The term “instance” is not clear here. A YANG data model does not
usually consist of “instances”. RFC7950 uses “instance” to indicate the
configuration data or operational data, so “instance” means “data instance”,
which is not part of a YANG data model that “describes how data is represented
and accessed”.

In Sec A.1, Example A-1 provides one data instance that contains all three
“parts” (or components) of the DetNet system, so there is no such a concept of
“App-flow instance”, “service sub-layer instance”, or “forwarding sub-layer
instance”, because one single “data instance” may contain all three parts.

6) In Sec 6. in the sub-tree app-flows,
    6.1) Does this sub-tree model App-flow or NetNet flow, or both?
    draft-ietf-detnet-flow-information-model-14 defines both terms and
    describes the distinctions between them.

    6.2) Except the leaf "name", all other leaves are optional. When a user
    creates an instance of app-flow with only the name specified, what does the
    system do?

    6.3) Containers “ingress” and “egress” are specified. However, based on Sec
    2.1 of draft-ietf-detnet-flow-information-model-14, An app-flow consists of
    source and destination, not ingress and egress. “DN Ingress” and “DN
    Egress” are parts of a “DetNet flow”, not app-flow.

    6.4) Container “ingress” should not contain both source and destination.
    Sec 2.1. of draft-ietf-detnet-flow-information-model-14 defines the
    “ingress” as the location where DetNet flow starts, so the ingress is
    related only to the source, not the destination.

    6.5) Based on Sec 5.4. of draft-ietf-detnet-flow-information-model-14, the
    choice data-flow-type under the container “ingress” is how a DN flow is
    identified, so this choice is not just related to the ingress of the flow,
    and should be outside of the container “ingress”.

    6.6) Based on Sec 5.6. of draft-ietf-detnet-flow-information-model-14, the
    ingress section of the model should identify the ingress node(s) and/or
    starting reference points. It would be good to have some description on how
    the ingress node(s) is (are) identified, especially in the case of
    mpls-app-flow. The description on the leaf name currently is "Ingress
    DetNet application", which is not consistent with the definition in
    draft-ietf-detnet-flow-information-model-14.

    6.7) Why is app-flow-status under the container ingress? app-flow-status is
    related to an instance of app-flow, not only the “ingress” part of the
    app-flow, right?

    6.8) The leaf interface in the container ingress would only work when this
    model is implemented on a device (but not on a controller) because
    ietf-interfaces is a device model. Based on my previous assumption that
    this model may be used as a network-wide model on a controller, the
    referencing to ietf-interfaces would not be appropriate.

    6.9) The relationship between app-flow and service-sub-layer needs better
    described. When an app-flow is served by a DN service, the packets of this
    app-flow are processed by this particular ND service, so there is only one
    supporting service. Why are there the “incoming-service” and
    “outgoing-service”? Are the terms “incoming-service” and “outgoing-service”
    defined and/or described in any document?

7) In Sec 6. in the  sub-tree traffic-profile:
   The container flow-spec is not consistent with
   draft-ietf-detnet-flow-information-model-14, which defines the leaves under
   this container as TrafficSpecification in Sec 4.1.

8) Before Sec 6, the document does not have an informative reference to the
YANG tree diagrams specification RFC8340. Refer to Section 2.2 of [RFC8349] for
an example of such a reference.

9) In Sec 7, the document text does not have the references to the RFCs cited
in the YANG module. These references also need to be listed in the document.
Sec 5 of RFC7317 provides a good example.

10) In Sec 9, the Security section is missing.

11) In Sec 8, the IANA Considerations section is incorrect, with missing IANA
requests.

12) For the IP address or prefix examples in the Appendix,  a mix of either
IPv4 and IPv6 addresses or IPv6 addresses exclusively SHOULD be used.

13) The module prefix ietf-detnet is better to be changed to detnet.
IETF-defined YANG modules usually do not use “ietf-” as part of the prefix name.

14) As described in Sec 4.3.1. of RFC8407, child nodes within a container or
list SHOULD NOT replicate the parent identifier. There are several occasions in
the YANG module, for example, profile-name, app-flow-bidir-congruent,
service-protection, and service-operation-type.

15) The node name in the list or leaf-list statements should not contain the
suffix “-list”, or in the plural format. For example, service-sub-layer-list is
better to be changed to service-sub-layer; member-services should be
member-service. The structure of app-flows/app-flow* is the common convention
used in IETF. It would be good to be consistent within this module, so it will
be a good idea to add a container "traffic-profiles" to the list
"traffic-profile".

16) Most of the leaves in this module are defined as optional, but only one
leaf has a default statement in the entire module. It is generally needed to
either provide a default value or an explanation in the description for such an
optional leaf.

17) There is no configuration node and operational state node to indicate
whether DetNet is enabled on the system. Please confirm that this is the
intention. The fact that this module is supported on a system may not mean that
DetNet is enabled.

Thanks,
- Xufeng

---

From: Don Fedyk <[email-masked]>
Date: Mon, 28 Jun 2021 12:28:46 +0000

Hi Xufeng

Thank you for your review.  We will address your points in some subsequent emails and updates. 
We are currently working some points on raised on the list by Tom Petch.
Your first point about missing diagrams I think is a IETF toolchain issue.  The HTML at 
https://www.ietf.org/archive/id/draft-ietf-detnet-yang-12.html
renders diagrams correctly but the PDF is not what I get when I generated using the local xml2rfc toolchain.  
I have sent off a request to the RFC tools team to see if the PDF can be corrected or if there is a submission item I missed.

Thanks,
Don and the YANG draft authors.

-----Original Message-----
From: Xufeng Liu via Datatracker <[email-masked]> 
Sent: Sunday, June 27, 2021 6:11 PM
To: [email-masked]
Cc: [email-masked]; [email-masked]
Subject: Yangdoctors early review of draft-ietf-detnet-yang-12

Reviewer: Xufeng Liu

1) Missing diagrams in the PDF and HTML versions of the document.
Sec A.1. says “Please consult the PDF or HTML versions for the Case A-1 Diagram”, but the this diagram and other diagrams are not present. Because of the missing diagrams, it is unclear how these examples work, so the following comments are based on some assumptions that may or may not be what the authors intend to hold.

3) In Sec 5., in the diagram, “TR” is used, but not defined. Amusing that “Ref”
means “referencing”, the abbreviation also needs to be defined, especially if this is using the YANG leafref mechanism.

4) In Sec 5., the last paragraph states “four parts of DetNet functions defined in section 3”, but “section 3” does not define the “four” parts.

5) In Sec 5., the last paragraph describes “three instances in DetNet YANG Model”. The term “instance” is not clear here. A YANG data model does not usually consist of “instances”. RFC7950 uses “instance” to indicate the configuration data or operational data, so “instance” means “data instance”, which is not part of a YANG data model that “describes how data is represented and accessed”.

In Sec A.1, Example A-1 provides one data instance that contains all three “parts” (or components) of the DetNet system, so there is no such a concept of “App-flow instance”, “service sub-layer instance”, or “forwarding sub-layer instance”, because one single “data instance” may contain all three parts.

8) Before Sec 6, the document does not have an informative reference to the YANG tree diagrams specification RFC8340. Refer to Section 2.2 of [RFC8349] for an example of such a reference.

9) In Sec 7, the document text does not have the references to the RFCs cited in the YANG module. These references also need to be listed in the document.
Sec 5 of RFC7317 provides a good example.

11) In Sec 8, the IANA Considerations section is incorrect, with missing IANA requests.

14) As described in Sec 4.3.1. of RFC8407, child nodes within a container or list SHOULD NOT replicate the parent identifier. There are several occasions in the YANG module, for example, profile-name, app-flow-bidir-congruent, service-protection, and service-operation-type.

15) The node name in the list or leaf-list statements should not contain the suffix “-list”, or in the plural format. For example, service-sub-layer-list is better to be changed to service-sub-layer; member-services should be member-service. The structure of app-flows/app-flow* is the common convention used in IETF. It would be good to be consistent within this module, so it will be a good idea to add a container "traffic-profiles" to the list "traffic-profile".

16) Most of the leaves in this module are defined as optional, but only one leaf has a default statement in the entire module. It is generally needed to either provide a default value or an explanation in the description for such an optional leaf.

17) There is no configuration node and operational state node to indicate whether DetNet is enabled on the system. Please confirm that this is the intention. The fact that this module is supported on a system may not mean that DetNet is enabled.

Thanks,
- Xufeng

---

From: Don Fedyk <[email-masked]>
Date: Tue, 29 Jun 2021 23:44:36 +0000

Hi Xufeng

Thanks for your detailed comments. 

Comments inline [Don]

-----Original Message-----
From: Xufeng Liu via Datatracker <[email-masked]> 
Sent: Sunday, June 27, 2021 6:11 PM
To: [email-masked]
Cc: [email-masked]; [email-masked]
Subject: Yangdoctors early review of draft-ietf-detnet-yang-12

Reviewer: Xufeng Liu

1) Missing diagrams in the PDF and HTML versions of the document.
Sec A.1. says “Please consult the PDF or HTML versions for the Case A-1 Diagram”, but the this diagram and other diagrams are not present. Because of the missing diagrams, it is unclear how these examples work, so the following comments are based on some assumptions that may or may not be what the authors intend to hold.
[Don] raised as a Tools issue to the editor. 

[Don] Will Clarify.  The model is per device. Either an operator or a Controller aligns the configuration of the devices. 

3) In Sec 5., in the diagram, “TR” is used, but not defined. Amusing that “Ref”
means “referencing”, the abbreviation also needs to be defined, especially if this is using the YANG leafref mechanism.
[Don] TR should be Traffic Profile. I have expanded this acronym. 

4) In Sec 5., the last paragraph states “four parts of DetNet functions defined in section 3”, but “section 3” does not define the “four” parts.
[Don] Wil fix this. This is referring to App-flow, Service Sub-layer, Forwarding Sub-layer and traffic Profile (as 4 but this this confusing.) The Modle has 4 main section but supports 3 layers (not instances) 

In Sec A.1, Example A-1 provides one data instance that contains all three “parts” (or components) of the DetNet system, so there is no such a concept of “App-flow instance”, “service sub-layer instance”, or “forwarding sub-layer instance”, because one single “data instance” may contain all three parts.
[Don] App-flow is a layer.  A node for example an edge node is a data instance that has the 3 layers. 

6) In Sec 6. in the sub-tree app-flows,
    6.1) Does this sub-tree model App-flow or NetNet flow, or both?
    draft-ietf-detnet-flow-information-model-14 defines both terms and
    describes the distinctions between them.
[Don] App-Flow is a DetNet flow it all depends on the granularity and where you are. At an edge a flow or set of flows come from an application. It could be a single applications i.e. a voice call or a bunch of flows - another DetTet network (with voice and video flows). 
Note it is all unidirectional.  

    6.2) Except the leaf "name", all other leaves are optional. When a user
    creates an instance of app-flow with only the name specified, what does the
    system do?
[Don] Will check but in essence if the Application is an ingress interface there is not much there. I think we can make interface mandatory (Multiple apps can aggregate multiple interfaces.)   Traffic flow qualifiers can be refine to less than an interface. 

[Don] DetNet flow equals App-Flow.  Source and destination are used as traffic qualifiers for IP flows.  

[Don] Ingress is for aggregation / filtering of traffic in a unidirectional.  It is like an Access control list for the traffic.  Will add a note. But source and destination is correct. 

[Don] This is Unidirectional. Ingress is the correct placement. The other direction is Ingress on the remote end. 

[Don] These are identified by interfaces and next-hops (interfaces). 

    6.7) Why is app-flow-status under the container ingress? app-flow-status is
    related to an instance of app-flow, not only the “ingress” part of the
    app-flow, right?
[Don] The status is unidirectional. The remote side has the other direction status.  

[Don] This is a device model. The controller configures multiple devices. If the configuration does not align the flow will not work. 

[Don] We support point to point and point to multipoint. (Replication is an example.) 

[Don] It is up the to WG if they want to change the name. I will ask the work Group. The flow specification is a functional model. The DetNet model does not map 1:1. 

8) Before Sec 6, the document does not have an informative reference to the YANG tree diagrams specification RFC8340. Refer to Section 2.2 of [RFC8349] for an example of such a reference.
[Don] Added. Tom Petch Commented. 

9) In Sec 7, the document text does not have the references to the RFCs cited in the YANG module. These references also need to be listed in the document.
Sec 5 of RFC7317 provides a good example.
[Don] Added. Tom Petch Commented.

10) In Sec 9, the Security section is missing.
[Don] Added. Tom Petch Commented.

11) In Sec 8, the IANA Considerations section is incorrect, with missing IANA requests.
[Don] Added. Tom Petch Commented.

12) For the IP address or prefix examples in the Appendix,  a mix of either
IPv4 and IPv6 addresses or IPv6 addresses exclusively SHOULD be used.
[Don] The model supports it but it is a royal pain to align the sample config and the SVG diagrams. I would much rather say IPv4 is shown and IPv6 is supported. I already did some conversion of the from Tom Petch's comments apparently there is a Ipv4 IETF space we have to use. You can see the updates here:  https://github.com/detnet-wg/draft-ietf-detnet-yang/blob/master/draft-ietf-detnet-yang-13.pdf
The issue is IPv6 examples do not clarify anything as far as I'm concerned. They just burn more text and diagram real estate (and editorial time). 

13) The module prefix ietf-detnet is better to be changed to detnet.
IETF-defined YANG modules usually do not use “ietf-” as part of the prefix name.
[Don] Changed to dnet per Tom Petch Comment. 

14) As described in Sec 4.3.1. of RFC8407, child nodes within a container or list SHOULD NOT replicate the parent identifier. There are several occasions in the YANG module, for example, profile-name, app-flow-bidir-congruent, service-protection, and service-operation-type.
[Don] Perhaps the YANG tools should prevent this! We evolved the naming several times. Personally. I prefer to make the container profile and the child name for example which works when reading the tree diagram and in the config examples. 

15) The node name in the list or leaf-list statements should not contain the suffix “-list”, or in the plural format. For example, service-sub-layer-list is better to be changed to service-sub-layer; member-services should be member-service. The structure of app-flows/app-flow* is the common convention used in IETF. It would be good to be consistent within this module, so it will be a good idea to add a container "traffic-profiles" to the list "traffic-profile".
[Don] Already removed -type.  Will remove -list. 

16) Most of the leaves in this module are defined as optional, but only one leaf has a default statement in the entire module. It is generally needed to either provide a default value or an explanation in the description for such an optional leaf.
[Don] Correct the leaves are optional - the model support various levels of traffic refinement (6-tuple) aggregation etc. 

17) There is no configuration node and operational state node to indicate whether DetNet is enabled on the system. Please confirm that this is the intention. The fact that this module is supported on a system may not mean that DetNet is enabled.
[Don] I don't know I don't see a system enable/disable being useful. Perhaps a feature statement? Then if a system does not support the feature it is disabled. 

Thanks again. 
Don  

Thanks,
- Xufeng

---

From: Don Fedyk <[email-masked]>
Date: Wed, 30 Jun 2021 13:40:09 +0000

DetNet

Currently The DetNet YANG uses Traffic Profile that contains the Traffic requirements and Traffic Specification (we use flow spec rather that the longer term). Xufeng thought the term Traffic Specification aligns more with the terminology in RFC 9016. In developing the YANG we had originally had (at one point anyway) the Traffic requirements and Traffic Specification in the application, service sub-layer etc there was no need for another grouping. At some point as we discussed the fact that many flows might have the same parameters we came up with a shared set of profiles that could be referenced by multiple objects. 

Rereading RFC 9016 the organization is:

App-Flow Characteristics
  Traffic Specification (also flow-spec)
 (other flow aspects) 

App-flow Requirements 
  Flow requirements.

The YANG has this grouping  (where the attributes are optional allowing the same structure to do requirements and/or specification). 

     +--rw traffic-profile* [profile-name]
     |  +--rw profile-name            string
     |  +--rw traffic-requirements
     |  |  +--rw min-bandwidth?                    uint64
     |  |  +--rw max-latency?                      uint32
     |  |  +--rw max-latency-variation?            uint32
     |  |  +--rw max-loss?                         uint32
     |  |  +--rw max-consecutive-loss-tolerance?   uint32
     |  |  +--rw max-misordering?                  uint32
     |  +--rw flow-spec
     |  |  +--rw interval?                uint32
     |  |  +--rw max-pkts-per-interval?   uint32
     |  |  +--rw max-payload-size?        uint32
     |  |  +--rw min-payload-size?        uint32
     |  |  +--rw min-pkts-per-interval?   uint32
     |  +--ro member-apps*            app-flow-ref
     |  +--ro member-services*        service-sub-layer-ref
     |  +--ro member-fwd-sublayers*   forwarding-sub-layer-ref

The question is, is there a better alignment of terms? 

I would argue the profile is about configuration grouping, a YANG construct. We can point this out in the text of the YANG and the document to make the linkage. 

(Also I have commented on this and other points raised by Xufeng - cc'd on the list - but this one I request we confirm consensus. Note the mapping of Flow Spec to YANG is not one to one because the Flow spec is a functional description and the DetNet YANG is a hierarchical in nature some of the App-Flow characteristics are in the application not the profile .) 

Please Comment
Thanks, 
Don

<Xuefeng's comment>