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

Migrate egress IP tracking code to pkg/network/common #20155

Merged

Conversation

danwinship
Copy link
Contributor

@danwinship danwinship commented Jun 29, 2018

Prelude to master-side egress IP tracking; the master will need to be doing the same egressIP-tracking that the node does, so move the code to pkg/network/common so they can share it.

pkg/network/common/egressip_test.go is mostly just a copy of pkg/network/node/egressip_test.go, with the "netlink" and "OVS" changes squished together into a single change stream, and the comments changed in a few places to reflect the fact that "local node" vs "remote node" doesn't matter to the common code.

The changes at the end of pkg/network/node/egressip_test.go are because if egressIPWatcher deletes OVS flows and then recreates them, the testing code requires that we call assertOVSChanges rather than assertNoOVSChanges even if the new flows are identical to the old ones. The new code doesn't delete and recreate flows when nothing changed, so we don't need to do that now.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jun 29, 2018
@danwinship danwinship requested review from pravisankar and dcbw and removed request for smarterclayton June 29, 2018 14:34
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 1, 2018
@openshift-bot openshift-bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2018
@danwinship danwinship force-pushed the auto-egress-ip-common branch 2 times, most recently from 90f7d66 to afd9049 Compare July 6, 2018 14:05
@dcbw
Copy link
Contributor

dcbw commented Jul 9, 2018

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2018
Copy link
Contributor

@dcbw dcbw left a comment

Choose a reason for hiding this comment

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

/lgtm

Copy link

@pravisankar pravisankar left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, dcbw, pravisankar

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@danwinship
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/test cross

@danwinship
Copy link
Contributor Author

/retest

1 similar comment
@danwinship
Copy link
Contributor Author

/retest

@openshift-merge-robot openshift-merge-robot merged commit e895703 into openshift:master Jul 10, 2018
@danwinship danwinship deleted the auto-egress-ip-common branch July 11, 2018 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/networking lgtm Indicates that a PR is ready to be merged. sig/networking size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants