Repository: kubernetes-sigs/gcp-filestore-csi-driver
Pull request #311 (2022-06-21 18:21:28)

Check for instance READY state before placing a share on it.
<!--  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**:

**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

```


k8s-ci-robot: @tyuchn: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our [release note process](https://git.k8s.io/community/contributors/guide/release-notes.md) to remove it.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>
k8s-ci-robot: Hi @tyuchn. Thanks for your PR.

I'm waiting for a [kubernetes-sigs](https://github.com/orgs/kubernetes-sigs/people) 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](https://git.k8s.io/community/community-membership.md#member) 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](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-filestore-csi-driver).

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.
</details>

k8s-ci-robot: [APPROVALNOTIFIER] This PR is **NOT APPROVED**

This pull-request has been approved by: *<a href="https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/pull/311#" title="Author self-approved">tyuchn</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **saad-ali** after the PR has been reviewed.
You can assign the PR to them by writing `/assign @saad-ali` in a comment when ready.

The full list of commands accepted by this bot can be found [here](https://go.k8s.io/bot-commands?repo=kubernetes-sigs%2Fgcp-filestore-csi-driver).

<details open>
Needs approval from an approver in each of these files:

- **[OWNERS](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/blob/master/OWNERS)**

Approvers can indicate their approval by writing `/approve` in a comment
Approvers can cancel approval by writing `/approve cancel` in a comment
</details>
<!-- META={"approvers":["saad-ali"]} -->
tyuchn: /ok-to-test
saikat-royc: /lgtm
/approve
k8s-ci-robot: @tyuchn: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-filestore-csi-driver-unit | e173c207fa93cc854c4dcf659de2dc8cb32c8c0d | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-filestore-csi-driver/311/pull-gcp-filestore-csi-driver-unit/1539383085097816064/) | true | `/test pull-gcp-filestore-csi-driver-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-filestore-csi-driver&pr=311). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Atyuchn). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @tyuchn: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-filestore-csi-driver-unit | e173c207fa93cc854c4dcf659de2dc8cb32c8c0d | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-filestore-csi-driver/311/pull-gcp-filestore-csi-driver-unit/1539385831284805632/) | true | `/test pull-gcp-filestore-csi-driver-unit`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-filestore-csi-driver&pr=311). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Atyuchn). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: New changes are detected. LGTM label has been removed.
tyuchn: /retest
k8s-ci-robot: @tyuchn: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-filestore-csi-driver-kubernetes-integration | 4c76cf209b8f0148919b2b0fcb943f5da6e60279 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-filestore-csi-driver/311/pull-gcp-filestore-csi-driver-kubernetes-integration/1539391144901742592/) | false | `/test pull-gcp-filestore-csi-driver-kubernetes-integration`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-filestore-csi-driver&pr=311). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Atyuchn). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->
k8s-ci-robot: @tyuchn: The following test **failed**, say `/retest` to rerun all failed tests or `/retest-required` to rerun all mandatory failed tests:

Test name | Commit | Details | Required | Rerun command
--- | --- | --- | --- | ---
pull-gcp-filestore-csi-driver-kubernetes-integration | 4c76cf209b8f0148919b2b0fcb943f5da6e60279 | [link](https://prow.k8s.io/view/gs/kubernetes-jenkins/pr-logs/pull/sigs.k8s.io_gcp-filestore-csi-driver/311/pull-gcp-filestore-csi-driver-kubernetes-integration/1539464493598248960/) | false | `/test pull-gcp-filestore-csi-driver-kubernetes-integration`

[Full PR test history](https://prow.k8s.io/pr-history?org=kubernetes-sigs&repo=gcp-filestore-csi-driver&pr=311). [Your PR dashboard](https://prow.k8s.io/pr?query=is%3Apr%20state%3Aopen%20author%3Atyuchn). Please help us cut down on flakes by [linking to](https://git.k8s.io/community/contributors/devel/sig-testing/flaky-tests.md#filing-issues-for-flaky-tests) an [open issue](https://github.com/kubernetes-sigs/gcp-filestore-csi-driver/issues?q=is:issue+is:open) when you hit one in your PR.

<details>

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md).  If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
</details>
<!-- test report -->

Patch:
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) {