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

[Tony Li <[e-mail]>]

John,

Thank you for your fantastic comments.  Please see inline.


> +++ 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. 
> +--


Done


> 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.
> +--
> +


Done


>    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.


Done


> @@ -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.
> +--


Done


>    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.
> +--


Flooding at small scale is never a problem, so yes, the concern is always about flooding
at large scale.

Reworded to: 
	  Whether or not the
	  network can remain stable in this condition is very
	  dependent on the number and location of the nodes that
	  support Dynamic Flooding.

It would be disingenous to suggest that this situation is stable. If the clique of 
legacy flooding is large enough, then havoc will undoubtedly ensue. Simply partitioning cliques 
of legacy flooding with boundaries of dynamic flooding is not sufficient: a single dynamic 
flooding system in a sea of legacy flooding only produces legacy flooding. It takes 
large swaths of dynamic flooding to encapsulate the churn of legacy flooding to give 
stability.


> @@ -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.


Done


> @@ -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 ?
> +--


Yes.


> 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.
> +--


Observe that if we have evenly distributed the leaves across the spines, then
each spine has ceil(M/N) leaves. In the worst case, the LSP is self-modified, so it needs
to be flooded to all ceil(M/N) neighbors. Finally, ceil(M/N) <= (M/N + 1), but the latter
is perhaps more clear.


> 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, ... ?
> +--


s/consistent with/part of/


>    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?
> +--


Agreed, done.


>    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?
> +--


Amended to 1-255.  6.4 prohibits zero 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.
> +--


Done.

> 
>    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.

Done


>    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.
> +--


Done


>       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.


Done

> 
> 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.
> +--
> 

Done



> 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?)
> +--


Changed to ID Type.


>       -  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.
> +--


I cannot speak to why, as I didn’t originate it. However, I have fixed it.


>    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
> +
> +--


Done


>       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.
> +--



Fixed.


>    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.
> +--


Fixed and fixed.


>          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.


Done.

> 
> 
> @@ -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"?
> +--


Fixed and removed.


>    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.
> +--


Done


> @@ -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.
> +--


I added a cross reference back to section 1. If this is insufficient, please let me know.


> 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".
> +--


Rewritten. The condition can happen if there are transients with IS-IS, if multiple LSP fragments are involved. One topology could appear in one set of fragments and another topology could appear in a second set of fragments. Thanks to race conditions during flooding, you could have both present
simultaneously.  The backup leader is not part of this. I think I’ve clarified this.


>    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.
> +--


Added text:

	  In centralized mode, it is not necessary for a node to
	  distinguish between the old and new flooding topologies. As
	  updated information comes in, it can be added to the
	  existing flooding topology. As old information is replaced
	  by subsequent updates, it can be removed, thereby converging
	  to the new information.

> 
> 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...?
> +--


Sure. More precisely, you would stop advertising the flooding request in your hellos.


>       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.)
> +--


I think we want “both” for safety.  If either node hasn’t yet learned the correct flooding topology, then that node should continue to advertise a flooding request and temporary flooding should continue.


> 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?
> +--


Added.

> 
> 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.
> +--


Rewritten and clarified.


>       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.
> +--


Rewritten and clarified.


>    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.
> +--


Added.


> 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


Fixed


> @@ -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


Fixed.


> @@ -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


Fixed.


> @@ -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.
> +--


The request for this was raised by a WG member (Les Ginsberg, if I remember correctly) who
felt that we should not touch the registry unless it was fixed.


>    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.)
> +--


Fixed twice.


> @@ -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.)
> +--


All fixed.
> 
>       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.


Fixed.

Tony