Add GetKernelVersion to ipvs.KernelHandler interface #80636

ebati · 2019-07-26T13:01:29Z

ipvs getProxyMode test fails on mac as utilipvs.GetRequiredIPVSMods
try to reach /proc/sys/kernel/osrelease to find version of the running
linux kernel. Linux kernel version is used to determine the list of required
kernel modules for ipvs.

Logic to determine kernel version is moved to GetKernelVersion
method in LinuxKernelHandler which implements ipvs.KernelHandler.
Mock KernelHandler is used in the test cases.

Fixes #80504.

/kind cleanup

Special notes for your reviewer:

I write NONE to release-note block. However this PR changes an exported interface, do i need to write a note?

Does this PR introduce a user-facing change?:

NONE

k8s-ci-robot · 2019-07-26T13:01:30Z

Welcome @ebati!

It looks like this is your first PR to kubernetes/kubernetes 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes/kubernetes has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

k8s-ci-robot · 2019-07-26T13:01:31Z

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://git.k8s.io/community/CLA.md#the-contributor-license-agreement 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.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please log a ticket with the Linux Foundation Helpdesk: https://support.linuxfoundation.org/
Should you encounter any issues with the Linux Foundation Helpdesk, send a message to the backup e-mail support address at: login-issues@jira.linuxfoundation.org

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 · 2019-07-26T13:01:37Z

Hi @ebati. 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 /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.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

andrewsykim · 2019-07-26T17:26:54Z

There's some work in progress at the moment to move GetKernelVersion into another repo - kubernetes/utils#99 cc @neolit123 @yastij

But also, should IPVS tests even run on MacOS?

andrewsykim · 2019-07-26T17:28:06Z

On second thought, running kube-proxy unit tests on MacOS is legit and we shouldn't gate on the kernel checks for those. We can probably continue kubernetes/utils#99 after this PR

/assign
/assign @vllry

andrewsykim · 2019-07-26T17:29:35Z

I write NONE to release-note block. However this PR changes an exported interface, do i need to write a note?

I think no release note is fine.

andrewsykim

Thanks for the PR @ebati, left some comments :)

andrewsykim · 2019-07-26T17:31:03Z

pkg/util/ipvs/ipvs_test.go

+			}
+			sort.Strings(got)
+			sort.Strings(test.want)
+			for i := range test.want {


Use reflect.DeepEqual here since the slice is already sorted?

andrewsykim · 2019-07-26T17:31:52Z

pkg/util/ipvs/ipvs_test.go

+		want          []string
+	}{
+		{
+			name:          "KernelPriorTo4.19",


test names can have spaces - "kernel version < 4.19"

andrewsykim · 2019-07-26T17:32:00Z

pkg/util/ipvs/ipvs_test.go

+			want:          []string{ModIPVS, ModIPVSRR, ModIPVSWRR, ModIPVSSH, ModNfConntrackIPV4},
+		},
+		{
+			name:          "Kernel4.19",


kernel version 4.19

andrewsykim · 2019-07-26T17:34:07Z

pkg/util/ipvs/ipvs.go

-	if ver1.LessThan(ver2) {
-		ipvsModules = append(ipvsModules, ModIPVS, ModIPVSRR, ModIPVSWRR, ModIPVSSH, ModNfConntrackIPV4)
+	if kernelVersion.LessThan(criticalVers) {
+		return []string{ModIPVS, ModIPVSRR, ModIPVSWRR, ModIPVSSH, ModNfConntrackIPV4}
 	} else {


else not needed

andrewsykim · 2019-07-26T17:36:03Z

pkg/proxy/ipvs/proxier.go

+	// parse kernel version
+	kernelVersion, err := version.ParseGeneric(kernelVersionStr)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing kernel version: %v(%s)", err, kernelVersion)


"error parsing kernel version %q: %v", kernelVersion, err?

I moved this block from the old version, but you are right.

andrewsykim · 2019-07-26T17:37:30Z

pkg/proxy/ipvs/proxier.go

+// GetKernelVersion returns currently running kernel version.
+func (handle *LinuxKernelHandler) GetKernelVersion() (*version.Version, error) {
+	kernelVersionFile := "/proc/sys/kernel/osrelease"
+	out, err := handle.executor.Command("cut", "-f1", "-d", " ", kernelVersionFile).CombinedOutput()


A bit overdue, but we should use strings.Fields here instead of execing into cut. Can we fix it here or add a TODO comment?

I read file with limiting buffer size as suggested in the other issue you linked. AFAIK we don't need to call Fields as this file contains only kernel version in this file. But i used it to match older behavior. I checked kernel documentation but couldn't conclude if this file can have multiple columns of text.

I also noticed we removed the strings.TrimSpace check, I agree it's probably not needed here because of strings.Fields but I can't say confidently that it wasn't added for no reason.

andrewsykim · 2019-07-26T18:21:07Z

/ok-to-test

neolit123 · 2019-07-26T19:11:04Z

But also, should IPVS tests even run on MacOS?

IPVS as a whole should be Linux scoped only.
(including e2e and unit tests).

for unit tests, these should be in a separate file with // +build linux IMO.

ebati · 2019-07-27T08:30:48Z

I updated the BUILD file inside util/ipvs package by running hack/update-bazel.sh because of the failing test. But i am not sure if i am allowed to add dependency to util/version. And don't understand why "//vendor/k8s.io/utils/exec:go_default_library" moved from deps to all select blocks separately.

andrewsykim · 2019-07-29T17:47:20Z

BUILD files lgtm

andrewsykim · 2019-07-29T17:48:50Z

pkg/util/ipvs/ipvs.go

 	// "nf_conntrack_ipv4" has been removed since v4.19
 	// see https://github.com/torvalds/linux/commit/a0ae2562c6c4b2721d9fddba63b7286c13517d9f
-	ver2, _ := version.ParseGeneric("4.19")
+	criticalVers := version.MustParseGeneric("4.19")


Why name criticalVers?

it is changing behavior so a critical point but i can change it to ver?

I think something more descriptive would be nice. Maybe something like versionNfConntrackIPv4Removed?

the comment is up one line and the scope is very limited so if it is ok for you, i think just ver is enough?

How about this?

if kernelVersion.LessThan(version.MustParseGeneric("4.19")) { return []string{KernelModuleIPVS, KernelModuleIPVSRR, KernelModuleIPVSWRR, KernelModuleIPVSSH, KernelModuleNfConntrackIPV4} } return []string{KernelModuleIPVS, KernelModuleIPVSRR, KernelModuleIPVSWRR, KernelModuleIPVSSH, KernelModuleNfConntrack}

Yes that is even better =)

andrewsykim · 2019-07-29T17:49:30Z

pkg/util/ipvs/ipvs_test.go

+	for _, test := range Tests {
+		t.Run(test.name, func(t *testing.T) {
+			got := GetRequiredIPVSModules(test.kernelVersion)
+			if len(got) != len(test.want) {


reflect.DeepEqual check below should cover this case

It left over from previous version, i will fix it.

andrewsykim · 2019-07-29T17:55:02Z

pkg/proxy/ipvs/proxier.go

+	}
+	defer f.Close()
+
+	bf := bufio.NewScanner(f)


Curious why not ioutil.ReadFile?

ReadFile reads all to memory and the issue you linked suggest 1K limit if i remember correctly.

hmm good point, I think iotuil.ReadFile is OK given the assumptions around /proc/sys/kernel/osrelease. If we want the 1KB limit, I don't think a Scanner is required can just use a bytes.Buffer (see bytes.NewBuffer) and read from os.File returned from os.Open.

vllry · 2019-07-29T21:03:12Z

Thanks for this PR! This problem has been a pain working on kube-proxy on Mac.

andrewsykim · 2019-07-29T21:07:16Z

pkg/proxy/ipvs/proxier.go

+	for i := range mods {
+		fields := strings.Fields(mods[i])
+		if len(fields) > 0 {
+			mods[i] = fields[0]


define a new slice for this instead of reusing mods

andrewsykim · 2019-07-29T21:11:54Z

pkg/proxy/ipvs/proxier.go

 	return append(mods, bmods...), nil
 }

+// getFirstWordOfEachLine reads all the content from r into memory and return a
+// slice which consists of the first word from each line.
+func getFirstWordOfEachLine(r io.Reader) ([]string, error) {


getFirstColumn?

andrewsykim · 2019-07-29T21:14:48Z

pkg/proxy/ipvs/proxier.go

+		return "", fmt.Errorf("error reading osrelease file %q: %v", kernelVersionFile, err)
+	}
+
+	fields := strings.Fields(string(fileContent))


On second thought, this can just be strings.TrimSpace - same as before. I think strings.Fields is only required for the modules

vllry · 2019-07-29T21:46:14Z

Looks good, though I'm not very familiar with IPVS.

ebati · 2019-07-30T12:16:23Z

I cant find any log for failed test so testing again...

/retest

andrewsykim · 2019-07-30T13:39:50Z

/approve
/lgtm

/assign @thockin @dcbw
for cmd/kube-proxy approval

Thanks @ebati!

dcbw · 2019-07-31T18:51:19Z

/approve
/lgtm

dcbw · 2019-07-31T18:51:31Z

/priority important-soon

k8s-ci-robot · 2019-07-31T18:51:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewsykim, dcbw, ebati

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:

~~cmd/kube-proxy/OWNERS~~ [dcbw]
~~pkg/proxy/ipvs/OWNERS~~ [andrewsykim,dcbw]
~~pkg/util/ipvs/OWNERS~~ [andrewsykim]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

andrewsykim · 2019-07-31T19:05:10Z

/hold

just noticed the commit messages, please squash them or group them into some meaningful way

ipvs `getProxyMode` test fails on mac as `utilipvs.GetRequiredIPVSMods` try to reach `/proc/sys/kernel/osrelease` to find version of the running linux kernel. Linux kernel version is used to determine the list of required kernel modules for ipvs. Logic to determine kernel version is moved to GetKernelVersion method in LinuxKernelHandler which implements ipvs.KernelHandler. Mock KernelHandler is used in the test cases. Read and parse file is converted to go function instead of execing cut.

andrewsykim · 2019-07-31T19:17:45Z

/lgtm
/hold cancel

k8s-ci-robot requested review from andrewsykim and dcbw July 26, 2019 13:02

k8s-ci-robot added area/ipvs sig/network Categorizes an issue or PR as relevant to SIG Network. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 26, 2019

ebati force-pushed the issue-80504-test-kubeproxy-mac branch from 5896031 to e46d051 Compare July 26, 2019 13:14

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 26, 2019

k8s-ci-robot assigned andrewsykim and vllry Jul 26, 2019

andrewsykim reviewed Jul 26, 2019

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 26, 2019

andrewsykim reviewed Jul 29, 2019

View reviewed changes

k8s-ci-robot assigned dcbw Jul 30, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 30, 2019

k8s-ci-robot assigned thockin Jul 30, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Jul 31, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 31, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 31, 2019

ebati force-pushed the issue-80504-test-kubeproxy-mac branch from 0fd3683 to 90ce2d5 Compare July 31, 2019 19:12

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2019

k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. and removed do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Jul 31, 2019

k8s-ci-robot merged commit 59142b2 into kubernetes:master Jul 31, 2019

k8s-ci-robot added this to the v1.16 milestone Jul 31, 2019

Add GetKernelVersion to ipvs.KernelHandler interface #80636

Add GetKernelVersion to ipvs.KernelHandler interface #80636

Conversation

ebati commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

k8s-ci-robot commented Jul 26, 2019

andrewsykim commented Jul 26, 2019

andrewsykim commented Jul 26, 2019 • edited

andrewsykim commented Jul 26, 2019

andrewsykim left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewsykim commented Jul 26, 2019

neolit123 commented Jul 26, 2019

ebati commented Jul 27, 2019

andrewsykim commented Jul 29, 2019

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

andrewsykim Jul 29, 2019 • edited

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

Choose a reason for hiding this comment

vllry commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vllry commented Jul 29, 2019

ebati commented Jul 30, 2019

andrewsykim commented Jul 30, 2019

dcbw commented Jul 31, 2019

dcbw commented Jul 31, 2019

k8s-ci-robot commented Jul 31, 2019

andrewsykim commented Jul 31, 2019 • edited

andrewsykim commented Jul 31, 2019

andrewsykim commented Jul 26, 2019 •

edited

andrewsykim Jul 29, 2019 •

edited

andrewsykim commented Jul 31, 2019 •

edited