Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #313 (2022-06-22 18:26:13)

Cherrypick #311, #312 to release-1.2
<!--  Thanks for sending a pull request!  Here are some tips for you:

1. If this is your first time, please read our contributor guidelines: https://git.k8s.io/community/contributors/guide#your-first-contribution and developer guide https://git.k8s.io/community/contributors/devel/development.md#development-guide
2. Follow the instructions for writing a release note: https://git.k8s.io/community/contributors/guide/release-notes.md
3. If the PR is unfinished, see how to mark it: https://git.k8s.io/community/contributors/guide/pull-requests.md#marking-unfinished-pull-requests
-->

**What type of PR is this?**
> Uncomment only one ` /kind <>` line, hit enter to put that in a new line, and remove leading whitespaces from that line:
>
> /kind api-change
> /kind bug
> /kind cleanup
> /kind design
/kind documentation
> /kind failing-test
> /kind feature
> /kind flake

**What this PR does / why we need it**:
Cherrypick #311, #312 to release-1.2
**Which issue(s) this PR fixes**:
<!--
*Automatically closes linked issue when PR is merged.
Usage: `Fixes #<issue number>`, or `Fixes (paste link of issue)`.
_If PR is about `failing-tests or flakes`, please post the related issues/tests in a comment and do not use `Fixes`_*
-->
Fixes #

**Special notes for your reviewer**:

**Does this PR introduce a user-facing change?**:
<!--
If no, just write "NONE" in the release-note block below.
If yes, a release note is required:
Enter your extended release note in the block below. If the PR requires additional action from users switching to the new release, include the string "action required".
-->
```release-note
NONE
```



Patch:
diff --git a/CHANGELOG/CHANGELOG-1.2.md b/CHANGELOG/CHANGELOG-1.2.md
index 3b9a80cc..8304b280 100644
--- a/CHANGELOG/CHANGELOG-1.2.md
+++ b/CHANGELOG/CHANGELOG-1.2.md
@@ -1,3 +1,11 @@
+# v1.2.6 - Changelog since v1.2.5
+
+## Changes by Kind
+
+### Bug or Regression
+
+- Check for instance READY state before placing a share on it ([#311](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/311), [@tyuchn](https://github.com/tyuchn))
+
 # v1.2.5 - Changelog since v1.2.4
 
 ## Changes by Kind
diff --git a/pkg/csi_driver/multishare_ops_manager.go b/pkg/csi_driver/multishare_ops_manager.go
index ead8fd66..8ac864f9 100644
--- a/pkg/csi_driver/multishare_ops_manager.go
+++ b/pkg/csi_driver/multishare_ops_manager.go
@@ -321,9 +321,23 @@ func (m *MultishareOpsManager) runEligibleInstanceCheck(ctx context.Context, ins
 	if err != nil {
 		return nil, 0, err
 	}
+	// An instance is considered as eligible if and only if the state is 'READY', and there's no ops running against it.
 	var readyEligibleInstances []*file.MultishareInstance
+	// An instance is considered as non-ready if the state is "CREATING", or the state is 'READY' but running ops are found on it.
 	nonReadyInstanceCount := 0
+
 	for _, instance := range instances {
+		if instance.State == "CREATING" {
+			klog.Infof("Instance %s/%s/%s with state %s is not ready", instance.Project, instance.Location, instance.Name, instance.State)
+			nonReadyInstanceCount += 1
+			continue
+		}
+		if instance.State != "READY" {
+			klog.Infof("Instance %s/%s/%s with state %s is not eligible", instance.Project, instance.Location, instance.Name, instance.State)
+			continue
+			// TODO: If we saw instance states other than "CREATING" and "READY", we may need to do some special handlding in the future.
+		}
+
 		op, err := containsOpWithInstanceTargetPrefix(instance, ops)
 		if err != nil {
 			klog.Errorf("failed to check eligibility of instance %s", instance.Name)
@@ -345,17 +359,7 @@ func (m *MultishareOpsManager) runEligibleInstanceCheck(ctx context.Context, ins
 			continue
 		}
 
-		// instances whose delete is in progress should not be accounted as non-ready instances.
-		if op.Type == util.InstanceDelete {
-			continue
-		}
-
-		if instance.State == "DELETING" || instance.State == "ERROR" {
-			klog.V(5).Infof("Instance %s/%s/%s with state %s is not eligible", instance.Project, instance.Location, instance.Name, instance.State)
-			continue
-		}
-
-		klog.Infof("Instance %s/%s/%s is not ready with ongoing operation %s type %s", instance.Project, instance.Location, instance.Name, op.Id, op.Type.String())
+		klog.Infof("Instance %s/%s/%s with state %s is not ready with ongoing operation %s type %s", instance.Project, instance.Location, instance.Name, instance.State, op.Id, op.Type.String())
 		nonReadyInstanceCount += 1
 		// TODO: If we see > 1 instances with 0 shares (these could be possibly leaked instances where the driver hit timeout during creation op was in progress), should we trigger delete op for such instances? Possibly yes. Given that instance create/delete and share create/delete is serialized, maybe yes.
 	}


Test patch:
diff --git a/pkg/csi_driver/multishare_controller_test.go b/pkg/csi_driver/multishare_controller_test.go
index b457c9d7..661fc608 100644
--- a/pkg/csi_driver/multishare_controller_test.go
+++ b/pkg/csi_driver/multishare_controller_test.go
@@ -765,6 +765,7 @@ func TestMultishareCreateVolume(t *testing.T) {
 					Network: file.Network{
 						Ip: testIP,
 					},
+					State: "READY",
 				},
 				{
 					Name:     testInstanceName2,
@@ -778,6 +779,7 @@ func TestMultishareCreateVolume(t *testing.T) {
 					Network: file.Network{
 						Ip: testIP,
 					},
+					State: "READY",
 				},
 			},
 			ops: []OpItem{
diff --git a/pkg/csi_driver/multishare_ops_manager_test.go b/pkg/csi_driver/multishare_ops_manager_test.go
index 64811d9a..1ebea604 100644
--- a/pkg/csi_driver/multishare_ops_manager_test.go
+++ b/pkg/csi_driver/multishare_ops_manager_test.go
@@ -1218,6 +1218,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 				{
 					Name:     "test-instance-1",
@@ -1226,6 +1227,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedReadyInstance: []*file.MultishareInstance{
@@ -1236,6 +1238,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 				{
 					Name:     "test-instance-2",
@@ -1244,6 +1247,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 		},
@@ -1258,6 +1262,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedNonReadyCount: 1,
@@ -1280,6 +1285,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedNonReadyCount: 1,
@@ -1302,6 +1308,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedNonReadyCount: 1,
@@ -1324,6 +1331,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedNonReadyCount: 1,
@@ -1346,6 +1354,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "DELETING",
 				},
 			},
 			ops: []*OpInfo{
@@ -1367,6 +1376,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 				{
 					Name:     "instance-2",
@@ -1375,6 +1385,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedReadyInstance: []*file.MultishareInstance{
@@ -1385,6 +1396,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			expectedNonReadyCount: 1,
@@ -1407,6 +1419,7 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 					Labels: map[string]string{
 						util.ParamMultishareInstanceScLabelKey: "testprefix",
 					},
+					State: "READY",
 				},
 			},
 			initShares: []*file.Share{
@@ -1522,6 +1535,99 @@ func TestRunEligibleInstanceCheck(t *testing.T) {
 				},
 			},
 		},
+		{
+			name:   "ready instance, non-ready instances, other instance state not count",
+			prefix: "testprefix",
+			initInstances: []*file.MultishareInstance{
+				{
+					Name:     "instance-1",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "CREATING",
+				},
+				{
+					Name:     "instance-2",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "REPAIRING",
+				},
+				{
+					Name:     "instance-3",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "READY",
+				},
+				{
+					Name:     "instance-4",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "READY",
+				},
+			},
+			expectedReadyInstance: []*file.MultishareInstance{
+				{
+					Name:     "instance-3",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "READY",
+				},
+			},
+			expectedNonReadyCount: 2,
+			ops: []*OpInfo{
+				{
+					Id:     "op1",
+					Target: "projects/test-project/locations/us-central1/instances/instance-4/shares/share-1",
+					Type:   util.ShareDelete,
+				},
+			},
+		},
+		{
+			name:   "creating instance count as non-ready",
+			prefix: "testprefix",
+			initInstances: []*file.MultishareInstance{
+				{
+					Name:     "instance-1",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "CREATING",
+				},
+				{
+					Name:     "instance-2",
+					Location: "us-central1",
+					Project:  "test-project",
+					Labels: map[string]string{
+						util.ParamMultishareInstanceScLabelKey: "testprefix",
+					},
+					State: "ERROR",
+				},
+			},
+			expectedNonReadyCount: 1,
+			ops: []*OpInfo{
+				{
+					Id:     "op1",
+					Target: "projects/test-project/locations/us-central1/instances/instance-1",
+					Type:   util.InstanceCreate,
+				},
+			},
+		},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {