Re: [yang-doctors] Yangdoctors last call review of draft-ietf-bfd-yang-09

"Reshad Rahman (rrahman)" <rrahman@cisco.com> Sun, 25 February 2018 16:28 UTC

Return-Path: <rrahman@cisco.com>
X-Original-To: yang-doctors@ietfa.amsl.com
Delivered-To: yang-doctors@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 79FBE126CF9; Sun, 25 Feb 2018 08:28:09 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -14.531
X-Spam-Level:
X-Spam-Status: No, score=-14.531 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, RCVD_IN_DNSWL_HI=-5, RCVD_IN_MSPIKE_H3=-0.01, RCVD_IN_MSPIKE_WL=-0.01, SPF_PASS=-0.001, T_RP_MATCHES_RCVD=-0.01, USER_IN_DEF_DKIM_WL=-7.5] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=cisco.com
Received: from mail.ietf.org ([4.31.198.44]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id QLf6r2OQqQvA; Sun, 25 Feb 2018 08:28:07 -0800 (PST)
Received: from rcdn-iport-4.cisco.com (rcdn-iport-4.cisco.com [173.37.86.75]) (using TLSv1.2 with cipher DHE-RSA-SEED-SHA (128/128 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id CCC791201FA; Sun, 25 Feb 2018 08:28:06 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=cisco.com; i=@cisco.com; l=18344; q=dns/txt; s=iport; t=1519576086; x=1520785686; h=from:to:cc:subject:date:message-id:references: in-reply-to:content-id:content-transfer-encoding: mime-version; bh=TxH0pGppZtl3l6e9wHCvGYxM18/55MScxfHQpbvw0Ak=; b=AWZlodLiUgt/oXcLGgbnADDVIAxzwpiXZVrViHdUoL0Y6svKmgTLRyuT Q2alDKp+BN15nFNW0V3UEYIUzl+0E/7l7/XKes25MAEDRgAnHnb8gyDWf o2T/gDaK7VnuAmNrM4ehM/vwVrOUtju99G3NqMy8D5W3hQB+3wzrtOnAs k=;
X-IronPort-Anti-Spam-Filtered: true
X-IronPort-Anti-Spam-Result: =?us-ascii?q?A0A5AQAi45Ja/4YNJK1cGQEBAQEBAQE?= =?us-ascii?q?BAQEBAQcBAQEBAYMeMWZwKAqDSooijXuDGJYFFIICCiOFEAIagh5UGAECAQE?= =?us-ascii?q?BAQEBAmsohSQGIxE+BxACAQgQCgImAgICMBUQAgQBDQUbhHUQq0GCJ4highQ?= =?us-ascii?q?BAQEBAQEBAQEBAQEBAQEBAQEBAQEYBYEPhjOBV4FmKYMEgy4CAwGBMggBEgE?= =?us-ascii?q?fFyECglMxghQgBYdYiVaQMwkChzeHO4UsggaFZop0iWeCaoh3AhEZAYEuAR4?= =?us-ascii?q?4JTtxcBUZISoBghiCQxyBegF3iioNGAeBA4EXAQEB?=
X-IronPort-AV: E=Sophos;i="5.47,393,1515456000"; d="scan'208";a="360139750"
Received: from alln-core-12.cisco.com ([173.36.13.134]) by rcdn-iport-4.cisco.com with ESMTP/TLS/DHE-RSA-AES256-GCM-SHA384; 25 Feb 2018 16:28:05 +0000
Received: from XCH-RCD-001.cisco.com (xch-rcd-001.cisco.com [173.37.102.11]) by alln-core-12.cisco.com (8.14.5/8.14.5) with ESMTP id w1PGS5RE029695 (version=TLSv1/SSLv3 cipher=AES256-SHA bits=256 verify=FAIL); Sun, 25 Feb 2018 16:28:05 GMT
Received: from xch-rcd-005.cisco.com (173.37.102.15) by XCH-RCD-001.cisco.com (173.37.102.11) with Microsoft SMTP Server (TLS) id 15.0.1320.4; Sun, 25 Feb 2018 10:28:04 -0600
Received: from xch-rcd-005.cisco.com ([173.37.102.15]) by XCH-RCD-005.cisco.com ([173.37.102.15]) with mapi id 15.00.1320.000; Sun, 25 Feb 2018 10:28:04 -0600
From: "Reshad Rahman (rrahman)" <rrahman@cisco.com>
To: =?utf-8?B?SsO8cmdlbiBTY2jDtm53w6RsZGVy?= <j.schoenwaelder@jacobs-university.de>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "rtg-bfd@ietf.org" <rtg-bfd@ietf.org>, "draft-ietf-bfd-yang.all@ietf.org" <draft-ietf-bfd-yang.all@ietf.org>
Thread-Topic: Yangdoctors last call review of draft-ietf-bfd-yang-09
Thread-Index: AQHTpkBKVunZZ+hp3EiObmH4QVr8yqOyUdqAgAMdoIA=
Date: Sun, 25 Feb 2018 16:28:04 +0000
Message-ID: <9A6B372F-2FD1-409A-BF3B-AFF48D1E74B4@cisco.com>
References: <151868731494.7525.9572645824096052010@ietfa.amsl.com> <6A04AE1F-F538-40CD-BFB4-3452B50C7F9D@cisco.com>
In-Reply-To: <6A04AE1F-F538-40CD-BFB4-3452B50C7F9D@cisco.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
user-agent: Microsoft-MacOutlook/10.a.0.180210
x-ms-exchange-messagesentrepresentingtype: 1
x-ms-exchange-transport-fromentityheader: Hosted
x-originating-ip: [10.86.249.164]
Content-Type: text/plain; charset="utf-8"
Content-ID: <B6DEE96072A73A43B57D052185FE398D@emea.cisco.com>
Content-Transfer-Encoding: base64
MIME-Version: 1.0
Archived-At: <https://mailarchive.ietf.org/arch/msg/yang-doctors/fbGac35iB52gs7c-NedtA19IvPs>
Subject: Re: [yang-doctors] Yangdoctors last call review of draft-ietf-bfd-yang-09
X-BeenThere: yang-doctors@ietf.org
X-Mailman-Version: 2.1.22
Precedence: list
List-Id: Email list of the yang-doctors directorate <yang-doctors.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/yang-doctors/>
List-Post: <mailto:yang-doctors@ietf.org>
List-Help: <mailto:yang-doctors-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/yang-doctors>, <mailto:yang-doctors-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 25 Feb 2018 16:28:09 -0000

Hi,

Regarding the following, making the change is easy but ietf-bfd-types is used by other modules so this will impact these other modules. E.g. the RIP draft (https://datatracker.ietf.org/doc/draft-ietf-rtgwg-yang-rip/) is currently in the RFC Editor queue, I presume it's too late then to make this name change?

Regards,
Reshad.

        * BFD types YANG Module
        
          - <snip> In fact, ietf-bfd-types is kind of a
            misnomer; perhaps this should be ietf-bfd-common (the -common was
            used in RFC 8194 but I am biased here).
    <RR> Will rename to -common


On 2018-02-23, 11:53 AM, "Reshad Rahman (rrahman)" <rrahman@cisco.com> wrote:

    Hi Jürgen,
    
    Thank you for the review, response inline.
    
    On 2018-02-15, 4:35 AM, "Jürgen Schönwälder" <j.schoenwaelder@jacobs-university.de> wrote:
    
        Reviewer: Jürgen Schönwälder
        Review result: Not Ready
        
        Review of draft-ietf-bfd-yang-09.txt.
        
        * General comments
        
          - Having requirements language below the abstract looks like a novel
            idea, I assume the RFC editor will edit this. Also note that
            nowadays authors are usually expected to cite RFC 8174 as well
            with the extended boilerplate text.
    <RR> Will make the changes.
        
          - Update 2017 to 2018 in copyright statements etc.
    <RR> Will make the changes.
    
          - References to RFC 7223, RFC 7277, RFC 8022 should be updated to
            references to the I-Ds replacing them (sitting in the RFC editor
            queue). This may also involve changes in the YANG model.
    <RR> Will make the changes. I don't think this will involve changes in the YANG model, but will pay attention to this.
        
          - State whether the model is NMDA compliant (which it likely should
            be), see also previous item.
    <RR> This is mentioned in section 2. I will look into moving this to the intro and changing the text to clarify.
        
          - I am not sure why you want to cite I-D.dsdt-nmda-guidelines. Would
            it not make more sense to cite the NMDA specification?
    <RR> Will make the change. By NMDA specification, you're referring to draft-ietf-netmod-revised-datastores?
        
          - There are some YANG validation errors that should be addressed (see
            the link on the datatracker).
    <RR> Those were in 8022bis I believe, gone now.
        
          - References YANG modules must be in the references and there must
            be citations in the text, hence there is the common phrase "This
            YANG module imports <bla> [RFCwxyz] and ...."
        
          - We generally prefer
        
              reference "RFC 6991: Common YANG Data Types"
        
            over
        
              reference "RFC 6991"
        
            since not everybody remembers all the RFC numbers (add the RFC
            title after the RFC number, separated by a colon). In some places
            you actually use the syntactic format but you do not use the RFC
            title. Please make this consistent, following the usual
            conventions.
    <RR> Will make the change. 
        
          - I have raised a question on yang-doctors concerning the pattern
        
              import ietf-inet-types {
                prefix "inet";
                reference "RFC 6991";
              }
        
            and whether this should perhaps be
        
              import ietf-inet-types {
                prefix inet;
                reference "RFC 6991: Common YANG Data Types
                           (at the time of this writing)";
              }
     <RR> Saw the discussion, will make the change once there is closure.
    
        * Design of the Data Model
        
          - Do I always have to use schema mount to use these YANG models? If
            so, one might consider I-D.ietf-netmod-schema-mount a normative
            reference. Are you not augmenting the routing model?
      <RR> I will clarify in the text. There was a follow-up email discussion on this, we are augmenting the routing model and schema mount can be used (but not mandatory).
      
          - I do not understand the explanations how the groupings solve the
            problem that IGPs are moving targets (they come and go). How do
            the groupings help the operator to configure BFD parameters for
            peers they do not know about yet?
    <RR> I will update the text. Basically as IGPs discover peers/neighbors, they create BFD sessions "internally" (i.e. via API, not via config).
        
          - How does a client know which choices of the "min-interval", used
            for both transmit and receive intervals, and "desired-min-tx-
            interval" and "required-min-rx-interval" are supported by an
            implementation?
     <RR> We'll think about this. We'll either add feature or make both mandatory.
       
          - The phrase 'operational model' probably means 'operational state
            model' and 'operational items' probably means 'operational state
            data'.
    <RR> Will make the change. 
        
          - You have summary information and detailed BFD session information.
            What would an implementation report if say access to some BFD
            sessions is restricted by access control? Would information about
            them still leak through the summary information? I assume so that
            this may be practically the way to do things but perhaps this
            needs to be mentioned in the security considerations.
    <RR> The summary information contains everything. I don't see what kind of security consideration is required if we know e.g. that we have so many sessions up and down.
        
          - In 2.3, you use 'clients of BFD' but I think this is very
            different from 'BFD clients'. Please clarify the terminology.
    <RR> Will use 'BFD clients' everywhere.
        
          - s/operational data/operational state data/g
    <RR> Yes
        
          - The *-count leafs seem to be gauge32, should yang-types:gauge32 be
            used?
    <RR> Yes
        
          - There are also real counters that should probably use
            yang-types:counter32 and yang-types:counter64 instead of uint32
            and uint64.
    <RR> Yes
        
          - Is [I-D.ietf-mpls-base-yang] not a normative reference? See text
            in 2.11.3.
    <RR> Yes
        
          - Is [I-D.ietf-teas-yang-te] not a normative reference? See text in
            2.11.4.
    <RR> Yes
        
        * IANA BFD YANG Module
        
          - Fix the phrase "a collection of YANG data types considered defined
            by IANA".
        
          - Does IANA understand how the typedefs diagnostic and auth-type is
            to be managed?  Does this relate to an existing registry? Or is
            this establishing a diagnostic registry? Should the last paragraph
            in more clearly spell out that any changes to the existing
            registry must lead to an update of the YANG module and that
            updates to the YANG module are not allowed without an update to
            the other registries? The current wording "intended to reflect"
            seems vague. Should the description text for the typedefs make an
            explicit reference to the IANA registry for these number spaces?
    <RR> This is for an existing registry. I will see how to modify the text.
        
        * BFD types YANG Module
        
          - The statement "This module contains a collection of BFD specific
            YANG data type definitions" seems wrong since you define way more
            things that just datatypes. In fact, ietf-bfd-types is kind of a
            misnomer; perhaps this should be ietf-bfd-common (the -common was
            used in RFC 8194 but I am biased here).
    <RR> Will rename to -common
        
          - s/Two interval values or 1 value/Two interval values or one value/
    <RR> Yes
        
          - I think this is unclear (for me):
        
              leaf down-count {
                 type uint32;
                 description "Session Down Count.";
               }
        
            Is this a counter counting how many times the BFD session was
            down? The terse description does not tell. If this is a counter,
            then make it clear by using yang:counter32:
        
              leaf down-count {
                 type yang:counter32;
                 description
                   "The number of times a BFD session transitioned into
                    the down state.";
               }
        
            Please describe clearly what is being counted. Perhaps my
            interpretation is wrong, then please insert the correct statement.
            Note that this comment also applies to several of the subsequent
            definitions.
    <RR> Yes it is for number of times the session has gone down. I will look at the subsequent descriptions also and will add clarifications.
        
          - Clarify counter relationships. Right now, I assume that
            admin-down-count is included in down-count (but my interpretation
            may also be wrong). Same for received/send and received/send bad
            packets.
    <RR>  admin-down-count is not included in down-count. For packet counters, yes the bad packet count is included in total packet count. Will clarify.
        
          - You have a container session-statistics and a grouping
            session-statistics and it seems they count very different things,
            the first seems to have statistics of stuff happening within a
            session (per session statistics) and the later seems to have
            statistics across all sessions. This is a bit unfortunate, if you
            search of session statistics you find stuff that leaves you
            puzzled. Please avoid this name clash and also make sure the
            description of the leafs makes it clear whether it is a per
            session leaf or a leaf for all sessions.
    <RR> Correct. I'll rename the container which is "across sessions" to summary or session-summary.
        
          - The *count leafs in "session-statistics" (ha) seem to be of
            type yang:gauge32, i.e., I would write something along these
            lines:
        
               leaf sessions {
                 type yang:gauge32;
                 description "Number of BFD sessions.";
               }
        
               leaf sessions-up {
                 type yang:gauge32;
                 description "Number of BFD sessions that are up.";
               }
        
               [...]
    <RR> Yes the "count" in session-statistics are actually gauges and should be changed (and -count taken out from the name).
     
        * BFD IP multihop YANG Module
        
          - This is indented differently. Well the RFC editor will fix I
            guess.
    <RR> Will fix.
        
        * Examples
        
          - I have not validated the examples. I do not know whether the IETF
            tooling is meanwhile able to do this - likely not. Did the authors
            confirm that they automatically validate the examples? Well,
            looking at the namespaces, likely not (the augmentations do not
            live in the ietf-routing namespace). So these examples need to be
            validated and fixed. I have used yanglint for this, works better
            for me than the pyang solutions, but the authors should figure out
            what works for them.
    <RR> I didn't verify them.. I was having issues with the verification tools. The examples will be verified.
        
          - There are special IP address blocks for examples; the IPv6 address
            you show seems to belong to APNIC...
    <RR> I'll use IP addresses from the example address block.
        
        * Security Considerations
        
          - Needs to be updated to the latest boilerplate.
    <RR> Ack.
        
          - You should discuss security properties of objects, there is more
            work to do here.
    <RR> Will look into this.
        
        * Appendix A
        
          - I do not understand what is going on here, I think this needs a
            bit more explanatory text. Why is the informal description of the
            two parameters more detailed than the description in the example
            YANG module? I suggest to have a proper description in the YANG
            module only.
        <RR> Will add text. Goal was to add an example of how echo could be configured.
    
        * Appendix B
        
          - What is an area-id? The description is not helpful.
        
          - list interface [...] description "List of interfaces" is not
            really useful.
    <RR> Considering removing this appendix.