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

I reviewed the -12 version. Thanks for taking care of most of my review comments. I reviwed -15 version again and found the following issues, some of which were also noticed in my earlier review.  

## Major

- I find these phrasing problematic - *"The controller can distribute new model for the flows to the network nodes."*, *"The controller can send a model with the same multicast flow information and the associated transport protocol (set to PIM) to the ingress node."*, *"When the controller manages all the ingress and egress routers for the flow, the model is sent with flow characters, ingress and egress nodes information to the ingress and egress nodes."*, *"When the controller manages the ingress nodes only for the flow, the model is sent with the flow characters to the ingress nodes."*, "The model is sent to every edge router from the controller."*, *"The model can be sent to the ingress nodes only"*.. ; The text repeatedly refers to “sending” or “distributing” the model. A YANG model defines a schema. The intent appears to be that one configures data conforming to this model. Please update all instances. 

- Section 2.3 needs a text description. There is no description of what is upstream. What is downstream! You need to explain the model structure at a high level first. The tree and the text in 2.4 do not align well. For instance, an explanation of why the neighbour list is per combination of signalling and transport would be useful to know. The text in 2.4 does not work well without examples. It is difficult for the reader to understand the model without it. 

- How do you handle a non-MVPN case if RD is a mandatory key?

````
module: ietf-multicast
  +--rw multicast-service
     +--rw multicast-flow* [vpn-rd source-address group-address]
        +--rw vpn-rd            rt-types:route-distinguisher
        +--rw source-address    ip-multicast-source-address
        +--rw group-address     rt-types:ip-multicast-group-address
````

- Explain the design for notification, for service up/down or protocol enabled/disabled. Do I really need the full tree with all leaves? 

- Section 4 needs work. You need to identify explicitly which nodes are vulnerable and not the whole model itself as "multicast-service". 

## Minor

- The title of the draft is very generic; think of a more specific title. How about "A YANG Data Model for Multicast Services"?

- Section 2.1.1.1 could use JSON corresponding to the example. 

- Section 2.3, please add this text - "Tree diagrams used in this document follow the notation defined in [RFC8340]."

- Section 3, in contact in the YANG model, add "WG Web:   https://datatracker.ietf.org/wg/mboned/"

- The description in the YANG model incorrectly has below text; please change to RFC XXXX

````
     This version of this YANG module is part of RFC 9719
     (https://www.rfc-editor.org/info/rfc9719); see the RFC itself
     for full legal notices.
````

- What is the purpose of this identity? I don't see it being used.

````
  identity multicast-service {
    base rt:control-plane-protocol;
    description
      "Identity for the multicast model.";
  }
````

- Are these correct references? Can these signalling types be set without BIER? 

````
  identity signaling-pim {
    base dynamic-signaling-type;
    description
      "Using PIM as multicast service signaling technology.";
    reference
      "I-D.ietf-bier-pim-signaling: PIM Signaling Through BIER
        Core";
  }

  identity mld {
    base dynamic-signaling-type;
    description
      "Using MLD as multicast service signaling technology.";
    reference
      "I-D.ietf-bier-mld: BIER Ingress Multicast Flow Overlay
        using Multicast Listener Discovery Protocols";
  }
````

- Better to rename these to service-up instead of event-up. 

````
  identity event-type {
    description
      "The events of the multicast service.";
  }

  identity event-up {
    base event-type;
    description
      "The multicast service works.";
  }

  identity event-down {
    base event-type;
    description
      "There is something wrong with upstream or downstream node,
       and node can't work properlay.";
  }
````

- Remove grouping encap-type as you are not using it anymore! 

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

````

- Unclear use of TE node/link template here. What is the point of calling the leaf "static-tunnel-name"? Why not just tunnel? 

````
      case rsvp-te-p2mp {
        if-feature "p2mp-te";
        description
          "Using RSVP TE P2MP as the transport technology.";
        reference
          "RFC 4875: Extensions to Resource Reservation Protocol -
             Traffic Engineering (RSVP-TE) for Point-to-Multipoint
             TE Label Switched Paths (LSPs)
           RFC 8776: Common YANG Data Types for Traffic
             Engineering";
        leaf template-name {
          type te-types:te-template-name;
          description
            "A type for the name of a TE node template or TE link
             template.";
        }
        leaf static-tunnel-name {
          type string;
          description
            "Optional for statically specifying the P2MP TE tunnel
             name.";
        }
      }
````

- Empty containers such as bidir, sr-p2mp, native. Is the presence statement missing? Are they empty containers for future augmentation?

````
           |  +--:(pim) {pim}?
           |  |  +--rw source-address?
           |  |  |       ip-multicast-source-address
           |  |  +--rw group-address
           |  |  |       rt-types:ip-multicast-group-address
           |  |  +--rw bidir {bidir}?
           |  |  +--rw tad* [mt-id fa-number data-plane]
           |  |     +--rw mt-id         uint16
           |  |     +--rw fa-number     uint8
           |  |     +--rw data-plane    uint8
           |  +--:(ir-tunnel) {ir-tunnel}?
           |  |  +--rw ir-tunnel-type?       uint8
           |  +--:(sr-p2mp) {sr-p2mp}?
           |  +--:(native)
````

- Please describe the need for empty case statements for protocol-type. The description says "The dynamic signaling technologies and associated parameter that may be set"; but there are no associated parameters! 

````
        |     +--rw (protocol-type)?
        |        +--:(evpn)
        |        +--:(mld)
        |        +--:(mld-snooping)
        |        +--:(mvpn)
        |        +--:(pim)
````

- The description of vni-type (multiple instances) needs to be corrected as you have removed vni-value leaf now. If this were the only need for RFC2119 keywords, you could also update the YANG module description text for the boilerplate.

````
          leaf vni-type {
            type identityref {
              base tunnel-encap-type;
            }
            description
              "The encapsulated type for the multicast flow,
               it is used to carry the virtual network identifier
               for the multicast service.
               When this type is set, the associated vni-value
               MUST be set.";
          }
````

Thanks! 
Dhruv