Thanks for your detailed review. We will address your comments before posting next rev.
Rgds.
On 2017-09-27, 9:45 AM, "Jan Lindblad" <janl@xxxxxxxxxx> wrote:
Reviewer: Jan Lindblad
Review result: Almost Ready
I had a read through of the draft RFC and looked at the YANG model in
particular. Generally speaking, I think the YANG looks good. I don't know much
about MPLS, however, so I can't judge the usefulness of the model. I'm bringing
up a few points for discussion below.
1) IETF, OpenConfig and NMDA
Early on in the draft, there is this section:
For the configuration and state data, this model follows the similar
approach described in [I-D.openconfig-netmod-opstate] to represent
the configuration (intended state) and operational (applied and
derived) state. This means that for every configuration (rw) item,
there is an associated (ro) item under "state" container to represent
the applied state. Furthermore, protocol derived state is also kept
under "state" tree corresponding to the protocol area (discovery,
peer etc.). [Ed note: This document will be (re-)aligned with
[I-D.openconfig-netmod-opstate] once that specification is adopted as
a WG document].
This alignment is sorely needed. I don't want to open up a new chapter in the
IETF vs. OpenConfig style modeling debate, but I will simply note that the
current model does not fit nicely into the surrounding MPLS, routing and
interface models. Since consistency is a primary factor for ease of use, the
model isn't very easy to use in its current form.
2) Lack of default/mandatory/description
As has become a standing item in my reviews, there are a number of leafs that
have no default, or mandatory statement, and no leaf name or description making
it obvious to the user what would happen if this leaf is not set. This is
particularly problematic with the "leaf enable {type boolean;} leafs. I get
"enabled false" and "enabled true" even without any description. But what if
enabled isn't specified? At the minimum level, the description should describe
this case. Better if there was a default making this clear. Or if a default
really isn't appropriate, so make it mandatory.
Here are a list of leafs/line numbers where I think this is a problem. There
are many others that have no default/mandatory/description, but where I suspect
people who understand LDP might have an opinion about reasonable behavior if
not set.
ietf-mpls-ldp:
251 leaf hello-holdtime {
261 leaf hello-interval {
335 leaf enable {
346 leaf hello-holdtime {
357 leaf hello-interval {
380 leaf enable {
385 leaf reconnect-time {
395 leaf recovery-time {
406 leaf forwarding-holdtime {
424 leaf enable {
429 leaf reconnect-time {
439 leaf recovery-time {
462 leaf lsr-id {
533 leaf session-ka-holdtime {
544 leaf session-ka-interval {
777 leaf enable {
892 leaf enable {
1011 leaf enable {
1131 leaf enable {
1261 leaf lsr-id {
1331 leaf lsr-id {
ietf-mpls-ldp-extended:
183 leaf transport-address {
193 leaf transport-address {
204 leaf key-chain {
218 leaf enable {
228 leaf enable {
238 leaf enable {
249 leaf igp-synchronization-delay {
300 leaf ldp-disable {
325 leaf helper-enable {
336 leaf transport-address {
354 leaf transport-address {
375 leaf igp-synchronization-delay {
473 leaf admin-down {
500 leaf enable {
505 leaf peer-list {
537 leaf enable {
620 leaf enable {
723 leaf enable {
728 leaf local-address {
781 leaf interface {
3) Odd naming convention for keys
Many list keys are named by the same (or similar) name as the list. Repetitions
of the same symbol makes it harder for users/programmers to get their code
right, and even discuss the code. A common practice when there is no obvious
identifier to use with the key is to use "name" or "id" for the key.
list peer {
key "peer advertisement-type";
The peer is keyed by "peer"? and advertisement-type. Looking at the "peer",
it's a leafref to an lsr-id. So maybe that's a good name?
list peer {
key "lsr-id advertisement-type";
ietf-mpls-ldp:
296 list peer { key "peer advertisement-type"; --> "lsr-id
advertisement-type"? 932 list address { key "address"; --> ipv4? 944 list
fec-label { key "fec"; --> prefix? 979 list interface { key "interface"; -->
name?
ietf-mpls-ldp-extended:
279 list interface { key "interface"; --> name?
288 list address-family { key "afi"; --> name?
577 list address { key "address"; --> ipv6?
589 list fec-label { key "fec"; --> prefix?
4) Password handling
The session auth password is commented out in the current model for some
reason. Password handling is somewhat of a wart in an inconvenient place in
YANG today, causing a whole array of interop issues of varying severity. One of
the solutions working best with the current YANG spec (could perhaps be fixed
in future YANG versions) is to not include passwords in the configuration, but
only have an operational element with the encrypted value or last changed
timestamp and and rpc/action to set the password.
/*
leaf session-authentication-md5-password {
type string {
length "1..80";
}
description
"Assigns an encrypted MD5 password to an LDP
peer";
} // md5-password
*/
There may be other possibilities as well at modeling this in a friendlier way.
5) Neighbor/prefix/peer list references
The ietf-mpls-ldp-extended module defined three reference types as strings. Are
these free form strings, or is there any guidance that can be provided to
users? Where would I go to find out what values are possible? I don't suppose
any of these could be a leafref.
typedef neighbor-list-ref {
type string;
description
"A type for a reference to a neighbor list.";
}
typedef prefix-list-ref {
type string;
description
"A type for a reference to a prefix list.";
}
typedef peer-list-ref {
type string;
description
"A type for a reference to a peer list.";
6) Some short/pointless descriptions
leaf prefix {
type inet:ip-prefix;
description
"FEC.";
}
leaf lsr-id {
type yang:dotted-quad;
description "Router ID.";
}
Descriptions like these aren't helpful. Explain how to arrive at a good value,
and what it means if the value is absent.
I stumbled over a copy-paste description at line 235. Presumably hello-dropped
should have a different description.
leaf hello-received {
type yang:counter64;
description
"The number of hello messages received.";
}
leaf hello-dropped {
type yang:counter64;
description
"The number of hello messages received.";
}
7) Captialized enum
The YANG convention is that all enums, all symbols actually, are in all lower
case.
ietf-mpls-ldp:
915 enum Ordered {
ietf-mpls-ldp-extended:
560 enum Ordered {
That's it, thank you.
/jan