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 _______________________________________________ OPSAWG mailing list OPSAWG@ietf.org https://www.ietf.org/mailman/listinfo/opsawg