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

Add iptables lock-file mount to kube-proxy manifest #46259

Merged
merged 1 commit into from
Jun 5, 2017

Conversation

Q-Lee
Copy link
Contributor

@Q-Lee Q-Lee commented May 23, 2017

What this PR does / why we need it: kube-proxy is broken in make bazel-release. The new iptables binary uses a lockfile in "/run", but the directory doesn't exist. This causes iptables-restore to fail. We need to share the same lock-file amongst all containers, so mount the host /run dir.

This is similar to #46132 but expediency matters, since builds are broken.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #46103

Special notes for your reviewer:

Release note:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 23, 2017
@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. release-note-label-needed labels May 23, 2017
@Q-Lee
Copy link
Contributor Author

Q-Lee commented May 23, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this

@eparis
Copy link
Contributor

eparis commented May 23, 2017

/assign @dcbw

@cblecker
Copy link
Member

@Q-Lee
Copy link
Contributor Author

Q-Lee commented May 23, 2017

@k8s-bot pull-kubernetes-unit test this

@Q-Lee
Copy link
Contributor Author

Q-Lee commented May 23, 2017

@cblecker I would find such a leak surprising. Either way, we're in the host netns, so it shouldn't matter.

I think the bigger concern is that we're mounting the entire /run directory. Unfortunately, if the file doesn't exist, then docker decides we want a dir....

@thockin Options? Perhaps we mount "/run" in an init container, touch the file, and then mount just the lock file in kube-proxy? Maybe we could touch the file in node-startup.sh, or any other script that executes after a boot, but before the kubelet creates kube-proxy?

@thockin
Copy link
Member

thockin commented May 26, 2017

What we need is #34058

Anyone want to scramble an implementation? :)

Maybe we can live with mounting all of /run, or initcontainer as you suggest. I don't have a better answer at hand..

@Q-Lee
Copy link
Contributor Author

Q-Lee commented May 26, 2017

@thockin - the PR delay for the API alone would exceed remaining time :P

I'll try an initcontainer later today, and see if that works well.

@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels May 26, 2017
@bboreham
Copy link
Contributor

What we need is #34058

Note that new-file, the case we need here, was excluded from that proposal, and is not implemented in #46597

@thockin
Copy link
Member

thockin commented May 31, 2017 via email

@Q-Lee
Copy link
Contributor Author

Q-Lee commented May 31, 2017

@thockin - so is the plan to try and get #46597 in for 1.7, and then fix the kube-proxy config?

@ixdy
Copy link
Member

ixdy commented Jun 1, 2017

@k8s-bot pull-kubernetes-e2e-gce-bazel test this

1 similar comment
@ixdy
Copy link
Member

ixdy commented Jun 1, 2017

@k8s-bot pull-kubernetes-e2e-gce-bazel test this

@ixdy
Copy link
Member

ixdy commented Jun 2, 2017

Lots of test failures still on a bazel-built cluster, probably due to #45298, but this does PR does at least get us running tests. (Compare against a test run at HEAD.)

- hostPath:
path: /run
name: run
- hostPath:
Copy link
Member

Choose a reason for hiding this comment

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

you should not need 2 volumes, just use subPath in the volumeMount

@thockin
Copy link
Member

thockin commented Jun 2, 2017

This is hacky but workable

@thockin
Copy link
Member

thockin commented Jun 2, 2017

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 2, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Q-Lee, thockin

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

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 2, 2017
@calebamiles calebamiles modified the milestone: v1.7 Jun 2, 2017
@eparis eparis added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed do-not-merge DEPRECATED. Indicates that a PR should not merge. Label can only be manually applied/removed. labels Jun 2, 2017
@eparis
Copy link
Contributor

eparis commented Jun 2, 2017

@k8s-bot pull-kubernetes-e2e-gce-bazel test this
@k8s-bot pull-kubernetes-unit test this
@k8s-bot pull-kubernetes-federation-e2e-gce test this

@ixdy
Copy link
Member

ixdy commented Jun 2, 2017

#46820 might be an alternate fix

@fejta
Copy link
Contributor

fejta commented Jun 2, 2017

@k8s-bot pull-kubernetes-federation-e2e-gce test this
ref: #46827

@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2017
@Q-Lee
Copy link
Contributor Author

Q-Lee commented Jun 4, 2017

@ixdy do we have a fix for the version parsing?

@k8s-ci-robot
Copy link
Contributor

@Q-Lee: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce-bazel 79bfe91b888dbc9e88480be0762fcff2f8575688 link @k8s-bot pull-kubernetes-e2e-gce-bazel test this
pull-kubernetes-federation-e2e-gce 6a380e8 link @k8s-bot pull-kubernetes-federation-e2e-gce test this

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.

@thockin thockin added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2017
@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 46734, 46810, 46759, 46259, 46771)

@k8s-github-robot k8s-github-robot merged commit 0cfef01 into kubernetes:master Jun 5, 2017
@ixdy
Copy link
Member

ixdy commented Jun 5, 2017

@Q-Lee half of the fix is bazelbuild/rules_go#505. I haven't yet worked on the changes for Kubernetes, but it should be easy once that's merged.

justinsb added a commit to justinsb/kops that referenced this pull request Mar 20, 2018
We only do this for >= 1.9 so we don't change existing clusters.

Equivalent of kubernetes/kubernetes#46259
vendrov pushed a commit to vendrov/kops that referenced this pull request Mar 21, 2018
We only do this for >= 1.9 so we don't change existing clusters.

Equivalent of kubernetes/kubernetes#46259
rdrgmnzs pushed a commit to rdrgmnzs/kops that referenced this pull request Apr 6, 2018
We only do this for >= 1.9 so we don't change existing clusters.

Equivalent of kubernetes/kubernetes#46259
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. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note-none Denotes a PR that doesn't merit a release note. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Running kube-proxy in a container breaks iptables locking