A few fixes to defaults on Windows. #374
Conversation
|
Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please follow instructions at https://git.k8s.io/community/CLA.md#the-contributor-license-agreement to sign the CLA. It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.
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. |
k8s-ci-robot
added
cncf-cla: no
|
@Random-Liu - Working on getting the CLA signed. |
|
It seems that the library we are using already supported windows. https://github.com/kubernetes/kubernetes/tree/master/pkg/kubelet/util @jterry75 Doesn't it work? And I think the dockershim windows support today is using that. |
|
@Random-Liu - Only supports TCP see util_windows.go#L48. Given that CRI is local only it should use npipe on Windows. |
|
@jterry75 I see. If containerd is going to provide npipe on Windows, we should eventually update the kubelet util library to support it, thus kubelet can use it. But I'm fine with starting from crictl/critest for development first. :) /cc @feiskyer who knows more about windows. Does |
|
@Random-Liu - I pushed and update that keep the old behavior if indeed people need tcp support for crictl on Windows. This will just inspect the EP and return an dialer for NamedPipes if the EP is a pipe path otherwise uses the util package as it used to. |
|
dockershim on Windows listens to We should update util package to support both tcp and npipe on Windows, so that containerd would work for kubelet. |
|
@feiskyer - I can update it there that's fine. This change would support both tcp and npipe on Windows now though. Do you see any issues with that? |
|
@feiskyer @Random-Liu Here is a change in my private fork of Kubernetes as well: jterry75/kubernetes:a2f144. I assume this is the right long term solution that you were looking for? |
|
@feiskyer - Changes the default to tcp as you requested but leaves support for npipe |
k8s-ci-robot
added
cncf-cla: yes
cmd/crictl/main_unix.go
Outdated
| defaultRuntimeEndpoint = "unix:///var/run/dockershim.sock" | ||
| ) | ||
|
|
||
| func GetAddressAndDialer(endpoint string) (string, func(addr string, timeout time.Duration) (net.Conn, error), error) { |
There was a problem hiding this comment.
Could you also add comments for this function?
Makes the default config location GOOS dependent to support Windows paths if the tool is running on Windows. Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
Sets the default connection endpoint for Windows to be tcp://localhost:3735 to match dockershim. Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
|
@feiskyer - PTAL. Rebased on latest master and added comments as requested. I will also make these comments when updating the kubernetes repo as they don't have any currently. |
|
@jterry75 Thanks. /lgtm |
k8s-ci-robot
added
the
lgtm
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer, jterry75 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 |
k8s-ci-robot
added
the
approved
Makes the default config location GOOS dependent to support Windows
paths if the tool is running on Windows.
Sets the default connection endpoint for Windows to be
\.\pipe\dockershim.