Hi Jan,
Thanks for your review. Snipping to...
On Sun, Nov 24, 2024 at 8:09 PM Jan Lindblad via Datatracker <
nore...@ietf.org> 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";
}
> === 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
Thanks!
Dhruv
> Best Regards,
> /jan
>
>
>
>
_______________________________________________
Pce mailing list -- pce@ietf.org
To unsubscribe send an email to pce-le...@ietf.org
