Subject: [yang-doctors] YANG doctor review of draft-ietf-pim-igmp-mld-yang-02

From: "Jan Lindblad (jlindbla)" <[email-masked]>
Date: Fri, 3 Mar 2017 22:44:35 +0000

I am the assigned YANG doctor for the document in the subject. My primary focus has been the YANG model itself, and I'm far from an expert at IGMP or MLD.

Here are my comments:

- In section 2.2, it is stated that:

   For the same reason, wide constant ranges (for example, timer
   maximum and minimum) will be used in the model.  It is expected that
   vendors will augment the model with any specific restrictions that
   might be required.

The above statement is false, YANG augmentations can never restrict a base model. If vendor restrictions of ranges etc are needed, YANG deviations could be used. Each such deviation significantly reduces the interoperability and thus value of this model, however, so for places where variation is expected even before hand, I would much rather prefer that this variability is built right into the model. One possible way of doing that for leafs with possible variations in range is described further down.

- In section 2.3, it is stated:

   The current draft contains IGMP and MLD as separate schema branches
   in the structure. The reason for this is to make it easier for
   implementations which may optionally choose to support specific
   address families. And the names of objects may be different between
   the ipv4 (IGMP) and ipv6 (MLD) address families.

The current YANG file organization causes a lot of repetition, which is tiring for model author and reader alike. A testament of this is that pretty much all mistakes in the model occur twice, i.e. a clear sign of copy-paste modeling. The cited paragraph also mentions the possibility for a device to implement only IGMP or only MLD. The current YANG model do not make either of them optional by e.g. an if-feature statement. Another possibility that comes to mind would be to separate the IGMP and MLD documents into separate modules. Implementations could then choose which one(s) to implement.

- Section 3.1 specifies a global level, interface-global level and an interface-specific level of shadowing attributes for an interface. Can't say I understand the difference between the global and interface-global level really. Also when it comes to the YANG manifestation of this, the grouping interfaces-config-attributes overlaps quite a bit with grouping interfaces-config-attributes-igmp-mld. Why are both these levels needed, and if they are, why don't they use the same grouping? Supposing there's a reason, why is much of the content duplicated in two groupings?

- Section 3.1 also states:

   Where fields are not genuinely essential to protocol operation, they
   are marked as optional. Some fields will be essential but have a
   default specified, so that they need not be configured explicitly.
   The module structure also applies, where applicable, to the
   operational state and notifications as well.

In fact, no leafs are marked mandatory anywhere in the model. Apart from keys, this means they are all optional. There are two sides to examine around this:
+ Configuration leafs: There is no description of what happens if a leaf isn't set. E.g if the igmp/global/enable leaf is true or false, I know how to intepret that. But what does it mean if it doesn't have a value? This is a general concern for more or less every leaf.
+ Operational leafs: Since no leafs are mandatory, there is no requirement to include any particular leaf in a reply. This will make it hard for implementations reading the igmp/mld status, since they will have to be coded to cope with any and every leaf not being present in the query result. It would be good if some core leafs were defined as mandatory, so that applications could count on them being present.

- At the end, section 3.1 says:

   The IGMP and MLD model augments "/rt:routing/rt:control-plane-
   protocols" as opposed to augmenting "/rt:routing/rt:control-plane-
   protocols/ rt:control-plane-protocol" as the latter would allow
   multiple protocol instances per VRF, which does not make sense for
   IGMP and MLD.

It would certainly be possible to locate the igmp and mld containers under /routing/control-plane-protocols/control-plane-protocol and still keep the requirement with a single instance of these containers, if desired. I'm not opposed to the current design, but I'd just like to point out that the statement above is not necessarily true.

Then on a more detailed level, I'd like to see (line numbers according to rfcstrip):

- Leaf last-member-query-interval on line 315, 447: Add a units statement

- Leaf robustness-variable on line 353, 491: What do these values mean?

- Leaf group-policy on line 393, 431, 544: Is any string value ok, or what would be a valid value?

- Leaf dr on line 577: Maybe a slightly more verbose name would be easier for operators?

- Leaf interface on line 780, 820: Maybe "name" would be a better name?

- Line 825, 917: This is an ipv6 leaf, but the description talks about IPv4.

- Leaf group on line 407, 558, 951, 985: Is any ipv4/6 address ok, or only a multicast address?

- Many description statements are missing or blank. I'm sure the authors are aware, but for completeness I include this comment here

- There are no notifications in the model. Aren't there any relevant events to be notified about?

Finally, an example of how a leaf with potentially different ranges could be handled in a reasonably interoperable way. So instead of this:

module std-inflexible {
  ..
  leaf some-property {
    range "1..1000"; // Large range to be sure everyone's use case is covered
  }
}

We could model this variability like this:

module std-flexible {
  ..
  choice property-variants {
    leaf some-property-basic {
      range "1..100"; // All implementations must be able to handle this
    }
    leaf some-property-extended {
      if-feature some-property-extended;
      range "1..200"; // Many implementations can do this
    }
  }
}

module vendor-x {
  ..
  augment /std-flexible:property-variants {
    leaf some-propery-vendor-x {
      range "10..700";
      must "current() mod 10 = 0"; // In steps of 10 using this variant
    }
  }
}

The application can now choose to use some-property-basic (any device), some-propery-extended (only on devices that announce feature some-property-extended) or some-property-vendor-x (only on devices that announe the vendor-x namespace).

/jan

---

From: Benoit Claise <[email-masked]>
Date: Mon, 6 Mar 2017 14:51:21 +0100

Thanks Jan,

Including the draft authors, chairs, and ADs.

Regards, Benoit
> I am the assigned YANG doctor for the document in the subject. My 
> primary focus has been the YANG model itself, and I'm far from an 
> expert at IGMP or MLD.
>
> Here are my comments:
>
> The above statement is false, YANG augmentations can never restrict a 
> base model. If vendor restrictions of ranges etc are needed, YANG 
> deviations could be used. Each such deviation significantly reduces 
> the interoperability and thus value of this model, however, so for 
> places where variation is expected even before hand, I would much 
> rather prefer that this variability is built right into the model. One 
> possible way of doing that for leafs with possible variations in range 
> is described further down.
>
> The current YANG file organization causes a lot of repetition, which 
> is tiring for model author and reader alike. A testament of this is 
> that pretty much all mistakes in the model occur twice, i.e. a clear 
> sign of copy-paste modeling. The cited paragraph also mentions the 
> possibility for a device to implement only IGMP or only MLD. The 
> current YANG model do not make either of them optional by e.g. an 
> if-feature statement. Another possibility that comes to mind would be 
> to separate the IGMP and MLD documents into separate modules. 
> Implementations could then choose which one(s) to implement.
>
> - Section 3.1 specifies a global level, interface-global level and an 
> interface-specific level of shadowing attributes for an interface. 
> Can't say I understand the difference between the global and 
> interface-global level really. Also when it comes to the YANG 
> manifestation of this, the grouping interfaces-config-attributes 
> overlaps quite a bit with grouping 
> interfaces-config-attributes-igmp-mld. Why are both these levels 
> needed, and if they are, why don't they use the same grouping? 
> Supposing there's a reason, why is much of the content duplicated in 
> two groupings?
>
> In fact, no leafs are marked mandatory anywhere in the model. Apart 
> from keys, this means they are all optional. There are two sides to 
> examine around this:
> + Configuration leafs: There is no description of what happens if a 
> leaf isn't set. E.g if the igmp/global/enable leaf is true or false, I 
> know how to intepret that. But what does it mean if it doesn't have a 
> value? This is a general concern for more or less every leaf.
> + Operational leafs: Since no leafs are mandatory, there is no 
> requirement to include any particular leaf in a reply. This will make 
> it hard for implementations reading the igmp/mld status, since they 
> will have to be coded to cope with any and every leaf not being 
> present in the query result. It would be good if some core leafs were 
> defined as mandatory, so that applications could count on them being 
> present.
>
> It would certainly be possible to locate the igmp and mld containers 
> under /routing/control-plane-protocols/control-plane-protocol and 
> still keep the requirement with a single instance of these containers, 
> if desired. I'm not opposed to the current design, but I'd just like 
> to point out that the statement above is not necessarily true.
>
> - Many description statements are missing or blank. I'm sure the 
> authors are aware, but for completeness I include this comment here
>
> Finally, an example of how a leaf with potentially different ranges 
> could be handled in a reasonably interoperable way. So instead of this:
>
> module std-inflexible {
>   ..
>   leaf some-property {
> The application can now choose to use some-property-basic (any 
> device), some-propery-extended (only on devices that announce feature 
> some-property-extended) or some-property-vendor-x (only on devices 
> that announe the vendor-x namespace).
>
> /jan
>
> .

---

From: Guofeng <[email-masked]>
Date: Tue, 7 Mar 2017 12:37:28 +0000

Hi Jan and Benoit,

Thanks for your comments. we/authors will discuss and answer the following comments.

Thanks again,
Feng

From: Benoit Claise [mailto:[email-masked]]
Sent: Monday, March 06, 2017 9:51 PM
To: Jan Lindblad (jlindbla) <[email-masked]>; Stig Venaas <[email-masked]>
Cc: YANG Doctors <[email-masked]>; Mehmet Ersue <[email-masked]>; [email-masked]
Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02

Thanks Jan,

Regards, Benoit

Here are my comments:

   might be required.

   IGMP and MLD.

module std-inflexible {
  ..
  leaf some-property {

module std-flexible {
  ..

module vendor-x {
  ..

/jan

.

---

From: Stig Venaas <[email-masked]>
Date: Tue, 7 Mar 2017 14:15:45 -0800

Hi

Great if you can post a new version by the cutoff on Monday, or more precisely
2017-03-13 (Monday): Internet Draft submission cut-off (for all
drafts, including -00) by UTC 23:59

We can then get a WGLC started.

Stig

On Tue, Mar 7, 2017 at 4:37 AM, Guofeng <[email-masked]> wrote:
> - Line 825, 917: This is an ipv6 leaf, but the description talks about IPv4

---

From: Stig Venaas <[email-masked]>
Date: Wed, 8 Mar 2017 12:03:30 -0800

Hi

Please also try to address the nits at
https://www.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-pim-igmp-mld-yang-02.txt

Stig

---

From: Guofeng <[email-masked]>
Date: Mon, 13 Mar 2017 12:37:03 +0000

Hi,

We modified the problem in the following URL and will submit out the updated draft today.
https://www.ietf.org/tools/idnits?url=https://www.ietf.org/archive/id/draft-ietf-pim-igmp-mld-yang-02.txt

Thanks,
Feng

-----Original Message-----
From: Stig Venaas [mailto:[email-masked]] 
Sent: Thursday, March 09, 2017 4:04 AM
To: Guofeng <[email-masked]>
Cc: Benoit Claise <[email-masked]>; Jan Lindblad (jlindbla) <[email-masked]>; Stig Venaas <[email-masked]>; YANG Doctors <[email-masked]>; Mehmet Ersue <[email-masked]>; [email-masked]; Xufeng Liu <[email-masked]>; Xufeng Liu <[email-masked]>; [email-masked]
Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02

Hi

Stig

On Tue, Mar 7, 2017 at 2:15 PM, Stig Venaas <[email-masked]> wrote:
>> Cc: YANG Doctors <[email-masked]>; Mehmet Ersue 
>> <[email-masked]>; [email-masked]

---

From: Guofeng <[email-masked]>
Date: Mon, 13 Mar 2017 12:44:17 +0000

Hi,

Please find below the answers to Jan’s comments(stating with “//Answer”).
We have updated the draft accordingly and will submit by today, thanks a lot

Feng

From: Benoit Claise
To: Jan Lindblad (jlindbla); Stig Venaas; 
Cc: YANG Doctors; Mehmet Ersue; [email-masked]; 
Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02
Time: 2017-03-06 21:51:39

Thanks Jan,

Regards, Benoit

Here are my comments:

//Answer: yes, augmentation cannot restrict, we will revise the model according to the following example.

module std-flexible {
  ..

module vendor-x {
  ..

// Answer: yes, we will choose separate modules for IGMP and MLD.

- Section 3.1 specifies a global level, interface-global level and an interface-specific level of shadowing attributes for an interface. Can't say I understand the difference between the global and interface-global level really. Also when it comes to the YANG manifestation of this, the grouping interfaces-config-attributes overlaps quite a bit with grouping interfaces-config-attributes-igmp-mld. Why are both these levels needed, and if they are, why don't they use the same grouping? Supposing there's a reason, why is much of the content duplicated in two groupings?
// Answer/Clarification: global level and interface-global level covers different scopes. Global level attributes consider status of the whole instance, not  interface. For example: global level max-entries counts entries of the whole instance.
   
// Answer/Clarification: interface-global covers interface scope, applied to an interface if there is no per interface configuration on the interface. For example:
Interface-global level: we have configured query interval with 200s and we have interface E1,E2 and E3
If on E1 we also have configured query interval with 100s, the actually on E1 IGMP uses query interval with 100s, and E2 and E3 both use query interval with 200s

// Answer/Clarification:  There are some differences between  interfaces-config-attributes and interfaces-config-attributes-igmp-mld , for example:
group-policy, verify-source-subnet, immediate-leave etc. only in interfaces-config-attributes-igmp-mld for per interface configuration

//Answer: Yes, we can add a default value for igmp and mld parameters. 

//Answer: Yes, I think we can fix that, this comment is reasonable.

- Leaf last-member-query-interval on line 315, 447: Add a units statement  //Answer: Yes, , we’ll add it.

- Leaf robustness-variable on line 353, 491: What do these values mean? // Answer/Clarification It is used to adapt packet loss in a network. If the network is not stable , we can increase the robustness-variable value.

- Leaf group-policy on line 393, 431, 544: Is any string value ok, or what would be a valid value? // Answer/Clarification This is a policy name, and we’ll add the description for the string value. 

- Leaf dr on line 577: Maybe a slightly more verbose name would be easier for operators? // Answer/Clarification: DR is a standard concept in PIM RFC.

- Leaf interface on line 780, 820: Maybe "name" would be a better name?  // Answer: Yes, we’ll fix that.

- Line 825, 917: This is an ipv6 leaf, but the description talks about IPv4. // Answer: Yes, we’ll fix that.

- Leaf group on line 407, 558, 951, 985: Is any ipv4/6 address ok, or only a multicast address? // Answer/Clarification only for a multicast address, we can just use the type for ipv4 or ipv6 address here and add the description

- Many description statements are missing or blank. I'm sure the authors are aware, but for completeness I include this comment here //Answer: I think we can add description if necessarily.

- There are no notifications in the model. Aren't there any relevant events to be notified about? // Answer/Clarification: This topic has been discussed in multicast yang design team. We have achieved the agreement that currently there is no requirements of IGMP or MLD notifications.

module std-inflexible {
  ..
  leaf some-property {

module std-flexible {
  ..

module vendor-x {
  ..

/jan

. 

-----Original Message-----
From: Stig Venaas [mailto:[email-masked]] 
Sent: Wednesday, March 08, 2017 6:16 AM
To: Guofeng <[email-masked]>
Cc: Benoit Claise <[email-masked]>; Jan Lindblad (jlindbla) <[email-masked]>; Stig Venaas <[email-masked]>; YANG Doctors <[email-masked]>; Mehmet Ersue <[email-masked]>; [email-masked]; Xufeng Liu <[email-masked]>; Xufeng Liu <[email-masked]>; [email-masked]
Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02

Hi

Great if you can post a new version by the cutoff on Monday, or more precisely
2017-03-13 (Monday): Internet Draft submission cut-off (for all drafts, including -00) by UTC 23:59

Stig

---

From: Benoit Claise <[email-masked]>
Date: Mon, 13 Mar 2017 15:24:18 +0100

Hi Feng,

Still a warning with yanglint.
http://www.claise.be/IETFYANGPageCompilation.html

Regards, Benoit

---

From: Radek Krejčí <[email-masked]>
Date: Mon, 13 Mar 2017 15:57:46 +0100

Hi,
this is the issue with the imported vs implemented modules. ietf-ip is only imported and not augmented, deviated nor referenced in leafref's path. With the yanglint's -i option there is no warning (but as mentioned in other thread, it would make failing other ietf modules, so, Benoit, do not use it for all the modules in your scripts). YANG does not provide mechanism to this module to state, that ietf-ip is expected to be implemented module, not only the imported one.

Radek

Dne 13.3.2017 v 15:24 Benoit Claise napsal(a):
> _______________________________________________
> yang-doctors mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/yang-doctors

-- 
Radek Krejci
mobile  : +420 732 212 714
office  : +420 234 680 256
e-mail  : [email-masked]
LinkedIn: http://www.linkedin.com/in/radekkrejci

CESNET, Association of Legal Entities
Zikova 4
160 00 Praha 6
Czech Republic