Re: [yang-doctors] Yangdoctors last call review of draft-ietf-nvo3-yang-cfg-05

[Mehmet Ersue <[e-mail]>]

Many Thanks Jan!

Cheers,
Mehmet

> -----Original Message-----
> From: yang-doctors <[e-mail]> On Behalf Of Jan
> Lindblad via Datatracker
> Sent: Thursday, September 16, 2021 10:11 AM
> To: [e-mail]
> Cc: [e-mail]; [e-mail];
[e-mail]
> Subject: [yang-doctors] Yangdoctors last call review of
draft-ietf-nvo3-yang-
> cfg-05
> 
> Reviewer: Jan Lindblad
> Review result: Not Ready
> 
> This is the YANG Doctor review of draft-ietf-nvo3-yang-cfg. In my
judgement
> the draft is not ready for last call. This is not to say that there isn't
plenty of
> good work that has gone into this draft and YANG module, but since I have
> been unable to compile it, due to unclear dependencies, the fundamental
> requirements for performing a review aren't met.
> 
> As soon as you import another module, you become dependent on that
> module and inherit any problems it may have. If the modules you depend
> upon are not stable enough, it may not be viable to perform a last call
review.
> As long as the modules you depend upon are not published, I would think
> you cannot go through with publication of your own module either.
> 
> There's always a bit of guesswork involved when a YANG module imports
> unpublished modules. By looking in the IETF Git repository, under
> "standards/ietf/RFC" as well as under "experimental/ietf-extracted-YANG-
> modules", i.e. the section for automatically extracted YANG modules from
> drafts, I was able to guess at which modules that are transitively
referenced.
> Based on this, I believe we will need to have stable versions of the
following
> modules before the draft-ietf-nvo3-yang-cfg can proceed. Experimental
> modules are marked (*).
> 
> iana-bfd-types.yang (*)
> iana-if-type.yang
> ietf-bfd-types.yang (*)
> ietf-bgp-common-multiprotocol.yang (*)
> ietf-bgp-common-structure.yang (*)
> ietf-bgp-common.yang (*)
> ietf-bgp-l3vpn.yang (*)
> ietf-bgp-neighbor.yang (*)
> ietf-bgp-peer-group.yang (*)
> ietf-bgp-policy.yang (*)
> ietf-bgp-rib-attributes.yang (*)
> ietf-bgp-rib-tables.yang (*)
> ietf-bgp-rib-types.yang (*)
> ietf-bgp-rib.yang (*)
> ietf-bgp-types.yang (*)
> ietf-bgp.yang (*)
> ietf-inet-types.yang
> ietf-interfaces.yang
> ietf-ip.yang
> ietf-key-chain.yang
> ietf-l2vpn.yang (*)
> ietf-netconf-acm.yang
> ietf-network-instance.yang
> ietf-pseudowires.yang (*)
> ietf-routing-policy.yang (*)
> ietf-routing-types.yang
> ietf-routing.yang
> ietf-tcp-common.yang (*)
> ietf-tcp.yang (*)
> ietf-yang-schema-mount.yang
> ietf-yang-types.yang
> 
> Since I already did a first reading and found a few things, I'll list them
here for
> the WG's benefit rather than me holding them to myself.
> 
> 1. YANG 1.1
> 
> The draft says "YANG [RFC6020] is a data definition language that was
> introduced to". Since you are using yang-version 1.1, it would be more
> appropriate with a reference to RFC 7950.
> 
> 2. Identity equality tests
> 
> There are a couple of places in the YANG that check that an interface is
of
> Nve type. Currently this is done using an equality test. While that can
work, a
> better and more future proof way is to use derived-from-or-self(). So
change
> 
>  must "(/if:interfaces/if:interface[if:name=current()]/if:type='Nve')";
> 
> to
> 
>  must
>
"derived-from-or-self(/if:interfaces/if:interface[if:name=current()]/if:type
,
>  'Nve')";
> 
> 3. Behavioral constraints defined in English text
> 
>           list static-peer {
>             key "peer-ip";
> ...
>             leaf out-vni-id {
>               type uint32 {
>                 range "1..16777215";
>               }
>               description
>                 "The ID of VNI for outbound. Do not support separate
deletion.";
>             }
> 
> I'm not completely clear as to the "Do not support separate deletion" is
> supposed to mean. If every static-peer must have an out-vni-id, it should
be
> marked mandatory. If this is supposed to mean that an out-vni-id may be
> added, but not deleted, that violates one of the basic principles in YANG.
The
> validity of a configuration should not depend on anything else than the
> configuration itself. In particular, the validity of an upcoming
configuration
> should not depend on the prior configuration.
> 
> If you don't want out-vni-id to be mandatory, but also don't want people
to
> be able to delete it without deleting the static-peer, I would recommend
> making out-vni-id part of the key for static-peer, and ensure it can take
a
> value that essentially means none. For example like this:
> 
>           list static-peer {
>             key "peer-ip out-vni-id";
> ...
>             leaf out-vni-id {
>               type union {
>                 type enumeration {
>                   enum none;
>                 }
>                 type uint32 {
>                   range "1..16777215";
>                 }
>               }
>               description
>                 "The ID of VNI for outbound. Do not support separate
deletion.";
>             }
> 
> 4. Repetition of config false
> 
> In many places within the module, there are config false statements inside
> elements that are already config false. While this is perfectly legal, it
is
> superfluous. Once a container or other element is marked config false,
> everything below that point will always be config false. The IETF
convention is
> to not write config false in more places than necessary.
> 
>       container peers {
>         config false;     <--- Everything within/below container peers is
now
>         config false description
>           "Operational data of remote NVE address in a VNI.";
>         list peer {
>           key "vni-id source-ip peer-ip";
>           config false;     <--- Superfluous
> 
> 5. Weak data format
> 
>       leaf up-time {
>         type string {
>           length "1..10";
>         }
>         config false;
>         description
>           "The continuous time as NVO3 tunnel is reachable.";
>       }
> 
> YANG strings are great when used for names that the client can choose
> arbitrarily. They are not so great when they encode information that is
not a
> name, unless there is a detailed description of the encoding. Since the
> encoding of the up-time leaf is not specified, the contents will not be
> interoperable. Either remove this leaf, or better, describe the format of
the
> contents so that it can be decoded. Changing the type from string to some
> time type might be a good option.
> 
> 6. Identities conventionally start with lowercase letters
> 
> By convention, all identities defined by IETF start with a lower case
letter.
> Citing the principle of least surprise, I would recommend changing
> 
>   identity Nve {
> 
> to
> 
>   identity nve {
> 
> 7. RPC or Action
> 
> In YANG 1.1, rpc:s that pertain to a certain element in the data tree can
be
> modeled as actions instead. This often leads to a more coherent model with
> less text. In this module, that seems to be the case for the two rpc:s
defined.
> Therefore I would suggest changing
> 
>   rpc reset-vni-peer-statistic {
>     description
>       "Clear traffic statistics about the VXLAN tunnel.";
>     input {
>       leaf vni-id {
>         type uint32 {
>           range "1..16777215";
>         }
>         mandatory true;
>         description
>           "The ID of the VNI.";
>       }
>       leaf peer-ip {
>         type inet:ip-address-no-zone;
>         mandatory true;
>         description
>           "The address of the remote NVE.";
>       }
>       leaf direction{
>         type direction-type;
>         mandatory true;
>         description
>           "Traffic statistics direction for the tunnel.";
>       }
>     }
>   }
> 
> to
> 
>         list statistic {
>           key "vni-id peer-ip direction"; ...
>           action reset-vni-peer-statistic {
>             description
>               "Clear traffic statistics about the VXLAN tunnel.";
>           }
> 
> Best Regards,
> /jan
> 
> 
> 
> _______________________________________________
> yang-doctors mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/yang-doctors