Re: [Teas] Benjamin Kaduk's Discuss on draft-ietf-teas-yang-te-types-10: (with DISCUSS and COMMENT)

[Benjamin Kaduk <[e-mail]>]

On Tue, Oct 01, 2019 at 05:41:37PM +0000, Tarek Saad wrote:
> Hi Benjamin,
> 
> Thank you very much for the thorough review and for the valuable comments. We have uploaded revision -11 that attempts to address those comments. Please see inline for detailed responses. Let us know if there are further outstanding comments.
> 
> On 8/20/19, 1:27 PM, "Benjamin Kaduk via Datatracker" <[e-mail]> wrote:
> 
>     Benjamin Kaduk has entered the following ballot position for
>     draft-ietf-teas-yang-te-types-10: Discuss
>     
>     When responding, please keep the subject line intact and reply to all
>     email addresses included in the To and CC lines. (Feel free to cut this
>     introductory paragraph, however.)
>     
>     
>     Please refer to https://urldefense.proofpoint.com/v2/url?u=https-3A__www.ietf.org_iesg_statement_discuss-2Dcriteria.html&d=DwICaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=Kd6NW5ctLWr7GB646PVmMByvi8wQxPILpKhHvGQeRHY&m=hnPuRIqXqStfK232Nk2hmq9WsTD2edVNgVLywB7I9S8&s=-tmE5kZMwh42TgMAaC3M4xeGTxYdYNt5fZ7gnVI59m0&e=
>  
>     for more information about IESG DISCUSS and COMMENT positions.
>     
>     
>     The document, along with other ballot positions, can be found here:
>     https://urldefense.proofpoint.com/v2/url?u=https-3A__datatracker.ietf.org_doc_draft-2Dietf-2Dteas-2Dyang-2Dte-2Dtypes_&d=DwICaQ&c=HAkYuh63rsuhr6Scbfh0UjBXeMK-ndb3voDTXcWzoCI&r=Kd6NW5ctLWr7GB646PVmMByvi8wQxPILpKhHvGQeRHY&m=hnPuRIqXqStfK232Nk2hmq9WsTD2edVNgVLywB7I9S8&s=RkaTEY5gt6bhkr_3YkKb0CkmkQyY4-GYjfuvWkkKVaQ&e= 
>     
>     
>     
>     ----------------------------------------------------------------------
>     DISCUSS:
>     ----------------------------------------------------------------------
>     
>     There's a couple points (describe further in the comment section) that
>     I'd like to get a bit more clarity on:
>     
>     (1) the semantics of admin-group hex strings of length smaller than 11
> [TS]: RFC6991 defines the hex-string as stream of octets - separated by colon. The below are acceptable representations that result in same value (bit-position 0 is set).
> 01
> 00:01
> 00:00:00:01

I'm not coming to the conclusion just based on what's written in this
document and RFC 6991.  You seem to be assuming that the hex string is
mapped to the four-octet bitmask using a little-endian representation of
the bitmask with zero padding, but I can't find where that's stated.

>      (2) the units for the te-bandwidth numerical values (and in what cases
>     no units are needed), and for performance-metrics-*-way-bandwidth values
> [TS]: The unit is technology dependent. For example for packet, the example we give in the description is inline with the type bandwidth-ieee-float32 defined in RFC8294. We can clarify this further in description if needed.

I was most concerned about the "packet switching type" since my cursory
look at OTN and/or DWDM indicated that the listed parameters do imply an
absolute bandwidth.  Please do further clarify what the units are for the
float32 value used for packet-switching types.

>     (3) whether threshold-accelerated-advertisement's use of relative vs.
>     absolute values requires any special handling
> [TS]: these are absolute values and inline with the rfc7810#section-5 and rfc7471#section-5

[comments below]

>     (4) whether we want to specify any requirements/error-handling/etc. when
>     there is potential for "nonsense" configuration, such as a
>     max-constraint being configured as numerically smaller than a
>     min-constraint
> [TS]: We have some checks where it was needed. Implementations can also make additional restrictive checks with a deviations if needed.

Okay.

>     (5) what the "variation" in the te-packet-types module's leafs is
>     intending to measure.
> [TS]: the per link delay variation as defined in RFC7823.    

I looked at RFC 7823 and it doesn't help me very much.  Is there a
mathematical formula for what it measures?

>     ----------------------------------------------------------------------
>     COMMENT:
>     ----------------------------------------------------------------------
>     
>     In general it's hard to examine many of these types and see whether they
>     are fit for purpose without some sense of how they would be used, but of
>     course following a chain of references for all of the types in question
>     wouldu be tedious to the point of exhaustion.  I'm reluctant to start
>     getting into examples (since such a list would necessarily be
>     incomplete), but if you want one, the 'suppression-interval' leaf has a
>     very terse description, that will presumably be supplemented by some
>     text in some other document about what exactly is getting suppressed and
>     when, but if I were to try to implement from just this document I'd be
>     doing a lot of guessing.
>     
> [TS]: Agreed. For 'suppression-interval', it is detailed in rfc7810#section-6. We will update the description/reference for this.
> 
>     Section 3
>     
>        types for TE generic types and ietf-te-packet-types for packet
>        specific types.  Other technology specific TE types are outside the
>     
>     nits: "packet-specific" and "technology-specific" to hyphenate the
>     compound adjectives.
> [TS]: updated.
>     
>     Section 3.1
>     
>        te-node-id:
>     
>           A type representing the identifier for a node in a TE topology.
>           The identifier is represented as 32-bit unsigned integer in the
>           dotted-quad notation.  This attribute MAY be mapped to the Router
>     
>     Why is te-node-id IPv4-centric?  E.g., RFC 6119 describes an "IPv6 TE
>     Router ID" TLV as well as the "TE Router ID" TLV that we reference here
>     for the node ID.
>     Furthermore, either it's a 32-bit unsigned integer or it's represented
>     in the dotted-quad notation, but the dotted-quad notation doesn't really
>     count as a 32-bit unsigned integer.
> [TS]: this was discussed on the WG mail list, and agreement was to keep TE node ID as dotted-quad 4 octets. We updated the description as below.. https://mailarchive.ietf.org/arch/msg/teas/bP5VurFZKQVx3hNWxUVz6alRIGg
> We can update description to:
> OLD:
>        The identifier is represented as 32-bit unsigned integer in
>        the dotted-quad notation.
> NEW:
>        The identifier is represented as 4 octets in dotted-quad notation.    
>        te-metric:
>     
>           A type representing the TE link metric as defined in [RFC3785].
>     
>     (editorial) the phrase "link metric" does not appear in RFC 3785.
> [TS]: Sure. 
> OLD:
> A type representing the TE <link> metric as defined in [RFC3785].
>  
> NEW:
> A type representing the TE metric as defined in [RFC3785].
>     
>     Section 3.2
>     
>        The ietf-te-packet-types module covers the common types and groupings
>        specific packet technology.
>     
>     nit: I think there's a word missing here.
> [TS]: update to: 
> NEW:
>        The ietf-te-packet-types module covers the common types and groupings
>        that are specific to packet technology.
>     
>        performance-metrics-attributes-packet:
>     
>           A YANG grouping for the augmentation of packet specific metrics to
>           the generic performance metrics grouping parameters.
>     
>     nit(?) I can't tell which way the augmentation is supposed to go, just
>     from this text.
> [TS]: update text to:
> NEW: A YANG grouping that contains the generic performance metrics and additional packet specific metrics.
>     
>     Section 4
>     
>       typedef admin-group {
>         type yang:hex-string {
>           /* 01:02:03:04 */
>           length "1..11";
>         }
>         description
>           "Administrative group/Resource class/Color representation in
>            hex-string type.";
>         reference "RFC3630 and RFC5305";
>     
>     RFC 5305 says to treat this as essentially a bitmask.  What are the
>     semantics when I only supply a string of (say) length 1 or 2?
> 
> [TS]: number is stored as 32-bits. As indicated earlier:
> 01
> 00:01
> 00:00:00:01 all indicate bit-position 0 is set and can internally be represented as 0x00000001
> 
>     
>       typedef performance-metrics-normality {
>            [...]
>         reference
>           "RFC7471: OSPF Traffic Engineering (TE) Metric Extensions.
>            RFC8570: IS-IS Traffic Engineering (TE) Metric Extensions.
>            RFC7823: Performance-Based Path Selection for Explicitly
>            Routed Label Switched Paths (LSPs) Using TE Metric
>            Extensions";
>     
>     None of these three references describes "normal" or "abnormal".
> [TS]: The RFCs refer to it as "anomaly or anomalous". We updated the description to indicate this.
>     
>       typedef te-bandwidth {
>         type string {
>           pattern
>             '0[xX](0((\.0?)?[pP](\+)?0?|(\.0?))|'
>           + '1(\.([\da-fA-F]{0,5}[02468aAcCeE]?)?)?[pP](\+)?(12[0-7]|'
>           + '1[01]\d|0?\d?\d)?)|0[xX][\da-fA-F]{1,8}|\d+'
>           + '(,(0[xX](0((\.0?)?[pP](\+)?0?|(\.0?))|'
>           + '1(\.([\da-fA-F]{0,5}[02468aAcCeE]?)?)?[pP](\+)?(12[0-7]|'
>           + '1[01]\d|0?\d?\d)?)|0[xX][\da-fA-F]{1,8}|\d+))*';
>         }
>     
>     Can the document shepherd please report what validation has been done
>     for this pattern?
>     
>            For packet switching type, a float number is used, such as
>            0x1p10.
>     
>     What kind of units are implied for this (and the other types)?
> [TS]: An answer was given earlier, let us know if that clarifies.
>     
>       typedef te-metric {
>         type uint32;
>         description "TE link metric";
>         reference "RFC3785";
>     
>     [same comment about "link metric" and 3785]
> [TS]: Addressed.
>     
>       typedef te-node-id {
>         type yang:dotted-quad;
>         description
>           "A type representing the identifier for a node in a TE
>            topology.
>            The identifier is represented as 32-bit unsigned integer in
>            the dotted-quad notation.
>     
>     [same comment about uint32 vs. dotted-quad]
> [TS]: Addressed.
>     
>       typedef te-oper-status {
>         [...]
>     
>     Is there some way (a grouping?) to deduplicate the admin and operational
>     status enumerations?  They seem to be nearly identical in terms of the
>     range of possible values.
> [TS]: Yes, created a common type and referenced it the respective typedefs.
>     
>       identity se-style-desired {
>         [...]
>     
>     (et seq) The rtgdir's comments about whether 'base' should be present
>     seems relevant for these as well.
> [TS]: thanks for pointing it out, added the missing base.
>     
>       identity oob-mapping-flag {
>     [....]
>       identity entropy-label-capability {
>     
>     Are all of the flags based on lsp-attributes-flags supposed to be
>     "desired" in the description (these aren't)?
> [TS]: These identities simply indicate the LSP attributes. Every flag has its own semantics. The respective RFCs describe whether any action is to be taken by nodes along the LSP path.
> 
>       identity oam-mep-entity-desired {
>         base lsp-attributes-flags;
>         description "OAM MEP entities desired";
>     
>     We probably should expand Maintenance Entity Group End Point (and MIP,
>     below).
> [TS]: expanded.
>     
>       identity loopback-desired {
>         base lsp-attributes-flags;
>         description
>           "This flag indicates a particular node on the LSP is
>            required to enter loopback mode.  This can also be
>     
>     Is there a mismatch between "desired" and "required"?
> [TS]: yes, RFC7571 uses the term "required" (as we have in description). We renamed the identity to be in sync with the RFC.
> OLD:
> identity loopback-desired {
> NEW:
> identity loopback-required {
>     
>       identity rtm-set-desired {
>         base lsp-attributes-flags;
>         description
>           "Residence Time Measurement (RTM) attribute flag";
>     
>     Is there a mismatch regarding "desired" in the identity name vs.
>     description?
> [TS]: RFC 8169 section 4.4 uses "requested" - we think this can be equated to desired.
> <snip>
> .. indicates to the egress node that RTM is requested
> </snip>
> 
> updated:
> NEW:
>       identity rtm-set-desired {
>         description
>           "Residence Time Measurement (RTM) attribute flag requested";
> 
> 
>     
>       identity link-protection-type {
>         description "Base identity for link protection type.";
>       }
>       identity link-protection-unprotected {
>         base link-protection-type;
>     
>     [I assume the WG had some good discussion about identities vs.
>     enumeration and don't want to second-guess that.]
> [TS]: yes, this was discussed and the choice was to pick identities over enums for extensibility. We used enums in places where extensions were not expected.
>     
>       identity association-type-recovery {
>         base association-type;
>         description
>           "Association Type Recovery used to association LSPs of
>            same tunnel for recovery";
>     
>     nit: the grammar seems weird, here; maybe a missing word?
> [TS]: NEW:
>           "Association Type Recovery used to associate LSPs of
>            same tunnel for recovery";
>     
>       identity path-locally-computed {
>         base path-computation-method;
>         description
>           "indicates a constrained-path LSP in which the
>           path is computed by the local LER";
>         reference "RFC3209";
>     
>     In line with the gen-art reviewer's comments, a section ref might be
>     useful here.
> [TS]: Added references.
>     
>       identity path-externally-queried {
>         base path-computation-method;
>         description
>           [...]
>           to the external source to aid computation); and the path that is
>           returned by the external source is not required to provide a
>           wholly resolved path back to the originating system - that is to
>           say, some local computation may also be required";
>     
>     nit: "provide a wholly resolved path back to the originating system"
>     could potentially be read ambiguously about whether "back to the
>     originating system refers to the path or where the (partial) result is
>     returned to.
> [TS]: reworded:
> NEW:
> ..................................................................................... The path that is
> returned by the external source may require further local
> computation on the device.";
>     
> 
>       identity te-tunnel-type {
>         [...]
>     
>     There is no need for a mp2p or mp2mp tunnel type?
>     
>       identity tunnel-state-type {
>         description
>           "Base identity for TE tunnel states";
> [TS]: no, none standardized for TE exists as of yet.
>     
>     This is just "tunnel state" not "operational state"?
>     
>       identity lsp-state-type {
>         description
>           "Base identity for TE LSP states";
>     
>     Is there a state machine for which transitions between states based on
>     this identity are allowed, or is it basically just a full mesh?
> [TS]: we've only modelled possible state outputs. There is no standard state-machine. Implementations may execute a state-machine for transitions, but we did not think it needs to be covered by the model.
>     
>       identity path-invalidation-action-drop-tear {
>         base path-invalidation-action-type;
>         description
>           "TE path invalidation action tear";
>     
>     Is there a reference for this (or the other
>     path-invalidation-action-type)?  My naive reading wants to complete it
>     to "teardown" but I have low confidence that is correct.
> [TS]: renamed identity 'teardown' and added reference to RFC3209 section 2.5 talks about these general principles.
>     
>       identity path-metric-optimize-includes {
>         [...]
>       identity path-metric-optimize-excludes {
>         base path-metric-type;
>         description
>           "A metric that optimizes the number of excluded resources
>            specified in a set";
>     
>     optimizes to minimize or maximize it?
> [TS]: Update to:
> NEW:
>           "A metric that optimizes to a maximum the number of excluded resources
>            specified in a set";
>     
>       grouping performance-metrics-one-way-bandwidth {
>         [....]
>     
>     What are the units on the bandwidth values herein?  bits per second?
> [TS]: The type bandwidth-ieee-float32 is defined in RFC8294. The expected units are bytes per second (this is also inline with rfc3630 and rfc5305#section-3.6).

I note that RFC 5305 even feels a need for a parenthetical: "units are
bytes (not bits!) per second".  To me, the risk/reward is clearly in favor
of being explicit.

>           container threshold-accelerated-advertisement {
>             description
>               "When the difference between the last advertised value and
>                current measured value exceed this threshold, anomalous
>                announcement will be triggered.";
>             uses performance-metrics-thresholds;
>     
>     Are we reusing the performance-metrics-threshold container, which up
>     until now has been used for absolute measurements, to hold the
>     corresponding relative values (differences)?
> [TS]: yes, but these are currently defined as absolute threshold values - not differences/relative.

The "difference between the last advertised value and current measured
value" is a difference with respect to a baseline, which, while still
measured in the same units, is qualitatively different than an absolute
value (i.e., against zero baseline).  The difference is much smaller than
when we go to a percentage difference, where the units change, but the
semantics are slightly different.  Anyway, it sounds like you don't think
there is a problem here, so I can accpet that.

>       grouping optimization-metric-entry {
>         [...]
>         leaf weight {
>     
>     Where is the sense of the 'weight' leaf defined?  That is, to larger
>     weight values get more traffic or less traffic?
> [TS]: this weight is input to optimization function when trying to optimize for multiple metrics. A higher weight signifies a more important metric when trying to find optimal path. 

What reference chain can the reader follow to learn this fact?

>         container path-srlgs-names {
>             [...]
>             leaf-list names {
>               type string;
>               description "List named SRLGs";
>     
>     nit: missing "of"?
> [TS]: updated.
>     
>       grouping generic-path-disjointness {
>         description "Path disjointness grouping";
>         leaf disjointness {
>           type te-path-disjointness;
>           description
>             "The type of resource disjointness.
>              Under primary path, disjointness level applies to
>              all secondary LSPs. Under secondary, disjointness
>              level overrides the one under primary";
>     
>     What does "under" mean?  It seems like it has something to do with the
>     position in the hierarchy where this grouping gets instantiated, which
>     is potentially subtle.
> [TS]: Reworded to clarify.
> NEW:
>             "The type of resource disjointness.
>              When configured for a primary path, the disjointness level applies to
>              all secondary LSPs. When configured for a secondary path, disjointness
>              level overrides the one configured for the primary path";
>     
>         container path-properties {
>           config false;
>           [...]
>           container path-route-objects {
>             description
>               "Container for the list of route objects either returned by
>                the computation engine or actually used by an LSP";
>             list path-route-object {
>               key index;
>               ordered-by user;
>     
>     (How do 'config false' and 'ordered-by user' interact?)
> [TS]: We discussed this within the authors. We think ‘ordered-by user’ for config false leafs will be ignored -- and hence would fallback to the default (‘ordered-by system’) as per RFC6020. Note, we think this is a common behavior that affect NMDA model(s) when extracting applied configuration. Please correct our thinking if this is not true and we’ll try to address it.

I have essentially no domain knowledge on this point; we'd need to call in
some outside expert if we feel an authoritative answer is important.

Thanks for the updates,

Ben

> 
>     
>     Section 5
>     
>        grouping performance-metrics-attributes-packet {
>          [...]
>          uses te-types:performance-metrics-attributes {
>            augment performance-metrics-one-way {
>              leaf one-way-min-delay {
>              [...]
>              leaf one-way-max-delay {
>     
>     What kind of sanity or error checking is there for the nodes in this
>     model, e.g., if max delay is configured to be smaller than min delay?
> [TS]: these are measured values over a time period and maintained as state. The minimum/maximum being over the last period.
>     
>              leaf one-way-delay-variation {
>                type uint32 {
>                  range 0..16777215;
>                }
>                description "One-way delay variation in micro seconds.";
>     
>     What does "variation" mean?  Deviation from mean?  Variance?  Mean
>     absolute error?
> [TS]: This attempts to model the parameters as described in RFC7823 (including delay variation/jitter).
>     
>              leaf one-way-packet-loss {
>                type decimal64 {
>                  fraction-digits 6;
>                  range "0 .. 50.331642";
>     
>     Is there a reason for the oddly specific upper limit?
> [TS]: this is described in https://tools.ietf.org/html/rfc7810#section-4.4 . Updated the description and reference to indicate this.
>    “Link Loss: This 24-bit field carries link packet loss as a percentage
>    of the total traffic sent over a configurable interval.  The basic
>    unit is 0.000003%, where (2^24 - 2) is 50.331642%. “
> 
> 
>     I also can't quite convince myself from the RFC 7950 grammar whether the
>     quotes on the range are valid and/or needed.
> [TS]: RF7950 expects a string for the range argument - so it's OK. Also, made it consistent across the modules
>     
>     I also am unsure as to how useful the "normality" is for some of these,
>     like the packet loss and delay-variation values.
> [TS]: RFC7823 suggests possible utility of those values.
> 
> Regards,
> Tarek
>     
>     [same comments about two-way metrics as one-way metrics, I think.
>     Also the -packet variants.]
>     
>     Section 7
>     
>     We probably could say more here if we want, though I'm as-yet decided on
>     whether we should.
>     That is, things like editing  the config could cause traffic to be
>     disallowed, routed on suboptimal paths, routed through locations more
>     easily targeted for other purposes, etc., and reading config/status
>     could reveal information about network topology, which sites might be
>     most sensitive as targets for subsequent attacks, etc..  It's less clear
>     if there's much that would allow for identification of specific
>     users/customers or their traffic, though.  Maybe some of the DSCP stuff,
>     in some cases.
>     
>     Section 10.2
>     
>     A question for the responsible AD: do references that are used in
>     "reference" statements need to be normative references?  (E.g., RFC
>     3471.)
>     
>     
>     
>