Repository: dell/csi-baremetal
Pull request #484 (2021-08-11 08:47:17)

[ISSUE-483] Race condition between Extender and Controller on disk replacement
## Purpose
### Issue #483

Don't release capacity of unhealthy volume

## PR checklist
- [x] Add link to the issue
- [x] Choose PR label
- [ ] New unit tests added
- [x] Modified code has meaningful comments
- [ ] All TODOs are linked with the issues
- [ ] All comments are resolved

## Testing

- Mark drive as failed
```
borism1@ubuntu:/workspace/csi-baremetal$ kubectl get drives | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
8e78d923-a33d-40a6-8130-90d4113b8d4c   VDH1599D           GOOD     HDD    4b3d480d-f892-4b9d-9bcb-16bfda076134   8001000000000   14
borism1@ubuntu:/workspace/csi-baremetal$ 
borism1@ubuntu:/workspace/csi-baremetal$ kubectl get volume | grep pvc-2dafca9f-34d7-4e8f-9d50-722c80d3faa6
pvc-2dafca9f-34d7-4e8f-9d50-722c80d3faa6   GOOD     4b3d480d-f892-4b9d-9bcb-16bfda076134   8001000000000   8e78d923-a33d-40a6-8130-90d4113b8d4c   HDD             PUBLISHED

borism1@ubuntu:/workspace/csi-baremetal$ kubectl get ac | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
33076f01-010a-43dd-836a-53868ce8866a   8e78d923-a33d-40a6-8130-90d4113b8d4c   4b3d480d-f892-4b9d-9bcb-16bfda076134   HDD             

borism1@ubuntu:/workspace/csi-baremetal$ kubectl describe ac 33076f01-010a-43dd-836a-53868ce8866a | grep Generation
  Generation:          4

borism1@ubuntu:/workspace/csi-baremetal$ kubectl annotate drive 8e78d923-a33d-40a6-8130-90d4113b8d4c health=bad
drive.csi-baremetal.dell.com/8e78d923-a33d-40a6-8130-90d4113b8d4c annotated

borism1@ubuntu:/workspace/csi-baremetal$ kubectl get drives | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
8e78d923-a33d-40a6-8130-90d4113b8d4c   VDH1599D           BAD      HDD    4b3d480d-f892-4b9d-9bcb-16bfda076134   8001000000000   14
borism1@ubuntu:/workspace/csi-baremetal$ kubectl get volumes | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
pvc-2dafca9f-34d7-4e8f-9d50-722c80d3faa6   BAD      4b3d480d-f892-4b9d-9bcb-16bfda076134   8001000000000   8e78d923-a33d-40a6-8130-90d4113b8d4c   HDD             PUBLISHED
```
- Delete PVC+Pod 
```
borism1@ubuntu:/workspace/csi-baremetal$ kubectl delete pvc logs-web-2 --wait=false
persistentvolumeclaim "logs-web-2" deleted
borism1@ubuntu:/workspace/csi-baremetal$ kubectl delete pod web-2 --wait=false
pod "web-2" deleted
borism1@ubuntu:/workspace/csi-baremetal$ kubectl get volumes | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
borism1@ubuntu:/workspace/csi-baremetal$ 
```
- AC size is nil, CR wasn't modified on volume deletion:
```
borism1@ubuntu:/workspace/csi-baremetal$ kubectl get ac | grep 8e78d923-a33d-40a6-8130-90d4113b8d4c
33076f01-010a-43dd-836a-53868ce8866a   8e78d923-a33d-40a6-8130-90d4113b8d4c   4b3d480d-f892-4b9d-9bcb-16bfda076134   HDD 
borism1@ubuntu:/workspace/csi-baremetal$ kubectl describe ac 33076f01-010a-43dd-836a-53868ce8866a | grep Generation
  Generation:          4
```

codecov[bot]: # [Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) Report
> Merging [#484](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) (8f328bd) into [master](https://codecov.io/gh/dell/csi-baremetal/commit/740c92d9d6a2879f20029b70e1763b2758ba9973?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) (740c92d) will **not change** coverage.
> The diff coverage is `0.00%`.

[![Impacted file tree graph](https://codecov.io/gh/dell/csi-baremetal/pull/484/graphs/tree.svg?width=650&height=150&src=pr&token=4D8WVNWX5O&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)

```diff
@@           Coverage Diff           @@
##           master     #484   +/-   ##
=======================================
  Coverage   59.13%   59.13%           
=======================================
  Files          60       60           
  Lines        6003     6003           
=======================================
  Hits         3550     3550           
  Misses       2246     2246           
  Partials      207      207           
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `59.13% <0.00%> (ø)` | |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell) | Coverage Δ | |
|---|---|---|
| [pkg/common/volume\_operations.go](https://codecov.io/gh/dell/csi-baremetal/pull/484/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell#diff-cGtnL2NvbW1vbi92b2x1bWVfb3BlcmF0aW9ucy5nbw==) | `0.00% <0.00%> (ø)` | |

------

[Continue to review full report at Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=footer&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell). Last update [740c92d...8f328bd](https://codecov.io/gh/dell/csi-baremetal/pull/484?src=pr&el=lastupdated&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dell).


Patch:
diff --git a/pkg/common/volume_operations.go b/pkg/common/volume_operations.go
index 4814ac2..6c7eb69 100644
--- a/pkg/common/volume_operations.go
+++ b/pkg/common/volume_operations.go
@@ -441,8 +441,9 @@ func (vo *VolumeOperationsImpl) UpdateCRsAfterVolumeDeletion(ctx context.Context
 		}
 	}
 
-	// if LogicalVolumeGroup wasn't deleted increase AC size
-	if !isDeleted {
+	// if LogicalVolumeGroup wasn't deleted and health of volume is GOOD increase AC size
+	// We don't increase AC size for unhealthy volume to avoid new allocations on top of unhealthy drive/lvg
+	if !isDeleted && volumeCR.Spec.Health == apiV1.HealthGood {
 		// Increase size of AC using volume size
 		acCR.Spec.Size += volumeCR.Spec.Size
 		if err = vo.k8sClient.UpdateCRWithAttempts(ctx, &acCR, 5); err != nil {
diff --git a/pkg/scheduler/extender/extender.go b/pkg/scheduler/extender/extender.go
index 90d200a..e195f88 100644
--- a/pkg/scheduler/extender/extender.go
+++ b/pkg/scheduler/extender/extender.go
@@ -40,6 +40,7 @@ import (
 	volcrd "github.com/dell/csi-baremetal/api/v1/volumecrd"
 	"github.com/dell/csi-baremetal/pkg/base"
 	"github.com/dell/csi-baremetal/pkg/base/capacityplanner"
+	baseerr "github.com/dell/csi-baremetal/pkg/base/error"
 	fc "github.com/dell/csi-baremetal/pkg/base/featureconfig"
 	"github.com/dell/csi-baremetal/pkg/base/k8s"
 	"github.com/dell/csi-baremetal/pkg/base/util"
@@ -106,7 +107,10 @@ func (e *Extender) FilterHandler(w http.ResponseWriter, req *http.Request) {
 	pod := extenderArgs.Pod
 	requests, err := e.gatherCapacityRequestsByProvisioner(ctxWithVal, pod)
 	if err != nil {
-		extenderRes.Error = err.Error()
+		// not found error is re-triable
+		if err != baseerr.ErrorNotFound {
+			extenderRes.Error = err.Error()
+		}
 		if err := resp.Encode(extenderRes); err != nil {
 			ll.Errorf("Unable to write response %v: %v", extenderRes, err)
 		}
@@ -246,7 +250,7 @@ func (e *Extender) gatherCapacityRequestsByProvisioner(ctx context.Context, pod
 			if err != nil {
 				ll.Errorf("Unable to read PVC %s in NS %s: %v. ", v.PersistentVolumeClaim.ClaimName, pod.Namespace, err)
 				// PVC can be created later. csi-provisioner repeat request if not error.
-				return nil, nil
+				return nil, baseerr.ErrorNotFound
 			}
 			if pvc.Spec.StorageClassName == nil {
 				continue


Test patch:
diff --git a/pkg/scheduler/extender/extender_test.go b/pkg/scheduler/extender/extender_test.go
index 674e9cd..da32c23 100644
--- a/pkg/scheduler/extender/extender_test.go
+++ b/pkg/scheduler/extender/extender_test.go
@@ -37,6 +37,7 @@ import (
 	volcrd "github.com/dell/csi-baremetal/api/v1/volumecrd"
 	"github.com/dell/csi-baremetal/pkg/base"
 	"github.com/dell/csi-baremetal/pkg/base/capacityplanner"
+	baseerr "github.com/dell/csi-baremetal/pkg/base/error"
 	fc "github.com/dell/csi-baremetal/pkg/base/featureconfig"
 	"github.com/dell/csi-baremetal/pkg/base/k8s"
 	annotations "github.com/dell/csi-baremetal/pkg/crcontrollers/operator/common"
@@ -188,7 +189,7 @@ func TestExtender_gatherVolumesByProvisioner_Fail(t *testing.T) {
 	})
 	volumes, err = e.gatherCapacityRequestsByProvisioner(testCtx, &pod)
 	assert.Nil(t, volumes)
-	assert.Nil(t, err) // PVC can be created later
+	assert.Equal(t, err, baseerr.ErrorNotFound) // PVC can be created later
 
 	// PVC doesn't contain information about size
 	pod.Namespace = testNs