kubeadm: use the CRI for preflights checks #55055

runcom · 2017-11-03T11:01:24Z

Signed-off-by: Antonio Murdaca runcom@redhat.com

What this PR does / why we need it:

Add preflights checks to be performed using crictl and the kubernetes CRI instead of relying on docker.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#285

Special notes for your reviewer:

Release note:

kubeadm: use the CRI for preflights checks

@luxas PTAL

runcom · 2017-11-03T11:10:32Z

/assign @luxas

runcom · 2017-11-03T11:37:02Z

/retest

runcom · 2017-11-06T16:47:31Z

/test pull-kubernetes-unit

runcom · 2017-11-07T09:10:25Z

/test pull-kubernetes-unit

runcom · 2017-11-07T13:57:42Z

@luxas PTAL, I've modified the code to not touch the config apis

runcom · 2017-11-07T14:30:47Z

/retest

runcom · 2017-11-07T15:07:42Z

/retest

runcom · 2017-11-08T14:43:15Z

~~does anybody know why some tests are failing? I ran hack/update-bazel.sh just fine also~~ seems like it passed now 😕

luxas

LGTM, only some small-ish comments/clarifications

luxas · 2017-11-08T14:55:53Z

build/root/BUILD.root

@@ -64,6 +64,12 @@ filegroup(
        "//docs:all-srcs",
        "//examples:all-srcs",
        "//hack:all-srcs",
+        "//kubernetes/cluster:all-srcs",


this seems unrelated?

I've just run hack/update-bazel.sh 😕

I think you should omit it, something was wrong with the autogeneration then:

W1108 17:14:07.326] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/docs': BUILD file not found on package path and referenced by '//:all-srcs'. W1108 17:14:07.502] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/examples': BUILD file not found on package path and referenced by '//:all-srcs'. W1108 17:14:07.503] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/federation/cluster': BUILD file not found on package path and referenced by '//:all-srcs'. W1108 17:14:07.504] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/third_party/htpasswd': BUILD file not found on package path and referenced by '//:all-srcs'. W1108 17:14:07.504] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/cluster': BUILD file not found on package path and referenced by '//:all-srcs'. W1108 17:14:07.505] ERROR: /workspace/k8s.io/kubernetes/BUILD.bazel:56:1: no such package 'kubernetes/hack/lib': BUILD file not found on package path and referenced by '//:all-srcs'.

not sure what's happening here though, I just run hack/update-bazel.sh and these lines are added

luxas · 2017-11-08T15:39:00Z

cmd/kubeadm/app/preflight/checks.go

+
+	// run the docker validator only with dockershim
+	if sysver.CRISocket == "/var/run/dockershim.sock" {
+		// TODO(runcom): move the DockerValidator to actually use the CRI.Version endpoint


can you create an issue to follow this up please?

created kubernetes/kubeadm#533

luxas · 2017-11-08T15:52:01Z

cmd/kubeadm/app/preflight/checks.go

+			InPathCheck{executable: "tc", mandatory: false, exec: execer},
+			InPathCheck{executable: "touch", mandatory: false, exec: execer})
+	}
+	checks = append(checks, criCtlChecker)


is it expected to always look for crictl here?

I'd say yes, read here #55055 (comment)

can you please move this up directly in the list where checks is defined instead of appending it later to the list?

luxas · 2017-11-08T15:53:04Z

cmd/kubeadm/app/preflight/checks.go

+	} else {
+		// assume docker
+		checks = append(checks,
+			ServiceCheck{Service: "docker", CheckIfActive: true},


any specific reason you didn't put all of this on a single line like the CRICheck above?

luxas · 2017-11-08T15:53:20Z

cmd/kubeadm/app/preflight/checks.go

+		InPathCheck{executable: "socat", mandatory: false, exec: execer},
+		InPathCheck{executable: "tc", mandatory: false, exec: execer},
+		InPathCheck{executable: "touch", mandatory: false, exec: execer},
+		criCtlChecker,


is it expected to always look for crictl here?

well, if we want to encourage people to install it and do the check via the CRI, yes. And btw, it's not mandatory :)

luxas · 2017-11-08T15:53:35Z

cmd/kubeadm/app/preflight/checks.go

+	} else {
+		// assume docker
+		checks = append(checks,
+			ServiceCheck{Service: "docker", CheckIfActive: true},


any specific reason you didn't put all of this on a single line like the CRICheck above?

nah, I can change it if you want

luxas · 2017-11-08T16:30:53Z

/test all

runcom · 2017-11-09T10:36:47Z

/retest

k8s-ci-robot · 2017-11-09T10:36:58Z

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

Test name	Commit	Details	Rerun command
pull-kubernetes-cross	b6af7f7601bf4bdcbf74c43f85a2836a416137b5	link	`/test pull-kubernetes-cross`

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.

luxas · 2017-11-09T10:37:40Z

cmd/kubeadm/app/preflight/checks.go

+			InPathCheck{executable: "socat", mandatory: false, exec: execer},
+			InPathCheck{executable: "tc", mandatory: false, exec: execer},
+			InPathCheck{executable: "touch", mandatory: false, exec: execer}),
+			criCtlChecker


I think this results in an compilation error. (no , in the end)
Shouldn't CRI be used in Windows though?

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

luxas

/lgtm
/approve

k8s-github-robot · 2017-11-09T13:15:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: luxas, runcom

Associated issue: 285

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

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-11-09T15:38:44Z

Automatic merge from submit-queue (batch tested with PRs 55380, 55399, 55377, 55055). If you want to cherry-pick this change to another branch, please follow the instructions here.

@luxas

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>. kubeadm init: skip checking cri socket in preflight checks **What this PR does / why we need it**: `kubeadm init` does not need to require `dockershim.sock` to be present. Remove the check for `dockershim.sock`. xref #55055 **Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*: Fixes kubernetes/kubeadm#657 **Special notes for your reviewer**: /area kubeadm /kind bug /assign @luxas /cc @kubernetes/sig-cluster-lifecycle-pr-reviews **Release note**: ```release-note kubeadm init: skip checking cri socket in preflight checks ```

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Nov 3, 2017

k8s-github-robot assigned krousey and errordeveloper Nov 3, 2017

k8s-ci-robot assigned luxas Nov 3, 2017

runcom force-pushed the check-with-crictl branch from ad0ae0d to c805cce Compare November 3, 2017 11:36

runcom force-pushed the check-with-crictl branch from c805cce to 8d583f7 Compare November 6, 2017 14:30

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Nov 6, 2017

runcom force-pushed the check-with-crictl branch from 8d583f7 to 1be7053 Compare November 6, 2017 14:31

runcom force-pushed the check-with-crictl branch 5 times, most recently from 98b2b1a to a1e1cc0 Compare November 7, 2017 14:27

runcom force-pushed the check-with-crictl branch from a1e1cc0 to 95c528a Compare November 7, 2017 14:34

runcom force-pushed the check-with-crictl branch 3 times, most recently from a9863c7 to 2eec36e Compare November 7, 2017 16:35

runcom force-pushed the check-with-crictl branch from 2eec36e to ae9c43a Compare November 8, 2017 14:51

luxas reviewed Nov 8, 2017

View reviewed changes

runcom force-pushed the check-with-crictl branch from ae9c43a to 6e34723 Compare November 9, 2017 10:22

runcom mentioned this pull request Nov 9, 2017

DockerValidator should use the CRI, not the Docker API kubernetes/kubeadm#533

Closed

runcom force-pushed the check-with-crictl branch 2 times, most recently from b6af7f7 to 93f5001 Compare November 9, 2017 10:35

luxas reviewed Nov 9, 2017

View reviewed changes

runcom force-pushed the check-with-crictl branch from 93f5001 to c280a7a Compare November 9, 2017 10:41

kubeadm: use the CRI for preflights checks

46683b7

Signed-off-by: Antonio Murdaca <runcom@redhat.com>

runcom force-pushed the check-with-crictl branch from c280a7a to 46683b7 Compare November 9, 2017 10:42

luxas approved these changes Nov 9, 2017

View reviewed changes

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2017

k8s-github-robot merged commit d215d4b into kubernetes:master Nov 9, 2017

runcom deleted the check-with-crictl branch November 9, 2017 15:40

runcom mentioned this pull request Nov 9, 2017

DockerValidator should use the CRI, not the Docker API #55414

Closed

dixudx mentioned this pull request Feb 2, 2018

kubeadm init: skip checking cri socket in preflight checks #58802

Merged

bart0sh mentioned this pull request Jun 6, 2018

Move runtime details into ContainerRuntime #64611

Merged

kubeadm: use the CRI for preflights checks #55055

kubeadm: use the CRI for preflights checks #55055

Conversation

runcom commented Nov 3, 2017

runcom commented Nov 3, 2017

runcom commented Nov 3, 2017

runcom commented Nov 6, 2017

runcom commented Nov 7, 2017

runcom commented Nov 7, 2017

runcom commented Nov 7, 2017

runcom commented Nov 7, 2017

runcom commented Nov 8, 2017 • edited

luxas 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

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

luxas commented Nov 8, 2017

runcom commented Nov 9, 2017

k8s-ci-robot commented Nov 9, 2017 • edited

luxas Nov 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

luxas left a comment

Choose a reason for hiding this comment

k8s-github-robot commented Nov 9, 2017

k8s-github-robot commented Nov 9, 2017

runcom commented Nov 8, 2017 •

edited

k8s-ci-robot commented Nov 9, 2017 •

edited

luxas Nov 9, 2017 •

edited