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

[[e-mail]]

Hi Jake, 


Very sorry for the late response. 


Though you have done the continued review for the draft in March, I think it may be better to respond to your review comments separately in order to avoid confusion.



Please find my comments with Sandy> inline.







原始邮件



发件人：Holland,Jake
收件人：MBONED WG;张征00007940;
日 期 ：2020年11月17日 19:27
主 题 ：draft-ietf-mboned-multicast-yang-model-04: partial review


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?
 Sandy> ODL has changed the project links, please find the project information through: https://wiki-archive.opendaylight.org/view/BIER:Main;


or find the coding information through: https://git.opendaylight.org/gerrit/gitweb?p=bier.git;a=tree;h=refs/heads/master;hb=refs/heads/master





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)
 Sandy> The EMS/NMS is the acronym of Element Management System/Network Management System; I'll add them in the draft.

 

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?)
 Sandy> Yes. I'll modify the nit.




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?
 Sandy> The keys should be set, but some of them can be set to zero. 

So when group address is mentioned here, it means that the other keys are set to zero. 

I'll add description for it. 




      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.)
 Sandy> Good idea. I'd like to add a table for it. And ask for your more review.


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.
 
Sandy> The optional leaves can be set or not, which depends on whether the controller supports the configuration or not. 


The if-feature is also a way to do it and it's more clear. I'll add some features for it.







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?
 Sandy> Yes. The description may need modification. 




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?
 Sandy> The mean is that some protocols may not be enabled or run in the device. 

So the model may not take effection. When the device notify the controller, the controller may get the information.


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.
 Sandy> Yes. Thank you very much for you point out of this. I'll add them 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?
 Sandy> Yes. You are right. I'll change the ingress node type to list. 
 
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.

Sandy> If there is the union, I would use it directly. But I didn't find the union for IPv4/IPv6 source address in rt-types definition, and the rt-types may not be modified (as it has been published as RFC8294), so I define a new union for it.

 

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.";
     }

Sandy> OK. I'll modify it.


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.";
          }
 Sandy> In some device, the protocol is also called a module. But I think you are right, the description may lead to confusion. 

Do you think it's better: protocol-enabled and protocol-disabled? 




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?
 Sandy> Because this model works together with other protocols model, PIM YANG can also be used. If there is some parameters need to be set, it should be set in PIM YANG model.




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?
 Sandy> I used boolean type for it, when it was set to 1 mean that this leaf is used. 

But when this model was review by YANG doctors, they said it's more better to use the current way.





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.
 Sandy> Please find my answer in 9. 
 
 Sandy> Thank you very much for your comments about the nits. I'll modify it in next version.

Best regards,

Sandy




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?