This is my second go at a last call YANG Doctor review of draft-ietf-ccamp-dwdm-if-param-yang-13. The first go was based on an incorrect set of imported modules, which gave a rather abysmal result. This time the result is better, I'll call it Almost Ready. There are a couple of blocking problems that will force some module redesign, but for most part the modules are in good shape with a lot of polishing work remaining. The modules under review this time are: ietf-wdm-interface.yang (from draft-ietf-ccamp-dwdm-if-param-yang-13) ietf-layer0-types@2025-08-06.yang (from draft-ietf-ccamp-rfc9093-bis-16) plus context modules: ietf-te-types@2025-01-24.yang ietf-interfaces@2018-02-20.yang ietf-network@2018-02-26.yang ietf-network-topology@2018-02-26.yang ietf-routing-types@2017-12-04.yang ietf-inet-types@2013-07-15.yang ietf-yang-types@2013-07-15.yang #1) Importing l0-types The wdm-interface module imports the l0-types module. But not just any l0-types module version, it requires one of recent date. This is not mentioned in the import statement, which will inevitably cause unnecessary delay and confusion among implementors (and reviewers). At the very least, add a comment that explains the situation, but I would recommend considering leveraging the work of the YANG module versioning design team. This way, you could even declare the dependency in machine readable way. See https://datatracker.ietf.org/doc/html/draft-ietf-netmod-yang-module-versioning-13 Change: import ietf-layer0-types { prefix "l0-types"; } To: import ietf-layer0-types { // Requires at least revision 2025-01-01 prefix "l0-types"; } Or: import ietf-layer0-types { rev:recommended-min-date 2025-01-01; // This module will not work with earlier versions prefix "l0-types"; } #2) Invalid leafrefs to operational state One of the key concepts in the module is a list of config false (i.e. controlled by the server) "Optical Interface explicit-modes" templates that are referenced by the interface WDM configuration. This sort of reference from config true (the interface config) to a config false list (the templates) is illegal in YANG. It actually breaks one of the core, fundamental tenets of YANG, which is that the validity of a particular configuration must never depend on the operational state of the device. I.e. a particular configuration is either valid, or it isn't. It never depends on the phase of the moon, the prior configuration or what some other system is up to. This is one of the core reasons NETCONF and YANG were invented; SNMP doesn't have this mindset, and it turned out really complicated to manage devices because they often behave like pinball machines with a lot of internal state to stumble over while the management client tries to tell the machine what to do. So how can this be fixed? There are a few options. a) Use "weak" leafrefs YANG allows references from config true to config false data if you add a sub-statement require-instance false; to the leafref. This means the reference will not be validated, and the client can provide any value for the reference, even including values that don't exist in the template list. I would recommend this option if you think the set of templates will stay pretty stable for a given implementation, e.g. the list only changes at software upgrade or perhaps when hot-inserting new hardware. If you choose this option, I would recommend that you describe what happens if a client configures a value that is not valid (never was or no longer is), e.g. because someone has ejected the piece of hardware that supported a given mode, and the server removed the associated template from the list. b) Let the client copy Instead of pointing to a template, you could model the mode selection as a container with all the leafs configurable. The client could then copy the complete set of values from the template of choice to the interface configuration. c) Model the template as config true data YANG would allow the reference to the template if the template was modeled as config true data. You could still make it not writable with a NACM access control rule, so that no user can modify that part of the configuration. leaf explicit-transceiver-mode-ref { type leafref { path "../../../../../../wdm-if:wdm-if-templates" + "/wdm-if:explicit-transceiver-modes" + "/wdm-if:explicit-transceiver-mode" + "/wdm-if:explicit-transceiver-mode-id"; } #3) Unusable and unneeded -or-null types In the l0-types module, there are a good number of typedefs named xxx-or-null, e.g. typedef power-dbm { ... } typedef power-dbm-or-null { type union { type power-dbm; type empty; } ... leaf rx-ref-channel-power { type power-dbm; This is legal YANG, but I would strongly advise against this. The wire representation of a type empty inside a union is a corner case that in practice isn't likely to be interoperable across protocols and vendors. If you really need such a type, I could explain how to do it in a way that works, but I'd bet you don't actually need any of theese xxx-or-null types at all. Normally in YANG, you would define a type, e.g. typedef power-dbm, and let the modeler using this type in a leaf decide if it can be null or not, like this: typedef power-dbm { ... } // Removed typedef power-dbm-or-null ... // Use this leaf construct when the value can be null: leaf rx-ref-channel-power { type power-dbm; } // Use this leaf construct when the value cannot be null: leaf rx-ref-channel-power { type power-dbm; mandatory true; } #4) Review leaf optionality globally Related to #3 above, I get a feeling the modelers have assumed that every leaf in YANG is mandatory unless declared otherwise. But in YANG, it's actually the other way around. Every leaf in YANG is optional unless specifically declared mandatory (or is a key in a list). I would advise that the modeling team takes a session to go through the modules top to bottom and consider which leafs are optional and which are mandatory, and maybe add defaults in some cases. When leafs are optional, it is often a good idea to explicitly define the expected behavior when no value is present. Different implementors may come to different conclusions otherwise, hurting interoperability. Here are three random examples of leafs that are optional, have no default and no description of what happens if they are not present: leaf number-of-tcas-supported { leaf min-osnr-margin { leaf central-frequency { Note that there are some restrictions on how you can use mandatory together with augment. #5) decimal64 usage I see many type definitions based on decimal64 with a description like below: typedef decimal-5 { type decimal64 { fraction-digits 5; } description "A decimal64 value with five digits."; } A decimal64 value allows up to 18 digits, and the fraction-digits statement says how many of them are behind the decimal point. In the leaf above, up to 13 digits before the decimal point and 5 behind. Here is a leaf with 18 fraction digits and no digits before the decimal point: leaf pre-fec-ber { type decimal64 { fraction-digits 18; } config false; description "Pre-FEC errored words"; } I don't know much about how pre-fec-ber's should be represented, but the way it is modeled now, it will be a value between 0 and 1 with 18 digits after the decimal point. If this is meant to be a really large counter, I'd suggest modeling as uint64 instead. #6) Extensible set of enums This question is found near the top of the module: // Need to verify if the enumeration is the proper approach for // the tca-types, since the list is not fully standardized adn // subject to change. typedef wdm-if-tca-types { Good insight -- if the set of valid values is partly unknown at the time of the modeling (e.g. can grow over time, or vendor extensions are welcome), then modeling the set as a collection of YANG "identity"s might be preferable. Enums are closed sets that can only be changed through a new revision of the module where they are defined. Identities can be defined in separate or vendor modules, and also support subclassing. One detail to consider is that this type is used as a key in a list: list tca-list { key "tca-type"; ... leaf tca-type { type wdm-if-tca-types; If tca-type is defined as an enum, exactly one of each type can be present in the list. With identities, things get a little more complex. Let's say all the enumeration values are remodeled as identities: identity cd-tca { base tca-type; description "The chromatic dispersion TCA"; Then a vendor comes around and implements some sort of more refined variant of cd-tca (I'm making this up out of the blue) identity cd-tca-red { base wdm-if:cd-tca; description ...; } identity cd-tca-blue { base wdm-if:cd-tca; description ...; } This would now allow the client to use cd-tca-red, cd-tca-blue and cd-tca as keys all at the same time in the list. If you go this way, maybe some explanation of what happens with extensions and possible future overlap would be in order. #7) Unhelpful descriptions Many descriptions have been added to silence warnings, but they are not helpful. Here's an example. What does the reader get out of the description here? I am no wiser as to what those abbreviations might mean superficially nor in depth. enum pre-fec-ber-tca { description "Pre Fec BER TCA"; I'd suggest going over all descriptions to ensure they describe the leafs and values properly, spell out any abbreviations at least, and perhaps what they should be used for. What does it mean if the leaf isn't set? #8) A few identifiers with uppercase symbols YANG is case sensitive. In order to make operators happy and programmers unsurprised, it is recommended to use all lowercase identifiers in YANG modules. These modules almost do, but I found a few that will have a high surprise factor. I'd suggest going all lower case. leaf min-OSNR { leaf min-Q-factor case G.698.2 { #9) transceiver-capabilities Structurally the grouping transceiver-capabilities in l0-types looks like this: list supported-mode { ... choice mode { mandatory true; ... case G.698.2 { // [introduce container here?] uses standard-mode; uses common-standard-organizational-mode; uses common-all-modes; // [end of container?] } case organizational-mode { container organizational-mode { uses organizational-mode; uses common-standard-organizational-mode; uses common-all-modes; } } case explicit-mode { container explicit-mode { uses common-all-modes; container compatible-modes { ... } } } } } The first case, G.698.2 does not have a container like the other two cases. That makes all the leafs from that case "spill out" directly in the list surrounding the choice. That might not give the best user experience. If you could introduce a container there like in the other cases, I think it would be appreciated by the users. Inside the explicit mode, there are two leaf-list I don't quite understand leaf-list supported-application-code { type leafref { path "../../../../supported-mode/mode-id"; } must '../../../../' + 'supported-mode[mode-id=current()]/' + 'standard-mode' { This is a config false leaf-list, meaning its contents is controlled by the server (not the client). It is not very common to have must statements in config false lists. It means you require the server implementors to only populate the list with list entries that fulfill the condition. In this case, the condition is that the server must populate the list only with entries of suppoerted-mode G.698.2 that have a value on the leaf standard-mode (it's an optional leaf). The next leaf-list looks like this: leaf-list supported-organizational-mode { type leafref { path "../../../../supported-mode/mode-id"; } must '../../../../' + 'supported-mode[mode-id=current()]/' + 'organizational-mode' { Here, the condition is that the server must populate the list only with entries of suppoerted-mode organizational-mode (it's a container, which exists for all those entries). I noted that grouping organizational-mode has a leaf operational-mode (not organizational-mode), which is different than the pattern in most other groupings in the module. Is this intended? grouping organizational-mode { ... leaf operational-mode { ... } leaf organization-identifier { ... } } In grouping standard-mode, I noticed that leaf standard-mode is optional. Is that intended? grouping standard-mode { ... leaf standard-mode { type standard-mode; description "Identifies an ITU-T G.698.2 standard application code. It MUST be a string with a format that follows the nomenclature defined in clause 5.3 of ITU-T G.698.2."; } } Let me know if you have any questions or comments.