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

[Don Fedyk <[e-mail]>]

Hi Xufeng


Thanks for your detailed comments. 

Comments inline [Don]

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

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.
[Don] raised as a Tools issue to the editor. 

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.

[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) 

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.
[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. 

    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.
[Don] DetNet flow equals App-Flow.  Source and destination are used as traffic qualifiers for IP flows.  

    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.
[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. 

    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”.
[Don] This is Unidirectional. Ingress is the correct placement. The other direction is Ingress on the remote end. 

    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.
[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.  

    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.
[Don] This is a device model. The controller configures multiple devices. If the configuration does not align the flow will not work. 

    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?
[Don] We support point to point and point to multipoint. (Replication is an example.) 

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.
[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