Support Kubelet PluginWatcher in Windows #81397

ddebroy · 2019-08-14T02:37:41Z

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test

/kind feature

/kind flake

What this PR does / why we need it:
This PR enhances the kubelet plugin watcher to work with files associated with Unix Domain Sockets on Windows by correctly detecting such files using FSCTL_GET_REPARSE_POINT. In Windows, os.ModeSocket does not get set correctly by golang (golang/go#33357).

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:
Once golang adds support for os.ModeSocket for Windows (golang/go#33357), the function IsUnixDomainSocket introduced here may be removed.

Does this PR introduce a user-facing change?:

Support Kubelet plugin watcher on Windows nodes.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

- [KEP]: https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190714-windows-csi-support.md

ddebroy · 2019-08-14T02:37:57Z

/sig windows

ddebroy · 2019-08-14T02:38:25Z

/cc @jingxu97 @wk8

ddebroy · 2019-08-14T02:40:11Z

/assign @tallclair @yujuhong

pkg/kubelet/util/util_unix.go

pkg/kubelet/util/util_windows.go

mattjmcnaughton

Thanks for your pr!

Overall, the unix code here looks good to me. I will leave the review of the windows code to someone more experienced in that realm :)

I'm also curious... are we confident this change is the only one necessary to support the plugin watcher in windows? Do we want some form of integration testing behavior responsible for verifying this feature is working on windows? In part, I think the answer to that could relates to how difficult it would be to add e2e tests. Any thoughts on the difficulty?

ddebroy · 2019-08-14T16:28:18Z

@mattjmcnaughton yes this is the key change necessary in the plugin-watcher to get CSI node plugins working end2end based on a prototyping effort. This is discussed in more details in the KEP.

e2e tests will certainly be put in place for Windows based CSI plugins which will exercise this code but that needs a few other pieces under development now. So the e2e test will be a separate PR.

pjh

Thanks for sending this PR!

pkg/kubelet/util/util_windows.go

pjh · 2019-08-15T01:43:05Z

pkg/kubelet/util/util_windows.go

+		return true, nil
+	}
+	return false, nil
+}


Does this return false or an error when the argument is a named pipe?

I'm not sure if that question even totally makes sense, I just know that named pipes are the other mechanism for IPC with Windows Kubernetes and could perhaps see someone mistakenly passing a named pipe path to this method.

For a named pipe, IsUnixDomainSocket returns false along with a non-nil err as it gets treated similar to an invalid file. Added a unit-test with a named pipe to clarify/check the behavior.

Note that in the context of the plugin watcher/fsnotify, named pipes won't be passed to IsUnixDomainSocket since the watcher is scoped to normal files with the plugin directory

mattjmcnaughton · 2019-08-15T13:15:31Z

On Wed, Aug 14, 2019 at 09:29:09AM -0700, Deep Debroy wrote: @mattjmcnaughton yes this is the key change necessary in the plugin-watcher to get CSI node plugins working end2end. This is discussed in more details in the [KEP](https://github.com/kubernetes/enhancements/blob/master/keps/sig-windows/20190714-windows-csi-support.md#implementation-details). e2e tests will certainly be put in place for Windows based CSI plugins which will exercise this code but that needs a few other pieces under development now. So the e2e test will be a separate PR. -- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: #81397 (comment)

Sounds good to me :) Small nit from me is that the pr title - "Support Kubelet PluginWatcher in Windows" makes it sound as if this diff is the final one needed for the feature to be completely working e2e. It sounds like that isn't quite the case, as we still have some e2e testing PRs to come. But again, a small nit :)

PatrickLang · 2019-08-16T19:49:08Z

/lgtm

wk8

Minor nits

wk8 · 2019-08-18T23:50:04Z

pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher.go

@@ -202,6 +208,12 @@ func (w *Watcher) handleCreateEvent(event fsnotify.Event) error {
 }

 func (w *Watcher) handlePluginRegistration(socketPath string) error {
+	if runtime.GOOS == "windows" {
+		socketPath = strings.Replace(socketPath, "/", "\\", -1)


nit: strings.ReplaceAll better?

wk8 · 2019-08-18T23:53:59Z

pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher.go

+		if strings.HasPrefix(socketPath, "\\") {
+			socketPath = "c:" + socketPath
+		}
+	}


nit: maybe this could be made part of a helper package somewhere? Seems this is the exact same as

kubernetes/pkg/util/mount/mount_windows.go

Lines 250 to 256 in 091a5dc

func normalizeWindowsPath(path string) string {

normalizedPath := strings.Replace(path, "/", "\\", -1)

if strings.HasPrefix(normalizedPath, "\\") {

normalizedPath = "c:" + normalizedPath

}

return normalizedPath

}

and

kubernetes/pkg/volume/util/util.go

Lines 314 to 320 in 091a5dc

func GetWindowsPath(path string) string {

windowsPath := strings.Replace(path, "/", "\\", -1)

if strings.HasPrefix(windowsPath, "\\") {

windowsPath = "c:" + windowsPath

}

return windowsPath

}

.

Maybe that func could live in pkg/kubelet/util/util.go ?

Thanks for highlighting the common functionality - one note, it makes me a little nervous to define a function in pkg/kubelet/util/util.go that is then imported by pkg/volume and pkg/util/mount.

+1 for factoring the function out. Maybe we should have a pkg/util/windows package.

For this PR, I think we can put the helper function in pkg/kubelet/util so that we don't get more code duplication within kubelet.

Done. Moved to pkg/kubelet/util for now.

yujuhong · 2019-08-19T17:09:55Z

pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher.go

+		if strings.HasPrefix(socketPath, "\\") {
+			socketPath = "c:" + socketPath
+		}
+	}


+1 for factoring the function out. Maybe we should have a pkg/util/windows package.

For this PR, I think we can put the helper function in pkg/kubelet/util so that we don't get more code duplication within kubelet.

yujuhong · 2019-08-19T17:19:09Z

pkg/kubelet/util/util_unix_test.go

+	}
+	for _, test := range tests {
+		f, err := ioutil.TempFile("", "test-domain-socket")
+		if err != nil {


nit: import github.com/stretchr/testify/require and use require.NoError

Applies to all the unit tests changes in this PR :-)

yujuhong · 2019-08-19T17:22:51Z

pkg/kubelet/util/util_windows.go

+	// Due to the absence of golang support for os.ModeSocket in Windows (https://github.com/golang/go/issues/33357)
+	// we need to get the Reparse Points (https://docs.microsoft.com/en-us/windows/win32/fileio/reparse-points)
+	// for the file (using FSCTL_GET_REPARSE_POINT) and check for reparse tag: reparseTagSocket
+	fd, err := windows.CreateFile(windows.StringToUTF16Ptr(filePath), windows.GENERIC_READ, 0, nil, windows.OPEN_EXISTING, windows.FILE_FLAG_OPEN_REPARSE_POINT|windows.FILE_FLAG_BACKUP_SEMANTICS, 0)


Would CreateFile leave any side effect?

I see a closeHandle call, does it also require a delete or remove file call?

CreateFile, when invoked with windows.OPEN_EXISTING (as is the case here) returns a handle to an existing file that the deferred CloseHandle will clean up. No other side effects involved. We do not need to delete/remove anything resulting from the CreateFile call as no new file gets created since CreateFile is invoked with windows.OPEN_EXISTING. I will add a comment and explain this to make it clear.

yujuhong · 2019-08-19T17:23:57Z

pkg/kubelet/util/util_windows.go

+	rdbbuf := make([]byte, syscall.MAXIMUM_REPARSE_DATA_BUFFER_SIZE)
+	var bytesReturned uint32
+	// Issue FSCTL_GET_REPARSE_POINT (https://docs.microsoft.com/en-us/windows-hardware/drivers/ifs/fsctl-get-reparse-point)
+	if err = windows.DeviceIoControl(fd, windows.FSCTL_GET_REPARSE_POINT, nil, 0, &rdbbuf[0], uint32(len(rdbbuf)), &bytesReturned, nil); err != nil {


nit: use err :=

yujuhong · 2019-08-19T17:26:55Z

pkg/kubelet/util/util_windows.go

+		return false, errors.Wrap(err, "FSCTL_GET_REPARSE_POINT failed")
+	}
+	rdb := (*reparseDataBuffer)(unsafe.Pointer(&rdbbuf[0]))
+	if rdb.Header.ReparseTag == reparseTagSocket {


nit: return (rdb.Header.ReparseTag == reparseTagSocket), nil

yujuhong · 2019-08-19T17:29:13Z

pkg/kubelet/pluginmanager/pluginwatcher/plugin_watcher.go

@@ -202,6 +208,12 @@ func (w *Watcher) handleCreateEvent(event fsnotify.Event) error {
 }

 func (w *Watcher) handlePluginRegistration(socketPath string) error {
+	if runtime.GOOS == "windows" {


What path format does fsnotify use? Is it just the typical unix /foo/bar/ path?

It reports /foo/bar for paths in Linux and \\foo\\bar for paths in Windows.

yujuhong · 2019-08-19T17:44:31Z

pkg/kubelet/util/util_windows_test.go

+		var pipeln net.Listener
+		var err error
+		if test.listenOnPipe {
+			// generate a random pipe name to listen on


nit: define a helper function getRandomPipeName := func(prefix string, len int) string{... may make the test code more readable

yujuhong · 2019-08-19T17:46:43Z

pkg/kubelet/util/util_windows_test.go

+			expectSocket: false,
+		},
+	}
+	for _, test := range tests {


Hmm.....I think there's too many if/else blocks in the test and the code reuse is not so high... Maybe it'd be easier to write separate test function for each test case?

ddebroy · 2019-08-21T22:01:10Z

@wk8 @yujuhong @jingxu97 @mattjmcnaughton thanks for the comments so far. I have addressed them. PTAL.

yujuhong · 2019-08-22T00:00:44Z

/approve

Leaving some time for others to chime in.

k8s-ci-robot · 2019-08-22T00:01:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ddebroy, yujuhong

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:

~~pkg/kubelet/OWNERS~~ [yujuhong]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ddebroy · 2019-08-23T05:19:17Z

/hold
Seeing some odd results when testing. Will remove hold once the investigation into observed failures is complete.

ddebroy · 2019-08-27T22:28:50Z

/hold cancel

ddebroy · 2019-08-27T22:32:41Z

The reason for the failures observed with the FSCTL logic: when a unix domain socket file is created from within a container (in a path bind-mounted inside the container), the host upon issuing the FSCTL to the file, cannot detect the reparse points associated with unix domain sockets. Most likely the file system filter associated with bind mounting paths in a container is not passing the reparse point data through for files. Switching to a simpler logic that just tries to dial the file works.

pjh · 2019-08-27T23:14:25Z

I tried the new approach for IsUnixDomainSocket in our CSI prototype setup and it worked for me.

/lgtm

Signed-off-by: Deep Debroy <ddebroy@docker.com>

pjh · 2019-08-27T23:33:36Z

/lgtm

PatrickLang · 2019-08-28T01:09:40Z

/retest

k8s-ci-robot added sig/windows Categorizes an issue or PR as relevant to SIG Windows. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 14, 2019

k8s-ci-robot requested review from jingxu97, wk8, tmrts and yujuhong August 14, 2019 02:38

ddebroy changed the title ~~Support Pluginwatcher in Windows~~ Support Kubelet plugin watcher in Windows Aug 14, 2019

k8s-ci-robot added area/kubelet sig/node Categorizes an issue or PR as relevant to SIG Node. labels Aug 14, 2019

k8s-ci-robot assigned tallclair and yujuhong Aug 14, 2019

jingxu97 reviewed Aug 14, 2019

View reviewed changes

pkg/kubelet/util/util_unix.go Outdated Show resolved Hide resolved

jingxu97 reviewed Aug 14, 2019

View reviewed changes

pkg/kubelet/util/util_windows.go Outdated Show resolved Hide resolved

ddebroy changed the title ~~Support Kubelet plugin watcher in Windows~~ Support Kubelet PluginWatcher in Windows Aug 14, 2019

mattjmcnaughton reviewed Aug 14, 2019

View reviewed changes

pjh reviewed Aug 15, 2019

View reviewed changes

k8s-ci-robot assigned PatrickLang Aug 16, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 16, 2019

wk8 reviewed Aug 19, 2019

View reviewed changes

yujuhong reviewed Aug 19, 2019

View reviewed changes

ddebroy mentioned this pull request Aug 20, 2019

Support CSI plugins in Windows kubernetes/enhancements#1122

Closed

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 21, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 22, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 23, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 27, 2019

k8s-ci-robot assigned pjh Aug 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2019

Support PluginWatcher in Windows

1321c91

Signed-off-by: Deep Debroy <ddebroy@docker.com>

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2019

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 27, 2019

k8s-ci-robot merged commit 08b6737 into kubernetes:master Aug 28, 2019

k8s-ci-robot added this to the v1.16 milestone Aug 28, 2019

	func normalizeWindowsPath(path string) string {
	normalizedPath := strings.Replace(path, "/", "\\", -1)
	if strings.HasPrefix(normalizedPath, "\\") {
	normalizedPath = "c:" + normalizedPath
	}
	return normalizedPath
	}

	func GetWindowsPath(path string) string {
	windowsPath := strings.Replace(path, "/", "\\", -1)
	if strings.HasPrefix(windowsPath, "\\") {
	windowsPath = "c:" + windowsPath
	}
	return windowsPath
	}

Support Kubelet PluginWatcher in Windows #81397

Support Kubelet PluginWatcher in Windows #81397

Conversation

ddebroy commented Aug 14, 2019 • edited Loading

ddebroy commented Aug 14, 2019

ddebroy commented Aug 14, 2019

ddebroy commented Aug 14, 2019

mattjmcnaughton left a comment • edited Loading

Choose a reason for hiding this comment

ddebroy commented Aug 14, 2019 • edited Loading

pjh left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

mattjmcnaughton commented Aug 15, 2019 via email

PatrickLang commented Aug 16, 2019

wk8 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ddebroy Aug 21, 2019 • edited Loading

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

ddebroy commented Aug 21, 2019

yujuhong commented Aug 22, 2019

k8s-ci-robot commented Aug 22, 2019

ddebroy commented Aug 23, 2019

ddebroy commented Aug 27, 2019

ddebroy commented Aug 27, 2019

pjh commented Aug 27, 2019

pjh commented Aug 27, 2019

PatrickLang commented Aug 28, 2019

ddebroy commented Aug 14, 2019 •

edited

Loading

mattjmcnaughton left a comment •

edited

Loading

ddebroy commented Aug 14, 2019 •

edited

Loading

ddebroy Aug 15, 2019 •

edited

Loading

ddebroy Aug 21, 2019 •

edited

Loading