Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implement CRI detection for Windows #78053

Merged
merged 1 commit into from May 28, 2019

Conversation

@ksubrmnn
Copy link
Contributor

commented May 17, 2019

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
This PR implements CRI detection for Windows using named pipes for either Docker or ContainerD. It also fixes up CRI socket validation so that it is compatible with Windows.
Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:
Ran the tests for validation and runtime with no failures.

Manually checked that Docker CRI could be detected on Windows machine. I verified that it was detecting the CRI and not just defaulting to Docker.

I0522 17:30:12.774696    7232 join.go:364] [preflight] found NodeName empty; using OS hostname as NodeName
I0522 17:30:12.836675    7232 initconfiguration.go:105] detected and using CRI socket: npipe:////./pipe/docker_engine
[preflight] Running pre-flight checks

Does this PR introduce a user-facing change?:

kubeadm: implement CRI detection for Windows worker nodes
@@ -233,9 +217,5 @@ func detectCRISocketImpl(isSocket func(string) bool) (string, error) {

// DetectCRISocket uses a list of known CRI sockets to detect one. If more than one or none is discovered, an error is returned.
func DetectCRISocket() (string, error) {

This comment has been minimized.

Copy link
@ksubrmnn

ksubrmnn May 17, 2019

Author Contributor

We may want to change the language from "socket" to "endpoint" throughout this PR

This comment has been minimized.

Copy link
@rosti

rosti May 22, 2019

Member

I agree with that. The term "socket" is not true in the Windows named pipe case, however let's not do that now.
The config format uses the term "CRISocket" too, so changing that is not convenient at this point.

This comment has been minimized.

Copy link
@neolit123

neolit123 May 25, 2019

Member

this was pretty much my conclusion about the existing "socket" naming. we cannot change it at this point.

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

/assign @neolit123


const (
dockerSocket = "//./pipe/docker_engine" // The Docker socket is not CRI compatible
containerdSocket = "//./pipe/containerd-containerd" // Proposed containerd named pipe for Windows

This comment has been minimized.

Copy link
@ksubrmnn

ksubrmnn May 17, 2019

Author Contributor

@PatrickLang is the ContainerD pipe correct?

This comment has been minimized.

Copy link
@rosti

rosti May 22, 2019

Member

According to this - yes.
However, what is more concerning to me are the unix path separators being used here vs windows ones. Is this correct?

This comment has been minimized.

Copy link
@ksubrmnn

ksubrmnn May 22, 2019

Author Contributor

It worked fine with the forward slashes but I changed them to backslashes

This comment has been minimized.

Copy link
@ksubrmnn

ksubrmnn May 23, 2019

Author Contributor

Reverted back to forward slashes per discussion below

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 17, 2019

/assign @rosti

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:crisocket branch 3 times, most recently from 56933b9 to a28268a May 20, 2019

@rosti
Copy link
Member

left a comment

Thanks @ksubrmnn !
Overall looks good. We need to ensure, that this is tested at least once with Docker & containerd on Windows. Though, I don't see any obvious reason for a failure.

Show resolved Hide resolved cmd/kubeadm/app/apis/kubeadm/validation/validation.go Outdated
@@ -233,9 +217,5 @@ func detectCRISocketImpl(isSocket func(string) bool) (string, error) {

// DetectCRISocket uses a list of known CRI sockets to detect one. If more than one or none is discovered, an error is returned.
func DetectCRISocket() (string, error) {

This comment has been minimized.

Copy link
@rosti

rosti May 22, 2019

Member

I agree with that. The term "socket" is not true in the Windows named pipe case, however let's not do that now.
The config format uses the term "CRISocket" too, so changing that is not convenient at this point.


const (
dockerSocket = "//./pipe/docker_engine" // The Docker socket is not CRI compatible
containerdSocket = "//./pipe/containerd-containerd" // Proposed containerd named pipe for Windows

This comment has been minimized.

Copy link
@rosti

rosti May 22, 2019

Member

According to this - yes.
However, what is more concerning to me are the unix path separators being used here vs windows ones. Is this correct?

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:crisocket branch 2 times, most recently from 88040d0 to a3f221d May 22, 2019

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

@rosti I tested this with Docker. Detects the CRI when the docker service is started and works as expected. If I stop the Docker service, it won't detect Docker and will just use it as a default. I verified this by adding some extra logs which I removed for the PR.

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:crisocket branch from a3f221d to 1f30c57 May 22, 2019

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 22, 2019

/retest

@rosti
Copy link
Member

left a comment

Thanks @ksubrmnn !
I wonder if you have tried this with containerd on Windows or was it only with Docker?
Other than the backslashes, if it's working (I can't test this ATM, @neolit123 can you?) it's ok by me.

@@ -22,5 +22,5 @@ const (
// DefaultCACertPath defines default location of CA certificate on Windows
DefaultCACertPath = "C:/etc/kubernetes/pki/ca.crt"
// DefaultUrlScheme defines default socket url prefix
DefaultUrlScheme = "tcp"
DefaultUrlScheme = "npipe"

This comment has been minimized.

Copy link
@rosti

rosti May 23, 2019

Member

Strictly speaking we shouldn't be doing this, but since we don't have a released working version of kubeadm on Windows, that has support for v1beta1, I don't think that someone uses it. Hence, in my view, it's OK to be changed.


const (
dockerSocket = "\\\\.\\pipe\\docker_engine" // The Docker socket is not CRI compatible
containerdSocket = "\\\\.\\pipe\\containerd-containerd" // Proposed containerd named pipe for Windows

This comment has been minimized.

Copy link
@rosti

rosti May 23, 2019

Member

My bad on not being clear enough in my previous comment here.
My concern was that the DialPipe below may not be able to use the path if an unix path separator is used.
Changing it to Windows one is not good, since (if detected) this one may be paired with DefaultUrlScheme to produce an URI and Windows path separators are not valid in there.

This comment has been minimized.

Copy link
@ksubrmnn

ksubrmnn May 23, 2019

Author Contributor

Reverted back to forward slashes. DialPipe can use it

@ksubrmnn ksubrmnn force-pushed the ksubrmnn:crisocket branch from 1f30c57 to 310bafe May 23, 2019

@neolit123

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@ksubrmnn
thanks for the PR.

please add a release note instead of NONE under Does this PR introduce a user-facing change?:
e.g. kubeadm: implement CRI detection for Windows worker nodes.

@rosti
the PR SGTM minus the release note.

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented May 25, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ksubrmnn, neolit123

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:

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

@neolit123

This comment has been minimized.

Copy link
Member

commented May 25, 2019

/priority important-longterm

@rosti

rosti approved these changes May 27, 2019

Copy link
Member

left a comment

/lgtm
/hold
For the release note update as suggested by @neolit123

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

@rosti @neolit123 updated the release note

@neolit123

This comment has been minimized.

Copy link
Member

commented May 27, 2019

/hold cancel

@ksubrmnn

This comment has been minimized.

Copy link
Contributor Author

commented May 28, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit 2f7eaa1 into kubernetes:master May 28, 2019

20 checks passed

cla/linuxfoundation ksubrmnn authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Skipped.
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-storage-slow Skipped.
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@neolit123 neolit123 referenced this pull request May 30, 2019

Open

tracking issue for Windows support #1393

3 of 8 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.