[Lsr] AD review of draft-ietf-lsr-dynamic-flooding-14

[John Scudder <[e-mail]>]

Hi Authors, WG,

Thanks for this document.

I’ve supplied my questions and comments in the form of an edited copy of the draft. Minor editorial suggestions I’ve made in place without further comment, more substantive questions and comments are done in-line and prefixed with “jgs:”. You can use your favorite diff tool to review them; I’ve attached the iddiff output for your convenience if you’d like to use it. I’ve also pasted a traditional diff below in case you want to use it for in-line reply.

Thanks,

—John

--- draft-ietf-lsr-dynamic-flooding-14.txt	2024-01-22 20:02:14.000000000 -0500
+++ draft-ietf-lsr-dynamic-flooding-14-jgs-comments.txt	2024-01-24 07:16:47.000000000 -0500
@@ -231,6 +231,10 @@
    The key words "MUST", "MUST NOT", "REQUIRED", "SHALL", "SHALL NOT",
    "SHOULD", "SHOULD NOT", "RECOMMENDED", "MAY", and "OPTIONAL" in this
    document are to be interpreted as described in RFC 2119 [RFC2119].
+   
+--
+jgs: please update to use current BCP 14 boilerplate. 
+--
 
 2.  Problem Statement
 
@@ -291,6 +295,22 @@
    topology, we can have efficient flooding and retain all of the
    resilience of existing protocols.  A node that supports flooding on
    the decoupled flooding topology is said to support dynamic flooding.
+--
+jgs: "scalable network" seems wrong. "Scaled network" is what a lot of
+people would say, and I wouldn't openly object to it, although I'd hold
+my nose and make a bad face. Perhaps,
+
+OLD:
+   We have observed that the combination of the dense topology and
+   flooding on the physical topology in a scalable network is sub-
+   optimal.
+   
+NEW:
+   We have observed that the combination of the dense topology and
+   flooding on the physical topology is sub-
+   optimal for network scaling.
+--
+
 
    With dynamic flooding, the flooding topology is computed within an
    IGP area with the dense topology either centrally on an elected node,
@@ -301,11 +321,11 @@
    operation.  If the flooding topology is computed in a distributed
    fashion, we call this the distributed mode of operation.  Nodes
    within such an IGP area would only flood on the flooding topology.
-   On links outside of the normal flooding topology, normal database
+   On links outside of the flooding topology, normal database
    synchronization mechanisms (i.e., OSPF database exchange, IS-IS
    CSNPs) would apply, but flooding may not.  Details are described in
    Section 6.  New link state information that arrives from outside of
-   the flooding topology suggests that the sender has a different or no
+   the flooding topology suggests that the sender has different or no
    flooding topology information and that the link state update should
    be flooded on the flooding topology as well.
 
@@ -317,6 +337,18 @@
    topology is stable.  The speed of the computation and its
    distribution, in the case of a centralized mode, is not a significant
    issue.
+--
+jgs: I think this is a little bit too terse, specifically the referent
+of "it" isn't immediately obvious. Perhaps,
+
+NEW:
+   Since the flooding topology is computed prior to topology changes, 
+   the effort required to compute it
+   does not factor into the convergence time and can be done when the
+   topology is stable.  The speed of the computation and its
+   distribution, in the case of a centralized mode, is not a significant
+   issue.
+--
 
    If a node does not have any flooding topology information when it
    receives new link state information, it should flood according to
@@ -383,6 +415,12 @@
    can remain stable in this condition is unknown and may be very
    dependent on the number and location of the nodes that support
    Dynamic Flooding.
+--
+jgs: Is the stability concern solely about scalability? Because, on the
+face of it, "stability is unknown" sounds more alarming than that. If
+it’s only scalability, some rewording seems indicated to help calm the
+reader.
+--
 
 
 
@@ -476,7 +514,7 @@
    Similarly, if additional redundancy is added to the flooding
    topology, specific nodes in that topology may end up with a very high
    degree.  This could result in overloading the control plane of those
-   nodes, resulting in poor convergence.  Thus, it may be optimal to
+   nodes, resulting in poor convergence.  Thus, it may be preferable to
    have an upper bound on the degree of nodes in the flooding topology.
    Again, the optimal trade-off between graph diameter, node degree,
    convergence time, and topology computation time is for further study.
@@ -523,6 +561,9 @@
    topologies that perform well.  Specifically, for N spines and M
    leaves, if M >= N(N/2-1), then there is a flooding topology that has
    a diameter of four.
+--
+jgs: Are you sure you don't mean M >= N(N-1)/2 ?
+--
 
 4.4.2.  Xia Topologies
 
@@ -575,6 +616,10 @@
    understood.  Each node in the spine cycle will never receive an
    update more than twice.  For M leaves and N spines, a spine never
    transmits more than (M/N +1) updates.
+--
+jgs: I'm willing to believe you, but I'm interested to know how this
+result is derived.
+--
 
 4.4.3.  Optimization
 
@@ -626,6 +671,11 @@
    Correct operation of the flooding topology requires that all nodes
    which participate in the flooding topology choose local links for
    flooding which are consistent with the calculated flooding topology.
+--
+jgs: It's possible to pack all kinds of nuance into "consistent with",
+so it makes me look for sleight-of-hand. Is this implied subtlety
+intentional, would it be possible to reword this more directly, ... ?
+--
    Failure to do so could result in an unexpected partition of the
    flooding topology and/or sub-optimal flooding reduction.  As an aid
    to diagnosing problems when dynamic flooding is in use, this document
@@ -749,6 +799,11 @@
 
    2.  Indicate the set of algorithms that it supports for distributed
        mode, if any.
+--
+jgs: the way 2 is worded implies that there is no mandate to advertise
+algorithm 0 (since that isn't "distributed mode"). That seems to conflict
+with Section 6.4 et al. Should "for distributed mode" be deleted?
+--
 
    In incremental deployments, understanding which nodes support Dynamic
    Flooding can be used to optimize the flooding topology.  In
@@ -767,6 +822,10 @@
       Type: TBD7
 
       Length: 0-255; number of Algorithms
+--
+jgs: what does it mean for a router to advertise support for no 
+algorithms?
+--
 
       Algorithm: zero or more numeric identifiers in the range 0-255
       that identifies the algorithm used to calculate the flooding
@@ -790,8 +849,15 @@
    Node IDs (System ID + pseudo-node ID) that it has used in computing
    the area flooding topology.  Conceptually, the Area Leader creates a
    list of node IDs for all nodes in the area (including pseudo-nodes
-   for all LANs in the topology), assigning indices to each node,
+   for all LANs in the topology), assigning an index to each node,
    starting with index 0.
+--
+jgs: I edited "indices" to "an index" assuming you intend that each node
+be uniquely identified by just one index. Also, although it's fairly 
+self-evident, it's likely worth mentioning that the indices are implicit
+and each node's index is the previous node's index + 1, i.e. they go
+0, 1, 2, 3, ... and not any other sequence.
+--
 
    Because the space in a single TLV is limited, more than one TLV may
    be required to encode all of the node IDs in the area.  This TLV may
@@ -898,7 +964,7 @@
 Internet-Draft              Dynamic Flooding                   June 2023
 
 
-   Nodes that support Dynamic Flooding MAY include the Flooding Request
+   A node that supports Dynamic Flooding MAY include the Flooding Request
    TLV in its IIH PDUs.
 
    The Flooding Request TLV has the format:
@@ -919,6 +985,17 @@
       encoded as the circuit type as specified in IS-IS [ISO10589]
 
       R bit: MUST be 0 and is ignored on receipt.
+--
+jgs: It’s evident that this field is the unused high bit in the flooding
+scopes field. I think something needs to be said a little more clearly,
+because if no flooding scopes are advertised, then the R bit doesn’t
+exist, and if additional scopes are advertised, each scope gets its own
+R bit.
+
+An expedient way to fix this would be to eliminate the R field and just
+specify that the (8-bit wide) Scope field's values are restricted to the
+range 0..127.
+--
 
       Scope: Flooding Scope for which the flooding is requested as
       defined in the LSP Flooding Scope Identifier Registry as created
@@ -1010,7 +1087,7 @@
 Internet-Draft              Dynamic Flooding                   June 2023
 
 
-   6.  A bit in the LLS Type 1 Extended Options and Flags requests
+   6.  A bit in the LLS Type 1 Extended Options and Flags that requests
        flooding from the adjacent node.
 
 5.2.1.  OSPF Area Leader Sub-TLV
@@ -1208,8 +1285,11 @@
    the Router IDs that it has used in computing the flooding topology.
    This includes the identifiers associated with Broadcast/NBMA networks
    as defined for Network LSAs.  Conceptually, the Area Leader creates a
-   list of Router IDs for all routers in the area, assigning indices to
+   list of Router IDs for all routers in the area, assigning an index to
    each router, starting with index 0.
+--
+jgs: same comment as for the IS-IS case.
+--
 
 5.2.5.1.  OSPFv2 Area Router ID TLV
 
@@ -1246,6 +1326,13 @@
                    Figure 3: OSPFv2 Router IDs TLV Entry
 
       Conn Type: 1 octet
+--
+jgs: What's a "conn"? I'm only familiar with this term from Star Trek, as
+in "Mr. Sulu, you have the conn". I suppose it might be an abbreviation of 
+"connection", in which case it would be appropriate to spell out in the
+description. (Although is "connection" the best word? Perhaps "ID Type"
+instead, see below?)
+--
 
       -  The following values are defined:
 
@@ -1260,6 +1347,17 @@
 
       Originating Router ID/DR Address:(4 * Number of IDs) octets as
       indicated by the ID Type
+--
+jgs: First, by "ID Type" do you mean "Conn Type"? Because AFAICT there's
+no "ID Type" defined. (Maybe "Conn Type" should be renamed "ID Type".)
+
+Second, it's pretty funky that you're telling me the number of bytes
+occupied by each field, but nothing of its semantics. This problem 
+wouldn't exist if Figure 3 followed Figure 4 instead of preceding it.
+Why the cart before the horse you put? More directly, I suggest you 
+rearrange this subsection to show the container TLV first, then the 
+contained information.
+--
 
    The format of the Area Router IDs TLV is:
 
@@ -1279,6 +1377,13 @@
       TLV Type: 1
 
       TLV Length: 4 + (8 * the number TLV entries)
+--
+jgs: this doesn't seem right, since the TLV entries are variable length.
+Perhaps adopt the language from the next subsection, viz
+
+      TLV Length: 4 + sum of the lengths of all TLV entries
+
+--
 
       Starting index: The index of the first Router/Designated Router ID
       that appears in this TLV.
@@ -1355,12 +1460,27 @@
        +-    Originating ID Entry                                     -+
        |                    ...                                        |
 
+--
+jgs: In the HTML rendering 
+(https://www.ietf.org/archive/id/draft-ietf-lsr-dynamic-flooding-14.html#name-ospfv3-area-router-id-tlv)
+everything from the diagram above up to (but not including) the "Figure
+5" caption, is artwork, which is ugly since the text directly below this
+comment would normally be expected to render as body text. This is minor
+of course, and the kind of rendering error the RFCEd will fix, but you
+might check to see if there's a bug in your source.
+--
    where
 
          Conn Type - 1 octet
             The following values are defined:
             1 - Router
             2 - Designated Router
+--
+jgs: Conn again
+
+Also, once again I suggest Figure 6 is the one that should come first, not
+Figure 5.
+--
 
          Number of IDs - 2 octets
 
@@ -1498,7 +1618,7 @@
    towards it on a specific link in the case where the connection to the
    adjacent node is not part of the current flooding topology.
 
-   Nodes that support Dynamic Flooding MAY include the FR-bit in its
+   A node that supports Dynamic Flooding MAY include the FR-bit in its
    OSPF LLS Extended Options and Flags TLV.
 
 
@@ -1514,9 +1634,12 @@
 Internet-Draft              Dynamic Flooding                   June 2023
 
 
-   If the FR-bit is signalled for a link on which flooding was disabled
+   If the FR-bit is signaled for a link on which flooding was disabled
    due to Dynamic Flooding, the flooding MUST be temporarily enabled
    over such link and area.  Flooding MUST be enabled until the FR-bit
+--
+jgs: Why "and area"?
+--
    is no longer advertised in the OSPF LLS Extended Options and Flags
    TLV or the OSPF LLS Extended Options and Flags TLV no longer appears
    in the OSPF Hellos.
@@ -1617,6 +1740,16 @@
    topology.
 
    The flooding topology SHOULD be bi-connected.
+--
+jgs: In Section 4.4.2, Xia Topologies, you told me that "At this point,
+M-N leaves remain unconnected. These can be distributed evenly across
+the remaining spines, connected by a single link." So Xia isn't
+bi-connected. This leads to a, shall we say, disconnect between these
+two sections. I imagine this might be addressed by elaborating on the
+circumstances under which the SHOULD can be disregarded (always a good
+practice anyway with SHOULD) which might lead you to add a reference back
+to §4.4.2.
+--
 
 
 
@@ -1661,6 +1794,11 @@
 
    Note that once Dynamic Flooding is enabled, disabling it risks
    destabilizing the network.
+--
+jgs: As with my much earlier comment about stability, I think it would 
+be wise to elaborate on this a little, with at least an outline of the
+potential consequences, magnitude of impact, etc.
+--
 
 6.5.  Distributed Flooding Topology Calculation
 
@@ -1810,6 +1948,14 @@
    In centralized mode, if multiple flooding topologies are present in
    the area link state database, the node SHOULD flood on each of these
    topologies.
+--
+jgs: How would there be multiple flooding topologies? Also, elsewhere you 
+suggest that the backup leader (you don't call it that, but still) should
+also distribute its own candidate topology. As written, the paragraph above
+is telling me to flood on that topo as well, or else it's asking me to be
+more nuanced than is reasonable to assume, in construing what it means 
+for a topology to be "present".
+--
 
    When the flooding topology changes on a node, either as a result of
    the local computation in distributed mode or as a result of the
@@ -1817,6 +1963,16 @@
    continue to flood on both the old and new flooding topology for a
    limited amount of time.  This is required to provide all nodes
    sufficient time to migrate to the new flooding topology.
+--
+jgs: given the lack of atomic updates in all these protocols, how is the
+router supposed to know when it has "the new flooding topology" anyway,
+and not just a piece of it?
+
+For that matter, how do we distinguish between old/new at all? The whole
+old/new thing seems hopelessly nebulous to me, all I see is flooding
+path TLVs, that I concatenate to form a topology, I don't see any way of
+assigning flooding path TLVs to some topology generation.
+--
 
 6.8.  Treatment of Topology Events
 
@@ -1855,6 +2011,13 @@
 
    The request for temporary flooding is withdrawn on the link when all
    of the following conditions are met:
+--
+jgs: you write "is withdrawn" but I think you mean "MUST be withdrawn", 
+right? This isn't some implicit side-effect of the conditions being
+met that Just Happens without protocol messages being sent. It means,
+when you see these conditions met, you have to send the requisite 
+message to withdraw the request. Or...?
+--
 
       The node itself is connected to the current flooding topology.
 
@@ -1866,6 +2029,10 @@
 
    Temporary flooding is stopped on the link when both adjacent nodes
    stop requesting temporary flooding on the link.
+--
+jgs: should this be "either" instead of "both"? (With accompanying 
+necessary grammar edits of course.)
+--
 
 6.8.2.  Local Link Addition
 
@@ -1909,6 +2076,10 @@
    If multiple local links are added to the topology before the flooding
    topology is updated, temporary flooding MUST be enabled on a subset
    of these links.
+--
+jgs: It's unclear why a subset is enough, perhaps a forward reference 
+to 6.8.12?
+--
 
 6.8.3.  Node Addition
 
@@ -1997,6 +2168,10 @@
    mode, or calculated locally in the case of distributed mode and the
    local link on the node that was not part of the flooding topology has
    been added to the flooding topology, the node MUST:
+--
+jgs: I think this needs a rewrite for clarity. If it's not evident to
+you what I'm talking about, I can go into more detail, LMK.
+--
 
       Re-synchronize the Link State Database over the link.  This is
       done using the standard protocol mechanisms.  In the case of IS-
@@ -2025,6 +2200,9 @@
    local link on the node that was part of the flooding topology has
    been removed from the flooding topology, the node MUST remove the
    link from the flooding topology.
+--
+jgs: Clarity again, same as previous subsection.
+--
 
    The node MUST keep flooding on such link for a limited amount of time
    to allow other nodes to migrate to the new flooding topology.
@@ -2043,6 +2221,10 @@
    check if there are any adjacent nodes that are disconnected from the
    current flooding topology.  Temporary flooding MUST be enabled
    towards a subset of the disconnected nodes.
+--
+jgs: It's not obvious why a subset is sufficient. Perhaps a forward
+reference to 6.8.12 would help.
+--
 
 6.8.10.  Failure of the Area Leader
 
@@ -2082,7 +2264,7 @@
 
 6.8.11.  Recovery from Multiple Failures
 
-   In the unlikely event of multiple failures on the flooding topology,
+   In the event of multiple failures on the flooding topology,
    it may become partitioned.  The nodes that remain active on the edges
    of the flooding topology partitions will recognize this and will try
    to repair the flooding topology locally by enabling temporary
@@ -2140,8 +2322,8 @@
 
 7.1.  IS-IS
 
-   This document requests the following code points from the "sub-TLVs
-   for TLV 242" registry (IS-IS Router CAPABILITY TLV).
+   This document requests the following code points from the "IS-IS 
+   Sub-TLVs for IS-IS Router CAPABILITY TLV" registry (IS-IS TLV 242).
 
       Type: TBD1
 
@@ -2156,7 +2338,7 @@
       Reference: This document (Section 5.1.2)
 
    This document requests that IANA allocate and assign code points from
-   the "IS-IS TLV Codepoints" registry.  One for each of the following
+   the "IS-IS Top-Level TLV Codepoints" registry.  One for each of the following
    TLVs:
 
       Type: TBD2
@@ -2199,6 +2381,20 @@
    |  
    |  N - This bit MUST NOT appear in the L2 Bundle Member Attributes
    |  TLV.
+   
+--
+jgs: I'm not sure why this document is requesting this change to the
+registry. It doesn't seem like it's essential to the technology you're
+defining here, just a drive-by patch to a registry you happen to be
+using. Is that right? Or is there some essential reason you need this
+column added? I'm concerned that someone will object to an experimental
+RFC modifying a registry that was established by a standards track RFC.
+If we can avoid the argument by removing this, that would be the easiest
+fix.
+
+If the authors/WG believe the request should remain, I'm willing to
+carry it forward, however.
+--
 
    This document requests that IANA allocate a new bit value from the
    "IS-IS Neighbor Link-Attribute Bit Values" registry.
@@ -2289,6 +2485,13 @@
    This specification also requests a new registry - "OSPF Dynamic
    Flooding LSA TLVs".  New values can be allocated via IETF Review or
    IESG Approval
+   
+--
+jgs: The "or IESG approval" part is probably unnecessary, although I
+don't object. You do need a period at the end of the sentence, though.
+Same thing for the next subsection. (I do see that you've followed the
+pattern of some existing OSPF registries.)
+--
 
 
 
@@ -2389,6 +2592,32 @@
       1-127: Available for standards action.  Individual values are to
       be assigned according to the "Specification Required" policy
       defined in [RFC8126].
+--
+jgs: three notes on this. 
+
+First, although it's a matter of some debate, there's a view that
+"specification required" is functionally the same as "RFC required" in
+the context of IETF specifications. That is, Internet drafts are
+considered insufficient by some. (The argument's foundation is that
+RFC 2026 makes a strong statement that Internet drafts must not be used
+as references, and of course the boilerplate in front of every Internet
+draft continues to say so.) One alternative to avoid a time-consuming
+disagreement about this, would be to use "expert review", including
+guidance to the expert that approval requires a specification, and
+optionally guidance as to what constitutes an adequate specification as
+well.
+
+Second, even "specification required" asks you to provide guidance to
+the designated expert. (See RFC 8126 section 1.3, item 7.) Many of the
+other IGP registries do this by referencing RFC 7370.
+
+Finally, I would cut the sentence "Available for standards action."
+It doesn't seem to add anything, other than confusion because "Standards
+Action" is the name of an IANA policy which is different from the one
+you're using. (And for that matter, formally speaking this document
+isn't a standard nor on standards track so it's a bit odd for it to
+be talking about standards actions.)
+--
 
       128-254: Reserved for private use.
 
@@ -2413,7 +2642,7 @@
    It is possible that an attacker could become Area Leader and
    introduce a flawed flooding algorithm into the network thus
    compromising the operation of the protocol.  Authentication methods
-   as describe in [RFC5304] and [RFC5310] for IS-IS, [RFC2328] and
+   as described in [RFC5304] and [RFC5310] for IS-IS, [RFC2328] and
    [RFC7474] for OSPFv2 and [RFC5340] and [RFC4552] for OSPFv3 SHOULD be
    used to prevent such attack.