Subject: Re: I-D Action: draft-ietf-rtgwg-yang-vrrp-05.txt

From: Robert Wilton <[email-masked]>
Date: Mon, 16 Oct 2017 13:49:18 +0100

Hi draft-ietf-rtgwg-yang-vrrp draft authors,

I have a couple of comments on the latest revision of this draft:

1) The top level "vrrp container" has the right name, but can be "config 
false" since it doesn't hold any configuration today. This is noting 
that RFC 7950 allows the "vrrp container" to become "config true" as a 
backwards compatible change in future if required.

2) I've also noticed that this draft defined two separate versions of 
the VRRP YANG module.  The second version in the appendix is for 
pre-NMDA implementations:

    <CODE BEGINS> file "[email-masked]"
    module ietf-vrrp {
      yang-version 1.1;
      namespace "urn:ietf:params:xml:ns:yang:ietf-vrrp";
      prefix "vrrp";

And
    <CODE BEGINS> file "[email-masked]"
    module ietf-vrrp {
      yang-version 1.1;
      namespace "urn:ietf:params:xml:ns:yang:ietf-vrrp";
      prefix "vrrp";

Generically, I don't think that it is a good idea to define two versions 
of the same YANG module in one draft.

If a backwards compatible NMDA version of a module is required, then an 
extra "-state" module should be put in the appendix instead (e.g. 
[email-masked]).  This "-state" module could be used in 
conjunction with [email-masked] until NMDA implementations 
are available.

Alternatively, if you must define an existing pre NMDA version of the 
VRRP module then it should definitely be given a different module name, 
e.g. [email-masked].  But I believe that this 
would be an inferior solution since, compared to a separate "-state" 
module, it will make it harder for clients to migrate to the NMDA 
modules in future.

Finally, having actually read at the main VRRP module, then on the 
assumption that VRRP is always configured before it is used, then the 
"NMDA version" may well be sufficient to use on both existing NETCONF 
implementations and NMDA compatible NETCONF implementations.  The only 
thing that you can't see when using the NMDA version of the module on a 
"pre NMDA" implementation is the applied VRRP configuration.  Whether 
this is important enough to not define the extra "-state" module is 
unclear, but my instinct would be that it is better to just leave it out.

Thanks,
Rob

On 30/09/2017 15:12, [email-masked] wrote:
> A New Internet-Draft is available from the on-line Internet-Drafts directories.
> This draft is a work item of the Routing Area Working Group WG of the IETF.
>
>          Title           : A YANG Data Model for Virtual Router Redundancy Protocol (VRRP)
>          Authors         : Xufeng Liu
>                            Athanasios Kyparlis
>                            Ravi Parikh
>                            Acee Lindem
>                            Mingui Zhang
> 	Filename        : draft-ietf-rtgwg-yang-vrrp-05.txt
> 	Pages           : 68
> 	Date            : 2017-09-30
>
> Abstract:
>     This document describes a data model for Virtual Router Redundancy
>     Protocol (VRRP).  Both version 2 and version 3 of VRRP are covered.
>
>
> The IETF datatracker status page for this draft is:
> https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-vrrp/
>
> There are also htmlized versions available at:
> https://tools.ietf.org/html/draft-ietf-rtgwg-yang-vrrp-05
> https://datatracker.ietf.org/doc/html/draft-ietf-rtgwg-yang-vrrp-05
>
> A diff from the previous version is available at:
> https://www.ietf.org/rfcdiff?url2=draft-ietf-rtgwg-yang-vrrp-05
>
>
> Please note that it may take a couple of minutes from the time of submission
> until the htmlized version and diff are available at tools.ietf.org.
>
> Internet-Drafts are also available by anonymous FTP at:
> ftp://ftp.ietf.org/internet-drafts/
>
> _______________________________________________
> rtgwg mailing list
> [email-masked]
> https://www.ietf.org/mailman/listinfo/rtgwg
> .
>

---

From: Xufeng Liu <[email-masked]>
Date: Tue, 17 Oct 2017 13:57:09 +0000

Hi Rob,

Thanks for examining this and providing the comments.

From: Robert Wilton [mailto:[email-masked]]
Sent: Monday, October 16, 2017 8:49 AM
To: [email-masked]; [email-masked]
Cc: Benoit Claise (bclaise) <[email-masked]>; Alia Atlas <[email-masked]>
Subject: Re: I-D Action: draft-ietf-rtgwg-yang-vrrp-05.txt

1) The top level "vrrp container" has the right name, but can be "config false" since it doesn't hold any configuration today.  This is noting that RFC 7950 allows the "vrrp container" to become "config true" as a backwards compatible change in future if required.

[Xufeng] However, making the container “config false” would prevent vendor augmentations from adding nodes that are “config true”.

2) I've also noticed that this draft defined two separate versions of the VRRP YANG module.  The second version in the appendix is for pre-NMDA implementations:

   <CODE BEGINS> file "[email-masked]"<mailto:[email-masked]>
   module ietf-vrrp {
     yang-version 1.1;
     namespace "urn:ietf:params:xml:ns:yang:ietf-vrrp";
     prefix "vrrp";

And
   <CODE BEGINS> file "[email-masked]"<mailto:[email-masked]>
   module ietf-vrrp {
     yang-version 1.1;
     namespace "urn:ietf:params:xml:ns:yang:ietf-vrrp";
     prefix "vrrp";
Generically, I don't think that it is a good idea to define two versions of the same YANG module in one draft.

[Xufeng] This is to address Alia’s comment about “that there is an implementation. That implies that the existing model should be in the appendix.”
I think that such a case has not been thoroughly discussed. In the NMDA Guidelines, this case falls into category  (b).(“either an existing model or ...

”). The goal here is to provide a model that does not break the existing implementation, and can work with previous version of ietf-interfaces in RFC7223. For such a purpose, none of the following suggestions works too well.

If a backwards compatible NMDA version of a module is required, then an extra "-state" module should be put in the appendix instead (e.g. [email-masked]<mailto:[email-masked]>).  This "-state" module could be used in conjunction with [email-masked]<mailto:[email-masked]> until NMDA implementations are available.
[Xufeng] There is no such a parent root “-state” module in either RFC7223 or RFC7223bis, so we cannot create such a module cleanly, following the conventions in the guideline. If we did, such a module would break the current implementations.

Alternatively, if you must define an existing pre NMDA version of the VRRP module then it should definitely be given a different module name, e.g. [email-masked]<mailto:[email-masked]>.  But I believe that this would be an inferior solution since, compared to a separate "-state" module, it will make it harder for clients to migrate to the NMDA modules in future.
[Xufeng] Why is the different name necessary. The name “ietf-vrrp-split-tree” has some issues: 1) breaks the currently implementation; 2) the naming is arbitrary, without any conventional rules; 3) brings more confusions.
Why can’t we use the same name? with one older version and one newer version, just like ietf-interfaces with two versions (one in RFC7223 and RFC7223bis). The older version reflects the older (current) implementation working with RFC7223 without NMDA. When the vendor moves to NMDA and RFC7223bis, the newer version will be used.

Finally, having actually read at the main VRRP module, then on the assumption that VRRP is always configured before it is used, then the "NMDA version" may well be sufficient to use on both existing NETCONF implementations and NMDA compatible NETCONF implementations.  The only thing that you can't see when using the NMDA version of the module on a "pre NMDA" implementation is the applied VRRP configuration.  Whether this is important enough to not define the extra "-state" module is unclear, but my instinct would be that it is better to just leave it out.
[Xufeng] The “applied configuration” is a feature that could be useful, but not something that the VRRP model has to have. If it were not for being close to the existing implementations, we’d use only the NMDA model. The “pre NMDA” implementation uses ietf-interfaces in RFC7223, which has the -state branch. A matching VRRP model would be more consistent and convenient for such implementations. If we had only the NMDA model, we’d force the implementation to update to the NMDA VRRP model (and better with RFC7223bis). The older version of the non-NMDA model would not require the implementation to change immediately.
It is all about the existing implementations. We can have several levels of convenience for the implementations. If it is ok to require them to upgrade now, we can drop the non-NMDA version.
Thanks,
- Xufeng

Thanks,
Rob

On 30/09/2017 15:12, [email-masked]<mailto:[email-masked]> wrote:

        Authors         : Xufeng Liu

                          Athanasios Kyparlis

                          Ravi Parikh

                          Acee Lindem

                          Mingui Zhang

 Pages           : 68

 Date            : 2017-09-30

Abstract:

rtgwg mailing list

[email-masked]<mailto:[email-masked]>

---

From: Robert Wilton <[email-masked]>
Date: Tue, 17 Oct 2017 15:39:05 +0100

Hi Xufeng,

Thanks, some comments inline ...

On 17/10/2017 14:57, Xufeng Liu wrote:
> *From:*Robert Wilton [mailto:[email-masked]]
> *Sent:* Monday, October 16, 2017 8:49 AM
> *To:* [email-masked]; [email-masked]
> *Cc:* Benoit Claise (bclaise) <[email-masked]>; Alia Atlas 
> <[email-masked]>
> *Subject:* Re: I-D Action: draft-ietf-rtgwg-yang-vrrp-05.txt
>
> 1) The top level "vrrp container" has the right name, but can be 
> "config false" since it doesn't hold any configuration today.  This is 
> noting that RFC 7950 allows the "vrrp container" to become "config 
> true" as a backwards compatible change in future if required.
>
> */[Xufeng] However, making the container “config false” would prevent 
> vendor augmentations from adding nodes that are “config true”./*
>
This equivalent logic could apply to any "config false" node in any YANG 
data tree ;-)

The generic solution here is that the standard model should be "config 
false", and the vendor model should have a deviation on the top level 
VRRP container to make it "config true" and then augment it with the 
vendor configuration.

> <mailto:[email-masked]>
>    module ietf-vrrp {
>      yang-version 1.1;
> <mailto:[email-masked]>
>    module ietf-vrrp {
>      yang-version 1.1;
> Generically, I don't think that it is a good idea to define two 
> versions of the same YANG module in one draft.
>
> */[Xufeng] /**/This is to address Alia’s comment about “/*that there is an implementation. That implies that the existing model should be in the appendix.”
> */I think that such a case has not been thoroughly discussed. In the 
> NMDA Guidelines, this case falls into category  (b).(“/*either an existing model or ...
> */”). The goal here is to provide a model that does not break the 
> existing implementation, and can work with previous version of 
> ietf-interfaces in RFC7223. For such a purpose, none of the following 
> suggestions works too well./*
>
>
I think that existing implementations should just implement whichever ID 
version of the module that they have currently implement.  I don't think 
that means that IETF should have to publish that ID version as part of 
an RFC.  It isn't like the old revision goes away once the RFC has been 
published.

Your new NMDA compatible module doesn't have any direct dependency on 
RFC7223bis, i.e. when I tried it compile your NMDA version of the VRRP 
module it works just fine with the existing RFC 7223 version of 
ietf-interfaces.yang (i.e. rev 2014-05-08).  The VRRP state nodes would 
appear under the interfaces/interface part of the tree, but given that 
VRRP would always expect to be configured to have any state this should 
be fine, and is valid YANG.

>
> If a backwards compatible NMDA version of a module is required, then 
> an extra "-state" module should be put in the appendix instead (e.g. 
> [email-masked] 
> <mailto:[email-masked]>). This "-state" module could 
> be used in conjunction with [email-masked] 
> <mailto:[email-masked]> until NMDA implementations are 
> available.
>
> */[Xufeng] There is no such a parent root “-state” module in either 
> RFC7223 or RFC7223bis, so we cannot create such a module cleanly, 
> following the conventions in the guideline. If we did, such a module 
> would break the current implementations./*
>
When generating a state tree, special consideration needs to be applied 
for modules that have already been published with existing state trees.  
In that case, the generated "-state" module would augment 
interfaces-state/interface/ipv6.

These steps here might be slightly more accurate for generating a state 
module:
https://github.com/netmod-wg/FAQ/wiki/NMDA-Modelling-FAQ#Q4

> *//*
> VRRP module then it should definitely be given a different module 
> name, e.g. [email-masked] 
> <mailto:[email-masked]>. But I believe that this 
> */[Xufeng] Why is the different name necessary. The name 
> “ietf-vrrp-split-tree” has some issues: 1) breaks the currently 
> implementation; 2) the naming is arbitrary, without any conventional 
> rules; 3) brings more confusions./*
>
I just think that it is very confusing for a single draft to publish two 
module revisions with the same name.  Particularly given that two 
modules don't follow the YANG upgrade rules.  Tooling like RFC strip 
will automatically extract both revisions etc.  The name doesn't need to 
be "ietf-vrrp-split-tree", just any unique name. But as mentioned below, 
I don't think that you should need to publish it at all.

> *//*
>
> */Why can’t we use the same name? with one older version and one newer 
> version, just like ietf-interfaces with two versions (one in RFC7223 
> and RFC7223bis). The older version reflects the older (current) 
> implementation working with RFC7223 without NMDA. When the vendor 
> moves to NMDA and RFC7223bis, the newer version will be used./*
>
The version in RFC7223bis is a backwards compatible upgrade to the 
existing RFC7223 version.  I.e. it hasn't removed any existing nodes, it 
had just marked part of the tree as deprecated.

I don't see the benefit in putting the old revision into the RFC, it was 
just a working draft version of the model, a foot note in history on the 
path of standardization.

> thing that you can't see when using the NMDA version of the module on 
> a "pre NMDA" implementation is the applied VRRP configuration.  
> Whether this is important enough to not define the extra "-state" 
> module is unclear, but my instinct would be that it is better to just 
> leave it out.
>
> */[Xufeng] The “applied configuration” is a feature that could be 
> useful, but not something that the VRRP model has to have. If it were 
> not for being close to the existing implementations, we’d use only the 
> NMDA model. The “pre NMDA” implementation uses ietf-interfaces in 
> RFC7223, which has the -state branch. A matching VRRP model would be 
> more consistent and convenient for such implementations. If we had 
> only the NMDA model, we’d force the implementation to update to the 
> NMDA VRRP model (and better with RFC7223bis). The older version of the 
> non-NMDA model would not require the implementation to change 
> immediately./*
>
> */It is all about the existing implementations. We can have several 
> levels of convenience for the implementations. If it is ok to require 
> them to upgrade now, we can drop the non-NMDA version./*
>
I don't think that we have to require them to upgrade now, I think that 
they should use whichever pre-standard version they are currently using, 
but I wouldn't put this into the RFC (not even in the appendix), it will 
just cause confusion in the long term.

E.g. for a protocol draft you wouldn't expect a RFC to describe other 
pre-standard specifications before it was standardized.  I'm not sure 
why YANG modules should be treated any differently.

Thanks,
Rob

> *//*
> */Thanks,
> - Xufeng/*
> *//*
>     [email-masked] <mailto:[email-masked]>
>