Re: [Teas] Yangdoctors early review of draft-ietf-teas-actn-vn-yang-10

[tom petch <[e-mail]>]

From: Teas <[e-mail]> on behalf of Dhruv Dhody <[e-mail]>
Sent: 20 February 2021 13:25

Hi Andy,

Thanks for your review. The new -11 version is out which takes your comments into consideration.

https://datatracker.ietf.org/doc/draft-ietf-teas-actn-vn-yang/
A diff from the previous version is available at:
https://www.ietf.org/rfcdiff?url2=draft-ietf-teas-actn-vn-yang-11

<tp>
I liked most of Andy's comments but not the one that leads to/vn/vn/vn-id or/ap/ap/ap-id or /ap/ap/vn-ap/vn-ap-id.  I agree that the original was not good but am not sure that repeated 'ap'  or 'vn' add clarity.

However, a more substantial comment relates to vn-compute where the compute status uses te-common-status which allows for up, down, testing, maintenance; mmmm.  This is not the only I-D to compute and they seem to be heading in different directions where success/failure and reasons therefore are concerned..

Ditto error-info which here is RYO while others import from  yang-te.

YANG is bad at having a list which users only want to use a part of (e.g. Hash algorithms) but the identifiers could be coordinated at least within one IETF WG..

Tom Petch



Thanks!
Dhruv

On Sun, Dec 20, 2020 at 9:16 PM Andy Bierman via Datatracker <[e-mail]<mailto:[e-mail]>> wrote:
Reviewer: Andy Bierman
Review result: Ready with Issues


Major Issues:

  None

Moderate Issues:

1)
   leaf /ap/access-point-list/access-point-id
   leaf /vn/vn-list/vn-id
   leaf /vn/vn-list/vn-member-list/vn-member-id

   These list keys use type inet:uri.
   You should consider the implementation complexity here.
   Will servers all correctly convert any arbitrary URI to its canonical
   representation? The draft should address this issue.

2)
  leaf /vn/vn-list/oper-status
  leaf /vn/vn-list/admin-status

  These objects use vn-status-type, vn-admin-state-type
  The use of identities for even simple "up/down" status types
  seems extreme. The conformance for an enumeration is clear
  (mandatory), but not for an identityref type.
    - E.g., Is is mandatory for a vendor to support vn-state-up,down?
      A vendor could write their own identities and ignore the standard
      identities.


3)
  rpc /vn-compute
  The procedure for this operation is not explained here.
  A full description or reference to normative test is needed.
   - what does the server do with the input?
   - what output is expected? Any variants based on the inputs
     should be explained.
   - any interoperability considerations wrt/ use of these
     common groupings in this RPC context?
   - what errors can occur? Specify any error-tags, etc. that
     the server MUST/SHOULD include in the response


Minor Issues:

4)
 - naming inconsistent within /ap and /vn
   access-point is spelled out and vn is not
   Suggest: shorten access-point to ap

5)
 - naming a list entry with the the suffix -list is redundant.
   YANG lists do not have any conceptual container or way to
   reference all the entries (if that what this naming intends)
   Suggest:
     s/access-point-list/ap/
     s/vn-list/vn/
     s/vn-member-list/vn-member/

6)
  - leaf /vn/vn-list/vn-member-list/src/multi-src
  - leaf /vn/vn-list/vn-member-list/dest/multi-dest
    Looks like these leafs should each have a default-stmt.
    What does it mean if the multi-src-dest feature is supported
    but these leafs are missing from the config?