Fixes to enable Windows CNI #51063

madhanrm · 2017-08-22T00:28:05Z

What this PR does / why we need it:
This PR has fixed which enables Kubelet to use Windows CNI plugin.
Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #
#49646
Special notes for your reviewer:

Release note:

k8s-ci-robot · 2017-08-22T00:28:13Z

Hi @madhanrm. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

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.

freehan · 2017-08-22T23:29:20Z

/ok-to-test

dineshgovindasamy · 2017-08-23T01:13:31Z

Tagging @jdumars for FYI

squeed · 2017-08-24T08:05:39Z

pkg/kubelet/network/cni/cni_windows.go

+	return nil
+}
+
+// GetPodNetworkStatus : Assuming addToNetwork is idempotent, we can call this API as many times as required to get the IPAddress


addToNetwork is definitely not idempotent. The CNI spec doesn't require it and no plugins that I'm aware of implement ADD in this manner. The comment from before is... optimistic.

We're currently working on a spec change for a GET method within CNI.

The plugin that I have is implemented in that way, assuming no one would require adding 2 endpoints of the same type. This also simplifies the implementation of the windows cni plugin, which doesn't have to keep a repo of these data and simply pass these down to the platform based on those assumptions.

Also, as soon as GET method is in CNI, we can switch GetPodNetworkStatus to use the new API.

But it's a plugin system. You cannot guarantee that users will use your plugin.

I agree with squeed here.

The Plugin doesn't enter the namespace and get the IP directly. It calls into Host Networking Service, to get the data. Kubernetes can also do that directly, but then we would destroy the very idea of abstracting that logic out behind the plugin and not bringing in a lot of platform specific APIs.

The main reason behind this approach is that, current containers are launched with process isolation. It could also be launched with hyper-v isolation (not supported in kubernetes but supported by docker and platform). To keep the implementation same across these modes, it is better to call the centralized to CNI plugin to get back the data, the current API would seamlessly support these.

@madhanrm I agree with @squeed. The CNI specification does not declare ADD idempotent. That has nothing to do with the platform at all, that's just how it is. Making Windows act differently is simply confusing. So can we discuss this a bit more so that we all understand the restrictions?

Saying "This is a requirement for Windows CNI Plugins." is a solution to a problem, but there may be other solutions that do not require making ADD idempotent for Windows CNI.

IIRC Windows has no shared "infra" (eg net namespace) container. Instead, every container gets network setup called on it. What I think we're arguing about here, and correct me if I'm wrong, is that in the current PR here we'll get an ADD for the same container/namespace/netns tuple multiple times, which would in fact require idempotency that CNI does not guarantee.

Instead of that, could we figure out some other way to pass the data that your Windows plugin requires to tie the containers together?

One other thing I'm not clear on. What actual CNI plugin are you working on? And why couldn't some other 3rd party Windows plugin have a completely different mechanism for tying containers together, that doesn't require ADD to be idempotent?

@dcbw Just for my own understanding, what is the argument against making ADD idempotent in the spec?

@dcbw
The current Idempotent nature of ADD is only required because any subsequent ADD is used as a GET (PodStatus) to get the IP. Once we have GET implemented, that will not be a criteria for ADD to be idempotent. (But IMO, it doesn't make sense for ADD not to be idempotent, which is a separate discussion altogether)

Also, your understanding of "every container gets network setup" is getting changed by this PR. This PR is also to start using shared infra container, for which CNI is a requirement.

You should also understand the difference between the CNI plugin in Linux and in Windows. You really do not have to write individual CNI plugins on Windows unlike Linux. Instead CNI plugin is like a proxy to program the Host Networking Service (HNS) on Windows, which takes care of everything. The information is simply constructed and passed on to HNS. So a single CNI plugin would work for most of the network modes that can work independently on a single host. Other may develop similar plugins, but they have to go thro HNS, anything other than that would not be supported in Windows.
The idempotency is required here, because the CNI plugin doenst have to manage anything and simply pass it on to HNS. Else, the CNI would also have to manage redundant data as HNS. This would simplify the requirement of the Windows CNI plugins.

Are there any open items here? Can we not relax the idempotent nature of ADD for Windows here?
Am not seeing any progress in this PR.

squeed · 2017-08-24T08:09:19Z

pkg/kubelet/network/cni/cni.go

@@ -319,6 +263,10 @@ func (plugin *cniNetworkPlugin) buildCNIRuntimeConf(podName string, podNs string
 	glog.V(4).Infof("Got netns path %v", podNetnsPath)
 	glog.V(4).Infof("Using netns path %v", podNs)

+	if podNetnsPath == "" {
+		podNetnsPath = "none"


"none" is theoretically a valid netns path on Linux - can you just use the empty string?

Sure, I can remove this change.

jdumars · 2017-08-24T16:37:40Z

/sig windows

jdumars · 2017-08-24T16:39:30Z

/sig node

jdumars · 2017-08-24T16:41:39Z

/sig network

jdumars · 2017-08-24T16:43:58Z

@caseydavenport could you or a delegate from SIG Network take a look at this implementation + #51064

Thanks!

freehan · 2017-08-24T17:30:34Z

Thanks for the PR!

This come up late in the 1.8 cycle and I am not comfortable to put it in. There is no test coverage for windows cni plugin and the test coverage for linux cni plugin is not great either. I would recommend we get it in during early 1.9 cycle and improve test coverage. I would also recommend you to join the sig-network meeting to get involved in feature planning. That would help the feature that you are working on to gain more visibility in the k8s community.

Same applies to #51064

JMesser81 · 2017-08-24T17:54:28Z

@freehan - if you look at the code, these are actually minor changes and we've isolated by separating into Windows and Linux helper function. AFAIK, there is no CNI plugin support on Windows today so this is net-new. We are very close to getting tests to pass.

Can we discuss in sig-network meet-up? This and #51064 ?

madhanrm · 2017-08-25T05:54:23Z

/test pull-kubernetes-e2e-kops-aws

madhanrm · 2017-08-25T05:55:06Z

/test pull-kubernetes-unit

dineshgovindasamy · 2017-10-31T16:44:09Z

@thockin @freehan Could you please review this PR?

freehan · 2017-11-01T18:14:16Z

Will take a look today. Thanks!

freehan · 2017-11-02T00:36:02Z

pkg/kubelet/network/cni/cni.go

-	if err != nil {
-		glog.Errorf("Error while adding to cni lo network: %s", err)
-		return err
+	if plugin.loNetwork != nil {


nit: add a comment to say linux comes with loNetwork and windows doesn't.

Taken care of

freehan · 2017-11-02T00:38:28Z

Just a nit.
/approve

At some point, we might just want to separate cni windows and linux. But it still seems maintainable for now.

yujuhong · 2017-11-02T00:42:35Z

pkg/kubelet/dockershim/helpers_linux.go

+func getNetworkNamespace(c *dockertypes.ContainerJSON) (string, error) {
+	if c.State.Pid == 0 {
+		// Docker reports pid 0 for an exited container.
+		return "", fmt.Errorf("Cannot find network namespace for the terminated container %q", c.ID)


Not your fault, but since you moved it.
s/Cannot/cannot

yujuhong · 2017-11-02T00:45:11Z

pkg/kubelet/dockershim/helpers_unsupported.go

@@ -19,6 +19,7 @@ limitations under the License.
 package dockershim

 import (
+	"fmt"


Add a new line after this to separate imports of builtin packages.

yujuhong · 2017-11-02T00:45:28Z

pkg/kubelet/dockershim/helpers_unsupported.go

@@ -47,3 +48,7 @@ func (ds *dockerService) determinePodIPBySandboxID(uid string) string {
 	glog.Warningf("determinePodIPBySandboxID is unsupported in this build")
 	return ""
 }
+
+func getNetworkNamespace(c *dockertypes.ContainerJSON) (string, error) {
+	return "", fmt.Errorf("Unsupported platform")


s/Unsupported/unsupported

yujuhong · 2017-11-02T00:52:02Z

pkg/kubelet/network/cni/cni_others.go

+// +build !windows
+
+/*
+Copyright 2014 The Kubernetes Authors.


s/2014/2017

yujuhong · 2017-11-02T00:54:41Z

pkg/kubelet/network/cni/cni_windows.go

+// +build windows
+
+/*
+Copyright 2014 The Kubernetes Authors.


s/2014/2017

yujuhong · 2017-11-02T01:09:23Z

pkg/kubelet/dockershim/helpers_windows.go

@@ -47,6 +47,8 @@ func (ds *dockerService) updateCreateConfig(
 	podSandboxID string, securityOptSep rune, apiVersion *semver.Version) error {
 	if networkMode := os.Getenv("CONTAINER_NETWORK"); networkMode != "" {
 		createConfig.HostConfig.NetworkMode = dockercontainer.NetworkMode(networkMode)
+	} else {
+		modifyHostNetworkOptionForContainer(false, podSandboxID, createConfig.HostConfig)


This function is in security_context.go. Calling this in directly seems weird. I understand that this part of code for windows and linux are very different, maybe refactor/clean this up in the future.

yujuhong · 2017-11-02T01:11:41Z

pkg/kubelet/network/cni/cni_windows.go

+		glog.Errorf("Error while cni parsing result: %s", err)
+		return nil, err
+	}
+	// Parse the result and get the IPAddress


nit: I don't see much parsing here, so perhaps you can just remove the comment.

Or maybe the comment was meant for line 51-55?

yujuhong · 2017-11-02T01:13:12Z

pkg/kubelet/network/cni/cni_windows.go

+
+	result, err := plugin.addToNetwork(plugin.getDefaultNetwork(), name, namespace, id, netnsPath)
+
+	glog.V(5).Infof("GetPodNetworkStatus result %+v", result)


Why printing the result before checking the error?

Would like to know the result either way. Error could contain some meaningful result to investigate issues.

yujuhong · 2017-11-02T01:14:18Z

pkg/kubelet/dockershim/helpers_windows.go

+		//
+		// This place is chosen as a hack for now, until we have a better place to do this,
+		// like immediately after ContainerCreation,
+		// Also this is very windows specific for now


remove Also this is very windows specific for now
I think it's pretty clear from the context that this is windows specific.

yujuhong · 2017-11-02T01:15:07Z

pkg/kubelet/dockershim/helpers_windows.go

+		// Using the CONTAINER_NETWORK flag to determine to choose old impl or current impl.
+		// CONTAINER_NETWORK is only kept for backward compatibility, since old OS version doens;t support
+		// Sandbox concept
+		if networkMode := os.Getenv("CONTAINER_NETWORK"); networkMode == "" {


Can you at least document what version relies on this env variable?

yujuhong · 2017-11-02T01:16:40Z

Mostly nits since @freehan already lgtm'd it.

Please ping me for approval after the comments are addressed.

madhanrm · 2017-11-02T23:38:04Z

@yujuhong I have addressed all the comments. But while testing I found an issue in the latest mainline code, where os.Rename is failing with file already in use.
I see that util/store/filestore.go is newly added and func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) is calling fs.Rename at the end of the method, without closing the file (tmpFile.Close()). File closing is done in defer.

Can you check?

Following are part of this commit +++++++++++++++++++++++++++++++++ * Windows CNI Support (1) Support to use --network-plugin=cni (2) Handled platform requirement of calling CNI ADD for all the containers. (2.1) For POD Infra container, netNs has to be empty (2.2) For all other containers, sharing the network namespace of POD container, should pass netNS name as "container:<Pod Infra Container Id>", same as the NetworkMode of the current container (2.3) The Windows CNI plugin has to handle this to call into Platform. Sample Windows CNI Plugin code to be shared soon. * Sandbox support for Windows (1) Sandbox support for Windows. Works only with Docker runtime. (2) Retained CONTAINER_NETWORK as a backward compatibilty flag, to not break existing deployments using it. (3) Works only with CNI plugin enabled. (*) Changes to reinvoke CNI ADD for every new container created. This is hooked up with PodStatus, but would be ideal to move it outside of this, once we have CNI GET support

yujuhong · 2017-11-03T00:59:22Z

Can you check?

Oops...I sent a fix #55034. Sorry about that.

madhanrm · 2017-11-03T01:17:36Z

/test pull-kubernetes-node-e2e

madhanrm · 2017-11-03T01:32:49Z

@yujuhong Validated your fix. Thanks for the quick response.
Can you approve this PR?

yujuhong · 2017-11-03T17:58:01Z

@yujuhong I have addressed all the comments. But while testing I found an issue in the latest mainline code, where os.Rename is failing with file already in use.
I see that util/store/filestore.go is newly added and func writeFile(fs utilfs.Filesystem, path string, data []byte) (retErr error) is calling fs.Rename at the end of the method, without closing the file (tmpFile.Close()). File closing is done in defer.

@madhanrm were you testing on Windows?

madhanrm · 2017-11-03T18:04:35Z

@yujuhong Yes, testing was done on windows.

yujuhong · 2017-11-03T18:19:25Z

/approve
based on @freehan's LGTM

madhanrm · 2017-11-03T18:48:31Z

@freehan @yujuhong can one of you add a lgtm label.

freehan · 2017-11-03T21:06:09Z

/lgtm

k8s-github-robot · 2017-11-03T21:06:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: freehan, jdumars, madhanrm, yujuhong

Associated issue: 49646

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/kubelet/OWNERS~~ [yujuhong]

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 · 2017-11-03T22:17:25Z

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

k8s-github-robot · 2017-11-03T22:45:11Z

Automatic merge from submit-queue (batch tested with PRs 53679, 51063). If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Aug 22, 2017

k8s-github-robot assigned thockin and smarterclayton Aug 22, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Aug 22, 2017

thockin assigned dnardo, freehan and dcbw and unassigned smarterclayton and thockin Aug 22, 2017

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 22, 2017

squeed suggested changes Aug 24, 2017

View reviewed changes

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

jdumars added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Aug 24, 2017

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

k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Aug 24, 2017

madhanrm force-pushed the cniwindows branch from 59f2b52 to 2c0c0f1 Compare August 24, 2017 19:18

freehan reviewed Nov 2, 2017

View reviewed changes

yujuhong reviewed Nov 2, 2017

View reviewed changes

yujuhong self-assigned this Nov 2, 2017

madhanrm mentioned this pull request Nov 2, 2017

K8S guide: error: failed to run Kubelet: failed to create kubelet: Network plugin "cni" failed init: exec: "nsenter": executable file not found in %PATH% microsoft/SDN#136

Closed

gkudra-msft mentioned this pull request Nov 2, 2017

Kubelet.exe Crashes on Windows Server Core #55031

Closed

madhanrm force-pushed the cniwindows branch from f5fd044 to a5f20b1 Compare November 2, 2017 23:40

madhanrm force-pushed the cniwindows branch from a5f20b1 to d7f9e22 Compare November 3, 2017 00:40

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 3, 2017

gkudra-msft mentioned this pull request Nov 3, 2017

Updates Kubernetes guide. microsoft/SDN#139

Merged

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2017

k8s-github-robot merged commit d837a6a into kubernetes:master Nov 3, 2017

bsteciuk mentioned this pull request Dec 1, 2017

Updates for Windows Server version 1709 with K8s v1.8 kubernetes/website#6180

Merged

CriaHu mentioned this pull request Jun 12, 2020

list 404 urls kubernetes/website#21683

Closed


		result, err := plugin.addToNetwork(plugin.getDefaultNetwork(), name, namespace, id, netnsPath)

		glog.V(5).Infof("GetPodNetworkStatus result %+v", result)

Fixes to enable Windows CNI #51063

Fixes to enable Windows CNI #51063

Conversation

madhanrm commented Aug 22, 2017

k8s-ci-robot commented Aug 22, 2017

freehan commented Aug 22, 2017

dineshgovindasamy commented Aug 23, 2017

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

Choose a reason for hiding this comment

jdumars commented Aug 24, 2017

jdumars commented Aug 24, 2017

jdumars commented Aug 24, 2017

jdumars commented Aug 24, 2017

freehan commented Aug 24, 2017

JMesser81 commented Aug 24, 2017

madhanrm commented Aug 25, 2017

madhanrm commented Aug 25, 2017

dineshgovindasamy commented Oct 31, 2017

freehan commented Nov 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

freehan commented Nov 2, 2017

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yujuhong commented Nov 2, 2017

madhanrm commented Nov 2, 2017

yujuhong commented Nov 3, 2017

madhanrm commented Nov 3, 2017

madhanrm commented Nov 3, 2017

yujuhong commented Nov 3, 2017

madhanrm commented Nov 3, 2017

yujuhong commented Nov 3, 2017

madhanrm commented Nov 3, 2017

freehan commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017

k8s-github-robot commented Nov 3, 2017