merge duplicated linux/windows kube-proxy setup code #118017

danwinship · 2023-05-15T15:10:54Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

There is currently a lot of duplicated code between the linux (iptables/ipvs) and windows (winkernel) kube-proxy setup. This extracts it out to the shared code, along with a bit of other platform-related refactoring.

extracted from #117436

Does this PR introduce a user-facing change?

NONE

/sig network

k8s-ci-robot · 2023-05-15T15:11:02Z

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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.

danwinship · 2023-05-15T19:18:43Z

Hm... initial push failed CI because replacing server_others.go with server_linux.go broke the kube-proxy build on OS X, which is surprising given that we don't build kube-proxy on OS X. I "fixed" this, but I feel like this is a build system bug, and I'm wondering if maybe I should just remove the server_others.go→server_linux.go renaming until it's fixed...

aojea · 2023-05-15T20:45:52Z

Hm... initial push failed CI because replacing server_others.go with server_linux.go broke the kube-proxy build on OS X, which is surprising given that we don't build kube-proxy on OS X.

@BenTheElder may have more historical context, or @dims

dims · 2023-05-15T21:24:04Z

For a few tools like verify-typecheck.sh it's easier to at least build these things even if we don't ship them

BenTheElder · 2023-05-15T22:01:50Z

For a few tools like verify-typecheck.sh it's easier to at least build these things even if we don't ship them

IIRC verify-typecheck.sh is setting both GOOS and GOARCH to target the platforms we build for, it shouldn't need to build this for mac.

I have no idea why we'd build kube-proxy for mac, we aren't in releases?

It is nice to have code compile in IDEs and run tests, but if it's calling out to OS specific stuff then that doesn't help a lot.

I would guess we just refactored to add windows support and added !windows where linux was more appropriate.

humblec · 2023-05-16T04:41:56Z

I have no idea why we'd build kube-proxy for mac, we aren't in releases?

@BenTheElder don't we have kube-proxy for Mac in downloads (https://kubernetes.io/releases/download/#container-images)? if it was the thing you were mentioning above.

BenTheElder · 2023-05-16T04:51:51Z

@BenTheElder don't we have kube-proxy for Mac in downloads (https://kubernetes.io/releases/download/#container-images)? if it was the thing you were mentioning above.

Er no? What would a Mac container image look like?

The kube-proxy image in the release notes is only for Linux, not even Windows.
https://explore.ggcr.dev/?image=registry.k8s.io%2Fkube-proxy%3Av1.27.1

We have Linux and Windows binaries for some node components.
We have Linux only for server components (kube-apiserver etc.)
And for macOS we have kubectl and kubectl plugins AFAIK:

$ gsutil ls gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl-convert
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl-convert.cert
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl-convert.sha256
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl-convert.sha512
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl-convert.sig
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl.cert
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl.sha256
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl.sha512
gs://kubernetes-release/release/v1.27.0/bin/darwin/amd64/kubectl.si

uablrek · 2023-05-16T05:11:53Z

I don't understand what's wrong in;

kubernetes > ./hack/update-gofmt.sh
go version go1.20.4 linux/amd64
error: size of cmd/kube-proxy/app/server_others.go changed during reading (from 1311 to 1310 bytes)

humblec · 2023-05-16T06:45:51Z

The kube-proxy image in the release notes is only for Linux, not even Windows. https://explore.ggcr.dev/?image=registry.k8s.io%2Fkube-proxy%3Av1.27.1

We have Linux and Windows binaries for some node components. We have Linux only for server components (kube-apiserver etc.) And for macOS we have kubectl and kubectl plugins AFAIK:

hmmm..looks like above is correct assumption. we can see windows binaries of kube-proxy available @https://www.downloadkubernetes.com though, not sure why those are missing in release notes.

danwinship · 2023-05-16T13:02:47Z

Sorry, I didn't provide a ton of context on the build stuff because I didn't mean for this bug to turn into a discussion of that (see #118020 instead). But to clarify since different people seem to be missing different parts of the context;

hack/lib/golang.sh has a bunch of variables that control which things get built on which platforms. On OS X, we intentionally only build kubectl and e2e.test (more or less). This is not a bug.
Prior to this PR, the Windows kube-proxy backend was handled by cmd/kube-proxy/app/server_windows.go and the Linux-specific backends (iptables/ipvs) were handled by cmd/kube-proxy/app/server_others.go. And in fact, although our normal build processes do not build kube-proxy on OS X, it actually was possible to "make WHAT=cmd/kube-proxy" by hand on OS X, and you'd end up with a copy of the Linux kube-proxy which you could run on OS X and it would immediately error out because it couldn't find an "iptables" binary.
Given that it doesn't make sense to build a Linux-specific kube-proxy backend on non-Linux platforms, I decided to redo the file names and build tags to clarify that the iptables and ipvs kube-proxy backends were only for "linux", not for "others"/"!windows".
Surprisingly, given point 1, making the change in point 3 caused verify-typecheck to fail on OS X, because verify-typecheck was trying to build kube-proxy, and kube-proxy now had missing symbols on any platform except linux and windows. (EDIT: This isn't about GOOS/GOARCH, it's about what directories verify-typecheck chooses to try to build in.)
(For now, I fixed point 4 by adding back a stub server_others.go file so that now you can build and run kube-proxy on OS X and it will immediately error out with a message about not supporting this platform.)

BenTheElder · 2023-05-16T18:17:49Z

I don't know who is depending on this, but for reference @ 3588d09 (current master)

$ git rev-parse HEAD
3588d091ff3549b50e9c993b12b169b4caeb105f

$ uname
Darwin

$ go build -o ./kubelet ./cmd/kubelet

$ ./kubelet
E0516 11:13:54.187405   69372 run.go:74] "command failed" err="failed to construct kubelet dependencies: mkdir /var/lib/kubelet: permission denied"

$ go build -o ./kube-proxy ./cmd/kube-proxy

$ ./kube-proxy
I0516 11:17:04.538852   69859 server.go:233] "Warning, all flags other than --config, --write-config-to, and --cleanup are deprecated, please begin using a config file ASAP"
I0516 11:17:04.539004   69859 server_others.go:75] "Using iptables proxy"
I0516 11:17:04.539251   69859 server.go:531] "Neither kubeconfig file nor master URL was specified, falling back to in-cluster config"
E0516 11:17:04.539272   69859 server.go:484] "Error running ProxyServer" err="unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"
E0516 11:17:04.539290   69859 run.go:74] "command failed" err="unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined"

k8s-ci-robot · 2023-05-22T20:15:06Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danwinship

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~~ [danwinship]

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

danwinship · 2023-05-22T20:15:10Z

I "fixed" this, but I feel like this is a build system bug, and I'm wondering if maybe I should just remove the server_others.go→server_linux.go renaming until it's fixed...

(done)

Move the Linux-specific conntrack setup code into a new "platformSetup" rather than trying to fit it into the generic setup code. Also move metrics registration there.

aojea · 2023-05-24T11:26:17Z

Unexpected final error while getting *v1.Pod: etcdserver: request timed out
At one point, however, the function did return successfully.

weird failures

/test pull-kubernetes-e2e-kind-ipv6

aojea · 2023-05-25T22:30:10Z

cmd/kube-proxy/app/server_others.go

@@ -329,7 +328,6 @@ func newProxyServer(
 		if err != nil {
 			return nil, fmt.Errorf("unable to create proxier: %v", err)
 		}
-		proxymetrics.RegisterMetrics()


this was kind of weird, no? initializing in two different places instead doing just in one, maybe a copy paste thing

aojea · 2023-05-25T22:37:07Z

cmd/kube-proxy/app/server.go

@@ -319,7 +322,7 @@ func (o *Options) Run() error {
 		return cleanupAndExit()
 	}

-	proxyServer, err := NewProxyServer(o)


I hope nobody is importing this

aojea · 2023-05-25T22:43:24Z

/lgtm

k8s-ci-robot · 2023-05-25T22:43:31Z

LGTM label has been added.

Git tree hash: 151d8ac0e7f0e6bcba0e8a8165d657ad8dc2e284

aojea · 2023-05-25T22:43:51Z

cmd/kube-proxy/app/server.go

 	NodeRef       *v1.ObjectReference
 	HealthzServer healthcheck.ProxierHealthUpdater
+	Hostname      string


this could be internal variables

you mean hostname rather than Hostname? yeah, the whole struct is wrong that way

k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/ipvs area/kube-proxy approved Indicates a PR has been approved by an approver from all required OWNERS files. labels May 15, 2023

k8s-ci-robot requested review from bowei and uablrek May 15, 2023 15:11

danwinship force-pushed the kube-proxy-platform branch from d8f80e8 to d8db722 Compare May 15, 2023 19:11

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 15, 2023

danwinship mentioned this pull request May 15, 2023

verify-typecheck checks binaries that don't actually get built #118020

Closed

danwinship force-pushed the kube-proxy-platform branch from d8db722 to 4e1f928 Compare May 22, 2023 20:14

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 22, 2023

danwinship added 2 commits May 23, 2023 16:17

Add ProxyServer.platformSetup

08ce580

Move the Linux-specific conntrack setup code into a new "platformSetup" rather than trying to fit it into the generic setup code. Also move metrics registration there.

Merge duplicated Linux/Windows kube-proxy setup code

6232ac7

danwinship force-pushed the kube-proxy-platform branch from 4e1f928 to 6232ac7 Compare May 23, 2023 20:17

aojea reviewed May 25, 2023

View reviewed changes

k8s-ci-robot assigned aojea May 25, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 25, 2023

aojea reviewed May 25, 2023

View reviewed changes

k8s-ci-robot merged commit d8e9a7b into kubernetes:master May 25, 2023
13 checks passed

k8s-ci-robot added this to the v1.28 milestone May 25, 2023

danwinship deleted the kube-proxy-platform branch May 26, 2023 12:38

danwinship mentioned this pull request Oct 27, 2023

move GetKernelVersion out of pkg/proxy/ipvs #120995

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge duplicated linux/windows kube-proxy setup code #118017

merge duplicated linux/windows kube-proxy setup code #118017

danwinship commented May 15, 2023

k8s-ci-robot commented May 15, 2023

danwinship commented May 15, 2023

aojea commented May 15, 2023

dims commented May 15, 2023

BenTheElder commented May 15, 2023 •

edited

humblec commented May 16, 2023

BenTheElder commented May 16, 2023

uablrek commented May 16, 2023

humblec commented May 16, 2023

danwinship commented May 16, 2023 •

edited

BenTheElder commented May 16, 2023

k8s-ci-robot commented May 22, 2023

danwinship commented May 22, 2023

aojea commented May 24, 2023

aojea May 25, 2023

aojea May 25, 2023

aojea commented May 25, 2023

k8s-ci-robot commented May 25, 2023

aojea May 25, 2023

danwinship May 26, 2023

aojea May 28, 2023

merge duplicated linux/windows kube-proxy setup code #118017

merge duplicated linux/windows kube-proxy setup code #118017

Conversation

danwinship commented May 15, 2023

What type of PR is this?

What this PR does / why we need it:

Does this PR introduce a user-facing change?

k8s-ci-robot commented May 15, 2023

danwinship commented May 15, 2023

aojea commented May 15, 2023

dims commented May 15, 2023

BenTheElder commented May 15, 2023 • edited

humblec commented May 16, 2023

BenTheElder commented May 16, 2023

uablrek commented May 16, 2023

humblec commented May 16, 2023

danwinship commented May 16, 2023 • edited

BenTheElder commented May 16, 2023

k8s-ci-robot commented May 22, 2023

danwinship commented May 22, 2023

aojea commented May 24, 2023

aojea May 25, 2023

Choose a reason for hiding this comment

aojea May 25, 2023

Choose a reason for hiding this comment

aojea commented May 25, 2023

k8s-ci-robot commented May 25, 2023

aojea May 25, 2023

Choose a reason for hiding this comment

danwinship May 26, 2023

Choose a reason for hiding this comment

aojea May 28, 2023

Choose a reason for hiding this comment

BenTheElder commented May 15, 2023 •

edited

danwinship commented May 16, 2023 •

edited