Re: [OPSAWG] Document shepherd review of draft-ietf-opsawg-yang-vpn-service-pm-06

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

Hi Adrian,

Many thanks for your detailed review. We have released Rev-07 to address these issues, see if they are fully addressed.
https://www.ietf.org/rfcdiff?url2=draft-ietf-opsawg-yang-vpn-service-pm-07

Please also find some replies inline.

Thanks,
Bo

-----Original Message-----
From: Adrian Farrel [mailto:[e-mail]] 
Sent: Saturday, April 23, 2022 12:35 AM
To: [e-mail]
Cc: 'opsawg' <[e-mail]>
Subject: Document shepherd review of draft-ietf-opsawg-yang-vpn-service-pm-06

Hi,

I'm the document shepherd for this document as it moves beyond the WG for requested publication as an RFC.

I reviewed the draft at -03 during WG last call, so my comments here are basically editorial with only a few small questions.

If the authors could produce a new revision, I will start work on the shepherd write-up.

One other point: can someone say whether this draft has been shared with the IPPM working group?

Thanks,
Adrian

===

Introduction.

First sentence could use a reference to RFC 6020.
[Bo Wu] Fixed.

---

Introduction

OLD
   It defines that the performance
   measurement telemetry model to be tied with the service, such as
   Layer 3 VPN and Layer 2 VPN, or network models to monitor the overall
   network performance or Service Level Agreement (SLA).
NEW
   It defines that the performance
   measurement telemetry model should be tied to the services (such as
   a Layer 3 VPN or Layer 2 VPN) or to the network models to monitor the
   overall network performance and the Service Level Agreements (SLAs).
END

 [Bo Wu] Fixed.
---

2.1

OLD
   SLA     Service Level Agreements
NEW
   SLA     Service Level Agreement
END

[Bo Wu] Fixed.
---

3.

   For example, the
   controller can use information from [RFC8345], [I-D.ietf-opsawg-sap]
   or VPN instances.

I think this is where there should be a reference to RFC 9182 and draft-ietf-opsawg-l2nm.

[Bo Wu] Fixed.
---

3.1

s/dynamic-changing/dynamic/
[Bo Wu] Fixed.

---

4.

OLD
   This document defines the YANG module, "ietf-network-vpn-pm", which
   is an augmentation to the "ietf-network" and "ietf-network-topology".
NEW
   This document defines the YANG module, "ietf-network-vpn-pm", which
   is an augmentation to the "ietf-network" and "ietf-network-topology"
   modules.
END

[Bo Wu] Fixed.
---

4.

Would it be more consistent if the box on the right of Figure 2 showed "ietf-network-vpn-pm"?
[Bo Wu] Fixed.

---

I think that Figure 3 could use a little tidying.
- Some gaps in lines
- A couple of lines slightly out of place
- S2A and S2B are confusinly places
- The cross-over of VN3-N2 and VN1-N1 is unclear
- Wording of the Legend a little unclear

How about...


                     VPN 1                       VPN 2
          +------------------------+   +------------------------+
         /                        /   /                        /
        / S1C_[VN3]:::           /   /                        /
       /         \   :::::      /   / S2A_[VN1]____[VN3]_S2B /
      /           \       :::  /   /      :          :      / Overlay
     /             \         :::::::::::: :          :     /
    / S1B_[VN2]____[VN1]_S1A /   /       :           :    /
   +---------:-------:------+   +-------:-:----------:---+
             :        :     ::::::::::::   :         :
             :         :   :                :        :
   Site-1A   :  +-------:-:------------------:-------:-----+ Site-1C
     [CE1]___:_/_______[N1]___________________[N2]___:____/__[CE3]
             :/       / / \             _____//     :    /
   [CE5]_____:_______/ /    \     _____/     /    ::    /
 Site-2A    /:        /       \  /          /   ::     /
           / :                [N5]         /  ::      / Underlay Network
          /   :     /       __/ \__       / ::       /
         /     :   /    ___/       \__   /::        /
Site-1B /       : / ___/              \ /:         /  Site-2B
[CE2]__/________[N4]__________________[N3]________/____[CE4]
      /                                          /
     +------------------------------------------+

    Legend:
    N:Node   VN:VPN-Node  S:Site  CE:Customer Edge
    __  Link within a network layer
    :   Mapping between network layers

[Bo Wu] Fixed. Thanks for helping to correct the figure.
---

4.1

s/topologies are both built/topologies are built/
[Bo Wu] Fixed.

---

The legend for Figure 4 should include "TP" (if TPs are actually relevant to the figure and aren't something you should remove - the text doesn't mention them, and they don't really seem to be important in Section 4.1).

Probably, TP should be added to the list in Section 2.1 with a reference to where TP is properly explained. 4.4 would then be able to lean on that definition.

[Bo Wu] Fixed. Thanks for catching this. The reference of TP has been added in Section 2.1.
---

4.1

s/VPN PM can provides/VPN PM can provide/

[Bo Wu] Fixed.
---

4.2

s/[RFC9181])./[RFC9181]./

[Bo Wu] Fixed.
---

4.2 etc.

Not sure why 'mac-num' has that name when you use 'ipv4' and 'ipv6'
not 'ipv4-num' and 'ipv6-num'.  This is highly unimportant, but might be something to fix purely for consistency of appearance.

[Bo Wu] Fixed.
---

4.4

   The 'links' are classified into two types: topology link defined in
   [RFC8345] and abstract link of a VPN between PEs.

Would be nice to give a reference for the abstract link as well.
[Bo Wu] Fixed. The abstract one is defined in this module.

---

4.4

   The performance data of a link is a collection of counters that
   report the performance status.

Perhaps "counters and gauges"?
[Bo Wu] Fixed.

---

5. and  10.

I think that all documents referenced from 'reference' clauses should be Normative References. I found 3 (4026, 4364, 8571) that are not.
There might be a good reason (if so tell me) or this could be an oversight.
[Bo Wu] Fixed. Sorry for the oversight, not fully corrected. Tom also pointed this out .

---

5.

It's not really your fault, but I hate to see types redefined, especially with the same name.

     typedef percentage {
       type decimal64 {
         fraction-digits 5;
         range "0..100";
       }
       description
         "Percentage.";
     }

...appears exactly like this in RFC 8532. This makes me think that it should possibly be in a common types module somewhere. Possibly nothing you can do about this at this stage.

Do we have a way of flagging desirable common types to Netmod?

Is there value in you using a different name for this type just to set it in the context of your work?
[Bo Wu] Thanks for pointing this out. And I need more guidance on this issue. The definition of percentage in this draft is the same as that in RFC 8532, which is also for "loss-ratio". Actually, this value may derive from the mechanism of RFC 8532. We have imported "ietf-lime-time-types" from RFC 8532. But "percentage" is defined in "ietf-connectionless-oam". As "ietf-connectionless-oam" is a device model, I'm not sure if a network configuration model could import "ietf-connectionless-oam".

---

5.

vpn-pm-type has a case for inter-vpn-access-interface that is empty and described as a placeholder. And that is all good.

But I expected some text (not a lot) explaining:
- why this is empty
- how/why it might be used in future (presumably through augmentation)

I suspect this belongs in the "VPN PM type" hanging text in Section 4.4
[Bo Wu] Our consideration is inter-vpn-access-interface PM is VPN-specific measurement, compared with the tunnel PM that may be shared by multiple VPNs. And based on this, the measurement could be CE-PE-PE-CE or PE-PE or other combination. The empty leaf is defined to specify the basic VPN specific measurement, and allow extension for other measurement combinations.
Please see whether the new text helps. Here is the text proposed:
"This is a placeholder for inter-vpn-access-interface PM, which is not bound to a specific VPN access interface. The source or destination VPN access interface of the measurement can be augmented as needed."

---

OLD
   Appendix A.  Illustrating Examples
NEW
   Appendix A.  Illustrative Examples
OR
   Appendix A.  Examples
END
[Bo Wu] Fixed.