Send events before adding watchers in traversePluginDir #75110

bertinatto · 2019-03-07T13:38:55Z

What type of PR is this?

/kind bug

What this PR does / why we need it:

This PR changes the function devicemanager.traversePluginDir in order to send events before adding watchers to directories.

Which issue(s) this PR fixes:

Fixes #75097

Does this PR introduce a user-facing change?:

NONE

/sig storage
/sig node
/assign @liggitt

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

bertinatto · 2019-03-11T19:39:49Z

/retest

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

bertinatto · 2019-04-17T08:31:10Z

CC @kubernetes/sig-node-pr-reviews

liggitt · 2019-04-17T13:28:52Z

/skip

liggitt · 2019-04-17T13:31:32Z

all the required CI jobs are green on this PR, ready for node/storage review

bertinatto · 2019-04-17T15:12:36Z

/remove-label do-not-merge/work-in-progress

tallclair · 2019-04-23T17:43:01Z

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

+				Op:   fsnotify.Create,
+			}
+			//TODO: Handle errors by taking corrective measures
+			if err := w.handleCreateEvent(event); err != nil {


Looking at the implementation of handleCreateEvent, I think it's more direct to just do:

if !w.containsBlacklistedDir(path) { return w.traversePluginDir(path) }

Not sure if I follow, do you mean inside handleCreateEvent? If so, then it wouldn't call handlePluginRegistration... or am I missing somehting?

I mean "handle create event" doesn't really seem like what this is doing, unless I'm not following the control flow. If you look at what handle create event is doing, the snippet I commented above is all that gets hit in that function, so I'm suggesting to just inline that piece here.

Ah, OK... I understand now...

This part of the code creates "synthetic create events" if we find a socket file. Then we "handle" this event right here (as opposed to sending it to a processing goroutine like it was before).

Since we're dealing with a socket file, we don't really want to traversePluginDir() it, but instead we want to register the plugin that's listening to this unix domain socket file (i.e., handlePluginRegistration()).

However, we also need to make sure that we ignore black-listed directories (like you pointed in the snippet above) and files prefixed with a "."; this would do it:

if !w.containsBlacklistedDir(path) { if !strings.HasPrefix(path, ".") { // TODO: Handle errors by taking corrective measures if err := w.handlePluginRegistration(path); err != nil { klog.Errorf("error %v when handling create event for file: %s", err, path) } } }

IMO handleCreateEvent() looks like would be a better fit though (since we're almost re-implementing it in-line).

Does this make sense?

Ah, sorry, I completely misread what handleCreateEvent was doing.

tallclair · 2019-04-23T17:44:42Z

pkg/kubelet/util/pluginwatcher/plugin_watcher_test.go

+	select {
+	case <-c:
+		return
+	case <-time.After(2 * time.Second):


Suggested change

case <-time.After(2 * time.Second):

case <-time.After(wait.ForeverTestTimeout):

You can't wait timeout before that in a unit test, as it will flake in the CI environment.

Fixed. I took the chance and fixed the other time.After calls from this file.

tallclair · 2019-04-23T17:49:09Z

pkg/kubelet/util/pluginwatcher/plugin_watcher_test.go

+		defer close(c)
+		require.True(t, waitForEvent(t, exampleEventValidate, hdlr.EventChan(p.pluginName)))
+		require.False(t, waitForPluginRegistrationStatus(t, p.registrationStatus))
+	}()


alternatively, you can just do this serially at the end of the test and rely on the whole test timing out for the failure condition.

Since we're processing plugins serially now, newWatcherWithHandler below would block forever trying to write to the event/registration channels. This goroutine prevents that by receiving info from these channels beforehand.

bertinatto · 2019-04-23T19:44:21Z

/skip

bertinatto · 2019-04-23T19:44:45Z

/retest

Currently, the method `pluginwatcher.traversePluginDir` descends into a directory adding filesystem watchers and creating synthetic `create` events when it finds sockets files. However, a race condition might happen when a recently-added watcher observes a `delete` event in a socket file before `pluginwatcher.traversePlugindir` itself notices this file. This patch changes this behavior by registering watchers on directories, enqueueing and processing `create` events from sockets found, and only then processing the events from the registered watchers.

k8s-ci-robot · 2019-04-23T20:28:23Z

@bertinatto: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
pull-kubernetes-godeps	b7c93d2e234b1c8d995c992a9c8a0518c492a4ee	link	`/test pull-kubernetes-godeps`
pull-kubernetes-cross	b7c93d2e234b1c8d995c992a9c8a0518c492a4ee	link	`/test pull-kubernetes-cross`
pull-kubernetes-e2e-gce-device-plugin-gpu	`f564557`	link	`/test pull-kubernetes-e2e-gce-device-plugin-gpu`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

bertinatto · 2019-04-23T20:33:47Z

/skip

bertinatto · 2019-04-23T20:43:16Z

/retest

sjenning · 2019-04-29T13:26:45Z

/assign @derekwaynecarr

tallclair · 2019-04-29T22:43:01Z

/lgtm

tallclair · 2019-04-29T22:43:22Z

/approve

k8s-ci-robot · 2019-04-29T22:43:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bertinatto, tallclair

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~~ [tallclair]

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

tedyu · 2019-04-30T02:46:43Z

pkg/kubelet/util/pluginwatcher/plugin_watcher.go

+				Op:   fsnotify.Create,
+			}
+			//TODO: Handle errors by taking corrective measures
+			if err := w.handleCreateEvent(event); err != nil {


Shouldn't the err be returned, in the same way error is returned for mode.IsDir() case ?

I created PR #77244

k8s-ci-robot assigned liggitt Mar 7, 2019

k8s-ci-robot requested review from dashpole and sjpotter March 7, 2019 13:39

k8s-ci-robot added the area/kubelet label Mar 7, 2019

liggitt reviewed Mar 7, 2019

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Show resolved Hide resolved

liggitt reviewed Mar 7, 2019

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

bertinatto force-pushed the fix_race_watcher branch 5 times, most recently from 5bb7b99 to 4a6ee18 Compare March 11, 2019 15:53

bertinatto force-pushed the fix_race_watcher branch from 4a6ee18 to 162ad75 Compare March 11, 2019 20:48

bertinatto marked this pull request as ready for review March 12, 2019 07:33

liggitt reviewed Mar 29, 2019

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

liggitt reviewed Mar 29, 2019

View reviewed changes

pkg/kubelet/util/pluginwatcher/plugin_watcher.go Outdated Show resolved Hide resolved

bertinatto force-pushed the fix_race_watcher branch from 4728f40 to b7c93d2 Compare April 2, 2019 08:17

liggitt removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 17, 2019

tallclair reviewed Apr 23, 2019

View reviewed changes

bertinatto force-pushed the fix_race_watcher branch from 5f01ebb to 9d99c7f Compare April 23, 2019 19:38

bertinatto force-pushed the fix_race_watcher branch from 9d99c7f to 5a32553 Compare April 23, 2019 19:59

bertinatto added 2 commits April 23, 2019 22:24

Remove unnecessary in-line function

f564557

bertinatto force-pushed the fix_race_watcher branch from 5a32553 to f564557 Compare April 23, 2019 20:24

k8s-ci-robot assigned derekwaynecarr Apr 29, 2019

k8s-ci-robot assigned tallclair Apr 29, 2019

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

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

k8s-ci-robot merged commit 1608578 into kubernetes:master Apr 30, 2019

tedyu reviewed Apr 30, 2019

View reviewed changes

saad-ali mentioned this pull request May 7, 2019

Add kubelet plugin manager #73891

Merged

This was referenced May 15, 2019

Remove unnecessary waitGroup #77907

Closed

Bugfix: fix chan leak when stop error #77304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Send events before adding watchers in traversePluginDir #75110

Send events before adding watchers in traversePluginDir #75110

bertinatto commented Mar 7, 2019

bertinatto commented Mar 11, 2019

bertinatto commented Apr 17, 2019 •

edited

liggitt commented Apr 17, 2019

liggitt commented Apr 17, 2019

bertinatto commented Apr 17, 2019

tallclair Apr 23, 2019

bertinatto Apr 23, 2019

tallclair Apr 29, 2019

bertinatto Apr 29, 2019

tallclair Apr 29, 2019

tallclair Apr 23, 2019

bertinatto Apr 23, 2019

tallclair Apr 23, 2019

bertinatto Apr 23, 2019

bertinatto commented Apr 23, 2019

bertinatto commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019 •

edited

bertinatto commented Apr 23, 2019

bertinatto commented Apr 23, 2019

sjenning commented Apr 29, 2019

tallclair commented Apr 29, 2019

tallclair commented Apr 29, 2019

k8s-ci-robot commented Apr 29, 2019

tedyu Apr 30, 2019 •

edited

	case <-time.After(2 * time.Second):
	case <-time.After(wait.ForeverTestTimeout):

Send events before adding watchers in traversePluginDir #75110

Send events before adding watchers in traversePluginDir #75110

Conversation

bertinatto commented Mar 7, 2019

bertinatto commented Mar 11, 2019

bertinatto commented Apr 17, 2019 • edited

liggitt commented Apr 17, 2019

liggitt commented Apr 17, 2019

bertinatto commented Apr 17, 2019

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

bertinatto commented Apr 23, 2019

bertinatto commented Apr 23, 2019

k8s-ci-robot commented Apr 23, 2019 • edited

bertinatto commented Apr 23, 2019

bertinatto commented Apr 23, 2019

sjenning commented Apr 29, 2019

tallclair commented Apr 29, 2019

tallclair commented Apr 29, 2019

k8s-ci-robot commented Apr 29, 2019

tedyu Apr 30, 2019 • edited

Choose a reason for hiding this comment

bertinatto commented Apr 17, 2019 •

edited

k8s-ci-robot commented Apr 23, 2019 •

edited

tedyu Apr 30, 2019 •

edited