Hi Authors,

Thanks first of all for working on this document. It has been a long time 
coming.

Please see my comments on the document, which I hope will go towards improving 
the document.

-------------------------------------------------------------------------------
COMMENT
-------------------------------------------------------------------------------

Section 2.5, paragraph 0
>    A maximum frame size configuration leaf (max-frame-size) is provided
>    to specify the maximum size of a layer 2 frame that may be
>    transmitted or received on an interface.  The value includes the
>    overhead of any layer 2 header, the maximum length of the payload,
>    and any frame check sequence (FCS) bytes.  If configured, the max-
>    frame-size leaf on an interface also restricts the max-frame-size of
>    any child sub-interfaces, and the available MTU for protocols.

If we are providing the ability to configure maximum frame size, should we 
providing a counter for packets that are discarded because the max size was 
exceeded?

Section 2.6, paragraph 0
>    The sub-interface feature specifies the minimal leaves required to
>    define a child interface that is parented to another interface.

I find the definition of sub-interface to be under specified. For example, if 
an implementation wants to enable configuration of a sub-interface that takes a 
ip-address, how would they go about it? And how would that relationship be tied 
to the parent-interface definition here? An example that shows how this can be 
enabled can go a long way in making this draft very useful for addressing a 
very common implementation.

The document introduces the concept of a sub-interface as a logical interface 
that handles a subset of the traffic received by the parent interface. However, 
it stops short of defining a container for it that could contain attributes 
specific to the sub-interface.

As I see it, the sub-interface will need some kind of index or id that uniquely 
identifies it. That index or id can come in the form of VLAN. To configure the 
VLAN, the ietf-if-vlan-encapsulation model could be used except that it 
dot1q-vlan is at an interface level, not at a sub-interface level. Similarly, 
if an ip-address needs to configured, ietf-ip module could have been used, but 
like the vlan encapsulation, the IP address can only be specified at the 
interface level.
This is common implementation, but this model and other related models fall 
short of supporting such an implementation.

If this document considers all the work of defining a sub-interface to be 
out-of-scope, it should remove this section and let another document define it.

Section 2.7, paragraph 1
>    The forwarding mode leaf provides additional information as to what
>    mode or layer an interface is logically operating and forwarding
>    traffic at.  The implication of this leaf is that for traffic
>    forwarded at a given layer that any headers for lower layers are
>    stripped off before the packet is forwarded at the given layer.
>    Conversely, on egress any lower layer headers must be added to the
>    packet before it is transmitted out of the interface.

I am assuming that 'forwarding-mode' applies to both the physical as well as 
the logical (sub-interface) level. Can that be made clear? Also, if we are 
identifying the forwarding mode, can it support counters related to them, e.g., 
VLAN discards or IP discards etc.?

Section 4, paragraph 9
>        "WG Web:   <http://tools.ietf.org/wg/netmod/>


Please update the link to the datatracker link.

Section 4, paragraph 26
>          presence
>            "Enable interface link flap dampening with default settings
>             (that are vendor/device specific).";

Not clear on how "default settings" can be vendor/device specific? Are default 
values not specified in the model with the vendor/device overriding the default 
values if necessary? 

Section 4, paragraph 26
>          leaf half-life {
>            type uint32;
>            units seconds;
>            description
>              "The time (in seconds) after which a penalty would be half
>               its original value.  Once the interface has been assigned
>               a penalty, the penalty is decreased at a decay rate
>               equivalent to the half-life.  For some devices, the
>               allowed values may be restricted to particular multiples
>               of seconds.  The default value is vendor/device
>               specific.";
>            reference "RFC XXXX, Section 2.3.2 Half-Life Period";
>          }

Same comment as above.

Section 5, paragraph 8
>        "WG Web:   <http://tools.ietf.org/wg/netmod/>

Please update the link to the datatracker link.
       

Section 5, paragraph 18
>        leaf in-pkts-64-octets {
>          type yang:counter64;
>          units frames;
>          description
>            "The total number of packets (including bad packets)
>             received that were 64 octets in length (excluding framing
>             bits but including FCS octets).

Is the unit "frames" or "packets"? The description uses "packets". Can we be 
consistent?

Section 6.1, paragraph 2
>    {
>      "ietf-interfaces:interfaces": {
>        "interface": [
>          {
>            "name": "eth0",
>            "type": "iana-if-type:ethernetCsmacd",
>            "oper-status": "up",
>            "statistics": {
>              "discontinuity-time": "2023-12-15T09:04:00-05:00"
>            },
>            "ietf-if-extensions:link-flap-suppression": {
>              "down": 0,
>              "up": 50
>            }
>          }
>        ]
>      }
>    }

Thanks for extensive and useful complete examples. It would be helpful if these 
were tagged with <CODE START> and <CODE ENDS> tag to enable extraction of them 
from the document and validate them against the model.

Section 6.3, paragraph 6
>    {
>      "ietf-interfaces:interfaces": {
>        "interface": [
>          {
>            "name": "eth0",
>            "type": "iana-if-type:ethernetCsmacd",
>            "phys-address": "00:00:5E:00:53:30",
>            "oper-status": "up",
>            "admin-status": "up",
>            "if-index": 1,
>            "ietf-if-ethernet-like:ethernet-like": {
>               "mac-address": "00:00:5E:00:53:30",
>               "bia-mac-address": "00:00:5E:00:53:30"
>            },
>            "statistics": {
>              "discontinuity-time": "2023-12-15T09:04:00-05:00"
>            }
>          }
>        ]
>      }
>    }
> 
>    The following example shows the intended configuration for interface
>    eth0 with an explicit MAC address configured.
> 
>    {
>      "ietf-interfaces:interfaces": {
>        "interface": [
>          {
>            "name": "eth0",
>            "type": "iana-if-type:ethernetCsmacd",
>            "ietf-if-ethernet-like:ethernet-like": {
>               "mac-address": "00:00:5E:00:53:30"
>            }
>          }
>        ]
>      }
>    }
> 
>    After the MAC address configuration has been successfully applied,
>    the operational state datastore reporting the interface MAC address
>    properties would contain the following, with the mac-address leaf
>    updated to match the configured value, but the bia-mac-address leaf
>    retaining the same value - which should never change.
> 
>    {
>      "ietf-interfaces:interfaces": {
>        "interface": [
>          {
>            "name": "eth0",
>            "type": "iana-if-type:ethernetCsmacd",
>            "phys-address": "00:00:5E:00:53:35",
>            "oper-status": "up",
>            "admin-status": "up",
>            "if-index": 1,
>            "ietf-if-ethernet-like:ethernet-like": {
>               "mac-address": "00:00:5E:00:53:35",
>               "bia-mac-address": "00:00:5E:00:53:30"
>            },
>            "statistics": {
>              "discontinuity-time": "2023-12-15T09:04:00-05:00"
>            }
>          }
>        ]
>      }
>    }


These examples are confusing, if they are related, as it appears from the 
description. Maybe part of the confusion is stemming from the fact that same 
MAC addresses are reused in the example.

The first example is that of the operational datastore before any config is 
applied and shows both "mac-address" and "bia-mac-address" as 
"00:00:5E:00:53:30”.

The second example is of for intended configuration datastore and is trying to 
set the "mac-address" to "00:00:5E:00:53:30". Could a different MAC address be 
used here to demonstrate the change, e.g., "00:00:5E:00:53:35" or something 
other than what is already showed in the first example.

The third example appears to be the state of the operational datastore once the 
above configuration is applied. According to the example, the "mac-address" 
does not seem to have changed from before because the address being configured 
is "00:00:5E:00:53:30", while the operational datastore is showing the 
mac-address as "00:00:5E:00:53:35". On the other hand, the bia-mac-address 
seems to have changed to "00:00:5E:00:53:30" because that is the address that 
is configured in the config example, when BIA MAC addresses are never supposed 
to change.

Section 9, paragraph 0
>    The YANG module defined in this memo is designed to be accessed via
>    the NETCONF protocol RFC 6241 [RFC6241].  The lowest NETCONF layer is
>    the secure transport layer and the mandatory to implement secure
>    transport is SSH RFC 6242 [RFC6242].  The NETCONF access control
>    model RFC 8341 [RFC8341] provides the means to restrict access for
>    particular NETCONF users to a pre-configured subset of all available
>    NETCONF protocol operations and content.

Please update this to reflect the latest changes in the template as documented 
in rfc8407bis.

-------------------------------------------------------------------------------
NIT
-------------------------------------------------------------------------------

All comments below are about very minor potential issues that you may choose to
address in some way - or ignore - as you see fit. Some were flagged by
automated tools (via https://github.com/larseggert/ietf-reviewtool), so there
will likely be some false positives. There is no need to let me know what you
did with these suggestions.

Section 1, paragraph 3
>    Several of the augmentations defined here are not backed by any
>    formal standard specification.  Instead, they are for features that
>    are commonly implemented in equivalent ways by multiple independent
>    network equipment vendors.  The aim of this document is to define
>    common paths and leaves for the configuration of these equivalent
>    features in a uniform way, making it easier for users of the YANG
>    model to access these features in a vendor independent way.  Where
>    necessary, a description of the expected behavior is also provided
>    with the aim of ensuring vendors implementations are consistent with
>    the specified behavior.

RFC 7950 does not use leaves. When referring to the plural of leaf, it uses 
leafs. I would suggest:

s/leaves/leafs/

in this paragraph and the rest of the document.

Section 4, paragraph 11
>        "This module contains common definitions for extending the IETF
>         interface YANG model (RFC 8343) with common configurable layer 2
>         properties.

Maybe one too many "common" occurances in the above statement?

Section 4, paragraph 48
>             Discontinuities in the values of this counter can occur at
>             re-initialization of the management system, and at other
>             times as indicated by the value of the 'discontinuity-time'
>             leaf defined in the ietf-interfaces YANG module
>             (RFC 8343).
>              ";

Does the closing quote and semicolon need to be on a newline?

These URLs point to tools.ietf.org, which has been taken out of service:

 * http://tools.ietf.org/wg/netmod/

Section 2, paragraph 12
> tate transition that occurs and reverts back within the specified time inter
>                                 ^^^^^^^^^^^^
Consider using just "reverts".

Section 4, paragraph 28
> specific. The penalty value for a link up->down state change is 1000 units."
>                                   ^^^^^^^
When "link-up" is used as a noun or modifier, it needs to be hyphenated.

Section 4, paragraph 28
> specific. The penalty value for a link up->down state change is 1000 units."
>                                   ^^^^^^^
When "link-up" is used as a noun or modifier, it needs to be hyphenated.

Section 4, paragraph 39
> f the number of frames that were well formed, but otherwise discarded because
>                                  ^^^^^^^^^^^
This word is normally spelled with a hyphen.

Section 5, paragraph 31
> iption "The 'burnt-in' MAC address. I.e the default MAC address assigned to 
>                                     ^^^
The abbreviation "i.e." (= that is) requires two periods.

Section 5, paragraph 33
> f the number of frames that were well formed, but otherwise discarded because
>                                  ^^^^^^^^^^^
This word is normally spelled with a hyphen.

Mahesh Jethanandani
mjethanand...@gmail.com



_______________________________________________
netmod mailing list -- netmod@ietf.org
To unsubscribe send an email to netmod-le...@ietf.org

Reply via email to