Re: [pim] AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09
Hongji Zhao <hongji.zhao@ericsson.com> Sun, 19 January 2020 03:04 UTC
Return-Path: <hongji.zhao@ericsson.com>
X-Original-To: pim@ietfa.amsl.com
Delivered-To: pim@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 3C31412003E; Sat, 18 Jan 2020 19:04:01 -0800 (PST)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.001
X-Spam-Level:
X-Spam-Status: No, score=-2.001 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIMWL_WL_HIGH=-0.001, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, SPF_PASS=-0.001, URIBL_BLOCKED=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (1024-bit key) header.d=ericsson.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 LhOyUEFDodNe; Sat, 18 Jan 2020 19:03:57 -0800 (PST)
Received: from EUR02-AM5-obe.outbound.protection.outlook.com (mail-am5eur02on061f.outbound.protection.outlook.com [IPv6:2a01:111:f400:fe07::61f]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id 617D8120025; Sat, 18 Jan 2020 19:03:56 -0800 (PST)
ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=dOO3StefytfAtMnaiP1LHlC8ondLuzMfj2x6wCsTzqhPG5bHN9YdWBsv+jF4GblAM57yzoEKeOHn4pcyDIME+TG/BTHXJXMakimfW2lGQYSNpS5CSByqn19M9JPRWwqj4CX1ALEcma+zZc0b/0VvLFi56Vaiov5kAVfZotmjliGvufCKfdoG3Gpl/S9DGK1BU/vpC/a9voYeU4j/dr5zWtQ09nwmMhu1vejrs3wEW29cj+XqnOXVSuij9gLpj84gP32pFthPf26vy5KWx2XbDhjAFn3Aij+ivv/NYLbYDs98QWGtA8TijJcX2zdesf7xN1aB7Ry0qGulWvz5mdXatA==
ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3hyBgem/9LES/yL/qK2v/PNVvgEZ8Ds0SKlHVYW7+3w=; b=fg0J+q5S1pEICAU86HN2BpHps1bGLzAS5Ai4Ou7ZoFnvypDVjXUT3YfStYhC0gDAJUcbiQwlyPbFzSuFXDfhQeeb4ctxfnh8+Gexrl+gFIzdM3jufRxGpjntMkj78dwZ0pIjwuV9M8ILrXh5q3a1YwuHGqf90phD2hfv3wnuCy5MZwV2lKUlyeg66qXrpPhU4e7GxGCmHTGKll94fU+YmsI8kKdnk3bIdQDLdiT3xrFyBqd7iMAw3R/ilKFKQJO/cwcdSHJTsRRwYLR8RnifalXDdQmixl82UAoUr2gCqK9hoaX6EQRFCJJTFVYLwupqDXe4OZ8Q6P2U6ejCoty+Cw==
ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=ericsson.com; dmarc=pass action=none header.from=ericsson.com; dkim=pass header.d=ericsson.com; arc=none
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ericsson.com; s=selector1; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=3hyBgem/9LES/yL/qK2v/PNVvgEZ8Ds0SKlHVYW7+3w=; b=f45+Y23It3WUSKfJnQzOs16bPuL29U7kbXiia1YN2zseysrApl9lzJms9NrCt6gTQNM/A5fWf5JQCWgAjUg/RxPts2DFVCx1Wfiz5ALVXa0jmu0Cef70MYv4utlV8vkfJlPmRNpG05XW5wyZ8F1nXSnxEgAx4sCBRtn/hNsfFJc=
Received: from HE1PR07MB3148.eurprd07.prod.outlook.com (10.170.247.15) by HE1PR07MB3276.eurprd07.prod.outlook.com (10.170.244.152) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2665.12; Sun, 19 Jan 2020 03:03:53 +0000
Received: from HE1PR07MB3148.eurprd07.prod.outlook.com ([fe80::95b6:84d4:100f:8760]) by HE1PR07MB3148.eurprd07.prod.outlook.com ([fe80::95b6:84d4:100f:8760%5]) with mapi id 15.20.2644.024; Sun, 19 Jan 2020 03:03:53 +0000
From: Hongji Zhao <hongji.zhao@ericsson.com>
To: Alvaro Retana <aretana.ietf@gmail.com>
CC: Stig Venaas <stig@venaas.com>, "pim-chairs@ietf.org" <pim-chairs@ietf.org>, "pim@ietf.org" <pim@ietf.org>
Thread-Topic: AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09
Thread-Index: AQHVzYeKaXJzNYyt5k+R4mTeZys2XKfxTt9w
Date: Sun, 19 Jan 2020 03:03:52 +0000
Message-ID: <HE1PR07MB3148BBF5316C9EC1FAC7742A96330@HE1PR07MB3148.eurprd07.prod.outlook.com>
References: <CAMMESsz-MvVF-2o2_Gso6yKocUKhRFrJTp0jsUwFpcznfj5qXw@mail.gmail.com>
In-Reply-To: <CAMMESsz-MvVF-2o2_Gso6yKocUKhRFrJTp0jsUwFpcznfj5qXw@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
X-MS-Has-Attach:
X-MS-TNEF-Correlator:
authentication-results: spf=none (sender IP is ) smtp.mailfrom=hongji.zhao@ericsson.com;
x-originating-ip: [119.28.22.196]
x-ms-publictraffictype: Email
x-ms-office365-filtering-correlation-id: f97162bb-740a-4a58-83ca-08d79c8c3722
x-ms-traffictypediagnostic: HE1PR07MB3276:
x-microsoft-antispam-prvs: <HE1PR07MB3276828717EA5810C0CC664596330@HE1PR07MB3276.eurprd07.prod.outlook.com>
x-ms-oob-tlc-oobclassifiers: OLM:7691;
x-forefront-prvs: 0287BBA78D
x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(346002)(39860400002)(376002)(366004)(136003)(396003)(199004)(189003)(9686003)(53546011)(316002)(6506007)(55016002)(186003)(2906002)(26005)(966005)(30864003)(71200400001)(52536014)(8676002)(66574012)(5660300002)(6916009)(81156014)(33656002)(81166006)(478600001)(4326008)(7696005)(66446008)(66556008)(64756008)(66946007)(66476007)(44832011)(76116006)(54906003)(86362001)(8936002)(579004)(357404004); DIR:OUT; SFP:1101; SCL:1; SRVR:HE1PR07MB3276; H:HE1PR07MB3148.eurprd07.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1;
received-spf: None (protection.outlook.com: ericsson.com does not designate permitted sender hosts)
x-ms-exchange-senderadcheck: 1
x-microsoft-antispam: BCL:0;
x-microsoft-antispam-message-info: 61GSCiBSIMoJe+8+bXMavjqs+XI9Nq8hBEUQiriMBKAzDaAXCzVC5mKQs8+loDL5fWbvuv/J6aWLbpelA1DhZzSzavqF5UztTJo1UeslrNZNNFazlqYS3khh1/Qv+sQC7YaRct+pkV01+SIKMTp9qYqujiQrf7foXLWf5nQlrTwsADOLG3R4wy2E7IF0QjAXI5ZAp1B+B5tiWKq50csDt7Av7nV+GI7RsnrbpGkcHHGz+qFTi1ieG7ILwy3hUx+e0iZphaGCFHeiARgMzzpAAjgToP75/Da+JgKaG0TsDFbr1qxZhde0lWp+eZb0LkHG6KYt8sBLk1X5WKxJLNI4V42Eonu+qLQHn2kzfdhmF5Bo4Nhq4WuWupJsxcA6Wn4nSuUkc7WJ+h912K6r8EBdObJr2AkDC3o0NqRBvpLnqMpwiZd3qQJG2E7BF5SAX1l9mYGg9lYEq0gJ5l2AsOd+YhBCozxQU4YMRiLSHYTDz4HeBsO5FHYg2n5EMQU2dRwm7zgAR97oODj9kEdi01dpX+rVPKG+nTzgHU1hYv7ifZ1FtrtrWe3eCf6cSQ+b5gew
x-ms-exchange-antispam-messagedata: SfuV3h89wUVdJUV1ywTFpcCF+FYDAVeNeRJn5POSFbjESmP3HNZkRwAEfjpQ11LJNpTG7eiIEkCgQnncJcoohdZj5ODaEnGyZlmsN7rBGEPNYUaf3APyQj2mJ+itwNrsMbm9dR9V5yKfVSTOQcEViw==
x-ms-exchange-transport-forked: True
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: base64
MIME-Version: 1.0
X-OriginatorOrg: ericsson.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f97162bb-740a-4a58-83ca-08d79c8c3722
X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Jan 2020 03:03:52.8389 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 92e84ceb-fbfd-47ab-be52-080c6b87953f
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: /GleXAh4PrAe+LGGzdFSskJCtrXpK8cNkvR/jxRF8kw2ZouuJIx/V++qOSWx/gl5HoXk3pIvl/GYJ6H+v6e1UwVsiT+/AtZoJHyUtmicnqw=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: HE1PR07MB3276
Archived-At: <https://mailarchive.ietf.org/arch/msg/pim/pgErvPgTqUP_3Bi4zplP6k5vXEI>
Subject: Re: [pim] AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09
X-BeenThere: pim@ietf.org
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Protocol Independent Multicast <pim.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/pim>, <mailto:pim-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/pim/>
List-Post: <mailto:pim@ietf.org>
List-Help: <mailto:pim-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/pim>, <mailto:pim-request@ietf.org?subject=subscribe>
X-List-Received-Date: Sun, 19 Jan 2020 03:04:01 -0000
Hi Alvaro, Thanks a lot for your comments! We will address them as soon as possible. BR/Hongji -----Original Message----- From: Alvaro Retana <aretana.ietf@gmail.com> Sent: Saturday, January 18, 2020 6:43 AM To: draft-ietf-pim-igmp-mld-snooping-yang@ietf.org Cc: Stig Venaas <stig@venaas.com>; pim-chairs@ietf.org; pim@ietf.org Subject: AD Review of draft-ietf-pim-igmp-mld-snooping-yang-09 Dear authors: Here is my review of the latest version (-09) of this document. In general, there are many basic issues in the contents of the document. For example: References are not present for fundamental work such as "BRIDGE and L2VPN", some of the sections don't follow the existing templates (Security Considerations, for example), etc. These are issues that shouldn't be present in documents past WGLC! :-( Also, there is a long list of idnits that must be fixed as well [1]. I have included comments in-line about those basic issues, and others. Please take a close look below. The datatracker is still showing several YANG Validation errors. These were brought up during the WGLC, and it was suggested that the errors are a tool issue [2]. But that comment was for pyang 1.7.8 -- the current tool uses pyang 2.1.1; do we think the error is still in the tool? Can someone please talk to the authors of the tool and point out the problem [3]? This document augments an IEEE YANG model. Has anyone (authors, Chairs) made the IEEE aware of it? To be honest, I don't know if there is a specific process we need to follow (I doubt it), but letting the IEEE know would be a minimum. I will wait for this review to be addressed before starting the IETF LC. Thanks! Alvaro. [1] https://tools.ietf.org/idnits?url=https://tools.ietf.org/id/draft-ietf-pim-igmp-mld-snooping-yang-09.txt [2] https://mailarchive.ietf.org/arch/msg/pim/xD8PAlTqGIiyb2H3TeYT17w8zMU [3] https://github.com/mbj4668/pyang [Line numbers from idnits.] ... 17 Abstract 19 This document defines a YANG data model that can be used to 20 configure and manage Internet Group Management Protocol (IGMP) and 21 Multicast Listener Discovery (MLD) Snooping devices. The YANG module in 22 this document conforms to Network Management Datastore Architecture 23 (NMDA). [nit] No indent needed. ... 82 1. Introduction 84 This document defines a YANG [RFC6020] data model for the management of 85 Internet Group Management Protocol (IGMP) and Multicast Listener 86 Discovery (MLD) Snooping devices. [major] Please add a reference to rfc4541. [minor] rfc4541 is "based on best current practices for IGMPv2, with further considerations for IGMPv3- and MLDv2-snooping." This document mostly uses IGMP and MLD (with no version indication). Which versions of the protocols are supported in this document? Just IGMPv2, IGMPv3 and MLDv2? Please explicitly indicate that in this section. ... 94 1.1. Terminology 96 The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT", 97 "SHOULD", "SHOULD NOT", "RECOMMENDED", "NOT RECOMMENDED", "MAY", and 98 "OPTIONAL" in this document are to be interpreted as described in BCP 14 99 [RFC2119]. [minor] There don't seem to be any rfc2119-keywords used, so this template is not needed. 101 The terminology for describing YANG data models is found in [RFC6020]. [] Please add text similar to §1.1/rfc8652. 103 1.2. Tree Diagrams [major] This section should be replaced with the following: Tree diagrams used in this document follow the notation defined in [RFC8340]. The reference to rfc8340 should be Normative. [major] A section similar to §1.4/rfc8652 (Prefixes in Data Node Names) is needed to indicate the prefixes used. ... 131 2.1. Overview 133 The IGMP and MLD Snooping YANG module defined in this document has all 134 the common building blocks for the IGMP and MLD Snooping protocol. [minor] "Snooping protocol" It is not a protocol -- rfc4541 doesn't refer to it that way. Instead, it uses simply "snooping switches". Please use the same terminology. 136 The YANG module includes IGMP and MLD Snooping instance definition, 137 instance reference in the scenario of BRIDGE and L2VPN. The module also 138 includes the RPC methods for clearing IGMP and MLD Snooping group 139 tables. [major] We need Normative references for where "the scenario of BRIDGE and L2VPN" are specified. 141 This YANG module conforms to Network Management Datastore Architecture 142 (NMDA)[RFC8342]. This NMDA architecture provides an architectural 143 framework for datastores as they are used by network management 144 protocols such as NETCONF [RFC6241], RESTCONF [RFC8040] and the YANG 145 [RFC7950] data modeling language. [major] There is no reference for rfc6241 -- it must be Normative. [major] There is no reference for rfc8040 -- it must be Normative. [major] There is no reference for rfc7950 -- it must be Normative. [major] Before going deeper into the structure, please include information about optional capabilities and how IPv4/IPv6 are handled. See §2.2/§2.3 of rfc8652 for examples. 147 2.2. IGMP Snooping Instances 149 The YANG module defines igmp-snooping-instance which augments 150 /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol. [major] Please use descriptive text outside of the model. For example: This model augments the core routing data model specified in [RFC8349]. +--rw routing +--rw router-id? +--rw control-plane-protocols | +--rw control-plane-protocol* [type name] | +--rw type | +--rw name | +--rw igmp-snooping-instance <= Augmented by this Model ... | +--rw mld-snooping-instance <= Augmented by this Model ... The "igmp-snooping-instance" container instantiates an IGMP Snooping Instance... Please take a look at rfc8652 for examples of what is needed in this document. [major] It may be confusing to other readers the reason for not augmenting, or even requiring, the IGMP/MLD model (rfc8652). Please include some text in §2.1 to explain the relationship between snooping and the IGMP/MLD protocols themselves. Specifically, the fact that the switches don't really nned to run the protocols. 152 All the IGMP Snooping related attributes have been defined in the igmp- 153 snooping-instance. The read-write attribute means configurable data. The 154 read-only attribute means state data. [nit] s/attribute means/attribute represents 156 One igmp-snooping-instance could be referenced in one BRIDGE instance or 157 L2VPN instance. One igmp-snooping-instance corresponds to one BRIDGE 158 instance or L2VPN instance. [major] These mentions of references from/correspondence to a "BRIDGE instance or L2VPN instance" makes me think that there are YANG modules somewhere that define those instances and that are not used as a reference in this document. 160 The value of scenario in igmp-snooping-instance is bridge or l2vpn. When 161 it is bridge, the igmp-snooping-instance will be referenced in the 162 BRIDGE scenario. When it is l2vpn, the igmp-snooping-instance will be 163 referenced in the L2VPN scenario. [major] The scenarios need a description and/or reference... 165 The value of bridge-mrouter-interface, l2vpn-mrouter-interface-ac, 166 l2vpn-mrouter-interface-pw are filled by snooping device dynamically. 167 They are different from static-bridge-mrouter-interface, static-l2vpn- 168 mrouter-interface-ac, and static-l2vpn-mrouter-interface-pw which are 169 configured statically. [major] The term "mrouter" is used in many places (alone and in variable names), but it is not defined anywhere. Please define it. [nit] s/by snooping device/by the snooping device 171 The attributes under the interfaces show the statistics of IGMP Snooping 172 related packets. 174 module: ietf-igmp-mld-snooping [minor] I believe that the "module" line is not needed here, which may help with the long lines below. 175 augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol: 176 +--rw igmp-snooping-instance {feature-igmp-snooping}? 177 | +--rw scenario? snooping-scenario-type 178 | +--rw enable? boolean 179 | +--rw forwarding-mode? enumeration 180 | +--rw explicit-tracking? boolean {explicit-tracking}? 181 | +--rw exclude-lite? boolean {exclude-lite}? 182 | +--rw send-query? boolean 183 | +--rw immediate-leave? empty {immediate-leave}? 184 | +--rw last-member-query-interval? uint16 185 | +--rw query-interval? uint16 186 | +--rw query-max-response-time? uint16 [minor] Why are timer-value-* (rfc8294) not used? Please consider using them for the variables above and others... ... 247 2.3. MLD Snooping Instances 249 The YANG module defines mld-snooping-instance which could be referenced 250 in the BRIDGE or L2VPN scenario to enable MLD Snooping. [major] These mentions of references from/correspondence to a "BRIDGE instance or L2VPN instance" makes me think that there are YANG modules somewhere that define those instances and that are not used as a reference in this document. 252 The mld-snooping-instance is the same as IGMP snooping except changing 253 IPv4 addresses to IPv6 addresses. 255 module: ietf-igmp-mld-snooping [minor] I believe that the "module" line is not needed here, which may help with the long lines below. 256 augment /rt:routing/rt:control-plane-protocols/rt:control-plane-protocol: [major] There is no mention in this section of the augmentation, as was mentioned in §2.2. Please be consistent and avoid shortcuts. ... 326 2.4. IGMP and MLD Snooping Instances Reference [] Please make sure that some of this information is available earlier in the document, or that a forward reference to this Section exists to avoid questions about referring to the snooping instances. 328 The igmp-snooping-instance could be referenced in the scenario of BRIDGE 329 or L2VPN to configure the IGMP Snooping. 331 For the BRIDGE scenario this model augments /dot1q:bridges/dot1q:bridge 332 to reference igmp-snooping-instance. It means IGMP Snooping is enabled 333 in the whole bridge. [major] There is no reference yet to what "dot1q" is. Where is that module specified? The reference should be Normative. ... 347 For the L2VPN scenario this model augments /ni:network-instances/ 348 ni:network-instance/ni:ni-type/l2vpn:l2vpn to reference igmp-snooping- 349 instance. It means IGMP Snooping is enabled in the specified l2vpn 350 instance. [major] There is no reference yet to what "ni" is. Where is that module specified? The reference should be Normative. ... 378 3. IGMP and MLD Snooping YANG Module [major] Please add a statement before the module that includes any documents referenced in it: "This module references..." ... 387 import ietf-inet-types { 388 prefix "inet"; 389 } [major] All the import statements need a reference, for example: import ietf-inet-types { prefix inet; reference "RFC 6991: Common YANG Data Types"; } Note that "RFC 6991", and not "RFC6991" is used inside the module. Please review *all* the references inside the module and change them as needed. ... 446 description 447 "The module defines a collection of YANG definitions common for 448 all Internet Group Management Protocol (IGMP) and Multicast 449 Listener Discovery (MLD) Snooping devices. [minor] Please add a reference here to RFC 4541. ... 475 feature feature-igmp-snooping { 476 description 477 "Support IGMP snooping protocol."; [minor] It is not a protocol... 478 reference 479 "RFC 4541, Section 1"; 480 } [major] §1 is the Introduction. Unless there is a very specific need to point to a section, just referencing the RFC is ok. 482 feature feature-mld-snooping { 483 description 484 "Support MLD snooping protocol."; [minor] It is not a protocol... 485 reference 486 "RFC 4541, Section 1"; 487 } [major] §1 is the Introduction. [] Some of the "features" introduced here are part of the snooping specification (rfc4541), or needed to operate the mechanism (for example static-mrouter-interface). In those cases, there's no need to include a reference for each one. 489 feature immediate-leave { 490 description 491 "Support configuration of immediate-leave."; 492 reference 493 "RFC 2236, Section 10"; 494 } [major] I couldn't find any mention of immediate leave (or anything similar) in rfc2236. And in any case, §10 is the Security Considerations. ... 503 feature static-l2-multicast-group { 504 description 505 "Support configuration of L2 multicast static-group."; 506 reference 507 "RFC 4541, Section 2.1"; 508 } [major] rfc4541 doesn't even contain the word "static". If the feature is defined there with a different name, please use the same terminology, at least in the description. 510 feature static-mrouter-interface { 511 description 512 "Support configuration of mrouter interface."; 513 reference 514 "RFC 4541, Section 2.1"; 515 } [major] Same as above... 516 feature rpc-clear-groups { 517 description 518 "Support clearing statistics by RPC for IGMP & MLD snooping."; 519 reference 520 "RFC 4541, Section 2.1"; 521 } [major] I doubt that this specifically mentioned in rfc4541. In this case, no reference is needed. 523 feature explicit-tracking { 524 description 525 "Support configuration of per instance explicit-tracking."; 526 reference 527 "RFC 3376, Appendix B"; 528 } [major] Appendix B (Summary of Changes from IGMPv2) doesn't specify what explicit tracking is. Following the lead from rfc8653, use Section 3 of rfc6636 as the reference. It should be Normative. ... 573 identity igmp-snooping { 574 base rt:control-plane-protocol; 575 description 576 "IGMP snooping protocol"; 577 } [minor] It is not a protocol... 579 identity mld-snooping { 580 base rt:control-plane-protocol; 581 description 582 "MLD snooping protocol"; 583 } [minor] It is not a protocol... ... 623 leaf version { 624 type uint8 { 625 range "1..3"; 626 } 627 default 2; 628 description "IGMP snooping version."; 629 } [minor] This is the version of what? Because of the range, I guess it is the IGMP protocol version, and not the "IGMP snooping version." Is that correct? [major] If the previous guess is correct... What does this version mean? Given that the default is 2, does it mean that only IGMPv2 packets should be snooped? I would assume that deployments already supporting IGMPv3 are common... ... 663 leaf-list l2vpn-outgoing-ac { 664 when 'derived-from-or-self(../../scenario,"ims:l2vpn")'; 665 type if:interface-ref; 666 description "Outgoing AC in L2VPN forwarding"; 667 } [minor] Expand AC on first use. 669 leaf-list l2vpn-outgoing-pw { 670 when 'derived-from-or-self(../../scenario,"ims:l2vpn")'; 671 type pw:pseudowire-ref; 672 description "Outgoing PW in L2VPN forwarding"; 673 } [minor] Expand PW on first use. ... 688 leaf forwarding-mode { 689 type enumeration { 690 enum "mac" { 691 description 692 "MAC-based lookup mode"; 693 } 694 enum "ip" { 695 description 696 "IP-based lookup mode"; 697 } 698 } 699 default "ip"; 700 description "The default forwarding mode is ip"; 701 } [minor] rfc4541 doesn't talk about forwarding modes. It talks about how "switches may maintain forwarding tables based on either MAC addresses or IP addresses" and how "if a switch supports both types of forwarding tables then the default behavior should be to use IP addresses." Perhaps forwarding-type of forwarding-table-type would be more descriptive. In any case, please at least add a better description. 703 leaf explicit-tracking { 704 if-feature explicit-tracking; [nit] Using the same name for the feature and the leaf may be confusing. This is done in several places. 705 type boolean; 706 default false; 707 description 708 "Track the IGMP v3 & MLD v2 membership reports 709 from individual hosts. It contributes to saving network 710 resources and shortening leave latency."; 711 } [minor] s/IGMP v3 & MLD v2/IGMPv3 and MLDv2 ... 738 leaf last-member-query-interval { 739 type uint16 { 740 range "1..1023"; 741 } 742 units seconds; 743 default 1; 744 description 745 "Last Member Query Interval, which may be tuned to modify 746 the leave latency of the network."; 747 reference "RFC3376. Sec. 8.8."; 748 } [major] The Last Member Query Interval has a default of 10 (in units of 1/10 second). While that does correspond to 1 second, it doesn't reflect the same values as in rfc3376. It may be clearer to the user of the module to count in seconds...but not all the granularity expected from rfc3376 can be displayed. If you want to maintain the units as seconds, please describe the use when the model is introduced (§2.*), and explain the potential loss of fidelity. Note that this is not the only parameter that has the same issue. ... 760 leaf query-max-response-time { 761 type uint16; 762 units seconds; 763 default 10; 764 description 765 "Query maximum response time specifies the maximum time 766 allowed before sending a responding report."; 767 reference "RFC3376. Sec. 4.1.1, 8.3, 8.14.3."; 768 } [minor] rfc3376 only uses "maximum response time" once (in §9.1). §4.1.1 uses "Max Resp Time" (in units of 1/10 second), §8.3 uses "Max Response Time" (also in units of 1/10 second), and §8.14.3 just talks about how to use the "Max Response Time". Please be consistent and specific. 770 leaf require-router-alert { 771 if-feature require-router-alert; 772 type boolean; 773 default false; 774 description 775 "When the value is true, router alert should exist 776 in the IP head of IGMP or MLD packet."; 777 } [nit] s/IP head/IP header ... 821 leaf version { 822 type uint8 { 823 range "1..2"; 824 } 825 default 2; 826 description "MLD snooping version."; 827 } [minor] This is the version of what? Because of the range, I guess it is the MLD protocol version, and not the "MLD snooping version." Is that correct? [major] If the previous guess is correct... What does this version mean? Given that the default is 2, does it mean that only MLDv2 packets should be snooped? ... 1122 grouping igmp-snooping-statistics { 1123 description 1124 "The statistics attributes for IGMP snooping."; 1126 leaf num-query { 1127 type yang:counter64; 1128 description 1129 "The number of query messages."; 1130 reference 1131 "RFC 2236, Section 2.1"; 1132 } [major] It is nice that you're putting specific references in. However, the terminology (in most cases in this group) doesn't match. For example, rfc2236 talks about a "Membership Query" (vs simply "query messages"). Given that this is a Standards Track document, it is important to be specific about the terminology. 1133 leaf num-membership-report-v1 { 1134 type yang:counter64; 1135 description 1136 "The number of membership report v1 messages."; 1137 reference 1138 "RFC 3376, Section 4"; 1139 } [major] rfc3376/§4 points at rfc1112 as the reference for where the Version 1 Membership Report is specified. 1140 leaf num-membership-report-v2 { 1141 type yang:counter64; 1142 description 1143 "The number of membership report v2 messages."; 1144 reference 1145 "RFC 3376, Section 4"; 1146 } [major] rfc3376/§4 points at rfc2236 as the reference for where the Version 2 Membership Report is specified. ... 1154 leaf num-leave { 1155 type yang:counter64; 1156 description 1157 "The number of leave messages."; 1158 reference 1159 "RFC 3376, Section 4"; 1160 } [major] rfc3376/§4 points at rfc2236 as the reference for where the Version 2 Leave Group is specified. Is that the message you're referring to? 1161 leaf num-non-member-leave { 1162 type yang:counter64; 1163 description 1164 "The number of non member leave messages."; 1165 reference 1166 "RFC 3376, Section 4"; 1167 } [major] I didn't find in rfc3376/§4 anything about non members. I'm assuming that the counter above is simply the number of Leave Group messages received from nodes that were not members of the group. Is that true? If so, it may not be necessary to add a reference. ... 1189 leaf num-report-v1 { 1190 type yang:counter64; 1191 description 1192 "The number of Version 1 Multicast Listener Report."; 1193 reference 1194 "RFC 3810, Section 5"; 1195 } [major] rfc3810/§5 points at rfc2710 as the correct reference. ... 1203 leaf num-done { 1204 type yang:counter64; 1205 description 1206 "The number of Version 1 Multicast Listener Done."; 1207 reference 1208 "RFC 3810, Section 5"; 1209 } [major] rfc3810/§5 points at rfc2710 as the correct reference. ... 1318 container igmp-snooping-instance { 1319 when 'derived-from-or-self(../rt:type, "ims:igmp-snooping")' { 1320 description 1321 "This container is only valid for IGMP snooping protocol."; [minor] It is not a protocol... ... 1346 container mld-snooping-instance { 1347 when 'derived-from-or-self(../rt:type, "ims:mld-snooping")' { 1348 description 1349 "This container is only valid for MLD snooping protocol."; [minor] It is not a protocol... ... 1371 augment "/dot1q:bridges/dot1q:bridge" { 1372 description 1373 "Reference IGMP & MLD snooping instance in BRIDGE scenario"; ... 1389 augment "/dot1q:bridges/dot1q:bridge"+ 1390 "/dot1q:component/dot1q:bridge-vlan/dot1q:vlan" { 1391 description 1392 "Reference IGMP & MLD snooping instance in BRIDGE scenario"; [nit] These two augmentations have the same description. ... 1491 4. Security Considerations [major] Please update the text in this section using the latest template, and make sure to update the references: https://trac.ietf.org/trac/ops/wiki/yang-security-guidelines ... 1505 There are a number of data nodes defined in this YANG module that are 1506 writable/creatable/deletable (i.e., config true, which is the default)
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Hongji Zhao
- [pim] AD Review of draft-ietf-pim-igmp-mld-snoopi… Alvaro Retana
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Alvaro Retana
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Hongji Zhao
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Alvaro Retana
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Hongji Zhao
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Hongji Zhao
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Alvaro Retana
- Re: [pim] AD Review of draft-ietf-pim-igmp-mld-sn… Hongji Zhao