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

[Guofeng <[e-mail]>]

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; [e-mail]; 
Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02
Time: 2017-03-06 21:51:39

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:

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


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


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



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

- 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.
//Answer: Yes, we can add a default value for igmp and mld parameters. 

- 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.
//Answer: Yes, I think we can fix that, this comment is reasonable.

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


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

. 




-----Original Message-----
From: Stig Venaas [mailto:[e-mail]] 
Sent: Wednesday, March 08, 2017 6:16 AM
To: Guofeng <[e-mail]>
Cc: Benoit Claise <[e-mail]>; Jan Lindblad (jlindbla) <[e-mail]>; Stig Venaas <[e-mail]>; YANG Doctors <[e-mail]>; Mehmet Ersue <[e-mail]>; [e-mail]; Xufeng Liu <[e-mail]>; Xufeng Liu <[e-mail]>; [e-mail]
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

We can then get a WGLC started.

Stig


On Tue, Mar 7, 2017 at 4:37 AM, Guofeng <[e-mail]> wrote:
> Hi Jan and Benoit,
>
> Thanks for your comments. we/authors will discuss and answer the following comments.
>
> Thanks again,
> Feng
>
> From: Benoit Claise [mailto:[e-mail]]
> Sent: Monday, March 06, 2017 9:51 PM
> To: Jan Lindblad (jlindbla) <[e-mail]>; Stig Venaas 
> <[e-mail]>
> Cc: YANG Doctors <[e-mail]>; Mehmet Ersue 
> <[e-mail]>; [e-mail]
> Subject: Re: YANG doctor review of draft-ietf-pim-igmp-mld-yang-02
>
> 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:
>
> - 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