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.

Sign up for GitHub

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

Jump to bottom

Initial work on running windows containers on Kubernetes #31707

Merged

k8s-github-robot merged 7 commits into kubernetes:master from apprenda:windows_infra_container

Nov 6, 2016

Contributor

alexbrand commented Aug 30, 2016 •

edited by k8s-oncall

Loading

This is the first stab at getting the Kubelet running on Windows (fixes #30279), and getting it to deploy network-accessible pods that consist of Windows containers. Thanks @csrwng, @jbhurat for helping out.

The main challenge with Windows containers at this point is that container networking is not supported. In other words, each container in the pod will get it's own IP address. For this reason, we had to make a couple of changes to the kubelet when it comes to setting the pod's IP in the Pod Status. Instead of using the infra-container's IP, we use the IP address of the first container.

Other approaches we investigated involved "disabling" the infra container, either conditionally on runtime.GOOS or having a separate windows-docker container runtime that re-implemented some of the methods (would require some refactoring to avoid maintainability nightmare).

Other changes:

The default docker endpoint was removed. This results in the docker client using the default for the specific underlying OS.

More detailed documentation on how to setup the Windows kubelet can be found at https://docs.google.com/document/d/1IjwqpwuRdwcuWXuPSxP-uIz0eoJNfAJ9MWwfY20uH3Q.

cc: @ikester @brendandburns @jstarks

This change is

googlebot commented Aug 30, 2016

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please let us know the company's name.

googlebot added the cla: no label

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

8 similar comments

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-bot commented Aug 30, 2016

Can one of the admins verify that this patch is reasonable to test? If so, please reply "ok to test".
(Note: "add to whitelist" is no longer supported. Please update configurations in kubernetes/test-infra/jenkins/job-configs/kubernetes-jenkins-pull instead.)

This message will repeat several times in short succession due to jenkinsci/ghprb-plugin#292. Sorry.

k8s-github-robot assigned pmorie

k8s-github-robot added size/L release-note-label-needed labels

brendandburns self-assigned this

Contributor

brendandburns commented Aug 30, 2016

@k8s-bot ok to test

@jbhurat can you validate that you have signed the CLA? I know that Alex and Caesar have since I can see their commit history...

brendandburns reviewed

View reviewed changes

pkg/apis/componentconfig/v1alpha1/defaults.go

               	if obj.DockerExecHandlerName == "" {
               		obj.DockerExecHandlerName = "native"
               	}
-              	if obj.DockerEndpoint == "" {

Contributor

brendandburns Aug 30, 2016

It seems like a better answer is conditionalize this on !windows

Contributor

jbhurat commented Aug 31, 2016

I signed it! (Covered under Corporate Agreement - Apprenda)

googlebot commented Aug 31, 2016

We found a Contributor License Agreement for you (the sender of this pull request) and all commit authors, but as best as we can tell these commits were authored by someone else. If that's the case, please add them to this pull request and have them confirm that they're okay with these commits being contributed to Google. If we're mistaken and you did author these commits, just reply here to confirm.

Contributor

csrwng commented Aug 31, 2016

@alexbrand I agree my commits can be used. Not sure how to tell that to k8s-bot.

k8s-github-robot added the needs-rebase label

alexbrand force-pushed the windows_infra_container branch from a4e3892 to 5ae8964 Compare

September 1, 2016 16:32

k8s-github-robot removed the needs-rebase label

Contributor Author

alexbrand commented Sep 1, 2016

Looks like the build ran into #27462 here.

k8s-bot commented Sep 1, 2016

GCE e2e build/test passed for commit 5ae8964.

k8s-github-robot added the needs-rebase label

brendandburns added cla: human-approved and removed cla: no labels

Contributor

k8s-ci-robot commented Oct 25, 2016

Jenkins verification failed for commit 1b8804b. Full PR test history.

The magic incantation to run this job again is @k8s-bot verify test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

k8s-github-robot added the needs-rebase label

csrwng and others added 6 commits

October 31, 2016 14:20


          Initial windows container runtime


          Changes to kubelet to support win containers


          Restore check when getting container IP

9ef3528


          Moved getSecurityOpts() function to OS specific manager variant

a659ac9


          Fixing Volumes on Windows

66a1ef2


          Fixed kubelet build.

9e6815e

pires force-pushed the windows_infra_container branch from 1b8804b to 9e6815e Compare

November 1, 2016 21:13

Contributor

pires commented Nov 1, 2016 •

edited

Loading

@brendandburns is it needed for both CLAs, Google and CNCF? Both @jbhurat and @csrwng are failing the Google one 🤔

k8s-github-robot removed the needs-rebase label

Contributor

k8s-ci-robot commented Nov 1, 2016

Jenkins unit/integration failed for commit 9e6815e. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.


          Volume mount with colon in path is now allowed because it's how it wo…

6daab26

…rks on Windows.

brendandburns added release-note and removed cla: no labels

brendandburns changed the title ~~WIP - Initial work on running windows containers on Kubernetes~~ Initial work on running windows containers on Kubernetes

Contributor

brendandburns commented Nov 3, 2016

This LGTM, with some small comments.

brendandburns reviewed

View reviewed changes

pkg/api/validation/validation.go

               		}
               		if len(mnt.MountPath) == 0 {
               			allErrs = append(allErrs, field.Required(idxPath.Child("mountPath"), ""))
-              		} else if strings.Contains(mnt.MountPath, ":") {

Contributor

brendandburns Nov 3, 2016

doesn't seem like this should be removed?

Contributor

brendandburns Nov 3, 2016

or at least conditionalize this on windows?

Contributor

pires Nov 3, 2016

My question here is, where exactly does this runs, the kubelet? I was under the impression this validation would happen at the API server and having that said, just couldn't conditionalize on Windows, e.g. if GOOS=="windows".

Contributor

brendandburns Nov 4, 2016

ah, good point. Can you instead make this check deeper?

e.g. `[A-Za-z]:' is ok, anything else isn't. Also if there are any forward slashes '/' then there better not be any colons.

Basically heuristically guess if it is a windows or linux path and then do the ':' validation if it looks like a Linux path.

Contributor

pires Nov 4, 2016

On it.

Contributor

pires Nov 4, 2016

Actually, it seems that:

c:/my/path works with Windows Containers (there's some translation in place)
/my:path is valid Unix path

Please, advise.

Contributor

pires Nov 4, 2016

As agreed on Slack between me and @brendandburns, this is without effect.

Contributor

brendandburns Nov 4, 2016

Ok, yeah, this is fine with me given that /foo:bar/baz is apparently a valid unix path. It's unclear why this check was there to begin with.

pkg/kubelet/dockertools/docker_manager.go

               	return fmtOpts, nil
               }
-              func (dm *DockerManager) getSecurityOpts(pod *api.Pod, ctrName string) ([]dockerOpt, error) {

Contributor

brendandburns Nov 3, 2016

Can you restore this function to this location so that we minimize the diff?

Contributor

brendandburns Nov 3, 2016

(ah, nm you moved this out to a different file...)

Contributor

pires Nov 3, 2016

Yeah, needs different impls for now.

jbhurat mentioned this pull request

Powershell script to start kubelet and kube-proxy #36250

Merged

brendandburns added the lgtm label

Contributor

brendandburns commented Nov 4, 2016

LGTM

Contributor

pires commented Nov 5, 2016

@brendandburns can you label this to be released with 1.5?

k8s-github-robot commented Nov 6, 2016

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot commented Nov 6, 2016

Automatic merge from submit-queue

k8s-github-robot merged commit 486a1ad into kubernetes:master

pires deleted the windows_infra_container branch

November 6, 2016 17:14

pires restored the windows_infra_container branch

November 7, 2016 17:58

chentao1596 mentioned this pull request

WIP:kubelet: support multi-headers when getting pod from HTTP source #38089

Closed

k8s-github-robot pushed a commit that referenced this pull request


          Merge pull request #36250 from apprenda/win_kube_scripts

920d8fd

Automatic merge from submit-queue

Powershell script to start kubelet and kube-proxy

**What this PR does / why we need it**:
This PR adds a powershell script to run kubelet and kube-proxy on Windows. It expects the required arguments like `API Server` location and uses appropriate defaults.

**Which issue this PR fixes** : 
fixes # #34270

**Special notes for your reviewer**:
This PR is for supporting Windows Server Containers for k8s, the work for which is covered under kubernetes/enhancements#116
This PR should be merged after #31707 and #36079 PRs are merged

**Release note**:

```release-note
```

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

Labels

lgtm release-note size/L