Hi Adrian, the Authors, and All, thank you Adrian for reviewing the draft and inviting further discussion. I've commented on this work earlier suggesting considering the performance metrics listed in the STAMP YANG data model. I appreciate that the Authors have found them helpful and included them in this model. But I still wonder what, if anything, is special for a VPN service from a performance metrics perspective compared to, for example, an underlay? Would it be possible and simpler to have a single PM data model applicable to any underlay or overlay network?
Regards, Greg On Sun, Jan 16, 2022 at 8:16 AM Adrian Farrel <[email protected]> wrote: > 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. > > _______________________________________________ > OPSAWG mailing list > [email protected] > https://www.ietf.org/mailman/listinfo/opsawg >
_______________________________________________ OPSAWG mailing list [email protected] https://www.ietf.org/mailman/listinfo/opsawg
