CSI fix for gRPC conn leak #64519

vladimirvivien · 2018-05-30T14:42:20Z

What this PR does / why we need it:
This PR is a bug fix for leaky gRPC connection that never closes (see issue #64341 for detail)

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

This fix was originally started with PR #64380

Fixed CSI gRPC connection leak during volume operations.

vladimirvivien · 2018-05-30T14:42:42Z

/sig storage
/kind bug

dims · 2018-05-30T14:58:09Z

/milestone v1.11
/sig storage
/priority important-soon

@kubernetes/sig-storage-pr-reviews please see if the milestone/priority is correct

vladimirvivien · 2018-05-30T17:09:26Z

/test pull-kubernetes-e2e-kops-aws

sbezverk · 2018-05-30T21:40:43Z

pkg/volume/csi/csi_client.go

+		return nil, fmt.Errorf("driver name is empty")
+	}
+
+	network := "unix"


What about defining network as a const at the top of the file? I think it would be more visible.

I thought about it, the value for network does not need to be global since it should always be "unix" and is only used when constructing the connection in that method.

saad-ali

Some minor comments

saad-ali · 2018-05-31T18:54:28Z

pkg/volume/csi/csi_client.go

@@ -61,45 +62,13 @@ type csiClient interface {

 // csiClient encapsulates all csi-plugin methods
 type csiDriverClient struct {


Add a struct interface implementation check:

var _ csiClient = &csiDriverClient{}

saad-ali · 2018-05-31T19:01:23Z

pkg/volume/csi/csi_client.go

-	if err := c.assertConnection(); err != nil {
-		glog.Errorf("%v: failed to assert a connection: %v", csiPluginName, err)
+
+	conn, err := newGrpcConn(c.driverName)


nit: as a general rule of thumb never call new anything inside of a method implementing business logic. That encodes the creation of a dependency with the business logic making it harder to test just the business logic.

Instead, have the dependency either passed in as a parameter (with an interface type) or make the method creating the external dependency another interface method (i.e. a new NewGrpcConnection() method in the csiClient interface). That way it makes it easy to create a fake of the dependency for testing.

You don't have to do this now. Can do it in a follow up PR.

Ok I will take a look at that.

i looked at your suggestion. While agree, I even had an impl that injected the connection. However, the grpc.ClientConn type (created with newGrpcCon) is a struct value which complicates mocking it. So, I kept it simple. But I agree.

saad-ali · 2018-05-31T19:03:46Z

pkg/volume/csi/csi_mounter.go

@@ -290,29 +291,12 @@ func (c *csiMountMgr) TearDownAt(dir string) error {
 		return nil
 	}

-	// load volume info from file


Why is this code being moved to NewUnmounter()?

Follow up, why isn't saveVolumeData(...) also being moved?

initialization of csiClient is moved to New{Moun|Unmoun}ter to keep value injection of csiMounterMgr consistent and in one location, where possible. This also helps clean the method of non-storage code. I agree, saveVolumeData probably should be moved.

Ok, saveVolumeData will stay. This is because it's called in the SetUpAt() method after the mount dir its created which is the location where the json file is saved.

Let's open up a an issue to clean this up later. It's really odd that save is done in one location and load is done in a completely different place.

Once you pointed it out, I could not resist to make things more uniformed. Good suggestion (included a bit of clean up too) Done.

vladimirvivien · 2018-06-01T13:07:20Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu
/test pull-kubernetes-e2e-gce

vladimirvivien · 2018-06-01T13:22:34Z

/test pull-kubernetes-e2e-gce

vladimirvivien · 2018-06-01T13:23:03Z

/test pull-kubernetes-e2e-gce-device-plugin-gpu

saad-ali

/lgtm
/approve

saad-ali · 2018-06-01T16:54:35Z

pkg/volume/csi/csi_client.go


-	return nil
+func newCsiDriverClient(driverName string) *csiDriverClient {


nit: return the interface (csiClient) not the struct

You are right. I missed that.

k8s-ci-robot · 2018-06-01T16:56:30Z

[APPROVALNOTIFIER] This PR is APPROVED

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

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/csi/OWNERS~~ [saad-ali,vladimirvivien]

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

dims · 2018-06-01T17:27:03Z

/status approved-for-milestone

k8s-github-robot · 2018-06-01T17:27:48Z

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@saad-ali @vladimirvivien

Pull Request Labels

sig/storage: Pull Request will be escalated to these SIGs if needed.
priority/important-soon: Escalate to the pull request owners and SIG owner; move out of milestone after several unsuccessful escalation attempts.
kind/bug: Fixes a bug discovered during the current release.

Help

k8s-github-robot · 2018-06-02T10:05:41Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-06-02T10:58:46Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

…f-#64519-upstream-release-1.10 Automatic merge from submit-queue. Automated cherry pick of #64519: CSI fix for gRPC conn leak, test updates Cherry pick of #64519 on release-1.10. #64519: CSI fix for gRPC conn leak, test updates ```release-note Fixed CSI gRPC connection leak during volume operations. ```

k8s-ci-robot requested review from rootfs and verult May 30, 2018 14:42

k8s-ci-robot added sig/storage Categorizes an issue or PR as relevant to SIG Storage. kind/bug Categorizes issue or PR as related to a bug. labels May 30, 2018

k8s-ci-robot added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label May 30, 2018

k8s-ci-robot added this to the v1.11 milestone May 30, 2018

k8s-github-robot added the milestone/needs-approval label May 30, 2018

vladimirvivien changed the title ~~CSI fix for gRPC conn leak, test updates~~ CSI fix for gRPC conn leak May 30, 2018

sbezverk reviewed May 30, 2018

View reviewed changes

saad-ali reviewed May 31, 2018

View reviewed changes

CSI fix for gRPC conn leak, test updates

d6d3962

saad-ali approved these changes Jun 1, 2018

View reviewed changes

k8s-ci-robot assigned saad-ali Jun 1, 2018

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

k8s-ci-robot added the status/approved-for-milestone label Jun 1, 2018

k8s-github-robot removed the milestone/needs-approval label Jun 1, 2018

k8s-github-robot merged commit 54900d7 into kubernetes:master Jun 2, 2018

vladimirvivien mentioned this pull request Jun 2, 2018

Automated cherry pick of #64519: CSI fix for gRPC conn leak, test updates #64658

Merged

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 Jun 4, 2018

flangewad mentioned this pull request Sep 28, 2018

The unit tests for csiDriverClient do not exercise any production code #69219

Closed

gnufied mentioned this pull request Dec 13, 2019

CSI Pod volume directories are not cleaned up if MountDevice fails #84878

Closed

msau42 mentioned this pull request Jul 27, 2022

Speed up SELinux volume relabeling using mounts MVP #108692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CSI fix for gRPC conn leak #64519

CSI fix for gRPC conn leak #64519

vladimirvivien commented May 30, 2018 •

edited

vladimirvivien commented May 30, 2018

dims commented May 30, 2018

vladimirvivien commented May 30, 2018

sbezverk May 30, 2018

vladimirvivien May 30, 2018

vladimirvivien May 31, 2018

saad-ali left a comment

saad-ali May 31, 2018

vladimirvivien Jun 1, 2018

saad-ali May 31, 2018

vladimirvivien May 31, 2018

vladimirvivien May 31, 2018

saad-ali May 31, 2018

saad-ali May 31, 2018

vladimirvivien May 31, 2018

vladimirvivien May 31, 2018

saad-ali May 31, 2018

vladimirvivien Jun 1, 2018

vladimirvivien commented Jun 1, 2018

vladimirvivien commented Jun 1, 2018

vladimirvivien commented Jun 1, 2018

saad-ali left a comment

saad-ali Jun 1, 2018

vladimirvivien Jun 1, 2018

k8s-ci-robot commented Jun 1, 2018

dims commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

k8s-github-robot commented Jun 2, 2018

k8s-github-robot commented Jun 2, 2018

		@@ -61,45 +62,13 @@ type csiClient interface {

		// csiClient encapsulates all csi-plugin methods
		type csiDriverClient struct {


		return nil
		func newCsiDriverClient(driverName string) *csiDriverClient {

CSI fix for gRPC conn leak #64519

CSI fix for gRPC conn leak #64519

Conversation

vladimirvivien commented May 30, 2018 • edited

vladimirvivien commented May 30, 2018

dims commented May 30, 2018

vladimirvivien commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

saad-ali 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

vladimirvivien commented Jun 1, 2018

vladimirvivien commented Jun 1, 2018

vladimirvivien commented Jun 1, 2018

saad-ali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 1, 2018

dims commented Jun 1, 2018

k8s-github-robot commented Jun 1, 2018

k8s-github-robot commented Jun 2, 2018

k8s-github-robot commented Jun 2, 2018

vladimirvivien commented May 30, 2018 •

edited