Add volume level serialization for controller operations #316

hantaowang · 2019-06-24T22:08:51Z

Per Issue 307 using logic similar to #303

Fixes: #307

Operations in the controller driver on a volume already being operated on now return an error.

k8s-ci-robot · 2019-06-24T22:09:05Z

Hi @hantaowang. Thanks for your PR.

I'm waiting for a kubernetes-sigs or kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

msau42 · 2019-06-24T22:12:31Z

/ok-to-test

msau42 · 2019-06-24T22:12:48Z

/assign @davidz627

pkg/gce-pd-csi-driver/controller.go

hantaowang · 2019-06-25T15:23:54Z

I created a VolumeLock struct that abstracts away the logic of the synchronized map. The earlier node level parallelization use it too now. Let me know if you think an actual Interface is required too.

davidz627

core logic mostly LGTM. some comments on naming and testing.
Thanks!

pkg/common/volume_lock.go

pkg/gce-pd-csi-driver/controller.go

pkg/common/volume_lock.go

davidz627 · 2019-06-27T01:25:24Z

pkg/gce-cloud-provider/compute/fake-gce.go

+type FakeBlockingCloudProvider struct {
+	*FakeCloudProvider
+	snapshotToExec chan SnapshotSourceAndTarget
+	readyToExecute chan struct{}


nit: naming. readyToExecute doesn't really tell me much about who is ready to execute what. Maybe createSnapshotCalled is more accurate?

pkg/gce-pd-csi-driver/controller_test.go

pkg/gce-cloud-provider/compute/fake-gce.go

k8s-ci-robot · 2019-06-27T01:44:35Z

@davidz627: GitHub didn't allow me to request PR reviews from the following users: misterikkit.

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

In response to this:

this puts the function into a tight loop when there are items in snapshotToExec but they are not for this function. I would like to avoid this, but I've been thinking about it for a while and im honestly not sure how

/cc @misterikkit @msau42

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.

pkg/common/volume_lock.go

pkg/gce-pd-csi-driver/controller.go

pkg/gce-cloud-provider/compute/fake-gce.go

pkg/gce-pd-csi-driver/controller_test.go

misterikkit · 2019-06-27T05:43:02Z

pkg/gce-pd-csi-driver/controller_test.go

+
+	// Check that vol1CreateSnapshotReqB is rejected, due to same volume ID
+	// Also allow vol1CreateSnapshotReqB to complete, in case it is allowed to Mount
+	snapshotToExecute <- sourceAndTargetForRequest(vol1CreateSnapshotReqB)


The pattern you are using here can be described as, "Signal on a channel once the request has begun, and wait for a return signal before allowing the request to complete."

I like this pattern.

A lot of the complexity in this test comes from the fact that you are using two independent channels to do this. An alternative is to use a channel of channels. That is, each request starts by creating a reply channel and sending it to the unit test through the "main" channel. Then each request waits for a reply on its own channel, and the test has clearer control over which request is allowed to proceed.

I just tried implementing this, but it actually don't because of how the call is set up. The test interface blocks in the cloudprovider function call, while logic that aborts the operation occurs in the layer above that. So if the operation is aborted, it will never make it to the cloud provider call. This means that the channel of channels won't get a chance to create and send the reply channel.

The problem arises from the fact that merging the two independent channels means that the caller will need to block on the "main" channel in to get the channel that allows the call to continue to completion. In the current setup, the operation that should be aborted has the "go ahead" SnapshotSourceAndTarget prequeued so that we can directly just go and check the error channel. In this set up, in order to give the go ahead we need the reply channel that is generated by the function call, but because that call is aborted, the reply channel isn't created.

I think you can still cause the first call to block while the second call gets an abort error. You don't need to block or unblock any threads for that. I'm a little sleep deprived, so maybe not understanding the concurrency correctly.

davidz627

this is awesome! Just a couple small final comments

davidz627 · 2019-06-28T21:16:05Z

pkg/gce-cloud-provider/compute/gce-compute.go

@@ -60,6 +60,12 @@ type GCECompute interface {
 	GetSnapshot(ctx context.Context, snapshotName string) (*compute.Snapshot, error)
 	CreateSnapshot(ctx context.Context, volKey *meta.Key, snapshotName string) (*compute.Snapshot, error)
 	DeleteSnapshot(ctx context.Context, snapshotName string) error
+	// Metadata
+	GetProject() string


This breaks the abstraction, the project comes from the metadata service not the Compute service

davidz627 · 2019-06-28T21:16:59Z

pkg/gce-pd-csi-driver/controller.go

+		return nil, status.Errorf(codes.InvalidArgument, "CreateVolume replication type '%s' is not supported", replicationType)
+	}
+
+	volumeID, err := common.KeyToVolumeID(volKey, gceCS.CloudProvider.GetProject())


gceCS.MetadataService.GetProject()

davidz627 · 2019-06-28T21:18:20Z

pkg/gce-pd-csi-driver/controller.go

 	}

 	attached, err := diskIsAttachedAndCompatible(deviceName, instance, volumeCapability, readWrite)
 	if err != nil {
-		return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("Disk %v already published to node %v but incompatbile: %v", volKey.Name, nodeID, err))
+		return nil, status.Errorf(codes.AlreadyExists, "Disk %v already published to node %v but incompatbile: %v", volKey.Name, nodeID, err)


I'm not gonna make you do this now but in the future this type of unrelated change should go in a seperate PR

+1

The added diffs make it harder to review this PR.

pkg/gce-pd-csi-driver/controller.go

pkg/gce-pd-csi-driver/controller_test.go

pkg/gce-pd-csi-driver/gce-pd-driver_test.go

pkg/common/volume_lock.go

misterikkit · 2019-06-28T21:47:50Z

pkg/gce-pd-csi-driver/controller.go

 	}

 	attached, err := diskIsAttachedAndCompatible(deviceName, instance, volumeCapability, readWrite)
 	if err != nil {
-		return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("Disk %v already published to node %v but incompatbile: %v", volKey.Name, nodeID, err))
+		return nil, status.Errorf(codes.AlreadyExists, "Disk %v already published to node %v but incompatbile: %v", volKey.Name, nodeID, err)


+1

The added diffs make it harder to review this PR.

misterikkit · 2019-06-28T21:49:54Z

pkg/gce-pd-csi-driver/controller_test.go

+		SourceVolumeId: testVolumeId + "2",
+	}
+
+	runRequest := func(req *csi.CreateSnapshotRequest) chan error {


nit: to make this a little more readable, make the return type <-chan error

pkg/gce-pd-csi-driver/controller_test.go

davidz627

mostly lgtm

pkg/gce-pd-csi-driver/controller_test.go

pkg/common/volume_lock.go

pkg/mount-manager/fake-safe-mounter.go

jingxu97 · 2019-07-01T19:24:04Z

pkg/gce-pd-csi-driver/node.go

@@ -39,7 +38,7 @@ type GCENodeServer struct {

 	// A map storing all volumes with ongoing operations so that additional operations


update the comments after changing to volumeLocks?

I'm not sure that a comment is actually even needed here, as what VolumeLocks does is explained by the source common.VolumeLocks

pkg/gce-pd-csi-driver/node.go

davidz627 · 2019-07-02T23:19:53Z

/lgtm
/approve

k8s-ci-robot · 2019-07-02T23:20:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: davidz627, hantaowang

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:

~~OWNERS~~ [davidz627]

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

…ze-attach-ops" This reverts commit 41f103f, reversing changes made to 965ba6a.

davidz627 · 2019-10-11T21:02:59Z

/kind feature

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 24, 2019

k8s-ci-robot requested review from jingxu97 and verult June 24, 2019 22:08

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 24, 2019

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 24, 2019

k8s-ci-robot assigned davidz627 Jun 24, 2019

msau42 reviewed Jun 24, 2019

View reviewed changes

pkg/gce-pd-csi-driver/controller.go Outdated Show resolved Hide resolved

pkg/gce-pd-csi-driver/controller.go Outdated Show resolved Hide resolved

hantaowang force-pushed the seralize-attach-ops branch from 896ea45 to 8f91f4e Compare June 24, 2019 23:49

davidz627 reviewed Jun 27, 2019

View reviewed changes

k8s-ci-robot requested a review from msau42 June 27, 2019 01:44

misterikkit reviewed Jun 27, 2019

View reviewed changes

hantaowang force-pushed the seralize-attach-ops branch from adcaa53 to a8fb9fb Compare June 27, 2019 19:05

hantaowang closed this Jun 27, 2019

hantaowang reopened this Jun 27, 2019

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 27, 2019

hantaowang force-pushed the seralize-attach-ops branch from d61ff34 to 130a31e Compare June 27, 2019 23:53

davidz627 reviewed Jun 28, 2019

View reviewed changes

misterikkit reviewed Jun 28, 2019

View reviewed changes

hantaowang force-pushed the seralize-attach-ops branch from d45e50d to c8759d2 Compare June 28, 2019 22:17

k8s-ci-robot removed the size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. label Jun 28, 2019

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jun 28, 2019

hantaowang force-pushed the seralize-attach-ops branch from d248c8f to 0d372dd Compare June 28, 2019 22:24

davidz627 reviewed Jun 28, 2019

View reviewed changes

pkg/gce-pd-csi-driver/controller_test.go Outdated Show resolved Hide resolved

pkg/common/volume_lock.go Outdated Show resolved Hide resolved

pkg/mount-manager/fake-safe-mounter.go Outdated Show resolved Hide resolved

add volume level seralization for controller operations

53a286c

hantaowang force-pushed the seralize-attach-ops branch from 0d372dd to 53a286c Compare June 28, 2019 23:35

jingxu97 reviewed Jul 1, 2019

View reviewed changes

pkg/gce-pd-csi-driver/node.go Show resolved Hide resolved

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 2, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 2, 2019

k8s-ci-robot merged commit 41f103f into kubernetes-sigs:master Jul 3, 2019

hantaowang pushed a commit to hantaowang/gcp-compute-persistent-disk-csi-driver that referenced this pull request Jul 4, 2019

Revert "Merge pull request kubernetes-sigs#316 from hantaowang/serali…

37c75e9

…ze-attach-ops" This reverts commit 41f103f, reversing changes made to 965ba6a.

hantaowang pushed a commit to hantaowang/gcp-compute-persistent-disk-csi-driver that referenced this pull request Jul 4, 2019

Revert "Merge pull request kubernetes-sigs#316 from hantaowang/serali…

f53a6ab

…ze-attach-ops" This reverts commit 41f103f, reversing changes made to 965ba6a.

misterikkit mentioned this pull request Oct 2, 2019

REQUEST: New membership for misterikkit kubernetes/org#1249

Closed

6 tasks

k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Oct 11, 2019

misterikkit mentioned this pull request Oct 24, 2019

REQUEST: New membership for misterikkit kubernetes/org#1337

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add volume level serialization for controller operations #316

Add volume level serialization for controller operations #316

hantaowang commented Jun 24, 2019 •

edited by davidz627

Loading

k8s-ci-robot commented Jun 24, 2019

msau42 commented Jun 24, 2019

msau42 commented Jun 24, 2019

hantaowang commented Jun 25, 2019

davidz627 left a comment

davidz627 Jun 27, 2019

k8s-ci-robot commented Jun 27, 2019

misterikkit Jun 27, 2019

hantaowang Jun 27, 2019

hantaowang Jun 27, 2019 •

edited

Loading

misterikkit Jun 27, 2019

davidz627 left a comment

davidz627 Jun 28, 2019

davidz627 Jun 28, 2019

davidz627 Jun 28, 2019

misterikkit Jun 28, 2019

misterikkit Jun 28, 2019

misterikkit Jun 28, 2019

davidz627 left a comment

jingxu97 Jul 1, 2019

hantaowang Jul 2, 2019 •

edited

Loading

davidz627 commented Jul 2, 2019

k8s-ci-robot commented Jul 2, 2019

davidz627 commented Oct 11, 2019

		@@ -39,7 +38,7 @@ type GCENodeServer struct {

		// A map storing all volumes with ongoing operations so that additional operations

Add volume level serialization for controller operations #316

Add volume level serialization for controller operations #316

Conversation

hantaowang commented Jun 24, 2019 • edited by davidz627 Loading

k8s-ci-robot commented Jun 24, 2019

msau42 commented Jun 24, 2019

msau42 commented Jun 24, 2019

hantaowang commented Jun 25, 2019

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 27, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantaowang Jun 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidz627 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

davidz627 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hantaowang Jul 2, 2019 • edited Loading

Choose a reason for hiding this comment

davidz627 commented Jul 2, 2019

k8s-ci-robot commented Jul 2, 2019

davidz627 commented Oct 11, 2019

hantaowang commented Jun 24, 2019 •

edited by davidz627

Loading

hantaowang Jun 27, 2019 •

edited

Loading

hantaowang Jul 2, 2019 •

edited

Loading