Re: [OPSAWG] Yangdoctors early review of draft-ietf-opsawg-vpn-common-02

["Joe Clarke (jclarke)" <[e-mail]>]

I thought I had sent this out, Radek, but I guess it got missed with the
winter break.

Thank you so much for your review!  This will help to shore up an
important piece of this overall network modeling work.

Joe

On 12/18/20 07:14, Radek Krejčí via Datatracker wrote:
> Reviewer: Radek Krejčí
> Review result: Ready with Issues
>
> *** draft ***
>
> Seems well written with a clear statement about its purpose and describing the
> groupings of the module.
>
> *** module ***
>
> * layout
> Use pyang's `--yang-canonical` option to unify the order of the statements and
> make the order canonical - e.g. starting the groupings with description
> improves readability of the module. Using pyang to print the module would also
> remove probably forgotten comment `//L2xMs`.
>
> * typo
> - Module's description: s/Section 4.c/Section 4/
>
> * features
> Almost none of the features is actually used in the module, which might be
> fine, but there are identities referring to the same areas as the features, so
> I believe that these identities should have if-feature statement referring to
> the appropriate feature defined in the module.
>
> * enumeration typedefs
> Since the enumeration cannot be extended, are you really sure, that the
> enumeration types you define are really complete forever? I would say that
> address families, negotiation modes and  control modes might need extensions in
> the modules that will use those types. Defining it as identityref with
> specified identities instead of enums would solve the problem.
>
> * `service-status` and `status-timestamp` groupings
> Both groupings seem to have config false meaning.  But only the
> service-status/status/oper-status container is defined as config false. The
> uses statement doesn't have its own config statement, so if you want to place
> the mentioned groupings into config true data, an extra grouping or refine will
> be required. The common sense of the groupings is config false, so define them
> this way. If there will be some exception to make them config true, the uses
> can refine it in such an exceptional case.
>
> * grouping rt-rd/rd-choice
> There are 2 cases (pool-assigned and full-autoassigned) which seem to have a
> node with the same meaning, but since there would be a name collision, they
> have different names `rd-assign` and `rd-assigneed`. I don't think that this is
> a good solution. If they have the same meaning, they should have also the same
> name. I see 2 possible solutions: - move the leafs in the cases into a
> container - it adds another level in data, but allows a common name
> `rd-assigned` - move the `rd-assigned` leaf outside the choice. If it makes
> sense, it can be constrained by must/when to the (non)-presence of the nodes
> from the choice. The main point here is that the rd-assigned status is then
> always at the same place.
>
> * grouping group/
> The grouping seems odd to me, like it misses something or it might be just by
> too generic name. When looking into the `placement-constraint` grouping, it
> seems that it refers to the groupings, but since they are separated, it is not
> possible to refer to the group-id explicitly. Is the `group` grouping intended
> to be instantiated standalone, without `placement-constraint`? If not, join
> them together and refer the group-id from target-flavor via leafref.
>
>
>
>