Hi,
Apologies for the delay, but I have now managed my AD review of
draft-ietf-opsawg-l2nm-12. (Also attached in case my email is truncated ...)
I would like to thank the authors, WG for their work on this document, and the
shepherd for his review. I found the document to be well written and pretty
straightforward to follow and understand. I also believe that this document is
a useful addition to the YANG and Network Management Ecosystem, to thank you
for that.
Anyway, here my comments. I think that they mostly pretty minor, but perhaps a
few that my need a bit more thought. Hopefully they will help improve the doc.
---
Moderate comments:
(1)
The VPN network access is comprised of:
'id': Includes an identifier of the VPN network access.
'description': Includes a textual description of the VPN network
access.
'interface-id': Indicates the interface on which the VPN network
access is bound.
'global-parameters-profile': Provides a pointer to an active
'global-parameters-profile' at the VPN node level.
Referencing an
active 'global-parameters-profile' implies that all associated
data nodes will be inherited by the VPN network access.
However,
some of the inherited data nodes (e.g., ACL policies) can be
overridden at the VPN network access level. In such case,
adjusted values take precedence over inherited ones.
It wasn't entirely clear to me how this works with the global parameters
defined at the VPN network access level and the VPN node level work. Is this
meant to be a 3 tier hierarchy, or is it always only 2 tiers? Are you allowed
to reference different global profiles both at the VPN network access level and
the VPN node level? Possibly, some slightly expanded description here may be
helpful (and/or in the YANG module).
(2) | +--rw encapsulation
| | +--rw encap-type?identityref
| | +--rw dot1q
| | | +--rw tag-type? identityref
| | | +--rw cvlan-id? uint16
Did you consider adding support for ranges or sets of VLAN Ids (e.g., a list of
non-overlapping ranges) (both for the single and double tagged cases)?
(3) | +--rw lag-interface
I'm slightly surprised that you don't have parameters for the physical
interfaces, and I can understand your justification for this, but then you do
have configuration for LAG, including configuration for the underlying member
interfaces. This feels slightly inconsistent to me at the level that the
configuration is being provided. Understanding the rationale a bit more here
might be helpful, and I think that we should double check that this should
definitely be in this model.
(4)+--rw svc-pe-to-ce-bandwidth
| {vpn-common:inbound-bw}?
| +--rw pe-to-ce-bandwidth* [bw-type]
Can you specify different bandwidths for multiple CoS fields? It looks like
the list key is the bw-type, and hence you would only be able to specify the
bandwidth for a single CoS field? Is that sufficient?
(5) 8.3. Ethernet Segments
The names used here don't seem to be particularly friendly. Is giving them
more human understandable identity names an option, or are these names directly
mirroring some other registry? Or perhaps even something like esi-type-1-lacp,
esi-type-3-mac, etc?
(6)leaf svc-mtu {
type uint32;
description
"Service MTU, it is also known as the maximum transmission
unit or maximum frame size. When a frame is larger than
the MTU, it is fragmented to accommodate the MTU of the
network.";
}
MTU's turn up in various places. Given that MTUs seem to mean different things
for different people, please can you check the descriptions and given that this
model is for L2VPN's be super explicit about whether these MTUs are
representing the L3 payload, or the max frame size including the L2 header and
presumably FCS sequence as well.
(7) identity mac-action {
description
"Base identity for a MAC action.";
}
Would an VPN implementation ever want to drop and log rather than just doing
one or the other?
(8) Hierarchical groupings and defaults
I want to check what the model/plan is regarding hierarchical config (e.g.,
grouping parameters-profile) and default values. It looks like some of the
leaves in the provide have default values, which I believe will be ambiguous
when it comes to hierarchical config. I.e., normally I would
