[OPSAWG] A review of draft-ietf-opsawg-yang-vpn-service-pm-01

[Adrian Farrel <[e-mail]>]

Hi authors,

This draft has been safely inside the OPSAWG for a while, so I though
it was probably due a review.

"The usual" two top-level issues:

- The draft expired earlier this month, so you need at a least a 
  keep-alive version.

- The draft has more than five front-page authors so the AD or RFC
  Editor will object. It is probably best for you to resolve this issue
  sooner rather than later.

Otherwise, my comments are a collection of small nits and nothing 
alarming.  Thanks for your work on this document.

Best,
Adrian

== Questions ==

Looking at Figure 1, an obvious question is why this model doesn't
augment the L2/L3NM or the common VPN model. It is OK (for me) that you
have chosen to augment the network topology model, but it is not clear
to the reader why you have done this.

---

I wonder whether the work in this document would benefit from using
data tags (draft-ietf-netmod-node-tags). I might be wrong, but it seems 
particularly related and useful.

---

If, in Figure 3, VPN1 is configured as hub and spoke with S1A as the 
hub, why is there a link in the virtual network between VN2 and VN3?

---

5.

General question about counters based on my memory of how we did MIBs
(So I am old! Quite possible that YANG does this differently.) Don't
you need something to handle resets? That is, to distinguish between
wrapping and resetting, we used to include a timestamp for when the
counters were last reset. Sometimes this was a timestamp per counter,
but usually enough for one timestamp across all counters.

(This probably makes a difference to the gauges and percentiles, too.)

Re-reading, it is possible that this is covered by 'reference-time' and
'measurement-interval'.  If so, this could be a lot clearer in 4.4.

---

5.

      leaf pm-source {
        type string;
        config false;
        description
          "The OAM tool used to collect the PM data.";
      }

I'm not convinced that using a string here is helpful. How does the
device know what string to use to convey meaning to the application? 

Or is the point that this is just printable information for display and
human consumption? If so, perhaps a note to this effect in Section 4.4.

== Nits ===

Abstract

It would be nice to say what the model in 8345 is. So...

   The data model for network topologies defined in RFC 8345 introduces
   vertical layering relationships between networks that can be
   augmented to cover network and service topologies.

---

Abstract

I think PM stands for 'performance monitoring' not 'network performance 
monitoring', so for the avoidance of doubt...

   This document defines a YANG module for 
   performance monitoring (PM) of both networks and VPN services that
   can be used to monitor and manage network performance on the topology
   at higher layer or the service topology between VPN sites.

---

2.

You have the BCP 14 boilerplate, but the uses of "should" and "must" in
the document are in lower case. There is one use of upper case "MAY" in
the Security Considerations which should be, I think, lower case since
it is a statement of fact not guidance to an implementer of this spec.

---

3.

s/Such an information/Such information/
s/should be setup/should be set up/
s/using network performance/using a network performance/
s/information from Traffic/information from the Traffic/

---

The Legend for Figure 3 could usefully add
   S:Site

It might also help to indicate the difference between links that are
shown as | and mappings that are shown as :

---

4.2

s/do not need to be extended./does not need to be extended./

---

4.2, 4.3

Figures 4 and 5 should be referred to from the text.

---

4.4

OLD
      Setting a percentile into
      0.00 indicates the client is not interested in receiving
      particular percentile.
NEW
      Setting a percentile to
      0.00 indicates the client is not interested in receiving
      that particular percentile.
END

s/metric (e.g./metric (e.g.,/

OLD   
   "reference-time" "measurement-interval"
NEW
   "reference-time" and "measurement-interval"
END

---

5.

   The "ietf-network-vpn-pm" module uses types defined in [RFC8345],
   [RFC6991], and [RFC8532].

You probably need to add [I-D.ietf-opsawg-vpn-common].

      "RFC CCCC: A Layer 2/3 VPN Common YANG Model";

I think you need a note to the RFC Editor to change that (although it
is likely that the RFC will come out relatively soon and you may get to
fix it up yourself).

Similarly...

     This version of this YANG module is part of RFC XXXX; see
     the RFC itself for full legal notices.";

  revision 2021-07-06 {
    description
      "Initial revision.";
    reference
      "RFC XXXX: A YANG Model for Network and VPN Service Performance
                 Monitoring";

You need a note to the RFC Editor to fix this up.

---

5.

Shouldn't 'percentile' also have a range?

---

6.

   There are a number of data nodes defined in this YANG module that are
   writable/creatable/deletable (i.e., config true, which is the
   default).  These data nodes may be considered sensitive or vulnerable
   in some network environments.  Write operations (e.g., edit-config)
   to these data nodes without proper protection can have a negative
   effect on network operations.  These are the subtrees and data nodes
   and their sensitivity/vulnerability:

   o  /nw:networks/nw:network/svc-topo:svc-telemetry-attributes

   o  /nw:networks/nw:network/nw:node/svc-topo:node-attributes

This feels like it may have been block copied from another document. I
think it needs updating for the specific writeable objects in this
module and the negative effects (which are surely not very great) of
them being unprotected.