Add support of hyperv isolation for windows containers #58751

feiskyer · 2018-01-24T14:22:11Z

What this PR does / why we need it:

Add support of hyperv isolation for windows containers.

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 #58750

Special notes for your reviewer:

Only one container per pod is supported yet.

Release note:

Windows containers now support experimental Hyper-V isolation by setting annotation `experimental.windows.kubernetes.io/isolation-type=hyperv` and feature gates HyperVContainer. Only one container per pod is supported yet.

feiskyer · 2018-01-24T14:22:42Z

/sig windows

feiskyer · 2018-01-24T14:26:14Z

/cc @PatrickLang @JiangtianLi @michmike

k8s-ci-robot · 2018-01-24T14:26:16Z

@feiskyer: GitHub didn't allow me to request PR reviews from the following users: PatrickLang, JiangtianLi.

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

In response to this:

/cc @PatrickLang @JiangtianLi @michmike

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.

feiskyer · 2018-01-24T14:27:13Z

/sig node

/cc @yujuhong

JiangtianLi · 2018-01-24T23:24:21Z

pkg/kubelet/dockershim/docker_sandbox.go

@@ -587,6 +587,8 @@ func (ds *dockerService) makeSandboxDockerConfig(c *runtimeapi.PodSandboxConfig,
 		return nil, fmt.Errorf("failed to generate sandbox security options for sandbox %q: %v", c.Metadata.Name, err)
 	}
 	hc.SecurityOpt = append(hc.SecurityOpt, securityOpts...)
+
+	applyExperimentalCreateConfig(createConfig, c.Annotations)


Do we need apply the isolation specific config to sandbox? For Windows Hyper-V container, the POD infra container is not used and needs to be minimal, and therefore process container is sufficient.

It is not used now, but not future. I think we'd better keep sandbox same with containers, WDYT?

That's fine with me. We can reserve it as place holder.

JiangtianLi · 2018-01-24T23:32:18Z

pkg/kubelet/dockershim/helpers_windows.go

@@ -52,6 +70,8 @@ func (ds *dockerService) updateCreateConfig(
 		modifyHostNetworkOptionForContainer(false, podSandboxID, createConfig.HostConfig)


For Hyper-V container, no need to call modifyHostNetworkOptionForContainer.

Ack, will fix

JiangtianLi · 2018-01-24T23:59:44Z

pkg/kubelet/dockershim/helpers_windows.go

-			// Do not return any IP, so that we would continue and get the IP of the Sandbox
-			ds.getIP(sandboxID, r)
+			if r.HostConfig.Isolation == hypervIsolationAnnotationValue {
+				if containerIP := ds.getIP(c.ID, r); containerIP != "" {


Can we add a comment here saying "Return the first non-sandbox container IP as POD IP for Hyper-V container"?

JiangtianLi · 2018-01-25T00:00:57Z

/cc @madhanrm

k8s-ci-robot · 2018-01-25T00:00:57Z

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

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

In response to this:

/cc @madhanrm

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.

JiangtianLi · 2018-01-25T00:07:52Z

pkg/kubelet/dockershim/helpers_windows.go

@@ -29,6 +29,11 @@ import (
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 )

+const (
+	hypervIsolationAnnotationKey   = "windows.kubernetes.io/isolation-type"
+	hypervIsolationAnnotationValue = "hyperv"


Refer to const in https://github.com/moby/moby/blob/master/api/types/container/host_config.go or add a comment with link to where this string comes from?

ack, will add a refer link

JiangtianLi

lgtm

tallclair · 2018-01-27T02:46:15Z

pkg/kubelet/dockershim/helpers_windows.go

@@ -40,18 +47,41 @@ func (ds *dockerService) getSecurityOpts(seccompProfile string, separator rune)
 	return nil, nil
 }

+func shouldIsolatedByHyperV(annotations map[string]string) bool {
+	if v, ok := annotations[hypervIsolationAnnotationKey]; ok {


nit: prefer

v, ok := annotations[hypervIsolationAnnotationKey] return ok && v == hypervIsolation

tallclair · 2018-01-27T02:47:09Z

pkg/kubelet/dockershim/helpers_windows.go

@@ -29,6 +29,13 @@ import (
 	runtimeapi "k8s.io/kubernetes/pkg/kubelet/apis/cri/v1alpha1/runtime"
 )

+const (
+	hypervIsolationAnnotationKey = "windows.kubernetes.io/isolation-type"


Please include experimental in the key. I want to make it clear that this is not an official API, and won't be supported long term -- eventually we will have a native secure container concept that this should move to.

tallclair · 2018-01-27T02:48:01Z

pkg/kubelet/dockershim/helpers_windows.go

+		createConfig.HostConfig.Isolation = hypervIsolation
+
+		if networkMode := os.Getenv("CONTAINER_NETWORK"); networkMode == "" {
+			createConfig.HostConfig.NetworkMode = dockercontainer.NetworkMode("none")


Does this mean that hyperV isolated containers will not have any networking by default?

no, hyperV container's network will be set by CNI plugins

tallclair · 2018-01-27T02:49:22Z

pkg/kubelet/dockershim/helpers_windows.go

-			// Do not return any IP, so that we would continue and get the IP of the Sandbox
-			ds.getIP(sandboxID, r)
+			if r.HostConfig.Isolation == hypervIsolation {
+				// Return the first non-sandbox container IP as POD IP for Hyper-V container.


Can you explain this to me?

As noticed here, HyperV only support one container per Pod yet. And container will receive a different IP from sandbox. So we need to get the real container's IP.

Could you include information that in the comment?

feiskyer · 2018-01-27T14:06:33Z

@tallclair Addressed comments. PTAL

feiskyer · 2018-01-30T01:35:08Z

@yujuhong Addressed comments and squashed. PTAL

tallclair · 2018-01-30T02:22:32Z

Should this be behind a feature gate? I'm on the fence...

feiskyer · 2018-01-30T02:25:58Z

Should this be behind a feature gate? I'm on the fence...

@tallclair I'm thinking only API changes requires feature gates, while the PR doesn't change any API. WDYT?

feiskyer · 2018-01-30T02:53:22Z

/retest

tallclair · 2018-01-30T03:11:27Z

I'm thinking only API changes requires feature gates, while the PR doesn't change any API. WDYT?

No, we require feature gates for changed functionality too. Besides, you are changing the API by adding the annotation (just not the "official" API).

What is the status of windows support? Are there users using it in production? If the answer is yes, this should be feature gated. Otherwise, I'm OK leaving it ungated.

feiskyer · 2018-01-30T03:37:46Z

What is the status of windows support? Are there users using it in production? If the answer is yes, this should be feature gated. Otherwise, I'm OK leaving it ungated.

Yep, there are. OK, let me add a feature gate in kubelet.

feiskyer · 2018-01-30T05:43:50Z

@tallclair Feature gates added. PTAL

michmike · 2018-01-30T17:24:04Z

/lgtm

tallclair · 2018-01-30T22:11:55Z

/lgtm

feiskyer · 2018-01-31T01:49:25Z

/assign @brendandburns

feiskyer · 2018-01-31T01:49:43Z

@brendandburns could you help to take a look?

brendandburns · 2018-01-31T23:24:36Z

/approve

k8s-ci-robot · 2018-01-31T23:24:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, feiskyer, michmike, tallclair

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~pkg/OWNERS~~ [brendandburns]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-01T04:32:05Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-01T05:10:17Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

yuwei1chen · 2023-02-28T08:39:39Z

@feiskyer Hi, we wanted to use this feature gate in v1.20 k8s, met the same issue in https://stackoverflow.com/questions/51634469/kubernetes-windows-hyper-v-no-network-in-pod.
Hyperv containers started do not have network configured. It even do not have a pod ip. Do you have any clues?

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jan 24, 2018

k8s-ci-robot requested review from dashpole and mtaufen January 24, 2018 14:22

k8s-ci-robot added the sig/windows Categorizes an issue or PR as relevant to SIG Windows. label Jan 24, 2018

k8s-ci-robot requested a review from michmike January 24, 2018 14:26

k8s-ci-robot requested a review from yujuhong January 24, 2018 14:27

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Jan 24, 2018

JiangtianLi reviewed Jan 24, 2018

View reviewed changes

JiangtianLi reviewed Jan 25, 2018

View reviewed changes

feiskyer force-pushed the hyperv branch from 7a9434a to 38fa4d2 Compare January 25, 2018 07:55

JiangtianLi approved these changes Jan 25, 2018

View reviewed changes

tallclair self-assigned this Jan 26, 2018

tallclair reviewed Jan 27, 2018

View reviewed changes

yujuhong self-assigned this Jan 29, 2018

feiskyer force-pushed the hyperv branch from 735dbbf to 31abd30 Compare January 30, 2018 01:34

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2018

Add experimental hyperv containers support on Windows

cabd2bb

feiskyer force-pushed the hyperv branch from c6d9914 to 3f5c739 Compare January 30, 2018 04:59

Add HyperVContainer feature gates

a6d0cd0

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 30, 2018

feiskyer force-pushed the hyperv branch from 3f5c739 to a6d0cd0 Compare January 30, 2018 05:00

michmike approved these changes Jan 30, 2018

View reviewed changes

k8s-ci-robot assigned michmike Jan 30, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 30, 2018

k8s-ci-robot assigned brendandburns Jan 31, 2018

bsteciuk approved these changes Jan 31, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 31, 2018

k8s-github-robot merged commit a644e61 into kubernetes:master Feb 1, 2018

feiskyer deleted the hyperv branch February 1, 2018 05:47

This was referenced Feb 22, 2018

Add HyperVContainer feature gates kubernetes/website#7480

Closed

Add HyperVContainer feature gates kubernetes/website#7502

Merged

jsturtevant mentioned this pull request Sep 3, 2020

Remove alpha support for Hyper-v on Docker for Windows #94490

Closed

		@@ -52,6 +70,8 @@ func (ds *dockerService) updateCreateConfig(
		modifyHostNetworkOptionForContainer(false, podSandboxID, createConfig.HostConfig)

Add support of hyperv isolation for windows containers #58751

Add support of hyperv isolation for windows containers #58751

Conversation

feiskyer commented Jan 24, 2018 • edited Loading

feiskyer commented Jan 24, 2018

feiskyer commented Jan 24, 2018

k8s-ci-robot commented Jan 24, 2018

feiskyer commented Jan 24, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiangtianLi commented Jan 25, 2018

k8s-ci-robot commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JiangtianLi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

feiskyer commented Jan 27, 2018

feiskyer commented Jan 30, 2018

tallclair commented Jan 30, 2018

feiskyer commented Jan 30, 2018 • edited Loading

feiskyer commented Jan 30, 2018

tallclair commented Jan 30, 2018

feiskyer commented Jan 30, 2018

feiskyer commented Jan 30, 2018

michmike commented Jan 30, 2018

tallclair commented Jan 30, 2018

feiskyer commented Jan 31, 2018

feiskyer commented Jan 31, 2018

brendandburns commented Jan 31, 2018

k8s-ci-robot commented Jan 31, 2018

k8s-github-robot commented Feb 1, 2018

k8s-github-robot commented Feb 1, 2018

yuwei1chen commented Feb 28, 2023

feiskyer commented Jan 24, 2018 •

edited

Loading

feiskyer commented Jan 30, 2018 •

edited

Loading