kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

pacoxu · 2021-03-26T07:19:29Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

kubeadm auto-detect the cri like below:

I0326 14:02:29.160784 1730154 initconfiguration.go:104] detected and using CRI socket: /run/containerd/containerd.sock

However, kubelet raises a warning

Mar 26 14:27:17 daocloud-1 kubelet[1738482]: W0326 14:27:17.285509 1738482 util_unix.go:103] Using "/run/containerd/containerd.sock" as endpoint is deprecated, please consider using full url format "unix:///run/containerd/containerd.sock".

Which issue(s) this PR fixes:

Fixes kubernetes/kubeadm#2426

Special notes for your reviewer:

avoid the warning for kubeadm + containerd init/join.

similar case:

kubernetes/cluster/gce/config-test.sh

Line 113 in ca91618

    
           export CONTAINER_RUNTIME_ENDPOINT=${KUBE_CONTAINER_RUNTIME_ENDPOINT:-unix:///run/containerd/containerd.sock}

are using 'unix:///run/containerd/containerd.sock'

Does this PR introduce a user-facing change?

ACTION REQUIRED: For existing clusters must "kubectl edit no node-name" and modify the "kubeadm.alpha.kubernetes.io/cri-socket" annotation value for all nodes.

kubeadm: modify the default CRI socket paths on Linux to always be prefixed with "unix://". The kubelet has deprecated sockets without "unix://" it might start erroring out in future releases. For new clusters please update your kubeadm configuration file / command-line flags. For existing clusters must "kubectl edit no node-name" and modify the "kubeadm.alpha.kubernetes.io/cri-socket" annotation value for all nodes.

k8s-ci-robot · 2021-03-26T07:20:18Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pacoxu
To complete the pull request process, please assign fabriziopandini after the PR has been reviewed.
You can assign the PR to them by writing /assign @fabriziopandini in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

cmd/kubeadm/OWNERS

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

pacoxu · 2021-03-26T07:21:45Z

Windows:

kubernetes/cmd/kubeadm/app/util/runtime/runtime_windows.go

Line 27 in cea1d4e

    
           containerdSocket = "//./pipe/containerd-containerd" // Proposed containerd named pipe for Windows

Unix:

kubernetes/cmd/kubeadm/app/util/runtime/runtime_unix.go

Line 27 in cea1d4e

containerdSocket = "/run/containerd/containerd.sock"

cmd/kubeadm/app/util/config/initconfiguration.go

cmd/kubeadm/app/util/runtime/runtime_unix.go

cmd/kubeadm/app/util/runtime/runtime.go

cmd/kubeadm/app/util/runtime/runtime_test.go

RA489 · 2021-12-21T06:36:27Z

/test pull-kubernetes-unit

cmd/kubeadm/app/util/runtime/runtime_windows.go

neolit123 · 2021-12-27T21:00:42Z

cmd/kubeadm/app/util/runtime/runtime_windows.go

+		}
+		return "npipe", fmt.Sprintf("//%s%s", host, u.Path), nil
+	} else if u.Scheme == "" {
+		return "", "", fmt.Errorf("using %q as endpoint is deprecated, please consider using full url format", endpoint)


but this error is actually breaking to existing users. we should make it a warning like on Linux
https://github.com/kubernetes/kubernetes/pull/100578/files#diff-6a2920d18500a74976b64e8e4a365a296ef20cd168a6bd8712316a400b6eabe0R39-R44

so instead i suggest the following:

combine isExistingSocket and parseEndpoint

print warning (like on Linux) for u.Scheme == "" and automatically prepend "npipe"

neolit123 · 2021-12-27T21:00:49Z

cmd/kubeadm/app/util/runtime/runtime_windows.go

+	} else if u.Scheme == "" {
+		return "", "", fmt.Errorf("using %q as endpoint is deprecated, please consider using full url format", endpoint)
+	} else {
+		return u.Scheme, "", fmt.Errorf("protocol %q not supported", u.Scheme)


once you combine isExistingSocket and parseEndpoint, this can be a klog.Warningf and just try to dial the endpoint

neolit123 · 2021-12-27T21:06:19Z

cmd/kubeadm/app/util/runtime/runtime_windows.go

-	_, err := winio.DialPipe(path, nil)
+	_, dialPath, err := parseEndpoint(path)
+	if err != nil {
+		return false


we should handle this error with a warning the same way we do for Linux

if err != nil { klog.Warningf("Could not parse the Windows container runtime endpoint: %v") return false }

neolit123 · 2021-12-27T21:08:49Z

cmd/kubeadm/app/util/runtime/runtime_unix.go

-
-	return fileInfo.Mode()&os.ModeSocket != 0
+	// TODO: remove this warning and Scheme override once paths without scheme are not supported
+	if u.Scheme == "" {


should be if u.Scheme != "unix" ?

neolit123

@pacoxu i had another review pass...i think this still needs some more changes.

if you don't have the time i can take your commit, change it, add Co-authored-by: Paco Xu <email> and we can close this PR. this will also be easier for me to start working on the other changes. WDYT?

…efix

pacoxu · 2021-12-28T03:48:17Z

@pacoxu i had another review pass...i think this still needs some more changes.

if you don't have the time i can take your commit, change it, add Co-authored-by: Paco Xu <email> and we can close this PR. this will also be easier for me to start working on the other changes. WDYT?

@neolit123 Ofcouse is OK. Thanks for reviewing.

I updated the PR and you can open a new one to close this PR if there are still some pending works and you have time. Thanks.

k8s-ci-robot · 2021-12-28T05:26:45Z

@pacoxu: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
pull-kubernetes-unit	`0285bf7`	link	true	`/test pull-kubernetes-unit`

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.

pacoxu · 2021-12-28T05:36:54Z

=== RUN   TestBuildKubeletArgMap/dockershim_socket_and_kubelet_version_with_built-in_dockershim
    flags_test.go:166: failed buildKubeletArgMap:
        	expected: map[network-plugin:cni]
        	  actual: map[container-runtime:remote container-runtime-endpoint:/var/run/dockershim.sock]
    --- FAIL: TestBuildKubeletArgMap/dockershim_socket_and_kubelet_version_with_built-in_dockershim (0.00s)

Test failed for docker shim settings. @neolit123 It seems that you will work on that trick handlings.

neolit123 · 2021-12-28T05:50:56Z

ok, i will send updated PR as soon as possible.

we changed the test for flags here for kubelet < 1.24 vs kubelet >= 1.24:
https://github.com/kubernetes/kubernetes/pull/106973/files#diff-89c847fcc9158087758d81af02ec756653d59d193bf2c0d4b42b7bf5db397109R132-R156

k8s-ci-robot requested review from neolit123 and yagonobre March 26, 2021 07:20

k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Mar 26, 2021

pacoxu force-pushed the full-url-format-containerd branch from 54fddd4 to ace775f Compare March 26, 2021 08:22

neolit123 reviewed Mar 26, 2021

View reviewed changes

cmd/kubeadm/app/util/config/initconfiguration.go Outdated Show resolved Hide resolved

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Mar 27, 2021

pacoxu force-pushed the full-url-format-containerd branch from ace775f to 8c3edd7 Compare March 31, 2021 08:50

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Mar 31, 2021

pacoxu force-pushed the full-url-format-containerd branch 3 times, most recently from d6ff5ec to 75af1f4 Compare March 31, 2021 08:57

pacoxu commented Mar 31, 2021

View reviewed changes

cmd/kubeadm/app/util/runtime/runtime_unix.go Show resolved Hide resolved

pacoxu force-pushed the full-url-format-containerd branch from 75af1f4 to f4123ad Compare March 31, 2021 09:23

neolit123 reviewed Mar 31, 2021

View reviewed changes

cmd/kubeadm/app/util/runtime/runtime.go Outdated Show resolved Hide resolved

neolit123 reviewed Mar 31, 2021

View reviewed changes

cmd/kubeadm/app/util/runtime/runtime_test.go Outdated Show resolved Hide resolved

neolit123 mentioned this pull request Mar 31, 2021

migrate users away from CRI socket paths that don't have URL scheme kubernetes/kubeadm#2426

Closed

4 tasks

k8s-ci-robot added release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 1, 2021

pacoxu marked this pull request as draft December 21, 2021 03:39

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2021

pacoxu force-pushed the full-url-format-containerd branch from 3cc3977 to 37ae668 Compare December 21, 2021 03:45

pacoxu marked this pull request as ready for review December 21, 2021 03:47

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2021

pacoxu marked this pull request as draft December 21, 2021 04:44

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2021

pacoxu force-pushed the full-url-format-containerd branch from 37ae668 to 4bd8054 Compare December 21, 2021 05:15

pacoxu marked this pull request as ready for review December 21, 2021 05:16

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 21, 2021

neolit123 reviewed Dec 27, 2021

View reviewed changes

cmd/kubeadm/app/util/runtime/runtime_windows.go Outdated Show resolved Hide resolved

neolit123 reviewed Dec 27, 2021

View reviewed changes

pacoxu force-pushed the full-url-format-containerd branch from 4bd8054 to dabc720 Compare December 28, 2021 03:21

pacoxu marked this pull request as draft December 28, 2021 03:44

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 28, 2021

kubeadm: add warnning message if the runtime config has no unix:// pr…

0285bf7

…efix

pacoxu marked this pull request as ready for review December 28, 2021 03:47

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 28, 2021

pacoxu force-pushed the full-url-format-containerd branch from dabc720 to 0285bf7 Compare December 28, 2021 03:47

pacoxu closed this Dec 28, 2021

neolit123 mentioned this pull request Jan 3, 2022

kubeadm: ensure CRI endpoints are managed with URL schemes #107295

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

pacoxu commented Mar 26, 2021 •

edited

Loading

k8s-ci-robot commented Mar 26, 2021

pacoxu commented Mar 26, 2021

RA489 commented Dec 21, 2021

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 Dec 27, 2021

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 left a comment •

edited

Loading

pacoxu commented Dec 28, 2021 •

edited

Loading

k8s-ci-robot commented Dec 28, 2021

pacoxu commented Dec 28, 2021

neolit123 commented Dec 28, 2021

kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

kubeadm set --container-runtime-endpoint with prefix 'unix://' #100578

Conversation

pacoxu commented Mar 26, 2021 • edited Loading

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

k8s-ci-robot commented Mar 26, 2021

pacoxu commented Mar 26, 2021

RA489 commented Dec 21, 2021

neolit123 Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

neolit123 Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

neolit123 Dec 27, 2021

Choose a reason for hiding this comment

neolit123 Dec 27, 2021 • edited Loading

Choose a reason for hiding this comment

neolit123 left a comment • edited Loading

Choose a reason for hiding this comment

pacoxu commented Dec 28, 2021 • edited Loading

k8s-ci-robot commented Dec 28, 2021

pacoxu commented Dec 28, 2021

neolit123 commented Dec 28, 2021

pacoxu commented Mar 26, 2021 •

edited

Loading

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 Dec 27, 2021 •

edited

Loading

neolit123 left a comment •

edited

Loading

pacoxu commented Dec 28, 2021 •

edited

Loading