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

From: Dhruv Dhody <[email-masked]>
Date: Sat, 20 Feb 2021 18:55:24 +0530

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

Thanks!
Dhruv

On Sun, Dec 20, 2020 at 9:16 PM Andy Bierman via Datatracker <
[email-masked]> 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?
>
>
>
>

---

From: tom petch <[email-masked]>
Date: Fri, 26 Feb 2021 12:07:29 +0000

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

Hi Andy,

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

<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 <[email-masked]<mailto:[email-masked]>> wrote:
Reviewer: Andy Bierman
Review result: Ready with Issues

Major Issues:

  None

Moderate Issues:

1)

2)

3)
  rpc /vn-compute

Minor Issues:

4)

5)

6)

---

From: Andy Bierman <[email-masked]>
Date: Fri, 26 Feb 2021 07:36:05 -0800

> Sent: 20 February 2021 13:25
I agree with your comments.
Perhaps we should not be using 2 letter node names since they are likely to
be ambiguous over time.

Andy

> 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..
>
> 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
>
> [email-masked]<mailto:[email-masked]>> wrote:
> Reviewer: Andy Bierman

---

From: tom petch <[email-masked]>
Date: Fri, 26 Feb 2021 16:52:31 +0000

From: Andy Bierman <[email-masked]>
Sent: 26 February 2021 15:36
On Fri, Feb 26, 2021 at 4:07 AM tom petch <[email-masked]<mailto:[email-masked]>> wrote:
From: Teas <[email-masked]<mailto:[email-masked]>> on behalf of Dhruv Dhody <[email-masked]<mailto:[email-masked]>>
Sent: 20 February 2021 13:25

Hi Andy,

I agree with your comments.
Perhaps we should not be using 2 letter node names since they are likely to be ambiguous over time.

<tp>
TEAS is rather rich in them.

ap, as, id, ip, nt, nw, pw, te, tp, vn

and the augment paths go ten deep so a few extra characters can turn a four line path statement into something even harder to use:-(

Tom Petch

Andy

<tp>

Tom Petch

Thanks!
Dhruv

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

Major Issues:

  None

Moderate Issues:

1)

2)

3)
  rpc /vn-compute

Minor Issues:

4)

5)

6)