*** 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.