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.

Sign up for GitHub

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

Make kubelet touch iptables lock file during initialization #47212

Merged
merged 1 commit into from
Jun 9, 2017
Merged

Make kubelet touch iptables lock file during initialization #47212

merged 1 commit into from
Jun 9, 2017

Conversation

MrHohn
Copy link
Member

@MrHohn MrHohn commented Jun 9, 2017

Fixes #47186.

/assign @freehan @yujuhong
cc @dchen1107 @Q-Lee

Release note:

NONE

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 9, 2017
@k8s-github-robot k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-none Denotes a PR that doesn't merit a release note. labels Jun 9, 2017
@MrHohn MrHohn mentioned this pull request Jun 9, 2017
Closed
ericchiang
ericchiang reviewed Jun 9, 2017
View reviewed changes
pkg/kubelet/kubelet.go Outdated
if err != nil {
glog.Warningf("Failed to open iptables lock file: %v", err)
}
if err := f.Close(); err != nil {
Copy link
Contributor

@ericchiang ericchiang Jun 9, 2017 •
edited

Choose a reason for hiding this comment

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

If OpenFile returns an error, f will be nil and this call will panic.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, done.

@k8s-github-robot k8s-github-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 9, 2017
yujuhong
yujuhong reviewed Jun 9, 2017
View reviewed changes
pkg/kubelet/kubelet.go Outdated
// TODO(#36485) Remove this workaround once we fix the init-container issue.
// Touch iptables lock file, which will be shared among all containers.
f, err := os.OpenFile(utilipt.LockfilePath16x, os.O_CREATE, 0600)
if f == nil || err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: don't think you'll need to check f == nil since you already checked err != nil

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

pkg/kubelet/kubelet.go Outdated
@@ -507,6 +507,15 @@ func NewMainKubelet(kubeCfg *componentconfig.KubeletConfiguration, kubeDeps *Kub
}
glog.Infof("Hairpin mode set to %q", hairpinMode)

// TODO(#36485) Remove this workaround once we fix the init-container issue.
// Touch iptables lock file, which will be shared among all containers.
Copy link
Contributor

Choose a reason for hiding this comment

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

Shared among all containers or shared among all processes accessing the iptables?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks, the latter seems more accurate. Applied.

@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 Jun 9, 2017
@cmluciano cmluciano mentioned this pull request Jun 9, 2017
Merged
@yujuhong
Copy link
Contributor

yujuhong commented Jun 9, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 9, 2017
@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 9, 2017
@dchen1107 dchen1107 added this to the v1.7 milestone Jun 9, 2017
@dchen1107
Copy link
Member

/lgtm

I am going to manually merge this to stop serial test failures.

@dchen1107 dchen1107 merged commit 2a5ac62 into kubernetes:master Jun 9, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: MrHohn, dchen1107, yujuhong

Associated issue: 47186

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

@dchen1107 dchen1107 mentioned this pull request Jun 12, 2017
Closed
@cmluciano cmluciano mentioned this pull request Jun 13, 2017
Closed
@MrHohn MrHohn mentioned this pull request Sep 27, 2017
Merged
k8s-github-robot pushed a commit that referenced this pull request Sep 28, 2017
Merge pull request #53157 from MrHohn/revert-kubelet-touch-lock
d0233d1 
Automatic merge from submit-queue (batch tested with PRs 53157, 52628). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Revert "Make kubelet touch iptables lock file during initialization"

**What this PR does / why we need it**: Revert #47212. #36485 is fixed so this is no longer needed.

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

**Special notes for your reviewer**:
/assign @yujuhong @dchen1107 

**Release note**:

```release-note
NONE
```
bboreham added a commit to weaveworks/weave that referenced this pull request Oct 2, 2017
@bboreham
Mount iptables lock file in weave-kube
ce25919 
iptables uses the file `/run/xtables.lock` so that if changes are
under way, another instance of the iptables command can wait till it
is finished.
We need to mount the lock file from the host to the container in order
for this mechanism to work at all.
Note we rely on kubelet touching the file before running any pods -
see kubernetes/kubernetes#47212
@bboreham bboreham mentioned this pull request Oct 2, 2017
Merged
bboreham added a commit to weaveworks/weave that referenced this pull request Oct 3, 2017
@bboreham
Mount iptables lock file in weave-kube (#3134)
c16653a 
Mount iptables lock file in weave-kube

iptables uses the file `/run/xtables.lock` so that if changes are
under way, another instance of the iptables command can wait till it
is finished.
We need to mount the lock file from the host to the container in order
for this mechanism to work at all.
Note we rely on kubelet touching the file before running any pods -
see kubernetes/kubernetes#47212
@MrHohn MrHohn deleted the kubelet-iptables-lock branch October 5, 2017 00:55
bboreham added a commit to bboreham/kops that referenced this pull request Oct 11, 2017
@bboreham
Add Weave Net template for Kubernetes 1.7 and above
6d9cc77 
This adds a volume-mount for the iptables lock file, which avoids
collisions between Weave components and kube-proxy that would result
in a half-configured Weave network.
This is only for version 1.7 and above because it requires the change
in kubernetes/kubernetes#47212
bboreham added a commit to bboreham/kops that referenced this pull request Oct 12, 2017
@bboreham
Add Weave Net template for Kubernetes 1.7 and above
eafc4b0 
This adds a volume-mount for the iptables lock file, which avoids
collisions between Weave components and kube-proxy that would result
in a half-configured Weave network.
This is only for version 1.7 and above because it requires the change
in kubernetes/kubernetes#47212
bboreham added a commit to bboreham/kops that referenced this pull request Oct 12, 2017
@bboreham
Update to Weave Net 2.0.5
3e450a7 
including a Weave Net template for Kubernetes 1.7 and above which adds
a volume-mount for the iptables lock file, which avoids collisions
between Weave components and kube-proxy that would result in a
half-configured Weave network.

This is only for version 1.7 and above because it requires the change
in kubernetes/kubernetes#47212
bboreham added a commit to bboreham/kops that referenced this pull request Oct 12, 2017
@bboreham
Update to Weave Net 2.0.5
a4cb2fd 
including a Weave Net template for Kubernetes 1.7 and above which adds
a volume-mount for the iptables lock file, which avoids collisions
between Weave components and kube-proxy that would result in a
half-configured Weave network.

This is only for version 1.7 and above because it requires the change
in kubernetes/kubernetes#47212
bboreham added a commit to bboreham/kops that referenced this pull request Oct 12, 2017
@bboreham
Update to Weave Net 2.0.5
88e001f 
including a Weave Net template for Kubernetes 1.7 and above which adds
a volume-mount for the iptables lock file, which avoids collisions
between Weave components and kube-proxy that would result in a
half-configured Weave network.

This is only for version 1.7 and above because it requires the change
in kubernetes/kubernetes#47212
@bboreham bboreham mentioned this pull request Oct 12, 2017
Merged
k8s-github-robot pushed a commit to kubernetes/kops that referenced this pull request Oct 12, 2017
Merge pull request #3614 from bboreham/weave-update-205
5e1f813 
Automatic merge from submit-queue.

Update Weave Net to version 2.0.5

This PR also adds a manifest with a volume-mount for the iptables lock file, which avoids collisions between Weave components and kube-proxy that can sometimes result in a half-configured Weave network.

Only do this for Kubernetes 1.7 and above because it requires the change in kubernetes/kubernetes#47212

I don't really know what I'm doing in `bootstrapchannelbuilder.go`; I just followed the pattern I saw.

Other relevant updates in Weave Net since version 2.0.1 ([more details](https://github.com/weaveworks/weave/releases)):

* Fix race condition in NetworkPolicy Controller which would intermittently block all traffic for a namespace
* Add comments to each NetworkPolicy iptables rule and ipset, to help when troubleshooting
* Fix netfilter rules to block containers from accessing the Weave Net control endpoint
* Remove code that checked for an outdated fallback address for Kubernetes api-server
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericchiang ericchiang ericchiang left review comments

@yujuhong yujuhong yujuhong left review comments

Assignees

@freehan freehan

@dchen1107 dchen1107

@yujuhong yujuhong

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
Milestone
v1.7
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@MrHohn @yujuhong @dchen1107 @k8s-github-robot @ericchiang @freehan @k8s-ci-robot