Early Review of draft-ietf-pim-igmp-mld-yang-02
review-ietf-pim-igmp-mld-yang-02-yangdoctors-early-lindblad-2017-03-06-00

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