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

Switch windows runtime endpoints to npipe #69516

Merged
merged 1 commit into from Oct 23, 2018

Conversation

@feiskyer
Member

feiskyer commented Oct 8, 2018

What this PR does / why we need it:

Switch default windows runtime endpoints to npipe. tcp endpoints are also supported for backward compatibility.

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 #

Special notes for your reviewer:

Refer kubernetes-sigs/cri-tools#374.

Release note:

Windows runtime endpoints is now switched to 'npipe:////./pipe/dockershim' from 'tcp://localhost:3735'.

/sig windows
/assign @PatrickLang

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 8, 2018

cc @jterry75

/retest

@jterry75

This comment has been minimized.

jterry75 commented Oct 9, 2018

@feiskyer - Can you take a look at: jterry75@a2f1f44. I did npipe support a while back including adding tests and supporting both path types (npipe://./path and \\.\pipe\path). I think that we should merge your commit with this one so we get all of it. Do you agree?

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 10, 2018

npipe://./path and \.\pipe\path

I'd suggest we only support npipe format. The reason it this is an option of kubelet, so it's format should be consistent with all platforms, e.g. unix:... for linux and npipe:... for windows.

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 10, 2018

/retest

@Random-Liu

This comment has been minimized.

Member

Random-Liu commented Oct 10, 2018

@jterry75 I do think npipe:... is more clear to me. Is there any particular reason that \\.\pipe\path should be supported? For example, compatibility? Or in some case only that works?

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 10, 2018

Is there any particular reason that \.\pipe\path should be supported? For example, compatibility?

I think backward compatibility only exists on volumes, e.g. what #69484 did. Kubelet options are new features, there are no backward issues.

@feiskyer feiskyer added this to the v1.13 milestone Oct 10, 2018

@jterry75

This comment has been minimized.

jterry75 commented Oct 10, 2018

@Random-Liu @feiskyer - I am totally fine supporting only npipe:// paths. But we still need this part of the code: jterry75@a2f1f44#diff-ae2806b52080b15d453c30c075eb0bc8R47. And I would prefer you take the test cases that I added for npipe:// paths.

IE:

protocol, addr, err := parseEndpoint("npipe://./pipe/mypipe")
// protocol == "npipe"
// addr == "\\\\.\\pipe\\mypipe"

We need to return addr as the \\.\pipe converted path so that winio.DialPipe or winio.ListenPipe works.

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 11, 2018

@jterry75 Thanks. But the commit you provided doesn't handle npipe:////./pipe/ correctly. I fixed that another way. PTAL

@jterry75

This comment has been minimized.

jterry75 commented Oct 11, 2018

@feiskyer - Your change looks good. Two questions:

  1. Is npipe:////./pipe (4 /'s) even a valid addr? I understand that npipe://./pipe (2 /'s) is valid and should be parse-able though. That would be like putting tcp:////127.0.0.1:8080 right?
  2. Are you saying that winio.DialPipe("//./pipe/mypipe", nil) works? I haven't tried it with forward slashes but I thought Windows paths had to be winio.DialPipe("\\\\.\\pipe\\mypipe", nil) which is why I suggested returning the addr portion from parse normalized.
@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 11, 2018

For your question:

  • npipe:////./pipe is valid only for npipe and the change doesn't influence tcp
  • winio.DialPipe("//./pipe/mypipe", nil) works of course for windows. It is actually same with winio.DialPipe("\\\\.\\pipe\\mypipe", nil)
@jterry75

This comment has been minimized.

jterry75 commented Oct 11, 2018

Perfect. LGTM then

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 12, 2018

@Random-Liu PTAL

btw, cri-tools should be updated after this PR merged. I will update it after this PR got merged.

@@ -30,6 +31,8 @@ func FromApiserverCache(opts *metav1.GetOptions) {
}
func parseEndpoint(endpoint string) (string, string, error) {
// url.Parse doesn't recognize \, so replace with / first.
endpoint = strings.Replace(endpoint, "\\", "/", -1)

This comment has been minimized.

@yujuhong

yujuhong Oct 15, 2018

Contributor

I don't think it's safe to do this on non-windows platforms.

Either use libraries that can handle paths differently on different OS, or rewrite this function in _windows.go
The latter might be good since protocols like unix:// cannot be accepted on windows.

This comment has been minimized.

@feiskyer

feiskyer Oct 16, 2018

Member

Good catch. Thanks.

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Oct 15, 2018

/cc @pjh

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 15, 2018

@yujuhong: GitHub didn't allow me to request PR reviews from the following users: pjh.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @pjh

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.

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M labels Oct 16, 2018

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 16, 2018

@yujuhong Addressed comments. PTAL

@yujuhong

Okay with the overall approach. Would like a windows folk to review the npipe path parsing

return "", "", err
}
if u.Scheme == "tcp" {

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

Hmm... we can just use switch here.

@@ -376,8 +376,8 @@ func (f *KubeletFlags) AddFlags(mainfs *pflag.FlagSet) {
fs.StringVar(&f.ExperimentalMounterPath, "experimental-mounter-path", f.ExperimentalMounterPath, "[Experimental] Path of mounter binary. Leave empty to use the default mount.")
fs.StringSliceVar(&f.AllowedUnsafeSysctls, "allowed-unsafe-sysctls", f.AllowedUnsafeSysctls, "Comma-separated whitelist of unsafe sysctls or unsafe sysctl patterns (ending in *). Use these at your own risk. Sysctls feature gate is enabled by default.")
fs.BoolVar(&f.ExperimentalKernelMemcgNotification, "experimental-kernel-memcg-notification", f.ExperimentalKernelMemcgNotification, "If enabled, the kubelet will integrate with the kernel memcg notification to determine if memory eviction thresholds are crossed rather than polling.")
fs.StringVar(&f.RemoteRuntimeEndpoint, "container-runtime-endpoint", f.RemoteRuntimeEndpoint, "[Experimental] The endpoint of remote runtime service. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'tcp://localhost:3735'")
fs.StringVar(&f.RemoteImageEndpoint, "image-service-endpoint", f.RemoteImageEndpoint, "[Experimental] The endpoint of remote image service. If not specified, it will be the same with container-runtime-endpoint by default. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'tcp://localhost:3735'")
fs.StringVar(&f.RemoteRuntimeEndpoint, "container-runtime-endpoint", f.RemoteRuntimeEndpoint, "[Experimental] The endpoint of remote runtime service. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'npipe:////./pipe/dockershim'")

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

npipe is also supported on windows, no?

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

Also, I guess tcp is supported on both platforms?

This comment has been minimized.

@feiskyer

feiskyer Oct 22, 2018

Member

Good catch. The usage should include such information.

fs.StringVar(&f.RemoteRuntimeEndpoint, "container-runtime-endpoint", f.RemoteRuntimeEndpoint, "[Experimental] The endpoint of remote runtime service. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'tcp://localhost:3735'")
fs.StringVar(&f.RemoteImageEndpoint, "image-service-endpoint", f.RemoteImageEndpoint, "[Experimental] The endpoint of remote image service. If not specified, it will be the same with container-runtime-endpoint by default. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'tcp://localhost:3735'")
fs.StringVar(&f.RemoteRuntimeEndpoint, "container-runtime-endpoint", f.RemoteRuntimeEndpoint, "[Experimental] The endpoint of remote runtime service. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'npipe:////./pipe/dockershim'")
fs.StringVar(&f.RemoteImageEndpoint, "image-service-endpoint", f.RemoteImageEndpoint, "[Experimental] The endpoint of remote image service. If not specified, it will be the same with container-runtime-endpoint by default. Currently unix socket is supported on Linux, and tcp is supported on windows. Examples:'unix:///var/run/dockershim.sock', 'npipe:////./pipe/dockershim'")

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

Same here.

This comment has been minimized.

@feiskyer
See the License for the specific language governing permissions and
limitations under the License.
*/

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

Do we have pre-submit tests running windows unit tests?

This comment has been minimized.

@feiskyer

feiskyer Oct 22, 2018

Member

Not yet. @PatrickLang is driving Node e2e tests for Windows, but it's still running outside. We hope this will be done within v1.13 release cycle.

This comment has been minimized.

@feiskyer

feiskyer Oct 22, 2018

Member

Not yet. @PatrickLang is driving Node e2e tests for Windows, but it's still running outside. We hope this will be done within v1.13 release cycle.

assert.NotNil(t, err, "Expect error during parsing %q", test.endpoint)
continue
}
assert.Nil(t, err, "Expect no error during parsing %q", test.endpoint)

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

require.Nil is better here since you don't want the test to continue if parsing failed.

This comment has been minimized.

@feiskyer
expectedAddr: "//./pipe/mypipe",
},
{
endpoint: "npipe:////./pipe/mypipe2",

This comment has been minimized.

@yujuhong

yujuhong Oct 18, 2018

Contributor

Curious, since I don't know much about the npipe and windows path syntax.
Is ./pipe required and what purpose does it serve?
Also, what does the // prefix do here?

This comment has been minimized.

@feiskyer

feiskyer Oct 22, 2018

Member

Yep, pipe is required in the pipe path ( . is actually the server name for local). The full path format is \\<serverName>\pipe\<pipeName>.

See more here.

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 22, 2018

@yujuhong Addressed comments. PTAL

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Oct 22, 2018

/approve

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Oct 22, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: feiskyer, yujuhong

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

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 23, 2018

/retest

@feiskyer

This comment has been minimized.

Member

feiskyer commented Oct 23, 2018

/test pull-kubernetes-integration

@PatrickLang

This comment has been minimized.

PatrickLang commented Oct 23, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 23, 2018

@k8s-ci-robot k8s-ci-robot merged commit c0974d7 into kubernetes:master Oct 23, 2018

14 of 18 checks passed

pull-kubernetes-e2e-gce Job triggered.
Details
pull-kubernetes-e2e-gce-100-performance Job triggered.
Details
pull-kubernetes-e2e-kops-aws Job triggered.
Details
pull-kubernetes-kubemark-e2e-gce-big Job triggered.
Details
cla/linuxfoundation feiskyer authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details

@feiskyer feiskyer deleted the feiskyer:win-npipe branch Oct 23, 2018

feiskyer added a commit to feiskyer/cri-tools that referenced this pull request Oct 31, 2018

feiskyer added a commit to feiskyer/cri-tools that referenced this pull request Oct 31, 2018

feiskyer added a commit to feiskyer/cri-tools that referenced this pull request Nov 6, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment