Re: [yang-doctors] Yangdoctors last call review of draft-ietf-opsawg-l2nm-07

[Ladislav Lhotka <[e-mail]>]

Hi Med,

On 05. 10. 21 11:06, [e-mail] wrote:
> Hi Lada,
> 
> Many thanks for the careful review.
> 
> Please see inline.
> 
> Cheers,
> Med
> 
>> -----Message d'origine-----
>> De : Ladislav Lhotka via Datatracker <[e-mail]>
>> Envoyé : mardi 5 octobre 2021 10:04
>> À : [e-mail]
>> Cc : [e-mail]; [e-mail];
>> [e-mail]
>> Objet : Yangdoctors last call review of draft-ietf-opsawg-l2nm-07
>>
>> Reviewer: Ladislav Lhotka
>> Review result: Ready with Issues
>>
>> **** General comments
>>
>> The ietf-l2vpn-ntw module with about 400 data nodes represents an
>> impressive amount of work. Its size, however, raises some concerns in
>> terms of manageability. For example, if the ITU-T Y-1731 recommendation
>> ever gets updated (I don't know how likely this is), the module will have
>> to be updated.
>> I would therefore suggest to consider factoring out such parts into
>> separate modules.
> 
> [Med] We have already made an effort to factorize many items in:
> * I-D.ietf-vpn-common
> * two separate IANA-maintained modules
> 
> One candidate "externalization" that I think would work to address your concern is to move ethernet-segments (and esi types) into a separate module.
> 
> I have a preference to work in that direction vs. touching the OAM part.
> 
> Would that solve your concern? Thank you.

I think it is up to the authors and WG to consider what to do, maybe 
nothing. I am not an expert in this domain, so I have no strong opinion, 
but it is certainly better to think twice because the module structure 
cannot be easily changed afterwards.

> 
>>
>> Apart from that, the YANG modules are in a good shape.
> 
> [Med] Thanks.
> 
>>
>> **** Specific comments
>>
>> ***** Instance data examples
>>
>> The appendices A.1--A.6 contain examples of instance data for six
>> different use cases. This would be laudable, but only if the examples were
>> valid according to the data model schema. It seems that the examples were
>> not updated during the data model development.
> 
> [Med] Many thanks for looking into these. Will check and fix as appropriate.

I discovered all these issues with Yangson. If you want, I can send you 
my testing environment.

> 
>   It is necessary to validate
>> the examples using the available tools such as yanglint or Yangson, and
>> correct all errors. Here is a non-exhaustive list of problems in the
>> example of appendix A.1: - The top-level container "ietf-l2vpn-ntw:l2vpn-
>> ntw" is missing, which complicates the use of validation tools.
> 
> [Med] We don't include the top-level as we assumed this will be included in the request URI (RESTCONF).

I understand, but in this case it is only two extra lines. In fact, the 
example in A.3 includes this top-level container, although with missing 
namespace (it should be "ietf-l2vpn-ntw:l2vpn-ntw").

Cheers, Lada

> 
>   - "vpn-
>> service" list contains "vpn-description" leaf, not "description".
> 
> [Med] ACK
> 
>   -
>> "global-parameters-profile" list contains "svc-mtu" leaf, not "mtu".
> 
> [Med] ACK
> 
>   - The
>> value of "global-parameters-profile/rd-suffix" should be uint16, not
>> string (e.g. 1 rather than "1").
> 
> [Med] ACK. This one was already fixed in our local copy as you can see in this commit: https://github.com/IETF-OPSAWG-WG/lxnm/commit/8cd6863c52e956ccd490df1a002d786d6de8628f#diff-17b3d33597c20b9786dd2dbedebdbcdb61e16e366431e0411761467ec8aae248.
> 
>   Similarly, "vpn-target/id" is uint8, not
>> string.
> 
> [Med] This one was already fixed in -07:
> 
>                     "vpn-target": [
>                       {
>                         "id": 1,
> 
> 
>   - Container "vpn-targets" encapsulating "vpn-target" list doesn't
>> exist in the schema.
> 
> [Med] ACK
> 
> 
>   - "vpn-target/route-targets" is defined as a list,
>> but its instance value is given as ["0:65550:1"] (it probably used to be a
>> leaf-list).
> 
> [Med] I confirm. We added the list thing and explained in the vpn-common the reason why we are using a list not a leaf-lsit.
> 
>> - Inside "vpn-service" list, the schema defines "vpn-nodes" container
>> encapsulating "vpn-node" list. This container is missing in the example.
> 
> [Med] ACK.
> 
>>
>> ***** RFC 8792 line folding
>>
>> My reading of sec. 7.1.1 in RFC 8792 is that each text chunk that uses the
>> single backslash strategy should be preceded with the header
>>
>> NOTE: '\' line wrapping per RFC 8792
> 
> [Med] Added this header, when appropriate:
> 
> =============== NOTE: '\' line wrapping per RFC 8792 ================
> 
>>
>> **** Nits
>>
>> - The naming of groupings is inconsistent: "cfm-802-grouping" versus e.g.
>> "y-1731". I'd suggest to remove the "-grouping" suffix in the former.
>>
> 
> [Med] Fixed in my local copy.
> 
> _________________________________________________________________________________________________________________________
> 
> 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.
> 

-- 
Ladislav Lhotka
Head, CZ.NIC Labs
PGP Key ID: 0xB8F92B08A9F76C67