IETF Last Call Review of draft-ietf-ccamp-dwdm-if-param-yang-13
review-ietf-ccamp-dwdm-if-param-yang-13-yangdoctors-lc-lindblad-2025-07-31-01

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.