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

Xufeng Liu <xufeng.liu.ietf@gmail.com> Tue, 06 July 2021 20:46 UTC

Return-Path: <xufeng.liu.ietf@gmail.com>
X-Original-To: detnet@ietfa.amsl.com
Delivered-To: detnet@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 904F83A0B1C; Tue, 6 Jul 2021 13:46:34 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.097
X-Spam-Level:
X-Spam-Status: No, score=-2.097 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id QkSWhJx5GgnG; Tue, 6 Jul 2021 13:46:29 -0700 (PDT)
Received: from mail-ej1-x62e.google.com (mail-ej1-x62e.google.com [IPv6:2a00:1450:4864:20::62e]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 9CED23A0B16; Tue, 6 Jul 2021 13:46:28 -0700 (PDT)
Received: by mail-ej1-x62e.google.com with SMTP id hc16so36255156ejc.12; Tue, 06 Jul 2021 13:46:28 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=RTgIxzpSpix6yNTB7yfRoL6Fk0KJrJmdcrrAR9S8KMI=; b=rrZRDL/xURwIUnBcdtPI+gvGoqEE2LK6JY6oV5yQ5Eg1X+aflwbh2ERpKRBJ8PmZLG EiA/WlQ0OW5CchB6Mn6pg9X0xh4pFanDlV2sOcOpC/uvrYHnFHEJltpOX98C0j34Ll3Q JKWHa/qZ4aaAGmblkpE2D4dVQm21XBBV9g1ydN5D8Y0bea7okHx7BRALYZg9BvD9Jasg DTdoiMPhdrWpgJd+48FMhD2PULYWR9uf+pCrJdE/GcO3IDwuI+l0Wai9pomW2FEcTap3 Rke61n5xx1tKXaZ+7IFPNn6KtMckvpSD4t5Z+Pfqr//qOXT1/XWLDry9uji2r906Zde5 rwaw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=RTgIxzpSpix6yNTB7yfRoL6Fk0KJrJmdcrrAR9S8KMI=; b=XLJJLRn/TRFsFNrKHagbQj5RCnJXm/dR0wuQBY7wnf5rjXOh6Qmb8oAph1d4tq120h Nipnzqg8nCRFKDQH7I2DkZ2w7uonzkHRK43P+ysPFjVXjJpdrYCzs3BzQLHxRwIdQIao gTbvtoiE3d/COEneMNmY+hMgdX+ACXU7G2LYNLAfssHAw6ilAKv+d6r6RuukzWnErRD8 mtU4dL0aIorfQJejOEjwwWqeMxO4YfWAT4XKzgVUzM29RRL75LjfT670r6JgZJpycoby ZKV/e+U7c3tYdWJVMmhWVVqgMHS+FujbizheAhFOt9TjTD/kPVVhdLXdHEbZ2YkF+B6X UxKw==
X-Gm-Message-State: AOAM5326ukCTIOG5OJ4hHvltBI3q1jYgbV7VYVm4hmC4dehQMaz/3x9t okxCCcO8m+04BGrO/B4ayRhcFlZStbwHkZlF35I=
X-Google-Smtp-Source: ABdhPJwDYSyJBjPxD7mhAbvO5wuW9iTUN8WEdbhOu+85s5Wpb0P095P2PSQnuuBi7LyXBG/NjesKH1MtT/s3eaphiX4=
X-Received: by 2002:a17:907:d03:: with SMTP id gn3mr20943919ejc.516.1625604385209; Tue, 06 Jul 2021 13:46:25 -0700 (PDT)
MIME-Version: 1.0
References: <162483184905.11951.18366537346797539633@ietfa.amsl.com> <MN2PR14MB40304BBE8F9D17405F4031DFBB029@MN2PR14MB4030.namprd14.prod.outlook.com>
In-Reply-To: <MN2PR14MB40304BBE8F9D17405F4031DFBB029@MN2PR14MB4030.namprd14.prod.outlook.com>
From: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Date: Tue, 6 Jul 2021 16:46:13 -0400
Message-ID: <CAEz6PPQoueOJm7eFSsk8rkkMN=a6GQzETX9feWo5CeWkKMWQVA@mail.gmail.com>
To: Don Fedyk <dfedyk@labn.net>
Cc: "yang-doctors@ietf.org" <yang-doctors@ietf.org>, "detnet@ietf.org" <detnet@ietf.org>, "draft-ietf-detnet-yang.all@ietf.org" <draft-ietf-detnet-yang.all@ietf.org>
Content-Type: multipart/alternative; boundary="000000000000ed023c05c67a831e"
Archived-At: <https://mailarchive.ietf.org/arch/msg/detnet/vdDWV1CsgpP-adxuljmYxtRLjpI>
Subject: Re: [Detnet] Yangdoctors early review of draft-ietf-detnet-yang-12
X-BeenThere: detnet@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Discussions on Deterministic Networking BoF and Proposed WG <detnet.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/detnet>, <mailto:detnet-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/detnet/>
List-Post: <mailto:detnet@ietf.org>
List-Help: <mailto:detnet-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/detnet>, <mailto:detnet-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 06 Jul 2021 20:46:42 -0000

Hi Don,

Thanks for addressing my comments. Look forward to the updated version.
Best,
- Xufeng

On Tue, Jun 29, 2021 at 7:44 PM Don Fedyk <dfedyk@labn.net> wrote:

> Hi Xufeng
>
>
> Thanks for your detailed comments.
>
> Comments inline [Don]
>
> -----Original Message-----
> From: Xufeng Liu via Datatracker <noreply@ietf.org>
> Sent: Sunday, June 27, 2021 6:11 PM
> To: yang-doctors@ietf.org
> Cc: detnet@ietf.org; draft-ietf-detnet-yang.all@ietf.org
> 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.
>
[Xufeng]: Thanks for the clarification, which also solves some of my
confusion below. It would be beneficial to add some description to the
document.

>
> 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.
>
[Xufeng]: Right. Currently, even the interface is optional.

>
>     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.
>
[Xufeng]: A note would be appreciated. Your above clarification that the
model is per device also helps me here and for some questions below.

>
>     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).
>
[Xufeng]: Your opinion is understandable, but the consensus was documented
in the guidelines [RFC8407], and it is a checkpoint during IESG review.

>
> 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.
>
[Xufeng]: It could be raised to the tools' authors, but I'm not sure that
they would be able to achieve this level of sophistication.

>
> 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.
>
[Xufeng]: If a default statement is not appropriate, some clarification in
the description statement would be appreciated.

>
> 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.
>
[Xufeng]: If such a capability is not useful, you don't have to model it. A
feature statement would not help in specifying the run-time system
behavior. The system identifies a YANG feature when it loads a module.

>
> Thanks again.
> Don
>
> Thanks,
> - Xufeng
>
>
>
>