Re: [yang-doctors] Yangdoctors early review of draft-ietf-opsawg-l3sm-l3nm-03

[Oscar González de Dios <[e-mail]>]

Dear Radek,

Thank you very much for the review. First of apologies for the delay in proving the reviewed version. The comments from the review and additional feedback from the mailing list drove us though a longer path by which it was evaluated the possibility of creating a common  YANG module to be reused  by various VPN-related modules such as Layer 3 VPN Service Model,  Layer 2 VPN Service Model, Layer 3 VPN Network Model, and Layer 2 VPN  Network Model. After discussions in the list and IETF virtual meeting, it was agreed among the WG to move though that path and create the separate module, , a new vpn-common module called " A Layer 2/3 VPN Common YANG Model", which you can find in  https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-00. Hence, the L3NM module was cleaned up to be used with the common module.

We have addressed all the comments in draft-ietf-opsawg-l3sm-l3nm-05: https://tools.ietf.org/html/draft-ietf-opsawg-l3sm-l3nm-05  . The version you reviewed was draft-ietf-opsawg-l3sm-l3nm-03.  Please use those two in the diff comparison.

Please find bellow the detailed answers to the review of L3NM in which the actions taken are explained.

Reviewer: Radek Krejčí
Review result: Ready with Issues

The I-D document contains a single YANG module ietf-l3vpn-ntw.

The module is quite complex, but despite I'm not an expert in the area, the text of the draft seems very descriptive and helpful to understand all the parts of the module tree. From my perspective, the most problematic part of the module is the overall use of groupings and the current status of their specification in the module.

Here are the specific issues:

- the module seems NMDA compliant, so it should be mentioned in Introduction (or some other) section:

    The YANG data model in this document conforms to the Network
    Management Datastore Architecture (NMDA) defined in RFC 8342.
[Oscar] The sentence has been added to the end of the introduction section and the reference to RFC 8342 has been added.

- the filename's revision part does not match the latest revision date
(2020-04-03) of the module:
  <CODE BEGINS>  file "[e-mail]"
[Oscar] They do match now. <CODE BEGINS>  file "[e-mail]"
                 revision 2020-10-16 {

- all the imported modules must have their document (RFC) in the normative references section, but RFC 6991, RFC 8294, RFC 8299 and RFC 8519 are missing.
[Oscar] All the references have been added in the imports in the Yang module and in the normative references section (RFC 6991, RFC 8519 ). In the new version, the commonalities with L3SM (RFC 8299) have been addressed in the vpn-common module,  so reference to RFC 8299 in the module is not needed . Similarly, the imports from RFC 8294 are used in the common module, so they are not needed any more in this draft. A reference to the common module has been added.

- since RFC 8277 is referenced in the module, it should appear in the normative references section.
[Oscar] RFC 8277 has been added to the normative references section

- additional indentation (3 spaces on each line except the first one) in the module's contact statement seems weird and unnecessary/unintentional.
[Oscar] Fixed.

- some of the descriptions in the module use RFC 2119 key words, so the module's description is supposed to contain the following text:

     The key words 'MUST', 'MUST NOT', 'REQUIRED', 'SHALL', 'SHALL
     NOT', 'SHOULD', 'SHOULD NOT', 'RECOMMENDED', 'NOT RECOMMENDED',
     'MAY', and 'OPTIONAL' in this document are to be interpreted as
     described in BCP 14 (RFC 2119) (RFC 8174) when, and only when,
     they appear in all capitals, as shown here.

[Oscar] In the description clauses we are don’t use any normative language. In this version we have removed all normative language from the description clauses.

- typedef protocol-type;
  - Are you really sure that the list of the underlying protocols is fixed
  forever in all its instances? Having it in the form of identity would allow
  later augmenting of the list of possible values.
[Oscar] Thanks for the suggestion. The protocol-type is now an identity. It has been moved to https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-00 as it can be reused by L2NM too.

- the whole groupings part need a cleanup and maybe some structural changes:
  - most of the groupings are instantiated only once - having them defined this
  way makes sense only in case you believe they are useful for other modules to
  use. Splitting a complex module into groupings just for keeping its parts
  separated IMO decreases the readability and usability of the module - it is
  quite challenging to find a specific path or subtree because of intensive
  skipping from grouping to another. - there are also groupings that are not
  instantiated at all, if they are intended exclusively for use by other
  modules, I would prefer to have some note about it in their descriptions. -
  there are several comment out uses statements, there is also whole /* UNUSED
  */ part

[Oscar] We have done a complete revision of the groupings. After several discussions within the working group, as mentioned earlier, we decided to find all the types and groupings that are reusable between L2NM and L3NM (l2nm is a complementary work covering layer 2 VPNs). These types and grouping have been revisited and as a result, a new vpn-common module,  https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-00, has been created. That document defines a common YANG module that is meant to be reused  by various VPN-related modules such as Layer 3 VPN Service Model,  Layer 2 VPN Service Model, Layer 3 VPN Network Model, and Layer 2 VPN  Network Model. Hence, in this l3nm draft the groupings from the common module are used. Now, only 2 groupings are left in the new l3nm, versus  34 groupings in the old version.

There are no longer "unused" groupings. The descriptions have also been updated and references added for better readability.

- description of the service-status grouping seems to be invalid

[Oscar]Fixed. Service –status now in https://tools.ietf.org/html/draft-ietf-opsawg-vpn-common-00

- descriptions of {grouping site-bearer-params}/site-bearers/bearer/bearer-id
and {grouping vpn-route-targets}/vpn-polices nodes are empty
[Oscar]Fixed. The vpn-policies leaf is in the common module now. There is no longer bearer-id.

- I'm afraid about the usability of appendix A. The status of the implementations will tent to change quite intensively and having such information in RFC does not make much sense to me.

[Oscar] The implementation status section follows the guidelines RFC6982. As per RFC6982 guidelines, the implementation status section in the Appendix  is to be removed once the document is published as an RFC.  We have added the boilerplate text suggested by RFC6982 with the motivation of the section and an editor's note for RFC Editor for the removal of the section upon RFC publication.

Best  Regards,

Oscar on behalf of L3NM authors and contributors.





________________________________

Este mensaje y sus adjuntos se dirigen exclusivamente a su destinatario, puede contener información privilegiada o confidencial y es para uso exclusivo de la persona o entidad de destino. Si no es usted. el destinatario indicado, queda notificado de que la lectura, utilización, divulgación y/o copia sin autorización puede estar prohibida en virtud de la legislación vigente. Si ha recibido este mensaje por error, le rogamos que nos lo comunique inmediatamente por esta misma vía y proceda a su destrucción.

The information contained in this transmission is privileged and confidential information intended only for the use of the individual or entity named above. If the reader of this message is not the intended recipient, you are hereby notified that any dissemination, distribution or copying of this communication is strictly prohibited. If you have received this transmission in error, do not read it. Please immediately reply to the sender that you have received this communication in error and then delete it.

Esta mensagem e seus anexos se dirigem exclusivamente ao seu destinatário, pode conter informação privilegiada ou confidencial e é para uso exclusivo da pessoa ou entidade de destino. Se não é vossa senhoria o destinatário indicado, fica notificado de que a leitura, utilização, divulgação e/ou cópia sem autorização pode estar proibida em virtude da legislação vigente. Se recebeu esta mensagem por erro, rogamos-lhe que nos o comunique imediatamente por esta mesma via e proceda a sua destruição