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 port forwarding for windows #75479

Merged
merged 1 commit into from Jun 1, 2019

Conversation

@benmoss
Copy link
Member

commented Mar 19, 2019

What type of PR is this?
/kind feature

What this PR does / why we need it:
Adds support for port forwarding when running kubelet on Windows nodes

Which issue(s) this PR fixes:
Fixes #64531

Special notes for your reviewer:
The only way I could figure out to expose access to the container's network interfaces is by running another container in the pod from which we can use a netcat-like program to proxy the TCP stream. The source for this container is here.

Right now on Windows nodes it is standard to build the pause image locally, as Microsoft documents in this script. I am thinking that we introduce a similar container that we assume is already on the node in order to avoid pulling this image at runtime and maintaining it in a remote registry.

Does this PR introduce a user-facing change?:

Kubelet will attempt to use wincat.exe in the pause container for port forwarding when running on Windows

Looking for feedback since this is a semi-radical departure from how port-forwarding works on Linux.

/hold
/sig windows
/sig node
cc @yujuhong @feiskyer @astrieanna

@benmoss benmoss force-pushed the pivotal-k8s:windows-portforward branch from 465eb1c to 13c6399 Mar 19, 2019

@benmoss benmoss force-pushed the pivotal-k8s:windows-portforward branch 2 times, most recently from 80d4588 to c3fdd06 Mar 19, 2019

@benmoss benmoss added this to Backlog (v.1.15) in SIG-Windows Mar 19, 2019

@benmoss benmoss moved this from Backlog (v.1.15) to In Progress in SIG-Windows Mar 19, 2019

wincat, err := client.CreateContainer(dockertypes.ContainerCreateConfig{
Name: name,
Config: &dockercontainer.Config{
Image: "gcr.io/cf-london-servces-k8s/windows-images/wincat:latest",

This comment has been minimized.

Copy link
@benmoss

benmoss Mar 19, 2019

Author Member

Worth pointing out that this specifically shouldn't be merged, my idea is that this would be something like "wincat" and would be expected to be on the node already and not pulled from a registry, just as the pause image is today

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 19, 2019

Member

hm, but isn't the pause image still pulled on Windows nodes if it's not present?
it was recently added to the official MS registry for that purpose.

This comment has been minimized.

Copy link
@benmoss

benmoss Mar 19, 2019

Author Member

I hadn't heard anything about that. In the SDN code the pause container is still being specified as kubeletwin/pause which doesn't appear to be on Dockerhub

This comment has been minimized.

Copy link
@benmoss

benmoss Mar 19, 2019

Author Member

Looks like it's the same for aks-engine

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 19, 2019

Member

this is the image that i saw (+tested):
mcr.microsoft.com/k8s/core/pause:1.0.0
(ML 1803/1809, nanoserver based)

#74218 (comment)

This comment has been minimized.

Copy link
@feiskyer

feiskyer Mar 20, 2019

Member

+1 of the pause container. @PatrickLang Have we included the wincat.exe in the pause image?

This comment has been minimized.

Copy link
@benmoss

benmoss Mar 20, 2019

Author Member

Good point, we could just use the pause container and exec wincat inside it if it's already inside

This comment has been minimized.

Copy link
@yujuhong

yujuhong Mar 23, 2019

Member

Packing wincat.exe in the pause image seems like a reasonable workaround. Question: how does this affect the resource usage and the stability of the pause container?

I don't think we need to build everything on the node, even though it may be faster. User/vendors can preload the images if they want. Otherwise, distributing the source to nodes become another issue.

BTW, whatever it's decided here may be used by the containerd integration as well.

This comment has been minimized.

Copy link
@yujuhong

yujuhong Mar 23, 2019

Member

Also, the equivalent of pod-level cgroup has not been implemented on Windows, right, @feiskyer @PatrickLang? Just wanted to know how this may affect the resource isolation/accounting in the future.

This comment has been minimized.

Copy link
@feiskyer

feiskyer Mar 26, 2019

Member

Also, the equivalent of pod-level cgroup has not been implemented on Windows, right,

Right.

@yujuhong yujuhong self-assigned this Mar 19, 2019

@neolit123
Copy link
Member

left a comment

/priority important-longterm

wincat, err := client.CreateContainer(dockertypes.ContainerCreateConfig{
Name: name,
Config: &dockercontainer.Config{
Image: "gcr.io/cf-london-servces-k8s/windows-images/wincat:latest",

This comment has been minimized.

Copy link
@neolit123

neolit123 Mar 19, 2019

Member

hm, but isn't the pause image still pulled on Windows nodes if it's not present?
it was recently added to the official MS registry for that purpose.

@benmoss

This comment has been minimized.

Copy link
Member Author

commented Mar 25, 2019

I changed the approach to assume that wincat.exe is in the pause image. Running kubelet with --pod-infra-container-image=gcr.io/cf-london-servces-k8s/wincat/pause, which I created using code from https://github.com/pivotal-k8s/kubernetes/tree/add-wincat

@benmoss

This comment has been minimized.

Copy link
Member Author

commented May 20, 2019

Bumping this, it would be nice to see this make it in.

@benmoss benmoss changed the title [WIP] Implement port forwarding for windows Implement port forwarding for windows May 20, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 30, 2019

@benmoss

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

Just looking this over again after realizing it was actually gonna get merged and had some changes I wanted to make, sorry.

  • Look for wincat on the path, rather than at /wincat.exe. This gives us more flexibility on where on in the fs it makes sense to actually put it.
  • Add a nice error message for when wincat isn't present on the path, since users will likely be seeing this as we settle the pause image standardization problem.
@benmoss

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

/hold cancel

@benmoss

This comment has been minimized.

Copy link
Member Author

commented May 30, 2019

/retest

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

/milestone v1.15
/lgtm

@yujuhong - yes, #75618 is one approach under review to build pause from this repo

@k8s-ci-robot k8s-ci-robot added this to the v1.15 milestone May 30, 2019

@k8s-ci-robot k8s-ci-robot added the lgtm label May 30, 2019

@madhanrm

This comment has been minimized.

Copy link
Contributor

commented May 30, 2019

@yujuhong

This comment has been minimized.

Copy link
Member

commented May 30, 2019

/hold

Implement port forwarding for windows
On Windows the only way to access the container's network interfaces is
by running another process in the pod from which we can use a
netcat-like program to proxy the TCP stream

Proposed wincat.exe can be found here: https://github.com/benmoss/wincat

@benmoss benmoss force-pushed the pivotal-k8s:windows-portforward branch from 02ad9ed to 202841d May 31, 2019

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 31, 2019

@feiskyer
Copy link
Member

left a comment

looks good now.

/lgtm
/hold cancel

@fejta-bot

This comment has been minimized.

Copy link

commented May 31, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@PatrickLang

This comment has been minimized.

Copy link
Contributor

commented May 31, 2019

/retest

looks like failure in bazel?

I0531 19:31:50.758] ================================================================================
I0531 19:31:50.784] COPY RELEASE
I0531 19:31:50.804] ================================================================================
I0531 19:31:50.829]
I0531 19:31:50.838]
I0531 19:31:50.839] push-build.sh::release::gcs::bazel_push_build(): rm -rf /go/src/k8s.io/kubernetes/_output/gcs-stage/v1.16.0-alpha.0.675+a3c5fadd668254
I0531 19:31:50.857]
I0531 19:31:50.858] push-build.sh::release::gcs::bazel_push_build(): mkdir -p /go/src/k8s.io/kubernetes/_output/gcs-stage/v1.16.0-alpha.0.675+a3c5fadd668254
I0531 19:31:54.203] Checking whether gs://kubernetes-release-pull/ci/pull-kubernetes-e2e-gce/v1.16.0-alpha.0.675+a3c5fadd668254 already exists: OK
I0531 19:31:54.225] Publish release artifacts to gs://kubernetes-release-pull using Bazel...
I0531 19:31:54.254] - Hashing and copying public release artifacts to
I0531 19:31:54.257] gs://kubernetes-release-pull/ci/pull-kubernetes-e2e-gce/v1.16.0-alpha.0.675+a3c5fadd668254:
W0531 19:31:54.361] $TEST_TMPDIR defined: output root default is '/bazel-scratch/.cache/bazel' and max_idle_secs default is '15'.
W0531 19:32:01.562]
W0531 19:32:01.563] Server terminated abruptly (error code: 14, error message: '', log file: '/bazel-scratch/.cache/bazel/_bazel_prow/48d5366022b4e3197674c8d6e2bee219/server/jvm.out')
W0531 19:32:01.563]
I0531 19:32:01.664]
I0531 19:32:01.666]
I0531 19:32:01.666] Signal ERR caught!
I0531 19:32:01.668]
I0531 19:32:01.675] Traceback (line function script):
I0531 19:32:01.676] 209 main /go/src/k8s.io/release/push-build.sh

@PatrickLang PatrickLang moved this from In Progress+Review to Done (v1.15) in SIG-Windows May 31, 2019

@fejta-bot

This comment has been minimized.

Copy link

commented Jun 1, 2019

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 89ae84d into kubernetes:master Jun 1, 2019

21 checks passed

cla/linuxfoundation benmoss 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-node-e2e-containerd 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
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.