Subject: Re: [yang-doctors] YANG doctor review of draft-ietf-rtgwg-yang-rip-02

From: Jeff Tantsura <[email-masked]>
Date: Mon, 06 Feb 2017 16:52:40 -0800

Hi Lada,

Thanks for the review.
Does update address your concerns? 

Thanks!

Cheers,
Jeff

On 1/16/17, 13:35, "Xufeng Liu" <[email-masked]> wrote:

    Hi Lada,
    
    Thanks for the review. An updated version https://tools.ietf.org/html/draft-ietf-rtgwg-yang-rip-03 has just been posted to address most of these items.
    Please let us know for any further issues.
    
    Thanks,
    - Xufeng
    
    > -----Original Message-----
    > From: Ladislav Lhotka [mailto:[email-masked]]
    > Sent: Wednesday, December 7, 2016 11:01 AM
    > To: [email-masked]
    > Cc: [email-masked]
    > Subject: YANG doctor review of draft-ietf-rtgwg-yang-rip-02
    > 
    > Hi,
    > 
    > I was assigned to be the YANG doctor for this document. Here is my
    > review:
    > 
    > **** General comments
    > 
    > Overall, the module is in a good shape and integrates nicely with the core
    > routing data model [RFC 8022].
    > 
    > The I-D text is rather scarce, apart from the module it essentially contains only
    > boilerplate stuff. Some information about the context in which this module is
    > supposed to be used, design decisions etc. would be certainly useful.
    > 
    [Xufeng] 
    Added sections 2.1 to 2.7  to describe.
    
    > A non-normative appendix with a configuration example (instance data) would
    > also help people that are not fluent in YANG to understand what data are
    > included in this model.
    > 
    [Xufeng] 
    Added Appendix A. to show an example.
    
    > Some parts of the model – "distribute-list", "redistribute", "bfd",
    > "authentication" on interfaces (or at least the "key" list) – are probably not
    > specific to RIP and could be used by other protocols as well. If it is so, it would
    > be better to model them separately as general mechanisms.
    > 
    [Xufeng] 
    The "bfd" and "authentication"  sections have been re-used by importing the corresponding models. The ways that we use them are based on the recommendations from the corresponding models. For some portions of "distribute-list" and "redistribute", we previously re-used some references from https://tools.ietf.org/html/draft-ietf-rtgwg-policy-model-01, but that draft has expired and got out of synch with the other current IETF model drafts. Moreover, some of the "redistribute" are not common with other protocols, and may be better to specified in this protocol specific way.
    
    > Documents containing YANG modules that are imported by ietf-rip should
    > appear among normative references. One way to achieve this (which is also
    > useful by itself) is to include a table of namespace prefixes, see e.g.
    > 
    > https://tools.ietf.org/html/rfc8022#section-2.3
    [Xufeng] Added according the reference. Thanks.
    > 
    > **** Specific comments
    > 
    >      - Matching identities is done in the old YANG 1.0 way, for
    >        example:
    > 
    >        when "rt:type = 'rip:ripv2' or rt:type = 'rip:ripng'"
    > 
    >        This is fragile because the identities are matched based on
    >        string equality, so the result depends on the choice of the
    >        namespace prefix. Unless there is a strong reason not to use
    >        YANG 1.1, this is much simpler and more rubust
    > 
    >        when "derived-from(rt:type, 'rip:rip')"
    [Xufeng] Done. Thanks.
    > 
    >      - In "redistribution", the must expressions are outright broken,
    >        e.g. for IS-IS:
    > 
    >        must "../../../../../rt:control-plane-protocol"
    >        + "[rt:name = current()]/type = 'isis'" { ... }
    > 
    >        First, the ietf-rip module also needs to import ietf-isis, and
    >        then the must statement can be rewritten like so:
    > 
    >        must "derived-from-or-self("
    >        + "../../../../../rt:control-plane-protocol"
    >        + "[rt:name = current()]/type, 'isis:isis')"
    [Xufeng] Fixed.
    > 
    >      - Some descriptions could mention that the parameter (feature
    >        etc.) only applies to RIP. For example, in
    > 
    >        feature interface-statistics {
    >          description
    >            "This feature indicates that the system supports collecting
    >             per-interface statistic data.";
    >        }
    > 
    >        I would add "... related to RIP." at the end.
    [Xufeng] Changed for this and some other descriptions.
    > 
    >      - Maybe it's absolutely clear to experts but the name and
    >        description of the "neighbor-configuration" feature look like
    >        the feature can make neighbors remotely configurable. I had to
    >        look up where it is used to find out what's the real
    >        meaning. Perhaps something like "explicit-neighbors" might be
    >        easier to understand?
    [Xufeng] Changed.
    > 
    >      - The name of "no-supply" leaf looks strange. I checked a couple
    >        of CLIs and they use "passive", "passive-interface" or
    >        "send-options" for preventing RIP updates from being sent.
    [Xufeng] Changed.
    > 
    >      - In "split-horizon" leaf, I would use "poison-reverse" enum
    >        rather than "poison".
    [Xufeng] Fixed.
    > 
    > Lada
    > 
    > --
    > Ladislav Lhotka, CZ.NIC Labs
    > PGP Key ID: E74E8C0C

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 07 Feb 2017 13:37:49 +0100

Hi Jeff,

> Does update address your concerns?

Indeed it does.

Lada

-- 
Ladislav Lhotka, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67

---

From: Jeff Tantsura <[email-masked]>
Date: Tue, 07 Feb 2017 11:32:22 -0800

Thanks!
We are starting WGLC

Cheers,
Jeff

On 2/7/17, 04:37, "Ladislav Lhotka" <[email-masked]> wrote:

    Hi Jeff,

    
    Indeed it does.
    
    Lada