# YANGDOCTORS LC Review of draft-ietf-mboned-multicast-yang-model-12

An LC review is requested but I see the document is not under WG or IETF LC. IMHO the I-D is not yet ready for LC and more work is needed. I have noted various issues, this is not a complete list and I can do another pass when the authors have made the update. 

## YANG Model

- Please update the file name with the latest date. It does not match the revision date or the date in the copyright. 

- Use a shorter prefix - "ietf-multicast-model"; it is unusual to have 'model' in the name too.  

- Please add a better description for the YANG model in the YANG file

- Please use the correct boilerplate as per https://trustee.ietf.org/documents/trust-legal-provisions/tlp-5/ (s/Simplified BSD/Revised BSD/)

- Should there be a feature statement for SR as well? 

- For 'typedef virtual-type' using enumeration limits the possibility of adding new encapsulations in future - did you consider using identity instead? And why is it called virtual-type where as the description says encapsulation type? Further, this text - "When this type is set, the associated vni-value MUST be set" should be in the description of vni-type and not at the typedef. It could also follow a must yang statement to check for this case.  

- Various identities use BIER as an example and list BIER I-Ds in the reference statement.  Also, BIER is an optional feature. Are there better references or more examples?

- You have the same description for mld and mld-snooping identity. Please check.

- Should cisco-mdt be part of an IETF standard model or it could be simply augmented in a vendor augmentation of this model? Moreover RFC 6037 is marked historic.

- For pim identity has two base statements, but the description mentions transport only.
````
  identity pim {
    base transport-type;
    base underlay-type;
    description
      "Using PIM as multicast transport technology.";
    reference
      "RFC 7761:
         Protocol Independent Multicast - Sparse Mode
         (PIM-SM): Protocol Specification (Revised).";
  }
````

- In the grouping general-multicast-key, are all leaves set at all times? Since this is a key and thus mandatory, what happens when a multicast service does not have a vni-value for example?

- For source-address, the description says zero means any source whereas typedef highlights '*', I am guessing both are in use? 
````
    leaf source-address {
      type ip-multicast-source-address;
      description
        "The IPv4/IPv6 source address of the multicast flow. The
         value set to zero means that the receiver interests
         in all source that relevant to one given group.";
    }
    
  where  
    
  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.";
  }    
````

- Can you reuse the bier-encapsulation identity from draft-ietf-bier-bier-yang, instead of defining a new grouping for it? 
````
  grouping encap-type {
    description
      "The encapsulation type used for flow forwarding.
       This encapsulation acts as the inner encapsulation,
       as compare to the outer multicast-transport encapsulation.";
    choice encap-type {
      case mpls {
        description "The BIER forwarding depends on mpls.";
        reference
          "RFC 8296: Encapsulation for Bit Index Explicit
           Replication (BIER) in MPLS and Non-MPLS Networks.";
      }
      case eth {
        description "The BIER forwarding depends on ethernet.";
        reference
          "RFC 8296: Encapsulation for Bit Index Explicit
           Replication (BIER) in MPLS and Non-MPLS Networks.";
      }
      case ipv6 {
        description "The BIER forwarding depends on IPv6.";
        reference
          "I-D.ietf-bier-bierin6: BIER in IPv6 (BIERin6)";
      }
      description "The encapsulation type in BIER.";
    }
  } // encap-type
````

- Also if the BIER-related grouping more suitable for BIER I-D? I see some common co-authors, so a change like this can be easily coordinated. 

- For transport-tech, the description says 'it's better to use only one' type, indicating that more types are possible. From a modelling perspective, you have a single leaf for type, thus only one can be supported! Which is it?
````
  grouping transport-tech {
    description
      "The transport technology selected for the multicast service.
       For one specific multicast flow, it's better to use only one
       transport technology for forwarding.";
       
    leaf type {
      type identityref {
        base transport-type;
      }
      description "The type of transport technology";
    }
````

- Also isn't a choice/case a better way to model this instead of a set of containers with a when statement for each choice (applicable to other groupings as well)? 

- For leaf template-name, use type te-types:te-template-name from RFC 8776 

- I am unsure how a TE node/link template would be used when transport is P2MP-RSVP-TE? This needs a better description and examples.

- The container sr-p2mp, what are segment list names used for ingress replication? I would have assumed you would want to refer to SR P2MP Policy (and thus Root, Tree-ID) but you only mention Tree-SID. This needs better description, references, and examples. I am unsure how I would use it. 

- In grouping underlay-tech, I have no idea what the topology leaf is all about. 
````
      leaf topology {
        type string;
        description
          "The designed topology name of ospf protocol.";
      }
````

- There are a lot more overlay identities defined (EVPN,MVPN,MLD-snooping,overlay-pim) but the grouping overlay-tech only cares about MLD? Are no leaves required for other overlay types? 

- Please improve this description - The terms like partial SDN are unclear and I am unsure about the point of that sentence.
````
      container multicast-overlay {
        description
          "The overlay information of multicast service.
           Overlay technology is used to exchange multicast
           flows information. Overlay technology may not be
           used in SDN controlled completely situation, but
           it can be used in partial SDN controlled situation
           or non-SDN controlled situation. Different overlay
           technologies can be chosen according to different
           deploy consideration.";
````

- For list ingress-nodes and egress-nodes, consider using leaf-list instead of a list with a single leaf, unless there is a specific reason for that design choice. Also, the description seems incorrect as the description of ingress-node says egress and vice-versa! 

- For the notifications, should there be an up event, so that you can notify when something that is down is up now? Further instead of enum, use identity to allow adding new events in future. It also made me wonder if there should be a state leaf in the model to know the current status of the multicast service.

- In some cases for references, you use references to the YANG RFCs instead of references to RFCs where the technology was defined. I found that unusual.  

- Please run "pyang --ietf --lint -f yang --keep-comments --yang-line-length 69" to fix the formatting in the YANG file. There is also the error because of date mismatch. 

- This model is supposed to invoke other models, but I don't see any clear mechanism listed to do that. There is no leafref, and no instructions on mapping. Is the information in this model enough to invoke and configure BIER/PIM/RSVP-TE? What am I missing? 

- When some elements are not set via configuration, say (ingress-node, egress-nodes) are not set and this information is collected via MLD/MVPN. Does the model allow one to get this MLD/MVPN collected data via this model? 
  
## Draft Review

- The title of the draft is very generic, think of a more specific title. 

- The introduction section should provide information on what it means to be Multicast Overlay/Transport/Underlay for the readers. Some text from section 3.3 can be pulled up.

- Section 1.4, the table does not match what you are using inside the YANG model

- Section 1.5, the figure uses the same notation for "Multicast Model" as network elements and controller. In reference to RFC 8309 where does this model fit? What does "distribute new model" mean in "Then the controller/ EMS/NMS can respond immediately due to the failure and distribute new model for the flows to the network nodes quickly." 

- Section 1.5, below text goes against keys in YANG, all leaves marked as key are mandatory! 
````
   Among the multicast-keys, the group-address of L3 multicast flow and
   the mac-address of BUM flow are the most important keys.  The other
   keys are optional, and need not be all set. 
````

- Section 2.1, I am not sure the model fully aligns with this. There is only one feature for BIER in the YANG model. Is that the only advanced feature? 

- Section 2.2, in YANG presence has a specific meaning. I don't think this text aligns with the YANG model.

- Section 3, "This model imports and augments the ietf-routing", there is no augment in the YANG model. 

- Section 3.1, Is it UML or not? What does UML-like mean? Better to not label it and just say it is a figure to describe the model design.  

- Section 3.4, please rephrase and mention NMDA

- Section 5, update as per https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-22.html#section-3.7.1

- Section 6, update as per https://www.ietf.org/archive/id/draft-ietf-netmod-rfc8407bis-22.html#section-3.8.3.1 

- Please add more examples in the appendix that cover various different options to better explain how the model is used.

## Nits

- "The IPv4/IPv6 source address" can just be "The IP source address" (and same for group address).

- s/Vxlan/VXLAN/

- s/ospf/OSPF/

- s/isis/IS-IS/

- This comment /*underlay*/ is misplaced in YANG model

- s/node failer/node failure/

- Expand abbreviations on first use (PIM, MLD, BIER, VNI etc)

- Phrases such as "it sends the model" and "The model is sent" are incorrect.

Thanks! 
Dhruv