Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-isis-cfg-24

tom petch <ietfc@btconnect.com> Tue, 27 November 2018 17:13 UTC

Return-Path: <ietfc@btconnect.com>
X-Original-To: lsr@ietfa.amsl.com
Delivered-To: lsr@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id E11471292AD; Tue, 27 Nov 2018 09:13:34 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: 1.738
X-Spam-Level: *
X-Spam-Status: No, score=1.738 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_MED=-1.459, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, RATWARE_MS_HASH=2.148, RATWARE_OUTLOOK_NONAME=2.95, RCVD_IN_DNSWL_NONE=-0.0001, SPF_PASS=-0.001] autolearn=no autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=btconnect.onmicrosoft.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 fVTf97tIo93Y; Tue, 27 Nov 2018 09:13:32 -0800 (PST)
Received: from EUR01-HE1-obe.outbound.protection.outlook.com (mail-eopbgr130104.outbound.protection.outlook.com [40.107.13.104]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 678DE126BED; Tue, 27 Nov 2018 09:13:31 -0800 (PST)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=btconnect.onmicrosoft.com; s=selector1-btconnect-com; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=l9Tx2TgRAGWJvl6LQVQ8kFeKg/N24NERplW82paa/34=; b=ZgGBhYcVAnZuWPJg9RGbCk4kh6DUspJW7w9y3FJU6+p5ogK9wqb8sGPM+scSQeAoXDaVi9uIrYjdVGw3PwjD4+1R5aAbfXh2BeBia1ihJOUfkmsoMalr4ZGNYlgG5LN+9zUxJZzJWIekejsMusWmRUeQi1V9O53GnHL97lzLLx8=
Received: from VI1PR07MB4717.eurprd07.prod.outlook.com (20.177.54.82) by VI1PR07MB3454.eurprd07.prod.outlook.com (10.175.244.144) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.1382.14; Tue, 27 Nov 2018 17:13:28 +0000
Received: from VI1PR07MB4717.eurprd07.prod.outlook.com ([fe80::1575:d33b:33dd:c7c4]) by VI1PR07MB4717.eurprd07.prod.outlook.com ([fe80::1575:d33b:33dd:c7c4%5]) with mapi id 15.20.1382.012; Tue, 27 Nov 2018 17:13:28 +0000
From: tom petch <ietfc@btconnect.com>
To: "stephane.litkowski@orange.com" <stephane.litkowski@orange.com>, Ebben Aries <exa@juniper.net>, "yang-doctors@ietf.org" <yang-doctors@ietf.org>
CC: "draft-ietf-isis-yang-isis-cfg.all@ietf.org" <draft-ietf-isis-yang-isis-cfg.all@ietf.org>, "lsr@ietf.org" <lsr@ietf.org>, "Tarek Saad (tsaad)" <tsaad@cisco.com>
Thread-Topic: [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-isis-cfg-24
Thread-Index: AQHUhkHafKiWVvfkhUKDIB9kEUVysw==
Date: Tue, 27 Nov 2018 17:13:28 +0000
Message-ID: <009501d48674$37c9fe20$4001a8c0@gateway.2wire.net>
References: <154025553381.13801.5009678921928527816@ietfa.amsl.com> <03ff01d48641$8f8d8600$4001a8c0@gateway.2wire.net> <19850_1543334259_5BFD6973_19850_302_6_9E32478DFA9976438E7A22F69B08FF924B7768B5@OPEXCLILMA4.corporate.adroot.infra.ftgroup>
Accept-Language: en-GB, en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
x-clientproxiedby: CWXP265CA0026.GBRP265.PROD.OUTLOOK.COM (2603:10a6:400:2d::14) To VI1PR07MB4717.eurprd07.prod.outlook.com (2603:10a6:803:69::18)
authentication-results: spf=none (sender IP is ) smtp.mailfrom=ietfc@btconnect.com;
x-ms-exchange-messagesentrepresentingtype: 1
x-originating-ip: [86.139.215.184]
x-ms-publictraffictype: Email
x-microsoft-exchange-diagnostics: 1; VI1PR07MB3454; 6:/y21ThI/2vw5jU8w5/goT4HFIOsax6SzGk1orzcM7gZHUO2OE/iyqFYjPsJ4oDCfKvzc+6lDlb6/pLgrYjfW8KLoJSdIPEaZe0XEYsyfyzcnFweDpNAaLEkqvWD4AEORW0qey1ayXzA4Y5kJcWqtL0oPkyW5Vqko/xLhoncLNNPx2nTEcNKZe/7nDXJvAHWwsael9gfxLrdBHFWLCiPvYjFbtdvGKhZ6SgCaIljFIre8SN61nh3ypGDgFYoWPyCouLu2rZEId62LeKQR9aiZ2hbk9+OQUUDiGP9ZTHLD95htvx4fQFoTZIICiqeg2RozS//QVIV60xH/YNmZLF82QnafOxgt/NfW2ORziSmN/e6KqkdKZNZwwc9u28usz2TXI/o8pD2+3okSv2mQQyCzYr2TDWuHUiVe4M1y0Jjhs/iZzcu0mcZ0aF7XvbWGFZ6B/8Xd7UF9WIzVgsqk06np7A==; 5:VxkCyFPrBlskHnR/ngHOo6HYAaT1/blvLr0hWn31SXjQxdctqKDJ38QXV3HlVCXWnIOVBXCOcZ3+WFkZP8LjuSNV3DzwOSmtEjmMfMa5gXbcLEU3cm+CXAeT0IR5VsM1M7oJKuZuVY1qyl41UScsCusyBzngG1VEDNY6Ig8MKXw=; 7:A3un1LWmrhly+RIV7x4VZ2cxreSZpyioAt7eBhhs70kpjuyn+yTCmrxTWOlO/skCpAqitRxfBcCiYZbiIFrorUOmlqKTmDfn3XrtQEX22nSglBz+D0t6Y0rtFFzh5DXOIcc+iO9pas9bnQAjoHYIWA==
x-ms-office365-filtering-correlation-id: 496e68a9-cafd-499b-18ff-08d6548ba5ce
x-microsoft-antispam: BCL:0; PCL:0; RULEID:(2390098)(7020095)(4652040)(8989299)(5600074)(711020)(4534185)(4627221)(201703031133081)(201702281549075)(8990200)(2017052603328)(7193020); SRVR:VI1PR07MB3454;
x-ms-traffictypediagnostic: VI1PR07MB3454:
x-microsoft-antispam-prvs: <VI1PR07MB345443E507EBBFA5AF3F542BA0D00@VI1PR07MB3454.eurprd07.prod.outlook.com>
x-ms-exchange-senderadcheck: 1
x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(6040522)(2401047)(5005006)(8121501046)(93006095)(93001095)(3002001)(10201501046)(3231443)(944501410)(52105112)(6055026)(148016)(149066)(150057)(6041310)(20161123562045)(20161123560045)(20161123564045)(20161123558120)(201703131423095)(201702281528075)(20161123555045)(201703061421075)(201703061406153)(201708071742011)(7699051)(76991095); SRVR:VI1PR07MB3454; BCL:0; PCL:0; RULEID:; SRVR:VI1PR07MB3454;
x-forefront-prvs: 086943A159
x-forefront-antispam-report: SFV:NSPM; SFS:(10019020)(366004)(136003)(376002)(346002)(396003)(39860400002)(51444003)(199004)(189003)(13464003)(110136005)(54906003)(6116002)(3846002)(25786009)(66066001)(84392002)(106356001)(2906002)(105586002)(14454004)(446003)(102836004)(316002)(2501003)(478600001)(476003)(97736004)(99286004)(26005)(186003)(486006)(81156014)(386003)(6506007)(86362001)(6436002)(76176011)(7736002)(53936002)(5660300001)(14496001)(81166006)(44736005)(6306002)(9686003)(6512007)(68736007)(86152003)(8936002)(33896004)(305945005)(4744004)(1556002)(71190400001)(256004)(14444005)(71200400001)(5024004)(229853002)(4326008)(1941001)(6486002)(6246003)(53546011)(8676002)(966005)(52116002); DIR:OUT; SFP:1102; SCL:1; SRVR:VI1PR07MB3454; H:VI1PR07MB4717.eurprd07.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:0; MX:1;
received-spf: None (protection.outlook.com: btconnect.com does not designate permitted sender hosts)
x-microsoft-antispam-message-info: p5t7DbkA4AhHYSde8YXQ5e397b3wBNCUu+R0yQmdfm3HRCK61Fxgo/5jQMFcWP28k9Ki3uToPCJzQihMbK9vIIh+UILXUGmaWOQrNLMLyCNHKjcYCnweJaJvRLKN24bmFEmCRT8QkNeKwJKBrv5IZmujjfbALNNyaG2tZKGIbGkf1i+9YN5Nxf+lXckjVxaO3XkZHnv0EDo44qOfIwH9nsWqgLrfOn2H0fOQpKOd7M/jRdnC2ISCdz4eKLuNi66zJ0+CmYOE3FSPQrI+GOU3tFqsjocQ9D97mXGlCH/dxf1brLT1DJ4IItQ3Yms7j3g/ECxztGTZphNLjtbkoN5K31/b8LPCrrfp8QNJo+6bAgk=
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="iso-8859-1"
Content-ID: <F6B16C030238AC4E81A5282123143DBA@eurprd07.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: btconnect.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 496e68a9-cafd-499b-18ff-08d6548ba5ce
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Nov 2018 17:13:28.1450 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: cf8853ed-96e5-465b-9185-806bfe185e30
X-MS-Exchange-Transport-CrossTenantHeadersStamped: VI1PR07MB3454
Archived-At: <https://mailarchive.ietf.org/arch/msg/lsr/PVxMR3sprBcMP5NUMttewVGZyy4>
Subject: Re: [Lsr] Yangdoctors last call review of draft-ietf-isis-yang-isis-cfg-24
X-BeenThere: lsr@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Link State Routing Working Group <lsr.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/lsr>, <mailto:lsr-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/lsr/>
List-Post: <mailto:lsr@ietf.org>
List-Help: <mailto:lsr-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/lsr>, <mailto:lsr-request@ietf.org?subject=subscribe>
X-List-Received-Date: Tue, 27 Nov 2018 17:13:35 -0000

---- Original Message -----
From: <stephane.litkowski@orange.com>
Sent: Tuesday, November 27, 2018 3:57 PM

Hi Tom,

Thanks for your comments. I will fix them asap.
Regarding:
" Line length is within the RFC limit but the effect is to spread many
of the description clauses over multiple lines with indentation of 56
characters, not user friendly e.g.
                                        description
                                                "List of max LSP
bandwidths for different
                                                 priorities.";
"
What's your suggestion on this one ?

<tp>

Fix it:-)

having a sentence of text jump about the page makes it hard to read.  A
worse example is
" leaf flags {
                                type bits {
                                        bit A {
                                                position 7;
                                                description
                                                        "The A bit
represents the Anomalous (A) bit.
                                                        The A bit is
set when the measured value of
                                                        this parameter
exceeds its configured
                                                        maximum
threshold.
                                                        The A bit is
cleared when the measured value
                                                        falls below its
configured reuse threshold.
                                                        If the A bit is
clear,
                                                        the value
represents steady-state link performance.";
                                        }
                                }
"

where each line of the description is split into two with part starting
indented 56 characters and the other part not indented at all.

Mahesh commented on
draft-ietf-pce-pcep-yang-08
recently that
"- The module uses indentation that varies all over the module, from 2
spaces to
5. Please fix the module to have consistent indentation."

Well, this I-D is worse by an order of magnitude so I think needs
fixing.

Tom Petch


Brgds,

-----Original Message-----
From: tom petch [mailto:ietfc@btconnect.com]
Sent: Tuesday, November 27, 2018 12:11
To: Ebben Aries; yang-doctors@ietf.org
Cc: draft-ietf-isis-yang-isis-cfg.all@ietf.org; lsr@ietf.org; Tarek Saad
(tsaad)
Subject: Re: [Lsr] Yangdoctors last call review of
draft-ietf-isis-yang-isis-cfg-24

Some quirks in-25

I see lots of YANG reference statements - good - but no mention of them
in the I-D references - not so good.  My list is

5130
5305
5306
5880
5881
6119
6232
7794
7810
7917
8405

Also perhaps
OLD
    reference "RFC XXXX - YANG Data Model for Bidirectional
               Forwarding Detection (BFD).Please replace YYYY with
                                                         published RFC
number for draft-ietf-bfd-yang.";

NEW
    reference "RFC YYYY - YANG Data Model for Bidirectional
               Forwarding Detection (BFD).

-- Note to RFC Editor Please replace YYYY with published RFC
number for draft-ietf-bfd-yang.";

OLD
      reference "draft-ietf-bfd-yang-xx.txt:
                 YANG Data Model for Bidirectional Forwarding
                 Detection (BFD)";
NEW
    reference "RFC YYYY - YANG Data Model for Bidirectional
               Forwarding Detection (BFD).

-- Note to RFC Editor Please replace YYYY with published RFC
number for draft-ietf-bfd-yang.";


Line length is within the RFC limit but the effect is to spread many of
the description clauses over multiple lines with indentation of 56
characters, not user friendly
e.g.
                                        description
                                                "List of max LSP
bandwidths for different
                                                 priorities.";


Acknowledgements is TBD. I note that the editor list of the YANG module
is somewhat longer than the editor list of the I-D.

I note that the augmentation of interfaces seems to have no conditional
and so will augment all interfaces. I think that this is a generic issue
but do not see it being addressed anywhere.

In a similar vein, you are defining MPLS objects and I am unclear
whether or not those should be conditional, or part of the MPLS YANG
modules or both (copying Tarek for this)

Tom Petch

----- Original Message -----
From: "Ebben Aries" <exa@juniper.net>
Sent: Tuesday, October 23, 2018 12:45 AM

> Reviewer: Ebben Aries
> Review result: On the Right Track
>
> 1 module in this draft:
> - ietf-isis@2018-08-09.yang
>
> No YANG compiler errors or warnings (from pyang 1.7.5 and yanglint
0.16.54)
>
> "ietf-isis@2018-08-09" module is compatible with the NMDA
architecture.
>
> Module ietf-isis@2018-08-09.yang:
> - Both the description and the draft name reference that this module
is
>   specific to configuration but contains operational state nodes in
addition
>   to RPCs and notifications.  Any wording suggesting this is only
>   configuration should be changed
> - Module description must contain most recent copyright notice per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.1
> - Module description reads "common across all of the vendor
implementations".
>   I don't think this needs to be called out as such as that is the
overall
>   intention of *all* IETF models
> - This module contains '22' features (and the respective OSPF module
currently
>   contains '26').  While it is understood the purpose of these
features in the
>   module, take precaution as to complexity for clients if they need to
>   understand >= quantity of features per module in use on a
>   network-element.  We are going to end up w/ feature explosion to
convey
>   *all* possible features of each network-element leading to
divergence back
>   towards native models at the end of the day.  A large amount of
these
>   feature names could be defined within a more global namespace (e.g.
nsr) but
>   this gives us a granular yet cumbersome approach (e.g. feature
isis:nsr,
>   ospf:nsr, etc..)
> - RPC 'clear-adjacency' does not have any input leaf that covers
clearing a
>   specific neighbor/adjacency (See comments below as well regarding
RPC
>   alignment w/ the OSPF model)
> - RPC 'clear-adjacency' has an input node of 'interface' however this
is just
>   a string type.  Is there any reason this is not a
leafref/if:interface-ref
>   (much like in the OSPF model)
> - Child nodes within a container or list SHOULD NOT replicate the
parent
>   identifier per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.3.
1.
>
>   A case in point is the list /afs/af that has a leaf of 'af'
>       <afs>
>         <af>
>           <af>ipv4</af>
>           <enable>true</enable>
>         </af>
>       </afs>
>
>   Not only is this replication, but we should likely not abbreviate
'afs' if
>   we are using the expanded 'address-family' in other IETF models such
as
>   ietf-i2rs-rib
>
>
> General comments on the draft + nits:
> - Since YANG tree diagrams are used, please include an informative
reference
>   per
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.4
> - Section 1.1 does not need to exist since this would be covered by
the
>   reference mentioned above
> - Reference to NMDA compliance should be contained within Section 1
(vs.
>   Section 2) per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.5
> - Section 2: It seems reference should be given to the location of
where the
>   ietf-routing module is defined (As well as reference to NMDA RFC in
the
>   above reference)
> - Section 2.1: "Additional modules may be created this to support..."
needs
>   slight rewording adjustment
> - Section 3: The RPC operations are named 'clear-adjacency' and
>   'clear-database' rather w/ reliance off namespacing for uniqueness.
This
>   section refers to 'clear-isis-database' and 'clear-isis-adjacency'
> - Section 4: Notification name mismatch in this section from actual
naming
>   within the module (e.g. 'adjacency-change' should rather be
>   'adjacency-state-change')
> - Section 7: Security Considerations will need updating to be
patterned after
>   the latest version of the template at
>   https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.7
> - Section 12: All modules imported within this module MUST be
referenced
>   within this section per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.9.
>   There are quite a few missing from this section right now
> - Appendix A: Some of the XML elements are off in alignment
> - Appendix A: Examples must be validated.  The example given has the
following
>   issues:
>   - /routing[name='SLI'] and /routing/description are invalid data
nodes and
>     do not exist.  I'm not sure why they are in the XML example here
>   - The example is meant to reference configuration however
>     /routing/interfaces is a r/o container
>   - The control-plane-protocol 'type' needs to be qualified - e.g.
>     <type
xmlns:isis="urn:ietf:params:xml:ns:yang:ietf-isis">isis:isis</type>
>   - The area-address does not validate against the pattern regex and
must end
>     with a '.'  e.g.
>     <area-address>49.0001.0000.</area-address>
>   - metric-type/value is set to 'wide' which is invalid.  This should
rather
>     be 'wide-only'
>   - isis/afs/af/af is set to 'ipv4-unicast' which is invalid.  This
should
>     rather be 'ipv4' per iana-routing-types
>   - /interfaces/interface/type must be populated and is invalid.  This
should
>     rather be qualified as such:
>     <type
xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:softwar
eLoopback</type>
>     <type
xmlns:ianaift="urn:ietf:params:xml:ns:yang:iana-if-type">ianaift:etherne
tCsmacd</type>
>   - /interfaces/interface/link-up-down-trap-enable must have a value
>     associated as such:
>     <link-up-down-trap-enable>enabled</link-up-down-trap-enable>
>   - NP container 'priority' has a must statement checking if an
interface-type
>     is set to 'broadcast' however if you take the XML example from
this
>     section, it will fail to validate even if <priority> is not
defined
>     underneath an interface-type of 'point-to-point'.  It seems to me
that
>     this logic may need to be readjusted or not exist at all (priority
can
>     still be set on implementations on loopback interfaces - which
would
>     default to 'broadcast' in the example here).  Could you not solve
this
>     with use of 'when' vs. 'must' as such:
>
>           when '../interface-type = "broadcast"' {
>               description "Priority can only be set for broadcast
interfaces.";
>           }
>
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-4.18
.2.
>
>   - /interfaces/interface/ipv4/mtu must contain a valid value (and
likely not
>     need to be defined for Loopback0)
>   - 'isis/mpls-te/ipv4-router-id' is invalid and should rather be
>     'isis/mpls/te-rid/ipv4-router-id'
>   - 'isis/afs/af/enabled' is invalid and should rather be
'isis/afs/af/enable'
>   - Examples should use IPv6 addresses where appropriate per
>
https://tools.ietf.org/html/draft-ietf-netmod-rfc6087bis-20#section-3.12

________________________________________________________________________
_________________________________________________

Ce message et ses pieces jointes peuvent contenir des informations
confidentielles ou privilegiees et ne doivent donc
pas etre diffuses, exploites ou copies sans autorisation. Si vous avez
recu ce message par erreur, veuillez le signaler
a l'expediteur et le detruire ainsi que les pieces jointes. Les messages
electroniques etant susceptibles d'alteration,
Orange decline toute responsabilite si ce message a ete altere, deforme
ou falsifie. Merci.

This message and its attachments may contain confidential or privileged
information that may be protected by law;
they should not be distributed, used or copied without authorisation.
If you have received this email in error, please notify the sender and
delete this message and its attachments.
As emails may be altered, Orange is not liable for messages that have
been modified, changed or falsified.
Thank you.