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

Implement IPVS-based in-cluster service load balancing #46580

Merged

Conversation

dujun1990
Copy link

@dujun1990 dujun1990 commented May 28, 2017

What this PR does / why we need it:

Implement IPVS-based in-cluster service load balancing. It can provide some performance enhancement and some other benefits to kube-proxy while comparing iptables and userspace mode. Besides, it also support more sophisticated load balancing algorithms than iptables (least conns, weighted, hash and so on).

Which issue this PR fixes

#17470 #44063

Special notes for your reviewer:

@thockin @quinton-hoole @kevin-wangzefeng @deepak-vij @haibinxie @dhilipkumars @fisherxu

Release note:

Implement IPVS-based in-cluster service load balancing

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 28, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @dujun1990. 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 @k8s-bot 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.

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.

@k8s-github-robot k8s-github-robot added kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. release-note-label-needed labels May 28, 2017
@dujun1990 dujun1990 changed the title implement ipvs mode kube proxy Implement IPVS-based in-cluster service load balancing May 28, 2017
}

if expectCommandExecCount != fexec.CommandCalls {
t.Errorf("Exepect comand executed %d times, but got %d", expectCommandExecCount, fexec.CommandCalls)
Copy link
Contributor

Choose a reason for hiding this comment

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

s/comand/command/

Copy link
Contributor

Choose a reason for hiding this comment

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

@m1093782566 m1093782566 force-pushed the kube-proxy-ipvs-pr branch from 7f47cbf to 951ea89 Compare May 28, 2017 14:41
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 28, 2017
@m1093782566
Copy link
Contributor

@fisherxu needs to sign cla?

@fisherxu
Copy link
Contributor

@k8s-ci-robot have signed the cla, thank you

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels May 28, 2017
@cmluciano
Copy link

@k8s-bot ok to test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label May 29, 2017
@m1093782566
Copy link
Contributor

Thanks. Let's fix test failures. @dhilipkumars @fisherxu

@dhilipkumars
Copy link

@dujun1990 and @fisherxu

Sorry i re-introduced the tests, we need them for sure. But just disabled it when run by non-root user and if the initialization failed.

The initialization process also should take care of loading the ip_vs kernel module?

how about moving below code into ipvs.InitIpvsInterface() from NewProxier

// The system command "modeprobe" is hard coded here.
	for _, module := range ipvsModules {
		_, err := exec.Command("modprobe", module).CombinedOutput()
		if err != nil {
			glog.Warningf("Warning: Can not load module: %s in lvs proxier", module)
		}
	}

@m1093782566
Copy link
Contributor

m1093782566 commented May 31, 2017

@dhilipkumars

I suggest keep modprobe ip_vs in NewProxier() which is called whiling initializing ipvs proxy instance.

We should keep it as ipvs.InitIpvsInterface() can be called by other proxy mode(userspace and iptables) in set up for the purpose of cleaning up ipvs rules.

Besides, UT failed again, please check?

@k8s-ci-robot k8s-ci-robot removed the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 31, 2017
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label May 31, 2017
@dhilipkumars dhilipkumars force-pushed the kube-proxy-ipvs-pr branch 2 times, most recently from ce9720b to 4a68b93 Compare May 31, 2017 14:11
@dhilipkumars
Copy link

@k8s-bot pull-kubernetes-unit test this

Conflicts:
	pkg/util/ipvs/ipvs_unsupported.go
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2017
@m1093782566
Copy link
Contributor

Rebased again to HEAD of master branch due to conflicted with other PR(#47263). Changes only happened in pkg/features/kube_features.go

@thockin
Copy link
Member

thockin commented Aug 30, 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 Aug 30, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

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

Associated issue: 17470

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

loadModules.Insert(mods...)
modules := wantModules.Difference(loadModules).List()
if len(modules) != 0 {
return false, fmt.Errorf("Failed to load kernel modules: %v", modules)
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't try to modprobe these modules first ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Because kube-proxy is running in container.

Copy link
Contributor

Choose a reason for hiding this comment

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

kube-proxy isn't always running in a container

@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 (batch tested with PRs 51377, 46580, 50998, 51466, 49749)

@k8s-github-robot k8s-github-robot merged commit 367cdb1 into kubernetes:master Aug 30, 2017
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Aug 30, 2017

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

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws 5ed2b44 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.

@m1093782566 m1093782566 mentioned this pull request Aug 30, 2017
13 tasks
@m1093782566
Copy link
Contributor

Good luck with resyncing this to head of the iptables. I suggest maybe replaying each commit to the iptables proxier against your own. It's not going to be fun, but it needs to be done.

Don't forget all the other todos in code and review process.

@thockin

I created an issue for tracking all these tasks, see #51602. I will ping you when they are ready for review or I needs your suggestions :) Thanks!

@ixdy
Copy link
Member

ixdy commented Nov 8, 2017

Was adding module-init-tools to the debian-iptables image needed here? I don't think we even rebuilt the iptables image, so I'm guessing not? @rphillips

@ixdy
Copy link
Member

ixdy commented Nov 8, 2017

also cc @tallclair re: my comment above ^

@m1093782566
Copy link
Contributor

@ixdy

That's because IPVS kube-proxy needs the util modprobe...

@gyliu513
Copy link
Contributor

@dujun1990 Did not found any information for how to upgrade kube-proxy working more from iptables to ipvs in CHANGELOG, any comments/suggestions for this?

@m1093782566
Copy link
Contributor

@gyliu513
Copy link
Contributor

@m1093782566 I mean how to upgrade? If I already have a cluster running with kube-proxy iptables , how can I enable it with ipvs without impacting current workloads?

@SEJeff
Copy link
Contributor

SEJeff commented Nov 15, 2017

@gyliu513 drain the node to evict all pods ala:

kubectl drain --force --ignore-daemonsets --delete-local-data $NODE_NAME_HERE

Then you can stop the kube-proxy service, reconfigure it, and restart it. It very much depends on how you've deployed kube-proxy as it might be trickier if you've deployed it via a daemonset.

@m1093782566
Copy link
Contributor

m1093782566 commented Nov 16, 2017

@gyliu513

Actually, IPVS mode is NOT an upgrade from iptables mode - not the relationship of etcd2 -> etcd3.

If your "upgrade" means upgrading the software and switch the proxy mode, it very depends on your deployment and kube-proxy provides the --proxy-mode=ipvs option. @SEJeff gave a good example.

Since ipvs rules is totally different from iptables rules and IPVS kube-proxy will clean up all iptables rules before running, service VIP will go down during "upgrade".

@gyliu513
Copy link
Contributor

got it, thanks @SEJeff @m1093782566

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. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/network Categorizes an issue or PR as relevant to SIG Network. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.