Repository: kubernetes-csi/external-provisioner
Pull request #475 (2020-08-31 08:46:39)

capacity: fix handling of topology changes with immediate binding SCs
**What type of PR is this?**
/kind bug

**What this PR does / why we need it**:

A copy-and-pasted "return" instead of "continue" caused processing to
stop after encountering a storage class with immediate binding, which
had the effect that further CSIStorageCapacity objects for other
storage classes never got created.

This only happened when topology changed at runtime. When topology
was already known when the controller started, all expected objects
got created.

**Does this PR introduce a user-facing change?**:

```release-note
CSIStorageCapacity objects were potentially incomplete when at least one storage class used "immediate binding" and topology changed while the controller was already running.
```


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

This pull-request has been approved by: *<a href="https://github.com/kubernetes-csi/external-provisioner/pull/475#" title="Author self-approved">pohly</a>*
To complete the [pull request process](https://git.k8s.io/community/contributors/guide/owners.md#the-code-review-process), please assign **saad-ali**
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-csi%2Fexternal-provisioner).

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

- **[OWNERS](https://github.com/kubernetes-csi/external-provisioner/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"]} -->
pohly: /assign @msau42 

I noticed this bug when trying out the steps in the upcoming blog post https://github.com/kubernetes/website/pull/23102

I'll work around the bug in the example, therefore we don't need to hurry with getting a bug fix release out.

Patch:
diff --git a/pkg/capacity/capacity.go b/pkg/capacity/capacity.go
index 9cd592752..123d5f063 100644
--- a/pkg/capacity/capacity.go
+++ b/pkg/capacity/capacity.go
@@ -299,7 +299,7 @@ func (c *Controller) onTopologyChanges(added []*topology.Segment, removed []*top
 			continue
 		}
 		if !c.immediateBinding && sc.VolumeBindingMode != nil && *sc.VolumeBindingMode == storagev1.VolumeBindingImmediate {
-			return
+			continue
 		}
 		for _, segment := range added {
 			c.addWorkItem(segment, sc)


Test patch:
diff --git a/pkg/capacity/capacity_test.go b/pkg/capacity/capacity_test.go
index 86a2423c4..004676b1e 100644
--- a/pkg/capacity/capacity_test.go
+++ b/pkg/capacity/capacity_test.go
@@ -95,6 +95,7 @@ func TestController(t *testing.T) {
 		expectedCapacities []testCapacity
 		modify             func(ctx context.Context, clientSet *fakeclientset.Clientset, expected []testCapacity) (modifiedExpected []testCapacity, err error)
 		capacityChange     func(ctx context.Context, storage *mockCapacity, expected []testCapacity) (modifiedExpected []testCapacity)
+		topologyChange     func(ctx context.Context, topology *mockTopology, expected []testCapacity) (modifiedExpected []testCapacity)
 	}{
 		"empty": {
 			expectedCapacities: []testCapacity{},
@@ -764,6 +765,162 @@ func TestController(t *testing.T) {
 				return expected
 			},
 		},
+		"add storage topology segment": {
+			topology: mockTopology{},
+			storage: mockCapacity{
+				capacity: map[string]interface{}{
+					// This matches layer0.
+					"foo": "1Gi",
+				},
+			},
+			initialSCs: []testSC{
+				// We intentionally create a SC with immediate binding first.
+				// It needs to be skipped while still processing the other one.
+				// Ordering of the objects is not guaranteed, but in practice
+				// the informer seems to be "first in, first out", which is what
+				// we need.
+				{
+					name:             "immediate-sc",
+					driverName:       driverName,
+					immediateBinding: true,
+				},
+				{
+					name:       "late-sc",
+					driverName: driverName,
+				},
+			},
+			expectedCapacities: nil,
+			topologyChange: func(ctx context.Context, topo *mockTopology, expected []testCapacity) []testCapacity {
+				topo.modify([]*topology.Segment{&layer0} /* added */, nil /* removed */)
+				return append(expected, testCapacity{
+					uid:              "CSISC-UID-1",
+					resourceVersion:  csiscRev + "0",
+					segment:          layer0,
+					storageClassName: "late-sc",
+					quantity:         "1Gi",
+				})
+			},
+		},
+		"add storage topology segment, immediate binding": {
+			immediateBinding: true,
+			topology:         mockTopology{},
+			storage: mockCapacity{
+				capacity: map[string]interface{}{
+					// This matches layer0.
+					"foo": "1Gi",
+				},
+			},
+			initialSCs: []testSC{
+				{
+					name:             "immediate-sc",
+					driverName:       driverName,
+					immediateBinding: true,
+				},
+				{
+					name:       "late-sc",
+					driverName: driverName,
+				},
+			},
+			expectedCapacities: nil,
+			topologyChange: func(ctx context.Context, topo *mockTopology, expected []testCapacity) []testCapacity {
+				topo.modify([]*topology.Segment{&layer0} /* added */, nil /* removed */)
+				// We don't check the UID here because we don't want to fail when
+				// ordering of storage classes isn't such that the "immediate-sc" is seen first.
+				return append(expected, testCapacity{
+					resourceVersion:  csiscRev + "0",
+					segment:          layer0,
+					storageClassName: "immediate-sc",
+					quantity:         "1Gi",
+				},
+					testCapacity{
+						resourceVersion:  csiscRev + "0",
+						segment:          layer0,
+						storageClassName: "late-sc",
+						quantity:         "1Gi",
+					},
+				)
+			},
+		},
+		"remove storage topology segment": {
+			topology: mockTopology{
+				segments: []*topology.Segment{&layer0},
+			},
+			storage: mockCapacity{
+				capacity: map[string]interface{}{
+					// This matches layer0.
+					"foo": "1Gi",
+				},
+			},
+			initialSCs: []testSC{
+				{
+					name:             "immediate-sc",
+					driverName:       driverName,
+					immediateBinding: true,
+				},
+				{
+					name:       "late-sc",
+					driverName: driverName,
+				},
+			},
+			expectedCapacities: []testCapacity{
+				{
+					uid:              "CSISC-UID-1",
+					resourceVersion:  csiscRev + "0",
+					segment:          layer0,
+					storageClassName: "late-sc",
+					quantity:         "1Gi",
+				},
+			},
+			topologyChange: func(ctx context.Context, topo *mockTopology, expected []testCapacity) []testCapacity {
+				topo.modify(nil /* added */, topo.segments[:] /* removed */)
+				return nil
+			},
+		},
+		"add and remove storage topology segment": {
+			topology: mockTopology{
+				segments: []*topology.Segment{&layer0},
+			},
+			storage: mockCapacity{
+				capacity: map[string]interface{}{
+					// This matches layer0.
+					"foo": "1Gi",
+					"bar": "2Gi",
+				},
+			},
+			initialSCs: []testSC{
+				{
+					name:             "immediate-sc",
+					driverName:       driverName,
+					immediateBinding: true,
+				},
+				{
+					name:       "late-sc",
+					driverName: driverName,
+				},
+			},
+			expectedCapacities: []testCapacity{
+				{
+					uid:              "CSISC-UID-1",
+					resourceVersion:  csiscRev + "0",
+					segment:          layer0,
+					storageClassName: "late-sc",
+					quantity:         "1Gi",
+				},
+			},
+			topologyChange: func(ctx context.Context, topo *mockTopology, expected []testCapacity) []testCapacity {
+				topo.modify([]*topology.Segment{&layer0other}, /* added */
+					topo.segments[:] /* removed */)
+				return []testCapacity{
+					{
+						uid:              "CSISC-UID-2",
+						resourceVersion:  csiscRev + "0",
+						segment:          layer0other,
+						storageClassName: "late-sc",
+						quantity:         "2Gi",
+					},
+				}
+			},
+		},
 	}
 
 	for name, tc := range testcases {
@@ -820,6 +977,13 @@ func TestController(t *testing.T) {
 					t.Fatalf("modified capacity: %v", err)
 				}
 			}
+			if tc.topologyChange != nil {
+				klog.Info("modifying topology")
+				expectedCapacities = tc.topologyChange(ctx, &tc.topology, expectedCapacities)
+				if err := validateCapacitiesEventually(ctx, c, clientSet, expectedCapacities); err != nil {
+					t.Fatalf("modified capacity: %v", err)
+				}
+			}
 		})
 	}
 }
@@ -1100,6 +1264,35 @@ func (mt *mockTopology) HasSynced() bool {
 	return true
 }
 
+func (mt *mockTopology) modify(add, remove []*topology.Segment) {
+	var added, removed []*topology.Segment
+	for _, segment := range add {
+		if mt.segmentIndex(segment) == -1 {
+			added = append(added, segment)
+			mt.segments = append(mt.segments, segment)
+		}
+	}
+	for _, segment := range remove {
+		index := mt.segmentIndex(segment)
+		if index != -1 {
+			removed = append(removed, segment)
+			mt.segments = append(mt.segments[0:index], mt.segments[index+1:]...)
+		}
+	}
+	for _, cb := range mt.callbacks {
+		cb(added, removed)
+	}
+}
+
+func (mt *mockTopology) segmentIndex(segment *topology.Segment) int {
+	for i, otherSegment := range mt.segments {
+		if otherSegment == segment {
+			return i
+		}
+	}
+	return -1
+}
+
 type testCapacity struct {
 	uid              types.UID
 	resourceVersion  string