Merge pull request #2034 from jsafrane/progressing-deployment

OCPBUGS-62634,	OCPBUGS-62624: Fix DeploymentController Progressing

Author: openshift-merge-bot[bot]

pkg/operator/deploymentcontroller/deployment_controller.go

@@ -17,6 +17,7 @@ import (
 	"github.com/openshift/library-go/pkg/operator/resource/resourceread"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
 	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/errors"
@@ -256,7 +257,6 @@ func (c *DeploymentController) syncManaged(ctx context.Context, opSpec *opv1.Ope
 	if err != nil {
 		return err
 	}
-
 	// Create an OperatorStatusApplyConfiguration with generations
 	status := applyoperatorv1.OperatorStatus().
 		WithGenerations(&applyoperatorv1.GenerationStatusApplyConfiguration{
@@ -357,6 +357,7 @@ func (c *DeploymentController) getDeployment(opSpec *opv1.OperatorSpec) (*appsv1
 }
 
 func isProgressing(deployment *appsv1.Deployment) (bool, string) {
+
 	var deploymentExpectedReplicas int32
 	if deployment.Spec.Replicas != nil {
 		deploymentExpectedReplicas = *deployment.Spec.Replicas
@@ -365,6 +366,8 @@ func isProgressing(deployment *appsv1.Deployment) (bool, string) {
 	switch {
 	case deployment.Generation != deployment.Status.ObservedGeneration:
 		return true, "Waiting for Deployment to act on changes"
+	case hasFinishedProgressing(deployment):
+		return false, ""
 	case deployment.Status.UnavailableReplicas > 0:
 		return true, "Waiting for Deployment to deploy pods"
 	case deployment.Status.UpdatedReplicas < deploymentExpectedReplicas:
@@ -374,3 +377,15 @@ func isProgressing(deployment *appsv1.Deployment) (bool, string) {
 	}
 	return false, ""
 }
+
+func hasFinishedProgressing(deployment *appsv1.Deployment) bool {
+	// Deployment whose rollout is complete gets Progressing condition with Reason NewReplicaSetAvailable condition.
+	// https://kubernetes.io/docs/concepts/workloads/controllers/deployment/#complete-deployment
+	// Any subsequent missing replicas (e.g. caused by a node reboot) must not not change the Progressing condition.
+	for _, cond := range deployment.Status.Conditions {
+		if cond.Type == appsv1.DeploymentProgressing {
+			return cond.Status == corev1.ConditionTrue && cond.Reason == "NewReplicaSetAvailable"
+		}
+	}
+	return false
+}

pkg/operator/deploymentcontroller/deployment_controller_test.go

@@ -2,34 +2,33 @@ package deploymentcontroller
 
 import (
 	"context"
-	clocktesting "k8s.io/utils/clock/testing"
 	"os"
 	"sort"
 	"time"
 
-	"github.com/google/go-cmp/cmp"
-	opv1 "github.com/openshift/api/operator/v1"
-	"github.com/openshift/library-go/pkg/operator/management"
-	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
-	appsv1 "k8s.io/api/apps/v1"
-	"k8s.io/apimachinery/pkg/api/equality"
-	"k8s.io/apimachinery/pkg/api/errors"
-	core "k8s.io/client-go/testing"
-
 	"testing"
 
+	"github.com/google/go-cmp/cmp"
 	configv1 "github.com/openshift/api/config/v1"
+	opv1 "github.com/openshift/api/operator/v1"
 	fakeconfig "github.com/openshift/client-go/config/clientset/versioned/fake"
 	configinformers "github.com/openshift/client-go/config/informers/externalversions"
 	"github.com/openshift/library-go/pkg/controller/factory"
 	"github.com/openshift/library-go/pkg/operator/events"
+	"github.com/openshift/library-go/pkg/operator/management"
+	"github.com/openshift/library-go/pkg/operator/resource/resourceapply"
 	"github.com/openshift/library-go/pkg/operator/v1helpers"
-
+	appsv1 "k8s.io/api/apps/v1"
+	corev1 "k8s.io/api/core/v1"
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
+	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	coreinformers "k8s.io/client-go/informers"
 	fakecore "k8s.io/client-go/kubernetes/fake"
+	core "k8s.io/client-go/testing"
+	clocktesting "k8s.io/utils/clock/testing"
 )
 
 const (
@@ -454,6 +453,7 @@ func sanitizeObjectMeta(meta *metav1.ObjectMeta) {
 }
 
 func TestSync(t *testing.T) {
+	const replica0 = 0
 	const replica1 = 1
 	testCases := []testCase{
 		{
@@ -535,20 +535,65 @@ func TestSync(t *testing.T) {
 			initialObjects: testObjects{
 				deployment: makeDeployment(
 					withDeploymentGeneration(1, 1),
-					withDeploymentStatus(replica1, replica1, replica1)),
+					withDeploymentStatus(replica1, replica1, replica1),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionTrue)), // Deployment is fully deployed
 				operator: makeFakeOperatorInstance(withGenerations(1)),
 			},
 			expectedObjects: testObjects{
 				deployment: makeDeployment(
 					withDeploymentGeneration(1, 1),
-					withDeploymentStatus(replica1, replica1, replica1)),
+					withDeploymentStatus(replica1, replica1, replica1),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionTrue)), // Deployment is fully deployed
 				operator: makeFakeOperatorInstance(
 					// withStatus(replica1),
 					withGenerations(1),
 					withTrueConditions(conditionAvailable),
 					withFalseConditions(conditionProgressing)),
 			},
 		},
+		{
+			// Deployment is fully deployed with a missing pod and its status is synced to CR
+			name: "pod missing after fully deployed",
+			initialObjects: testObjects{
+				deployment: makeDeployment(
+					withDeploymentGeneration(1, 1),
+					withDeploymentStatus(replica0, replica0, replica0),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionTrue)), // Deployment is fully deployed
+				operator: makeFakeOperatorInstance(withGenerations(1)),
+			},
+			expectedObjects: testObjects{
+				deployment: makeDeployment(
+					withDeploymentGeneration(1, 1),
+					withDeploymentStatus(replica0, replica0, replica0),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionTrue)), // Deployment is fully deployed
+				operator: makeFakeOperatorInstance(
+					// withStatus(replica1),
+					withGenerations(1),
+					withFalseConditions(conditionAvailable),    // No pod is running
+					withFalseConditions(conditionProgressing)), // Despite missing pod, the operator is not progressing
+			},
+		},
+		{
+			name: "pod missing before fully deployed",
+			initialObjects: testObjects{
+				deployment: makeDeployment(
+					withDeploymentGeneration(1, 1),
+					withDeploymentStatus(replica0, replica0, replica0),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionFalse)), // Deployment is not fully deployed
+				operator: makeFakeOperatorInstance(withGenerations(1)),
+			},
+			expectedObjects: testObjects{
+				deployment: makeDeployment(
+					withDeploymentGeneration(1, 1),
+					withDeploymentStatus(replica0, replica0, replica0),
+					withDeploymentConditions(appsv1.DeploymentProgressing, "NewReplicaSetAvailable", corev1.ConditionFalse)), // Deployment is not fully deployed
+				operator: makeFakeOperatorInstance(
+					// withStatus(replica1),
+					withGenerations(1),
+					withFalseConditions(conditionAvailable),   // No pod is running
+					withTrueConditions(conditionProgressing)), // A pod is missing, the operator is progressing
+			},
+		},
 		{
 			// Deployment has wrong nr. of replicas, modified by user, and gets replaced by the operator.
 			name: "deployment modified by user",

pkg/operator/deploymentcontroller/helpers_test.go

@@ -66,6 +66,17 @@ func withDeploymentGeneration(generations ...int64) deploymentModifier {
 	}
 }
 
+func withDeploymentConditions(conditionType appsv1.DeploymentConditionType, reason string, status v1.ConditionStatus) deploymentModifier {
+	return func(instance *appsv1.Deployment) *appsv1.Deployment {
+		instance.Status.Conditions = append(instance.Status.Conditions, appsv1.DeploymentCondition{
+			Type:   conditionType,
+			Status: status,
+			Reason: reason,
+		})
+		return instance
+	}
+}
+
 func TestWithReplicasHook(t *testing.T) {
 	var (
 		masterNodeLabels = map[string]string{"node-role.kubernetes.io/master": ""}