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

[Xufeng Liu <[e-mail]>]

Hi Dhruv,

Thanks for updating the document. The changes look good, and the examples
can be verified later.
Best,
- Xufeng

On Mon, Jul 11, 2022 at 10:04 AM Dhruv Dhody <[e-mail]> wrote:

> 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 <
> [e-mail]> 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) [e-mail]
>>
>> 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) [e-mail]
>>
>> 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
>>
>>
>>
>>
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=icon>
Virus-free.
www.avast.com
<https://www.avast.com/sig-email?utm_medium=email&utm_source=link&utm_campaign=sig-email&utm_content=webmail&utm_term=link>
<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>