[MBONED] draft-ietf-mboned-multicast-yang-model-04: partial review

["Holland, Jake" <[e-mail]>]

Hi Sandy,

Before the review, I had one question about the link on
page 5 of your slides here:
https://datatracker.ietf.org/meeting/109/materials/slides-109-mboned-draft-ietf-mboned-multicast-yang-model-00#page=5

There's a link to here:
https://wiki.opendaylight.org/view/BIER:Main

But I get a 404 at that location.  I searched the wiki for bier,
but I only saw these 2, both of which looked like empty stubs:
https://wiki.opendaylight.org/display/ODL/BIER
https://wiki.opendaylight.org/display/ODL/BIER+App

Is there another link that describes how the BIER project is
using this model, or an example walkthrough for how to get
started or something?


Anyway, here's the draft review:

First, thanks for this update, the draft looks like there's a lot
of improvements since the last time I read it.  But I noticed a
number of issues.  Below I've divided it into "Technical" and "Nits".

This is not complete, I'm stopping without having done a detailed
review of sections 3.3 on, and at a brief overview, it looks like
the text in 3.3, 3.4, and 3.5 could use some editorial review for
clarity and grammar.  And I barely looked at the descriptions in
the model, or at section 5.  But I thought I'd send along what I've
got so far anyway, and maybe send a 2nd half later if I can get to it.

I hope it's helpful.


Technical:

1.
Section 1.5:
Fig 1 and 2nd paragraph:
- EMS/NMS needs a definition before its use (I hadn't seen these
  terms and had to web search for them)


2.
4th paragraph:
   In this model, the correspond key is set to 233.252.0.0/16, the
   transport technology is set to BIER.

I'm not sure I understand what this sentence is trying to say.  My
best guess is that it's referring to the key to the "multicast-keys"
list, is that right?  (And nit: I think it's "corresponding", right?)

But in the model I see this key requiring 5 values, so I'm not clear
on how the key can just be set to the group-address, don't all these
require a value?

      list multicast-keys{
        key "vpn-rd source-address group-address vni-type vni-value";

Anyway, the paragraph then goes on to describe a bunch of other
nodes in the model and the values they'd be set to.  I think I'd have
a better idea what this paragraph is talking about if it used the
names from the model to describe what the actual locations in the
tree get set, and what actual values they get set to.  And I think
that would be a lot clearer if it provided a json or xml example,
rather than a text paragraph.  (It also might work to use a table,
if the example would be too big.)


3.
Section 2.1:
   Depending on the implementation choices, some systems may
   not allow some of the advanced parameters to be configurable.

This sounds like it's describing things that should be controlled
with an "if-feature" statement, but I don't see any in the model.
(see: https://tools.ietf.org/html/rfc7950#section-7.20.2 )  Am I
misunderstanding this part, or why aren't these done as features?

Also, which parameters are these?  I think if they're associated
with features in the model, the model would make it clear, but
if it's just from description in the text it seems like this would
need a lot more spelling out about which features are optional and
under what circumstances.


4.
Section 2.2:
The notification includes the error reason and the
   associated data nodes.

I don't understand this sentence.  I think it's referring
to the "head-end-event" notification in the model?  Is this trying
to say something like "The head-end-event notification alerts
listeners to failures?

I also don't understand the head-end-event, it seems to have a
failure report option, which is I think what's describe here, but
also a module load and unload report, and I don't understand how
those get used?


5.
Section 3.1:
Figure 3: PIM is listed as an underlay option, but it's not in the
model as an underlay.
PIM also is in the model as an overlay technology, but is not listed
in the figure as an overlay.  EVPN also is missing from the overlay
technologies in the figure.


6.
Section 3.2:
I'm a little confused about why there's 0 or 1 ingress-node
entries in the ingress-egress.  I think it's possible to have
redundant ingress nodes, right?  And is it correct that it's
not mandatory to have at least 1?


7.
Section 3.2:
I noticed the definition and use of the ip-multicast-source-address
type while reading the tree diagram.  I guess I'm confused on 2
points:
a. it seems like this union should have been defined in rt-types,
   and it probably should be defined there instead of inside this
   model.
b. From the definitions of ipv4- and ipv6- multicast-source-address
   in rt-types, it looks like the purpose is to add "*" as an option
   in addition to the ip address, which is for designating (*,G)s I
   assume.  Does the type ambiguity for '*' cause a problem here?
   Would it be better to define it this way instead?

     typedef ip-multicast-source-address {
       type union {
         type enumeration {
           enum * {
             description
               "Any source address.";
           }
         }
         type inet:ipv4-address;
         type inet:ipv6-address;
       }
       description
         "Multicast source IP address type.";
     }

8.
In the model:
I don't understand the "module-loaded" and "module-unloaded" options
in the head-end-event notification.  It sounds like these are
referring to a change in the RESTCONF/NETCONF server, rather than a
change in the head-end?  Or I don't understand what this is saying,
or what a client might do in response to this.
          enum module-loaded {
            description
              "The new modules that can be used by multicast
               flows have been loaded.";
          }
          enum module-unloaded {
            description
              "The new modules that can be used by multicast
               flows have been unloaded.";
          }


9.
In the model:
I'm confused about the transport-pim grouping that's empty.  Is
the point of that to have something that can be augmented?  Is
there an example of augmentations that are used here?

This is an extension point?  If so, would it be reasonable to
add any guidance explaining when it would be useful or necessary
to use it?


10.
Same thing for the several empty cases in overlay-tech and
underlay-tech.

Would it make sense to define an identity like
"underlay-technology-identity" and use that as a base for a
type that would appear in the data instance?  The way it stands
now, I'm not sure I see how it would distinguish between an
underlay-tech that's bgp vs. isis, since both are empty, so the
underlay-tech container would just be empty in both cases.



Nits:
1.
Section 1.5, first paragraph: which -> that
OLD:
 the network resources which are used to deliver
NEW:
 the network resources that are used to deliver

(There are several resources online that cover this, but see for
example: https://www.grammarly.com/blog/which-vs-that/)


2.
Figure 1, the line doesn't match up on the "Multicast Model" line.
                  +------------------------+
                  |    Multicast Model   |
                  +------------------------+


3.
Section 1.5 2nd paragraph:
I don't think "Detailly" is a word.  I think you could replace the
whole first sentence though, and I'll suggest some text:
OLD:
   Detailly, in figure 1, there is an example of usage of this multicast
   model.  
NEW:
   Figure 1 illustrates example use cases for this multicast model.


4.
3rd paragraph section 1.5: Correspoding -> Corresponding
   Correspoding IGP protocol which is used to build BIER transport layer


5.
4th paragraph section 1.5: egde->edge
OLD:
  The model is sent to every egde router from the
NEW:
  The model is sent to every edge router from the


6. 4th paragraph 1.5: which->that
OLD:
   the BIER YANG model which is defined in [I-D.ietf-bier-bier-yang]
NEW:
   the BIER YANG model that is defined in [I-D.ietf-bier-bier-yang]


7. 5th paragraph 1.5: response->respond
OLD:
   the controller.  Then the controller/ EMS/NMS can response
NEW:
   the controller.  Then the controller/ EMS/NMS can respond


8. last paragraph 1.5: existed->existing:
OLD:
   model, instead, it depends on many existed multicast protocol models
NEW:
   model, instead, it depends on many existing multicast protocol models


9. There's 4 or 5 instances of the word "failer", including section 3.5
and several of the descriptions in the model.  I think these all should
be "failure", if I'm understanding correctly?