Subject: [yang-doctors] Yangdoctors early review of draft-ietf-babel-yang-model-03

From: Radek Krejčí via Datatracker <[email-masked]>
Date: Mon, 14 Oct 2019 07:01:07 -0700

Reviewer: Radek Krejčí
Review result: Ready with Nits

There is a single module in the draft: [email-masked].

There are 2 warnings reported by pyang 2.0.2, but both warnings seem to be
false.

[email-masked]:53: warning: RFC 8407: 3.1: The IETF Trust Copyright
statement seems to be missing (see pyang --ietf-help for details).
[email-masked]:208: warning: the module seems to use RFC 2119
keywords, but the required text from RFC 8174 is not found (see pyang
--ietf-help for details).

The draft is well written with an informative overview of the YANG module
sections. Maybe some set of example configuration data (some Appendix) would be
useful.

Regarding the YANG module, I have just a few notes:

- In multiple description texts, nodes are referenced by a path in format with
dashes (e.g. babel-mac-default-apply). This format is confusing especially when
some element in the path contains dash(es). Why the common format with slashes
is not used?

- Description and reference statements in containers and lists are placed as
the last statement (after children nodes). To increase readability and allow
people to decide if the content of the container/list is interesting for
detailed view, please move the descriptions / references to the beginning.

- grouping routes
  - just think if the grouping is reusable - in the module itself, the grouping
  is used just once and needless creation of groupings decrease module
  readability

- routes/routes/received-metric
  -  ... indicate a the route ... -> indicate the route ...

- babel/interfaces/metric-algorithm
  - the text "The value MUST be one of those listed in
  'metric-comp-algorithm'." in description just duplicates the type's
  information (identityref with metric-comp-algorithms base).

- babel/interfaces/mac-key-sets
  - what does the babel-mac-key-sets in description refer to?

- babel/interfaces/stats/reset
  - I'm confused, is it per-interface reset (as placed in the module) or
  system-wide interfaces stats reset (as information model defines it?). If it
  is the system-wide reset, why tha action cannot be placed in babel container?

- babel/mac/test/output/resulting-hash
  - according to the content, it seems to me that the description is actually
  the missing description of the test action.

- babel/dtls/test/output
  - seems to me more as a description of the action itself (where the
  description is missing)

---

From: Martin Bjorklund <[email-masked]>
Date: Mon, 14 Oct 2019 16:06:42 +0200 (CEST)

> There are 2 warnings reported by pyang 2.0.2, but both warnings seem to be

Actually, I think they are correct.

> statement seems to be missing (see pyang --ietf-help for details).

Ok it isn't missing, but it doesn't use the eaxct words it should
use.

> --ietf-help for details).

This one seems legit as well.

/martin

---

From: Mahesh Jethanandani <[email-masked]>
Date: Mon, 14 Oct 2019 18:38:26 -0700

> On Oct 14, 2019, at 7:06 AM, Martin Bjorklund <[email-masked]> wrote:
> 

Yes, the words are not exact. I see two versions of the copyright statement. Do not know which one is the correct one.

In the model and output of ‘pyang —ietf-help’

     Copyright (c) 2019 IETF Trust and the persons identified as
     the document authors.  All rights reserved.

     Redistribution and use in source and binary forms, with or
     without modification, is permitted pursuant to, and subject
     to the license terms contained in, the Simplified BSD
     License set forth in Section 4.c of the IETF Trust's Legal
     Provisions Relating to IETF Documents
     (http://trustee.ietf.org/license-info).

In the draft:

   Copyright (c) 2019 IETF Trust and the persons identified as the
   document authors.  All rights reserved.

   This document is subject to BCP 78 <https://tools.ietf.org/html/bcp78> and the IETF Trust's Legal
   Provisions Relating to IETF Documents
   (https://trustee.ietf.org/license-info <https://trustee.ietf.org/license-info>) in effect on the date of
   publication of this document.  Please review these documents
   carefully, as they describe your rights and restrictions with respect
   to this document.  Code Components extracted from this document must
   include Simplified BSD License text as described in Section 4.e of
   the Trust Legal Provisions and are provided without warranty as
   described in the Simplified BSD License.

> This one seems legit as well.

Yes, it is missing the word “NOT RECOMMENDED”. I will add it.

Thanks.

> /martin

Mahesh Jethanandani
[email-masked]

---

From: Martin Bjorklund <[email-masked]>
Date: Tue, 15 Oct 2019 08:38:16 +0200 (CEST)

Mahesh Jethanandani <[email-masked]> wrote:
> Yes, the words are not exact. I see two versions of the copyright
> statement. Do not know which one is the correct one.
> 

pyang only checks the text in the YANG model.

But the text in the the model and the pyang --ietf-help are not the
same (they are similar but not the same).

The text that pyang enforces is the text found in the template in
Appendix B in RFC 8407 (it allows (and recommends) a link to the RFC
which is not present in the 8407 template).

The template text in 8407 is taken from paragraph 6.d in
https://trustee.ietf.org/license-info/IETF-TLP-5.htm.

>    (https://trustee.ietf.org/license-info
>    <https://trustee.ietf.org/license-info>) in effect on the date of

Note that pyang just checks the module, not the draft.  It asks you to
include the 8174 text in the module's description.  It is included in
all "recently" published modules (that use 8174 language).  See
e.g. RFC 8366.

This recommendation is not included in RFC 8407, and I don't remember
the exact origin (perhaps Kent?), but it came out of the discussions
with the RFC editor.

/martin

---

From: Mahesh Jethanandani <[email-masked]>
Date: Tue, 15 Oct 2019 15:43:32 -0700

Hi Radek,

Thank you first of all for your thorough review. Please see comments inline.

> On Oct 14, 2019, at 7:01 AM, Radek Krejčí via Datatracker <[email-masked]> wrote:
> 
> Reviewer: Radek Krejčí

I have addressed them as part of my discussion with Martin.

> sections. Maybe some set of example configuration data (some Appendix) would be

I am not sure of the ask. Could you elaborate?

> some element in the path contains dash(es). Why the common format with slashes

Ok. I can see that the text can be confusing. The text is from the information model (I-d.ietf-babel-information-model), where the name of the node just happens to follow the path in the YANG model. I can modify the text to use the node names in the YANG model itself.

> detailed view, please move the descriptions / references to the beginning.

Done.

>  is used just once and needless creation of groupings decrease module

It is a grouping so that it can be used with routing policy module.

>  -  ... indicate a the route ... -> indicate the route …

Fixed

>  information (identityref with metric-comp-algorithms base).

How about if it says “The value MUST be one of those listed in ‘metric-comp-algorithm’ typedef.”?

>  - what does the babel-mac-key-sets in description refer to?

It now says:

            "List of references to the mac entries that apply
             to this interface. When an interface instance is
             created, all mac instances with default-apply 'true'
             will be included in this list.”

>  is the system-wide reset, why tha action cannot be placed in babel container?

The action statement defines an operation connected to a specific container. What would it mean to have an action statement on the whole babel container? Reset everything?

Even though the information model calls for a system-wide reset of stats, the model defines it at a per-interface level. It can be called recursively for every interface instance to realize a system-wide reset of stats.

>  - seems to me more as a description of the action itself (where the

Fixed.

Thanks.

Mahesh Jethanandani
[email-masked]

---

From: Mahesh Jethanandani <[email-masked]>
Date: Tue, 15 Oct 2019 15:43:56 -0700

Hi Martin,

> On Oct 14, 2019, at 11:38 PM, Martin Bjorklund <[email-masked]> wrote:
> 

Ok. I literary had to copy the text from the output of —ietf-help to make it happy.

> all "recently" published modules (that use 8174 language).  See

Ok. I have added the text in the module also.

Cheers.

Mahesh Jethanandani
[email-masked]

---

From: Radek Krejčí <[email-masked]>
Date: Wed, 16 Oct 2019 12:51:57 +0200

Hi Mahesh,

> I am not sure of the ask. Could you elaborate?

ahh, sorry, my fault - I have missed the appendixes - that actually already contain exactly what I meant.

> Ok. I can see that the text can be confusing. The text is from the information model (I-d.ietf-babel-information-model), where the name of the node just happens to follow the path in the YANG model…

I would prefer it. For example the mentioned babel-mac-default-apply - if you're not familiar with the tree, you don't know if to search for babel/mac-default-apply, babel/mac/default-apply or babel/mac/default/apply node.

> It is a grouping so that it can be used with routing policy module.

ok

> How about if it says “The value MUST be one of those listed in ‘metric-comp-algorithm’ typedef.”?

I see no such typedef in the module (neither in the information model). This is probably again a confusion between information model and yang model. If it refers to information model, I don't think it is a good idea, but it should be at least explicitly stated. If it refers to the leaf's base identity, I don't think that it is necessary to duplicate this information (but I'm not strongly against it, it just make me believe that actually it is some additional restriction to the type itself), because there is already:

type identityref {
  base metric-comp-algorithms;
}

which also says that the value MUST be one of the identities based on metric-comp-algorithms. And technically, such identities can be added so the possible values do not need to be listed in some text/list/typedef. If you really want to limit the values to some static list, the type should be actually enumeration.

>              will be included in this list.”

perfect!

> The action statement defines an operation connected to a specific container. What would it mean to have an action statement on the whole babel container? Reset everything?

but that is what the information model says, right? System-wide reset of the stats.Or is the meaning different?

> Even though the information model calls for a system-wide reset of stats, the model defines it at a per-interface level. It can be called recursively for every interface instance to realize a syste…

that's what I miss in the action description. There is what the reset is supposed to do by information model (system-wide reset) and that in YANG the reset action has to be placed where the action needs to be performed. So according to the description, the reset is in a wrong container because to perform system-wide reset, it probably don't need to be performed in a specific interface, right? There is no word about intention to change what the reset is supposed to do (reset of stats for the specific interface).

Regards,
Radek

---

From: Mahesh Jethanandani <[email-masked]>
Date: Wed, 16 Oct 2019 11:30:35 -0700

[Trimming it down to open issues]

> which also says that the value MUST be one of the identities based on metric-comp-algorithms. And technically, such identities can be added so the possible values do not need to be listed in some t…

How about we say just that, i.e. “The value MUST be one of the identities based on metric-comp-algorithm”?

> that's what I miss in the action description. There is what the reset is supposed to do by information model (system-wide reset) and that in YANG the reset action has to be placed where the action…

Ok. How about this text?

              The information model [RFC ZZZZ] defines reset
               action as a system-wide reset of Babel statistics.
               In YANG the reset action is associated with
               container where the action is defined. In this case
               the action is associated with the interfaces
               container, so the action will reset statistics at an
               interface level. 

               Implementations that want to support a system-wide
               reset of Babel statistics need to call this action
               for every instance of the interface.

Thanks.

Mahesh Jethanandani
[email-masked]

---

From: Radek Krejčí <[email-masked]>
Date: Thu, 17 Oct 2019 10:06:43 +0200

> How about we say just that, i.e. “The value MUST be one of the identities based on metric-comp-algorithm”?

my point from the beginning is that it is actually duplicated information. The leaf specification can be read as:

leaf metric-algorithm {
  type:  “The value MUST be one of the identities based on metric-comp-algorithms”;
  it is mandatory;
  description:  “... The value MUST be one of the identities based on metric-comp-algorithms”;
}

So I don't think that it is necessary to have that sentence in the description text at all.

I've looked at the model again and I've realized, that there is actually babel/metric-comp-algorithms leaf-list. So despite the model defines set of identities, the server is actually allowed to implement just a subset of them, right? Maybe the intention of the description was to require the value to be one of the values of the leaf-list. Confusion is caused by naming - identity as well as the leaf-list have the same name and it is referenced in the text as metric-comp-algorithm. I see that you cannot use leafref from configuration to state data. So, I see 2 options. First, the sentence in the description should be:

"The value MUST be one of those listed by server as babel/metric-comp-algorithms leaf-list."

or, and I think this is the right solution, remove the babel/metric-comp-algorithms leaf-list and create features for each (there are 2) metric-comp-algorithms identity (add if-feature into them) so the server can say via YANG features mechanism which identities are actually implemented. Then the value of the metric-algorithm leaf will be limited to the metric-comp-algorithms identities with the feature enabled by the server.

>                for every instance of the interface.

Perfect, thanks!

Radek

---

From: Mahesh Jethanandani <[email-masked]>
Date: Thu, 17 Oct 2019 15:33:24 -0700

[Trimming the list further down]

> On Oct 17, 2019, at 1:06 AM, Radek Krejčí <[email-masked]> wrote:
> 

I am ok with the second option. 

Except I do not know why pyang complains about this.

  identity two-out-of-three {
    if-feature two-out-of-three-supported;
    base "metric-comp-algorithms";
    description
      "2-out-of-3 algorithm.";
  }

  identity etx {
    if-feature etx-supported;
    base "metric-comp-algorithms";
    description
      "Expected Transmission Count.";
  }

yang/[email-masked]:88: error: keyword "base" not in canonical order (see RFC 6020, Section 12)
yang/[email-masked]:95: error: keyword "base" not in canonical order (see RFC 6020, Section 12)

Where the ABNF grammar says:

   identity-stmt       = identity-keyword sep identifier-arg-str optsep
                         (";" /
                          "{" stmtsep
                              ;; these stmts can appear in any order
                              *if-feature-stmt
                              *base-stmt
                              [status-stmt]
                              [description-stmt]
                              [reference-stmt]
                          "}") stmtsep

Mahesh Jethanandani
[email-masked]

---

From: Martin Bjorklund <[email-masked]>
Date: Fri, 18 Oct 2019 08:50:54 +0200 (CEST)

>                               ;; these stmts can appear in any order

You are right, this is a bug in pyang, now fixed.

/martin