Recompute layer-2,layer-3,vxlan topologies in iBDP #3835
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewed 7 of 11 files at r1.
Reviewable status: 7 of 11 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, @haverma, and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/TopologyContext.java, line 29 at r1 (raw file):
private @Nonnull BgpTopology _bgpTopology; private @Nonnull Set<Edge> _edgeBlacklist;
This gives me pause.
- Why should DP be aware of any blacklists? This feels like a code smell.
- Did we not agree do deprecate edge blacklists?
projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 735 at r1 (raw file):
_storage.storeBgpTopology(topologies.getBgpTopology(), networkSnapshot); _storage.storeEigrpTopology(topologies.getEigrpTopology(), networkSnapshot); _storage.storeLayer2Topology(topologies.getLayer2Topology().orElse(null), networkSnapshot);
how is this not going to throw? storage expects a non-null topology
projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Topology.java, line 15 at r1 (raw file):
import javax.annotation.ParametersAreNonnullByDefault; /** Represents physical (logical) wiring between physical (logical) interfaces. */
what is physical (logical)? physical or logical? both at the same time?
Codecov Report
@@ Coverage Diff @@
## master #3835 +/- ##
============================================
- Coverage 74.16% 74.04% -0.12%
+ Complexity 24336 24199 -137
============================================
Files 2039 2039
Lines 98762 98246 -516
Branches 11871 11779 -92
============================================
- Hits 73248 72751 -497
+ Misses 20228 20212 -16
+ Partials 5286 5283 -3
|
There was a problem hiding this comment.
Reviewable status: 4 of 18 files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, @haverma, and @progwriter)
projects/batfish/src/main/java/org/batfish/main/Batfish.java, line 735 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
how is this not going to throw? storage expects a non-null topology
fixed
projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Topology.java, line 15 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
what is
physical (logical)? physical or logical? both at the same time?
better?
There was a problem hiding this comment.
Reviewed 3 of 11 files at r1.
Reviewable status: 7 of 18 files reviewed, 7 unresolved discussions (waiting on @arifogel, @corinaminer, @haverma, and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 130 at r1 (raw file):
// Update topologies TopologyContext newTopologyContext = currentTopologyContext;
An optional suggestion:
I think creating a copy of a reference (TopologyContext newTopologyContext = currentTopologyContext) and then overwriting the reference (with a new TopologyContext object) in each of the subsequent lines in order to carryover two values (vxlanTopology and layer2Topology) is a unnecessary and confusing. We don't need the intermediate instances of TopologyContexts created here and it makes it a bit difficult to understand. I think something like the following would be easier to understand:
VxlanTopology vxlanTopology =
prunedVxlanTopology(
initialTopologyContext.getVxlanTopology(),
configurations,
new TracerouteEngineImpl(
partialDataplane, currentTopologyContext.getLayer3Topology()));
Optional<Layer2Topology> layer2Topology =
currentTopologyContext
.getLayer1LogicalTopology()
.map(
layer1Topology ->
computeLayer2Topology(layer1Topology, vxlanTopology, configurations));
TopologyContext newTopologyContext =
currentTopologyContext
.toBuilder()
.setLayer3Topology(
computeLayer3Topology(
computeRawLayer3Topology(
currentTopologyContext.getRawLayer1PhysicalTopology(),
layer2Topology,
configurations),
currentTopologyContext.getEdgeBlacklist(),
currentTopologyContext.getNodeBlacklist(),
currentTopologyContext.getInterfaceBlacklist(),
configurations))
.setVxlanTopology(vxlanTopology)
.setLayer2Topology(layer2Topology)
.build();
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 138 at r1 (raw file):
.setVxlanTopology( prunedVxlanTopology( initialTopologyContext.getVxlanTopology(),
why do we not run the pruner on the updated vxlanTopology of currentTopologyContext instead of always using the initialTopologyContext? a design choice ?
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalDataPlanePlugin.java, line 52 at r1 (raw file):
.setOspfTopology(_batfish.getTopologyProvider().getInitialOspfTopology(networkSnapshot)) .setRawLayer1PhysicalTopology( _batfish.getTopologyProvider().getRawLayer1PhysicalTopology(networkSnapshot))
_batfish.getTopologyProvider() can be stored in a var and reused
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/FixedPointTopologyTest.java, line 53 at r1 (raw file):
private static final String E1_NAME = "E1";
new line intentional?
There was a problem hiding this comment.
Reviewed 1 of 11 files at r1, 11 of 11 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arifogel, @corinaminer, and @progwriter)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @arifogel, @corinaminer, @haverma, and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 138 at r1 (raw file):
Previously, haverma (Harsh Verma) wrote…
why do we not run the pruner on the updated
vxlanTopologyofcurrentTopologyContextinstead of always using theinitialTopologyContext? a design choice ?
The initial VXLAN topology is actually the maximum possible topology assuming full reachability between endpoints. Since any of these tunnels can come up or down each iteration, we need to prune from the initial. If we instead pruned from the most recent one, then the topology would shrink monotonically. That would be incorrect in general.
There was a problem hiding this comment.
Reviewable status: 15 of 18 files reviewed, 7 unresolved discussions (waiting on @arifogel, @corinaminer, @haverma, and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 130 at r1 (raw file):
Previously, haverma (Harsh Verma) wrote…
An optional suggestion:
I think creating a copy of a reference (TopologyContext newTopologyContext = currentTopologyContext) and then overwriting the reference (with a new TopologyContext object) in each of the subsequent lines in order to carryover two values (vxlanTopology and layer2Topology) is a unnecessary and confusing. We don't need the intermediate instances of TopologyContexts created here and it makes it a bit difficult to understand. I think something like the following would be easier to understand:VxlanTopology vxlanTopology = prunedVxlanTopology( initialTopologyContext.getVxlanTopology(), configurations, new TracerouteEngineImpl( partialDataplane, currentTopologyContext.getLayer3Topology())); Optional<Layer2Topology> layer2Topology = currentTopologyContext .getLayer1LogicalTopology() .map( layer1Topology -> computeLayer2Topology(layer1Topology, vxlanTopology, configurations)); TopologyContext newTopologyContext = currentTopologyContext .toBuilder() .setLayer3Topology( computeLayer3Topology( computeRawLayer3Topology( currentTopologyContext.getRawLayer1PhysicalTopology(), layer2Topology, configurations), currentTopologyContext.getEdgeBlacklist(), currentTopologyContext.getNodeBlacklist(), currentTopologyContext.getInterfaceBlacklist(), configurations)) .setVxlanTopology(vxlanTopology) .setLayer2Topology(layer2Topology) .build();
Of course! I don't know what I was thinking.
Fixed.
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalDataPlanePlugin.java, line 52 at r1 (raw file):
Previously, haverma (Harsh Verma) wrote…
_batfish.getTopologyProvider()can be stored in a var and reused
done
projects/batfish/src/test/java/org/batfish/dataplane/ibdp/FixedPointTopologyTest.java, line 53 at r1 (raw file):
Previously, haverma (Harsh Verma) wrote…
new line intentional?
nope. fixed.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @arifogel, @corinaminer, and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/IncrementalBdpEngine.java, line 138 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
The initial VXLAN topology is actually the maximum possible topology assuming full reachability between endpoints. Since any of these tunnels can come up or down each iteration, we need to prune from the initial. If we instead pruned from the most recent one, then the topology would shrink monotonically. That would be incorrect in general.
got it. Thanks !
There was a problem hiding this comment.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @corinaminer and @progwriter)
projects/batfish/src/main/java/org/batfish/dataplane/ibdp/TopologyContext.java, line 29 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
This gives me pause.
- Why should DP be aware of any blacklists? This feels like a code smell.
- Did we not agree do deprecate edge blacklists?
Deferring to follow-on PR.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved (waiting on @corinaminer)
projects/batfish-common-protocol/src/main/java/org/batfish/common/topology/Layer1Topology.java, line 15 at r1 (raw file):
Previously, arifogel (Ari Fogel) wrote…
better?
yep
No description provided.