Re: [Lsr] Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT)
Hi Ben, Thanks for the excellent review - see one inline. On 10/8/19, 6:13 AM, "Benjamin Kaduk" wrote: Hi Stephane, Thanks for pulling in the fixes; just a few notes inline (and trimming)... On Tue, Oct 08, 2019 at 10:33:49AM +0200, slitkows.i...@gmail.com wrote: > Hi Benjamin, > > Thanks for your comment. > Please find some inline answers. > I'm doing the changes as part of the -41. > > > Stephane > > -Original Message- > From: Benjamin Kaduk via Datatracker > Sent: mercredi 2 octobre 2019 03:17 > To: The IESG > Cc: draft-ietf-isis-yang-isis-...@ietf.org; Yingzhen Qu ; aretana.i...@gmail.com; lsr-cha...@ietf.org; yingzhen.i...@gmail.com; lsr@ietf.org > Subject: Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT) > [...] > > > In the "protection-available" container, is there some sort of constraint that two of the alternate-metrics add up to the third? > [SLI] It is not really a constraint from a YANG point of view, these are operational counters. Sorry, I shouldn't have used the word "constraint", since that's a YANG verb. I was just asking if the description should reiterate to the reader that in normal circumstances the two will add up to the third (and "no" is a fine answer). [...] >container delay-metric { > leaf metric { >type std-metric; >description "IS-IS delay metric for IPv4 prefix"; > } > leaf supported { >type boolean; > > Should the "metric" leaf be conditional on "supported" being true? > (Same for the other flavors of metric, as well as when they appear later on in the "neighbor" grouping.) > > > [SLI] This is not a configuration leaf, so I don't think that there is a need for enforcement at YANG level. Oh, I had missed that it was config false; sorry about that. [...] > >container unidirectional-link-delay-variation { > description >"Container for the average delay variation >from the local neighbor to the remote one."; > leaf value { >type uint32; >units usec; >description > "Delay variation value expressed in microseconds."; > > Is this a standard deviation (variance), mean absolute error, ...? > [SLI] We just mimic what is defined in RFC8570. As the reference is defined in the model. People should refer to it for more details. > Moreover this is a pure operational state. https://tools.ietf.org/html/rfc8570#section-4.3 still leaves me a little unclear about what exactly is being reported (it sounds like it's just an average link delay so the word "variation" confuses me), but I agree that we should not say any more in this document. >container unidirectional-link-loss { > [...] > leaf value { >type uint32; >units percent; >description > "Link packet loss expressed as a percentage > of the total traffic sent over a configurable interval."; > > This document is all about specifying configuration. How do I configure the interval in question? > [SLI] This is an operational state. The leaf value is operational state, yes, but it reports a valid computed "over a configurable interval". How do I configure that interval? I don't disagree that there is more configuration and state associated with these traffic metrics. However, these are not really under the purview of this model since IS-IS (and OSPF for that matter) are only serving as a mechanism to advertise these traffic metrics. Thanks, Acee Thanks! -Ben ___ Lsr mailing list Lsr@ietf.org https://www.ietf.org/mailman/listinfo/lsr
Re: [Lsr] Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT)
Hi Benjamin, Thanks for your comment. Please find some inline answers. I'm doing the changes as part of the -41. Stephane -Original Message- From: Benjamin Kaduk via Datatracker Sent: mercredi 2 octobre 2019 03:17 To: The IESG Cc: draft-ietf-isis-yang-isis-...@ietf.org; Yingzhen Qu ; aretana.i...@gmail.com; lsr-cha...@ietf.org; yingzhen.i...@gmail.com; lsr@ietf.org Subject: Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT) Benjamin Kaduk has entered the following ballot position for draft-ietf-isis-yang-isis-cfg-40: 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://www.ietf.org/iesg/statement/discuss-criteria.html for more information about IESG DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-isis-yang-isis-cfg/ -- DISCUSS: -- Perhaps the most minor thing that could be Discuss-level, and should be trivial to resolve, but: The "i-e" leaf in groupings prefix-ipv4-std and neighbor does not say whether boolean value true corresponds to internal or external. [SLI] Right, I have added the following statement in the description: "Set to false to indicate an internal metric" -- COMMENT: -- Section 1 This document defines a YANG [RFC7950] data model for IS-IS routing protocol. nit: "the IS-IS routing protocol" [SLI] Fixed Section 2.8 This YANG module supports LFA (Loop Free Alternates) [RFC5286] and remote LFA [RFC7490] as IP Fast Re-Route (FRR) techniques. The "fast-reroute" container may be augmented by other models to support other IP FRR flavors (MRT, TI-LFA, etc.). If we're going to give examples of other flavors, do we want to have informative references for them? (This is particularly relevant since we define enumeration values for them in the "alternate-type" enumeration.) [SLI] Sounds good, I have added some references. Section 6 identity lsp-attached-default-metric-flag { base lsp-flag; description "Set when originator is attached to another area using the referred metric."; nit(?): I'm not sure whether the "referred" in the description is appropriate given the "default" in the identity name. [SLI] Fixed feature ldp-igp-sync { description "LDP IGP synchronization."; nit: the surrounding context suggests that "Support for" would give a more consistent style. Maybe for auto-cost, te-rid, max-ecmp, lsp-refresh, and admin-control as well. [SLI] Fixed feature nlpid-control { description "This feature controls the advertisement of support NLPID within IS-IS configuration."; nit: "support for" [SLI] Fixed feature maximum-area-addresses { description "Support of maximum-area-addresses config."; nit: s/of/for/ [SLI] Fixed leaf alternate-type { type enumeration { [...] enum other { description "Unknown alternate type."; Do I remember correctly that enumerations are not extensible in the future? I don't know how relevant that would be here, though. [SLI] Right, I have changed to identity In the "protection-available" container, is there some sort of constraint that two of the alternate-metrics add up to the third? [SLI] It is not really a constraint from a YANG point of view, these are operational counters. container unprotected-routes { config false; list address-family-stats { nit: the name/description of address-family-stats doesn't match up with the parent container that just claims to be a list of unprotected prefixes (no stats). [SLI] I agree, I have changed the address-family-stats to "prefixes" list protection-statistics { side note: I wonder whether the various uint32 leaves are better as gauge32 than plain uint32. Also, perhaps the description could mention a relationship bteween total-routes/protected-routes+unprotected-routes, and/or protected-routes/link-protected-routes+node-protected-routes. grouping route-content { [...] leaf-list tag { type uint64; description "List of tags associated with the route. The leaf describes both 32-bit and 64-bit tags."; Are these the admin tags from RFC 5130? Where is it specified that the 32- and 64-bit variants are just different views into a single consolidated tag namespace? [SLI] Right, I have added a better description.
[Lsr] Benjamin Kaduk's Discuss on draft-ietf-isis-yang-isis-cfg-40: (with DISCUSS and COMMENT)
Benjamin Kaduk has entered the following ballot position for draft-ietf-isis-yang-isis-cfg-40: 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://www.ietf.org/iesg/statement/discuss-criteria.html for more information about IESG DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-isis-yang-isis-cfg/ -- DISCUSS: -- Perhaps the most minor thing that could be Discuss-level, and should be trivial to resolve, but: The "i-e" leaf in groupings prefix-ipv4-std and neighbor does not say whether boolean value true corresponds to internal or external. -- COMMENT: -- Section 1 This document defines a YANG [RFC7950] data model for IS-IS routing protocol. nit: "the IS-IS routing protocol" Section 2.8 This YANG module supports LFA (Loop Free Alternates) [RFC5286] and remote LFA [RFC7490] as IP Fast Re-Route (FRR) techniques. The "fast-reroute" container may be augmented by other models to support other IP FRR flavors (MRT, TI-LFA, etc.). If we're going to give examples of other flavors, do we want to have informative references for them? (This is particularly relevant since we define enumeration values for them in the "alternate-type" enumeration.) Section 6 identity lsp-attached-default-metric-flag { base lsp-flag; description "Set when originator is attached to another area using the referred metric."; nit(?): I'm not sure whether the "referred" in the description is appropriate given the "default" in the identity name. feature ldp-igp-sync { description "LDP IGP synchronization."; nit: the surrounding context suggests that "Support for" would give a more consistent style. Maybe for auto-cost, te-rid, max-ecmp, lsp-refresh, and admin-control as well. feature nlpid-control { description "This feature controls the advertisement of support NLPID within IS-IS configuration."; nit: "support for" feature maximum-area-addresses { description "Support of maximum-area-addresses config."; nit: s/of/for/ leaf alternate-type { type enumeration { [...] enum other { description "Unknown alternate type."; Do I remember correctly that enumerations are not extensible in the future? I don't know how relevant that would be here, though. In the "protection-available" container, is there some sort of constraint that two of the alternate-metrics add up to the third? container unprotected-routes { config false; list address-family-stats { nit: the name/description of address-family-stats doesn't match up with the parent container that just claims to be a list of unprotected prefixes (no stats). list protection-statistics { side note: I wonder whether the various uint32 leaves are better as gauge32 than plain uint32. Also, perhaps the description could mention a relationship bteween total-routes/protected-routes+unprotected-routes, and/or protected-routes/link-protected-routes+node-protected-routes. grouping route-content { [...] leaf-list tag { type uint64; description "List of tags associated with the route. The leaf describes both 32-bit and 64-bit tags."; Are these the admin tags from RFC 5130? Where is it specified that the 32- and 64-bit variants are just different views into a single consolidated tag namespace? grouping authentication-global-cfg { I see how "global" is in contrast to a smaller scope (hello, interface, adjacency, etc.) both here and elsewhere, but when we have a global defeault as well as per-level configuration that use the same grouping, it ceases to be universally "global". But, it's probably too late to be worth changing so many names just for aesthetics... leaf poi-tlv { if-feature poi-tlv; type boolean; default false; description "Enable advertisement of IS-IS purge TLV."; nit(?): I thought the purge origin identification TLV was an extension to the generic purge capability but this description ("purge TLV") is very generic. Should any of the uint32 leaves in "packet-counters" instead be of type counter32? grouping spf-log { [...] leaf id { type uint32; description "Event identifier - purely internal value."; Is there anything