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:

=== 32: Reference to I-D, RFC 9645 now exists
    reference
      "I-D.ietf-netconf-tls-client-server: YANG Groupings for TLS
       Clients and TLS Servers";

=== 580: Spelling error on “brought”
Old:	"LSP administratively bought down. LSP Error Code value
New:	"LSP administratively brought down. LSP Error Code value
  
=== 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.
 
=== 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.

=== 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.

=== 662: “info” is not a great name for a leaf
The name is too generic for people to understand what it refers to.

=== 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.

=== 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.

=== 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?

=== 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.
  
=== 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.

=== 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;
…

=== 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.

=== 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}
-	/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}

=== 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";
              }
            }
          }
        }

=== 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.

=== 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.

=== 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.

=== 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.

=== 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' {

=== 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?


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.

=== 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.

=== 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.";
        }

Best Regards,
/jan