Re: [yang-doctors] Yangdoctors early review of draft-ietf-babel-yang-model-03

[Radek Krejčí <[e-mail]>]

Hi Mahesh,

Dne 16. 10. 19 v 0:43 Mahesh Jethanandani napsal(a):
>> The draft is well written with an informative overview of the YANG module
>> sections. Maybe some set of example configuration data (some Appendix) would be
>> useful.
> I am not sure of the ask. Could you elaborate?

ahh, sorry, my fault - I have missed the appendixes - that actually already contain exactly what I meant.

>> Regarding the YANG module, I have just a few notes:
>>
>> - In multiple description texts, nodes are referenced by a path in format with
>> dashes (e.g. babel-mac-default-apply). This format is confusing especially when
>> some element in the path contains dash(es). Why the common format with slashes
>> is not used?
> Ok. I can see that the text can be confusing. The text is from the information model (I-d.ietf-babel-information-model), where the name of the node just happens to follow the path in the YANG model. I can modify the text to use the node names in the YANG model itself.

I would prefer it. For example the mentioned babel-mac-default-apply - if you're not familiar with the tree, you don't know if to search for babel/mac-default-apply, babel/mac/default-apply or babel/mac/default/apply node.

>> - grouping routes
>>  - just think if the grouping is reusable - in the module itself, the grouping
>>  is used just once and needless creation of groupings decrease module
>>  readability
> It is a grouping so that it can be used with routing policy module.

ok

>> - babel/interfaces/metric-algorithm
>>  - the text "The value MUST be one of those listed in
>>  'metric-comp-algorithm'." in description just duplicates the type's
>>  information (identityref with metric-comp-algorithms base).
> How about if it says “The value MUST be one of those listed in ‘metric-comp-algorithm’ typedef.”?

I see no such typedef in the module (neither in the information model). This is probably again a confusion between information model and yang model. If it refers to information model, I don't think it is a good idea, but it should be at least explicitly stated. If it refers to the leaf's base identity, I don't think that it is necessary to duplicate this information (but I'm not strongly against it, it just make me believe that actually it is some additional restriction to the type itself), because there is already:

type identityref {
  base metric-comp-algorithms;
}

which also says that the value MUST be one of the identities based on metric-comp-algorithms. And technically, such identities can be added so the possible values do not need to be listed in some text/list/typedef. If you really want to limit the values to some static list, the type should be actually enumeration.

>> - babel/interfaces/mac-key-sets
>>  - what does the babel-mac-key-sets in description refer to?
> It now says:
>
>             "List of references to the mac entries that apply
>              to this interface. When an interface instance is
>              created, all mac instances with default-apply 'true'
>              will be included in this list.”

perfect!

>> - babel/interfaces/stats/reset
>>  - I'm confused, is it per-interface reset (as placed in the module) or
>>  system-wide interfaces stats reset (as information model defines it?). If it
>>  is the system-wide reset, why tha action cannot be placed in babel container?
> The action statement defines an operation connected to a specific container. What would it mean to have an action statement on the whole babel container? Reset everything?

but that is what the information model says, right? System-wide reset of the stats.Or is the meaning different?

> Even though the information model calls for a system-wide reset of stats, the model defines it at a per-interface level. It can be called recursively for every interface instance to realize a system-wide reset of stats.

that's what I miss in the action description. There is what the reset is supposed to do by information model (system-wide reset) and that in YANG the reset action has to be placed where the action needs to be performed. So according to the description, the reset is in a wrong container because to perform system-wide reset, it probably don't need to be performed in a specific interface, right? There is no word about intention to change what the reset is supposed to do (reset of stats for the specific interface).

Regards,
Radek