Reviewer: Jan Lindblad
Review result: Ready with Issues
YD review of draft-ietf-ccamp-mw-yang
I have now reviewed the YANG modules and corresponding examples of the -05
version of this draft. I have not read much of the RFC text, so I can't vouch
for how well the text aligns with the model. I find the proposed modules in
good shape. Most of my comments below are alternate ways of modeling something
that the wg may consider, or a few things where I propose a better option for
something that would be acceptable even as it is. There is a single issue that
must be fixed IMO, see #8 below.
Let's start with module ietf-microwave-types.
#1) Consider adding structure to related identities
This module consists mostly of a long list of identities based off of
coding-modulation. If it makes sense that in the future someone might be
interested in doing something with all qam-4096 based identities or all -strong
identities, say, it may make sense to model the identities as based on each
other in a tree style. E.g.
identity qam-4096 {
base coding-modulation;
description
"4096 QAM coding and modulation scheme.";
}
identity qam-4096-strong {
base qam-4096;
description
"4096 QAM strong coding and modulation scheme.";
}
identity qam-4096-light {
base qam-4096;
description
"4096 QAM light coding and modulation scheme.";
}
Or even go to "multiple inheritance" with multiple bases for identities, e.g.
for qam-4096 and strong. This would allow future applications to filter the
identities on such criteria. Just a thought.
#2) Convention to use all lowercase in YANG symbols
There are a couple of identities with capitals. Consider changing to all
lowercase; that is the YANG convention.
identity E1 {
identity STM-1 {
Next, let's look at module ietf-interface-protection.
I can't say I understand exactly why this is a separate module. It publishes a
single grouping, which is required by ietf-microwave-radio-link, and as far as
I understand would probably never be used anywhere else. When the grouping is
used a single time in ietf-microwave-radio-link, it is immediately refined.
Would probably reduce the clutter by merging the two modules and resolving the
refine.
#3) Config true leaf name status
I find it counter-intuitive that a leaf called status (or state) is a
configuration item. I had to re-read the model several times to get my head
around the fact that this is indeed meant to be config true. Perhaps consider a
name change?
leaf status {
#4) Action external-commands
There is a single action called external-commands (even in plural). It takes a
single argument, which is the name of the operation to execute. No output. To
me, a more natural modeling would be to make each of the external commands an
action, over time possibly with different input and output.
Finally, we have module ietf-microwave-radio-link.
#5) Use derived-from when comparing identities
It's more future-proof and more likely to be interoperable if you use proper
XPATH functions to determine kinship than using plain equality
augment "/if:interfaces/if:interface" {
when "if:type = 'mw-types:radio-link-terminal'";
is better written as
augment "/if:interfaces/if:interface" {
when "derived-from-or-self(if:type, 'mw-types:radio-link-terminal')";
This allows future sub-typing (sub-classing) of radio-link-terminal, i.e. new
identities that are based on radio-link-terminal to reflect some special kind
of RLT. It also improves chances of interoperability.
#6) Blank id reasonable?
Leafs that function as an id usually do not have defaults. Does a blank id make
sense here? If it does, maybe it would make more sense to leave it without a
default and explain what happens if not set in the description instead? Or mark
it as mandatory if it has to be set.
leaf id {
type string;
default "";
#7) Use derived-from when comparing identities (again)
leaf-list carrier-terminations {
type if:interface-ref;
must "/if:interfaces/if:interface[if:name = current()]"
+ "/if:type = 'mw-types:carrier-termination'" {
is better written as
must "derived-from-or-self(/if:interfaces/if:interface[if:name =
current()]"
+ "/if:type, 'mw-types:carrier-termination')" {
It is possible to write this in a more compact way, but there's nothing wrong
with the above.
must "derived-from-or-self(deref(current())/.."
+ "/if:type, 'mw-types:carrier-termination')" {
#8) Badly broken frequency duplex config
If you read the descriptions in these related leafs:
leaf tx-frequency {
type uint32;
units "kHz";
mandatory true;
description
"Selected transmitter frequency.";
}
leaf rx-frequency {
type uint32;
units "kHz";
description
"Selected receiver frequency.
Overrides existing value in duplex-distance.
Calculated from tx-frequency and duplex-distance if
only duplex-distance is configured.
Must match duplex-distance if both leaves are
configured in a single operation.";
}
leaf duplex-distance {
type uint32;
units "kHz";
description
"Distance between Tx & Rx frequencies.
Used to calculate rx-frequency when
rx-frequency is not specifically configured.
Overrides existing value in rx-frequency.
Calculated from tx-frequency and rx-frequency if only
rx-frequency is configured.
Must match rx-frequency if both leaves are configured
in a single operation.";
}
It appears that the author intends the system to fill in the value for one of
these leaves based on the value set for the other. This is a big no-no. A
system should never alter its own configuration, or automation flows (which is
the whole purpose with YANG and NETCONF, remember) will break. Also, the
validity of the configuration should not depend on how many operations are used
to inject it.
I find this a serious flaw that must be fixed before the module can be released.
I propose fixing it like this:
leaf tx-frequency {
type uint32;
units "kHz";
mandatory true;
description
"Selected transmitter frequency.";
}
choice freq-or-distance {
leaf rx-frequency {
type uint32;
units "kHz";
description
"Selected receiver frequency."
}
leaf duplex-distance {
type uint32;
units "kHz";
description
"Distance between Tx & Rx frequencies."
}
}
If you would like to have read-only computed values accessible in the model,
maybe you could add:
leaf actual-rx-frequency {
type uint32;
units "kHz";
description
"Computed receiver frequency."
config false;
}
leaf actual-duplex-distance {
type uint32;
units "kHz";
description
"Computed distance between Tx & Rx frequencies."
config false;
}
Many other ways of doing this properly are also possible. Let me know if you'd
like to discuss options.
#9) Check that lower threshold is less than upper threshold
Would it make sense to add a must statement checking that the lower threshold
is less than (or equal?) to the upper threshold?
leaf atpc-lower-threshold {
when "../power-mode = 'atpc'";
type power {
range "-99..-30";
}
units "dBm";
mandatory true;
description
"The lower threshold for the input power at far-end
used in the ATPC mode.";
reference "ETSI EN 302 217-1";
}
leaf atpc-upper-threshold {
#10) Choice more convenient
There are a few leafs that act as discriminators for when clauses in other
leafs. Such constructs might be a little smoother when modeled as a choice
instead. I'll take one and show as an example. This power-mode construct:
leaf power-mode {
type enumeration {
enum rtpc {
description
"Remote Transmit Power Control (RTPC).";
reference "ETSI EN 302 217-1";
}
enum atpc {
description
"Automatic Transmit Power Control (ATPC).";
reference "ETSI EN 302 217-1";
}
}
mandatory true;
description
"A choice of Remote Transmit Power Control (RTPC)
or Automatic Transmit Power Control (ATPC).";
}
leaf maximum-nominal-power {
type power {
range "-99..40";
}
units "dBm";
mandatory true;
description
"Selected output power in RTPC mode and selected
maximum output power in ATPC mode. Minimum output
power in ATPC mode is the same as the system
capability, available-min-output-power.";
reference "ETSI EN 302 217-1";
}
leaf atpc-lower-threshold {
when "../power-mode = 'atpc'";
type power {
range "-99..-30";
}
units "dBm";
mandatory true;
description
"The lower threshold for the input power at far-end
used in the ATPC mode.";
reference "ETSI EN 302 217-1";
}
leaf atpc-upper-threshold {
when "../power-mode = 'atpc'";
type power {
range "-99..-30";
}
units "dBm";
mandatory true;
description
"The upper threshold for the input power at far-end
used in the ATPC mode.";
reference "ETSI EN 302 217-1";
}
could be modeled as:
choice power-mode {
container rtpc {
description
"Remote Transmit Power Control (RTPC).";
reference "ETSI EN 302 217-1";
leaf maximum-nominal-power {
type power {
range "-99..40";
}
units "dBm";
mandatory true;
description
"Selected output power.";
reference "ETSI EN 302 217-1";
}
}
container atpc {
description
"Automatic Transmit Power Control (ATPC).";
reference "ETSI EN 302 217-1";
leaf maximum-nominal-power {
type power {
range "-99..40";
}
units "dBm";
mandatory true;
description
"Selected maximum output power. Minimum output
power is the same as the system
capability, available-min-output-power.";
reference "ETSI EN 302 217-1";
}
leaf atpc-lower-threshold {
type power {
range "-99..-30";
}
units "dBm";
mandatory true;
description
"The lower threshold for the input power at far-end
used in the ATPC mode.";
reference "ETSI EN 302 217-1";
}
leaf atpc-upper-threshold {
type power {
range "-99..-30";
}
units "dBm";
mandatory true;
description
"The upper threshold for the input power at far-end
used in the ATPC mode.";
reference "ETSI EN 302 217-1";
}
}
mandatory true;
description
"A choice of Remote Transmit Power Control (RTPC)
or Automatic Transmit Power Control (ATPC).";
}
#11) Choice more convenient (again)
Same thing again for
leaf coding-modulation-mode {
#12) Unusual exponential notation
Do you really mean 10e-9 (=10*10^-9 =10^-8), or do you mean the more
traditional notation 1e-9?
leaf ber-alarm-threshold {
type enumeration {
enum "10e-9" {
#13) Separate module with grouping, used a single time with refine
Module ietf-interface-protection defines a grouping (protection-groups), which
is used a single time, yet is refined when it is used below. As noted before
comment #3, I find this way of laying out the YANG unnecessarily hard to read
and understand, for no clear benefit.
container radio-link-protection-groups {
....
uses ifprot:protection-groups {
refine protection-group/members {
must "/if:interfaces/if:interface[if:name = current()]"
+ "/if:type = 'mw-types:carrier-termination'" {
Also, as noted in comment #7, the must statement is better written using
derived-from-or-self. This applies regardless of the current refine statement
is kept, or if the must statement moves to the actual leaf-list it applies to.
Appendix A.1 & A.2
Besides the actual YANG modules, there are also a couple of examples in
Appendix A.1 and A.2. I tried to use them and uncovered a couple of issues.
#14) Config false leaf in config example
Both examples list
"tx-oper-status": on
This is a config false item, which could never be part of a configuration
message (and it also lacks comma at the end).
#15) Wrong type in example
Both examples list
"coding-modulation-mode": 0,
0 not a legal value, should be "single" to match the rest of the example data.
#16) Missing mandatory parameter
Both examples lacks leaf maximum-nominal-power, which is mandatory according to
the YANG, so the transaction fails to validate.
#17) Examples perhaps a tad basic
The examples are demonstrating only a small part of the module functionality. A
bigger example, e.g. including xpic with interface pointers might be useful.
Feel free to reach out to me to discuss any of this. Thank you.
/jan
