Add kubelet plugin manager #73891

taragu · 2019-02-10T21:33:46Z

What type of PR is this?
/kind feature

What this PR does / why we need it:
Implement a controller that manages plugin registration/unregistration

Which issue(s) this PR fixes:
Fixes #73371

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Kubelet plugin registration now has retry and exponential backoff logic for when registration of plugins (like CSI or device plugin) fail.

taragu · 2019-02-10T21:34:41Z

* "reconciler" should use the [`nestedpendingoperations` library]
to execute registration and unregistration operations. 
This library automatically takes care of exponential backoff and ensures no more
then one operation (registration or unregistration) can happen at a given time for 
a given socket.

@saad-ali I was looking at nestedpendingoperations but wasn't sure how to use it to register/unregister plugin. Could you please point me to a similar example?

taragu · 2019-02-10T21:50:44Z

cc @vikaschoudhary16

saad-ali · 2019-02-20T02:15:59Z

@taragu there is a generic version of the nestedpendingoperation that is simpler and not volume specific called goroutinemap. See https://github.com/kubernetes/kubernetes/blob/master/pkg/util/goroutinemap/goroutinemap.go (edited)

The operation_executor is just a wrapper around nestedpendingoperation (or goroutinemap). I would define a new operation_executor for the plugin manager that defines two operations: RegisterPlugin() and UnregisterPlugin(). This operation_executor would use goroutinemap to ensure that no more then one operation starts for the same socket. So the key that you use in the goroutinemap (I think) would be the socket path.

Once you have an operation_executor defined, you can create two caches: desired and actual that keep track of the sockets that we want to register and the sockets that we actually have successfully registered.

Then you can create the dumb diff loop, the reconciler, it will (maybe every 100 ms) diff desired and actual. If a socket is in desired but not in actual it will call operation_executor.Register(). If a socket is in actual but not desired it will call operation_executor.Unregister(). And because operation_executor uses goroutinemap it will refuse to start a new register or unregister operation while one is pending (and automatically handle exponential backoff).

Once you have that, you need to write a populator -- in this case that will basically just be the existing code that currently calls the old Register method -- and instead it would add to the desired state cache on register, and remove from desired state cache on unregister (populator in this case doesn't need to be a new package).

taragu · 2019-02-20T14:29:34Z

@saad-ali thank you so much!! This is very comprehensive. I'll try to digest this later this evening.

saad-ali

@taragu, one more thing I forgot, we should undo the temporary fix from #72873 as well. We could do that in a follow up PR if you want.

taragu · 2019-05-10T00:53:07Z

@saad-ali sounds good. I will undo #72873 in a followup PR

msau42 · 2019-05-13T17:58:13Z

/hold
Spoke with @vishh. There are still some discussions happening with @saad-ali around if we can just have the drivers restart

vishh · 2019-05-30T21:43:50Z

/approve
(after offline discussion with @saad-ali who patiently explained that k'let can silently fail re-registration when the plugins assume normal operation)

k8s-ci-robot · 2019-05-30T21:44:28Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: saad-ali, taragu, vishh

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:

~~hack/OWNERS~~ [vishh]
~~pkg/kubelet/OWNERS~~ [vishh]

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

saad-ali · 2019-05-30T22:29:26Z

/lgtm

saad-ali · 2019-05-30T22:30:14Z

/milestone v1.15

…ion/unregistration

saad-ali · 2019-05-30T23:02:03Z

/lgtm

taragu · 2019-05-31T00:36:41Z

/hold cancel

taragu · 2019-05-31T10:54:40Z

/test pull-kubernetes-integration

taragu · 2019-05-31T11:32:17Z

/test pull-kubernetes-integration

tedyu · 2019-05-31T17:11:47Z

pkg/kubelet/pluginmanager/cache/actual_state_of_world.go

+	asw.Lock()
+	defer asw.Unlock()
+
+	if _, ok := asw.socketFileToInfo[socketPath]; ok {


Not sure why the check is needed - we can delete the entry directly.
It is fine if the key doesn't exist.

tedyu · 2019-05-31T17:13:42Z

pkg/kubelet/pluginmanager/cache/desired_state_of_world.go

+	dsw.Lock()
+	defer dsw.Unlock()
+
+	if _, ok := dsw.socketFileToInfo[socketPath]; ok {


No need to check prior to deletion

tedyu · 2019-05-31T17:23:36Z

pkg/kubelet/pluginmanager/operationexecutor/operation_generator.go

+	return nil
+}
+
+// Dial establishes the gRPC communication with the picked up plugin socket. https://godoc.org/google.golang.org/grpc#Dial


Should be lowercase: dial

tedyu · 2019-05-31T17:32:35Z

pkg/kubelet/pluginmanager/reconciler/reconciler.go

+}
+
+func (rc *reconciler) getHandlers() map[string]cache.PluginHandler {
+	rc.Lock()


Getting read lock should suffice

lujinda · 2020-01-14T16:44:34Z

pkg/kubelet/pluginmanager/operationexecutor/operation_generator.go

+	actualStateOfWorldUpdater ActualStateOfWorldUpdater) func() error {
+
+	unregisterPluginFunc := func() error {
+		client, conn, err := dial(socketPath, dialTimeoutDuration)


If the plugin has already exited, the dial here will fail.

The client is needed to retrieve from pluginHandlers using infoResp.Type

Do you have suggestion on how to get plugin handler in that case ?

The client is needed to retrieve from pluginHandlers using infoResp.Type

Do you have suggestion on how to get plugin handler in that case ?

The csi plugin will delete the sock file when it exits. When the operationExecutor.UnregisterPlugin is called, the sock file is not existed.
Can add a sock file to the plugin mapping table. ≧◠◡◠≦ Just like 1.14:

func (w *Watcher) getPlugin(socketPath string) (pathInfo, bool) { w.mutex.Lock() defer w.mutex.Unlock() plugin, ok := w.plugins[socketPath] return plugin, ok }

From my POV, we can save socketPath: infoResp.Type in actualStateOfWorld on RegisterPlugin phase, and then it can be used on UnregisterPlugin phase

I made similar suggestion over #87282

Let's see what other people think.

k8s-ci-robot added release-note-none do-not-merge/work-in-progress kind/feature cncf-cla: yes needs-sig needs-priority area/kubelet sig/node and removed needs-sig labels Feb 10, 2019

k8s-ci-robot requested review from feiskyer and krmayankk February 10, 2019 21:34

taragu force-pushed the plugin-manager branch from c472ca1 to ec659e5 Compare February 18, 2019 02:00

k8s-ci-robot added the size/L label Feb 18, 2019

taragu force-pushed the plugin-manager branch from ec659e5 to 5f07de8 Compare February 25, 2019 01:49

k8s-ci-robot added size/XXL and removed size/L labels Feb 25, 2019

taragu force-pushed the plugin-manager branch 7 times, most recently from 17aa31b to a6f39d6 Compare March 3, 2019 22:34

taragu force-pushed the plugin-manager branch 3 times, most recently from 1853dcf to dc863d8 Compare March 9, 2019 14:10

saad-ali reviewed May 9, 2019

View reviewed changes

saad-ali mentioned this pull request May 9, 2019

Kubelet plugin registration should retry on failures #71487

Closed

k8s-ci-robot added the do-not-merge/hold label May 13, 2019

k8s-ci-robot added release-note and removed release-note-none labels May 30, 2019

taragu force-pushed the plugin-manager branch from 63aaa8f to e3e10f1 Compare May 30, 2019 20:59

k8s-ci-robot added the approved label May 30, 2019

taragu force-pushed the plugin-manager branch from e3e10f1 to 9cab985 Compare May 30, 2019 22:10

k8s-ci-robot added the lgtm label May 30, 2019

k8s-ci-robot added this to the v1.15 milestone May 30, 2019

Implement plugin manager - a controller that manages plugin registrat…

5e18554

…ion/unregistration

taragu force-pushed the plugin-manager branch from 9cab985 to 5e18554 Compare May 30, 2019 23:01

k8s-ci-robot removed the lgtm label May 30, 2019

k8s-ci-robot added the lgtm label May 30, 2019

k8s-ci-robot removed the do-not-merge/hold label May 31, 2019

k8s-ci-robot merged commit fe37733 into kubernetes:master May 31, 2019

tedyu reviewed May 31, 2019

View reviewed changes

tedyu mentioned this pull request May 31, 2019

Use read lock for reconciler#getHandlers and clean up for the pluginmanager #78592

Merged

taragu mentioned this pull request Jun 1, 2019

Add metrics for plugin operations for plugin manager #78620

Closed

lujinda reviewed Jan 14, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add kubelet plugin manager #73891

Add kubelet plugin manager #73891

taragu commented Feb 10, 2019 •

edited by saad-ali

Loading

taragu commented Feb 10, 2019 •

edited

Loading

taragu commented Feb 10, 2019

saad-ali commented Feb 20, 2019

taragu commented Feb 20, 2019

saad-ali left a comment

taragu commented May 10, 2019

msau42 commented May 13, 2019

vishh commented May 30, 2019

k8s-ci-robot commented May 30, 2019

saad-ali commented May 30, 2019

saad-ali commented May 30, 2019

saad-ali commented May 30, 2019

taragu commented May 31, 2019

taragu commented May 31, 2019

taragu commented May 31, 2019

tedyu May 31, 2019

tedyu May 31, 2019

tedyu May 31, 2019

tedyu May 31, 2019

lujinda Jan 14, 2020

tedyu Jan 14, 2020

lujinda Jan 14, 2020

k2huang Jan 16, 2020

tedyu Jan 16, 2020

Add kubelet plugin manager #73891

Add kubelet plugin manager #73891

Conversation

taragu commented Feb 10, 2019 • edited by saad-ali Loading

taragu commented Feb 10, 2019 • edited Loading

taragu commented Feb 10, 2019

saad-ali commented Feb 20, 2019

taragu commented Feb 20, 2019

saad-ali left a comment

Choose a reason for hiding this comment

taragu commented May 10, 2019

msau42 commented May 13, 2019

vishh commented May 30, 2019

k8s-ci-robot commented May 30, 2019

saad-ali commented May 30, 2019

saad-ali commented May 30, 2019

saad-ali commented May 30, 2019

taragu commented May 31, 2019

taragu commented May 31, 2019

taragu commented May 31, 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

taragu commented Feb 10, 2019 •

edited by saad-ali

Loading

taragu commented Feb 10, 2019 •

edited

Loading