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

[Stig Venaas <[e-mail]>]

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


On Tue, Mar 7, 2017 at 2:15 PM, Stig Venaas <[e-mail]> wrote:
> 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