CSI: remove global vars #74966

hoegaarden · 2019-03-05T15:40:18Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

This commit removes global state in the variables

csi.csiDrivers
csi.nim
csi.PluginHandler

The CSI plugin struct now holds the methods to implement a volume plugin
and a plugin handler. The state formerly held in the package
global variables now has been moved into this struct, too.
Therefore, any instance of the CSI plugin now is completely indipendent
and does not share anything with any other instance.

Because the CSI plugin is a very special plugin, the volume plugin
manager knows some specifics about is: it has a method to specifically
get hold of the plugin and return its plugin handler aspect.
This is used by the kubelet to register this plugin handler with its
plugin watcher without the need to know specifics about the CSI plugin,
all those are handled in the volume plugin manager.

Additional notes:

Because the the kubelet fails to start now if the CSI
plugin cannot be found, a CSI plugin now also needs to be registered
for kubemark's hollow kubelet. We use a faked plugin here.
The fake implementations (for the plugin manager & kubelet watchable
volume plugin) have been automatically generated by counterfeiter and
can therefore be updated automatically.
To break an import cycle (the CSI plugin imports the volume plugin,
now that the volume plugin manager needs to query for the CSI plugin,
it in turn imports the CSI plugin -> import cycle) we've split out the
const for the plugin name into its own package. This pacakge can be
imported by all packages in question: the volume & the csi package.

Release notes

NONE

/assign @vladimirvivien

hoegaarden · 2019-03-05T16:22:09Z

/test pull-kubernetes-integration

hoegaarden · 2019-03-05T18:01:58Z

/milestone v1.15

hoegaarden · 2019-03-26T15:12:51Z

/test pull-kubernetes-e2e-gce
boskos seems to have issues

hoegaarden · 2019-03-26T20:44:03Z

/test pull-kubernetes-kubemark-e2e-gce-big

hoegaarden · 2019-04-01T21:05:22Z

/retest

hoegaarden · 2019-04-08T04:23:44Z

/retest

tallclair · 2020-01-21T18:38:43Z

pkg/kubelet/kubelet.go

 	// Adding Registration Callback function for CSI Driver
-	kl.pluginManager.AddHandler(pluginwatcherapi.CSIPlugin, plugincache.PluginHandler(csi.PluginHandler))
+	if err := kl.registerCSIPluginHandler(); err != nil {
+		kl.recorder.Eventf(kl.nodeRef, v1.EventTypeWarning, events.KubeletSetupFailed, err.Error())


Is this a synchronous call, or is it possible that for the following fatal call to happen before the event is sent to the apiserver?

tallclair · 2020-01-21T18:39:08Z

pkg/kubelet/kubelet.go

-	kl.pluginManager.AddHandler(pluginwatcherapi.CSIPlugin, plugincache.PluginHandler(csi.PluginHandler))
+	if err := kl.registerCSIPluginHandler(); err != nil {
+		kl.recorder.Eventf(kl.nodeRef, v1.EventTypeWarning, events.KubeletSetupFailed, err.Error())
+		klog.Fatalf("failed to register CSI Plugin Handler. err: %v", err)


Under what circumstances would an error occur here? Adding a Fatal call in the kubelet startup path makes me nervous.

derekwaynecarr · 2020-01-28T21:21:10Z

defer to @tallclair for final review

jtslear · 2020-02-12T12:42:49Z

Hello, @hoegaarden, @cheftako, and @sjpotter
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. Is this PR still intended for milestone 1.18? It looks like we are awaiting reviews and there are some conflicting files.

jtslear · 2020-02-28T16:49:10Z

Hello, @hoegaarden, @tallclair
Bug Triage team here for the 1.18 release. This is a friendly reminder that code freeze is scheduled for 5 March. Is this PR still intended for milestone 1.18? There hasn't been any movement on this since the end of January. We are pending reviews, and conflicts.

cc @kubernetes/sig-apps-pr-reviews, @kubernetes/sig-node-pr-reviews, @kubernetes/sig-storage-pr-reviews

jtslear · 2020-03-05T18:23:41Z

With no response during the course of this release, I'm going to clear the milestone tracking of this PR.

/milestone clear

fejta-bot · 2020-06-03T18:46:23Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

cheftako · 2020-06-15T23:22:30Z

Are we still trying to merge this or should we just close it?

hoegaarden · 2020-06-27T21:48:49Z

I think I have more bandwidth again to look into that. I have no idea what happened in the CSI world in the last months, so I will check if I can rebase it or close it otherwise.

/remove-lifecycle stale

k8s-ci-robot · 2020-07-27T16:47:26Z

New changes are detected. LGTM label has been removed.

k8s-ci-robot · 2020-07-27T16:48:17Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: hoegaarden, jsafrane
To complete the pull request process, please assign vishh, wojtek-t
You can assign the PR to them by writing /assign @vishh @wojtek-t in a comment when ready.

The full list of commands accepted by this bot can be found 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

This commit removes global state in the variables - `csi.csiDrivers` - `csi.nim` - `csi.PluginHandler` The CSI plugin struct now holds the methods to implement a volume plugin and a plugin handler. The state formerly held in the package global variables now has been moved into this struct, too. Therefore, any instance of the CSI plugin now is completely indipendent and does not share anything with any other instance. Because the CSI plugin is a very special plugin, the volume plugin manager knows some specifics about it: it has a method to specifically get hold of the plugin and return its plugin handler aspect. This is used by the kubelet to register this plugin handler with its plugin watcher without the need to know specifics about the CSI plugin, all those are handled in the volume plugin manager. Additional notes: - If the plugin handler cannot be registered at the start of the kubelet we retry the registration every 10 secs in the background. For each failed registration attempt we generate a log message and an event - Because the the kubelet fails to start now if the CSI plugin cannot be found, a CSI plugin now also needs to be registered for kubemark's hollow kubelet. We use a faked plugin here. - The fake implementations (for the plugin manager & kubelete watchable volume plugin) have been automatically generated by `counterfeiter` and can therefore be updated automatically. - To break an import cycle (the CSI plugin imports the volume plugin, now that the volume plugin manager needs to query for the CSI plugin, it in turn imports the CSI plugin -> import cycle) we've split out the const for the plugin name into its own package. This pacakge can be imported by all packages in question: the volume & the csi package.

hoegaarden · 2020-07-29T15:19:38Z

/retest

fejta-bot · 2020-10-27T17:34:25Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

k8s-ci-robot · 2020-10-27T17:34:33Z

@hoegaarden: PR needs rebase.

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.

fejta-bot · 2020-11-26T18:17:20Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

fejta-bot · 2020-12-26T19:02:00Z

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

k8s-ci-robot · 2020-12-26T19:02:14Z

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/close

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.

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Mar 5, 2019

k8s-ci-robot assigned vladimirvivien Mar 5, 2019

k8s-ci-robot requested review from cheftako and sjpotter March 5, 2019 15:41

k8s-ci-robot added this to the v1.15 milestone Mar 5, 2019

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 9, 2019

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Mar 31, 2019

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 7, 2019

hoegaarden changed the title ~~CSI: remove the singleton~~ CSI: remove global vars Apr 8, 2019

This was referenced Apr 8, 2019

kubelet: get CSI plugin by name #74963

Closed

CSI: remove globals in pkg/volume/csi #73919

Closed

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 8, 2019

tallclair reviewed Jan 21, 2020

View reviewed changes

derekwaynecarr removed their assignment Jan 28, 2020

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

k8s-ci-robot removed this from the v1.18 milestone Mar 5, 2020

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 3, 2020

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jun 27, 2020

k8s-ci-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 Jul 27, 2020

k8s-ci-robot added the sig/instrumentation Categorizes an issue or PR as relevant to SIG Instrumentation. label Jul 27, 2020

k8s-ci-robot added lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 27, 2020

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Nov 26, 2020

k8s-ci-robot closed this Dec 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSI: remove global vars #74966

CSI: remove global vars #74966

hoegaarden commented Mar 5, 2019 •

edited

hoegaarden commented Mar 5, 2019

hoegaarden commented Mar 5, 2019

hoegaarden commented Mar 26, 2019

hoegaarden commented Mar 26, 2019

hoegaarden commented Apr 1, 2019

hoegaarden commented Apr 8, 2019

tallclair Jan 21, 2020

tallclair Jan 21, 2020

derekwaynecarr commented Jan 28, 2020

jtslear commented Feb 12, 2020

jtslear commented Feb 28, 2020

jtslear commented Mar 5, 2020

fejta-bot commented Jun 3, 2020

cheftako commented Jun 15, 2020

hoegaarden commented Jun 27, 2020

k8s-ci-robot commented Jul 27, 2020

k8s-ci-robot commented Jul 27, 2020

hoegaarden commented Jul 29, 2020

fejta-bot commented Oct 27, 2020

k8s-ci-robot commented Oct 27, 2020

fejta-bot commented Nov 26, 2020

fejta-bot commented Dec 26, 2020

k8s-ci-robot commented Dec 26, 2020

CSI: remove global vars #74966

CSI: remove global vars #74966

Conversation

hoegaarden commented Mar 5, 2019 • edited

hoegaarden commented Mar 5, 2019

hoegaarden commented Mar 5, 2019

hoegaarden commented Mar 26, 2019

hoegaarden commented Mar 26, 2019

hoegaarden commented Apr 1, 2019

hoegaarden commented Apr 8, 2019

tallclair Jan 21, 2020

Choose a reason for hiding this comment

tallclair Jan 21, 2020

Choose a reason for hiding this comment

derekwaynecarr commented Jan 28, 2020

jtslear commented Feb 12, 2020

jtslear commented Feb 28, 2020

jtslear commented Mar 5, 2020

fejta-bot commented Jun 3, 2020

cheftako commented Jun 15, 2020

hoegaarden commented Jun 27, 2020

k8s-ci-robot commented Jul 27, 2020

k8s-ci-robot commented Jul 27, 2020

hoegaarden commented Jul 29, 2020

fejta-bot commented Oct 27, 2020

k8s-ci-robot commented Oct 27, 2020

fejta-bot commented Nov 26, 2020

fejta-bot commented Dec 26, 2020

k8s-ci-robot commented Dec 26, 2020

hoegaarden commented Mar 5, 2019 •

edited