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

Kubelet: only sync iptables on linux #67690

Merged
merged 2 commits into from
Sep 6, 2018
Merged

Kubelet: only sync iptables on linux #67690

merged 2 commits into from
Sep 6, 2018

Conversation

feiskyer
Copy link
Member

What this PR does / why we need it:

Iptables is only supported on Linux, kubelet should only sync NAT rules on Linux.

Without this PR, Kubelet on Windows would logs following errors on each syncNetworkUtil():

kubelet.err.log:4692:E0711 22:03:42.103939    2872 kubelet_network.go:102] Failed to ensure that nat chain KUBE-MARK-DROP exists: error creating chain "KUBE-MARK-DROP": executable file

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #65713

Special notes for your reviewer:

Release note:

Kubelet now only sync iptables on Linux.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Aug 22, 2018
@k8s-ci-robot k8s-ci-robot requested review from sjpotter and tmrts August 22, 2018 07:05
@feiskyer
Copy link
Member Author

/sig windows

/assign @dchen1107 @PatrickLang

@k8s-ci-robot k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Aug 22, 2018
@feiskyer feiskyer added the kind/bug Categorizes issue or PR as related to a bug. label Aug 22, 2018
@feiskyer
Copy link
Member Author

/retest

@dims
Copy link
Member

dims commented Aug 22, 2018

please add a note about the tests moving to linux too (TestMakeBlockVolumes and TestMakeVolumes )

@feiskyer
Copy link
Member Author

@dims TestMakeVolumes is both included for Linux and Windows, while TestMakeBlockVolumes is only for Linux now. Since these are only unit tests, I think they shouldn't be included in release notes. WDYT?

@dims
Copy link
Member

dims commented Aug 23, 2018

@feiskyer just the commit message, not the release note.

@feiskyer
Copy link
Member Author

@feiskyer just the commit message, not the release note.

Hmm, yep. Let me add that.

@feiskyer
Copy link
Member Author

Commit message updated with tests moves.

@feiskyer
Copy link
Member Author

/retest

@feiskyer feiskyer added this to the v1.12 milestone Aug 24, 2018
@PatrickLang
Copy link
Contributor

/LGTM for windows

@dims
Copy link
Member

dims commented Sep 2, 2018

/assign @dchen1107
/assign @derekwaynecarr

@dchen1107
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 5, 2018
@feiskyer
Kubelet: only sync iptables on linux
aeea967
@feiskyer
Fix unit tests for Windows
376b45c 
* TestMakeBlockVolume is moved to Linux only.
* TestMakeMounts are running on both Linux and Windows
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 5, 2018
@feiskyer
Copy link
Member Author

feiskyer commented Sep 5, 2018

Rebased to master and solved compile errors.

@feiskyer
Copy link
Member Author

feiskyer commented Sep 5, 2018

/retest

andyzhangx
andyzhangx reviewed Sep 6, 2018
View reviewed changes
pkg/kubelet/kubelet_network_others.go
package kubelet

// Do nothing.
func (kl *Kubelet) syncNetworkUtil() {}
Copy link
Member

Choose a reason for hiding this comment

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

what about naming as pkg/kubelet/kubelet_network_unsupported.go

Copy link
Member Author

Choose a reason for hiding this comment

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

it's same but _others is preferred because _linux.go is used for Linux builds.

andyzhangx
andyzhangx approved these changes Sep 6, 2018
View reviewed changes
Copy link
Member

@andyzhangx andyzhangx left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 6, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andyzhangx, dchen1107, feiskyer, PatrickLang

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

@feiskyer feiskyer added the priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. label Sep 6, 2018
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here: https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md.

@k8s-github-robot k8s-github-robot merged commit 4bc9e94 into kubernetes:master Sep 6, 2018
@feiskyer feiskyer deleted the iptables-cross branch September 6, 2018 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andyzhangx andyzhangx andyzhangx approved these changes

@sjpotter sjpotter Awaiting requested review from sjpotter

@tmrts tmrts Awaiting requested review from tmrts

Assignees

@andyzhangx andyzhangx

@derekwaynecarr derekwaynecarr

@dchen1107 dchen1107

@PatrickLang PatrickLang

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/windows Categorizes an issue or PR as relevant to SIG Windows. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
v1.12
Development

Successfully merging this pull request may close these issues.

KUBE-MARK-DROP not found on windows kubelet stderr
8 participants
@feiskyer @dims @PatrickLang @dchen1107 @k8s-ci-robot @k8s-github-robot @andyzhangx @derekwaynecarr