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

Batfish: aggregate with no bound interfaces has bandwidth 0 #4125

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

dhalperi
Copy link
Member

Not null, which breaks downstream pipeline.

Fix #4124

Not null, which breaks downstream pipeline.

Fix batfish#4124
@batfish-bot
Copy link

This change is Reviewable

@dhalperi dhalperi requested a review from progwriter June 21, 2019 16:51
Copy link
Member Author

@dhalperi dhalperi 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: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @progwriter)

a discussion (no related file):
Some thoughts:

  • PC1 itself does not appear in the VI Model. That's bad, right?
  • Is this the right fix?

@codecov
Copy link

codecov bot commented Jun 21, 2019

Codecov Report

Merging #4125 into master will decrease coverage by 0.1%.
The diff coverage is 88.88%.

@@             Coverage Diff              @@
##             master    #4125      +/-   ##
============================================
- Coverage     75.23%   75.13%   -0.11%     
+ Complexity    24877    24656     -221     
============================================
  Files          2030     2026       -4     
  Lines         99493    98628     -865     
  Branches      11796    11723      -73     
============================================
- Hits          74857    74106     -751     
+ Misses        19293    19194      -99     
+ Partials       5343     5328      -15
Impacted Files Coverage Δ Complexity Δ
...atfish/src/main/java/org/batfish/main/Batfish.java 75.4% <88.88%> (+0.02%) 298 <2> (ø) ⬇️
.../java/org/batfish/symbolic/state/ExitsNetwork.java 50% <0%> (-50%) 1% <0%> (-1%)
...fish/symbolic/state/NodeInterfaceExitsNetwork.java 66.66% <0%> (-33.34%) 1% <0%> (-1%)
...eachability/transition/RemoveSourceConstraint.java 53.33% <0%> (-13.34%) 5% <0%> (-1%)
.../src/main/java/org/batfish/datamodel/flow/Hop.java 53.33% <0%> (-6.67%) 5% <0%> (-1%)
...c/main/java/org/batfish/dataplane/rib/RibTree.java 89.79% <0%> (-4.09%) 27% <0%> (-1%)
...rc/main/java/org/batfish/datamodel/flow/Trace.java 87.09% <0%> (-3.23%) 11% <0%> (-2%)
...rc/main/java/org/batfish/bddreachability/Edge.java 67.64% <0%> (-2.95%) 11% <0%> (-1%)
...sh/bddreachability/SessionCreationNodeVisitor.java 39.39% <0%> (-2.64%) 13% <0%> (-16%)
...h/bddreachability/ReversePassOriginationState.java 41.17% <0%> (-2.49%) 14% <0%> (-17%)
... and 15 more

Copy link
Contributor

@corinaminer corinaminer 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: 0 of 3 files reviewed, 1 unresolved discussion (waiting on @dhalperi and @progwriter)

a discussion (no related file):

Previously, dhalperi (Dan Halperin) wrote…

Some thoughts:

  • PC1 itself does not appear in the VI Model. That's bad, right?
  • Is this the right fix?

If i understand portchannel interfaces and subinterfaces correctly, i believe you're right that PC1 ought to appear. However, CiscoConfiguration relies on it being explicitly defined; see also this test config.

I think we want to automatically create a VI portchannel interface when a subinterface is defined but its parent isn't (i.e., at the point in CiscoConfiguration linked above). Then i don't think it would be possible for an AGGREGATE_CHILD interface to have no BIND dependency (at least for Cisco portchannel interfaces).


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 r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi)

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.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dhalperi)

a discussion (no related file):
Another issue is why is ospf cost being computed in the first place. This interface should be down.
Based on offline discussion, it appears that conversion should create dependencies on non-existent interfaces and code in snapshot post-processing should turn down interfaces with bind dependencies that point to non-existent interfaces. Some other code (such as l1 logical topology computation) might need to be special cased as well.


@dhalperi dhalperi merged commit 074d670 into batfish:master Jun 21, 2019
@dhalperi dhalperi deleted the batfish-issue branch June 21, 2019 21:14
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.

Interface Int-Name on Cisco ASA is missing bandwidth, cannot compute OSPF cost
4 participants