Hi Vladamir,
> -----Original Message-----
> From: netmod <[email protected]> On Behalf Of Rob Wilton (rwilton)
> Sent: 29 August 2019 17:20
> To: Vladimir Vassilev <[email protected]>; [email protected]
> Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
>
> Hi Vladimir,
>
> Please see inline ...
>
> > -----Original Message-----
> > From: Vladimir Vassilev <[email protected]>
> > Sent: 27 August 2019 15:55
> > To: Rob Wilton (rwilton) <[email protected]>; [email protected]
> > Subject: Re: [netmod] WG Last Call: draft-ietf-netmod-intf-ext-yang-07
> >
> > 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.
> [RW]
>
> I've renamed the module to ietf-if-extensions.yang.
>
> I still don't see that splitting this to a separate YANG module is
> helpful.
>
[RW]
I've now closed this issue (with the module renaming).
>
> >
> > >
> > >> 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.
> [RW]
>
> But it is not just the oper-status that is delayed (which would only
> affect manageability).
>
> Instead, it is the internal notification to the higher layer protocols
> that the underlying interface link state has changed. E.g. with carrier-
> delay down, the IP layer may still think that the interface is up when the
> Ethernet layer signalling indicates that the interface is actually down.
>
> I'll have a think and see if I can come up with a clearer name for this.
[RW]
Possibly, this could be renamed to something like "link-flap-suppression" or
"state-flap-suppression"?
I've included this as one of the open issues, and will track on a separate
thread.
>
>
> >
> > >> 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.
>
[RW]
I've removed the default values because they could potentially confusion if no
value is returned (although RFC 8342 section 5.3 indicates the expected
semantics in this case).
>
>
> > >
> > >
> > >> 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.
>
> OK. So, I think that this issue is really the same issue as (5) below.
> I.e. you are asking for a separate module to keep the name of the
> identities shorter.
>
>
> > >
> > >> 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?
>
> There are some choices here:
> (1) Keep with identities with a "loopback-" prefix. This causes
> loopback='loopback-internal'
> (2) Keep with identities, but loose the common prefix, i.e. the identities
> become "internal", "line", "connector"
> (3) Use shorter identities, but also put them in a separate module.
> (4) Use an enum rather than identities. Although this has the potential
> issue that enums cannot be extended.
>
> I've also asked the YANG doctors for their input on this question -
> consistent modelling behaviour across modules is also important.
>
> So far the consensus is to keep them in the same module but drop the
> "loopback-" prefix. Hence, this is my proposed resolution.
[RW]
Issue closed. I've keep the prefixes in the same module, but with shorter
names by loosing the prefix.
>
>
> > >
> > >> 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.
>
> OK.
[RW]
Issue closed.
>
>
> > >
> > > 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.
>
> OK, I will keep with the current definitions.
>
[RW]
Issue closed.
>
>
>
> > >> 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
>
> I've closed this issue.
>
> > >
> > > 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).
>
> Even then, I still think that loopback internal and line can both work.
>
> Loopback internal - you loop all frames that would have been transmitted
> out of the radio interface.
> Loopback line - you loop and frames received on the radio interface.
>
> Having a connector on a radio interface probably doesn't make as much
> sense.
>
> But still suggest that we rely on deviations to handle this case, and
> hence close this issue.
[RW]
Issue closed.
>
>
> > > 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.
[RW]
Draft updated.
> > >
> > >
> > >
> > >> 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.
[RW]
I've made this leaf config false, and changed the names to "physical",
"data-link", and "network".
> > >
> > >
> > >> 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).
> > >
[RW]
I have made this change (the text is in the security section).
> > >
> > >> 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.
>
> I don't think that those systems are correctly implementing the "in-
> unicast-pkts" counter.
>
> I.e. you shouldn't be handing packets off to the IP layer to forward, if
> it fails an L2 dest MAC check. If you don't hand the packet to IP, then
> you shouldn't be incrementing "in-unicast-pkts" as per the "delivered by
> this sub-layer to a higher (sub-layer)" comment.
>
> leaf in-unicast-pkts {
> type yang:counter64;
> description
> "The number of packets, delivered by this sub-layer to a
> higher (sub-)layer, that were not addressed to a
> multicast or broadcast address at this sub-layer.
> ..."
>
> And also the "prevent their being deliverable to a higher-layer protocol"
> comment in in-discards:
>
> leaf in-discards {
> type yang:counter32;
> description
> "The number of inbound packets that were chosen to be
> discarded even though no errors had been detected to
> prevent their being deliverable to a higher-layer
> protocol. One possible reason for discarding such a
> packet could be to free up buffer space.
>
[RW]
I've kept the original text, it is useful clarification, and does not
change the meaning of the existing counter.
>
>
>
>
> >
> > >
> > >
> > >
> > >>
> > >> 14. I propose the in-pkts and out-pkts counters standardized too.
> > >>
> > https://github.com/YangModels/yang/blob/master/vendor/cisco/xe/1641/ie
> > tf-
> > >> interfaces-ext.yang.
> > >> And yes someone forgot to update the boilerplate text.
> > > This one I think that we need to get further input on.
[RW]
I've kept this as an open issue to get closure 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
[RW]
I've kept this as an open issue.
> > >
> > >
> > >> 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.
>
> My interpretation of RFC 8343 is that ingress packets to an interface are
> all included in the in-octets counter, and then categorized into one of 6
> packet counters:
> - good packets (delivered to a high layer protocol) are counted against
> one of in-unicast-pkts, in-broadcast-pkts or in-multicast-pkts
> - valid packets dropped because the higher layer protocol is unknown (or
> not configured/enabled) are counted against in-unknown-protos
> - erroneous packets (e.g. all framing errors, too long, too short) are
> counted against in-errors,
> - all other packets dropped on the ingress interface for any other reason
> not covered above are counted against in-discards
>
> in-drop-unknown-dest-mac-pkts doesn't change the above definitions. Is
> this the subset of in-discards that are dropped because the dest MAC
> address is invalid. If frames counted against in-drop-unknown-dest-mac-
> pkts weren't also counted against in-discards then that would effectively
> changes the definition in-discards, and would break implementations that
> are not checking the in-drop-unknown-dest-mac-pkts counter.
>
[RW]
I have kept in-drop-unknown-dest-mac-pkts, because it is useful counter.
Thanks,
Rob
> Thanks,
> Rob
>
>
> > >
> > >
> > >> 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
_______________________________________________
netmod mailing list
[email protected]
https://www.ietf.org/mailman/listinfo/netmod
