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

[Benoit Claise <[e-mail]>]

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