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

WIP: Fix LocalEndpoint on Windows and clean up paths #78671

Closed

Conversation

@PatrickLang
Copy link
Contributor

commented Jun 3, 2019

What type of PR is this?

/kind bug

What this PR does / why we need it:

#77274 enabled the pod-resource endpoint by default, which wasn't tested on Windows. The code as-written creates a malformed path, and starting the kubelet fails.

Which issue(s) this PR fixes:

Fixes #78628

Special notes for your reviewer:

This is the slightly more involved fix when compared with #78670. It shortens the path used on Windows to \\.\pipe\pod-resources\kubelet from \\.\pipe\var\lib\kubelet\pod-resources\kubelet.

(more changes will probably be added to fix bazel)

Does this PR introduce a user-facing change?:

Set pod-resource endpoint to \\.\pipe\pod-resources\kubelet for Windows
@PatrickLang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 3, 2019

/assign @dashpole
/assign @yujuhong
/milestone v1.15
/priority critical-urgent
/sig windows
/sig node

// getPodResourcesDir returns the full path to the directory containing the pod resources socket
func (kl *Kubelet) getPodResourcesDir() string {
return filepath.Join(kl.getRootDir(), config.DefaultKubeletPodResourcesDirName)
}

This comment has been minimized.

Copy link
@yujuhong

yujuhong Jun 4, 2019

Member

Need a newline

This comment has been minimized.

Copy link
@PatrickLang

PatrickLang Jun 4, 2019

Author Contributor

ok, added to all 3 files


// getPodResourcesDir returns the full path to the directory containing the pod resources socket
func (kl *Kubelet) getPodResourcesDir() string {
return filepath.Join("//./pipe", config.DefaultKubeletPodResourcesDirName)

This comment has been minimized.

Copy link
@yujuhong

yujuhong Jun 4, 2019

Member

I think kubelet tries to create this directory. Would that be okay?

This comment has been minimized.

Copy link
@PatrickLang

PatrickLang Jun 4, 2019

Author Contributor

hah, yeah good point. will see if it breaks or not. if it doesn't fail I don't think it will have any effect.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: PatrickLang
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: yujuhong

If they are not already assigned, you can assign the PR to them by writing /assign @yujuhong in a comment when ready.

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

@yujuhong

This comment has been minimized.

Copy link
Member

commented Jun 4, 2019

@PatrickLang is there a presubmit test for Windows that we can run?

PatrickLang added some commits Jun 4, 2019

@PatrickLang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

@yujuhong The problem with presubmit is that we disabled this feature as a mitigation in #78668 and kubernetes-sigs/windows-testing#70 . The presubmit job won't hit this codepath.

@PatrickLang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

Works - I see \\.\pipe\pod-resources\kubelet open. I guess you can mkdir \\.\pipe\pod-resources after all.

@PatrickLang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

release team - we'll discuss whether #78670 or #78671 will proceed tomorrow. We need one of them for sure

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@PatrickLang: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-gce b95fa91 link /test pull-kubernetes-e2e-gce
pull-kubernetes-e2e-gce-device-plugin-gpu b95fa91 link /test pull-kubernetes-e2e-gce-device-plugin-gpu
pull-kubernetes-bazel-build b95fa91 link /test pull-kubernetes-bazel-build
pull-kubernetes-bazel-test b95fa91 link /test pull-kubernetes-bazel-test
pull-kubernetes-e2e-gce-100-performance b95fa91 link /test pull-kubernetes-e2e-gce-100-performance
pull-kubernetes-kubemark-e2e-gce-big b95fa91 link /test pull-kubernetes-kubemark-e2e-gce-big
pull-kubernetes-verify b95fa91 link /test pull-kubernetes-verify

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.

@PatrickLang

This comment has been minimized.

Copy link
Contributor Author

commented Jun 4, 2019

/close
Rather than leaving untested functionality on by default, we're disabling it until device plugins and the needed local endpoints are tested on Windows. We'll merge #78704 instead.

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Jun 4, 2019

@PatrickLang: Closed this PR.

In response to this:

/close
Rather than leaving untested functionality on by default, we're disabling it until device plugins and the needed local endpoints are tested on Windows. We'll merge #78704 instead.

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.

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.