Re: [Lsvr] Shepherd review of draft-ietf-lsvr-bgp-spf-28

[Ketan Talaulikar <[e-mail]>]

Hi Acee/Authors,

Thanks for posting the update v29 and giving an overview of the changes to
the WG:
https://mailarchive.ietf.org/arch/msg/lsvr/sb4jZsjkSbdHHWQcJ_Q-pMfq0s0/

However, there were other comments raised during the shepherd review below
that need inline responses for the benefit of the WG. This gives the
opportunity for the WG members to understand the disposition of the
technical and editorial comments, setting up the context and focus for the
2nd WGLC.

We will start the WGLC as soon as the authors have shared the inline
responses to the shepherd review below.

Thanks,
Ketan


On Fri, Sep 22, 2023 at 12:48 PM Ketan Talaulikar <[e-mail]>
wrote:

>
>
>
> *Summary:*
> This shepherd review has been focussed from an implementer's
> perspective and therefore brings up the need for a few more updates in the
> form of clarifications, editorial changes in the form of consolidation of
> certain procedures, adding certain specification details and some
> suggestions
> for fixing certain technical issues.
>
> I expect that some of these comments/suggestions will likely require
> active discussions and WG members should follow/review them closely.
>
>
> *Suggested editorial changes for consolidation of protocol procedures:*
> The flooding procedures (that is the term that I am using in this review)
> is the most important change introduced in BGP after the SPF computation.
> While an existing BGP implementation can "outsource" certain portions
> related
> to SPF, the flooding related changes are very close to the core BGP code
> flows.
>
> One way to look at it (the way I did), is to see that almost the entire
> BGP
> decision process as related mainly to flooding. There is only the route
> computation part which is replaced by the SPF.
>
> How about the following?
> - Sec 5: Here focus only on the encodings and their semantic aspects. Do
> not
>   keep text related to procedures/processing.
> - A new section 6 that covers the origination and flooding (i.e.
> propagation
>   & update in the LSDB) of the NRLIs. This includes the special handling
> when
>   self-originated NLRIs are received. It includes the procedures for
>   origination due to link/prefix/node failures. Most importantly it should
>   include the LSDB synchronization on peering up.
> - Then most of the existing section 6 that covers the SPF and route
> computation.
>
> IMHO the document will improve vastly for implementers if text were
> consolidated in
> this manner and it would make a huge impact on the clarity of this
> protocol
> specification.
>
> This is just a suggestion.
>
>
> *Key Technical Issues:*
> a) This spec (as it stands today) is entirely dependent on some mechanism
> for
> adjacency formation and liveness. I would say normatively dependent.
> Without
> that aspect being covered, I am concerned that the spec may not get to RFC.
> This is especially blocking for the RR/controller peering model. Perhaps
> for the other two peering models, we can address this with configuration
> of the
> directly connected interface neighbors and then using an existing liveness
> mechanism such as BFD (which is mentioned in the document).
>
> b) EOR being optional there are some issues with the initial database sync
> during a session bringup between two routers. I've suggested some solutions
> for this.
>
> c) The use of "algo" and participation of nodes using SPF Capability TLV is
> problematic. The purpose or goal for this is not clarified by the spec - if
> this similar to IGP algo (as in flexalgo), then the encoding does not seem
> proper.
>
> d) The document keeps some encoding aspects loose when it comes to the
> descriptor TLVs in the NLRI part. This would be problematic for interop and
> robustness of the protocol. Please consider tightening things in this area.
>
> e) Issues with the advertisement of prefix mask as a link attribute and
> then
> using the link identifier is a broken method of advertisement of prefix
> reachability when there is a Prefix NLRI for this purpose. There are also
> issues with the encoding.
>
> f) More clarity is required about topology and prefix computation -
> especially
> in the context of dual stack the determination of IPv4/IPv6 enablement on
> the
> links is not signalled explicitly.
>
> g) The document includes the SPF under the BGP decision process currently,
> however it is perhaps better to introduce it as a separate functional block
> that happens after the decision process. The reason is that the decision
> process seems to be intricately associated with the flooding. Some clarity
> on
> this would be helpful.
>
> I expect that these issues and their solutions are discussed within the WG.
>
>
> *Detailed review using the idnits rendering of the draft:*
> 148   This solution avails the benefits of both BGP and SPF-based IGPs.
> 149   These include TCP based flow-control, no periodic link-state refresh,
> 150   and completely incremental NLRI advertisement.  These advantages can
> 151   reduce the overhead in MSDCs where there is a high degree of Equal
> 152   Cost Multi-Path (ECMPs) and the topology is very stable.
> 153   Additionally, using an SPF-based computation can support fast
> 154   convergence and the computation of Loop-Free Alternatives (LFAs).
> 155   The SPF LFA extensions defined in [RFC5286] can be similarly applied
> 156   to BGP SPF calculations.  However, the details are a matter of
> 157   implementation detail.  Furthermore, a BGP-based solution lends
>
> [minor] Perhaps you meant to say "However, the details are implementation
> specific and outside the scope of this document." ?
>
> 158   itself to multiple peering models including those incorporating
> 159   route-reflectors [RFC4456] or controllers.
>
> [major] There are several aspects or features that are available in the
> link-state IGPs which are referenced in passing all through the document in
> various sections - e.g., LFA, MT, SRLG, etc. It would be good to have a
> separate section towards the end of the document to list them all -
> potentially they all form future work? That way this document stays
> focussed on the base protocol spec.
>
> [major] There are some aspects or features in IGPs that are not covered by
> this document and will make readers wonder if they apply - e.g, LANs, P2MP
> circuits, multi-area/level, advertisement of external routes into the
> BGP-SPF
> domain, etc. If they are future work, they can be called out as such or
> there
> is also the option to entirely leave them out of the protocol given the
> focus on DCs.
>
> 180 1.2.  BGP Shortest Path First (SPF) Motivation
>
> 182   Given that [RFC7938] already describes how BGP could be used as the
> 183   sole routing protocol in an MSDC, one might question the motivation
> 184   for defining an alternate BGP deployment model when a mature solution
> 185   exists.  For both alternatives, BGP offers the operational benefits
> 186   of a single routing protocol as opposed to the combination of an IGP
> 187   for the underlay and BGP as an overlay.  However, BGP SPF offers some
> 188   unique advantages above and beyond standard BGP distance-vector
> 189   routing.  With BGP SPF, the standard hop-by-hop peering model is
> 190   relaxed.
>
> [minor] Did you mean to say ... hop-by-hop route propagation is relaxed?
>
> 192   A primary advantage is that all BGP SPF speakers in the BGP SPF
> 193   routing domain have a complete view of the topology.  This allows
> 194   support for ECMP, IP fast-reroute (e.g., Loop-Free Alternatives),
> 195   Shared Risk Link Groups (SRLGs), and other routing enhancements
> 196   without advertisement of additional BGP paths [RFC7911] or other
> 197   extensions.
>
> [minor] Please add reference to RFC5286 (LFA) and RFC4202 section-2.3
> (SRLG)
>
> 205   Another primary advantage is a potential reduction in NLRI
> 206   advertisement.  With standard BGP distance-vector routing, a single
> 207   link failure may impact 100s or 1000s prefixes and result in the
> 208   withdrawal or re-advertisement of the attendant NLRI.  With BGP SPF,
> 209   only the BGP SPF speakers corresponding to the link NLRI need to
> 210   withdraw the corresponding BGP-LS-SPF Link NLRI.  Additionally, the
>
> [minor] Perhaps you meant to say "only the BGP SPF speakers that originated
> the link NLRI need ..." ?
>
> 211   changed NLRI is advertised immediately as opposed to normal BGP where
> 212   it is only advertised after the best route selection.  These
> 213   advantages provide NLRI dissemination throughout the BGP SPF routing
> 214   domain with efficiencies similar to link-state protocols.
>
> 216   With controller and route-reflector peering models, BGP SPF
>
> [minor] Please provide a forward reference to section 4 where these
> peering
> models are specified.
>
> 217   advertisement and distributed computation require a minimal number of
> 218   sessions and copies of the NLRI since only the latest version of the
> 219   NLRI from the originator is required.  Given that verification of the
> 220   adjacencies is done outside of BGP (see Section 4), each BGP SPF
> 221   speaker only needs as many sessions and copies of the NLRI as
> 222   required for redundancy.
>
> [minor] I think what you want to convey is that a sparse peering approach
> also
> works unlike traditional BGP. And also unlike IGPs there is no need for a
> BGP
> peering with every connected router since adjacency verification is done by
> some other entity. This is not obvious. Please clarify with the help of
> suitable references.
>
> Additionally, a controller could inject
> 223   topology that is learned outside the BGP SPF routing domain.
>
> [major] Please elaborate what is meant by this injection from a
> controller,
> and how this works. Did you mean about advertising external routes that
> are
> redistributed into the domain? Or something else? Please clarify. Or
> perhaps
> just remove this sentence.
>
> 225   Given BGP-LS NLRI is already consumed [I-D.ietf-idr-rfc7752bis], this
> 226   functionality can be reused for BGP-LS-SPF NLRI.
>
> [minor] What is meant by "consumed" here? Please rephrase for clarity.
>
> 228   Another advantage of BGP SPF is that both IPv6 and IPv4 can be
> 229   supported using the BGP-LS-SPF SAFI with the same BGP-LS-SPF NLRIs.
> 230   In many MSDC fabrics, the IPv4 and IPv6 topologies are congruent
> 231   (refer to Section 5.2.2 and Section 5.2.3).  Although beyond the
> 232   scope of this document, multi-topology extensions could be used to
> 233   support separate IPv4, IPv6, unicast, and multicast topologies while
> 234   sharing the same NLRI.
>
> [minor] Perhaps you meant "... while sharing the same AFI/SAFI session" or
> perhaps "... while sharing the same BGP-LS NLRI encoding" or both? Please
> clarify since the same NLRI is not used for different MTs. There will be
> different NLRIs for a prefix for different MTs.
>
> 261 2.  Base BGP Protocol Relationship
>
> 263   With the exception of the decision process, the BGP SPF extensions
> 264   leverage the BGP protocol [RFC4271] without change.  This includes
> 265   the BGP protocol Finite State Machine, BGP messages and their
> 266   encodings, processing of BGP messages, BGP attributes and path
> 267   attributes, BGP NLRI encodings, and any error handling defined in
> 268   [RFC4271] and [RFC7606].
>
> [minor] Reference to RFC4760 is also required here.
>
> 270   Due to the changes to the decision process, there are mechanisms and
> 271   encodings that are no longer applicable.  Unless explicitly specified
> 272   in the context of BGP SPF, all optional path attributes SHOULD NOT be
> 273   advertised.  If received, all path attributes MUST be accepted,
> 274   validated, and propagated consistent with the BGP protocol [RFC4271],
> 275   even if not needed by BGP SPF.
>
> 277   Section 9 of [RFC4271] defines the decision process that is used to
> 278   select routes for subsequent advertisement by applying the policies
> 279   in the local Policy Information Base (PIB) to the routes stored in
> 280   its Adj-RIBs-In.  The output of the Decision Process is the set of
> 281   routes that are announced by a BGP speaker to its peers.  These
> 282   selected routes are stored by a BGP speaker in the speaker's Adj-
> 283   RIBs-Out according to policy.
>
> [major] Link-state protocols do not apply any policy to filter or alter the
> information being flooded as this can lead to routing loops or broken
> forwarding paths. An exception is at area/domain boundaries where prefixes
> are
> filtered. Why does it need to be any different here?
>
> 285   The BGP SPF extension fundamentally changes the decision process, as
> 286   described herein.  Specifically:
>
> [minor] Consider severely trimming the text in the 3 bullets below - i.e.
> don't
> say "how" but only say "what aspect is changed" and then point to the
> section.
> A reader may get confused when reading the document top down.
>
> 288   1.  BGP advertisements are readvertised to neighbors immediately
>
> [minor] I suggest introducing and using the term "flooding" in this
> document
> for this behavior. This way it is immediately clear to readers that this is
> flooding as in IGPs and not propagation as in classic BGP.
>
> 289       without waiting or dependence on the route computation as
> 290       specified in phase 3 of the base BGP decision process.  Multiple
> 291       peering models are supported as specified in Section 4.
>
> [major] How are flooding loops avoided? Basically, if we have learnt the
> same
> NLRI instance from a peer, we should not advertise it back to that peer.
> Right? Does this not need to be specified? Is AS-PATH used? If so, please
> clarify since it will have impact in how this is provisioned (perhaps a
> suitable reference from RFC7938?).
>
> 319 4.  BGP SPF Peering Models
>
> 321   Depending on the topology, scaling, capabilities of the BGP SPF
> 322   speakers, and redundancy requirements, various peering models are
> 323   supported.
>
> The only requirement is that all BGP SPF speakers in the
> 324   BGP SPF routing domain adhere to this specification.
>
> [minor] The above sentence is confusing. Isn't it a given? Or was it
> something else that needs to be conveyed?
>
> 326 4.1.  BGP Single-Hop Peering on Network Node Connections
>
> 328   The simplest peering model is the one where EBGP single-hop sessions
> 329   are established over direct point-to-point links interconnecting the
> 330   nodes in the BGP SPF routing domain.  Once the single-hop BGP session
> 331   has been established and the Multi-Protocol Extensions Capability
> 332   with the BGP-LS-SPF AFI/SAFI has been exchanged [RFC4760] for the
> 333   corresponding session, then the link is considered up from a BGP SPF
> 334   perspective and the corresponding BGP-LS-SPF Link NLRI is advertised.
>
> 336   An End-of-RIB (EoR) Marker [RFC4724] for the BGP-LS-SPF SAFI MAY be
> 337   expected prior to advertising the BGP-LS Link NLRI for to peer.
>
> 339   A failure to consistently configure the use of the EoR marker can
> 340   result in transient micro-loops and dropped traffic due to incomplete
> 341   forwarding state.
>
> [major] Shouldn't EOR be mandatory and form the core requirement of the
> spec
> to ensure database sync at the start of the peering.
>
> 343   If the session goes down, the corresponding Link NLRI are withdrawn.
> 344   Topologically, this would be equivalent to the peering model in
> 345   [RFC7938] where there is a BGP session on every link in the data
> 346   center switch fabric.  The content of the Link NLRI is described in
> 347   Section 5.2.2.
>
> 349 4.2.  BGP Peering Between Directly-Connected Nodes
>
> 351   In this model, BGP SPF speakers peer with all directly-connected
> 352   nodes but the sessions may be between loopback addresses (i.e., two-
> 353   hop sessions) and the direct connection discovery and liveliness
> 354   detection for the interconnecting links are independent of the BGP
> 355   protocol.  For example, liveliness detection could be done using the
> 356   BFD protocol [RFC5880].  Precisely how discovery and liveliness
> 357   detection is accomplished is outside the scope of this document.
>
> [major] There is a normative dependence of this protocol spec on this
> "adjacency" maintenance mechanism. I would consider it to blocking
> progressing
> this document to RFC unless we include a solution that consists of a
> static
> config + something like BFD as an option to unblock.
>
> 358   Consequently, there is a single BGP session even if there are
> 359   multiple direct connections between BGP SPF speakers.  The BGP-LS-SPF
> 360   Link NLRI is advertised as long as a BGP session has been
> 361   established, the BGP-LS-SPF AFI/SAFI capability has been exchanged
> 362   [RFC4760], the link is operational as determined using liveliness
> 363   detection mechanisms, and, optionally, the EoR Marker has been
> 364   received as described in the Section 4.1.  This is much like the
> 365   previous peering model only peering is between loopback addresses and
> 366   the interconnecting links can be unnumbered.  However, since there
> 367   are BGP sessions between every directly-connected node in the BGP SPF
> 368   routing domain, there is a reduction in BGP sessions when there are
> 369   parallel links between nodes.  Hence, this peering model is
> 370   RECOMMENDED over the single-hop peering model Section 4.1.
>
> [minor] I would think that the per-link BGP session in 4.1 also takes care
> of
> adjacency/liveness and hence is somewhat better than 4.2. The protocol spec
> can already specify that in the case of multiple sessions with the same
> neighbor router (identified by BGP Router-ID), the updates need not be sent
> over all the sessions. IMO, this would therefore be the recommended model
> - at
> least while there isn't a good adjacency discovery mechanism spec.
>
> 372   An End-of-RIB (EoR) Marker [RFC4724] for the BGP-LS-SPF SAFI MAY also
> 373   be expected prior to advertising the BGP-LS Link NLRI for the link(s)
> 374   to this peer.
>
> 376 4.3.  BGP Peering in Route-Reflector or Controller Topology
>
> 378   In this model, BGP SPF speakers peer solely with one or more Route
> 379   Reflectors [RFC4456] or controllers.  As in the previous model,
> 380   direct connection discovery and liveliness detection for those links
> 381   in the BGP SPF routing domain are done outside of the BGP protocol.
> 382   BGP-LS-SPF Link NLRI is advertised as long as the corresponding link
> 383   is considered up as per the chosen liveness detection mechanism.
>
> 385   This peering model, known as sparse peering, allows for fewer BGP
> 386   sessions and, consequently, fewer instances of the same NLRI received
> 387   from multiple peers.  Normally, the route-reflectors or controller
> 388   BGP sessions would be on directly-connected links to avoid dependence
> 389   on another routing protocol for session connectivity.  However,
> 390   multi-hop peering is not precluded.  The number of BGP sessions is
> 391   dependent on the redundancy requirements and the stability of the BGP
> 392   sessions.  This is discussed in greater detail in
> 393   [I-D.ietf-lsvr-applicability].
>
> 395   The controller may use constraints to determine when to advertise
> 396   BGP-LS-SPF NLRI for BGP-LS peers.  For example, a controller may
> 397   defer advertisement until the EoR marker has been received from both
> 398   BGP peers and both have received each other's NLRI.  These
> 399   constraints are outside the scope of this document and, since they
> 400   are internal to the controller, need not be standardized.
>
> [major] This is problematic. The reliability of flooding and consistency is
> paramount for any distributed link-state computation. Please also see
> comments
> related to the use of EOR. If the controller or RR is involving in flooding
> then it just needs to behave just like any other BGP-SPF router when it
> comes
> to flooding.
>
> 402 5.  BGP Shortest Path Routing (SPF) Protocol Extensions
>
> 404 5.1.  BGP-LS Shortest Path Routing (SPF) SAFI
>
> 406   This document introduces the BGP-LS-SPF SAFI with a value of 80.  The
> 407   SPF-based decision process (Section 6) applies only to the BGP-LS-SPF
> 408   SAFI and MUST NOT be used with other combinations of the BGP-LS AFI
> 409   (16388).  In order for two BGP SPF speakers to exchange BGP-LS-SPF
> 410   NLRI, they MUST exchange the Multiprotocol Extensions Capability
> 411   [RFC4760] to ensure that they are both capable of properly processing
> 412   such NLRI.  This is done with AFI 16388 / SAFI 80.  The BGP-LS-SPF
> 413   SAFI is used to advertise IPv4 and IPv6 prefix information in a
> 414   format facilitating an SPF-based decision process.
>
> [minor] What is advertised is not prefixes but the link-state database.
> Please
> consider removing the last sentence above and instead introduce a new
> section
> before the current 5.1.1 that introduces the concept of LSDB for
> BGP-SPF and what it comprises. The LSDB is what is advertised using the
> NLRIs. In the same section, describe that each router will advertise its
> own
> Node NLRI and its half-link (rfc7752bis sec 5.2.2) using the link NLRI and
> its
> local prefixes (plus any externals, if that is to be covered). This would
> set
> the context for the sections that follow.
>
> 416 5.1.1.  BGP-LS-SPF NLRI TLVs
>
> 418   All the TLVs defined for BGP-LS [I-D.ietf-idr-rfc7752bis] are
> 419   applicable and can be used with the BGP-LS-SPF SAFI to describe
> 420   links, nodes, and prefixes comprising IGP link-state information.
>
> [minor] Please replace "IGP link-state" with "BGP-SPF LSDB"
>
> 422   The NLRI and comprising TLVs MUST be processed as specified in
> 423   section 5.1 [I-D.ietf-idr-rfc7752bis].
>
> [minor] Perhaps you meant to say that the encoding follows the rules as in
> sec
> 5.1 of rfc7752bis?
>
> TLVs specified as mandatory
> 424   in [I-D.ietf-idr-rfc7752bis] are considered mandatory for the BGP-LS-
> 425   SPF SAFI as well.  If a mandatory TLV is not present, the NLRI MUST
>
> [major] Please remove the above line and instead actually specify exactly
> what is mandatory for BGP-SPF in the respective sections below. There is no
> point in referencing the rfc7752bis for this since a reader will trip over
> the
> text there as it focuses almost entirely on the IGPs.
>
> 426   NOT be used in the BGP SPF route calculation.  All the other TLVs are
> 427   considered as an optional TLVs.
>
> [major] Please consider if we should tighten the NLRI TLVs for BGP-SPF
> since
> they are so critical to consistent LSDB and computation. Unknown TLVs are
> ok
> in the attribute but in NLRI, they would be problematic. At least a very
> strong
> warning for any future protocol extensions to not introduce any other TLV
> in
> the NLRI and if there is anything to introduce then the backward
> compatibility
> aspects are covered.
>
> 429 5.1.2.  BGP-LS Attribute
>
> 431   The BGP-LS attribute of the BGP-LS-SPF SAFI uses exactly same format
> 432   of the BGP-LS AFI [I-D.ietf-idr-rfc7752bis].
>
> [minor] Same reference to sec 5.1 of rfc7752bis can be repeated here or
> perhaps in some common section like section 3?
>
> In other words, all the
> 433   TLVs used in BGP-LS attribute of the BGP-LS AFI are applicable and
> 434   used for the BGP-LS attribute of the BGP-LS-SPF SAFI.  This attribute
> 435   is an optional, non-transitive BGP attribute that is used to carry
> 436   link, node, and prefix properties and attributes.  The BGP-LS
> 437   attribute is a set of TLVs.
>
> [major] Doesn't this need to become a mandatory attribute for BGP-SPF?
>
> 439   All the TLVs defined for the BGP-LS Attribute
> 440   [I-D.ietf-idr-rfc7752bis] are applicable and can be used with the
> 441   BGP-LS-SPF SAFI to carry link, node, and prefix properties and
> 442   attributes.
>
> 444   The BGP-LS attribute may potentially grow large in size depending on
> 445   the amount of link-state information associated with a single Link-
> 446   State NLRI.  The BGP specification [RFC4271] mandates a maximum BGP
> 447   message size of 4096 octets.  It is RECOMMENDED that an
> 448   implementation support [RFC8654] in order to accommodate larger size
> 449   of information within the BGP-LS Attribute.  BGP SPF speakers MUST
> 450   ensure that they limit the TLVs included in the BGP-LS Attribute to
> 451   ensure that a BGP update message for a single Link-State NLRI does
> 452   not cross the maximum limit for a BGP message.  The determination of
> 453   the types of TLVs to be included by the BGP SPF speaker originating
> 454   the attribute is outside the scope of this document.  When a BGP SPF
> 455   speaker finds that it is exceeding the maximum BGP message size due
> 456   to addition or update of some other BGP Attribute (e.g., AS_PATH), it
> 457   MUST consider the BGP-LS Attribute to be malformed and the attribute
> 458   discard handling of [RFC7606] applies.
>
> [minor] Please consider removing the entire paragraph above and instead
> putting
> a reference to the section in rfc7752bis which says exactly the same thing.
>
> 460 5.2.  Extensions to BGP-LS
>
> 462   [I-D.ietf-idr-rfc7752bis] describes a mechanism by which link-state
> 463   and TE information can be collected from IGPs and shared with
> 464   external components using the BGP protocol.  It describes both the
> 465   definition of the BGP-LS NLRI that advertise links, nodes, and
> 466   prefixes comprising IGP link-state information and the definition of
> 467   a BGP path attribute (BGP-LS attribute) that carries link, node, and
> 468   prefix properties and attributes, such as the link and prefix metric
> 469   or auxiliary Router-IDs of nodes, etc.  This document extends the
> 470   usage of BGP-LS NLRI for the purpose of BGP SPF calculation via
> 471   advertisement in the BGP-LS-SPF SAFI.
>
> 473   The protocol identifier specified in the Protocol-ID field
> 474   [I-D.ietf-idr-rfc7752bis] represents the origin of the advertised
> 475   NLRI.  For Node NLRI and Link NLRI, this MUST be the direct protocol
> 476   (4).  Node or Link NLRI with a Protocol-ID other than the direct
> 477   protocol is considered malformed.
>
> [major] Why not BGP (7) as in RFC9086? The descriptors would align with
> RFC9086. If for any use-case, this information needs to be advertised out
> via
> BGP-LS to a multi-domain controller, it would make it easier to identify
> that
> the domain was actually running BGP protocol.
>
>  For Prefix NLRI, the specified
> 478   Protocol-ID MUST be the origin of the prefix.
>
> [major] Why not BGP again? At least for the local IP addresses of the node.
> In any case, this does not seem to have any impact on route computation?
>
>  The local and remote
> 479   node descriptors for all NLRI MUST include the BGP Identifier (TLV
> 480   516) [RFC9086] and the AS Number (TLV 512) [I-D.ietf-idr-rfc7752bis].
>
> [major] The name of the TLV is BGP Router-ID and not BGP Identifier - it
> might
> be confused with BGP-LS Identifier (TLV 513)
>
> 481   The BGP Confederation Member (TLV 517) [RFC9086] is currently not
> 482   applicable.
>
> [major] Why "currently"? Why not just make it not applicable? Please see
> previous comment about keeping things "loose" in the NLRI part.
>
> 484 5.2.1.  Node NLRI Usage
>
> 486   The Node NLRI MUST be advertised unconditionally by all routers in
> 487   the BGP SPF routing domain.
>
> 489 5.2.1.1.  BGP-LS-SPF Node NLRI Attribute SPF Capability TLV
>
> 491   The SPF capability is an additional Node Attribute TLV.  This
> 492   attribute TLV MUST be included with the BGP-LS-SPF SAFI and SHOULD
> 493   NOT be used for other SAFIs.  The TLV type is 1180.  The Node
> 494   Attribute TLV contains a single-octet SPF algorithm as defined in
> 495   [RFC8665].
>
> [major] Is the reference to RFC8665 the SR algorithm TLV? I am not sure why
> the existing SR Algorithm TLV (1035) is not used to indicated the
> algorithm if
> that was the goal. Then again, there is probably no need to advertise algo
> 0
> since that is considered as the default. If it was to explicitly advertise
> BGP-SPF capability as the name suggests then something like the SRv6
> Capabilities TLV (1038) would have been more appropriate - it also gives
> the
> ability to signal some actual "capabilities" down the line. Also, if down
> the
> line if the router is participating/supporting multiple algos will it
> advertise the same TLV multiple times? - here too using SR Algorithm TLV
> would
> have been better.
>
> 497    0                   1                   2                   3
> 498    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 499   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 500   |              Type (1180)      |     Length - (1 Octet)        |
> 501   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 502   | SPF Algorithm |
> 503   +-+-+-+-+-+-+-+-+
>
> 505   The SPF Algorithm field is used to advertise the algorithm used by
> 506   the router to calculate the paths to other routers in the BGP SPF
> 507   routing domain.  The SPF algorithm inherits the values from the IGP
> 508   Algorithm Types registry [RFC8665].  Algorithm 0, (Shortest Path
> 509   Algorithm (SPF) based on link metric, is supported and described in
> 510   Section 6.3.  Support for other algorithm types is beyond the scope
> 511   of this specification.
>
> 513   When computing the SPF for a given BGP routing domain, only BGP nodes
> 514   advertising the SPF capability TLV with same SPF algorithm are
> 515   included in the SPF computation Section 6.3.  An implementation MAY
> 516   optionally log detection of a BGP node that has either not advertised
> 517   the SPF capability TLV or is advertising the SPF capability TLV with
> 518   an algorithm type other than 0.
>
> [major] What about multiple instances of this TLV being advertised?
>
>
> 520 5.2.1.2.  BGP-LS-SPF Node NLRI Attribute SPF Status TLV
>
> 522   A BGP-LS Attribute TLV of the BGP-LS-SPF Node NLRI is defined to
> 523   indicate the status of the node with respect to the BGP SPF
> 524   calculation.  This is used to rapidly take a node out of service
> 525   (refer to Section 6.5.2) or to indicate the node is not to be used
> 526   for transit (i.e., non-local) traffic (refer to Section 6.3).  If the
> 527   SPF Status TLV is not included with the Node NLRI, the node is
> 528   considered to be up and is available for transit traffic.  The SPF
> 529   status is acted upon with the execution of the next SPF calculation
> 530   (refer to Section 6.3).
>
> [minor] What is the need for this clarification "... the next SPF .."
> required
> for this and other TLVs? Is that not the norm for all NLRIs?
>
> 532    0                   1                   2                   3
> 533    0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 534   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 535   |   Type (1184)                 |       Length (1 Octet)        |
> 536   +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 537   | SPF Status    |
> 538   +-+-+-+-+-+-+-+-+
>
> 540   SPF Status Values: 0 - Reserved
> 541                      1 - Node unreachable with respect to BGP SPF
> 542                      2 - Node does not support transit with respect
> 543                          to BGP SPF
> 544                      3-254 - Undefined
> 545                      255 - Reserved
>
> 547   The BGP-LS-SPF Node Attribute SPF Status TLV, Link Attribute SPF
> 548   Status TLV, and Prefix Attribute SPF Status TLV use the same TLV Type
> 549   (1184).
>
> 551   If a BGP SPF speaker received the Node NLRI but the SPF Status TLV is
> 552   not received, then any previously received information is considered
> 553   as implicitly withdrawn and the update is propagated to other BGP SPF
> 554   speakers.  A BGP SPF speaker receiving a BGP Update containing a SPF
>
> [minor] Why is this special handling required only for this TLV? Should it
> not
> be a generic behavior to flood a newer copy of the NLRI regardless of the
> TLVs
> carried within it?
>
> 555   Status TLV in the BGP-LS attribute [I-D.ietf-idr-rfc7752bis] with a
> 556   value that is undefined SHOULD be advertised to other BGP SPF
> 557   speakers.  However, a BGP SPF speaker MUST NOT use the Status TLV in
>
> [minor] I think you meant "unknown" instead of "undefined"? Also, please
> qualify the MUST NOT for clarity - i.e. Status TLVs with unknown values
> MUST
> NOT be used in the SPF.
>
> [major] This makes it very difficult to extend the protocol for new status
> since it would break the consistency of the distributed SPF computation.
> Not sure if this is a good idea when the protocol is not providing some
> actual "flags" to indicate the Router's BGP SPF capability for such things.
> At a minimum, there needs to be some guidance in extending this status. If
> we
> had capability flags, then the routers could verify/check support across
> the
> domain before actually using new(er) extensions.
>
> 558   its SPF computation.  An implementation MAY log this condition for
> 559   further analysis.
>
> 561 5.2.2.  Link NLRI Usage
>
> 563   The criteria for advertisement of Link NLRI are discussed in
> 564   Section 4.
>
> 566   Link NLRI is advertised with unique local and remote node descriptors
> 567   dependent on the IP addressing.  For IPv4 links, the link's local
> 568   IPv4 (TLV 259) and remote IPv4 (TLV 260) addresses are used.  For
> 569   IPv6 links, the local IPv6 (TLV 261) and remote IPv6 (TLV 262)
>
> [minor] It is clarified in rfc7752bis but you may wish to indicate that the
> reference is to global IPv6 addresses here and not link-locals. Link-locals
> are not advertised.
>
> 570   addresses are used.  For links supporting having both IPv4 and IPv6
> 571   addresses, both sets of descriptors MAY be included in the same Link
> 572   NLRI.
>
> [major] I would recommend a tighter spec - leaving things open to MAY in
> the
> NLRI will complicate interop. Perhaps mandate both. For multiple
> addresses,
> perhaps mandate the highest (or lowest) in the NLRI and rest in the
> attribute?
>
> 574   For unnumbered links, the Link Local/Remote Identifiers (TLV 258) are
>
> [major] Same applies for IPv6 link-local only links.
>
> 575   used.  The Link Remote Identifier isn't normally exchanged in BGP and
>
> [major] There is no way anyway for BGP to determine this - so "normally"
> does
> not make sense to me here. I would recommend that a config option be
> provided to
> provision the remote identifier. This way it is not entirely dependent on
> the
> underlying mechanism that is used for adjacency verification.
>
> 576   discovering the Link Remote Identifier is beyond the scope of this
> 577   document.  If the Link Remote Identifier is unknown, a Link Remote
> 578   Identifier of 0 MUST be advertised.  When 0 is advertised and there
>
> [major] These link identifiers are the only always available and
> deterministic
> way to correlated links. It would be good to mandate that they are always
> there and used. Then we don't need to use IP addresses in the descriptor
> and
> things become simpler and more deterministic (like in ISIS and OSPFv3, as
> opposed to OSPFv2). The link identifiers being AF agnostic will greatly
> help
> implementations and deployments.
>
> 579   parallel unnumbered links between a pair of BGP SPF speakers, there
> 580   may be transient intervals where the BGP SPF speakers don't agree on
> 581   which of the parallel unnumbered links are operational.  For this
> 582   reason, it is RECOMMENDED that the Link Remote Identifiers be known
> 583   (e.g., discovered using alternate mechanisms or configured) in the
> 584   presence of parallel unnumbered links.
>
> 586   The link descriptors are described in table 4 of
> 587   [I-D.ietf-idr-rfc7752bis].
>
> 589   For a link to be used in SPF computation for a given address family,
> 590   i.e., IPv4 or IPv6, both routers connecting the link MUST have an
> 591   address in the same subnet for that address family.  However, an IPv4
> 592   or IPv6 prefix associated with the link MAY be installed without the
> 593   corresponding address on the other side of link.
>
> [major] Again, the spec should be tighter. There is no reason to install
> any address from the link (or node) NLRI into the forwarding. If some
> prefix
> needs to be installed in the forwarding, then it MUST be advertised in the
> Prefix NLRI.
>
> 595   The IGP metric attribute TLV (TLV 1095) MUST be advertised.  If a BGP
>
> [minor] RFC7752bis indicates that the IGP metric value is variable. It
> specifies the length for ISIS and OSPF. Suggest to follow this immediately
> with the sentence further below about the length being 4 octets for
> BGP-SPF. The
> spec should also include guidance to ensure that during SPF calculations
> the
> value is capped at a suitable maximum value to ensure there is no overflow
> -
> refer RFC5305 sec 3.7 as an example.
>
> 596   SPF speaker receives a Link NLRI without an IGP metric attribute TLV,
> 597   then it MUST consider the received NLRI as a malformed and the
> 598   receiving BGP SPF speaker MUST handle such malformed NLRI as 'Treat-
> 599   as-withdraw' [RFC7606].
>
> [minor] I think it would be sufficient to just say when any TLV would be
> malformed in its respective section and then leave the handling in the
> Error
> Handling section. Perhaps only the exceptions (if any - I don't see them),
> need to be covered in the respective section.
>
> The BGP SPF metric length is 4 octets.  A
> 600   metric is associated with the output side of each router interface.
> 601   This metric is configurable by the system administrator.  The lower
> 602   the metric, the more likely the interface is to be used to forward
> 603   data traffic.  One possible default for metric would be to give each
> 604   interface a metric of 1 making it effectively a hop count.
>
> [minor] Since we are using the link and prefix metric TLVs that were
> originally defined for IGPs, it is good to clarify here and in the Prefix
> Metric section that those metrics do not convey any "max-metric" or
> "unreachability" notion and that is conveyed by the new TLVs introduced
> for
> BGP-SPF.
>
> 606   The usage of other link attribute TLVs is beyond the scope of this
> 607   document.
>
> 609 5.2.2.1.  BGP-LS-SPF Link NLRI Attribute Prefix-Length TLVs
>
> 611   Two BGP-LS Attribute TLVs of the BGP-LS-SPF Link NLRI are defined to
> 612   advertise the prefix length associated with the IPv4 and IPv6 link
> 613   prefixes derived from the link descriptor addresses.  The prefix
> 614   length is used for the optional installation of prefixes
> 615   corresponding to Link NLRI as defined in Section 6.3.
>
> 617      0                   1                   2                   3
> 618      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 619     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 620     |IPv4 (1182) or IPv6 Type (1183)|          Length (1 Octet)     |
> 621     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 622     | Prefix-Length |
> 623     +-+-+-+-+-+-+-+-+
>
> 625     Prefix-length - A one-octet length restricted to 1-32 for IPv4
> 626                     Link NLRI endpoint prefixes and 1-128 for IPv6
> 627                     Link NLRI endpoint prefixes.
>
> 629   The Prefix-Length TLV is only relevant to Link NLRIs.  When received
> 630   with any NLRIs other than Link NRLIs, the corresponding Link NLRI is
> 631   considered as malformed and MUST be handled as 'Treat-as-withdraw'
> 632   [RFC7606].  An implementation MAY log an error for further analysis.
>
> [major] Why not use the Prefix NLRI for this purpose? This seems to follow
> OSPFv2 while we should be following ISIS or OSPFv3.
>
> 634   The maximum prefix-length is 32 bits for an IPv4 Prefix-Length TLV
> 635   and 128 bits for an IPv6 Prefix-Length TLV.  A prefix-length field
> 636   indicating a larger value is in error and the corresponding Link NLRI
> 637   is considered as malformed and MUST be handled as 'Treat-as-withdraw'
> 638   [RFC7606].  An implementation MAY log an error for further analysis.
>
> [major] When the link has multiple IP addresses, how does one correlate
> which
> prefix has what prefix length?
>
> 640 5.2.2.2.  BGP-LS-SPF Link NLRI Attribute SPF Status TLV
>
> 642   This BGP-LS-SPF Attribute TLV of the BGP-LS-SPF Link NLRI is defined
> 643   to indicate the status of the link with respect to the BGP SPF
> 644   calculation.  This is used to expedite convergence for link failures
> 645   as discussed in Section 6.5.1.  If the SPF Status TLV is not included
> 646   with the Link NLRI, the link is considered up and available.  The SPF
> 647   status is acted upon with the execution of the next SPF calculation
> 648   Section 6.3.  A single TLV type is shared by the Node, Link, and
> 649   Prefix NLRI.  The TLV type is 1184.
>
> 651      0                   1                   2                   3
> 652      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 653     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 654     |   Type (1184)                 |      Length (1 Octet)         |
> 655     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 656     | SPF Status    |
> 657     +-+-+-+-+-+-+-+-+
>
> 659     BGP Status Values: 0 - Reserved
> 660                        1 - Link Unreachable with respect to BGP SPF
> 661                        2-254 - Undefined
> 662                        255 - Reserved
>
> 664   The BGP-LS-SPF Node Attribute SPF Status TLV, Link Attribute SPF
> 665   Status TLV, and Prefix Attribute SPF Status TLV use the same TLV Type
> 666   (1184).  This implies that a BGP Update cannot contain multiple NLRI.
>
> [major] Not sure why this "cannot" is needed. Consider a case where
> multiple
> links are going down, the originator can pack an update with multiple link
> NLRIs with MP_UNREACH and then have a BGP-LS Attribute for them only with
> this
> SPF attribute set to 1. The metric and other things probably don't matter.
>
> 668   If a BGP SPF speaker received the Link NLRI but the SPF Status TLV is
> 669   not received, then any previously received information is considered
> 670   as implicitly withdrawn and the update is propagated to other BGP SPF
> 671   speakers.  A BGP SPF speaker receiving a BGP Update containing an SPF
>
> [minor] When you say "update" is propagated, I believe you mean the BGP
> decision process is followed and if necessary the withdraw is propagated.
> It
> might be easier to just stop after saying considered malformed - which
> results
> in treat-as-withdraw?
>
> 672   Status TLV in the BGP-LS attribute [I-D.ietf-idr-rfc7752bis] with a
> 673   value that is undefined SHOULD be advertised to other BGP SPF
> 674   speakers.  However, a BGP SPF speaker MUST NOT use the Status TLV in
> 675   its SPF computation.  An implementation MAY log this information for
> 676   further analysis.
>
> 678 5.2.3.  IPv4/IPv6 Prefix NLRI Usage
>
> 680   IPv4/IPv6 Prefix NLRI is advertised with a Local Node Descriptor and
> 681   the prefix and length.  The Prefix Descriptors field includes the IP
> 682   Reachability Information TLV (TLV 265) as described in
> 683   [I-D.ietf-idr-rfc7752bis].  The Prefix Metric TLV (TLV 1155) MUST be
> 684   advertised.  The IGP Route Tag TLV (TLV 1153) MAY be advertised.  The
>
> [major] I would suggest to qualify this MUST. Perhaps " ... MUST be
> advertised
> to be considered for route calculation". This would allow for future
> extensibility and use of the Prefix NLRI for conveying other prefix
> information. Otherwise, such NLRIs would be considered malformed - and we
> don't
> want them to be considered malformed.
>
> 685   usage of other BGP-LS attribute TLVs is beyond the scope of this
> 686   document.
>
> 688 5.2.3.1.  BGP-LS-SPF Prefix NLRI Attribute SPF Status TLV
>
> 690   A BGP-LS Attribute TLV to BGP-LS-SPF Prefix NLRI is defined to
> 691   indicate the status of the prefix with respect to the BGP SPF
> 692   calculation.  This is used to expedite convergence for prefix
> 693   unreachability as discussed in Section 6.5.1.  If the SPF Status TLV
> 694   is not included with the Prefix NLRI, the prefix is considered
> 695   reachable.  A single TLV type is shared by the Node, Link, and Prefix
> 696   NLRI.  The TLV type is 1184.
>
> 698        0                   1                   2                   3
> 699        0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 700       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 701       |   Type (1184)                 |      Length (1 Octet)         |
> 702       +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 703       | SPF Status    |
> 704       +-+-+-+-+-+-+-+-+
>
> 706       BGP Status Values: 0 - Reserved
> 707                          1 - Prefix Unreachable with respect to SPF
> 708                          2-254 - Undefined
> 709                          255 - Reserved
>
> 711   The BGP-LS-SPF Node Attribute SPF Status TLV, Link Attribute SPF
> 712   Status TLV, and Prefix Attribute SPF Status TLV use the same TLV Type
> 713   (1184).  This implies that a BGP Update cannot contain multiple NLRI.
>
> 715   If a BGP SPF speaker received the Prefix NLRI but the SPF Status TLV
> 716   is not received, then any previously received information is
> 717   considered as implicitly withdrawn and the update is propagated to
> 718   other BGP SPF speakers.  A BGP SPF speaker receiving a BGP Update
> 719   containing an SPF Status TLV in the BGP-LS attribute
> 720   [I-D.ietf-idr-rfc7752bis] with a value that is undefined SHOULD be
> 721   advertised to other BGP SPF speakers.  However, a BGP SPF speaker
> 722   MUST NOT use the Status TLV in its SPF computation.  An
> 723   implementation MAY log this information for further analysis.
>
> 725 5.2.4.  BGP-LS Attribute Sequence-Number TLV
>
> 727   A BGP-LS Attribute TLV of the BGP-LS-SPF NLRI types is defined to
> 728   assure the most recent version of a given NLRI is used in the SPF
> 729   computation.  The Sequence-Number TLV is mandatory for BGP-LS-SPF
> 730   NLRI.  The TLV type 1181 has been assigned by IANA.  The BGP-LS
>
> [minor] Perhaps there should be a mandate that this TLV is the first one in
> the attribute when used for BGP-SPF to ensure efficient flooding?
>
> 731   Attribute TLV contains an 8-octet sequence number.  The usage of the
> 732   Sequence Number TLV is described in Section 6.1.
>
> 734      0                   1                   2                   3
> 735      0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> 736     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 737     |   Type (1181)                 |      Length (8 Octets)        |
> 738     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 739     |                Sequence Number (High-Order 32 Bits)           |
> 740     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> 741     |                Sequence Number (Low-Order 32 Bits)            |
> 742     +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
>
> 744   Sequence Number: The 64-bit strictly-increasing sequence number MUST
> 745   be incremented for every self-originated version of a BGP-LS-SPF
> 746   NLRI.  BGP SPF speakers implementing this specification MUST use
> 747   available mechanisms to preserve the sequence number's strictly
> 748   increasing property for the deployed life of the BGP SPF speaker
> 749   (including cold restarts).  One mechanism for accomplishing this
> 750   would be to use the high-order 32 bits of the sequence number as a
> 751   wrap/boot count that is incremented any time the BGP router loses its
> 752   sequence number state or the low-order 32 bits wrap.
>
> 754   When incrementing the sequence number for each self-originated NLRI,
> 755   the sequence number should be treated as an unsigned 64-bit value.
> 756   If the lower-order 32-bit value wraps, the higher-order 32-bit value
> 757   should be incremented and saved in non-volatile storage.  If a BGP
> 758   SPF speaker completely loses its sequence number state (e.g., the BGP
> 759   SPF speaker hardware is replaced or experiences a cold-start), the
> 760   BGP NLRI selection rules (see Section 6.1) insure convergence, albeit
> 761   not immediately.
>
> 763   If the Sequence-Number TLV is not received, then the corresponding
> 764   NLRI is considered as malformed and MUST be handled as 'Treat-as-
> 765   withdraw'.  An implementation MAY log an error for further analysis.
>
> 767 5.3.  NEXT_HOP Attribute Manipulation
>
> 769   The rules for setting the next hop information for the BGP-LS-SPF
> 770   SAFI follow the specification in section 5.5 of
> 771   [I-D.ietf-idr-rfc7752bis].  All BGP peers that support SPF extensions
> 772   will locally compute the Local-RIB Next-Hop as a result of the SPF
> 773   process.
>
> [major] It is important to specify what impact the NH reachability has on
> the
> flooding process. In classic BGP, the NH unreachability results in the path
> being considered ineligible for best path computation. For BGP-SPF, that is
> not the case. However, the question is whether the NLRIs would be withdrawn
> when the NH becomes unreachable. There is no need for doing this since we
> are
> only using the session for flooding and the actual NH used are going to be
> determined by the SPF. Also for each of the peering models, would be good
> to
> provide guidance on what to use as an NH by default.
>
> 775 6.  Decision Process with SPF Algorithm
>
> 777   The Decision Process described in [RFC4271] takes place in three
> 778   distinct phases.  The Phase 1 decision function of the Decision
> 779   Process is responsible for calculating the degree of preference for
> 780   each route received from a BGP SPF speaker's peer.  The Phase 2
> 781   decision function is invoked on completion of the Phase 1 decision
> 782   function and is responsible for choosing the best route out of all
> 783   those available for each distinct destination, and for installing
> 784   each chosen route into the Local-RIB.  The combination of the Phase 1
> 785   and 2 decision functions is characterized as a Path Vector algorithm.
>
> 787   The SPF based Decision process replaces the BGP Decision process
> 788   described in [RFC4271].  This process starts with selecting only
> 789   those Node NLRI whose SPF capability TLV matches with the local BGP
> 790   SPF speaker's SPF capability TLV value.  Since Link-State NLRI always
> 791   contains the local node descriptor as described in Section 5.2, each
> 792   NLRI is uniquely originated by a single BGP SPF speaker in the BGP
> 793   SPF routing domain (the BGP node matching the NLRI's Node
> 794   Descriptors).  Instances of the same NLRI originated by multiple BGP
> 795   SPF speakers would be indicative of a configuration error or a
> 796   masquerading attack (refer to Section 9).  These selected Node NLRI
> 797   and their Link/Prefix NLRI are used to build a directed graph during
> 798   the SPF computation as described below.  The best routes for BGP
> 799   prefixes are installed in the RIB as a result of the SPF process.
>
> 801   When BGP-LS-SPF NLRI is received, all that is required is to
> 802   determine whether it is the most recent by examining the Node-ID and
> 803   sequence number as described in Section 6.1.  If the received NLRI
> 804   has changed, it is advertised to other BGP-LS-SPF peers.  If the
> 805   attributes have changed (other than the sequence number), a BGP SPF
> 806   calculation is triggered.  However, a changed NLRI MAY be advertised
> 807   immediately to other peers and prior to any SPF calculation.  Note
>
> [minor] The above is really the processing of received BGP-LS-SPF NLRIs.
> In fact, this is the flooding procedure that perhaps deserves it own
> section
> where the handling of reachable as well as unreachable is described, There
> some
> bits and pieces of information that is spread in different sections that
> pertain
>  to the flooding procedure/function - it would help if they were
> consolidated.
>
> 808   that the BGP MinRouteAdvertisementIntervalTimer and
> 809   MinASOriginationIntervalTimer [RFC4271] timers are not applicable to
> 810   the BGP-LS-SPF SAFI.  The scheduling of the SPF calculation, as
>
> [major] Actually, MRAI would be useful with some exponetial backoff
> component
> for BGP-SPF as well. It enables throttling of continuous flapping/churning
> NLRIs. Please consider MinLSArrival and MinLSInterval of OSPF.
>
> 811   described in Section 6.3, is an implementation issue.  Scheduling MAY
> 812   be dampened consistent with the SPF back-off algorithm specified in
> 813   [RFC8405].
>
> [major] We cannot leave SPF scheduling to implementation since the use of
> inconsistent values or designs in implementations aggravate microloops. Why
> not mandate behavior with existing references - either RFC8405 or something
> basic that involves specific parameters (initial wait, max wait and the
> expo
> backoff behavior) - as indicated in Sec 10.5 that implementations MUST
> provide for configuration. This is required for interoperability and smooth
> deployments.
>
> 815   The Phase 3 decision function of the Decision Process [RFC4271] is
> 816   also simplified since under normal SPF operation, a BGP SPF speaker
> 817   MUST advertise the changed NLRIs to all BGP peers with the BGP-LS-SPF
> 818   AFI/SAFI and install the changed routes in the GLOBAL-RIB.  The only
> 819   exception are unchanged NLRIs or stale NLRIs, i.e., NLRI received
> 820   with a less recent (numerically smaller) sequence number.
>
> 822 6.1.  BGP SPF NLRI Selection
>
> 824   The rules for all BGP-LS-SPF NLRIs selection for phase 1 of the BGP
> 825   decision process, section 9.1.1 [RFC4271], no longer apply.
>
> 827   1.  NLRI originated by directly connected BGP SPF peers are
> 828       preferred.  This condition can be determined by comparing the BGP
> 829       Identifiers in the received Local Node Descriptor and the BGP
> 830       OPEN message.  This rule assures that stale NLRI is updated even
> 831       if a BGP-LS router loses its sequence number state due to a cold-
> 832       start.
>
> [major] Is the above special case needed when we have the procedures in
> section
> 6.1.1? It seems somewhat contradictory?
>
> 834   2.  The NLRI with the most recent Sequence Number TLV, i.e., highest
> 835       sequence number is selected.
>
> [minor] Could the router delete all instances of the NLRI that it has
> received
> from its neighbors that have lesser sequence number than a recently
> received
> NLRI instance? If this were done when processing the received updated,
> then it
> would removes the above step and perhaps also help simplify matters?
>
> 837   3.  The NLRI received from the BGP SPF speaker with the numerically
> 838       larger BGP Identifier is preferred.
>
> 840   When a BGP SPF speaker completely loses its sequence number state,
> 841   i.e., due to a cold start, or in the unlikely possibility that 64-bit
> 842   sequence number wraps, the BGP routing domain will still converge.
> 843   This is due to the fact that BGP SPF speakers adjacent to the router
> 844   always accept self-originated NLRI from the associated speaker as
> 845   more recent (rule # 1).  When a BGP SPF speaker reestablishes a
> 846   connection with its peers, any existing sessions are taken down and
> 847   stale NLRI are replaced.  The adjacent BGP SPF speakers update their
> 848   NLRI advertisements and advertise to their neighbors until the BGP
> 849   routing domain has converged.
>
> 851   The modified SPF Decision Process performs an SPF calculation rooted
> 852   at the local BGP SPF speaker using the metrics from the Link
> 853   Attribute IGP Metric TLV (1095) and the Prefix Attribute Prefix
> 854   Metric TLV (1155) [I-D.ietf-idr-rfc7752bis].  As a result, any other
> 855   BGP attributes that would influence the BGP decision process defined
> 856   in [RFC4271] including ORIGIN, MULTI_EXIT_DISC, and LOCAL_PREF
> 857   attributes are ignored by the SPF algorithm.  The NEXT_HOP attribute
> 858   is discussed in Section 5.3.  The AS_PATH and AS4_PATH [RFC6793]
> 859   attributes are preserved and used for loop detection [RFC4271].  They
> 860   are ignored during the SPF computation for BGP-LS-SPF NRLIs.
>
> [major] I note that there are no "route types" in this specification. Does
> this imply that all prefixes are treated as equal and considered purely on
> the
> basis of the link and prefix metric values? If so, please specify the same.
>
> 862 6.1.1.  BGP Self-Originated NLRI
>
> 864   Node, Link, or Prefix NLRI with Node Descriptors matching the local
> 865   BGP SPF speaker are considered self-originated.  When self-originated
> 866   NLRI is received and it doesn't match the local node's NLRI content
> 867   (including sequence number), special processing is required.
>
> 869   *  If self-originated NLRI is received and the sequence number is
> 870      more recent (i.e., greater than the local node's sequence number
> 871      for the NLRI), the NLRI sequence number is advanced to one greater
> 872      than the received sequence number and the NLRI is readvertised to
> 873      all peers.
>
> 875   *  If self-originated NLRI is received and the sequence number is the
> 876      same as the local node's sequence number but the attributes
> 877      differ, the NLRI sequence number is advanced to one greater than
> 878      the received sequence number and the NLRI is readvertised to all
> 879      peers.
>
> 881   *  If self-originated Link or Prefix NLRI is received and the Link or
> 882      Prefix NLRI is no longer being advertised by the local node, the
> 883      NLRI is withdrawn.
>
> 885   The above actions are performed immediately when the first instance
> 886   of a newer self-originated NLRI is received.  In this case, the newer
> 887   instance is considered to be a stale instance that was advertised by
> 888   the local node prior to a restart where the NLRI state is lost.
> 889   However, if subsequent newer self-originated NLRI is received for the
> 890   same Node, Link, or Prefix NLRI, the readvertisement or withdrawal is
> 891   delayed by 5 seconds since it is likely being advertised by a
> 892   misconfigured or rogue BGP SPF speaker (refer to Section 9).
>
> [major] Suggest to introduce a configurable parameter here - this should
> normally be the same as the interval between 2 originations of an NLRI and
> there should be an initial wait, max wait and expo backoff between them.
>
> 894 6.2.  Dual Stack Support
>
> 896   The SPF-based decision process operates on Node, Link, and Prefix
> 897   NLRIs that support both IPv4 and IPv6 addresses.  Whether to run a
> 898   single SPF computation or multiple SPF computations for separate AFs
> 899   is an implementation matter.  Normally, IPv4 next-hops are calculated
> 900   for IPv4 prefixes and IPv6 next-hops are calculated for IPv6
> 901   prefixes.
>
> [major] There is the topology computation and then the prefix calculation
> that
> is done post the topology computation. The spec needs to clarify whether
> IPv4 and IPv6 topologies are to be computed separately (this requires the
> indication of AFI enablement per link) or if we are considering a single
> (congruent) topology. I assume we want the former. In that case, the use of
> addresses as link identifiers is not a clear enough indication - consider
> IPv6
> link-local only scenario. No matter how we may approach - this needs to be
> specified in detail. It cannot be an implementation matter.
>
>
> 1128 6.5.  NLRI Advertisement
>
> 1130 6.5.1.  Link/Prefix Failure Convergence
>
> 1132   A local failure prevents a link from being used in the SPF
> 1133   calculation due to the IGP bi-directional connectivity requirement.
> 1134   Consequently, local link failures SHOULD always be given priority
> 1135   over updates (e.g., withdrawing all routes learned on a session) in
> 1136   order to ensure the highest priority propagation and optimal
> 1137   convergence.
>
> [minor] Why would all the NLRIs received over a session be withdrawn from
> other peers if the session went down? This needs to happen only if there
> is no
> other instance of the same NLRI with the same sequence num received from
> another peer, right? This may not be obvious since we are not following the
> classic BGP and this is perhaps part of the flooding procedure that does
> not
> need to wait for the Route Calculation (i.e. SPF) - or rather should be
> running
> parallel or independent of the SPF.
>
> 1139   With a BGP advertisement, the link would continue to be used until
> 1140   the last copy of the BGP-LS-SPF Link NLRI is withdrawn.  In order to
> 1141   avoid this delay, the originator of the Link NLRI SHOULD advertise a
> 1142   more recent version with an increased Sequence Number TLV for the
> 1143   BGP-LS-SPF Link NLRI including the SPF Status TLV (refer to
> 1144   Section 5.2.2.2) indicating the link is down with respect to BGP
> SPF.
> 1145   The configurable LinkStatusDownAdvertise timer controls the interval
> 1146   that the BGP-LS-LINK NLRI is advertised with SPF Status indicating
> 1147   the link is down prior to withdrawal.  If BGP-LS-SPF Link NLRI has
> 1148   been advertised with the SPF Status TLV and the link becomes
> 1149   available in that period, the originator of the BGP-LS-SPF LINK NLRI
> 1150   MUST advertise a more recent version of the BGP-LS-SPF Link NLRI
> 1151   without the SPF Status TLV in the BGP-LS Link Attributes.  The
> 1152   suggested default value for the LinkStatusDownAdvertise timer is 2
> 1153   seconds.
>
> 1155   Similarly, when a prefix becomes unreachable, a more recent version
> 1156   of the BGP-LS-SPF Prefix NLRI SHOULD be advertised with the SPF
> 1157   Status TLV (refer to Section 5.2.3.1) indicating the prefix is
> 1158   unreachable in the BGP-LS Prefix Attributes and the prefix will be
> 1159   considered unreachable with respect to BGP SPF.  The configurable
> 1160   PrefixStatusDownAdvertise timer controls the interval that the BGP-
> 1161   LS-Prefix NLRI is advertised with SPF Status indicating the prefix
> is
> 1162   unreachable prior to withdrawal.  If the BGP-LS-SPF Prefix has been
> 1163   advertised with the SPF Status TLV and the prefix becomes reachable
> 1164   in that period, the originator of the BGP-LS-SPF Prefix NLRI MUST
> 1165   advertise a more recent version of the BGP-LS-SPF Prefix NLRI
> without
> 1166   the SPF Status TLV in the BGP-LS Prefix Attributes.  The suggested
> 1167   default value for the PrefixStatusDownAdvertise timer is 2 seconds.
>
> 1169 6.5.2.  Node Failure Convergence
>
> 1171   By default [RFC4271], all the NLRI advertised by a node are
> withdrawn
> 1172   when a session failure is detected.  If fast failure detection such
> 1173   as BFD is utilized, and the node is on the fastest converging path,
> 1174   the most recent versions of BGP-LS-SPF NLRI may be withdrawn.  This
> 1175   results in an older version of the NLRI received on a different path
> 1176   being used until the new versions arrive and, potentially,
> 1177   unnecessary route flaps.  For the BGP-LS-SPF SAFI, NLRI received
> from
> 1178   the failing node SHOULD NOT be implicitly withdrawn immediately to
> 1179   prevent such unnecessary route flaps.  The configurable
>
> [major] Why is this special case for Node NLRI alone? It should be the same
> for all NLRIs - do not withdraw as long as we have the same instance
> received
> from another neighbor. The flooding procedure for BGP-SPF entirely replace
> the
> propagation procedures for classic BGP, right?
>
> 1180   NLRIImplicitWithdrawalDelay timer controls the interval that NLRI
> 1181   from the failed node is retained prior to implicit withdrawal after
> a
> 1182   BGP SPF speaker has transitioned out of Established state.  This
> does
> 1183   delay convergence since the adjacent nodes detect the link failure
>
> [minor] Perhaps you meant "This does not delay convergence ..."?
>
> 1184   and advertise a more recent NLRI indicating the link is down with
> 1185   respect to BGP SPF (refer to Section 6.5.1) and the bi-directional
> 1186   connectivity check fails during the BGP SPF calculation (refer to
> 1187   Section 6.3).  The suggested default value for the
> 1188   NLRIImplicitWithdrawalDelay timer is 2 seconds.
>
> 1190 7.  Error Handling
>
> 1192   This section describes the Error Handling actions, as described in
> 1193   [RFC7606], that are specific to SAFI BGP-LS-SPF BGP Update message
> 1194   processing.
>
> 1196 7.1.  Processing of BGP-LS-SPF TLVs
>
> 1198   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1199   Node NLRI SPF Status TLV in the BGP-LS Attribute
> 1200   [I-D.ietf-idr-rfc7752bis], the corresponding Node NLRI is considered
> 1201   as malformed and MUST be handled as 'Treat-as-withdraw'.  An
> 1202   implementation SHOULD log an error (subject to rate-limiting) for
> 1203   further analysis.
>
> 1205   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1206   Link NLRI SPF Status TLV in the BGP-LS Attribute
> 1207   [I-D.ietf-idr-rfc7752bis], the corresponding Link NLRI is considered
> 1208   as malformed and MUST be handled as 'Treat-as-withdraw'.  An
> 1209   implementation SHOULD log an error (subject to rate-limiting) for
> 1210   further analysis.
>
> 1212   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1213   Prefix NLRI SPF Status TLV in the BGP-LS Attribute
> 1214   [I-D.ietf-idr-rfc7752bis], the corresponding Prefix NLRI is
> 1215   considered as malformed and MUST be handled as 'Treat-as-withdraw'.
> 1216   An implementation SHOULD log an error (subject to rate-limiting) for
> 1217   further analysis.
>
> 1219   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1220   SPF Capability TLV in the Node NLRI BGP-LS Attribute
> 1221   [I-D.ietf-idr-rfc7752bis], the corresponding Node NLRI is considered
> 1222   as malformed and MUST be handled as 'Treat-as-withdraw'.  An
> 1223   implementation SHOULD log an error (subject to rate-limiting) for
> 1224   further analysis.
>
> 1226   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1227   IPv4 Prefix-Length TLV in the Link NLRI BGP-LS Attribute
> 1228   [I-D.ietf-idr-rfc7752bis], the corresponding Link NLRI is considered
> 1229   as malformed and MUST be handled as 'Treat-as-withdraw'.  An
> 1230   implementation SHOULD log an error (subject to rate-limiting) for
> 1231   further analysis.
>
> 1233   When a BGP SPF speaker receives a BGP Update containing a malformed
> 1234   IPv6 Prefix-Length TLV in the Link NLRI BGP-LS Attribute
> 1235   [I-D.ietf-idr-rfc7752bis], the corresponding Link NLRI is considered
> 1236   as malformed and MUST be handled as 'Treat-as-withdraw'.  An
> 1237   implementation SHOULD log an error (subject to rate-limiting) for
> 1238   further analysis.
>
> 1240   When a BGP SPF speaker receives a BGP Update containing any
> malformed
> 1241   BGP-LS Attribute TE and IGP Metric TLV, the corresponding NLRI is
> 1242   considered as a malformed and MUST be handled as 'Treat-as-withdraw'
> 1243   [RFC7606].  An implementation SHOULD log an error (subject to rate-
> 1244   limiting) for further analysis.
>
> [minor] There is a lot of repetition above. Basically, missing mandatory
> TLVs
> result in the NLRI being considered malformed and the same handling. If
> what
> is mandatory is covered in section 5.2 then this text can be greatly
> condensed.
>
> 1246   The BGP-LS Attribute consists of Node attribute TLVs, Link attribute
> 1247   TLVs, and the Prefix attribute TLVs.  Node attribute TLVs and their
> 1248   error handling rules are either defined in [I-D.ietf-idr-rfc7752bis]
> 1249   or derived from [RFC5305] and [RFC6119].  If a BGP SPF speaker
> 1250   receives a BGP-LS Attribute which is considered malformed based on
> 1251   these error handling rules, then it MUST consider the received NLRI
> 1252   as a malformed and the receiving BGP SPF speaker MUST handle such
> 1253   malformed NLRI as 'Treat-as-withdraw' [RFC7606].
>
> 1255   Node Descriptor TLVs and their error handling rules are either
> 1256   defined in section 5.2.1 of [I-D.ietf-idr-rfc7752bis].  Node
> 1257   Attribute TLVs and their error handling rules are either defined in
> 1258   [I-D.ietf-idr-rfc7752bis] or derived from [RFC5305] and [RFC6119].
>
> 1260   Link Descriptor TLVs and their error handling rules are either
> 1261   defined in section 5.2.2 of [I-D.ietf-idr-rfc7752bis].  Link
> 1262   Attribute TLVs and their error handling rules are either defined in
> 1263   [I-D.ietf-idr-rfc7752bis] or derived from [RFC5305] and [RFC6119].
>
> 1265   Prefix Descriptor TLVs and their error handling rules are either
> 1266   defined in section 5.2.3 of [I-D.ietf-idr-rfc7752bis].  Prefix
> 1267   Attribute TLVs and their error handling rules are either defined in
> 1268   [I-D.ietf-idr-rfc7752bis] or derived from [RFC5130] and [RFC2328].
>
> [minor] Same as previous comment.
>
> 1270   If a BGP SPF speaker receives NLRI with a Node Descriptor TLV, Link
> 1271   Descriptor TLV, or Prefix Descriptor TLV that is considered
> malformed
> 1272   based on error handling rules defined in the above references, then
> 1273   it MUST consider the received NLRI as a malformed and the receiving
> 1274   BGP SPF speaker MUST handle such malformed NLRI as 'Treat-as-
> 1275   withdraw' [RFC7606].
>
> 1277   When a BGP SPF speaker receives a BGP Update that does not contain
> 1278   any BGP-LS Attribute, then a BGP SPF speaker MUST consider the
> 1279   corresponding NLRI as a malformed and MUST handle it as 'Treat-as-
> 1280   withdraw' [RFC7606].  An implementation SHOULD log and error
> (subject
> 1281   to rate-limiting) for further analysis.
>
> 1283 7.2.  Processing of BGP-LS-SPF NLRIs
>
> 1285   A BGP-LS-SPF Speaker MUST perform the syntactic validation checks of
> 1286   the BGP-LS-SPF NLRI listed in Section 8.2.2 of
> 1287   [I-D.ietf-idr-rfc7752bis] to determine if it is malformed.
>
> 1289   In common deployment scenarios, the unicast routes installed during
> 1290   BGP-LS-SPF AFI/SAFI SPF computation serve as the underlay for other
> 1291   BGP AFI/SAFIs.  To avoid errors encountered in other AFI/SAFIs from
> 1292   impacting the BGP-LS-SPF AFI/SAFI or vice-versa, isolation
> mechanisms
> 1293   such as separate BGP instances or separate BGP sessions (e.g., using
> 1294   different addresses for peering) for BGP SPF Link-State information
> 1295   distribution SHOULD be used.
>
> [minor] The above paragraph seems more appropriate as an operational
> consideration.
>
> 1297 7.3.  Processing of BGP-LS Attribute
>
> 1299   A BGP-LS-SPF Speaker MUST perform the syntactic validation checks of
> 1300   the BGP-LS Attribute listed in Section 8.2.2 of
> 1301   [I-D.ietf-idr-rfc7752bis] to determine if it is malformed.
>
> 1303   An implementation SHOULD log an error for further analysis for
> 1304   problems detected during syntax validation.
>
> [minor] The text in 7.2 and 7.3 would ideally be covered in the procedures
> for
> handling reception of updates.
>
> 1306 7.4.  BGP-LS-SPF Link State NLRI Database Synchronization
>
> [major] There is no discussion of the need for the initial LSDB sync when a
> peering is brought up. The EOR is termed as optional while it should be
> mandatory. The EOR should be sent after the router has sent over all the
> NLRIs
> in its LSDB to its neighbor. If there are multiple sessions, then given we
> are
> using TCP, the LSDB can be sent over any of the sessions (without
> duplication). If there is an existing session between the two peers and a
> new
> (parallel) one comes up, an EOR can be sent immediately. Unless a router
> has
> received an EOR from its peer, it should not advertise the link NLRI
> corresponding to any of its links to that peer. This dependency on holding
> back the Link NLRI only applies for directly connected routers and not
> peerings to remote controllers/RR. Once we have EOR for any session between
> two routers, there is no dependency on the Link NLRI for subsequent (say
> parallel) link NLRIs. Basically, the EOR can be used to ensure at least the
> initial LSDB sync between over a peering - then we rely on TCP to keep
> things
> in sync.
>
> 1308   While uncommon, there may be situations where the LSNDBs of two BGP-
> 1309   LS-SPF speakers lose synchronization.  In these situations, the BGP
> 1310   session MUST be reset.  The mechanisms to detect loss of
> 1311   synchronization are beyond the scope of this document.
>
> [major] How does one determine that the synchronization is lost in the
> first
> place?
>
> [minor] Could route refresh help or play a role in any of these procedures?
>
> 1313 8.  IANA Considerations
>
> 1315 8.1.  BGP-LS-SPF Allocation in SAFI Parameters Registry
>
> 1317   IANA has assigned value 80 for BGP-LS-SPF from the First Come First
> 1318   Served range in the "Subsequent Address Family Identifiers (SAFI)
> 1319   Parameters" registry.  IANA is requested to update the registration
> 1320   to reference only to this document.
>
> 1322 8.2.  BGP-LS-SPF Assignments to BGP-LS NLRI and Attribute TLV Registry
>
> 1324   IANA has assigned five TLVs for BGP-LS-SPF NLRI in the "BGP-LS NLRI
> 1325   and Attribute TLV" registry.  These TLV types include the SPF
> 1326   capability TLV, Sequence Number TLV, IPv4 Link Prefix-Length TLV,
> 1327   IPv6 Link Prefix-Length TLV, and SPF Status TLV.
>
> 1329       +==========+=============+=================================+
> 1330       | TLV Code | Description | Reference                       |
> 1331       | Point    |             |                                 |
> 1332       +==========+=============+=================================+
> 1333       | 1180     | SPF         | RFCXXXX ([this document]),      |
> 1334       |          | Capability  | Section 5.2.1.1                 |
> 1335       +----------+-------------+---------------------------------+
> 1336       | 1184     | SPF Status  | Section 5.2.1.2, RFCXXXX ([this |
> 1337       |          |             | document]), Section 5.2.2.2 and |
> 1338       |          |             | Section 5.2.3.1                 |
>
> [major] A single SPF Status TLV (1184) is defined here but it is called
> different names depending on the NLRI type along with the BGP-LS Attribute.
> In fact, it would be really great if many different types of NLRIs could be
> packed together in a single update with the BGP-LS Attribute simply
> containing
> the SPF status "unreachable" and nothing else to cause rapid/efficient
> propagation of "down" events. In any case, it just seems semantically
> wrong
> to overload in this manner. I would suggest that we still retain a single
> SPF
>  Status TLV and which will indicate the status for the object (i.e. NLRI)
> which it is associated with. In addition, harmonize the status values in
> sec 8.3/4/5 into a single registry and simply clarify in section 5 which
> status values are relevant for which object and others can be ignored.
>
> 1339       +----------+-------------+---------------------------------+
> 1340       | 1182     | IPv4 Link   | RFCXXXX ([this document]),      |
> 1341       |          | Prefix      | Section 5.2.2.1                 |
> 1342       |          | Length      |                                 |
> 1343       +----------+-------------+---------------------------------+
> 1344       | 1183     | IPv6 Link   | RFCXXXX ([this document]),      |
> 1345       |          | Prefix      | Section 5.2.2.1                 |
> 1346       |          | Length      |                                 |
> 1347       +----------+-------------+---------------------------------+
> 1348       | 1181     | Sequence    | RFCXXXX ([this document]),      |
> 1349       |          | Number      | Section 5.2.4                   |
> 1350       +----------+-------------+---------------------------------+
>
> 1352                       Table 1: NLRI Attribute TLVs
>
> 1354 8.3.  BGP-LS-SPF Node NLRI Attribute SPF Status TLV Status Registry
>
> 1356   IANA is requested to create the "BGP-LS-SPF Node NLRI Attribute SPF
> 1357   Status TLV Status" Registry for status values in a new BGP SPF
> group.
> 1358   Initial values for this registry are provided below.  Future
> 1359   assignments are to be made using the IETF Review registration policy
> 1360   [RFC8126].
>
> 1362           +========+==========================================+
> 1363           | Values | Description                              |
> 1364           +========+==========================================+
> 1365           | 0      | Reserved                                 |
> 1366           +--------+------------------------------------------+
> 1367           | 1      | Node unreachable with respect to BGP SPF |
> 1368           +--------+------------------------------------------+
> 1369           | 2      | Node does not support transit traffic    |
> 1370           |        | with respect to BGP SPF                  |
> 1371           +--------+------------------------------------------+
> 1372           | 3-254  | Unassigned                               |
> 1373           +--------+------------------------------------------+
> 1374           | 255    | Reserved                                 |
> 1375           +--------+------------------------------------------+
>
> 1377                Table 2: BGP-LS-SPF Node NLRI Attribute SPF
> 1378                   Status TLV Status Registry Assignments
>
> 1380 8.4.  BGP-LS-SPF Link NLRI Attribute SPF Status TLV Status Registry
>
> 1382   IANA is requested to create the "BGP-LS-SPF Link NLRI Attribute SPF
> 1383   Status TLV Status" Registry for status values in a new BGP SPF
> group.
> 1384   Initial values for this registry are provided below.  Future
> 1385   assignments are to be made using the IETF Review registration policy
> 1386   [RFC8126].
>
> 1388           +=======+==========================================+
> 1389           | Value | Description                              |
> 1390           +=======+==========================================+
> 1391           | 0     | Reserved                                 |
> 1392           +-------+------------------------------------------+
> 1393           | 1     | Link unreachable with respect to BGP SPF |
> 1394           +-------+------------------------------------------+
> 1395           | 3-254 | Unassigned                               |
> 1396           +-------+------------------------------------------+
> 1397           | 255   | Reserved                                 |
> 1398           +-------+------------------------------------------+
>
> 1400               Table 3: BGP-LS-SPF Link NLRI Attribute SPF
> 1401                  Status TLV Status Registry Assignments
>
> 1403 8.5.  BGP-LS-SPF Prefix NLRI Attribute SPF Status TLV Status Registry
>
> 1405   IANA is requested to create the "BGP-LS-SPF Prefix NLRI Attribute
> SPF
> 1406   Status TLV Status" Registry for status values in a new BGP SPF
> group.
> 1407   Initial values for this registry are provided below.  Future
> 1408   assignments are to be made using the IETF Review registration policy
> 1409   [RFC8126].
>
> 1411          +=======+============================================+
> 1412          | Value | Description                                |
> 1413          +=======+============================================+
> 1414          | 0     | Reserved                                   |
> 1415          +-------+--------------------------------------------+
> 1416          | 1     | Prefix unreachable with respect to BGP SPF |
> 1417          +-------+--------------------------------------------+
> 1418          | 3-254 | Unassigned                                 |
> 1419          +-------+--------------------------------------------+
> 1420          | 255   | Reserved                                   |
> 1421          +-------+--------------------------------------------+
>
> 1423              Table 4: BGP-LS-SPF Prefix NLRI Attribute SPF
> 1424                  Status TLV Status Registry Assignments
>
> 1467 10.  Management Considerations
>
> 1469   This section includes unique management considerations for the BGP-
> 1470   LS-SPF address family.
>
> 1472 10.1.  Configuration
>
> 1474   All routers in BGP SPF Routing Domain are under a single
> 1475   administrative domain allowing for consistent configuration.
>
> [major] AS numbering is not discussed.
>
> 1507 10.4.  Adjacency End-of-RIB (EOR) Marker Requirement
>
> 1509   Depending on the peering model, topology, and convergence
> 1510   requirements, an End-of-RIB (EoR) Marker [RFC4724] for the
> BGP-LS-SPF
> 1511   SAFI MAY be required from the peer prior to advertising a BGP-LS
> Link
> 1512   NLRI for the peer.  If configuration is supported, this SHOULD be
> 1513   configurable at the BGP SPF instance level and SHOULD be configured
> 1514   consistently throughout the BGP SPF routing domain.
>
> [major] I think the EOR is required. It is about end-of-sync over a peering
> rather than each adjacency/link. This ensures that a router coming up
> after say a
> reboot does not start attracting transit traffic until it has recovered its
> LSDB. For peering with remote RR/controller, it is not possible to have
> this
> dependency check between Link NLRI update and EOR - this should be
> captured as
> a caveat/limitation of that peering model in section 4.3
>
>
> 1516 10.5.  backoff-config
>
> 1518   In addition to configuration of the BGP-LS-SPF address family,
> 1519   implementations SHOULD support the "Shortest Path First (SPF) Back-
> 1520   Off Delay Algorithm for Link-State IGPs" [RFC8405].  If supported,
> 1521   configuration of the INITIAL_SPF_DELAY, SHORT_SPF_DELAY,
> 1522   LONG_SPF_DELAY, TIME_TO_LEARN, and HOLDDOWN_INTERVAL MUST be
> 1523   supported [RFC8405].  Section 6 of [RFC8405] recommends consistent
> 1524   configuration of these values throughout the IGP routing domain and
> 1525   this also applies to the BGP SPF Routing Domain.
>
> [major] As shared in previous comments, these should be included as
> architectural constructs in the spec formally with a default value
> specified
> and which MUST be modifiable via config. Also, the usual guidance for
> consistency of values across the domain.
>
>
> 1527 10.6.  Operational Data
>
> [minor] The document is missing operational consideration. Perhaps check
> what
> could be leveraged from rfc7752bis and there is also some operational
> aspects
> that are there in other sections.
>
> 1529   In order to troubleshoot SPF issues, implementations SHOULD support
> 1530   an SPF log including entries for previous SPF computations.  Each
> SPF
> 1531   log entry would include the BGP-LS-SPF NLRI SPF triggering the SPF,
> 1532   SPF scheduled time, SPF start time, SPF end time, and SPF type if
> 1533   different types of SPF are supported.  Since the size of the log is
> 1534   finite, implementations SHOULD also maintain counters for the total
> 1535   number of SPF computations and the total number of SPF triggering
> 1536   events.  Additionally, to troubleshoot SPF scheduling and back-off
> 1537   [RFC8405], the current SPF back-off state, remaining time-to-learn,
> 1538   remaining hold-down interval, last trigger event time, last SPF
> time,
> 1539   and next SPF time should be available.
>
> [minor] Any guidance for mechanisms for checking and verifying that the
> flooding procedure is working well? Oper commands for verifying LSDB and
> what
> NLRIs are received from which peers - normally we would expect to receive
> and
> retain a copy of the same NLRI instance from all peers. If this is not
> happening, then perhaps it indicates an issue with the flooding?
>
> [EOR - end of review]
>
>