Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from
Apr 3, 2018

Conversation

linyouchong
Copy link
Contributor

@linyouchong linyouchong commented Jan 19, 2018

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. 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 Jan 19, 2018
@linyouchong
Copy link
Contributor Author

/sig storage
@verult PTAL

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

verult commented Jan 20, 2018

/ok-to-test

@k8s-ci-robot 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
Copy link
Contributor

@verult verult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

type Operation uint32
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Name should be more specific. Maybe "ProbeOperation"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parameter name should be more specific, maybe ProbeAddOrUpdate

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check isn't necessary

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is rate limiting removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for two reasons:

  1. watchEventInterval is used in func updateProbeNeeded(), but probeNeeded is removed
  2. In my approach, we can't miss any event, because probe() will not scan the whole pluginDir except the first time it is invoked

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@linyouchong linyouchong Jan 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

  1. 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.
  2. Typically multiple inotify events are triggered when a file is created or modified. This will cause a driver to be initialized multiple times.

@linyouchong
Copy link
Contributor Author

@verult fixed, PTAL

Copy link
Contributor

@verult verult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

// Act
updated, plugins, err := prober.Probe()
events, err := prober.Probe()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}

prober.updateProbeNeeded()
prober.mutex.Lock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

// event about subdir of pluginDirAbs
if parentPathAbs == pluginDirAbs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, you are right.


// 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.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment no longer applies


// Assert
assert.True(t, updated)
assert.Equal(t, 2, len(events))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

initTestEnvironment(t) create one driver

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

linyouchong commented Jan 26, 2018

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

@shay-berman
Copy link

Hello

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

thanks

@verult
Copy link
Contributor

verult commented Mar 22, 2018

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.

Copy link
Contributor

@verult verult left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

assert.Equal(t, volume.ProbeAddOrUpdate, events[0].Op)
assert.NoError(t, err)

// Call probe again, should 0 event.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"should return 0 event"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

@@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@linyouchong linyouchong Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO added

}

// event of subdir of pluginDirAbs
if parentPathAbs == pluginDirAbs {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@linyouchong linyouchong Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh ,got it !


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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good idea!

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@linyouchong linyouchong Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the check will be removed

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done!

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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@linyouchong linyouchong Mar 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should trigger ProbeRemove event in 2 cases:

  1. The driverDir is removed
  2. The driver executable is removed
    other case we should trigger ProbeAddOrUpdate event

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding this test!

@linyouchong linyouchong force-pushed the lyc-probe branch 2 times, most recently from 13a0a9d to 9d77be6 Compare March 26, 2018 03:14
// getExecutableName returns the executableName of a flex plugin
func getExecutableName(driverDirName string) string {
parts := strings.Split(driverDirName, "~")
return parts[len(parts)-1]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done!
PTAL

@verult
Copy link
Contributor

verult commented Mar 28, 2018

/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.

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

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

@verult
Copy link
Contributor

verult commented Mar 29, 2018

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

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

verult commented Mar 30, 2018

/lgtm

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

verult commented Apr 2, 2018

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

@thockin
Copy link
Member

thockin commented Apr 2, 2018

/approve

@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot 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
Copy link

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.

@k8s-github-robot k8s-github-robot merged commit fc19fc9 into kubernetes:master Apr 3, 2018
@verult
Copy link
Contributor

verult commented Apr 3, 2018

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

@linyouchong
Copy link
Contributor Author

@verult ok, I am working on it

@k8s-ci-robot 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
@shay-berman
Copy link

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
Copy link
Contributor

verult commented Jun 25, 2018

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
Copy link
Contributor Author

@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
Copy link
Contributor

verult commented Jun 30, 2018

No problem, thanks for the update!

@linyouchong
Copy link
Contributor Author

@verult, e2e test is added, PTAL
#66206

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/storage Categorizes an issue or PR as relevant to SIG Storage. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
8 participants