[mpls] Yangdoctors early review of draft-ietf-mpls-mldp-yang-11
Joe Clarke via Datatracker <noreply@ietf.org> Mon, 29 July 2024 20:11 UTC
Return-Path: <noreply@ietf.org>
X-Original-To: mpls@ietf.org
Delivered-To: mpls@ietfa.amsl.com
Received: from [10.244.2.81] (unknown [104.131.183.230]) by ietfa.amsl.com (Postfix) with ESMTP id B0F87C151707; Mon, 29 Jul 2024 13:11:05 -0700 (PDT)
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
From: Joe Clarke via Datatracker <noreply@ietf.org>
To: yang-doctors@ietf.org
X-Test-IDTracker: no
X-IETF-IDTracker: 12.19.0
Auto-Submitted: auto-generated
Precedence: bulk
Message-ID: <172228386533.1733274.11964599070161338137@dt-datatracker-659f84ff76-9wqgv>
Date: Mon, 29 Jul 2024 13:11:05 -0700
Message-ID-Hash: R6QPEOT2J5V7DAF7IT3NHQHSFDRDB47O
X-Message-ID-Hash: R6QPEOT2J5V7DAF7IT3NHQHSFDRDB47O
X-MailFrom: noreply@ietf.org
X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; header-match-mpls.ietf.org-0; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header
CC: draft-ietf-mpls-mldp-yang.all@ietf.org, mpls@ietf.org
X-Mailman-Version: 3.3.9rc4
Reply-To: Joe Clarke <jclarke@cisco.com>
Subject: [mpls] Yangdoctors early review of draft-ietf-mpls-mldp-yang-11
List-Id: Multi-Protocol Label Switching WG <mpls.ietf.org>
Archived-At: <https://mailarchive.ietf.org/arch/msg/mpls/PBb7oBNh5QRVj1TLkZ3QJT0R_uM>
List-Archive: <https://mailarchive.ietf.org/arch/browse/mpls>
List-Help: <mailto:mpls-request@ietf.org?subject=help>
List-Owner: <mailto:mpls-owner@ietf.org>
List-Post: <mailto:mpls@ietf.org>
List-Subscribe: <mailto:mpls-join@ietf.org>
List-Unsubscribe: <mailto:mpls-leave@ietf.org>
Reviewer: Joe Clarke Review result: Almost Ready I have been tasked to review the latest rev on behalf of the YANG Doctors. This draft defines two YANG modules for multipoint label distribution protocol (mLDP) in support of an overall YANG data model for the technology. The intent is to augment the existing ietf-mpls-ldp module. Overall, I would suggest running a spell check. I found multiple typos in the text (augement, minumum, hiearchy, alongwith, Alteratively, acheived, familty, digram, avaiable, higlighting, mutipoint). The modules adhere to NMDA, which is stated in the Overview, but missing from the Introduction (as mentioned in RFC8407), but I don't think that's a huge deal. The tree diagrams do reference RFC8340. In terms of the modules themselves, they fall victim to one of my pet peeves: tautological descriptions. Take for example "timeout" in the mldp-capabilities grouping. The description is just "Timeout in seconds." This might be obvious to someone versed in the technology, but it doesn't help those that might be working more on the automation side of mLDP. There are a scattering of "reference" properties for mLDP, but I was hoping to see more (e.g., references for MBB). The extended module does a much better job than the base module in this regard. I would suggest revisiting descriptions and make sure they are clear, informative, and have references to documents that describe the mLDP protocol semantics. In terms of structure, I noticed a few groupings (e.g., mldp-binding-label-state-attributes) that have a relative leafref to an object outside of the grouping itself. In a different context, that leafref might not resolve. This is not a problem per se, but the grouping description must state the context(s) in which the grouping can be used (see Section 4.6.1 of 8407). You have presence containers (one in each module). The description is slightly off with what one might expect with a presence container. According to 7950, a presence container, when created, can enable a protocol or capability. The example there is with SSH. In your case of ipv4 and ipv6, I might restate the presence as: ipv4 { presence "Enables IPv4 mLDP support."; ... } ipv6 { presence "Enables IPv6 mLDP support."; ... } Speaking of this ipv6 container, I see that you have a child object in the opaque-element-transit container, source-address. This is of type inet:ip-address vs. inet:ipv6-address (same kind of comment for group-address and the rp and group-address leafs in opaque-element-bidir child container). Additionally, the description of this container is "The type of opaque value element is the transit IPv4 source". Should that be "IPv6 source"? The same applies for the augmented notification in the "opaque-element-transit" container. It reads IPv4, but should this be "IPv4 or IPv6"?
- [mpls] Yangdoctors early review of draft-ietf-mpl… Joe Clarke via Datatracker