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

Dataplane changes for BGP next hop IP unchanged #4722

Merged
merged 9 commits into from
Sep 6, 2019
Merged

Conversation

haverma
Copy link

@haverma haverma commented Sep 6, 2019

  • also adds a check in UnchangedNextHop expr to only apply when original route is BgpRoute

@batfish-bot
Copy link

This change is Reviewable

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 4 of 5 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @haverma)


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 827 at r2 (raw file):

            Direction.OUT);

    // setting next hop IP for outgoing route

Why not move this to transformBgpRoutePostExport?

Copy link
Author

@haverma haverma 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: 2 of 7 files reviewed, all discussions resolved (waiting on @progwriter)


projects/batfish/src/main/java/org/batfish/dataplane/ibdp/BgpRoutingProcess.java, line 827 at r2 (raw file):

Previously, progwriter (Victor Heorhiadi) wrote…

Why not move this to transformBgpRoutePostExport?

done, also for iBGP now checking if the next hop IP in the output builder was unset before setting it to anything. So that condition is not hit while exporting non BGP routes to BGP (where nhip would be present already).

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

@codecov
Copy link

codecov bot commented Sep 6, 2019

Codecov Report

Merging #4722 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #4722   +/-   ##
=========================================
  Coverage     75.38%   75.38%           
  Complexity    27894    27894           
=========================================
  Files          2215     2215           
  Lines        110548   110548           
  Branches      13337    13337           
=========================================
  Hits          83334    83334           
  Misses        20854    20854           
  Partials       6360     6360

@haverma haverma merged commit c02e7a9 into master Sep 6, 2019
@haverma haverma deleted the unchanged-nhip-dp branch September 6, 2019 23: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.

None yet

3 participants