Re: [Teas] Yangdoctors early review of draft-ietf-teas-te-service-mapping-yang-10

Dhruv Dhody <dhruv.ietf@gmail.com> Mon, 11 July 2022 14:04 UTC

Return-Path: <dhruv.ietf@gmail.com>
X-Original-To: teas@ietfa.amsl.com
Delivered-To: teas@ietfa.amsl.com
Received: from localhost (localhost [127.0.0.1]) by ietfa.amsl.com (Postfix) with ESMTP id 039DBC15791D; Mon, 11 Jul 2022 07:04:51 -0700 (PDT)
X-Virus-Scanned: amavisd-new at amsl.com
X-Spam-Flag: NO
X-Spam-Score: -2.104
X-Spam-Level:
X-Spam-Status: No, score=-2.104 tagged_above=-999 required=5 tests=[BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, DKIM_VALID_AU=-0.1, DKIM_VALID_EF=-0.1, FREEMAIL_FROM=0.001, HTML_MESSAGE=0.001, RCVD_IN_ZEN_BLOCKED_OPENDNS=0.001, SPF_HELO_NONE=0.001, SPF_PASS=-0.001, T_SCC_BODY_TEXT_LINE=-0.01, URIBL_BLOCKED=0.001, URIBL_DBL_BLOCKED_OPENDNS=0.001, URIBL_ZEN_BLOCKED_OPENDNS=0.001] autolearn=ham autolearn_force=no
Authentication-Results: ietfa.amsl.com (amavisd-new); dkim=pass (2048-bit key) header.d=gmail.com
Received: from mail.ietf.org ([50.223.129.194]) by localhost (ietfa.amsl.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id GiZxT26fVmDt; Mon, 11 Jul 2022 07:04:48 -0700 (PDT)
Received: from mail-io1-xd2d.google.com (mail-io1-xd2d.google.com [IPv6:2607:f8b0:4864:20::d2d]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by ietfa.amsl.com (Postfix) with ESMTPS id B2DE5C159486; Mon, 11 Jul 2022 07:04:45 -0700 (PDT)
Received: by mail-io1-xd2d.google.com with SMTP id r76so4965780iod.10; Mon, 11 Jul 2022 07:04:45 -0700 (PDT)
DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=0YZohNehQJNKBNIEu9WHif8WanIQErF6UH8IRRqzBhs=; b=R/Yu4chgdMUF2WBCaAEfHS3zooJd7Mu/nL9zlIh6mae43z6LmMOLQqdDEyZ+mFbEnk TFAC73/eIvA9D/bUdffloHqp68OPvGfn//byVDL1zE+kNkoiHZdfB9+dfKF7aLHdPepk xp4efYJwm8/ooJCrUKu+/EgoB3kt0r8+DW7ttV71Qjs1Gt6aAN8oUcuUONsEajzmoh88 D+3zx33vidbCX66dNisnJqElRW3XN0IC1XHWpb9Lpzg3btMODemCg7WBeAOjyPU9xR5n dCZhAuAYwb7mH2XMFcybmlAbiRG9Z4m3RAZAgPkxkH1DFEa55q+W6nmXEUWT7dQJKRRA x5Lw==
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=0YZohNehQJNKBNIEu9WHif8WanIQErF6UH8IRRqzBhs=; b=QerbMJU32zRvOPU7CKIYN0yDG8MiMwmkMQiMO0tQXv+EAgDQroJKJoNnz+s6hMeHw0 wA0LECMh4w4N68PJT94TPh1Wtu/G4uuO4UdOFxkiafjZpSH0yPH8bMbmfKtZC53gsMAs ulwEE4Sgt4oIblZ+lg5O1Zgz3udNvunDGkYE4YepdJmKxWh3OW3/JnuqR3a99i+YC6e7 g6UktocoyQfok0NwGgh7g4U/8VQz8746KsvYQXvegC+RdJ1OfD5y821T/UBh42CfGg4j lJ9law/nbLUfg/Qi+xOjO5d849tLqZs4LjZP/ItYNdUPKNiLcToLwKlNaBt4fhpWvtlD 1rYg==
X-Gm-Message-State: AJIora9xNFZcaowDL1sVfVSsXnI2eClgRz04YraV6PiiwjNR61KUINdh doWzs3uWR315805u+xKxT7EDHsIIRKDJQAYsQVF6Ebj7dAN9tw==
X-Google-Smtp-Source: AGRyM1v6tZyrUp4ClpM0g94L127CCBRBqvBhT7u+pYFUudgHrmATu4vf0IbgJ0P4+d+u+Ns/Jkwbory3V8UfGidFMF0=
X-Received: by 2002:a02:606f:0:b0:335:ae88:68c6 with SMTP id d47-20020a02606f000000b00335ae8868c6mr9986248jaf.320.1657548284473; Mon, 11 Jul 2022 07:04:44 -0700 (PDT)
MIME-Version: 1.0
References: <164730395314.8331.2676797734526513659@ietfa.amsl.com>
In-Reply-To: <164730395314.8331.2676797734526513659@ietfa.amsl.com>
From: Dhruv Dhody <dhruv.ietf@gmail.com>
Date: Mon, 11 Jul 2022 19:34:08 +0530
Message-ID: <CAB75xn4RuNKKTCk0-+ZikHuTDcHE5YB2vEzUonQQkxvgV00-QQ@mail.gmail.com>
To: Xufeng Liu <xufeng.liu.ietf@gmail.com>
Cc: yang-doctors@ietf.org, draft-ietf-teas-te-service-mapping-yang.all@ietf.org, "TEAS WG (teas@ietf.org)" <teas@ietf.org>
Content-Type: multipart/alternative; boundary="000000000000b1b35905e38808da"
Archived-At: <https://mailarchive.ietf.org/arch/msg/teas/PWy0inU8WPODYY_JZqKYtVurhAk>
Subject: Re: [Teas] Yangdoctors early review of draft-ietf-teas-te-service-mapping-yang-10
X-BeenThere: teas@ietf.org
X-Mailman-Version: 2.1.39
Precedence: list
List-Id: Traffic Engineering Architecture and Signaling working group discussion list <teas.ietf.org>
List-Unsubscribe: <https://www.ietf.org/mailman/options/teas>, <mailto:teas-request@ietf.org?subject=unsubscribe>
List-Archive: <https://mailarchive.ietf.org/arch/browse/teas/>
List-Post: <mailto:teas@ietf.org>
List-Help: <mailto:teas-request@ietf.org?subject=help>
List-Subscribe: <https://www.ietf.org/mailman/listinfo/teas>, <mailto:teas-request@ietf.org?subject=subscribe>
X-List-Received-Date: Mon, 11 Jul 2022 14:04:51 -0000

Hi Xufeng,

Thanks for your early Yangdoctors review. I have posted an update -
https://www.ietf.org/rfcdiff?url2=draft-ietf-teas-te-service-mapping-yang-11

Please see inline.

On Tue, Mar 15, 2022 at 5:55 AM Xufeng Liu via Datatracker <noreply@ietf.org>
wrote:

> Reviewer: Xufeng Liu
> Review result: Almost Ready
>
> This is a review of the YANG modules in
> draft-ietf-teas-te-service-mapping-yang-10.
>
> 1) ietf-te-service-mapping-types@2022-03-07.yang
>
> 1.1) In list sr-policy, there is the line: “/*Headend should also be
> there!*/”.
> I would agree with the comment since the color and (tail)-endpoint only
> uniquely identify an sr-policy within the headend node. Does this mean that
> something still needs to be done here?
>
>
The correct thing to do is the headend is part of the
draft-ietf-spring-sr-policy-yang-01 -- this way the model works on the
controller as well as the device! I gave that comment some while back, and
the document has not been updated in a while.

For now, I will add the headend in our model (but keep the comment so that
this could be tracked)!



> 1.2) As described in Sec 4.3.1. of RFC8407, child nodes within a container
> or
> list SHOULD NOT replicate the parent identifier. The prefix “policy-” in
> “policy-color-ref” and “policy-endpoint-ref” should be dropped.
>
>
Done



> 1.3) Similar to 1.2), “te-mapping-template-ref” can be improved by
> dropping the
> prefix “te-mapping”.
>
>
Done



> 1.4) The leaf color under the container te-policy has no reference. Is
> this the
> same as the “admin-group” defined in RFC 8776,  RFC 7308, RFC 5305, and
> RFC
> 3630?
>
>
Thanks for pointing this out. Changed to generic-path-affinities from
te-types.



> 1.5) In “map-type”, is the “map” the same as the “te-mapping” and
> “te-service-mapping” in the parent containers? If these terms are the
> same, it
> should be considered to drop the prefix “map”. Furthermore, these three
> terms
> should be consistent, or their differences are explained.
>
>
Removed te-mapping and updated to type only!



> 1.6) In this YANG module, there are many lines of  “//grouping”. What are
> they
> used for?
>
>
To mark the end of grouping, but they can be removed and it is done!



> 1.7) In this YANG module, there are section headers like “Typedef”, and
> “Groupings”, etc., but there is no section header for “container
> te-mapping-templates”, so it falls into the “Groupings” section, which is
> misleading.
>
>
Updated



> 1.8) One identity is named “detnet-hard-isolation”. Does it mean to use
> “detnet” architecture and implementation as defined in the DETNET Working
> Group? If so, more detailed descriptions on how to use would be good. If
> not,
> it would be better to avoid the same term as used in
> https://datatracker.ietf.org/doc/html/draft-ietf-detnet-yang.
>
>
Removed!



> 2) ietf-l3sm-te-service-mapping@2022-03-07.yang
>
> 2.1) This document uses the term “Layer 3 Service Model (L3SM)” which has
> been
> changed to “L3VPN Service Delivery” by RFC 8299. The term “L3SM” is not
> used in
> RFC 8299 any more, but is still used in this document extensively. Some
> explanations in the document would be useful to avoid confusion.
>
>
Good catch! Updated the terminology section.



> 2.2) Why is the container te-service-mapping “presence”? What does an empty
> container te-service-mapping indicate?
>
>
It is marked that the VPN service is relying on the TE techniques, even
though the mapping is not yet populated.
Updated the description to say - "Indicates L3 service is relying on
underlying TE"



> 2.3) Why is there container te-mapping under te-service-mapping? Are the
> two
> containers duplicated?
>
>
Removed.



> 2.4) In the augmentation to site-network-access, why is vn-ap a list, but
> ltp
> single? It is notable that site-network-access is a list by itself, so we
> can
> support multiple parallel access points by using multiple entries of the
> site-network-access list.
>
>
Changed to a list.



> 2.5) In the YANG module, there are a few lines of “//augment”. What do they
> mean?
>
>
Removed.



> 2.6) The module l3vpn-svc supports two types of vpn-topology: any-to-any
> and
> hub-spoke. For hub-spoke, there are site-roles of hub-role and
> spoke-role. It
> would be beneficial for this document to describe how each type of
> vpn-topology
> is mapped, and how the site-roles are mapped.
>
>
I don't think there is a change in the YANG model required -- each site
belonging to a VPN has a "site-role" and mapping of the site to TE takes
care of it. I have added text in the document to explain this!



> 3) All modules
>
> 3.1) All leaves are optional. Please double-check and/or add meaningful
> explanations. For example, for a template, what does an empty template do?
> For
> a l3-tsm, what does an empty mapping do? Are there any implications when a
> leaf
> is not configured?
>
>
Good suggestion! Updated.



> 3.2) There is no default on any leaf. Please double-check and/or add
> meaningful
> explanations. For example, if availability-type is not configured, what is
> the
> expected behavior?
>
>
Updated.



> 4) Examples
>
> 4.1) Sec 3.12 in RFC 8407 requires that “Example modules MUST be
> validated”.
> This can be done either by a validation tool or on a NETCONF/RESTCONF
> server.
> Some notable issues with the current examples are: 4.1.1) There are no
> namespaces on any of the data nodes. 4.1.2) Some leafref values need to
> appear
> in the referenced module.
>
> I used yanglint to verify. The xml one is still giving me an error, there
might be something off in my setup.



> 5) Editorial
>
> 5.1) Use of the term “YANG model”. The document mostly uses correct terms
> of
> “YANG data model” and “YANG module”, but there are still occurrences of
> “YANG
> model” that should be changed to “YANG data model”.
>
>
Ack



> 5.2) Sec 1.4. “Section 5 of this this document” should be “Section 6 of
> this
> document”
>
>
Updated



> 5.3)  “YANG codes” is better changed to “YANG modules”.
>
>
Ok



> 5.4) In each subsection of Sec 7, the document text does not have the
> references to the RFCs cited in the YANG module. These references also
> need to
> be listed in the document. Sec 5 of RFC7317 provides a good example.
>
>
Added in section 1.6



> 5.5) “Import network model” should be “Import network module”. The
> "import"
> statement makes definitions from one module available inside another
> module.
>
>
Ack.

Thanks!
Dhruv



> Thanks,
> - Xufeng
>
>
>
>