Hi CCAMP,
Regarding this one:
“I would like to ask CCAMP for support with the question about if the protection group is likely to be reused elsewhere or if it can be merged with the microwave module again.”
Protection groups already exist in various drafts, e.g. the following in TE-tunnel.
leaf protection-group-ingress-node-id {
type te-types:te-node-id;
description
"Indicates the te-node-id of the protection group
ingress node when the APS state represents an extenal
command (LoP, SF, MS) applied to it or a WTR timer
running on it. If the external command is not applied to
the ingress node or the WTR timer is not running on it,
this attribute is not specified. If value 0.0.0.0 is used
when the te-node-id of the protection group ingress node is
unknown (e.g., because the ingress node is outside the scope
of control of the server)";
i’m not sure if we already have a generalized protection group or we have a specific one already there and hence this one would be a specific one for microwave.
If the former I’d remove the definition of a protection group in the MW draft and point and extend the existing one, if the latter it’s ok to merge the two modules since everything is
microwave specific.
Can you help me understand what’s the case?
BR
Daniele
From: Jan Lindblad <janl@xxxxxxxxxx>
Sent: martedì 12 giugno 2018 02:34
To: Jonas Ahlberg <jonas.ahlberg@xxxxxxxxxxxx>
Cc: Yemin (Amy) <amy.yemin@xxxxxxxxxx>; yang-doctors@xxxxxxxx; ccamp@xxxxxxxx; draft-ietf-ccamp-mw-yang.all@xxxxxxxx; ietf@xxxxxxxx
Subject: Re: [CCAMP] Yangdoctors last call review of draft-ietf-ccamp-mw-yang-05
Jonas,
Comments below marked [janl3].
See comments & questions in green [Jonas2].
Thank you for your review and valuable comments.
I’ve not responded to the latest mail since I have added comments to some of the items removed in that mail.
I’ve copied and pasted your comments into this mail instead, in order to make the thread of comments complete.
I’ve added my comments in green [Jonas].
[janl2] Very good. Sorry for deleting text prematurely ;-)
I would like to ask CCAMP for support with the question about if the protection group is likely to be reused elsewhere or if it can be merged with the microwave module again.
Thanks very much for the comments. I found them are quiet useful to improve the models.
Please see my reply below in blue.
The draft co-authors may add more reply to your comments.
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.
"4096 QAM coding and modulation scheme.";
identity qam-4096-strong {
"4096 QAM strong coding and modulation scheme.";
identity qam-4096-light {
"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.
[Amy] change to base qam-4096. Multiple bases seem not applicable here.
#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.
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.
[Amy] They were in one model. During the WG discussion, comment was raised that the interface protection function could be generic and be used by other technologies in future, so we split the models.
I’m open to discuss about this.
[janl] Ok, I didn't quite see how the grouping would be used anywhere else, but if it indeed is usable elsewhere, having it in a separate module isn't a bad idea.
[Jonas] I’m in favor of reverting back to one single module gain, since it will simplify the model as you mention above. The argument for breaking it out into a separate module is that it can be used elsewhere. I’m doubtful if it
ever will be, but here I think we need input from the rest of the community (at least from the CCAMP WG).
#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?
[Amy] status should be config false.
[Jonas] Correct. Our mistake.
#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.
[Amy] add output to describe the action result (success, fail, inprogress). But prefer to use one action.
Change the name to external-command.
[janl] Hmm, I don't quite understand your preference for a single action. I see how this choice closes doors, but no real benefit. This is no big deal, though, what you have works. Just feels less evolvable for no reason.
[Jonas] I’m not quite sure I’ve understood what you propose. Do you mean that we should replace the single action statement and the list of identifiers with several action statements:
action manual-switch-working;
action manual-switch-protection;
…
action clear;
The reason for using identities was that we wanted to make it easy to extend the module with additional, potentially vendor specific, commands by augmentation.
But maybe there are other ways to handle that in case of using the alternative approach you suggest.
[janl2] Yes, exactly. Separate actions like this has the advantage that they can take (different) input and output parameters, and that they can be access controlled separately using NACM. This way different operators could have/not have
access to different operations. Future standard or vendor modules could still augment whatever additional actions they like to the same location in the tree.
[Jonas2] OK I understand and I see the advantage of this approach. Amy, should we consider change to this or are there other benefits with the original approach?
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'";
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.
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.
[Amy] remove default. Add mandatory.
[Jonas] I think that the name of the attribute could be misleading. It’s not a formal id of the terminal, it is a text string that is used by an optional system feature in a far-end RLT (on the opposite end of
the microwave link) to verify that the correct two RLTs (far-end and near-end) are connected.
I don’t think we should make it mandatory. I would suggest to leave it without default and clarify the consequences in the description.
#7) Use derived-from when comparing identities (again)
leaf-list carrier-terminations {
must "/if:interfaces/if:interface[if:name = current()]"
+ "/if:type = 'mw-types:carrier-termination'" {
must "derived-from-or-self(/if:interfaces/if:interface[if:name =
+ "/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:
"Selected transmitter frequency.";
"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.";
"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
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:
"Selected transmitter frequency.";
choice freq-or-distance {
"Selected receiver frequency."
"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 {
"Computed receiver frequency."
leaf actual-duplex-distance {
"Computed distance between Tx & Rx frequencies."
Many other ways of doing this properly are also possible. Let me know if you'd like to discuss options.
[Amy] I think choice is a good way to model those leafs. I suggest to use it.
That’s the real value of YANG doctors! Thanks.
[Jonas] Thanks for your help. We have discussed this a lot without finding a good solution. I agree that we should use it.
The only question I have is the name of the attributes. Should the attribute used for configuration or the attribute showing the resulting be called rx-frequency?
config-rx-frequency & rx-frequency v.s. rx-frequency & actual-rx-frequency
[janl2] I used actual-rx-frequency because you had some other leafs called actual-...
Yet another possibility could be to model it like this (pseudo YANG):
leaf dist { config false; }
leaf rx { config false; }
I.e. three leaves with the same names, just different paths (since the container names differ) and different configness. I think my initial proposal is a little cleaner, tho, which is why I went with that one earlier. Or you could move
the actual-... leaves into the choice, so that only the one that's meaningful exists.
[Jonas2] I think that your first proposal has a benefit of always providing access to the actual configuration through the same leafs (and path) (tx-frequency, actual-rx-frequency, actual-duplex-distance, independent
of how they have been configured.
[janl3] Yes, I agree. I find this easier to read and use.
One use-case we have discussed is that an operator uses duplex-distance for the initial configuration of the rx-frequency and then in a second step fine tuning it by setting the rx-frequency directly. Would that
work with a choice statement, i.e. that you first choose duplex-distance and then in a subsequent operation overrides that and choose rx-frequency?
[janl3] Well, yes. The operator/manager can choose whichever method he likes and switch from one to the other at any time. If a value for distance is configured, the rx-frequency is implicitly deleted, and vice versa. That's how a choice
works, something like radio buttons (but there could be several elements inside each "button").
One question there: the distance is currently unsigned, so how will the system know if the rx freq is dist kHz above or below the tx freq? Or is this obvious, the tx is always lower or something? There's no validation for any such rule
in the YANG for sure.
Maybe we should also make the choice mandatory. I think I left it optional in my suggestion, and I'm not sure that makes sense. So:
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."
}
#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'";
"The lower threshold for the input power at far-end
reference "ETSI EN 302 217-1";
leaf atpc-upper-threshold {
[Amy] can add the must statement.
#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:
"Remote Transmit Power Control (RTPC).";
reference "ETSI EN 302 217-1";
"Automatic Transmit Power Control (ATPC).";
reference "ETSI EN 302 217-1";
"A choice of Remote Transmit Power Control (RTPC)
or Automatic Transmit Power Control (ATPC).";
leaf maximum-nominal-power {
"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'";
"The lower threshold for the input power at far-end
reference "ETSI EN 302 217-1";
leaf atpc-upper-threshold {
when "../power-mode = 'atpc'";
"The upper threshold for the input power at far-end
reference "ETSI EN 302 217-1";
"Remote Transmit Power Control (RTPC).";
reference "ETSI EN 302 217-1";
leaf maximum-nominal-power {
"Selected output power.";
reference "ETSI EN 302 217-1";
"Automatic Transmit Power Control (ATPC).";
reference "ETSI EN 302 217-1";
leaf maximum-nominal-power {
"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 {
"The lower threshold for the input power at far-end
reference "ETSI EN 302 217-1";
leaf atpc-upper-threshold {
"The upper threshold for the input power at far-end
reference "ETSI EN 302 217-1";
"A choice of Remote Transmit Power Control (RTPC)
or Automatic Transmit Power Control (ATPC).";
[Amy] Choice is a better way. Is it possible to further refine your proposal?
Since maximum-nominal-power will be used by both RTPC and ATPC, how about to move it out of the choice, then use maximum-nominal-power in the choice? Like this:
leaf maximum-nominal-power {
"Selected maximum output power. Minimum output
power is the same as the system
capability, available-min-output-power.";
reference "ETSI EN 302 217-1";
"Remote Transmit Power Control (RTPC).";
reference "ETSI EN 302 217-1";
use maximum-nominal-power;
"Automatic Transmit Power Control (ATPC).";
reference "ETSI EN 302 217-1";
use maximum-nominal-power;
leaf atpc-lower-threshold {
"The lower threshold for the input power at far-end
reference "ETSI EN 302 217-1";
leaf atpc-upper-threshold {
"The upper threshold for the input power at far-end
reference "ETSI EN 302 217-1";
"A choice of Remote Transmit Power Control (RTPC)
or Automatic Transmit Power Control (ATPC).";
[janl] Yes, this is certainly very reasonable. I placed the maximum-nominal-power inside the choice because the description string was giving the leaf different interpretations in each case. In that case I felt it better to have separate
leaves with clear descriptions. If you feel the leaf can be shared between the options with a clear definition of its meaning, this is reasonable.
[Jonas] One purpose of the mode attribute was to show if the carrier-termination was configured for RTPC or ATPC, but I guess the same can be achieved by looking for which container, rtpc or atpc, has been instantiated.
[janl2] Yes, that's the idea.
I suggest that we go for Jan’s proposal since the attribute has different meaning in the two cases. One purpose of the
#11) Choice more convenient (again)
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 {
[Amy] should be 1e-9. Need to confirm with the co-authors.
[Jonas] Yes, we mean 10^-9, 10^-8, …, 10^-1.
[janl2] A perpahs simpler way to model this would be to only keep the part that contains information, i.e. as
leaf ber-alarm-threshold {
description "Specification of at which BER as a power of 10 an alarm should be raised. 10^-6 is the default.";
reference "ETSI EN 302 217-1";
#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.
[Amy] Will update the must statement.
[Jonas] If I remember correctly, I think it was done with the ambition that it should be made generic so that other interface technologies could use the same protection module. In case, we revert back to one
single module I suggest that we also simply the structure as Jan suggests.
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
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
"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.
[Amy] will add additional example.
[Jonas] Good suggestion. Amy, let’s what configuration to show.
|
