Shepherd review of draft-ietf-teas-yang-te-38 #306
Description
idnits shows two issues:
- Your examples are based on IPv4 addresses. Is there a good reason to
not include an example using IPv6 addresses?
- check what other documents do (e.g., provide examples for both IPv4 and IPv6 addresses)
- Your section 2.1 contains the usual BCP 14 boilerplate. But you don't
use the language, so you don't need the section or the two referenced
RFCs.
- remove it
You need to jiggle the XML so that the Appendixes do not show as numbered
sections, but as lettered Appendixes.You do this by moving the
from the into the Hopefully, the references will automatically resolve.
Bracketed plural(s)
The use of the bracketed plural is unnecessary and sometimes makes reading
harder. E.g.,
The elements of the generic TE YANG data model, including TE Tunnels,
LSPs, and interfaces have leaf(s) that identify the technology layer
where they reside.
In this case, the singular would be "...have a leaf that identifies..."Fortunately, in English, the plural includes the singular. So it is fine to write...
The elements of the generic TE YANG data model, including TE Tunnels,
LSPs, and interfaces have leafs that identify the technology layer
where they reside.If you are really worried, you should write...
The elements of the generic TE YANG data model, including TE Tunnels,
LSPs, and interfaces have one of more leafs that identify the
technology layer where they reside.I suggest a search for "(s)" and replace with clearer text.
- Fix editorial
There are some abbreviations that you need to expand on first use. I
found:APS
LoP
MS
SF
WTR
-
@italobusi Provide expansion for LoP abbreviation
-
Expand abbreviations on first use:
| APS | Automatic Protection Switching |
| LoP | Lockout for Protection |
| MS | Manual Switch |
| SF | Signal Fail |
| WTR | Wait-to-Restore |
You need to sort out how you are referring to the two YANG modules.
You have:
'ietf-te.yang'
'ietf-te'
'ietf-te-device.yang'
'ietf-te-device'
- Remove the .yang from the text
Document name. Add a comma as
... Label Switched Paths, ...
- Fix editorial
Abstract
I think it would be helpful if the Abstract noted that there are two YANG
modules in this document.
- Reply: The last sentence of the first paragraph says that the model is divided into two YANG modules
The signaling protocol used to instantiate TE LSPs
are outside the scope of this document and expected to be covered by
augmentations defined in other document(s).
s/protocol/protocols/
- Fix
Note that you sometimes confuse "model" with "module". For example, in
Section 3 you have...
- The generic TE YANG data model 'ietf-te' contains device
independent data and can be used to model data off a device (e.g.
on a TE controller). When the model is used to manage a specific
device, the model contains the TE Tunnels originating from the
specific device. When the model is used to manage a TE
controller, the 'tunnel' list contains all TE Tunnels and TE
tunnel segments originating from device(s) that the TE controller
manages.But 'ietf-te' is a module not a model.
Similarly, in Section 4 there is...
The data models defined in this document cover the core TE features
But there is only one data model in this document.
I suggest searching the whole document for "model" and checking each case.
- Check and fix
- In general, minimal elements in the model are designated as
"mandatory" to allow freedom to vendors to adapt the data model to
their specific product implementation."In general" is a red flag. It suggests that there are exceptions to this, but no
explanation is offered.
- c/In general, minimal/Minimal/
You list as one of the "other design considerations"...
- The model declares a number of TE functions as features that can
be optionally supported.This is certainly true as a feature, but not as a design consideration.
Such a consideration might be...
- Where TE functions or features might be optional within the
deployed TE network, the model declares them as optional.
- Update text as proposed
4.1
The TE data models for specific instances of data plane technology
exist in separate YANG modules that augment the generic TE YANG data
model. The TE data models for specific instances of signaling
protocols are outside the scope of this document and are defined in
other documents. For example, the RSVP-TE YANG model augmentation of
the TE model is covered in a separate document.This text certainly implies that the other/separate documents already exist. If
so, then some references would be nice.
- Add reference and rephrase:
The TE data models for specific instances of data plane technology and signaling protocols are outside the scope of this document.
They could be defined in separate YANG modules that augment the generic TE YANG data model.
For example, the RSVP-TE YANG model augmentation of the TE model is covered in [<>].
Figure 1
I'm not clear why this figure is placed where it is and not in Section 3 where it
is referenced.
- Move it
Figure 1
I think this could be cleaned up a little. (View it in a non-proportional
font.)TE generic +---------+ module | ietf-te |o-------------+ +---------+ \ o o \ | \ \ | \ TE device module \ | +----------------+ \ | | ietf-te-device | \ | +----------------+ \ | o \ | | \ | | \ +---------------+ +---------------+ RSVP-TE module | ietf-rsvp-te^ |o | ietf-te-mpls^ | +---------------+ \ +---------------+ | \ | \ | \ | \ | \ o +-------------------+ +------------+ | ietf-rsvp-otn-te^ | RSVP module | ietf-rsvp^ | +-------------------+ +------------+ RSVP-TE with OTN extensions X---oY indicates that module X augments module Y ^ indicates a module defined in other documents
- Update as proposed
OLD
A full tree diagram of the TE model is shown in the Appendix in
Figure 13.
NEW
A full tree diagram of the TE model is shown in Figure 13 in
Appendix B.
END
- Update as proposed
5.1
OLD
Below provides further descriptions of containers that
exist under the 'te' top level container.
NEW
Further descriptions of containers that exist under the 'te' top
level container is provided below.
END
- Update as proposed
5.1
globals:
The 'globals' container maintains the set of global TE attributes that can be applicable to TE Tunnels and interfaces.Include a forward pointer to 5.1.1?
- Add the forward pointer
5.1
tunnels-path-compute:
A RPC to request path computation for a specific TE Tunnel. Thes/A RPC/An RPC/
- Fix
5.3 Figure 7
I think that you do not need a figure number or caption for the YANG model.
You just leave out the anchor and the
Ditto Figure 10 in section 6.3
- Remove figure number and caption
Page 11 bullet list is indented differently from the bullet on page 10 Page 12
indentation is broken.
- Check and fix indentations
5.1.1
It seems to me that the description of the route-object-include-exclude list is
complex. Are you sure you are not overloading the meaning? That is, the
interpretation of an empty list might be less convoluted if it was conveyed by
distinct objects.There is also some confusion in my mind about the details.
A. The processing of the list is not clear to me. It doesn't seem to
model the objects in PCEP (Include Route in 5440, Exclude Route in
5521) nor the objects in RSVP-TE (Explicit Route in 3209, Exclude
Route in 4874).I considered a simply topology...
A--B
\ |
|
C--DSuppose the LSP is to be from A to D.
Suppose that the include-exclude list is {exclude C}
This, it would, seem would allow the path ABCD.
But if the include-exclude list was {include B, exclude C} then the
path must be ABD.This, I think, means that the list is intended to describe the
formation of the path, not necessarily the "path computation" which
may use any algorithm.It may be that a little more description is needed to clarify things.
See #307
B. I don't understand the following...
2. An empty 'route-object-include-exclude' list for the primary path of a TE Tunnel Segment indicates that no primary LSP is required for that TE Tunnel.What does it mean to have no primary path? Also, should an additional
"Segment" be added to the end of the paragraph?Further, the previous point says:
1. An empty 'route-object-include-exclude' list for the primary path of an end-to-end TE Tunnel indicates that there are no route objects to be included or excluded in the path computation....and it seems to me that a segment could be the entire e2e tunnel.
See #308
C. I can see why it is useful to indicate that a secondary path has the
same end points as the primary path...4. An empty 'route-object-include-exclude' list for the secondary (forward) path indicates that the secondary path has the same endpoints as the primary path.... but don't you need the ability to specify some inclusion and
exclusion along the secondary path?
See #309
REVIEWED UP TO THIS POINT (2025-06-20)
5.1.1
You have:
o path-in-segment: A YANG container that contains a list of label restrictions that have to be taken into considerations when crossing domains. This TE tunnel segment in this case is being stitched to the upstream TE tunnel segment. o path-out-segment: A YANG container that contains a list of label restrictions that have to be taken into considerations when crossing domains. The TE tunnel segment in this case is being stitched to the downstream TE tunnel segment.When you say "crossing domains," do you mean "crossing domain
boundaries"? However, consider that stitching can also happen within a
domain. Perhaps you need "when stitching to another tunnel segment such
as at a domain boundary."
5.1.2
Please expand LER on first use.
Are all tunnels used from MPLS? If not then the mention of LER is probably
wrong. If so, then I wonder whether the file name, Abstract, and Introduction
should mention MPLS.
5.1.2
Why is a TE Tunnel Segment necessarily part of a multi-domain TE Tunnel?
I suppose it may depend on what is meant by "domain". But there is no
reason why an end-to-end tunnel cannot be constructed from many segments
stitched together.
5.1.2 operational-state:
Why is the text about operational-state presented twice?
Why is there a clear explanation of admin-state, but no explanation of
operational-state?
5.1.2 name:
Something wrong with "For initiated TE Tunnels from the controller..."
Maybe, "For TE Tunnels initiated by the controller..."
5.1.2 name: alias: identifier:
The text explains that the identifier is unique within the context of
the tunnel source. But is there any uniqueness for the name or alias?
5.1.2 admin-state:
A YANG leaf that holds the tunnel administrative state. The administrative status in state datastore transitions to 'tunnel- admin-up' when the tunnel used by the client layer, and to 'tunnel-admin-down' when it is not used by the client layer.Some things broken. Not sure what you meant, but possibly...
A YANG leaf that holds the tunnel administrative state. The administrative status transitions to 'tunnel-admin-up' when the tunnel is in use by the client layer, and to 'tunnel-admin-down' when it is not used by the client layer.
5.1.2 source/destination:
s/endpoints identities/endpoint identities/
5.1.2 source/destination:
Something wrong with both of these? Identifier of an identifier?
- te-node-id: A YANG leaf that holds the identifier of the source or destination of the TE Tunnel TE node identifiers as defined in [I-D.draft-ietf-teas-rfc8776-update]. - node-id: A YANG leaf that holds the identifier of the source or destination of the TE Tunnel node identifiers as defined in [RFC8345].
5.1.2
Expand PCEP on first use.
5.1.2 association-objects:
I'd like some more clarification of how the tunnel-level associations are
overridden by the path-level associations. It seems like it might be
complicated.
5.1.2 hierarchy:
The indentation of the two bullets is different. Is this intentional?
5.1.2 dependency-tunnels:
I'm struggling with "dependency tunnel" versus "dependent tunnel".
Initially, you say that the dependency tunnel is in the lower layer, but then
you say that dependant tunnel is created only when the dependency tunnel is
provisioned.I also think that the paragraph confuses tunnel and link.
Compare the terms client tunnel, client link, and hierarchical-link.
You are limiting dependency tunnels to being either:
- set up on demand if it is "used" by a client tunnel
- or (presumably) available for use when a further client tunnel when
already set up for use by an initial client tunnel I wonder why you don't
support a dependency tunnel being set up in advance. This approach allows:- more rapid establishment of the client tunnel
- more certain establishment of the client tunnel (which can be routed
alternately if the dependency tunnel fails) I think not including this function
is an option (it's up to the WG), but I wanted to check that this was a
conscious decision.
A lot of containers have leafs called 'name'.
That's not against the rules, but it does make the text confusing from time to
time. Any reason not to give them more specific names in each case?For example...
primary-paths:
A YANG container that holds the list of primary paths. A primary
path is identified by 'name'....which is true, but confusing because this is in the context of the tunnel
container which has its own 'name'.
5.1.2 secondary-reverse-paths:
A secondary reverse path contains attributes similar to a primary path.
What does "similar" mean?
5.1.2 compute-only:
A path of a TE Tunnel is, by default, provisioned so that it can be instantiated in the forwarding plane so that it can carry traffic as soon as a valid path is computed.This seems a bit confused, to me. Firstly, each tunnel has several lists
of paths, but not all paths are provisioned. And, we've already had
dependency tunnels that are, themselves, not always provisioned.Then, secondly, I don't think you can carry traffic as soon as the path is
computed - you need it to be provisioned first.A 'compute-only' path is configured as a usual with the associated per path constraint(s) and properties on a device or TE controller.s/as a usual/as usual/
5.1.2 lockdown:
A YANG leaf that when set indicates the existing path should not be reoptimized after a failure on any of its traversed links.I think that "reoptimized" is the wrong word for handling a link failure. Maybe
"recomputed and repaired".Same comment for the description clause of 'lockdown' in the YANG
5.1.2 path-scope:
A YANG leaf that specifies the path scope if segment or an end-to- end path.NEW
A YANG leaf that specifies whether the path is a segment or an
end-to-end path.
END
5.1.2 preference:
The description here is not enough. Yes, you tell us the ordering of the
preference (lower number is higher preference). But how is the preference
used?Under primary-paths you have, "The list of primary paths is visited by order of
preference." And, by context, we may assume this means that this applies to
the selection of a primary path to instantiate, choosing from an unordered list.
There is similar text and interpretation for candidate-secondary-paths.Perhaps...
OLD
A YANG leaf that specifies the preference for the path. The lower
the number higher the preference.
NEW
A YANG leaf that specifies the preference for the path, used to
choose between paths in a list. The lower the number higher the
preference. Paths with the same preference are treated as equal
and other methods are used to choose between them.
END
5.1.2 link-protection:
s/included the/included in the/
5.1.2 lsp-provisioning-error-infos:
Everything so far has been about paths. Then suddenly this leaf is about LSPs.
Is that right? Maybe it needs to be moved to below lsps: in the container.
Maybe the errors need to be indicated to the specific LSPs by placing them in
the lsps container.
5.3
Good that you have text for the references:
This module references the following documents: [RFC4206], [RFC4427],
[RFC4872], [RFC3209], [RFC6780], [RFC7471], [RFC9012], [RFC8570],
[RFC8232], [RFC7271], [RFC8234], [RFC7308], and [ITU_G.808.1].But you have draft-ietf-teas-rfc8776-update in a Reference clause in the YANG.
Shouldn't you include it in the list? (You do in 6.3)Additionally, since the Reference clause doesn't use the RFC Editor
might not spot them, so I suggest including a note for the RFC Editor here and
in 6.3 to make sure they are updated to the RFC.
5.3
namespace "urn:ietf:params:xml:ns:yang:ietf-te"; /* Replace with IANA when assigned */I assume the comment applies to the namespace line. I think you want the
RFC Editor to take care of this, so I suggest something more detailed such as:namespace "urn:ietf:params:xml:ns:yang:ietf-te"; /* RFC Editor Note: Please replace the above line with the URN assigned by IANA and remove this note. */Ditto section 6.2.
5.3
The contacts clause lists 7 names, but there are only 5 on the front page.
Ditto section 6.2.
5.3 and 6.2
The copyright date in the main description clause is wrong.
5.3
typedef tunnel-ref { description "This type is used by data models that need to reference configured TE tunnel.";s/configured/a configured/
5.3
container path-computation-server { uses te-types:te-generic-node-id; description "Address of the external path computation server.";You say "address" but you use te-generic-node-id. Are you requiring that the
'type' leaf is set to 'ip'? If so, I think you have to say so. If not, perhaps don't
say "address".
5.3
leaf compute-only { type empty; description "When present, the path is computed and updated whenever the topology is updated. No resources are committed or reserved in the network."; }Are you sure this is "whenever"? That implies a lot of computational churn.
Sure, an implementation may choose to do this, but is it a requirement? A
more moderate approach, for example, would be to do the computation only
when links/nodes used by the path are updated in the topology, or
periodically for reoptimisation.How to handle this? Probably s/is/may be/
5.3
What would it mean for compute-only to be present, but use-path-
computation is set to 'false'?
5.3
leaf path-scope { description "Indicates whether the path is a segment or portion of of the full path., or is the an end-to-end path for the TE Tunnel.";s/of of/of/
s/.,/,/
s/the an/an/
5.3 has multiple instance of
/* This grouping is re-used in path-computation rpc */ But the rpc is called
tunnels-path-compute
5.3
grouping path-forward-properties { leaf preference { description "Specifies a preference for this path. The lower the number higher the preference.";s/number higher/number, the higher/
5.3
grouping k-requested-paths { leaf k-requested-paths { description "The number of k-shortest-paths requested from the path computation server and returned sorted by its optimization objective.";Unclear what "sorted" means. Possibly "assigned 'preference' values to sort
the returned paths according to how well they meet the optimization
objectives."
5.3
grouping path-state { container lsp-provisioning-error-infos { list lsp-provisioning-error-info { leaf error-description { description "The textual representation of the error occurred during path computation.";I think this is a block copy from the path computation error container.
Here you should be talking about errors that occur during LSP provisioning.Similarly, in the same grouping...
leaf lsp-id {
description
"The LSP-ID for which path computation was performed."; ...
presumably this should be LSP-ID of the LSP for which the provisioning error
was returned.
5.3
I'd like some clarity about LSP errors that clear. are they removed from
container, or do they persist but we know they are legacy because the LSP is
now active? Or what?
5.3
container lsps { list lsp { leaf node { description "The node where the LSP state resides on.";s/ on././
But, I don't understand this! What do you mean by saying LSP state resides on
a node? LSP state is definitely a distributed thing. Possibly you mean the node
from which the state was retrieved to be present in this module, but even
then it is making great assumptions about how the LPS is provisioned and
monitored.
5.3 computed-path-error-infos
I can't see any information about how this list is refreshed. Does it contain all
errors for all time? Does it only carry the most recent error for any one LSP-ID?
Does the error get flushed when there has been a subsequent successful
computation for a given LSP.In the context of patch computation, what is meant by error-node-id and
error-link-id. It seems to me that when a computation fails, it just
fails: there is no specific point in the topology where the computation fails.
5.3
container globals { container named-admin-groups { list named-admin-group { leaf bit-position { type uint32; description "Bit position representing the administrative group.";After a little panic and reference to 7308, I finally realised what you we're
doing here! Not a 32 bit mask, but an index. I know that is exactly what you
say, but it might be good to add a few words as the cited references are bit
masks, not indices.
5.3
container named-srlgs { list named-srlg { leaf cost { type uint32; description "SRLG associated cost. Used during path to append the path cost when traversing a link with this SRLG.";Some word missing? s/path/path computation/ ?
But "append the path cost" means what? Add?
Is there a reference for this processing?
5.3
container named-path-constraints { list named-path-constraint { leaf name { type string; description "A string name that uniquely identifies a path constraint set.";s/string name/name/
5.3
container tunnels { list tunnel { container controller { leaf controller-entity-id { type string; description "An identifier unique within the scope of visibility that associated with the entity that controls the tunnel.";I think s/that/that is/
But this is ambiguous. It the identifier associated with the entity, or is the
scope of visibility associated with the entity?
Anyway, what is a scope of visibility?
5.3
leaf reoptimize-timer { type uint16; units "seconds"; description "Frequency of reoptimization of a traffic engineered LSP."; }Three questions:
- How do I set "never"? Perhaps zero?
- Do you think that allowing a 1 second reoptimization is reasonable? I
know it isn't our job to stop people doing stupid things, but perhaps
there should be a minimum?- Shouldn't there be a default?
5.3
action protection-external-commands { leaf protection-group-ingress-node { type boolean; default "true"; description "When 'true', indicates that the action is applied on ingress node. By default, the action applies to the ingress node only."; } leaf protection-group-egress-node { type boolean; default "false"; description "When set to 'true', indicates that the action is applied on egress node. By default, the action applies to the ingress node only."; }
- s/applied on/applied on the/ (twice)
- I see the default settings, but the text is a little off. Perhaps
no text is needed. Or...
By default, the action applies to the ingress node.";
...and...
By default, the action does not apply to the egress node.";
5.3
action protection-external-commands { input { leaf traffic-type { type enumeration { enum normal-traffic { description "The manual-switch or forced-switch command applies to the normal traffic (this Tunnel)."; } enum null-traffic { description "The manual-switch or forced-switch command applies to the null traffic."; } enum extra-traffic { description "The manual-switch or forced-switch command applies to the extra traffic (the extra-traffic Tunnel sharing protection bandwidth with this Tunnel)."; } } description "Indicates whether the manual-switch or forced-switch commands applies to the normal traffic, the null traffic or the extra-traffic.";This is an enumeration so I cannot apply the switch commands to more than
one traffic type at once. Right? Why?
5.3 container lsps
Many of these leafs (as hinted by the Reference clauses) are specific to MPLS
(see previous comment about the document making that clear), but are also
specific to RSVP-TE. How are these leafs used when the LSP is established in a
different way?
5.3
container lsps { list lsp { leaf origin-type { type enumeration {As an enumeration, this prevents a two-link-single-node LSP. Don't you want
to allow that?
5.3
rpc tunnels-actions { input { container action-info { leaf disruptive { description "When present, specifies whether or not the reoptimization action is allowed to be disruptive.";Some explanation of "disruptive" is needed, or a reference.
5.3 Figure 7
I think that you do not need a figure number or caption for the YANG model.
You just leave out the anchor and the
Ditto Figure 10 in section 6.3
6.1.1
You seem to be missing the equivalent of 5.1.2 carrying a description of the
main containers and leafs in the module. Would this be useful?Yes, you do have "some examples".
6.3
Especially in the absence of the text descriptions in the previous point, many
of the Description clauses are insufficient. References are also absent.For example,
leaf lsp-install-interval {
type uint32;
units "seconds";
description
"TE LSP installation delay time."; Here, it is clear that the leaf is used to
carry the "installation time delay" but not what it actually is or how it is used.I think you need to go through all of this module and check each description
and look to add more references. I would say that some constructs have really
good descriptions, so it is not a universal problem.
6.3
leaf time-to-destroy { type uint32; units "seconds"; description "The time remaining for a existing LSP to be deleted from forwarding."; }I think the inverse of "instantiated" is usually "torn down" rather than
"deleted from forwarding."
s/a existing/an existing/
6.3
container upstream-info { when "../te:origin-type != 'ingress'" { description "Upstream information of the LSP."; }s/Upstream/upstream/ to be compatible with all the other descriptions.