Repository: argoproj/argo-rollouts (go)

Issue:
Add conditions to allow to wait for rollout pauses
We have a workflow where we use `make` to rollout a new version, wait until the preview deployment is available, run some smoke tests, and then promote or rollback based on the result. Pretty basic. But for this to work we have to figure out when blue green rollout has paused.

I have found the `kubectl wait` command and looked a bit at how it works, waiting for conditions. But it seems it blocks the flow until the condition has it's `status==true`, which is not the case for paused BlueGreen rollouts. I didn't find a different way to wait for other metadata information in an object, so I had to implement my own loop and put it in a separate shell script.

I'd prefer to be able to use something like `kubectl wait rollout $rollout --for=condition=PausedForPreview` or something like that.

I haven't looked into the way canaries behave, but this would also be beneficial for canaries with (multiple) indefinite pause steps.

---

For reference:

Paused BlueGreen rollout (`kubectl get rollout $rollout --output=json | jq .status`):
```json
{
  "HPAReplicas": 1,
  "availableReplicas": 1,
  "blueGreen": {
    "activeSelector": "6c4444d9b",
    "previewSelector": "cd564d8cc",
    "scaleUpPreviewCheckPoint": true
  },
  "canary": {},
  "conditions": [
    {
      "lastTransitionTime": "2020-01-10T14:47:39Z",
      "lastUpdateTime": "2020-01-10T14:47:39Z",
      "message": "Rollout has minimum availability",
      "reason": "AvailableReason",
      "status": "True",
      "type": "Available"
    },
    {
      "lastTransitionTime": "2020-01-13T16:08:34Z",
      "lastUpdateTime": "2020-01-13T16:08:34Z",
      "message": "Rollout is paused",
      "reason": "RolloutPaused",
      "status": "Unknown",
      "type": "Progressing"
    }
  ],
  "controllerPause": true,
  "currentPodHash": "cd564d8cc",
  "observedGeneration": "6b658b7868",
  "pauseConditions": [
    {
      "reason": "BlueGreenPause",
      "startTime": "2020-01-13T16:08:34Z"
    }
  ],
  "readyReplicas": 2,
  "replicas": 2,
  "selector": "app=$rollout,rollouts-pod-template-hash=6c4444d9b",
  "updatedReplicas": 1
}
```

Fully deployed BlueGreen rollout:
```json
{
  "HPAReplicas": 1,
  "availableReplicas": 1,
  "blueGreen": {
    "activeSelector": "cd564d8cc",
    "previewSelector": "cd564d8cc"
  },
  "canary": {},
  "conditions": [
    {
      "lastTransitionTime": "2020-01-10T14:47:39Z",
      "lastUpdateTime": "2020-01-10T14:47:39Z",
      "message": "Rollout has minimum availability",
      "reason": "AvailableReason",
      "status": "True",
      "type": "Available"
    },
    {
      "lastTransitionTime": "2020-01-13T16:09:55Z",
      "lastUpdateTime": "2020-01-13T16:09:55Z",
      "message": "ReplicaSet \"$rollout-cd564d8cc\" has successfully progressed.",
      "reason": "NewReplicaSetAvailable",
      "status": "True",
      "type": "Progressing"
    }
  ],
  "currentPodHash": "cd564d8cc",
  "observedGeneration": "6b658b7868",
  "readyReplicas": 2,
  "replicas": 2,
  "selector": "app=$rollout,rollouts-pod-template-hash=cd564d8cc",
  "updatedReplicas": 1
}
```

This is the script I currently use to wait for that condition:
```
#!/bin/bash

rolloutname=${1?No rollout name provided}
KUBECTL=${KUBECTL:-kubectl}
KUBECTLFLAGS=${KUBECTLFLAGS}
JQ=${JQ:-jq}

set -ux

function is_paused() {
    # if you get 'jq: error (at <stdin>:384): Cannot iterate over null (null)'
    # that means that the object doesn't have a pauseConditions object, probably
    # because it isn't paused yet but still progressing
    $KUBECTL $KUBECTLFLAGS get rollout "$rolloutname" --output=json \
        | $JQ --exit-status '.status.pauseConditions[]|.reason=="BlueGreenPause"'
    return $?
}

function wait_for_pods_ready() {
    spec_hash=$(
        $KUBECTL $KUBECTLFLAGS get rollout "$rolloutname" --output=json \
        | $JQ --exit-status -r '.status.blueGreen.previewSelector'
    )

    $KUBECTL $KUBECTLFLAGS wait pods \
        --timeout=2m \
        --selector "rollouts-pod-template-hash=$spec_hash" \
        --for=condition=Ready
}


until is_paused; do
    sleep 2
done;

wait_for_pods_ready
```

Pull request description:
feat: Add RolloutCompleted condition
Closes #352 

Patch:
diff --git a/pkg/apis/rollouts/v1alpha1/types.go b/pkg/apis/rollouts/v1alpha1/types.go
index 9ab780962..f24e522fc 100644
--- a/pkg/apis/rollouts/v1alpha1/types.go
+++ b/pkg/apis/rollouts/v1alpha1/types.go
@@ -652,6 +652,8 @@ const (
 	RolloutReplicaFailure RolloutConditionType = "ReplicaFailure"
 	// RolloutPaused means that rollout is in a paused state. It is still progressing at this point.
 	RolloutPaused RolloutConditionType = "Paused"
+	// RolloutCompleted means that rollout is in a completed state. It is still progressing at this point.
+	RolloutCompleted RolloutConditionType = "Completed"
 )
 
 // RolloutCondition describes the state of a rollout at a certain point.
diff --git a/rollout/sync.go b/rollout/sync.go
index 0d6768e30..13fea3149 100644
--- a/rollout/sync.go
+++ b/rollout/sync.go
@@ -563,7 +563,20 @@ func isIndefiniteStep(r *v1alpha1.Rollout) bool {
 }
 
 func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutStatus) v1alpha1.RolloutStatus {
-	if len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused {
+	isPaused := len(c.rollout.Status.PauseConditions) > 0 || c.rollout.Spec.Paused
+
+	completeCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutCompleted)
+	if !isPaused && conditions.RolloutComplete(c.rollout, &newStatus) {
+		updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionTrue, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason)
+		conditions.SetRolloutCondition(&newStatus, *updateCompletedCond)
+	} else {
+		if completeCond != nil {
+			updateCompletedCond := conditions.NewRolloutCondition(v1alpha1.RolloutCompleted, corev1.ConditionFalse, conditions.RolloutCompletedReason, conditions.RolloutCompletedReason)
+			conditions.SetRolloutCondition(&newStatus, *updateCompletedCond)
+		}
+	}
+
+	if isPaused {
 		return newStatus
 	}
 
@@ -571,6 +584,7 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt
 	// a new rollout and this is a resync where we don't need to estimate any progress.
 	// In such a case, we should simply not estimate any progress for this rollout.
 	currentCond := conditions.GetRolloutCondition(c.rollout.Status, v1alpha1.RolloutProgressing)
+
 	isCompleteRollout := newStatus.Replicas == newStatus.AvailableReplicas && currentCond != nil && currentCond.Reason == conditions.NewRSAvailableReason
 	// Check for progress only if the latest rollout hasn't completed yet.
 	if !isCompleteRollout {
@@ -590,8 +604,8 @@ func (c *rolloutContext) calculateRolloutConditions(newStatus v1alpha1.RolloutSt
 			if c.newRS != nil {
 				msg = fmt.Sprintf(conditions.ReplicaSetCompletedMessage, c.newRS.Name)
 			}
-			condition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg)
-			conditions.SetRolloutCondition(&newStatus, *condition)
+			progressingCondition := conditions.NewRolloutCondition(v1alpha1.RolloutProgressing, corev1.ConditionTrue, conditions.NewRSAvailableReason, msg)
+			conditions.SetRolloutCondition(&newStatus, *progressingCondition)
 		case conditions.RolloutProgressing(c.rollout, &newStatus):
 			// If there is any progress made, continue by not checking if the rollout failed. This
 			// behavior emulates the rolling updater progressDeadline check.
diff --git a/utils/conditions/conditions.go b/utils/conditions/conditions.go
index 871ab9f86..4ac20c9e5 100644
--- a/utils/conditions/conditions.go
+++ b/utils/conditions/conditions.go
@@ -87,6 +87,8 @@ const (
 	// within the given deadline (progressDeadlineSeconds).
 	ReplicaSetTimeOutMessage = "ReplicaSet %q has timed out progressing."
 
+	// RolloutCompletedReason is added in a rollout when it is completed.
+	RolloutCompletedReason = "RolloutCompleted"
 	// RolloutCompletedMessage is added when the rollout is completed
 	RolloutCompletedMessage = "Rollout %q has successfully progressed."
 	// ReplicaSetCompletedMessage is added when the rollout is completed


Test patch:
diff --git a/rollout/analysis_test.go b/rollout/analysis_test.go
index a09c89df1..ca5d49ff5 100644
--- a/rollout/analysis_test.go
+++ b/rollout/analysis_test.go
@@ -1628,7 +1628,7 @@ func TestDoNotCreatePrePromotionAnalysisAfterPromotionRollout(t *testing.T) {
 
 	f.run(getKey(r2, t))
 
-	newConditions := generateConditionsPatch(true, conditions.NewRSAvailableReason, rs2, true, "")
+	newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true)
 	expectedPatch := fmt.Sprintf(`{
 		"status":{
 			"conditions":%s
diff --git a/rollout/bluegreen_test.go b/rollout/bluegreen_test.go
index 894a95155..7357506e0 100644
--- a/rollout/bluegreen_test.go
+++ b/rollout/bluegreen_test.go
@@ -1,6 +1,7 @@
 package rollout
 
 import (
+	"encoding/json"
 	"fmt"
 	"strconv"
 	"testing"
@@ -1012,7 +1013,7 @@ func TestBlueGreenRolloutCompleted(t *testing.T) {
 
 	f.run(getKey(r2, t))
 
-	newConditions := generateConditionsPatch(true, conditions.NewRSAvailableReason, rs2, true, "")
+	newConditions := generateConditionsPatchWithComplete(true, conditions.NewRSAvailableReason, rs2, true, "", true)
 	expectedPatch := fmt.Sprintf(`{
 		"status":{
 			"conditions":%s
@@ -1022,6 +1023,49 @@ func TestBlueGreenRolloutCompleted(t *testing.T) {
 	assert.Equal(t, cleanPatch(expectedPatch), patch)
 }
 
+func TestBlueGreenRolloutCompletedFalse(t *testing.T) {
+	f := newFixture(t)
+	defer f.Close()
+
+	r1 := newBlueGreenRollout("foo", 1, nil, "bar", "")
+	completedCondition, _ := newCompletedCondition(true)
+	conditions.SetRolloutCondition(&r1.Status, completedCondition)
+
+	r2 := bumpVersion(r1)
+	rs2 := newReplicaSetWithStatus(r2, 1, 1)
+	progressingCondition, _ := newProgressingCondition(conditions.PausedRolloutReason, rs2, "")
+	conditions.SetRolloutCondition(&r2.Status, progressingCondition)
+	pausedCondition, _ := newPausedCondition(true)
+	conditions.SetRolloutCondition(&r2.Status, pausedCondition)
+
+	f.kubeobjects = append(f.kubeobjects, rs2)
+	f.replicaSetLister = append(f.replicaSetLister, rs2)
+	rs2PodHash := rs2.Labels[v1alpha1.DefaultRolloutUniqueLabelKey]
+
+	serviceSelector := map[string]string{v1alpha1.DefaultRolloutUniqueLabelKey: rs2PodHash}
+	s := newService("bar", 80, serviceSelector, r2)
+	f.kubeobjects = append(f.kubeobjects, s)
+
+	r2 = updateBlueGreenRolloutStatus(r2, "", rs2PodHash, rs2PodHash, 1, 1, 1, 1, true, false)
+	r2.Status.ObservedGeneration = strconv.Itoa(int(r2.Generation))
+
+	f.rolloutLister = append(f.rolloutLister, r2)
+	f.objects = append(f.objects, r2)
+	f.serviceLister = append(f.serviceLister, s)
+
+	patchIndex := f.expectPatchRolloutAction(r1)
+	f.run(getKey(r2, t))
+
+	patch := f.getPatchedRollout(patchIndex)
+	rolloutPatch := v1alpha1.Rollout{}
+	err := json.Unmarshal([]byte(patch), &rolloutPatch)
+	assert.NoError(t, err)
+
+	index := len(rolloutPatch.Status.Conditions) - 1
+	assert.Equal(t, v1alpha1.RolloutCompleted, rolloutPatch.Status.Conditions[index].Type)
+	assert.Equal(t, corev1.ConditionFalse, rolloutPatch.Status.Conditions[index].Status)
+}
+
 func TestBlueGreenUnableToReadScaleDownAt(t *testing.T) {
 	f := newFixture(t)
 	defer f.Close()
diff --git a/rollout/controller_test.go b/rollout/controller_test.go
index c3c7157db..315e6b391 100644
--- a/rollout/controller_test.go
+++ b/rollout/controller_test.go
@@ -185,6 +185,26 @@ func newPausedCondition(isPaused bool) (v1alpha1.RolloutCondition, string) {
 	return condition, string(conditionBytes)
 }
 
+func newCompletedCondition(isCompleted bool) (v1alpha1.RolloutCondition, string) {
+	status := corev1.ConditionTrue
+	if !isCompleted {
+		status = corev1.ConditionFalse
+	}
+	condition := v1alpha1.RolloutCondition{
+		LastTransitionTime: metav1.Now(),
+		LastUpdateTime:     metav1.Now(),
+		Message:            conditions.RolloutCompletedReason,
+		Reason:             conditions.RolloutCompletedReason,
+		Status:             status,
+		Type:               v1alpha1.RolloutCompleted,
+	}
+	conditionBytes, err := json.Marshal(condition)
+	if err != nil {
+		panic(err)
+	}
+	return condition, string(conditionBytes)
+}
+
 func newProgressingCondition(reason string, resourceObj runtime.Object, optionalMessage string) (v1alpha1.RolloutCondition, string) {
 	status := corev1.ConditionTrue
 	msg := ""
@@ -307,6 +327,16 @@ func generateConditionsPatchWithPause(available bool, progressingReason string,
 	return fmt.Sprintf("[%s, %s, %s]", progressingCondition, pauseCondition, availableCondition)
 }
 
+func generateConditionsPatchWithComplete(available bool, progressingReason string, progressingResource runtime.Object, availableConditionFirst bool, progressingMessage string, isCompleted bool) string {
+	_, availableCondition := newAvailableCondition(available)
+	_, progressingCondition := newProgressingCondition(progressingReason, progressingResource, progressingMessage)
+	_, completeCondition := newCompletedCondition(isCompleted)
+	if availableConditionFirst {
+		return fmt.Sprintf("[%s, %s, %s]", availableCondition, completeCondition, progressingCondition)
+	}
+	return fmt.Sprintf("[%s, %s, %s]", completeCondition, progressingCondition, availableCondition)
+}
+
 // func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active string, availableReplicas, updatedReplicas, hpaReplicas int32, pause bool, available bool, progressingStatus string) *v1alpha1.Rollout {
 func updateBlueGreenRolloutStatus(r *v1alpha1.Rollout, preview, active, stable string, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas int32, pause bool, available bool) *v1alpha1.Rollout {
 	newRollout := updateBaseRolloutStatus(r, availableReplicas, updatedReplicas, totalReplicas, hpaReplicas)
diff --git a/test/e2e/functional_test.go b/test/e2e/functional_test.go
index 4bab71b3d..81ded0cbb 100644
--- a/test/e2e/functional_test.go
+++ b/test/e2e/functional_test.go
@@ -904,3 +904,64 @@ spec:
 		}).
 		ExpectActiveRevision("2")
 }
+
+func (s *FunctionalSuite) TestKubectlWaitForCompleted() {
+	s.Given().
+		RolloutObjects(`
+kind: Service
+apiVersion: v1
+metadata:
+  name: rollout-bluegreen-active
+spec:
+  selector:
+    app: rollout-bluegreen
+  ports:
+  - protocol: TCP
+    port: 80
+    targetPort: 8080
+---
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  name: rollout-bluegreen
+spec:
+  replicas: 1
+  revisionHistoryLimit: 2
+  selector:
+    matchLabels:
+      app: rollout-bluegreen
+  template:
+    metadata:
+      labels:
+        app: rollout-bluegreen
+    spec:
+      containers:
+      - name: rollouts-demo
+        image: argoproj/rollouts-demo:blue
+        imagePullPolicy: Always
+        ports:
+        - containerPort: 8080
+  strategy:
+    blueGreen:
+      activeService: rollout-bluegreen-active
+      autoPromotionEnabled: true
+`).
+		When().
+		ApplyManifests().
+		WaitForRolloutReplicas(1).
+		WaitForRolloutStatus("Healthy").
+		UpdateSpec().
+		Then().
+		ExpectRollout("Completed", func(r *v1alpha1.Rollout) bool {
+			cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=False", fmt.Sprintf("rollout/%s", r.Name))
+			return cmd.Run() == nil
+		}).
+		When().
+		PromoteRollout().
+		Then().
+		ExpectRollout("Completed", func(r *v1alpha1.Rollout) bool {
+			cmd := exec.Command("kubectl", "wait", "--for=condition=Completed=True", fmt.Sprintf("rollout/%s", r.Name))
+			return cmd.Run() == nil
+		}).
+		ExpectActiveRevision("2")
+}