kubeadm: fix RequiredIPVSKernelModulesAvailableCheck #70051

bart0sh · 2018-10-20T18:10:17Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubeadm IPVS check confuses users as it doesn't
take into account installed modules. It only checks loaded
and builtin modules. If it can't find required module among
loaded or/and builtin modules it fails.
However, it shouldn't be considered an error as installed
modules can still be loaded.

Modified RequiredIPVSKernelModulesAvailableCheck code to
also check installed modules using 'modinfo '

Which issue(s) this PR fixes:

Fixes: kubernetes/kubeadm#975

Does this PR introduce a user-facing change?:

kubeadm: Fixed IPVS check to consider installed kernel modules

bart0sh · 2018-10-20T18:10:48Z

/cc @neolit123

neolit123 · 2018-10-20T18:30:05Z

/area kubeadm
@kubernetes/sig-network-pr-reviews
@kubernetes/sig-cluster-lifecycle-pr-reviews

neolit123

thanks @bart0sh
added some minor comments / questions.

pkg/util/ipvs/kernelcheck_linux.go

bart0sh · 2018-10-20T18:57:41Z

@neolit123 thank you for the review!

kvaps · 2018-10-20T19:19:01Z

We probably should fix also kube-proxy, there is similar check, because it's disables IPVS if required kernel modules not found

bart0sh · 2018-10-20T19:23:18Z

@neolit123 Do you want me to fix it in this PR or a separate PR would be ok too?

neolit123 · 2018-10-20T19:25:28Z

@neolit123 Do you want me to fix it in this PR or a separate PR would be ok too?

@bart0sh probably best to fix it in a separate PR once this PR get approved.

m1093782566 · 2018-10-22T09:10:09Z

Thanks @neolit123

A quick question: are you using IPVS proxier?

cc @islinwb @stewart-yu @Lion-Wei

for review.

m1093782566 · 2018-10-22T09:10:24Z

Thanks @neolit123

A quick question: are you using IPVS proxier?

cc @islinwb @stewart-yu @Lion-Wei

for review.

stewart-yu · 2018-10-23T02:59:42Z

In the function, first part is checking installed modules.
And, why delete the builtin mode check?

kvaps · 2018-10-23T08:02:18Z

And, why delete the builtin mode check?

Why is it needed, if all modules are existing and working well, just not in listed in modules.builtin?

Example, my case:
kubernetes/kubeadm#975 (comment)

bart0sh · 2018-10-23T08:11:04Z

@stewart-yu

In the function, first part is checking installed modules.

It's checking loaded modules as far as I understand the code.

And, why delete the builtin mode check?

It's not deleted. I just added check for installed modules.

I've tested this solution. When ip_vs* modules are not builtin and not loaded before the kubeadm run they're loaded automatically if they're installed. The same reported in the issue:
kubernetes/kubeadm#975 (comment)

islinwb · 2018-10-23T08:57:18Z

pkg/util/ipvs/kernelcheck_linux.go

 			if !match {
-				builtInModules.Insert(string(builtInMode))
+				out, err = r.Executor.Command("modinfo", module).CombinedOutput()


why add code in the check of built-in modules? first check ipvs moduels if they are loaded and if they exists would solve the probelm

it wouldn't as far as I understood. If they're not loaded and not installed they're still be built into the kernel. We should do all 3 checks in my opinion.

We should do all 3 checks in my opinion.

Yes. I think it's better put it seperate and check before builtin modules (few OSes have those in builtin modules).

That makes sense. Will do. Thank you for the idea!

any update?

@stewart-yu I'm travelling. will update this PR today/tomorrow.

k8s-ci-robot · 2018-11-11T20:04:02Z

@bart0sh: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-local-e2e-containerized	22909b6e65bac35781e84e4d16fe163552351372	link	`/test pull-kubernetes-local-e2e-containerized`
pull-kubernetes-e2e-gke	22909b6e65bac35781e84e4d16fe163552351372	link	`/test pull-kubernetes-e2e-gke`

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.

bart0sh · 2018-11-12T08:34:46Z

/test pull-kubernetes-verify

stewart-yu · 2018-11-13T00:13:06Z

Kube-proxy will load those modules automatically, on demand.

No need to load those modules automatically. #59566 (comment)

Another words, if user have all necessary modules, he will not see this warning anymore, even if they are not loaded into the system.

If user have all necessary modules, he will not see this warning anymore, that's true. And we have this function now.

bart0sh · 2018-11-13T11:29:30Z

@stewart-yu

If user have all necessary modules, he will not see this warning anymore, that's true.

That's absolutely not true. I have all required modules installed:

$ find /lib/modules/$(uname -r) -name ip_vs.ko -o -name ip_vs_rr.ko -o -name ip_vs_wrr.ko -o -name ip_vs_sh.ko -o -name nf_conntrack_ipv4.ko
/lib/modules/4.4.140-62-default/kernel/net/ipv4/netfilter/nf_conntrack_ipv4.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_wrr.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_rr.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_sh.ko

However, the check still makes incorrect and confusing warning message:

I1113 13:18:01.781297   14832 kernelcheck_linux.go:45] validating the kernel module IPVS required exists in machine or not
	[WARNING RequiredIPVSKernelModulesAvailable]: the IPVS proxier will not be used, because the following required kernel modules are not loaded: [ip_vs_rr ip_vs_wrr ip_vs_sh ip_vs] or no builtin kernel ipvs support: map[ip_vs:{} ip_vs_rr:{} ip_vs_wrr:{} ip_vs_sh:{} nf_conntrack_ipv4:{}]
you can solve this problem with following methods:
 1. Run 'modprobe -- ' to load missing kernel modules;
2. Provide the missing builtin kernel ipvs support

This is plain wrong: the IPVS proxier will not be used

As IPVS proxier is used just fine after kubeadm init is finished:

$ sudo /sbin/ipvsadm -ln
IP Virtual Server version 1.2.1 (size=4096)
Prot LocalAddress:Port Scheduler Flags
  -> RemoteAddress:Port           Forward Weight ActiveConn InActConn
TCP  10.96.0.1:443 lc
  -> 10.237.72.179:6443           Masq    1      0          0         
TCP  10.96.0.10:53 lc
UDP  10.96.0.10:53 lc

Therefore both recommendations:

1. Run 'modprobe -- ' to load missing kernel modules;
2. Provide the missing builtin kernel ipvs support

don't make sense and meaningless.

@neolit123, @kad, @timothysc please, help me to get this PR merged. It's stuck under review for 24 days already. It's a valid fix for kubernetes/kubeadm#975

kvaps · 2018-11-13T11:57:51Z

@stewart-yu

If user have all necessary modules, he will not see this warning anymore, that's true.

That's absolutely not true. I have all required modules installed:

Agree, warning still there if modules not loaded but existing on the system.

Kube-proxy will load those modules automatically, on demand.

No need to load those modules automatically. #59566 (comment)

kube-proxy already doing that, we don't need to load them on the preflight check by kubeadm.
But we need normal check if modules acceptable to load on the machine. Current check is not cover this case, this PR fixes that.

stewart-yu · 2018-11-14T02:53:45Z

@kvaps

That's absolutely not true. I have all required modules installed:

$ find /lib/modules/$(uname -r) -name ip_vs.ko -o -name ip_vs_rr.ko -o -name ip_vs_wrr.ko -o -name ip_vs_sh.ko -o -name nf_conntrack_ipv4.ko
/lib/modules/4.4.140-62-default/kernel/net/ipv4/netfilter/nf_conntrack_ipv4.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_wrr.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_rr.ko
/lib/modules/4.4.140-62-default/kernel/net/netfilter/ipvs/ip_vs_sh.ko

However, the check still makes incorrect and confusing warning message:

I1113 13:18:01.781297   14832 kernelcheck_linux.go:45] validating the kernel module IPVS required exists in machine or not
	[WARNING RequiredIPVSKernelModulesAvailable]: the IPVS proxier will not be used, because the following required kernel modules are not loaded: [ip_vs_rr ip_vs_wrr ip_vs_sh ip_vs] or no builtin kernel ipvs support: map[ip_vs:{} ip_vs_rr:{} ip_vs_wrr:{} ip_vs_sh:{} nf_conntrack_ipv4:{}]
you can solve this problem with following methods:
 1. Run 'modprobe -- ' to load missing kernel modules;
2. Provide the missing builtin kernel ipvs support

This is plain wrong: the IPVS proxier will not be used

That's wrong.
First, must be those file in you system. file list in command

find /lib/modules/$(uname -r) -name ip_vs.ko -o -name ip_vs_rr.ko -o -name ip_vs_wrr.ko -o -name ip_vs_sh.ko -o -name nf_conntrack_ipv4.ko

Secondary, loaded those file in system, like commad

modprobe -- <name>

Third, check loaded modules
https://github.com/kubernetes/kubernetes/blob/master/pkg/util/ipvs/kernelcheck_linux.go#L47-L64

Finally, If no kernel modules, check builtin modules
https://github.com/kubernetes/kubernetes/blob/master/pkg/util/ipvs/kernelcheck_linux.go#L67-L88

You missing the secondary step, that's true? So the third and finally step throw warning.

BTW, No need to load those modules automatically. https://github.com/kubernetes/kubernetes/pull/59566#issuecomment-390212614， you should load
Manually(step 2)
@bart0sh @kvaps If not mind, could you like ping me in Slack? we can discuss more detail

kvaps · 2018-11-14T09:43:58Z

@stewart-yu
The problem is that modules can be installed on the machine but not listed on modules.builtin file. This PR removes warning in this case.

bart0sh · 2018-11-14T10:00:25Z

@stewart-yu

If not mind, could you like ping me in Slack? we can discuss more detail

sure. Which channel are you at? What's your nick? I couldn't find you there.

bart0sh · 2018-11-14T10:04:00Z

Secondary, loaded those file in system, like command

This is a main point of misunderstanding. You don't need to do any 'modprobe -- ' to be able to work with IPVS. Current implementation of RequiredIPVSKernelModulesAvailable check insist on that and this is confusing. That was the reason for this issue: kubernetes/kubeadm#975 and this PR.

Lion-Wei · 2018-11-15T08:23:31Z

@bart0sh @kvaps
Hi, actually I think this check is to check whether the required modules already loaded, not whether we can load those modules.
I know kube-proxy can execute modprobe ..., but when kube-proxy running inside container, then we need privilege to run this command, that is insecurity, and we are trying to revoke the privilege of kube-proxy container.
So we have this warning, to remind people manually load kernel module.

That's the information I got, Please consider, thanks.

bart0sh · 2018-11-16T15:10:59Z

@Lion-Wei It still looks confusing to me. Why to throw a warning that can be ignored in most of the cases and everything would work just fine?

fejta-bot · 2019-02-14T15:59:09Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

neolit123 · 2019-02-14T16:04:02Z

@bart0sh please close this PR as it seems the PR replaces it.
thanks.

kad · 2019-02-14T16:18:27Z

@Lion-Wei

I know kube-proxy can execute modprobe ..., but when kube-proxy running inside container, then we need privilege to run this command, that is insecurity, and we are trying to revoke the privilege of kube-proxy container.

kube-proxy anyway executed in privileged mode as it need access to control host kernel IPVS and iptables rules.

kad · 2019-02-14T16:20:52Z

pkg/util/ipvs/kernelcheck_linux.go

+
+	// Check if builtin modules exist
+	builtinModsFilePath := fmt.Sprintf("/lib/modules/%s/modules.builtin", kernelVersion)
+	out, err = r.Executor.Command("cut", "-f1", "-d", " ", builtinModsFilePath).CombinedOutput()


Can we get rid of this by replacing it with simple read parse function?

I'd be happy to do that if we reach overall agreement to accept this patch with @Lion-Wei and @stewart-yu

1 would be great if we can use some of the existing util functions for checking these i.e. https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/iptables/proxier.go#L129-L138

bart0sh · 2019-02-14T16:28:37Z

@neolit123

please close this PR as it seems the PR replaces it.

Let's give @Lion-Wei and @stewart-yu another chance to accept it. I still think it's a correct fix and after @kad's comment I don't see any reason of not accepting it.

bart0sh · 2019-02-21T23:48:45Z

/assign @stewart-yu

bart0sh · 2019-02-21T23:50:12Z

@stewart-yu kindly ping for review&acceptance. Please consider last comment from @kad.

k8s-ci-robot · 2019-03-05T23:52:55Z

@bart0sh: PR needs rebase.

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.

cmluciano · 2019-03-12T14:16:25Z

@bart0sh Can you please address the remaining comments and rebase this PR so we can continue reviewing?

bart0sh · 2019-03-12T15:34:20Z

@cmluciano

Can you please address the remaining comments and rebase this PR so we can continue reviewing?

Can you please review this pr instead?
I'm going to deprecate this if that one is approved.

cmluciano · 2019-03-12T19:43:17Z

@bart0sh Reviewed, please close this one out

bart0sh · 2019-03-12T20:05:29Z

@cmluciano

Reviewed, please close this one out

Will do after that one will be approved.

bart0sh · 2019-03-27T00:21:30Z

closed as #75036 has been merged

k8s-ci-robot requested review from m1093782566 and thockin October 20, 2018 18:10

k8s-ci-robot requested a review from neolit123 October 20, 2018 18:10

bart0sh force-pushed the PR0033-kubeadm-fix-RequiredIPVSKernelModulesAvailableCheck branch from cdf54b0 to 432a4a8 Compare October 20, 2018 18:12

k8s-ci-robot added sig/network Categorizes an issue or PR as relevant to SIG Network. area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Oct 20, 2018

neolit123 reviewed Oct 20, 2018

View reviewed changes

pkg/util/ipvs/kernelcheck_linux.go Outdated Show resolved Hide resolved

pkg/util/ipvs/kernelcheck_linux.go Outdated Show resolved Hide resolved

pkg/util/ipvs/kernelcheck_linux.go Outdated Show resolved Hide resolved

bart0sh force-pushed the PR0033-kubeadm-fix-RequiredIPVSKernelModulesAvailableCheck branch from 432a4a8 to 63bf08e Compare October 20, 2018 18:56

bart0sh mentioned this pull request Oct 23, 2018

RequiredIPVSKernelModulesAvailable warning seems confusing kubernetes/kubeadm#975

Closed

islinwb reviewed Oct 23, 2018

View reviewed changes

bart0sh force-pushed the PR0033-kubeadm-fix-RequiredIPVSKernelModulesAvailableCheck branch from 63bf08e to abbd30a Compare October 25, 2018 12:13

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 25, 2018

bart0sh mentioned this pull request Feb 13, 2019

kubeadm: fix RequiredIPVSKernelModulesAvailable warning message #74033

Merged

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 14, 2019

kad reviewed Feb 14, 2019

View reviewed changes

k8s-ci-robot assigned stewart-yu Feb 21, 2019

mwielgus removed the sig/autoscaling Categorizes an issue or PR as relevant to SIG Autoscaling. label Feb 28, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 5, 2019

bart0sh closed this Mar 27, 2019

kubeadm: fix RequiredIPVSKernelModulesAvailableCheck #70051

kubeadm: fix RequiredIPVSKernelModulesAvailableCheck #70051

Conversation

bart0sh commented Oct 20, 2018

bart0sh commented Oct 20, 2018

neolit123 commented Oct 20, 2018

neolit123 left a comment

Choose a reason for hiding this comment

bart0sh commented Oct 20, 2018

kvaps commented Oct 20, 2018

bart0sh commented Oct 20, 2018

neolit123 commented Oct 20, 2018

m1093782566 commented Oct 22, 2018

m1093782566 commented Oct 22, 2018

stewart-yu commented Oct 23, 2018

kvaps commented Oct 23, 2018

bart0sh commented Oct 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 11, 2018 • edited Loading

bart0sh commented Nov 12, 2018

stewart-yu commented Nov 13, 2018

bart0sh commented Nov 13, 2018

kvaps commented Nov 13, 2018 • edited Loading

stewart-yu commented Nov 14, 2018 • edited Loading

kvaps commented Nov 14, 2018 • edited Loading

bart0sh commented Nov 14, 2018

bart0sh commented Nov 14, 2018

Lion-Wei commented Nov 15, 2018

bart0sh commented Nov 16, 2018

fejta-bot commented Feb 14, 2019

neolit123 commented Feb 14, 2019

kad commented Feb 14, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bart0sh commented Feb 14, 2019

bart0sh commented Feb 21, 2019

bart0sh commented Feb 21, 2019

k8s-ci-robot commented Mar 5, 2019

cmluciano commented Mar 12, 2019

bart0sh commented Mar 12, 2019

cmluciano commented Mar 12, 2019

bart0sh commented Mar 12, 2019

bart0sh commented Mar 27, 2019

bart0sh commented Oct 23, 2018 •

edited

Loading

k8s-ci-robot commented Nov 11, 2018 •

edited

Loading

kvaps commented Nov 13, 2018 •

edited

Loading

stewart-yu commented Nov 14, 2018 •

edited

Loading

kvaps commented Nov 14, 2018 •

edited

Loading

kad commented Feb 14, 2019 •

edited

Loading