On 22/08/2019 12.13, Rob Wilton (rwilton) wrote:
Hi Vladimir,
Thanks for your detailed review. Sorry for the slow reply, I've been away.
I'm also about to be away again for a couple of days.
Please see my comments inline ...
I'll also track these issues to closure on
https://github.com/netmod-wg/interface-extensions-yang/issues
-----Original Message-----
From: netmod <[email protected]> On Behalf Of Vladimir Vassilev
Sent: 13 August 2019 17:05
To: Kent Watsen <[email protected]>; [email protected]
Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
I have reviewed the draft. I have the following (19) IMO useful proposals:
1. Dedicated module (ietf-if-oper-status-debounce.yang) for the oper-
status debouncing/dampening functionality currently in ietf-interfaces-
common.yang.
I don't think that we want a proliferation of too many separate YANG modules
for small features. Each of the areas of different functionality within this
module are already conditional on if-feature, so I don't think that there is a
strong justification to separating this out as a separate module.
I still think that especially the "dampening" mechanism is not common
enough and is quite complex to be added to ietf-interfaces-common. If a
feature is not common or does not enable the use of generic modeling
mechanism (like sub-interfaces etc.) it should not be in
ietf-interfaces-common. I do not think "dampening" (maybe at some point
we should go back to damping instead e.g. rfc2439 ... seems there is
difference between dampening and damping and damping seems to be the
correct one) is that common to deserve a place in ietf-interfaces-common.
2. In sec "3.1 Carrier delay" use of the under-defined "Carrier"
definition can be replaced with direct reference to the oper-status leaf
(which is what is actually targeted by the algorithm) "Operational status
transition debouncing".
I think that different vendors have different names for this technology. I've
just used the one that our products use. I think that this is just a name,
rather than something that has to be defined. I could add a comment that this
feature is sometimes called hold time?
I looked for precedents - "carrier-delay" leaf Cisco,
"debouncing-interval" leaf Juniper, "interface-phys-holdtime-config"
leaf OpenConfig.
I think "Carrier" is confusing since what is delayed actually is the
transition of the oper-status.
3. "timer-running" and "suppressed" leafs are both "config false" and have
"default" statements. Although this is valid YANG I do not think the
"default" statements are intended.
I think that this is a more general question that needs a bit more discussion.
Here, I am using defaults for the config false node to document what the normal
value is expected.
Well not a real issue but I thought it was an unusual use of default.
4. Dedicated module (ietf-if-loopback.yang) for the loopback functionality
currently in ietf-interfaces-common.yang.
Same answer as for 1. I don't think that we should have too many really small
modules.
If the loopback was modeled as a boolean leaf (as in OpenConfig) I would
have agreed. However even small modules that define base identities
benefit from dedicated namespace. For me ietf-if-loopback.yang will pay
off since loopback='internal' is better then
loopback='loopback-internal' and there are going to be many test cases
that use that line. An last but not least I never had problems with too
much modularity.
5. Less verbose loopback identities. With dedicated module the
(loopback-* identities can be shortened skipping the prefix).
I think that it is normal to bind the identity names to the common base
identity. I don't see that the length of the identities should really be an
issue.
For me the length of identities does matter since I often use command
line tools. But it is mostly the irritation caused by the tautology
loopback='loopback-internal' that everyone writing network interconnect
testcases is going to be stuck with forever if we leave the loopback
control model as part of ietf-interfaces-common and not separate
ietf-if-loopback. What do others think?
6. The draft introduces "loopback-internal", "loopback-line" and
"loopback-connector" loopback identities. What is confusing is that
"internal loopback" is historically the opposite of "external loopback"
which is a loopback with a connector. I think terminology already in use
like "near-end" and "far-end" is less confusing.
The internal/line loopback configuration has been used in parts of the industry
for at least 20 years, so this terminology is already in use.
I'm not sure that "near-end" and "far-end" would be less confusing. Assuming that "loopback
far-end" was equivalent to "loopback-line" then it would be somewhat of a misnomer since it acts on the
near end, not the far end.
I.e. both loopback internal, and loopback line act on the local interface, the only
difference is in which direction they reflect the signals, i.e. Egress -> Ingress
(internal), or Ingress -> Egress (line).
I can live with local/line/connector but I do not agree
near-end/far-end/connector is more confusing.
Perhaps the description text could be slightly clarified here to help avoid
confusion?
OLD:
The following loopback modes are defined:
o Internal loopback - All egress traffic on the interface is
internally looped back within the interface to be received on the
ingress path.
o Line loopback - All ingress traffic received on the interface is
internally looped back within the interface to the egress path.
o Loopback Connector - The interface has a physical loopback
connector attached that loops all egress traffic back into the
interface's ingress path, with equivalent semantics to internal
loopback.
NEW:
The following loopback modes are defined:
o Internal loopback - All frames that egress out of the interface
are looped back internally within the interface hardware
to be received on the ingress path.
o Line loopback - All ingress frames received on the interface from
the line are looped back within the interface hardware and
transmitted back out of the interface.
o Loopback connector - The interface has a physical loopback
connector attached that loops all egress frames back into the
interface's ingress path, with equivalent semantics to internal
loopback.
Adding frames excludes continuous stream (no frame) devices like legacy
serial links and high speed transceivers, optical links. Loopback is
applicable there too. IMO the OLD text was better.
7. I am not sure standardizing the "loopback-connector" identity is
justified. All usecases of connecting a loopback connector I can think of
require the system to not be aware there is special external loopback
connector on the interface.
I think that it will depend on how smart of dumb the external loopback
connector is. If it is just a dumb electrical or optical loopback then the
source and destination MAC addresses need to be swapped, or otherwise any
egress frames out of the interface will fail the destination MAC address filter
when they are looped around.
OK
Some implementations also use this configuration to force self ping packets out
through the interface, so that the full datapath is tested, rather than the
packets being looped internally within the L3 forwarding code.
8. Some interfaces that implement "loopback-internal" do not implement
"loopback-line" - e.g. classical ethernetCsmacd (Carrier-sense multiple
access with collision detection) has a physical layer that by design can
not implement such loopback. Maybe introducing a dedicated feature to
enable the "loopback-line" is a good idea.
I'm not sure on this one, i.e. whether it really helps or just adds extra
clutter.
Realistically, I think that ethernetCsmacd is dead. Do you have other examples
of interface types that do support loopback, but not in both directions?
All interfaces that are not point-to-point (e.g. common wifi).
This might be something better handled via a deviation, or the device failing
the configuration when it is verified.
OK
As a side note, one of the limitations of features and deviations is that the
apply to all interfaces on the device, but the actual properties of an
interface might vary depending on the speed, type and hardware associated with
the interface.
9. Appropriate entry in the "11. Security Considerations" noting the
possibility of DoS attacks and broadcast traffic storms resulting from
loopbacks:
OLD:
The following leaf could cause the interface to go down, and stop
processing any ingress or egress traffic on the interface:
o /if:interfaces/if:interface/loopback
NEW:
The following leaf could cause the interface to go down, and stop
processing any ingress or egress traffic on the interface. It could
cause broadcast traffic storms.
o /if:interfaces/if:interface/loopback
Ack.
10. Introducing config true "forwarding-mode" leaf breaks clients that
support e.g. rfc8344 ietf-ip (which has its dedicated forwarding leafs
e.g. /ietf-interfaces:interfaces/interface/ietf-ip:ipv4/forwarding ) by
introducing this new module with a new leaf they know nothing about. I
support this leaf as config false. If NETCONF was not transactional a
global leaf enabling the forwarding configuration would be a feature.
But NETCONF is transactional.
I don't get the relevance of transactions, but it isn't intended to break
existing clients/YANG modules.
The idea of this leaf is that if it is configured then the system can use it to
check other constraints. E.g. to validate that an L2 QoS policy isn’t being
configured on an L3 interface. If the leaf isn't configured then those
constraints are not checked.
If this leaf is only enabling optional additional constrains (and this
is the only backward compatible option) It won't be that useful as
config true.
11. The "forwarding-mode" leaf has the following set of identities
{optical-layer, l2-forwarding, network-layer}. We could make the identity
names shorter and consistent. l1,l2,l3 or physical,data-link,network.
I've tried to use names here that network engineers are most likely to be
familiar with.
I think that using the OSI layer names (e.g. l1, l2, l3) would be too terse.
We could change "l2-forwarding" to "data-link-layer", but I would think that people would
be more familiar with "l2-forwarding" as a term. E.g. related to L2VPN.
12. I do not agree we need this text. Normally NETCONF devices should
accept transactions to any valid configuration:
OLD:
...
Normally devices will not allow the parent-interface leaf to be
changed after the interfce has been created. If an implementation
did allow the parent-interface leaf to be changed then it could cause
all traffic on the affected interface to be dropped. The affected
leaf is:
o /if:interfaces/if:interface/parent-interface
...
NEW:
...
Changing the parent-interface leaf could cause
all traffic on the affected interface to be dropped.
The affected leaf is:
o /if:interfaces/if:interface/parent-interface
...
This isn't about transactions so much as valid configuration.
Normally, the name of the sub-interface is tightly bound to the parent interface. E.g. if the parent in
"Ethernet0/1" then the sub-interface would be "Ethernet0/1.1". If you tried to change the
parent-interface leaf of "Ethernet0/1.1" to "Ethernet2/2" then I would expect the system to reject
that change (because the configuration is invalid not because of transactions).
13. The in-drop-unknown-dest-mac-pkts changes the behavior of the in-
unicast-pkts,in-multicast-pkts and in-broadcast-pkts. I do not agree any
discarded packets in the forwarding process should be subtracted from the
interface counters.
Here is the current description:
OLD:
For consistency, frames counted against this drop
counters are also counted against the IETF interfaces
statistics. In particular, they are included in
in-octets and in-discards, but are not included in
in-unicast-pkts, in-multicast-pkts or in-broadcast-pkts,
because they are not delivered to a higher layer.
NEW:
The implementation of this counter does not
change the behavior of the counters defined in
IETF interfaces statistics.
It is not changing the definitions of those counters at all. It is just
explaining the relationship between them.
The problem I see is that today there are existing systems that
implement ietf-interfaces and discard packets because they have unknown
MAC destination. But those systems do not subtract the number of
discarded packets from the "in-unicast-pkts" counter. Those systems will
have to change their behavior in a non-backward compatible way to be
able to implement the in-drop-unknown-dest-mac-pkts with this
description. In my view this text changes the definition of
in-unicast-pkts enforcing certain design that was not enforced before.
14. I propose the in-pkts and out-pkts counters standardized too.
https://github.com/YangModels/yang/blob/master/vendor/cisco/xe/1641/ietf-
interfaces-ext.yang.
And yes someone forgot to update the boilerplate text.
This one I think that we need to get further input on.
https://github.com/YangModels/yang/blob/master/standard/ieee/published/802.3/ieee802-ethernet-interface.yang
defines in-frames and out-frames, but these are only for Ethernet, but you are
probably looking for a counter across all interface types.
Yes. in-pkts and out-pkts are used by OpenFlow, OpenDaylight,
OpenConfig, Cisco etc. Those are interface independent. It is impossible
to implement ietf-interfaces for devices that provide only in-pkts and
out-pkts (that would be all OpenFlow switches) which is a good argument
to have the counters standardized.
15. I propose that new "ietf-interfaces-common:in-discards-overflow"
counter is added. Currently the "ietf-interfaces:in-discards" can contain
both discards like the ones accumulated in in-drop-unknown-dest-mac-pkts
and discards caused by overflows (performance related loss of packets like
freeing buffer space in devices that in certain cases are forwarding
slower then the line speed). Turns out knowing if device is discarding
(loosing) packets due to performance shortage and discarding (filtering)
unwanted packets are two different events that one needs to differentiate
between are currently in the same in-discards counter. We can fix that
with the introduction of in-discards-overflow counter.
This one I think that we need to get further input on. I think that this might
be useful. But we might need some care to ensure that it fits cleanly with QoS
drops.
If we were to add this then the definition of "ietf-interfaces:in-discards" cannot
change. i.e. In-discards-overflow would be a subset of "in-discards".
OK
16. We can replace
"ietf-interfaces-ethernet-like:in-drop-unknown-dest-mac-pkts" with (in-
discards - in-discards-overflow) for MAC Bridges or any other Ethernet
interface plus save us the introduction of technology specific similar
counters for the rest of the Bridges and non-Ethernet interfaces.
For Ethernet, having a technology specific in-drop-unknown-dest-mac-pkts is
useful.
In the WG discussion, there was agreement to also add a drop counter for
packets that are dropped because they cannot be demuxed to any sub-interface.
Personally, I think that it is useful to have an overall drop counter that
captures everything, along with more specific drop counters that sometimes give
more information as to what has causes specific drops. Specifically, just
because a more specific drop counter has been defined, that doesn't mean that
it shouldn't also be included in the general drop counter.
IMO the in-discards counter should be incremented in very rare
circumstances (e.g. ingress clock frequency above supported) and ideally
reserved for reporting performance issues in the MAC that should
normally not exist. The "unknown-dest-mac" and "packets that are dropped
because they cannot be demuxed to any sub-interface" should be handled
at another "sub-layer" and do not need to be subtracted from the
ietf-interfaces in-*-pkt counters because this is very confusing. But
probably there are systems out there that already use "in-discards" for
all sorts of discards.
17. I have separately posted my arguments against introduction of leaf
named l2-mtu and the need of a config false leaf that has similar
semantics as the ifMtu object from IF-MIB.
OK, lets keep this issue on that other thread.
OK
18. Some references to relevant IEEE standards and IEEE maintained YANG
modules should be added (in the scope of ietf-interfaces-ethernet-like).
Also a few lines explaining the policy change and why none of the
RFC3635 managed objects are part of the new ietf-interfaces-ethernet-like
YANG module.
Yes, OK.
19. ietf-if-common.yang and ietf-if-ethernet-like.yang instead of ietf-
interfaces-common.yang and ietf-interfaces-ethernet-like.yang.
Setting a shorter naming precedent for future modules augmenting ietf-
interfaces.
I'm not opposed to shorter names, but would be interested in the views of
others in the WG.
OK
/Vladimir
Thanks again for the review. It is appreciated.
Rob
/Vladimir
_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod
