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

[kube-proxy/ipvs] Protect Netlink calls with a mutex #72361

Merged
merged 1 commit into from Dec 27, 2018

Conversation

@lbernail
Copy link
Contributor

lbernail commented Dec 27, 2018

What type of PR is this?
/kind bug

What this PR does / why we need it:
We have a race condition between the proxier and the gracefulTerminationManager goroutines: when both use the netlink socket at the same time we can end up in a deadlock. This PR protects netlink calls with a mutex to make sure a single goroutine is using the socket at any given time.
An alternative would be to use two different sockets but it is probably safer to avoid parallel netlink calls.
Not sure if this is the best design, happy to discuss it.

Which issue(s) this PR fixes:
Fixes #71071

Special notes for your reviewer:
Currently being tested by users that reported the issue (more details in #71071)

Does this PR introduce a user-facing change?:

Fix race condition introduced by graceful termination which can lead to a deadlock in kube-proxy

/sig network
/area ipvs
/assign @m1093782566

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 27, 2018

Hi @lbernail. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

@m1093782566

This comment has been minimized.

Copy link
Member

m1093782566 commented Dec 27, 2018

/ok-to-test

@m1093782566

This comment has been minimized.

Copy link
Member

m1093782566 commented Dec 27, 2018

I am open to discuss the two-sockets approach, but I think we should fix the dead-lock issue first. Obviously, this PR can avoid dead lock.

@m1093782566

This comment has been minimized.

Copy link
Member

m1093782566 commented Dec 27, 2018

/lgtm

/approve

Let's see...

@k8s-ci-robot k8s-ci-robot added the lgtm label Dec 27, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Dec 27, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: lbernail, m1093782566

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

@k8s-ci-robot k8s-ci-robot merged commit bb7973a into kubernetes:master Dec 27, 2018

19 checks passed

cla/linuxfoundation lbernail authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
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-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-godeps Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
ipvsSvc, err := runner.ipvsHandle.GetService(svc)
runner.mu.Unlock()

This comment has been minimized.

@tedyu

tedyu Dec 27, 2018

Contributor

Why not use 'defer' ?

This comment has been minimized.

@lbernail

lbernail Dec 27, 2018

Author Contributor

We could get the lock at the beginning of each function and use defer everywhere to make everything consistent. I chose to release the lock as quickly as possible. We do not need to keep the lock to call toVirtualServer in GetVirtualServer for instance.

I'm completely open to changing this if it makes the code easier to read and the optimization is not worth it.

This comment has been minimized.

@tedyu

tedyu Dec 27, 2018

Contributor

What happens if GetService() encounters error ?
I am yet to get familiar with Go lang. Not sure if the unlocking would be skipped in some situation.

This comment has been minimized.

@matthiasr

matthiasr Dec 27, 2018

Member

The error is returned as a value, and only inspected after unlocking, so the mutex will be released in any case.

This comment has been minimized.

@m1093782566

k8s-ci-robot added a commit that referenced this pull request Jan 4, 2019

Merge pull request #72429 from DataDog/automated-cherry-pick-of-#7236…
…1-upstream-release-1.12

Automated cherry pick of #72361 upstream release 1.12

k8s-ci-robot added a commit that referenced this pull request Jan 4, 2019

Merge pull request #72430 from DataDog/automated-cherry-pick-of-#7236…
…1-upstream-release-1.11

Automated cherry pick of #72361 upstream release 1.11

k8s-ci-robot added a commit that referenced this pull request Jan 5, 2019

Merge pull request #72426 from DataDog/automated-cherry-pick-of-#7236…
…1-upstream-release-1.13

Automated cherry pick of #72361 upstream release 1.13
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.