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

["Wubo (lana)" <[e-mail]>]

Hi Adrian,

Many thanks for your valuable and helpful review. Please see the reply inline.

Best regards,
Bo

> -----邮件原件-----
> 发件人: Adrian Farrel [mailto:[e-mail]]
> 发送时间: 2022年1月17日 0:16
> 收件人: [e-mail]
> 抄送: [e-mail]
> 主题: A review of draft-ietf-opsawg-yang-vpn-service-pm-01
> 
> 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.
[Bo] Thanks for the kind reminder, the authors plan to submit a new version this week and will address your comments in this 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.
[Bo] Thank you. we will correct this in the new version.
> 
> 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.
> 
[Bo]Thanks. We think it is useful to add some text to explain. How about adding some text like this:
The performance of VPN services is associated with the performance changes of the underlay network that carries VPN services, such as the delay of the underlay tunnels and the packet loss status of the device interfaces.
Additionally, the integration of Layer 2/Layer 3 VPN performance and network performance data enables the orchestrator to subscribe to VPN service performance in a unified manner. 
Therefore, this document defines a YANG module for both network and VPN service performance monitoring (PM).
> ---
> 
> 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.
> 
[Bo]Agree. We think draft-ietf-netmod-node-tags can helps PM accurately subscribe to metric-related data. How about the following text in section 3 Network and VPN Service Performance Monitoring Model Usage:
VPN and network performance management focus on the performance metric data of network devices. To avoid receiving a large amount of operational data of VPN instances, VPN interfaces, or tunnels, 
The controller can specifically subscribe to metric-specific data using the methods defined in draft-ietf-netmod-node-tags.

> ---
> 
> 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?
> 
[Bo] Thank you for catching this. We will remove the link.
> ---
> 
> 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.
> 
[Bo] Yes. Your understanding about 'reference-time' and 'measurement-interval' is correct. We will add text in 4.4 to make it clearer.
'reference-time': Indicates the start time of the performance measurement.
'measurement-interval' : Specifies the performance measurement interval, in seconds.
> ---
> 
> 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.
> 
[Bo] This is a pending comment we have and, as discussed in the last IETF meeting, identities will be used instead.

> == 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.
> 
[Bo] Thanks. We will make the change you suggested.
> ---
> 
> 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.
> 
[Bo] Thanks. We will make the change you suggested.
> ---
> 
> 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.
> 
[Bo] OK. We will remove BCP boilerplate.
> ---
> 
> 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/
> 
[Bo] Fixed.
> ---
> 
> 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 :
> 
[Bo] Fixed.
> ---
> 
> 4.2
> 
> s/do not need to be extended./does not need to be extended./
> 
[Bo] Fixed.
> ---
> 
> 4.2, 4.3
> 
> Figures 4 and 5 should be referred to from the text.
> 
[Bo] Fixed.
> ---
> 
> 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
> 
[Bo] Fixed.
> ---
> 
> 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.
> 
[Bo] Fixed.
> ---
> 
> 5.
> 
> Shouldn't 'percentile' also have a range?
[Bo] Thanks. We will add a range "1..100".
> 
> ---
> 
> 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.

[Bo] Thanks for pointing this out. We will fix this.