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

Jump to bottom

Unidling: minimize iptables lock contention #8

Merged
merged 3 commits into from Jul 29, 2019
Merged

Unidling: minimize iptables lock contention #8

merged 3 commits into from Jul 29, 2019

Conversation

squeed
Copy link
Contributor

@squeed squeed commented Jul 19, 2019

This PR includes

  • backport of upstream PR 71735, which moves the userspace proxy over to an asynchronous model
  • Changes needed to use that backport
  • A small carry patch that lets us tweak the proxy internals slightly
  • A patch that combines both proxy's sync loops, preventing lock contention

Fixes: BZ 1699341

(This is the 4.2 version of openshift/origin#23426)

@openshift-ci-robot openshift-ci-robot added the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jul 19, 2019
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 19, 2019
@squeed
Copy link
Contributor Author

squeed commented Jul 19, 2019

@dcbw PTAL.
/hold because this is honestly sketchy and should not be accidentally merged.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Jul 19, 2019
@squeed
Copy link
Contributor Author

squeed commented Jul 22, 2019

/hold
Will need to re-work this when #10 merges.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2019
@deads2k
Copy link
Contributor

deads2k commented Jul 22, 2019

/hold
Will need to re-work this when #10 merges.

Yeah, you'll want to open pulls against openshift/kubernetes:sdn-4.2-kuberentes-1.14.0, do a fake bump here to confirm they work, then merge the fork and bump here

@squeed squeed mentioned this pull request Jul 25, 2019
Merged
@squeed
Copy link
Contributor Author

squeed commented Jul 26, 2019

/hold cancel

This is now ready to go. The changes are in our upstream fork, and vendor is updated. @dcbw, PTAL.

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 26, 2019
@squeed squeed force-pushed the fix-unidling branch 2 times, most recently from 318df92 to c228cb2 Compare July 26, 2019 10:28
@squeed
Copy link
Contributor Author

squeed commented Jul 26, 2019

OK, hammered on this in a test cluster, seems to work pretty well.

@squeed
sdn-proxy: update for combined service/endpoints handler
8a6d881
@squeed
network/unidler: Consolidate proxy apply loops.
89fcb4c 
Both proxies have an asynchronous apply loop, but we really need them
to apply sequentially. So, run them as part of the same runner.
squeed added a commit to squeed/openshift-sdn that referenced this pull request Jul 26, 2019
@squeed
iptables: delete unneeded old rules (from old versions)
de94c0d 
bz1726045 / PR openshift#8, we changed the source chain for OPENSHIFT-MASQUERADE.
However, we don't clean up the old rule.

For now, just hard-code a list of rules that shouldn't exist. No need to
over-complicate things.
@squeed squeed mentioned this pull request Jul 26, 2019
Merged
@danwinship
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship, squeed

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

@squeed squeed mentioned this pull request Jul 29, 2019
Merged
@openshift-merge-robot openshift-merge-robot merged commit 1425b7e into openshift:master Jul 29, 2019
dcbw pushed a commit to dcbw/origin that referenced this pull request Aug 20, 2019
@squeed @dcbw
proxy: Consolidate proxy apply loops.
35251f7 
This backports openshift/sdn#8

Both proxies have an asynchronous apply loop, but we really need them
to apply sequentially. So, run them as part of the same runner.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danwinship danwinship Awaiting requested review from danwinship

@dcbw dcbw Awaiting requested review from dcbw

Assignees

@danwinship danwinship

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@squeed @deads2k @danwinship @openshift-ci-robot @openshift-merge-robot