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

proxy/userspace: respect minSyncInterval #71735

Merged
merged 6 commits into from Apr 17, 2019

Conversation

@dcbw
Copy link
Member

commented Dec 5, 2018

The userspace proxy does not have any ratelimiting and when many
services are used will hammer iptables every time a service or
endpoint change occurs.

https://bugzilla.redhat.com/show_bug.cgi?id=1590589

/kind bug
/sig network

The userspace proxy now respects the IPTables proxy's minSyncInterval parameter.

@k8s-ci-robot k8s-ci-robot requested review from lavalamp and smarterclayton Dec 5, 2018

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch 2 times, most recently from 2cfe08c to d3ef149 Dec 5, 2018

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

/priority important-soon

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

/assign @thockin

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from d3ef149 to 9954110 Dec 6, 2018

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

/test pull-kubernetes-e2e-kops-aws

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

/test pull-kubernetes-kubemark-e2e-gce-big

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from 9954110 to 6397110 Dec 6, 2018

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Dec 7, 2018

/lgtm

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

@thockin @bowei any thoughts on this one?

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Dec 19, 2018

/retest

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Jan 2, 2019

@thockin @bowei any thoughts on this one after the holidays?

@thockin
Copy link
Member

left a comment

IMO this code is as dead as it could be. The only significant user is OpenShift as far as I know. I'd rather never touch it again, but I know that is not realistic.

Also, it seems like maybe this could be broken into a couple commits for easier review?

I raised some questions about this design, but I think you should add yourselves as approvers in OWNERS for this subdir. If it evolves, I will lose context on the impl. I don't think it is covered by e2e, either (more argument for breaking it to a separate repo and having its own e2e tests)

Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
Show resolved Hide resolved pkg/proxy/userspace/proxier.go
Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
Show resolved Hide resolved pkg/proxy/userspace/roundrobin.go Outdated

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from 6397110 to 7bcade9 Jan 17, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label Jan 17, 2019

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Jan 17, 2019

Also, it seems like maybe this could be broken into a couple commits for easier review?

@thockin broken into two commits; second commit is largely testcase updates.

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from 9e2728b to 2a42fac Apr 1, 2019

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 1, 2019

@danwinship could you do another review too? Thanks!

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 3, 2019

@JacobTanenbaum also might be of interest to you since you're looking at the proxy these days.

Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
Show resolved Hide resolved pkg/proxy/userspace/proxier_test.go
Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
@danwinship
Copy link
Contributor

left a comment

typo in the last commit message ("consoldiate")

Show resolved Hide resolved pkg/proxy/userspace/proxier.go
Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
Show resolved Hide resolved pkg/proxy/userspace/proxier.go Outdated
Show resolved Hide resolved pkg/proxy/userspace/proxier.go
Show resolved Hide resolved pkg/proxy/userspace/proxier_test.go Outdated

dcbw added some commits Dec 5, 2018

proxy: consolidate ServicesHandler/EndpointsHandler into ProxyProvider
Proxies should be able to cleanly figure out when endpoints have been synced,
so make all ProxyProviders also implement EndpointsHandler and pass those
through to loadbalancers when required.
proxy/userspace: add proxy shutdown function and use in testcases
If a testcase does time out and 'go test' prints the call stack,
make sure everything from previous tests is cleaned up so the call
stack is easier to understand.
proxy/userspace: track initial service/endpoints sync
We'll use this shortly to prevent premature syncing before all
initial endpoints and services have been received from the
apiserver.
proxy/userspace: replace IsServiceIPSet() with ShouldSkipService()
Keeps things consistent with iptables/IPVS proxies. Proxies don't
handle ServiceTypeExternalName even if the ClusterIP is set.

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from 2a42fac to 9a8e4d3 Apr 5, 2019

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@danwinship @thockin I believe I've addressed all comments. PTAL thanks!

proxy/userspace: respect minSyncInterval and simplify locking
The userspace proxy does not have any ratelimiting and when many
services are used will hammer iptables every time a service or
endpoint change occurs. Instead build up a map of changed
services and process all those changes at once instead of each
time an event comes in. This also ensures that no long-running
processing happens in the same call chain as the OnService*
calls as this blocks other handlers attached to the proxy's
parent ServiceConfig object for long periods of time.

Locking can also now be simplified as the only accesses to the
proxy's serviceMap happen from syncProxyRules(). So instead of
locking in many functions just lock once in syncProxyRules()
like the other proxies do.

https://bugzilla.redhat.com/show_bug.cgi?id=1590589
https://bugzilla.redhat.com/show_bug.cgi?id=1689690

@dcbw dcbw force-pushed the dcbw:userspace-proxy-ratelimiting branch from 9a8e4d3 to cc2b31a Apr 5, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

@dcbw: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws aa3f1b0 link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

/retest

@danwinship

This comment has been minimized.

Copy link
Contributor

commented Apr 8, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Apr 8, 2019

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

@thockin PTAL thanks!

dcbw added a commit to dcbw/kubernetes that referenced this pull request Apr 8, 2019

proxy/userspace: add dcbw and danwinship to OWNERS approvers
Per recommendation of @thockin:

kubernetes#71735 (review)

---
IMO this code is as dead as it could be. The only significant user is OpenShift as far as I know. I'd rather never touch it again, but I know that is not realistic.

Also, it seems like maybe this could be broken into a couple commits for easier review?

I raised some questions about this design, but I think you should add yourselves as approvers in OWNERS for this subdir. If it evolves, I will lose context on the impl. I don't think it is covered by e2e, either (more argument for breaking it to a separate repo and having its own e2e tests)
---
@thockin
Copy link
Member

left a comment

/lgtm
/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Apr 17, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, thockin

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

@dcbw

This comment has been minimized.

Copy link
Member Author

commented Apr 17, 2019

@thockin thanks Tim!

@k8s-ci-robot k8s-ci-robot merged commit 2490e03 into kubernetes:master Apr 17, 2019

18 checks passed

cla/linuxfoundation dcbw authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-godeps Context retired. Status moved to "pull-kubernetes-dependencies".
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

hprateek43 added a commit to hprateek43/kubernetes that referenced this pull request Apr 29, 2019

proxy/userspace: add dcbw and danwinship to OWNERS approvers
Per recommendation of @thockin:

kubernetes#71735 (review)

---
IMO this code is as dead as it could be. The only significant user is OpenShift as far as I know. I'd rather never touch it again, but I know that is not realistic.

Also, it seems like maybe this could be broken into a couple commits for easier review?

I raised some questions about this design, but I think you should add yourselves as approvers in OWNERS for this subdir. If it evolves, I will lose context on the impl. I don't think it is covered by e2e, either (more argument for breaking it to a separate repo and having its own e2e tests)
---
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.