Hi Jan,
Thanks for your review. Snipping to...
On Sun, Nov 24, 2024 at 8:09 PM Jan Lindblad via Datatracker <noreply@xxxxxxxx> wrote:
Reviewer: Jan Lindblad
Review result: Ready with Issues
Dear WG,
This is my YANG Doctor Last Call review of draft-ietf-pce-pcep-yang-26. My
conclusion is that the two YANG modules contained in this draft are ready with
issues. I believe all issues are relatively straightforward to fix.
I did not find any fundamental questions to discuss around these modules, so I
will simply list what I found in order of appearance, from top to bottom. The
numbers after the === are line numbers in each module.
ietf-pcep.yang:
<snip>
=== 608: Unclear use of additional association-type identities
The ietf-pcep module defines four additional association-types
(path-protection, disjoint, policy, vn) besides the ones already defined in
ietf-te-types (association-type-recovery, association-type-resource-sharing,
association-type-double-sided-bidir, association-type-single-sided-bidir).
The base identity te-types:association-type is used in two leafs in the module:
- /pcep/entity/lsp-db/association-list/type (line 1754)
- /pcep/entity/peers/peer/sessions/session/assoc-type-list/assoc-type/at
(line 2624)
Do all 8 association types make sense for those leaves? As currently modeled,
they are all allowed values. Do all 8 association types make sense for all
other modules that use the ietf-te-types association-type values? As currently
modeled, those other modules will also have the new values defined in ietf-pcep
as allowed values.
Dhruv: Yes, all the association types and any future association type are all allowed.
=== 637: “vn” is not a great name for an identity
Few people will know this abbreviation, and the other identities in this family
are not abbreviated. The other related identities are path-protection,
disjoint, policy. Suggest spelling out this acronym.
Dhruv: changed to virtual-network
=== 662: Domain “type” and “info” not necessarily compatible
Domains are modeled as having a “type” (ospf-area, isis-area, as-number) and
“info”, which is a union of the address formats for each type. As currently
modeled, it would be entirely possible to configure the “type” as an ospf-area,
but specifying an “info” for an isis-address or an as-number. This is not
ideal, and a different modelling could resolve this. Happy to discuss modelling
options, if desired.
Dhruv: I see this union approach in yang-types (ex typedef host). I was inspired by that.
=== 662: “info” is not a great name for a leaf
The name is too generic for people to understand what it refers to.
Dhruv: changed to domain
=== 669: “info” is not a great name for a grouping
The name is too generic for people to understand what it refers to. Especially
with a leaf by the same name close by.
Dhruv: changed to domain-info
=== 680: List domain key simplification possible
The domains are keyed by “type” and “info”. Since “info” in itself is a unique
identifier (I believe the address formats are disjunct), it would be possible
to remove “type” from the key statement, if desired.
Dhruv: No, I want to differentiate between ospf area 100 and AS 100 as both can co-exist and mean two different things.
=== 705: Unclear capabilities configuration
It’s not very clear from the YANG module how the configured capabilities will
be used. Is this to enable features on the local system, or to declare features
when communicating with peers? There is no default, which I would interpret as
all optional capabilities are off unless configured. Is that what is intended?
Dhruv: This container is used mainly to advertise the capability when communicating with peers. And your interpretation is correct.
=== 861, etc: No default, no mandatory, no description
The Boolean leafs on lines 861-925, 953-992 do not have any default, mandatory
statement or description of what it means if they are left unconfigured. Please
add a default, a mandatory statement or add text to the description explaining
what the system will do if the leaf is left with no value.
Dhruv: I added the default.
=== 993: No default, no mandatory, no description
The leaf role has no default, is not mandatory and has no text in the
description describing the system behavior if not set.
Dhruv: I added the default.
=== 1135: Disconnected leafrefs in notification-instance-hdr and
notification-session-hdr A number of notification are using the groupings
notification-instance-hdr to reference the specific peer that the notification
pertains to. Some of those notifications also use the notification-session-hdr
to distinguish which of the (two possible) sessions to that peer the
notification pertains to.
Since this the latter grouping is only ever used in notifications, this is
maybe more of a nit, but technically the leafref needs to dereference the path
provided in the notification-instance-hdr in order to be meaningful.
Instead of having two seemingly unrelated groupings, it would be better to
model this as one grouping for the case where only the peer is needed, and
another for the case where both peer and initiator is needed, and make sure
that latter grouping is using the first grouping and dereferencing the path.
Since this sounds more complicated than it is, let me show what I suggest here
(removed some descriptions to enhance readability):
grouping notification-instance-hdr {
leaf peer-addr {
type leafref {
path "/pcep/entity/peers/peer/addr";
}
}
}
grouping notification-session-hdr {
uses notification-instance-hdr;
leaf session-initiator {
type leafref {
path
"/pcep/entity/peers/peer[addr=current()/../peer-addr]/sessions/session/initiator";
}
}
}
…
notification pcep-session-up {
description
"This notification is sent when the value of
'/pcep/peers/peer/sessions/session/state'
enters the 'session-up' state.";
// Needs to reference both peer and initiator, so uses
notification-session-hdr uses notification-session-hdr;
…
notification pcep-session-down {
description
"This notification is sent when the value of
'/pcep/peers/peer/sessions/session/state'
leaves the 'session-up' state.";
// Needs to reference only peer, so uses notification-instance-hdr
uses notification-instance-hdr;
…
Dhruv: Thanks for this suggestion, I have made the update.
=== 1287: Unclear behavior when configuring role unknown
A user can configure /pcep/entity/role to be “unknown”. The system behavior for
this case it not well described in the module.
Dhruv: I have added a must statement that the configured entity role must not be unknown. The unknown is used only for the role of the remote peer when it is not known.
=== 1381, etc: No default, not mandatory, no description
The leafs below have no default, no mandatory statement and have no text in the
description describing the system behavior if not set. Please add default,
mandatory or describing text to the leafs: -
/pcep/entity/pce-info/path-key/{enabled, pce-id} -
Dhruv: Added default and mandatory!
/pcep/entity/{init-back-off-timer, max-back-off-timer, max-keepalive-timer,
max-dead-timer, min-keepalive-timer, min-dead-timer, request-timer,
max-sessions} - /pcep/entity/stateful-parameter/{state-timeout,
redelegation-timeout}
Dhruv: Since there is no default defined in the RFC, I think making them mandatory is the only safe option.
=== 1787: Disconnected leafrefs
The list /pcep/entity/lsp-db/association-list/lsp is meant to reference entries
in the list /pcep/entity/lsp-db/lsp. As currently modeled, however, the
leafrefs are disconnected. This is in a config false list, which lessens the
severity of the problem, but technically the references need to be linked like
this (descriptions & line breaks removed for readability):
list association-list {
…
list lsp {
key "plsp-id pcc-id lsp-id";
leaf plsp-id {
type leafref {
path /pcep/entity/lsp-db/lsp/plsp-id;
}
}
leaf pcc-id {
type leafref {
path
"/pcep/entity/lsp-db/lsp[plsp-id=current()/../plsp-id]/pcc-id";
}
}
leaf lsp-id {
type leafref {
path "/pcep/entity/lsp-db/lsp[plsp-id=current()/../plsp-id]
[pcc-id=current()/../pcc-id]/lsp-id";
}
}
}
}
Dhruv: Updated! Thanks!
=== 1916: Non-effective must-statement on config false
If the must-constraint was found to be violated, who would you report the error
message to? It might be better to describe the must-constraint on the config
false leafs as when-statements instead, indicating that the leaf is only
relevant under certain conditions. Or as text in the description, if you prefer.
Similar constructs also appear on lines 1953, 2108, 2332, 2356, 2378, 2410,
2434, 2477, 2517.
Dhruv: Yes, this got discussed on the netmod list at https://mailarchive.ietf.org/arch/msg/netmod/axg7RoufH5ydHMySCv07EJScEf4/ but the resolution was not applied.
I will follow the recommendation and change 'must' to 'when'.
=== 1991: More disconnected leafrefs
List /pcep/entity/lsp-db/lsp/association-list has the same problem with
disconnected leafrefs as mentioned above for
/pcep/entity/lsp-db/association-list/lsp on line 1787. Let me know if you’d
like some help to connect the leafrefs.
Dhruv: Updated.
=== 2177: Sibling containers have the same description
The uses info and containe pce-info have the same description, and thereby
provide little navigational value.
uses info {
description
"PCE Peer information";
}
container pce-info {
uses pce-info {
description
"PCE Peer information";
}
=== 2189: No default, no mandatory, no description
Add a default, mandatory or text describing the system behavior when not set
for leaf delegation-pref.
Dhruv: Added mandatory.
=== 2279: Likely wrong leafref
The /pcep/entity/peers/peer/sessions/session/role leaf is modeled as a leafref
to the singleton /pcep/entity/role. This means the only valid value for this
leaf is the same value as /pcep/entity/role. I believe this is not the intent.
leaf role {
type leafref {
path "/pcep/entity/role";
}
If the intent is that the role leaf should be able to take any of the role
values, model as “type role;” instead.
Dhruv: I want this to reference the role of the peer's role only. I have modified it to -
leaf role {
type leafref{
path "../../../role";
}
type leafref{
path "../../../role";
}
=== 2465: Possible when-_expression_ simplification
A handful of when-expressions are written as “= true()”:
when '(../overloaded = true())' {
A simpler, and perhaps easier to read, way to achieve exactly the same thing is
when '../overloaded' {
Dhruv: Updated.
=== 2835: RPC input is optional
The rpc trigger-resync takes a single leaf as input, leaf pcc. It is optional.
I would think it should be mandatory. Or what happens if someone invokes this
rpc without setting this leaf?
Dhruv: Updated.
I also have a couple of comments on
Ietf-pcep-stats.yang:
=== 65: Groupings with leaking paths
For the record, it’s not recommended to have groupings with paths reaching
outside the grouping. Makes the grouping hard to reuse and understand.
Dhruv: But the relative path works because leaf role exists at both the peer and session level. I have added a text warning about using the grouping with care.
=== 65, etc: Repeating when-expressions
Several dozen leafs have a when _expression_ ensuring they only appear on pcc or
pce systems. Instead of repeating this when _expression_ leaf by leaf, it may be
better (easier to read, better structure) to create a pcc and pce container,
place one instance of the when _expression_ on the container, and then move all
the conditional leafs into the appropriate container. The module would become
more than 100 lines shorter by removing the repeition this way.
Dhruv: Updated.
=== 844: RPC with unclear choice structure
The rpc statistics-reset has a choice with two options: case peer and case all.
Case peer has one parameter pointing out which peer to reset the statistics
for. Case all does not need that, so that case is empty.
The choice is not mandatory, so it is quite possible for a client to say
nothing about which case to invoke. Does that imply a reset all? If so, I think
that should be clearly documented in the rpc description.
Since the case all branch of the choice has no content, it will not be possible
for a client to express this intent. At least one leaf type empty would have to
be added for this branch to be accessible.
case all {
leaf all {
type empty;
}
description
"This resets all the statistics collected.";
}
Dhruv: As per Mahesh comments, I have used action to reset stats at a particular container level. The rpc is used to reset all only.
URL: https://www.ietf.org/archive/id/draft-ietf-pce-pcep-yang-27.txt
Status: https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/
HTMLized: https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang
Diff: https://author-tools.ietf.org/iddiff?url2=draft-ietf-pce-pcep-yang-27
Status: https://datatracker.ietf.org/doc/draft-ietf-pce-pcep-yang/
HTMLized: https://datatracker.ietf.org/doc/html/draft-ietf-pce-pcep-yang
Diff: https://author-tools.ietf.org/iddiff?url2=draft-ietf-pce-pcep-yang-27
Thanks!
Dhruv
Best Regards,
/jan
-- last-call mailing list -- last-call@xxxxxxxx To unsubscribe send an email to last-call-leave@xxxxxxxx
