BgpTopologyUtils: only check valid candidates for BGP peers

[dhalperi]

With a little pre-work and moving the Host/VRF check up, we can
reduce from quadratic to linear complexity.

[batfish-bot]

This change is 

[dhalperi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @corinaminer, @dhalperi, and @progwriter)

a discussion (no related file):
Haven't updated any tests yet. Just an fyi.

---

[codecov]

Codecov Report

Merging #6093 into master will decrease coverage by 0.00%.
The diff coverage is 91.30%.

@@             Coverage Diff              @@
##             master    #6093      +/-   ##
============================================
- Coverage     72.69%   72.68%   -0.01%     
+ Complexity    34659    34656       -3     
============================================
  Files          2815     2815              
  Lines        141422   141434      +12     
  Branches      16969    16972       +3     
============================================
- Hits         102813   102808       -5     
- Misses        30440    30452      +12     
- Partials       8169     8174       +5     

Impacted Files	Coverage Δ	Complexity Δ
...va/org/batfish/datamodel/bgp/BgpTopologyUtils.java	80.82% <91.30%> (+0.11%)	80.00 <7.00> (+3.00)
...rg/batfish/identifiers/StorageBasedIdResolver.java	88.88% <0.00%> (-4.45%)	27.00% <0.00%> (ø%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java	87.09% <0.00%> (-3.23%)	12.00% <0.00%> (-1.00%)
...col/src/main/java/org/batfish/role/InferRoles.java	89.09% <0.00%> (-1.82%)	49.00% <0.00%> (-2.00%)
...src/main/java/org/batfish/coordinator/PoolMgr.java	62.92% <0.00%> (-1.13%)	15.00% <0.00%> (-1.00%)
...ain/java/org/batfish/storage/FileBasedStorage.java	86.29% <0.00%> (-0.92%)	257.00% <0.00%> (ø%)

[corinaminer]

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dhalperi and @progwriter)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/bgp/BgpTopologyUtils.java, line 161 at r1 (raw file):

      for (BgpPeerConfigId peer : graph.nodes()) {
        if (peer.getType() == BgpPeerConfigType.UNNUMBERED) {
          continue;

Please add comment explaining why unnumbered peers are excluded from receivers

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/bgp/BgpTopologyUtils.java, line 241 at r1 (raw file):

                        && (!checkReachability
                            || isReachableBgpNeighbor(
                                neighborId, candidateId, neighbor, tracerouteEngine)))

This means the traceroute will still be run for every peer in any vrf that owns the initiator's remote IP. We could do better: canInitiateBgpSession() could be tweaked to return a Multimap<String, String> representing all VRFs that the initiator's peering request can reach successfully. Then we could use that map in place of possibleVrfs, and we'd no longer need to check reachability per candidate.

But in the great majority of cases there's probably only one VRF that owns a given IP, and only one compatible peer in that VRF, so your approach wouldn't usually end up running traceroute a lot more.

[dhalperi]

Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @corinaminer, @dhalperi, and @progwriter)

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/bgp/BgpTopologyUtils.java, line 161 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

Please add comment explaining why unnumbered peers are excluded from receivers

Preserved the comment from where this logic originally was.

---

projects/batfish-common-protocol/src/main/java/org/batfish/datamodel/bgp/BgpTopologyUtils.java, line 241 at r1 (raw file):

Previously, corinaminer (Corina Miner) wrote…

This means the traceroute will still be run for every peer in any vrf that owns the initiator's remote IP. We could do better: canInitiateBgpSession() could be tweaked to return a Multimap<String, String> representing all VRFs that the initiator's peering request can reach successfully. Then we could use that map in place of possibleVrfs, and we'd no longer need to check reachability per candidate.

But in the great majority of cases there's probably only one VRF that owns a given IP, and only one compatible peer in that VRF, so your approach wouldn't usually end up running traceroute a lot more.

This PR is not a semantic change, only a trivial reordering of checks to do less work.

Refactoring is out of scope - when we take that on, we should probably reimplement as described in internal bug 2451.

[dhalperi]

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @corinaminer and @progwriter)

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

Haven't updated any tests yet. Just an fyi.

Seems like none are needed - the larger scale refactoring work would, but this just moves checks earlier.

---

[corinaminer]

Reviewed 1 of 1 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @progwriter)