Gunter Van de Velde has entered the following ballot position for draft-ietf-pce-sid-algo-25: 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/about/groups/iesg/statements/handling-ballot-positions/ for more information about how to handle DISCUSS and COMMENT positions. The document, along with other ballot positions, can be found here: https://datatracker.ietf.org/doc/draft-ietf-pce-sid-algo/ ---------------------------------------------------------------------- DISCUSS: ---------------------------------------------------------------------- # Gunter Van de Velde, RTG AD, comments for draft-ietf-pce-sid-algo-25 # The line numbers used are rendered from IETF idnits tool: https://author-tools.ietf.org/api/idnits?url=https://www.ietf.org/archive/id/draft-ietf-pce-sid-algo-25.txt # Many thanks for the RTGDIR review from Russ White and the shepherd writeup from Dhruv Dhody. This draft specifies a useful extension and is a timely technology extension. # a general comment i have is that when the document is read in detail all in a single go, that some parts are written in different style as some other parts. This does not always make the document easy to digest for a reader. # I have a few simple to resolve blocking DISCUSS items (easy to resolve) and a bunch of non-blocking COMMENTS for which i hope they can be considered by the author team. # DISCUSS # ======= # [DISCUSS#1] 461 increments. Each such extension MUST be indicated by a dedicated 462 SEBF in the Flags field (similar to the A-flag) and MUST be 463 accompanied by capability signaling in the appropriate capability 464 sub-TLV. GV> The above seems to instruct procedure in normative language for a dedicated SEBF in the flags field similar to the A-Flag, but it does not define that specific entity. How to make sure it is interoperable? Can the exact procedure be defined in this specification to make these future flags interoperable, by mentioning the register etc? # [DISCUSS#2] 583 * S (Strict): If set, the path computation at the PCE MUST fail 584 if the specified SR-Algorithm constraint cannot be satisfied. 585 If unset, the PCE MUST try to compute the path with SR- 586 algorithm constraint specified. If the path computation using 587 the specified SR-Algorithm constraint fails, the PCE MUST try 588 to compute a path that does not satisfy the constraint. GV> "does not satisfy the constraint". Does this allow to use any other algorithm or does this imply falling back to using algorithm 0 (the default SPF)? If this refers to using any other Algorithm topology then i get a hint of under specification , as different devices may use different approach causing unpredictable behavior and potential interop complexities. # [DISCUSS#4] 691 The conversion from 24-bit integer to 32-bit IEEE floating point 692 could introduce some loss of precision. GV> Suddenly there is discussion on 24 and 32 bits in a formal encoding procedure section. Where do these vaklues come from? Can this be clarified? is there risk for interop issues? 1223 | | | TBD4:Unsupported combination of | 1224 | | | constraints GV> The above copy/paste is found in the IANA section. I get suspicion that this is missing some fields a=or explanation why the first columns are left empty? ---------------------------------------------------------------------- COMMENT: ---------------------------------------------------------------------- # comments # ======== 132 [RFC8664] and [RFC9603] specify PCEP extensions to support Segment 133 Routing (SR) over MPLS and IPv6 respectively. GV> RFC9603 uses explicit mentioning that it is about dataplanes, hence i suggest: s/and IPv6/and IPv6 dataplanes/ 143 Signaling SR-Algorithm in ERO and RRO: Mechanisms are introduced for 144 PCEP peers to exchange information about the SR-Algorithm 145 associated with each SID. This includes extending SR-ERO, SR-RRO 146 and SRv6-ERO, SRv6-RRO subobjects to carry an Algorithm field. 147 This document updates [RFC8664] and [RFC9603] to enable such 148 encoding. GV> I am wondering if the wording "about the SR-Algorithm associated with each SID" is correct here. I understand the intent. My understanding is that when we know the SID, then the algorithm is already implicitly known by the IGP through the segment routing extensions for both sr-mpls and srv6. What i am wondering about is if the signaling SR-Algorithm here is not intended for NAI instead so that the a corresponding algorithm aware SID can be associated? Maybe i missed understanding the exact intent of this phrase? 169 2. Terminology GV> This section is slightly inconsistent. Sometimes it expands acronyms and sometimes it doesn't eventhough in both instances a reference RFC is provided 191 The term extension block is used in this document to identify the 192 additional bytes appended to a PCEP Object, which may exist depending 193 on the inclusion of a flag in that object GV> Is this a specific flag for the extension block that can be called out here? 204 Subobject Extension Block: Optional, variable-length extension block 205 for SR-ERO and SR-RRO subobjects defined in Section 4.2.1 of this 206 document. GV> a search through the body of text in the draft seems to indicate that sometimes subobject is used and Subobject. Not sure if that is the intent or if it should be consistent through the document? 214 Existing PCEP specifications lack the mechanisms to explicitly signal 215 and negotiate SR-Algorithm capabilities and constraints. This limits GV> It could be worthwhile to add the word "constraints" to the terminology section to set the scene for what is a 'constraint' in the context of this document. 235 between PCEP peers for purposes such as network monitoring and 236 troubleshooting. In scenarios involving multiple (redundant) PCEs, GV> redundant or resilient? I tend to look at resilient as resiliency and redundant as useless overhead (duplicates for example) 258 However, the implicit algorithm of BSID is independent from SR 259 algorithm used for the SR Policy associated with that BSID. GV> Is this saying that the the SRv6 BSID has through the locator an associated SR-Algorithm, but that when the BSID is used in an SR Policy, then that policy SR-Algorithm overwrites such SR-Algorithm. Which means fully decoupled and no inheritance in any way? is that correct understanding? 261 * Topologies with two Interior Gateway Protocol (IGP) domains, each 262 using the same FAD but with differing algorithm numbers. GV> This confuses me. Would this for example not be a algo 129 in topology#1 and algo 200 in topology#2? how can the PCEP SR-ALgorithm be any different for each of these topologies. I am missing an abstraction which is implied here with the text 276 * SR-Algorithm Capability (S): If the S-flag is set, a PCEP speaker 277 indicates support for the Algorithm field and the Subobject 278 Extension Block in the SR-ERO subobject described in Section 4.2 279 and the SR-Algorithm TLV described in Section 4.4 for LSPs setup 280 using Path Setup Type 1 (Segment Routing) [RFC8664]. It does not 281 indicate support for these extensions for other Path Setup Types. GV> What does it mean as the S-flag is not set? What behavior should PCEP speakers assume? (similar for the SRv6 dataplane) 323 * A-flag (SR-Algorithm Flag): If set to '1' by a PCEP speaker, the 324 Subobject Extension Block MUST be included in the SR-ERO subobject GV> Here is mentioned set to '1' while in prior section it was just mentioned that the flag S-flag was set. Maybe align language for consistency GV> Is there anything that must be assumed if the flag was set to '0'? 335 - the Subobject Extension Block is included (due to an SEBF in a 336 future document) and the Algorithm field MUST be ignored. GV> due to Future document? not sure what this is intends to indicate? 423 SEBF in the subobject's Flags field (e.g., the A-flag defined in this 424 document, or flags defined by future documents). GV> why not simply say flags defined in the future? 428 * If the A bit is 1, and no other SEBF is set, the block Length MUST 429 be 4. GV> A bit, A-Flag, A Flag, i assume all is the same bit? using the same name through the document may help readers 437 * Future documents may define additional SEBFs and corresponding 438 fields, allowing the block to be increased in size beyond the 439 initial 4 bytes as needed. GV> Is this not the explicit intent of TLVs to allow extensions to the field. I do not feel convinced that adding this text blob contributes to the formal procedure definition. Can this be removed? No need to make predictions about the future or what technology will extend the field. 450 Unassigned (24 bits): This field is reserved for future use and MUST 451 be set to zero when sending and ignored when receiving, unless 452 redefined by a future extension that is indicated by an associated 453 SEBF and capability. GV> The text "unless redefined by a future extension that is indicated by an associated SEBF and capability." sounds a bit wishy washy. Can this not be removed? If it is changed in the future it will update this rfc-to-be anyway 458 Future extensions SHOULD first re-use the Reserved portion of the GV> Why re-use? was it used before? would this not be simply 'used"? 461 increments. Each such extension MUST be indicated by a dedicated 462 SEBF in the Flags field (similar to the A-flag) and MUST be 463 accompanied by capability signaling in the appropriate capability 464 sub-TLV. GV> [DISCUSS#1] The above seems to instruct in normative was a dedicated SEBF in the flags field similar to the A-Flag, but it dies not define that entity. How to make sure it is interoperable? Can the exact procedure be defined in this specification? 466 When receiving a Subobject Extension Block longer than 4 bytes, 467 receivers that do not recognize or have not negotiated support for 468 additional flags MUST ignore the unknown additional bytes beyond 469 those defined in this document. GV> Beyond ignoring must the receiver do something else? logging? not forwarding? 473 Future documents extending the Subobject Extension Block MUST: GV> i suspect that it is not Documents, but Future "enhancements" extending.... 475 * Define a new SEBF in the Flags field to indicate their extension, 476 and specify corresponding capability signaling. GV> i guess i am thrown off the rails by not understanding PCEP in the greatest detail. The language used here says "their extension". What does that exactly indicate? DOes that mean a new flag (assigned in the future when describing the extension) needs to be added to indicate the a specific extension ? anything else? 482 * The reserved bits in the initial 4 bytes are reused when possible, 483 and the block is extended only when additional space is necessary. GV> Mentioning of reused again, Would simply saying 'used' not be more correct? they were not used before, so there is no reuse i think. 485 * Future documents may define additional SEBFs and corresponding 486 fields, allowing the block to be increased in size beyond the 487 initial 4 bytes as needed. GV> s/Future documents/Future extensions/ ... i think the intent is to describe extensions and not documents. 489 Example: Future extension introducing a Z-flag and a new Z field (8 490 bits): GV> For clarity, will there be a reserved Z-flag in a register somewhere with a very specific location in the flags field? 508 Reserved field. Further, a new "A" flag in defined in the existing 509 Flags field as shown in Figure 3. GV> s/in/is/ 520 | (128-bit) | 530 A new bit in the Flags field: GV> The flag is being defined by this document, and by implicit understanding it is new. Maybe simply remove this statement? 532 A-flag (SR-Algorithm Flag): If set to '1' by a PCEP speaker, the 533 Algorithm field is included in SRv6-ERO subobject as specified in 534 Figure 3. If this flag is set to 0, then the Algorithm field is 535 absent and processing described in Section 5.2.1 of [RFC9603] 536 applies. GV> set to '1' and set to 0... different use of of accents. maybe use consistent markups. 538 Reserved (8 bits): Reduced from 16 to 8 bits. It MUST be set to zero 539 while sending and ignored on receipt. GV> Why mention it is reduced? reduced from where and why? In this formal encoding description the field length is exactly 8 bits. maybe remove the 'reduced'? 544 Note: Subobject Extension Block is applicable to SRv6-ERO Subobject, 545 but is not required by this specific document as existing reserved 546 space is re-used. When additional space is needed in the SRv6-ERO GV> s/document/specification/ 552 A new TLV for the LSPA Object is introduced to carry the SR-Algorithm GV> s/A new TLV for the LSPA/The LSPA/ 552 A new TLV for the LSPA Object is introduced to carry the SR-Algorithm 553 constraint (Section 5.2). This TLV SHOULD only be used when PST 554 (Path Setup type) = 1 or 3 for SR-MPLS and SRv6, respectively. Only 555 the first instance of this TLV MUST be processed, subsequent 556 instances MUST be ignored. GV> What happens if it used for other path setups? maybe this is a MUST instead of a SHOULD? 570 Type (16 bits): 66. 571 572 Length (16 bits): 4. GV> These are values defined in this specification, correct? maybe call that out explicitly instead of just displaying a numbers 579 Flags (8 bits): This document defines the following flag bits. The 580 other bits MUST be set to zero by the sender and MUST be ignored 581 by the receiver. GV> s/bits/bit/ GV> or is it a flag instead of a bit? 583 * S (Strict): If set, the path computation at the PCE MUST fail 584 if the specified SR-Algorithm constraint cannot be satisfied. 585 If unset, the PCE MUST try to compute the path with SR- 586 algorithm constraint specified. If the path computation using 587 the specified SR-Algorithm constraint fails, the PCE MUST try 588 to compute a path that does not satisfy the constraint. GV> [DISCUSS#2] GV> "does not satisfy the constraint". Does this allow to use any other algorithm or does this imply falling back to using algorithm 0 (the default SPF)? If this refers to using any other Algorithm topology then i get a hint of under specification , as different devices may use different approach causing unpredictable behavior and potential interop complexities. 596 document specifies new types for the METRIC object to enable the GV> s/new types/additional types/ 614 * A network comprises of a set of N links {Li, (i=1...N)}. GV> What is Li, i exact (it not so complicated to deduct from the text, however nailing it down in text seems to allow it to be more error proof) 691 The conversion from 24-bit integer to 32-bit IEEE floating point 692 could introduce some loss of precision. GV> [DISCUSS#3] where is the 24 and 32 bit coming from? 984 5.3. New Metric types GV> it reads odd to see new metric types. in few years they are not new anymore. Suggest to rename this section to something that describes that it is about metrics types being specified in this document and in the future 1223 | | | TBD4:Unsupported combination of | 1224 | | | constraints GV> [DISCUSS#4] is this missing some entries as Error-type and meaning? Kind Regards, Gunter Van de Velde RTG Area Director _______________________________________________ Pce mailing list -- [email protected] To unsubscribe send an email to [email protected]
