Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Incomplete Layer-1 Topology Handling #3396

Merged
merged 3 commits into from
Mar 15, 2019

Conversation

arifogel
Copy link
Member

Robustness in presence of:

  • One-sided layer-1 edges
  • Inconsistent availability of layer-1 information
  • Incorrect/unusable layer-1 information

Robustness in presence of:
- One-sided layer-1 edges
- Inconsistent availability of layer-1 information
- Incorrect/unusable layer-1 information
@batfish-bot
Copy link

This change is Reviewable

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #3396 into master will increase coverage by 0.03%.
The diff coverage is 95.83%.

@@             Coverage Diff              @@
##             master    #3396      +/-   ##
============================================
+ Coverage     72.96%   72.99%   +0.03%     
- Complexity    23485    23561      +76     
============================================
  Files          2040     2045       +5     
  Lines         98466    98767     +301     
  Branches      11777    11812      +35     
============================================
+ Hits          71844    72094     +250     
- Misses        21386    21417      +31     
- Partials       5236     5256      +20
Impacted Files Coverage Δ Complexity Δ
...n/java/org/batfish/common/topology/Layer1Edge.java 65.38% <100%> (+1.38%) 12 <1> (+1) ⬆️
...ava/org/batfish/topology/TopologyProviderImpl.java 70.79% <100%> (+0.52%) 26 <1> (ø) ⬇️
...java/org/batfish/common/topology/TopologyUtil.java 90.61% <94.73%> (+1.01%) 133 <12> (+7) ⬆️
...g/batfish/representation/f5_bigip/BgpNeighbor.java 60% <0%> (-36%) 8% <0%> (-5%)
...c/main/java/org/batfish/dataplane/rib/RibTree.java 78.72% <0%> (-10.64%) 23% <0%> (-1%)
..._bigip_imish/F5BigipImishConfigurationBuilder.java 90.47% <0%> (-9.53%) 50% <0%> (+48%)
...java/org/batfish/datamodel/OspfInterAreaRoute.java 59.25% <0%> (-7.41%) 8% <0%> (-1%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0%> (-6.67%) 5% <0%> (-1%)
...org/batfish/datamodel/flow/BidirectionalTrace.java 81.81% <0%> (-2.28%) 14% <0%> (-1%)
...batfish/representation/f5_bigip/RouteMapEntry.java 77.77% <0%> (-2.23%) 8% <0%> (+1%)
... and 17 more

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 5 of 5 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @anothermattbrown and @dhalperi)


projects/batfish-common-protocol/src/test/java/org/batfish/common/topology/TopologyUtilTest.java, line 805 at r1 (raw file):

     * Incorrect/Unusable Layer-1 information
     * Expected L1: N1 <=> N2
     * Provided L1: N1 => N2, N2 => NCorrupt

nit: seems to me that you really mean "missing" and not corrupt

@progwriter
Copy link
Contributor


projects/batfish-common-protocol/src/test/java/org/batfish/common/topology/TopologyUtilTest.java, line 838 at r1 (raw file):

        layer1PhysicalTopology.getGraph().edges(),
        containsInAnyOrder(new Layer1Edge(n1, n2), new Layer1Edge(n2, n1)));
  }

Also add a test case that utilizes getIspConfigurations and make sure links to ISPs are established

Copy link
Member Author

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, 1 unresolved discussion (waiting on @anothermattbrown, @arifogel, @dhalperi, and @progwriter)


projects/batfish-common-protocol/src/test/java/org/batfish/common/topology/TopologyUtilTest.java, line 805 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

nit: seems to me that you really mean "missing" and not corrupt

The use case was bad copypaste.
Is this better?

Copy link
Member Author

@arifogel arifogel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 4 of 5 files reviewed, all discussions resolved (waiting on @anothermattbrown, @dhalperi, and @progwriter)


projects/batfish-common-protocol/src/test/java/org/batfish/common/topology/TopologyUtilTest.java, line 838 at r1 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

Also add a test case that utilizes getIspConfigurations and make sure links to ISPs are established

done

Copy link
Contributor

@progwriter progwriter left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @anothermattbrown and @dhalperi)

@arifogel arifogel merged commit 96acd38 into batfish:master Mar 15, 2019
@arifogel arifogel deleted the ari-layer-1-incomplete branch March 15, 2019 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants