Early Review of draft-ietf-opsawg-yang-vpn-service-pm-08
review-ietf-opsawg-yang-vpn-service-pm-08-rtgdir-early-dhody-2022-05-23-00

Hello

I have been selected to do a routing directorate “early” review of this
draft.

The routing directorate will, on request from the working group chair,
perform an “early” review of a draft before it is submitted for publication
to the IESG. The early review can be performed at any time during the
draft’s lifetime as a working group document. The purpose of the early
review depends on the stage that the document has reached.

As this document is post-working group last call, my focus for the review
was to determine whether the document is ready to be published. Please
consider my comments along with the other last call comments.

For more information about the Routing Directorate, please see
http://trac.tools.ietf.org/area/rtg/trac/wiki/RtgDir

Document: draft-ietf-opsawg-yang-vpn-service-pm
Reviewer: Dhruv Dhody
Review Date: 2022-05-12
IETF LC End Date: Unknown
Intended Status: Standards Track


*Summary:*I have some minor concerns about this document that I think
should be resolved before publication.


*Comments:*The document is easy to read. The YANG model seems to be useful
for VPN service assurance and is well-formatted.

*Major Issues:*
None


*Minor Issues:*
Earlier vpn-pm-type was a choice, it was changed in the -08 version and now
both inter-vpn-access-interface and underlay-tunnel can be configured
together. In the draft text, it states that "usually only one of the two
methods is used". But the YANG allows both of them to be configured at the
same time.

       +--rw vpn-pm-type
          +--rw inter-vpn-access-interface
          |  +--rw inter-vpn-access-interface?   empty
          +--rw underlay-tunnel!
             +--ro vpn-underlay-transport-type?   identityref

It is not clear to me how to interpret the measured PM delay value? Is it
the VPN access interface or for the underlay tunnel? Maybe you should allow
only one of them to be configured at a time?

*Query:*
The vpn-underlay-transport-type in underlay-tunnel identifies the type of
underlay tunnel between the PEs. But isn't it possible to have multiple
types of tunnels and even load balancing between them? Is the assumption
that each instance of underlay tunnel would have its own link?

*Nits:*
- Expand on first use: LMAP
- Section 1: s/to display the//
- Section 3.1: s/VPN service assurance model/VPN Service Performance
Monitoring/
- Section 4.1: s/and node 3 (N3)5/and node 3 (N3)/
- Section 4.2: Is this text correct - "For network performance monitoring,
the container of "networks" in [RFC8345
<https://www.ietf.org/archive/id/draft-ietf-opsawg-yang-vpn-service-pm-08.html#RFC8345>]
does
not need to be extended."? Or is "not" there by mistake?

*YANG Model:*
1)
  identity pe {
    base node-type;
    description
      "Provider Edge (PE) node type. A PE is the name of the device
       or set of devices at the edge of the provider network with the
       functionality that is needed to interface with the customer.";
  }

What do you mean by "the name of the device"?

2) In the grouping entry-summary, the description says it for "VPN or
network" but for all the leaves (maximum-routes, total-active-routes,
mac-num etc) it says VPN only! Please update to "VPN or network".

3)
      leaf packet-loss-count {
        type yang:counter64;
        description
          "Total received packet drops count.";
      }
Not sure about the description, it reads as if a packet is received but
dropped, and I don't think that is what you mean?

4)
    leaf reference-time {
      type yang:date-and-time;
      config false;
      description
        "Indicates the time when the statistics are collected.";
    }
I am not sure what collected means here. Do you mean when the current
counters were last updated? Or when it was last aggregated/filtered at the
controller?

5)
    leaf inbound-nunicast {
      type yang:counter64;
      description
        "The total number of inbound non-unicast
         (i.e., broadcast or multicast) packets.";
    }

suggest changing the name to inbound-non-unicast (check other instances as
well)

6)

leaf network-access-id {
           type vpn-common:vpn-id;
           description
             "References to an identifier for the VPN network
              access, e.g. L3VPN or VPLS.";
         }

The description of network-access-id is not clear. Is it name of VPN?
or the site-network-access-id in LxSM?


Thanks!
Dhruv