Repository: helm/helm (go)

Issue:
StatefulSet never ready on no-op deployment with partitions.
Hi,

We're running into an issue where our StatefulSets are never considered ready when using `partition` in the `updateStrategy.rollingUpdate`.

The scenario is as follows:

* A StatefulSet with 3 replicas is set to `partition: 3`.
* A chart is applied which doesn't cause a change to the statefulSet.
* When waiting for the statefulset, this message appears:
```
StatefulSet is not ready: namespace-example/statefulset-example. 3 out of 0 expected pods have been scheduled
```

In the StatefulSet's status, the `currentRevision` matches the `updateRevision` and both the `currentReplicas` and `updatedReplicas` are set to `3`.

The related code here: https://github.com/helm/helm/blob/662090a37e977922828779d249f9cc4f8fc7ed1d/pkg/kube/wait.go#L321-L331 also matches the behaviour

`expectedReplicas := replicas - partition` (equals 0)

and `int(sts.Status.UpdatedReplicas) != expectedReplicas` since `UpdatedReplicas` is 3.

Getting a StatefulSet in this state can happen when updating the partition attribute after a rolling update:

* Start with a STS partition: 3
* Deploy a change.
* Rolling update by setting partition 0.
* Set partition back to 3.

I'm not sure if this would catch other cases, but it seems like taking the revisions into account would work:

```
expectedReplicas := replicas
if (sts.Status.UpdateRevision != sts.Status.CurrentRevision) {
   expectedReplicas -= partition
}
```

<!-- If you need help or think you have found a bug, please help us with your issue by entering the following information (otherwise you can delete this text): -->

Output of `helm version`: 

```
version.BuildInfo{Version:"v3.2.4", GitCommit:"0ad800ef43d3b826f31a5ad8dfbb4fe05d143688", GitTreeState:"dirty", GoVersion:"go1.14.3"}
```

Output of `kubectl version`:

```
Client Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.10", GitCommit:"575467a0eaf3ca1f20eb86215b3bde40a5ae617a", GitTreeState:"clean", BuildDate:"2019-12-11T12:41:00Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"14", GitVersion:"v1.14.10", GitCommit:"575467a0eaf3ca1f20eb86215b3bde40a5ae617a", GitTreeState:"clean", BuildDate:"2019-12-11T12:32:32Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}
```

Cloud Provider/Platform (AKS, GKE, Minikube etc.): N/A




Pull request description:
fix(pkg/kube): statefulSetReady: handle partition cases correctly
**What this PR does / why we need it**:
The partition value can be greater than number of replicas, in that case no pods are rolled out. The expectedReplicas becomes a negative number. https://kubernetes.io/docs/concepts/workloads/controllers/statefulset/#partitions

In the cases where the update does not change anything in the pod template, the updatedReplicas value from StatefulSet status remains unchanged. Such updates can still set some partition value, and UpdatedReplicas is always greater than expectedReplicas. Basically, the StatefulSet is ready / rolled-out.

In both the above scenarios, providing `--wait` flag causes it to timeout waiting indefinitely. Because updatedReplicas can never be negative, or be equal to the expectedReplicas for the second case.

This commit handles both the above scenarios by checking if UpdatedReplicas is smaller than expectedReplicas. If it is, then the StatefulSet is not ready yet.

**Special notes for your reviewer**:
I have done the following manual testing for this change, in all the cases the update succeeds as expected, and the wait flag waits till the pods are rolled-out.

1. Download and install a chart where partitioning + StatefulSets are used.  
   Partition: 0, Replicas: 3

   <details>
   <summary>Commands</summary>
   <pre>
   curl -O https://charts.yugabyte.com/yugabyte-2.11.0.tgz
   ~/src/helm/bin/helm install yb ./yugabyte-2.11.0.tgz \
     --namespace helm-test --create-namespace \
     --set resource.master.requests.cpu=0.25 \
     --set resource.master.requests.memory=0.35Gi \
     --set resource.tserver.requests.cpu=0.25 \
     --set resource.tserver.requests.memory=0.35Gi \
     --set storage.ephemeral=true \
     --wait
   </pre>
   </details>

2. Do a partitioned update where there is a change in the template.  
   expectedReplicas: 2, updatedReplicas: 2, partition: 1

   <details>
   <summary>Commands</summary>
   <pre>
   ~/src/helm/bin/helm upgrade yb ./yugabyte-2.11.0.tgz \
     --namespace helm-test --create-namespace \
     --set resource.master.requests.cpu=0.25 \
     --set resource.master.requests.memory=0.35Gi \
     --set resource.tserver.requests.cpu=0.25 \
     --set resource.tserver.requests.memory=0.35Gi \
     --set storage.ephemeral=true \
     --set gflags.master.default_memory_limit_to_ram_ratio=0.87 \
     --set partition.master=1 \
     --wait
   </pre>
   </details>

3. Complete the partitioned update. (All three replicas are updated)  
   expectedReplicas: 1, updatedReplicas: 1, partition: 0

   <details>
   <summary>Commands</summary>
   <pre>
   ~/src/helm/bin/helm upgrade yb ./yugabyte-2.11.0.tgz \
     --namespace helm-test --create-namespace \
     --set resource.master.requests.cpu=0.25 \
     --set resource.master.requests.memory=0.35Gi \
     --set resource.tserver.requests.cpu=0.25 \
     --set resource.tserver.requests.memory=0.35Gi \
     --set storage.ephemeral=true \
     --set gflags.master.default_memory_limit_to_ram_ratio=0.87 \
     --set partition.master=0 \
     --wait
   </pre>
   </details>

4. Do a no-op update with partition value.  
   expectedReplicas: 2, updatedReplicas: 3 (because nothing has changed), partition: 1

   <details>
   <summary>Commands</summary>
   <pre>
   ~/src/helm/bin/helm upgrade yb ./yugabyte-2.11.0.tgz \
     --namespace helm-test --create-namespace \
     --set resource.master.requests.cpu=0.25 \
     --set resource.master.requests.memory=0.35Gi \
     --set resource.tserver.requests.cpu=0.25 \
     --set resource.tserver.requests.memory=0.35Gi \
     --set storage.ephemeral=true \
     --set gflags.master.default_memory_limit_to_ram_ratio=0.87 \
     --set partition.master=1 \
     --wait
   </pre>
   </details>

5. Change in the pod template with partition value greater than replicas.  
   expectedReplicas: -2, updatedReplicas: 3 (no pods will change), partition: 5

   <details>
   <summary>Commands</summary>
   <pre>
   ~/src/helm/bin/helm upgrade yb ./yugabyte-2.11.0.tgz \
     --namespace helm-test --create-namespace \
     --set resource.master.requests.cpu=0.25 \
     --set resource.master.requests.memory=0.35Gi \
     --set resource.tserver.requests.cpu=0.25 \
     --set resource.tserver.requests.memory=0.35Gi \
     --set storage.ephemeral=true \
     --set gflags.master.default_memory_limit_to_ram_ratio=0.85 \
     --set partition.master=5 \
     --wait
   </pre>
   </details>

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility

Based on the code from kubectl rollout:
https://github.com/kubernetes/kubectl/blob/a450ebd59c1e8917df23d37c6f05d8e16c3746aa/pkg/polymorphichelpers/rollout_status.go#L138-L141

Closes #8674

Patch:
diff --git a/pkg/kube/ready.go b/pkg/kube/ready.go
index 106c0be51..5d080d9bf 100644
--- a/pkg/kube/ready.go
+++ b/pkg/kube/ready.go
@@ -377,7 +377,7 @@ func (c *ReadyChecker) statefulSetReady(sts *appsv1.StatefulSet) bool {
 	expectedReplicas := replicas - partition
 
 	// Make sure all the updated pods have been scheduled
-	if int(sts.Status.UpdatedReplicas) != expectedReplicas {
+	if int(sts.Status.UpdatedReplicas) < expectedReplicas {
 		c.log("StatefulSet is not ready: %s/%s. %d out of %d expected pods have been scheduled", sts.Namespace, sts.Name, sts.Status.UpdatedReplicas, expectedReplicas)
 		return false
 	}


Test patch:
diff --git a/pkg/kube/ready_test.go b/pkg/kube/ready_test.go
index cece5352d..931b8fa19 100644
--- a/pkg/kube/ready_test.go
+++ b/pkg/kube/ready_test.go
@@ -157,10 +157,24 @@ func Test_ReadyChecker_statefulSetReady(t *testing.T) {
 		{
 			name: "statefulset is not ready when partition is set",
 			args: args{
-				sts: newStatefulSet("foo", 1, 1, 1, 1),
+				sts: newStatefulSet("foo", 2, 1, 1, 0),
 			},
 			want: false,
 		},
+		{
+			name: "statefulset is ready when partition is set and no change in template",
+			args: args{
+				sts: newStatefulSet("foo", 2, 1, 2, 2),
+			},
+			want: true,
+		},
+		{
+			name: "statefulset is ready when partition is greater than replicas",
+			args: args{
+				sts: newStatefulSet("foo", 1, 2, 1, 1),
+			},
+			want: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {