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

[Adrian Farrel <[e-mail]>]

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.

---

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

---

2.1

OLD
   SLA     Service Level Agreements
NEW
   SLA     Service Level Agreement
END

---

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.

---

3.1

s/dynamic-changing/dynamic/

---

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

---

4.

Would it be more consistent if the box on the right of Figure 2 showed
"ietf-network-vpn-pm"?

---

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

---

4.1

s/topologies are both built/topologies are built/

---

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.

---

4.1

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

---

4.2

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

---

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.

---

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.

---

4.4

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

Perhaps "counters and gauges"?

---

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.

---

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?

---

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

---

OLD
   Appendix A.  Illustrating Examples
NEW
   Appendix A.  Illustrative Examples
OR
   Appendix A.  Examples
END