Subject: [OPSAWG] Yangdoctors last call review of draft-ietf-opsawg-l2nm-07

From: Ladislav Lhotka via Datatracker <[email-masked]>
Date: Tue, 05 Oct 2021 01:03:58 -0700

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.

Apart from that, the YANG modules are in a good shape.

**** 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. 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. - "vpn-service" list contains "vpn-description" leaf, not
"description". - "global-parameters-profile" list contains "svc-mtu" leaf, not
"mtu". - The value of "global-parameters-profile/rd-suffix" should be uint16,
not string (e.g. 1 rather than "1"). Similarly, "vpn-target/id" is uint8, not
string. - Container "vpn-targets" encapsulating "vpn-target" list doesn't exist
in the schema. - "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).
- Inside "vpn-service" list, the schema defines "vpn-nodes" container
encapsulating "vpn-node" list. This container is missing in the example.

***** 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

**** 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.

---

From: <[email-masked]>
Date: Tue, 5 Oct 2021 09:06:07 +0000

Hi Lada, 

Many thanks for the careful review.

Please see inline. 

Cheers,
Med

> -----Message d'origine-----
> De : Ladislav Lhotka via Datatracker <[email-masked]>
> Envoyé : mardi 5 octobre 2021 10:04
> À : [email-masked]
> Cc : [email-masked]; [email-masked];
> [email-masked]
> Objet : Yangdoctors last call review of draft-ietf-opsawg-l2nm-07
> 
> 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. 

> Apart from that, the YANG modules are in a good shape.

[Med] Thanks.

> 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. 

 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).

 - "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. 

> encapsulating "vpn-node" list. This container is missing in the example.

[Med] ACK.

> NOTE: '\' line wrapping per RFC 8792

[Med] Added this header, when appropriate: 

=============== NOTE: '\' line wrapping per RFC 8792 ================

> "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.

---

From: Ladislav Lhotka <[email-masked]>
Date: Tue, 5 Oct 2021 12:25:22 +0200

Hi Med,

> 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.

> [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.

> [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

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

---

From: "Joe Clarke (jclarke)" <[email-masked]>
Date: Tue, 5 Oct 2021 12:20:37 +0000

Thank you for you review, Lada.
> cannot be easily changed afterwards.

Agreed.  I'd like to hear other thoughts from the WG (and other authors)
on how/if this modularization should be done.

Joe