Use final L3 topology for OSPF session compatibility #6561
Conversation
|
This change is |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @dhalperi, @progwriter, and @ratulm)
a discussion (no related file):
Hmm, usually the "compatibility" version is Pre-Dataplane and the "status" version is Post-Dataplane.
The Post-Dataplane version can indeed have edges that don't appear pre-dataplane.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dhalperi, @progwriter, and @ratulm)
projects/question/src/main/java/org/batfish/question/ospfsession/OspfSessionCompatibilityAnswerer.java, line 64 at r1 (raw file):
_batfish.getSnapshot()
definitely don't want this change -- the snapshot passed in is the one to use.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @dhalperi, @progwriter, and @ratulm)
a discussion (no related file):
Previously, dhalperi (Dan Halperin) wrote…
Hmm, usually the "compatibility" version is Pre-Dataplane and the "status" version is Post-Dataplane.
The Post-Dataplane version can indeed have edges that don't appear pre-dataplane.
I see. There is no 'status' version for OSPF. Let's close this PR if that is the invariant we want to maintain. The PR was inspired by user confusion in which there edges in ospfEdges question that didn't appear in ospfSessionCompatibility question. If we don't make this change, we should update the documentation for ospfSessionCompatibility.
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 2 unresolved discussions (waiting on @progwriter and @ratulm)
a discussion (no related file):
Previously, ratulm wrote…
I see. There is no 'status' version for OSPF. Let's close this PR if that is the invariant we want to maintain. The PR was inspired by user confusion in which there edges in ospfEdges question that didn't appear in ospfSessionCompatibility question. If we don't make this change, we should update the documentation for ospfSessionCompatibility.
We could rename this one to status, use the final topology, and then fix it up, too. Compatibility vs Status may make less sense for OSPF than BGP?
Codecov Report
@@ Coverage Diff @@
## master #6561 +/- ##
============================================
- Coverage 73.39% 73.38% -0.01%
+ Complexity 35804 35797 -7
============================================
Files 2840 2840
Lines 144549 144549
Branches 17491 17491
============================================
- Hits 106092 106080 -12
- Misses 30044 30051 +7
- Partials 8413 8418 +5
|
|
There is value in having a compatibility check for OSPF. 2 routers could be physically connected, but the MTU, area number, md5 key, network type (p2p, broadcast, nbma), etc... could be wrong. Also, in some scenarios like nbma you configure the remote peer ip explicitly, instead of relying on multicast to discover the remote end. |
|
Circling back here, it looks like BGP compatibility considers all possible pairs (modulo some gaps around NAT of course) and judges that based on config settings. BGP status will then help decide if connections happens in the data plane. Taking that view, I think we should leave this question as OspfSessionCompatibility since it considers configuration-level compatibility and simply expand the set of pairs considered. An OspfStatus question will then judge actual sessions (which may still get blocked via L2/L3 ACLs). @dhalperi - makes sense? (I will fix the which-snapshot-to-use thing). |
There was a problem hiding this comment.
Reviewable status: 0 of 1 files reviewed, 3 unresolved discussions (waiting on @progwriter and @ratulm)
a discussion (no related file):
Planned change makes sense.
- mark the question are requiring dataplane
- update templates/tests
- update docs, if needed, in pybatfish
There was a problem hiding this comment.
Reviewed 4 of 4 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @progwriter)
Without this change, any OSPF edges that get established later (e.g., tunnel-based edges) were not being considered.