flexvolume prober: trigger plugin init only for the relevant plugin #58519

linyouchong · 2018-01-19T13:35:20Z

What this PR does / why we need it:
The automatic discovery trigger init only to the specific plugin directory that was updated, and not to all the plugins in the flexvolume plugin directory.

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

Special notes for your reviewer:
NONE

Release note:

flexvolume: trigger plugin init only for the relevant plugin while probe

linyouchong · 2018-01-19T13:36:24Z

/sig storage
@verult PTAL

verult · 2018-01-20T00:53:25Z

/ok-to-test

verult

Thanks a lot for the PR! My biggest concern is around how plugins are instantiated and how Probe() is implemented.

Probe() is called by the volume plugin manager to discover Flexvolume plugins, so the bulk of probing logic should be in here. Most of the heavy-lifting involving file operations should be in Probe(). This includes things like scanning the plugin directory and instantiating a probed plugin (NewFlexVolumePlugin() calls Init() on the Flexvolume driver, which ultimately executes the driver file and can take arbitrarily long depending on driver implementation). On the other hand, filesystem watcher handler should be kept fast, and I provided some justifications inline with the handler code.

verult · 2018-01-20T00:31:28Z

pkg/volume/plugins.go

+type Operation uint32
+type ProbeEvent struct {
+	Plugin     VolumePlugin // VolumePlugin that was added/updated/removed.
+	PluginName string


Is this necessary?

if Op is Remove, PluginName will be set, and Plugin is nil

verult · 2018-01-20T00:32:28Z

pkg/volume/plugins.go

@@ -35,12 +35,22 @@ import (
 	"k8s.io/kubernetes/pkg/util/mount"
 )

+type Operation uint32


Name should be more specific. Maybe "ProbeOperation"?

nit: Prefer to keep Flex probing-related constructs close to the DynamicPluginProber interface.

verult · 2018-01-20T00:34:18Z

pkg/volume/plugins.go

 const (
 	// Common parameter which can be specified in StorageClass to specify the desired FSType
 	// Provisioners SHOULD implement support for this if they are block device based
 	// Must be a filesystem type supported by the host operating system.
 	// Ex. "ext4", "xfs", "ntfs". Default value depends on the provisioner
 	VolumeParameterFSType = "fstype"
+
+	AddOrUpdate Operation = 1 << iota


Parameter name should be more specific, maybe ProbeAddOrUpdate

verult · 2018-01-20T00:37:39Z

pkg/volume/plugins.go

-				glog.Errorf("Error initializing dynamically probed plugin %s; error: %s",
-					plugin.GetPluginName(), err)
-				continue
+	if len(events) > 0 {


This check isn't necessary

verult · 2018-01-20T00:43:59Z

pkg/volume/flexvolume/probe.go

+	events = []volume.ProbeEvent{}
+	for k, event := range prober.eventsMap {
+		events = append(events, event)
+		delete(prober.eventsMap, k)


I'd advise against modifying a map while iterating over it.

I think it is safe to do this in golang.
https://stackoverflow.com/questions/23229975/is-it-safe-to-remove-selected-keys-from-golang-map-within-a-range-loop

verult · 2018-01-20T00:52:03Z

pkg/volume/flexvolume/probe.go

-const (
-	// TODO (cxing) Tune these params based on test results.
-	// watchEventLimit is the max allowable number of processed watches within watchEventInterval.
-	watchEventInterval = 5 * time.Second


Why is rate limiting removed?

for two reasons:

watchEventInterval is used in func updateProbeNeeded(), but probeNeeded is removed

In my approach, we can't miss any event, because probe() will not scan the whole pluginDir except the first time it is invoked

Rate limiting is still necessary to prevent a malicious actor from overwhelming controller-manager / kubelet with frequent probes. For example, someone could constantly write to a dummy file inside a driver directory and cause every Probe() call to scan that directory. It's designed to drop events exceeding the quota.

But since the rate limiting logic would be quite different with this change, we could add it in a separate PR and remove it here for the time being.

I thought for a long time, and couldn't find any remedy after missing some event. I think we only need to consider the normal situation, malicious attacks as a security issue should be resolved from a higher level.

verult · 2018-01-20T01:15:34Z

pkg/volume/flexvolume/probe.go

+				probeEvent := volume.ProbeEvent{
+					Op: volume.AddOrUpdate,
+				}
+				plugin, pluginErr := prober.factory.NewFlexVolumePlugin(prober.pluginDir, parentInfo.Name())


Plugin should not be initialized inside the filesystem watch handler, for two reasons:

The handler has to be very light, and ideally it only provides a status update for the main Probe() goroutine to process. A heavy handler delays the processing of inotify events.

Typically multiple inotify events are triggered when a file is created or modified. This will cause a driver to be initialized multiple times.

linyouchong · 2018-01-21T02:35:53Z

@verult fixed, PTAL

verult

The bulk of the logic is good. Three themes in my comments:

If there's a file change in any of the subdirectories of a driver directory (regardless of how nested it is), the driver should be re-initialized
Keep locking to a minimum
More unit tests for new functionality

verult · 2018-01-24T00:54:26Z

pkg/volume/plugins.go

-			pm.probedPlugins = append(pm.probedPlugins, plugin)
+			pm.probedPlugins[event.Plugin.GetPluginName()] = event.Plugin
+		} else if event.Op == ProbeRemove {
+			delete(pm.probedPlugins, event.Plugin.GetPluginName())
 		}


nit: Consider logging an error if event.Op is not matched.

verult · 2018-01-24T01:01:57Z

pkg/volume/plugins.go

-			pm.probedPlugins = append(pm.probedPlugins, plugin)
+			pm.probedPlugins[event.Plugin.GetPluginName()] = event.Plugin
+		} else if event.Op == ProbeRemove {
+			delete(pm.probedPlugins, event.Plugin.GetPluginName())


Is event.Plugin going to be nil? If so, please also include comments in the ProbeEvent struct describing this behavior for remove events

verult · 2018-01-24T01:23:30Z

pkg/volume/flexvolume/probe_test.go

@@ -38,24 +38,22 @@ func TestProberExistingDriverBeforeInit(t *testing.T) {
 	driverPath, _, watcher, prober := initTestEnvironment(t)

 	// Act
-	updated, plugins, err := prober.Probe()
+	events, err := prober.Probe()


Could you add some test cases / assertions for existing test cases to make sure the events are correct?

verult · 2018-01-24T01:41:50Z

pkg/volume/flexvolume/probe.go

-const (
-	// TODO (cxing) Tune these params based on test results.
-	// watchEventLimit is the max allowable number of processed watches within watchEventInterval.
-	watchEventInterval = 5 * time.Second


Rate limiting is still necessary to prevent a malicious actor from overwhelming controller-manager / kubelet with frequent probes. For example, someone could constantly write to a dummy file inside a driver directory and cause every Probe() call to scan that directory. It's designed to drop events exceeding the quota.

But since the rate limiting logic would be quite different with this change, we could add it in a separate PR and remove it here for the time being.

verult · 2018-01-24T01:47:07Z

pkg/volume/flexvolume/probe.go

-func (prober *flexVolumeProber) Probe() (updated bool, plugins []volume.VolumePlugin, err error) {
-	probeNeeded := prober.testAndSetProbeNeeded(false)
+func (prober *flexVolumeProber) Probe() (events []volume.ProbeEvent, err error) {
+	prober.mutex.Lock()


Keep locking to a minimum. Currently the filesystem watcher would block waiting for a probe to finish, which is unnecessary. I recommend separating all map and probeAllNeed operations into separate functions and only lock in those functions.

verult · 2018-01-24T02:44:58Z

pkg/volume/flexvolume/probe.go

 		}
 	}

-	prober.updateProbeNeeded()
+	prober.mutex.Lock()


I'd recommend separating out the logic here to a function, too

verult · 2018-01-24T02:46:55Z

pkg/volume/flexvolume/probe.go

+		if prober.probeAllNeeded {
+			return nil
+		}
+		if eventOpIs(event, fsnotify.Create) || eventOpIs(event, fsnotify.Write) || eventOpIs(event, fsnotify.Rename) {


I think it's safer to include Chmod as well, i.e. everything besides Remove

verult · 2018-01-24T19:10:07Z

pkg/volume/flexvolume/probe.go

+	}
+
+	// event about subdir of pluginDirAbs
+	if parentPathAbs == pluginDirAbs {


If anything inside a driver directory changes, even if it's many levels of subdirectories in, the driver should re-initialize. With your logic, newly added subdirectories inside a driver directory won't be watched. I believe the existing logic already works, please correct me if I'm wrong.

Yes, you are right.

verult · 2018-01-24T19:31:04Z

pkg/volume/flexvolume/probe_test.go


 	// Assert
-	assert.True(t, updated)
-	assert.Equal(t, 2, len(plugins)) // Number of plugins should not change.
+	assert.Equal(t, 0, len(events)) // Number of plugins should not change.


Comment no longer applies

verult · 2018-01-24T19:39:01Z

pkg/volume/flexvolume/probe_test.go


 	// Assert
-	assert.True(t, updated)
+	assert.Equal(t, 2, len(events))


Shouldn't it be 1, since it's the same driver?

initTestEnvironment(t) create one driver

linyouchong · 2018-01-26T07:16:15Z

@verult Thank you very much for your reviews, I learned a lot.
I have fixed them, and PTAL!

shay-berman · 2018-03-10T15:41:52Z

Hello

what is the status of this PR?
on which k8s version its going to be merged?

thanks

verult · 2018-03-22T18:23:31Z

Sorry I haven't had the chance to review it; will get to it soon. The current plan is to cherry-pick this back to all PRs since dynamic plugin discovery was introduced, which I believe was 1.8.

verult

The unit tests are great, most of the logic looks good

verult · 2018-03-09T02:18:55Z

pkg/volume/flexvolume/probe_test.go

+	assert.Equal(t, volume.ProbeAddOrUpdate, events[0].Op)
+	assert.NoError(t, err)
+
+	// Call probe again, should 0 event.


"should return 0 event"

verult · 2018-03-10T02:03:50Z

pkg/volume/flexvolume/probe.go

@@ -60,8 +53,8 @@ func GetDynamicPluginProber(pluginDir string) volume.DynamicPluginProber {
 }

 func (prober *flexVolumeProber) Init() error {
-	prober.testAndSetProbeNeeded(true)
-	prober.lastUpdated = time.Now()
+	prober.testAndSetProbeAllNeeded(true)


Is testAndSetProbeAllNeeded() still necessary? Init() and Probe() are never executed at the same time

I try to remove testAndSetProbeNeeded, but pull-kubernetes-bazel-test complaints race detected during execution of test

OK, I'm guessing it's because updateEventMap() reads probeAllNeeded.

verult · 2018-03-10T02:19:37Z

pkg/volume/flexvolume/probe.go

+
+func (prober *flexVolumeProber) probeMap() (events []volume.ProbeEvent, err error) {
+	prober.mutex.Lock()
+	defer prober.mutex.Unlock()


I think locking the entire map is OK for now. In the future I'd like to use something like a concurrent map, where only operations on individual elements are locked, rather than the entire map. If you could leave a TODO that'd help a lot.

Locking the entire map will delay other inotify events from being processed.

verult · 2018-03-23T22:44:17Z

pkg/volume/flexvolume/probe.go

+	}
+
+	// event of subdir of pluginDirAbs
+	if parentPathAbs == pluginDirAbs {


Why is this check necessary? Upon prober initialization and every time the pluginDir is recreated, a recursive watch is added in all of pluginDir

If a driver directory is created under pluginDir after prober initialization, I think we should add a watch, according to the original code: https://github.com/kubernetes/kubernetes/blob/master/pkg/volume/flexvolume/probe.go#L146

But it already does without this check: the check at line 193 (if eventOpIs(event, fsnotify.Create)) applies for a newly created driver directory, since the pluginDir is always under watch.

oh ,got it !

verult · 2018-03-23T22:47:34Z

pkg/volume/flexvolume/probe.go

+
+	// event inside specific driver dir
+	if len(eventRelPathToPluginDir) > 0 {
+		driverDirName := topDirName(eventRelPathToPluginDir)


What about strings.Split(eventRelPathToPluginDir, os.PathSeparator)[0]?

verult · 2018-03-23T23:01:45Z

pkg/volume/flexvolume/probe.go

+	if len(eventRelPathToPluginDir) > 0 {
+		driverDirName := topDirName(eventRelPathToPluginDir)
+		driverDirAbs := filepath.Join(pluginDirAbs, driverDirName)
+		if len(driverDirAbs) > 0 {


I don't think this check is necessary - pluginDirAbs always exists

Yes, the check will be removed

verult · 2018-03-23T23:03:37Z

pkg/volume/flexvolume/probe.go

+		driverDirName := topDirName(eventRelPathToPluginDir)
+		driverDirAbs := filepath.Join(pluginDirAbs, driverDirName)
+		if len(driverDirAbs) > 0 {
+			parts := strings.Split(driverDirName, "~")


nit: Could you separate the logic to compute the path to the executable in a separate helper function? Easier to read that way

verult · 2018-03-23T23:19:30Z

pkg/volume/flexvolume/probe_test.go

-	assert.True(t, updated)
-	assert.Equal(t, 2, len(plugins)) // Number of plugins should not change.
+	assert.Equal(t, 1, len(events))
+	assert.Equal(t, volume.ProbeRemove, events[0].Op)


I'm surprised op isn't ProbeAddOrUpdate, because the driverPath event is triggered last. If this is the case, maybe delete the executable, probe and test, delete the driver dir and probe again

driverPath event handling returns fast, and will not trigger updateEventsMap
https://github.com/linyouchong/kubernetes/blob/0c4d0421bd693a4a28bbb9f72091ca6bd4368ce0/pkg/volume/flexvolume/probe.go#L189

if parentPathAbs == pluginDirAbs {
......................
return nil
}

Gotcha. What if the driver directory gets renamed? The plugin name would change so we'd want to trigger an ProbeAddOrUpdate event. IMO it's safer to not return fast, what do you think?

I think we should trigger ProbeRemove event in 2 cases:

The driverDir is removed

The driver executable is removed
other case we should trigger ProbeAddOrUpdate event

I'm OK with either this or the previous logic (only trigger ProbeRemove when executable is removed). If the dir is removed, the executable will be removed too. This logic would trigger two ProbeRemove events, which is OK.

verult · 2018-03-23T23:27:18Z

pkg/volume/flexvolume/probe_test.go

+	// Arrange
+	_, fs, watcher, _ := initTestEnvironment(t)
+	// Assert
+	assert.Equal(t, 2, len(watcher.watches)) // 2 from initial setup


nit: It's not necessary to test functions written in the test

verult · 2018-03-23T23:31:21Z

pkg/volume/flexvolume/probe_test.go

@@ -154,7 +211,37 @@ func TestRemovePluginDir(t *testing.T) {
 	assert.Equal(t, pluginDir, watcher.watches[len(watcher.watches)-1])
 }

-// Issue multiple events and probe multiple times. Should give true, false, false...
+// Issue an event to remove plugindir. New directory should still be watched.
+func TestNestedDriverDir(t *testing.T) {


Thanks for adding this test!

verult · 2018-03-26T18:27:58Z

pkg/volume/flexvolume/probe.go

+// getExecutableName returns the executableName of a flex plugin
+func getExecutableName(driverDirName string) string {
+	parts := strings.Split(driverDirName, "~")
+	return parts[len(parts)-1]


nit: You could also do the join here and return the abs path :)

verult · 2018-03-28T17:17:38Z

/lgtm

PTAL @chakri-nelluri @jingxu97

@linyouchong we'll eventually need an e2e test to test the driver isolation behavior (here). If you are interested, it'd be very helpful to add it either here or as a separate PR.

linyouchong · 2018-03-29T02:11:21Z

@verult I will add an e2e test as a separate PR.
Does this PR need to be squash?

verult · 2018-03-29T19:04:25Z

Sounds good, and yes please squash the commits. Thanks!

…levant plugin

verult · 2018-03-30T22:45:26Z

/lgtm

verult · 2018-04-02T17:32:53Z

/assign @thockin
for review of pkg/volume/plugins.go

thockin · 2018-04-02T21:38:44Z

/approve

k8s-ci-robot · 2018-04-02T21:38:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: linyouchong, thockin, verult

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/volume/OWNERS~~ [thockin]

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

k8s-github-robot · 2018-04-03T00:07:01Z

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

verult · 2018-04-03T18:01:31Z

@linyouchong after the e2e test is added, please cherry-pick this to 1.8, 1.9, 1.10 as well (instructions here)

linyouchong · 2018-04-04T00:41:05Z

@verult ok, I am working on it

shay-berman · 2018-06-25T03:22:28Z

thanks for fixing this issue
i guess it will be part of k8s v1.11?

btw is the fix relevant only for flexvolume plugins or its generic for all plugins?

verult · 2018-06-25T17:55:08Z

Yep it'll be part of 1.11. @linyouchong any update on the e2e tests?

This is only relevant for Flexvolume - other plugins don't have a filesystem watch for scanning plugins

linyouchong · 2018-06-27T02:55:54Z

@verult I am very sorry for the delay, for some reason, I can't commit my test code right now.
May be one or two weeks later.

verult · 2018-06-30T00:25:37Z

No problem, thanks for the update!

linyouchong · 2018-07-14T08:32:02Z

@verult, e2e test is added, PTAL
#66206

k8s-github-robot assigned jingxu97 and chakri-nelluri Jan 19, 2018

k8s-ci-robot added the sig/storage Categorizes an issue or PR as relevant to SIG Storage. label Jan 19, 2018

linyouchong force-pushed the lyc-probe branch from 30de551 to 68e74ff Compare January 19, 2018 13:41

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 Jan 20, 2018

verult suggested changes Jan 20, 2018

View reviewed changes

linyouchong force-pushed the lyc-probe branch from 2897ad8 to 782af82 Compare January 21, 2018 02:34

linyouchong force-pushed the lyc-probe branch from 782af82 to 8b2ddab Compare January 21, 2018 03:24

verult suggested changes Jan 24, 2018

View reviewed changes

linyouchong force-pushed the lyc-probe branch 4 times, most recently from a752c57 to 0c4d042 Compare January 26, 2018 07:13

verult suggested changes Mar 23, 2018

View reviewed changes

linyouchong force-pushed the lyc-probe branch 2 times, most recently from 13a0a9d to 9d77be6 Compare March 26, 2018 03:14

verult reviewed Mar 26, 2018

View reviewed changes

linyouchong force-pushed the lyc-probe branch from 9d77be6 to ab4b125 Compare March 28, 2018 03:21

k8s-ci-robot assigned verult Mar 28, 2018

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

automatic plugin discovery should trigger plugin init only for the re…

128c07f

…levant plugin

linyouchong force-pushed the lyc-probe branch from ab4b125 to 128c07f Compare March 30, 2018 06:24

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

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

k8s-ci-robot assigned thockin Apr 2, 2018

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

k8s-github-robot merged commit fc19fc9 into kubernetes:master Apr 3, 2018

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Apr 18, 2018

flexvolume prober: trigger plugin init only for the relevant plugin #58519

flexvolume prober: trigger plugin init only for the relevant plugin #58519

Conversation

linyouchong commented Jan 19, 2018 • edited Loading

linyouchong commented Jan 19, 2018

verult commented Jan 20, 2018

verult 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

linyouchong Jan 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

linyouchong commented Jan 21, 2018

verult 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

linyouchong commented Jan 26, 2018 • edited Loading

shay-berman commented Mar 10, 2018

verult commented Mar 22, 2018

verult 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

linyouchong Mar 26, 2018 • 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

linyouchong Mar 26, 2018 • 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

Choose a reason for hiding this comment

linyouchong Mar 26, 2018 • 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

linyouchong Mar 26, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verult commented Mar 28, 2018

linyouchong commented Mar 29, 2018

verult commented Mar 29, 2018

verult commented Mar 30, 2018

verult commented Apr 2, 2018

thockin commented Apr 2, 2018

k8s-ci-robot commented Apr 2, 2018

k8s-github-robot commented Apr 3, 2018

linyouchong commented Jan 19, 2018 •

edited

Loading

linyouchong Jan 26, 2018 •

edited

Loading

linyouchong commented Jan 26, 2018 •

edited

Loading

linyouchong Mar 26, 2018 •

edited

Loading

linyouchong Mar 26, 2018 •

edited

Loading

linyouchong Mar 26, 2018 •

edited

Loading

linyouchong Mar 26, 2018 •

edited

Loading