Dynamic Flexvolume plugin discovery, probing with filesystem watch. #50031

verult · 2017-08-02T18:14:35Z

What this PR does / why we need it: Enables dynamic Flexvolume plugin discovery. This model uses a filesystem watch (fsnotify library), which notifies the system that a probe is necessary only if something changes in the Flexvolume plugin directory.

This PR uses the dependency injection model in #49668.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #51470

Release Note:

Dynamic Flexvolume plugin discovery. Flexvolume plugins can now be discovered on the fly rather than only at system initialization time.

/sig-storage

/assign @jsafrane @saad-ali
/cc @bassam @chakri-nelluri @kokhang @liggitt @thockin

k8s-ci-robot · 2017-08-02T18:14:38Z

@verult: GitHub didn't allow me to request PR reviews from the following users: kokhang, bassam, chakri-nelluri.

Note that only kubernetes members can review this PR, and authors cannot review their own PRs.

In response to this:

What this PR does / why we need it: Enables dynamic Flexvolume plugin discovery. This model reloads Flexvolume plugins every time the system tries to match a volume with a plugin (e.g. during attach/detach, mount/unmount), which occurs very frequently.

This PR uses the dependency injection model in #49668.

do-not-merge

/release-note-none

/sig-storage

/assign @jsafrane @saad-ali
/cc @bassam @chakri-nelluri @kokhang @liggitt @thockin

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.

liggitt · 2017-08-02T18:34:12Z

pkg/controller/volume/persistentvolume/pv_controller_base.go

@@ -90,7 +90,7 @@ func NewController(p ControllerParameters) (*PersistentVolumeController, error)
 		resyncPeriod:                  p.SyncPeriod,
 	}

-	if err := controller.volumePluginMgr.InitPlugins(p.VolumePlugins, controller); err != nil {
+	if err := controller.volumePluginMgr.InitPlugins(p.VolumePlugins, nil, controller); err != nil {


add volume.DynamicPluginProber to controllerparameters?

AFAIK PV controller was never aware of Flexvolume.

verult · 2017-08-05T05:05:45Z

Added filesystem watch implementation of the prober.

kokhang · 2017-08-08T02:32:32Z

@verult I tested this locally and it did work. Kubelet was able to load the Rook flexvolume without having to restart. Here is the log:
I0808 02:22:21.564582 17759 plugins.go:420] Loaded volume plugin "flexvolume-kubernetes.io/rook"

However there are still some concerns that i have listed in kubernetes/community#833. Namely about how to obtain or construct a client config so that the driver can talk to the K8S api.

verult · 2017-08-08T21:47:54Z

/cc @msau42

saad-ali

Took a first pass

saad-ali · 2017-08-14T16:55:37Z

pkg/controller/volume/persistentvolume/pv_controller_base.go

@@ -90,7 +90,7 @@ func NewController(p ControllerParameters) (*PersistentVolumeController, error)
 		resyncPeriod:                  p.SyncPeriod,
 	}

-	if err := controller.volumePluginMgr.InitPlugins(p.VolumePlugins, controller); err != nil {
+	if err := controller.volumePluginMgr.InitPlugins(p.VolumePlugins, nil, controller); err != nil {


nit: Whenever you pass in a parameter where it is not obvious what it is, add a small inline comment with the paramter name, for clarity:

if err := controller.volumePluginMgr.InitPlugins(p.VolumePlugins, nil /* prober */, controller); err != nil {

Also add an inline comment explaining why you are leaving the value nil. e.g. PV controller was never aware of Flexvolume...

saad-ali · 2017-08-14T17:16:49Z

pkg/volume/flexvolume/probe.go

+		}
+	}
+
+	if time.Since(prober.lastUpdated) > time.Second { // Reduce the frequency of probes.


nit: add to const at top of file.

saad-ali · 2017-08-14T17:19:18Z

pkg/volume/flexvolume/probe.go

+	}
+
+	if eventOpIs(event, fsnotify.Remove) && eventPathAbs == pluginDirAbs {
+		// pluginDir needs to exist in order to be watched.


Post a warning to log.

saad-ali · 2017-08-14T17:20:59Z

pkg/volume/flexvolume/probe.go

+		return err
+	}
+
+	if eventOpIs(event, fsnotify.Remove) && eventPathAbs == pluginDirAbs {


Add a comment above this block indicating what you're doing and why.

saad-ali · 2017-08-14T17:26:15Z

pkg/volume/flexvolume/probe.go

+		prober.lastUpdated = time.Now()
+	}
+
+	return nil


There is a race here if files are dropped quickly in succession, and the read happens before the 2nd event is processed.

Let's make sure to add testing to see how often this might happen, and if it is infrequent put a comment indicating you know about it.

Fixing this by adding a limit on number of events processed per second, and ignoring events from .dot files.

Users need to install drivers atomically, so if there are ever multiple files in an installation, O(1) events are triggered regardless. Multiple events are triggered only when multiple Flexvolume drivers are installed.

kokhang · 2017-08-17T18:03:34Z

I know this is unrelated but what do you recommend to deploy the flexvolume drivers on nodes that are not configured to have the same flexvolumeDriverDir? I followed the proposed example using daemon set. But i was thinking that this would not work in the case where the driverDir is different in other nodes.

verult · 2017-08-17T19:04:04Z

@kokhang There was some discussion about this topic here: kubernetes/community#833 (comment)

liggitt · 2017-08-24T02:28:20Z

I would recommend reporting the issue to kubeadm as a bug… I wouldn't expect that to be a desired deployment configuration

edit: is the controller-manager making use of flex plugins net new to this PR? If so, it wouldn't be a bug, it would be part of adding this enhancement

kokhang · 2017-08-24T02:45:03Z

@liggitt ill contact to the kubeadm folks. But im also wondering if the controller-manager needs to probe for the flexvolume driver as well? Or is this case already handled by this kubelet probe?

verult · 2017-08-24T03:20:53Z

In order to perform attach-detach operations, the controller-manager would have to probe for Flexvolume driver. One possible solution for now, while waiting for a response from the kubeadm team, is to use one of controller-manager pod's existing hostpath. I'm not sure if the kubeadm installation uses this but, /etc/srv/kubernetes might be a possibility.

kokhang · 2017-08-24T03:31:05Z

In order to perform attach-detach operations, the controller-manager would have to probe for Flexvolume driver.

Does this mean that this dynamic flexvolume discovery fix is only applicable for non attacher-detacher-controller based flexvolume plugins?

kokhang · 2017-08-24T03:34:00Z

I just saw the code. It seems you are doing the probe in the controller-manager as well

saad-ali · 2017-08-25T18:36:47Z

/lgtm
/approve

…em watch.

saad-ali · 2017-08-25T18:47:36Z

/lgtm

k8s-github-robot · 2017-08-25T18:48:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: saad-ali, verult
We suggest the following additional approvers: derekwaynecarr, mikedanese

Assign the PR to them by writing /assign @derekwaynecarr @mikedanese in a comment when ready.

No associated issue. Update pull-request body to add a reference to an issue, or get approval with /approve no-issue

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

cmd/kube-controller-manager/OWNERS
cmd/kubelet/OWNERS
~~pkg/controller/volume/attachdetach/OWNERS~~ [saad-ali]
~~pkg/controller/volume/persistentvolume/OWNERS~~ [saad-ali]
pkg/kubelet/OWNERS
~~pkg/volume/OWNERS~~ [saad-ali]
~~test/OWNERS~~ [saad-ali]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

fejta-bot · 2017-08-25T21:49:33Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to @fejta).

Review the full test history for this PR.

k8s-github-robot · 2017-08-26T09:05:32Z

Automatic merge from submit-queue (batch tested with PRs 51054, 51101, 50031, 51296, 51173)

@saad-ali

Automatic merge from submit-queue (batch tested with PRs 51805, 51725, 50925, 51474, 51638) Flexvolume dynamic plugin discovery: Prober unit tests and basic e2e test. **What this PR does / why we need it**: Tests for changes introduced in PR #50031 . As part of the prober unit test, I mocked filesystem, filesystem watch, and Flexvolume plugin initialization. Moved the filesystem event goroutine to watcher implementation. **Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #51147 **Special notes for your reviewer**: First commit contains added functionality of the mock filesystem. Second commit is the refactor for moving mock filesystem into a common util directory. Third commit is the unit and e2e tests. **Release note**: ```release-note NONE ``` /release-note-none /sig storage /assign @saad-ali @liggitt /cc @mtaufen @chakri-nelluri @wongma7

@jsafrane

Automatic merge from submit-queue (batch tested with PRs 51054, 51101, 50031, 51296, 51173) Dynamic Flexvolume plugin discovery, probing with filesystem watch. **What this PR does / why we need it**: Enables dynamic Flexvolume plugin discovery. This model uses a filesystem watch (fsnotify library), which notifies the system that a probe is necessary only if something changes in the Flexvolume plugin directory. This PR uses the dependency injection model in kubernetes#49668. **Release Note**: ```release-note Dynamic Flexvolume plugin discovery. Flexvolume plugins can now be discovered on the fly rather than only at system initialization time. ``` /sig-storage /assign @jsafrane @saad-ali /cc @bassam @chakri-nelluri @kokhang @liggitt @thockin

k8s-ci-robot assigned jsafrane and saad-ali Aug 2, 2017

k8s-ci-robot requested review from liggitt and thockin August 2, 2017 18:14

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 2, 2017

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

liggitt reviewed Aug 2, 2017

View reviewed changes

verult changed the title ~~WIP: Dynamic Flexvolume plugin discovery, probing with polling.~~ WIP: Dynamic Flexvolume plugin discovery, probing with filesystem watch. Aug 5, 2017

verult force-pushed the ConnectedProbe branch 2 times, most recently from b2accc5 to 7737dfb Compare August 5, 2017 04:37

k8s-ci-robot requested a review from msau42 August 8, 2017 21:47

verult force-pushed the ConnectedProbe branch from 7737dfb to ded24ab Compare August 9, 2017 23:03

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

verult force-pushed the ConnectedProbe branch from ded24ab to 8c60dc2 Compare August 9, 2017 23:22

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 9, 2017

verult force-pushed the ConnectedProbe branch from 8c60dc2 to a9931e2 Compare August 11, 2017 01:12

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 12, 2017

saad-ali suggested changes Aug 14, 2017

View reviewed changes

verult mentioned this pull request Aug 16, 2017

WIP: Auto Flexvolume plugin discovery, with prober dependency injection through VolumePluginMgr initializer. #49668

Closed

verult force-pushed the ConnectedProbe branch from a9931e2 to 22f12cf Compare August 18, 2017 21:11

k8s-github-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 18, 2017

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

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 25, 2017

verult force-pushed the ConnectedProbe branch from b24e55b to f3261fd Compare August 25, 2017 18:39

k8s-github-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 25, 2017

Adding dynamic Flexvolume plugin discovery capability, using filesyst…

396c3c7

…em watch.

verult force-pushed the ConnectedProbe branch from f3261fd to 396c3c7 Compare August 25, 2017 18:44

saad-ali approved these changes Aug 25, 2017

View reviewed changes

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

saad-ali added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 25, 2017

k8s-github-robot merged commit 932e07a into kubernetes:master Aug 26, 2017

klausenbusk mentioned this pull request Aug 27, 2017

Block storage support? digitalocean/digitalocean-cloud-controller-manager#19

Closed

This was referenced Aug 28, 2017

Flexvolume dynamic plugin discovery: Prober unit tests and basic e2e test. #51474

Merged

Flexvolume Dynamic Plugin Discovery #51470

Closed

verult mentioned this pull request Sep 13, 2017

Improve Flexvolume Deployment kubernetes/enhancements#377

Closed

tengqm mentioned this pull request Nov 8, 2017

Split persistent volume concept article kubernetes/website#6215

Merged

verult mentioned this pull request Jan 17, 2018

Automatic plugin discovery should trigger plugin init only for the relevant plugin (and not to all the existing plugins) #58352

Closed

verult deleted the ConnectedProbe branch March 24, 2018 01:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Dynamic Flexvolume plugin discovery, probing with filesystem watch. #50031

Dynamic Flexvolume plugin discovery, probing with filesystem watch. #50031

verult commented Aug 2, 2017 •

edited

Loading

k8s-ci-robot commented Aug 2, 2017

liggitt Aug 2, 2017

verult Aug 5, 2017

verult commented Aug 5, 2017

kokhang commented Aug 8, 2017

verult commented Aug 8, 2017

saad-ali left a comment

saad-ali Aug 14, 2017

saad-ali Aug 14, 2017 •

edited

Loading

saad-ali Aug 14, 2017

saad-ali Aug 14, 2017

saad-ali Aug 14, 2017

verult Aug 17, 2017

kokhang commented Aug 17, 2017

verult commented Aug 17, 2017

liggitt commented Aug 24, 2017 •

edited

Loading

kokhang commented Aug 24, 2017 •

edited

Loading

verult commented Aug 24, 2017

kokhang commented Aug 24, 2017

kokhang commented Aug 24, 2017

saad-ali commented Aug 25, 2017

saad-ali commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

fejta-bot commented Aug 25, 2017

k8s-github-robot commented Aug 26, 2017

Dynamic Flexvolume plugin discovery, probing with filesystem watch. #50031

Dynamic Flexvolume plugin discovery, probing with filesystem watch. #50031

Conversation

verult commented Aug 2, 2017 • edited Loading

k8s-ci-robot commented Aug 2, 2017

liggitt Aug 2, 2017

Choose a reason for hiding this comment

verult Aug 5, 2017

Choose a reason for hiding this comment

verult commented Aug 5, 2017

kokhang commented Aug 8, 2017

verult commented Aug 8, 2017

saad-ali left a comment

Choose a reason for hiding this comment

saad-ali Aug 14, 2017

Choose a reason for hiding this comment

saad-ali Aug 14, 2017 • edited Loading

Choose a reason for hiding this comment

saad-ali Aug 14, 2017

Choose a reason for hiding this comment

saad-ali Aug 14, 2017

Choose a reason for hiding this comment

saad-ali Aug 14, 2017

Choose a reason for hiding this comment

verult Aug 17, 2017

Choose a reason for hiding this comment

kokhang commented Aug 17, 2017

verult commented Aug 17, 2017

liggitt commented Aug 24, 2017 • edited Loading

kokhang commented Aug 24, 2017 • edited Loading

verult commented Aug 24, 2017

kokhang commented Aug 24, 2017

kokhang commented Aug 24, 2017

saad-ali commented Aug 25, 2017

saad-ali commented Aug 25, 2017

k8s-github-robot commented Aug 25, 2017

fejta-bot commented Aug 25, 2017

k8s-github-robot commented Aug 26, 2017

verult commented Aug 2, 2017 •

edited

Loading

saad-ali Aug 14, 2017 •

edited

Loading

liggitt commented Aug 24, 2017 •

edited

Loading

kokhang commented Aug 24, 2017 •

edited

Loading