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

BGP redistribution: set MED to IGP metric #6425

Merged
merged 2 commits into from Dec 4, 2020

Conversation

progwriter
Copy link
Contributor

In the fullness of time this would be probably be done by routing policy because vendors may have knobs to control this behavior, but for now this is a default that seems to apply to big vendors

@batfish-bot
Copy link

This change is Reviewable

@progwriter progwriter marked this pull request as draft November 13, 2020 01:19
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.

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@progwriter
Copy link
Contributor Author

Did not expect this much difference in behavior. Closing pending some closer validation

@progwriter progwriter closed this Dec 1, 2020
In the fullness of time this would be probably be done by routing policy because vendors may have knobs to control this behavior, but for now this is a default that seems to apply to big vendors
@progwriter
Copy link
Contributor Author

A more detailed investigation reveals that the main RIB for the example network is not affected. the BGP ribs are already not-as-correct-as-we-like so the changes reflected in the DP ref test do not make the situation any worse.

@progwriter progwriter reopened this Dec 4, 2020
@progwriter progwriter marked this pull request as ready for review December 4, 2020 21:14
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.

Reviewed 4 of 4 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @progwriter)


projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 3985 at r2 (raw file):

    EigrpRoute noMatchEigrp = internalRb.setNetwork(noMatchRm).build();

    // TODO BGP metric should match original route's metric

can remove this todo 😍
looks like you already rooted it out in the nxos test.

@codecov
Copy link

codecov bot commented Dec 4, 2020

Codecov Report

Merging #6425 (7606923) into master (ff66fba) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##             master    #6425   +/-   ##
=========================================
  Coverage     73.30%   73.30%           
+ Complexity    35476    35474    -2     
=========================================
  Files          2824     2824           
  Lines        143432   143433    +1     
  Branches      17325    17325           
=========================================
+ Hits         105148   105149    +1     
+ Misses        29955    29952    -3     
- Partials       8329     8332    +3     
Impacted Files Coverage Δ Complexity Δ
...batfish/dataplane/protocols/BgpProtocolHelper.java 94.02% <100.00%> (+0.04%) 46.00 <0.00> (ø)
...g/batfish/datamodel/flow/IncomingSessionScope.java 84.61% <0.00%> (-15.39%) 7.00% <0.00%> (-1.00%)
.../org/batfish/dataplane/rib/RouteAdvertisement.java 83.67% <0.00%> (-4.09%) 18.00% <0.00%> (-1.00%)
...java/org/batfish/dataplane/ibdp/VirtualRouter.java 90.54% <0.00%> (-0.34%) 208.00% <0.00%> (-1.00%)
...a/org/batfish/representation/aws/LoadBalancer.java 82.18% <0.00%> (-0.32%) 70.00% <0.00%> (-1.00%)
...src/main/java/org/batfish/coordinator/WorkMgr.java 75.96% <0.00%> (+0.15%) 247.00% <0.00%> (ø%)
...ain/java/org/batfish/storage/FileBasedStorage.java 86.46% <0.00%> (+0.27%) 249.00% <0.00%> (ø%)
...src/main/java/org/batfish/coordinator/PoolMgr.java 60.71% <0.00%> (+1.19%) 16.00% <0.00%> (+1.00%)
...rg/batfish/identifiers/StorageBasedIdResolver.java 91.17% <0.00%> (+5.88%) 22.00% <0.00%> (ø%)

Copy link
Contributor Author

@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: 5 of 6 files reviewed, all discussions resolved (waiting on @corinaminer)


projects/batfish/src/test/java/org/batfish/grammar/cisco/CiscoGrammarTest.java, line 3985 at r2 (raw file):

Previously, corinaminer (Corina Miner) wrote…

can remove this todo 😍
looks like you already rooted it out in the nxos test.

to-done.

Copy link
Contributor Author

@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 1 of 1 files at r3.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@progwriter progwriter merged commit 1cb1859 into batfish:master Dec 4, 2020
@progwriter progwriter deleted the bgp-med branch December 4, 2020 22:31
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