From 9d0ad43fb60e1f14fe818b4fd1c7f92cbb844045 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Filip=20K=C5=99epinsk=C3=BD?= Date: Tue, 22 Oct 2024 21:06:57 +0200 Subject: [PATCH] Fix Progressing and Degraded condition during workload rollouts. MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Degraded condtion should not become False during the deployment rollout (DeploymentProgressing condition. There can be unavailable pods during the rollout. This is capped by a 15 minute deadline. Progressing condition should be True until all of the old pods have terminated as they can still accept connections. Co-authored-by: Standa Láznička --- .../apiserver/controller/workload/workload.go | 53 +++- .../controller/workload/workload_test.go | 239 +++++++++++++++++- 2 files changed, 287 insertions(+), 5 deletions(-) diff --git a/pkg/operator/apiserver/controller/workload/workload.go b/pkg/operator/apiserver/controller/workload/workload.go index 5607995030..7063f73158 100644 --- a/pkg/operator/apiserver/controller/workload/workload.go +++ b/pkg/operator/apiserver/controller/workload/workload.go @@ -287,9 +287,26 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o desiredReplicas = *(workload.Spec.Replicas) } + selector, err := metav1.LabelSelectorAsSelector(workload.Spec.Selector) + if err != nil { + return fmt.Errorf("failed to construct label selector: %v", err) + } + matchingPods, err := c.podsLister.Pods(c.targetNamespace).List(selector) + if err != nil { + return err + } + // Terminating pods don't account for any of the other status fields but + // still can exist in a state when they are accepting connections and would + // contribute to unexpected behavior if we report Progressing=False. + // The case of too many pods might occur for example if `TerminationGracePeriodSeconds` is set. + terminatingPods := countTerminatingPods(matchingPods) + // If the workload is up to date, then we are no longer progressing workloadAtHighestGeneration := workload.ObjectMeta.Generation == workload.Status.ObservedGeneration - workloadIsBeingUpdated := workload.Status.UpdatedReplicas < desiredReplicas + // update is done when all pods have been updated to the latest revision + // and the deployment controller has reported NewReplicaSetAvailable + workloadIsBeingUpdated := !hasDeploymentProgressed(workload.Status) + workloadHasOldTerminatingPods := workload.Status.AvailableReplicas == desiredReplicas && terminatingPods > 0 workloadIsBeingUpdatedTooLong, err := isUpdatingTooLong(previousStatus, *deploymentProgressingCondition.Type) if !workloadAtHighestGeneration { deploymentProgressingCondition = deploymentProgressingCondition. @@ -300,7 +317,12 @@ func (c *Controller) updateOperatorStatus(ctx context.Context, previousStatus *o deploymentProgressingCondition = deploymentProgressingCondition. WithStatus(operatorv1.ConditionTrue). WithReason("PodsUpdating"). - WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas)) + WithMessage(fmt.Sprintf("deployment/%s.%s: %d/%d pods have been updated to the latest generation and %d/%d pods are available", workload.Name, c.targetNamespace, workload.Status.UpdatedReplicas, desiredReplicas, workload.Status.AvailableReplicas, desiredReplicas)) + } else if workloadHasOldTerminatingPods { + deploymentProgressingCondition = deploymentProgressingCondition. + WithStatus(operatorv1.ConditionTrue). + WithReason("PreviousGenerationPodsPresent"). + WithMessage(fmt.Sprintf("deployment/%s.%s: %d pod(s) from the previous generation are still present", workload.Name, c.targetNamespace, terminatingPods)) } else { deploymentProgressingCondition = deploymentProgressingCondition. WithStatus(operatorv1.ConditionFalse). @@ -353,6 +375,33 @@ func isUpdatingTooLong(operatorStatus *operatorv1.OperatorStatus, progressingCon return progressing != nil && progressing.Status == operatorv1.ConditionTrue && time.Now().After(progressing.LastTransitionTime.Add(15*time.Minute)), nil } +// hasDeploymentProgressed returns true if the deployment reports NewReplicaSetAvailable +// via the DeploymentProgressing condition +func hasDeploymentProgressed(status appsv1.DeploymentStatus) bool { + for i := range status.Conditions { + c := status.Conditions[i] + if c.Type == appsv1.DeploymentProgressing { + return c.Status == corev1.ConditionTrue && c.Reason == "NewReplicaSetAvailable" + } + } + return false +} + +func countTerminatingPods(pods []*corev1.Pod) int32 { + numberOfTerminatingPods := 0 + for _, p := range pods { + if isPodTerminating(p) { + numberOfTerminatingPods += 1 + } + } + return int32(numberOfTerminatingPods) +} + +func isPodTerminating(p *corev1.Pod) bool { + return p.Status.Phase != corev1.PodFailed && p.Status.Phase != corev1.PodSucceeded && + p.DeletionTimestamp != nil +} + // EnsureAtMostOnePodPerNode updates the deployment spec to prevent more than // one pod of a given replicaset from landing on a node. It accomplishes this // by adding a label on the template and updates the pod anti-affinity term to include that label. diff --git a/pkg/operator/apiserver/controller/workload/workload_test.go b/pkg/operator/apiserver/controller/workload/workload_test.go index a518940743..06b9bafaf3 100644 --- a/pkg/operator/apiserver/controller/workload/workload_test.go +++ b/pkg/operator/apiserver/controller/workload/workload_test.go @@ -140,6 +140,9 @@ func TestUpdateOperatorStatus(t *testing.T) { }, Status: appsv1.DeploymentStatus{ AvailableReplicas: 0, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionFalse, LastUpdateTime: metav1.NewTime(time.Now().Add(-6 * time.Minute)), LastTransitionTime: metav1.NewTime(time.Now().Add(-6 * time.Minute)), Reason: "ProgressDeadlineExceeded", Message: "timed out"}, + }, }, }, pods: []*corev1.Pod{ @@ -192,7 +195,7 @@ func TestUpdateOperatorStatus(t *testing.T) { Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing), Status: operatorv1.ConditionTrue, Reason: "PodsUpdating", - Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation", + Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available", }, } return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) @@ -211,6 +214,9 @@ func TestUpdateOperatorStatus(t *testing.T) { }, Status: appsv1.DeploymentStatus{ AvailableReplicas: 0, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"}, + }, }, }, pods: []*corev1.Pod{ @@ -262,7 +268,7 @@ func TestUpdateOperatorStatus(t *testing.T) { Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing), Status: operatorv1.ConditionTrue, Reason: "PodsUpdating", - Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation", + Message: "deployment/apiserver.openshift-apiserver: 0/3 pods have been updated to the latest generation and 0/3 pods are available", }, } return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) @@ -282,6 +288,9 @@ func TestUpdateOperatorStatus(t *testing.T) { Status: appsv1.DeploymentStatus{ AvailableReplicas: 2, UpdatedReplicas: 3, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"}, + }, }, }, pods: []*corev1.Pod{ @@ -345,6 +354,9 @@ func TestUpdateOperatorStatus(t *testing.T) { Status: appsv1.DeploymentStatus{ AvailableReplicas: 3, UpdatedReplicas: 3, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"}, + }, }, }, validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error { @@ -389,6 +401,9 @@ func TestUpdateOperatorStatus(t *testing.T) { Status: appsv1.DeploymentStatus{ AvailableReplicas: 3, ObservedGeneration: 99, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"}, + }, }, }, validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error { @@ -469,6 +484,9 @@ func TestUpdateOperatorStatus(t *testing.T) { AvailableReplicas: 2, UpdatedReplicas: 1, ObservedGeneration: 2, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"}, + }, }, }, validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error { @@ -493,7 +511,7 @@ func TestUpdateOperatorStatus(t *testing.T) { Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing), Status: operatorv1.ConditionTrue, Reason: "PodsUpdating", - Message: "deployment/apiserver.openshift-apiserver: 1/3 pods have been updated to the latest generation", + Message: "deployment/apiserver.openshift-apiserver: 1/3 pods have been updated to the latest generation and 2/3 pods are available", }, } return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) @@ -512,6 +530,9 @@ func TestUpdateOperatorStatus(t *testing.T) { Status: appsv1.DeploymentStatus{ AvailableReplicas: 3, UpdatedReplicas: 3, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"}, + }, }, }, previousConditions: []operatorv1.OperatorCondition{ @@ -547,6 +568,218 @@ func TestUpdateOperatorStatus(t *testing.T) { return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) }, }, + { + name: "all pods rolled out but there is an old terminating pod", + workload: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver", + Namespace: "openshift-apiserver", + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}}, + Replicas: ptr.To[int32](2), + }, + Status: appsv1.DeploymentStatus{ + AvailableReplicas: 2, + ReadyReplicas: 2, + UpdatedReplicas: 2, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "NewReplicaSetAvailable", Message: "has successfully progressed"}, + }, + }, + }, + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-1-1", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + DeletionTimestamp: ptr.To(metav1.Now()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-2-1", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-2-2", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + }, + validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error { + expectedConditions := []operatorv1.OperatorCondition{ + { + Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeAvailable), + Status: operatorv1.ConditionTrue, + Reason: "AsExpected", + Message: "", + }, + { + Type: fmt.Sprintf("%sWorkloadDegraded", defaultControllerName), + Status: operatorv1.ConditionFalse, + }, + { + Type: fmt.Sprintf("%sDeploymentDegraded", defaultControllerName), + Status: operatorv1.ConditionFalse, + Reason: "AsExpected", + Message: "", + }, + { + Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing), + Status: operatorv1.ConditionTrue, + Reason: "PreviousGenerationPodsPresent", + Message: "deployment/apiserver.openshift-apiserver: 1 pod(s) from the previous generation are still present", + }, + } + return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) + }, + }, + { + name: "some pods rolled out and waiting for old terminating pod before we can progress further", + workload: &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver", + Namespace: "openshift-apiserver", + }, + Spec: appsv1.DeploymentSpec{ + Selector: &metav1.LabelSelector{MatchLabels: map[string]string{"foo": "bar"}}, + Template: corev1.PodTemplateSpec{ObjectMeta: metav1.ObjectMeta{Labels: map[string]string{"foo": "bar"}}}, + Replicas: ptr.To[int32](3), + }, + Status: appsv1.DeploymentStatus{ + AvailableReplicas: 2, + ReadyReplicas: 2, + UpdatedReplicas: 2, + Conditions: []appsv1.DeploymentCondition{ + {Type: appsv1.DeploymentProgressing, Status: corev1.ConditionTrue, LastUpdateTime: metav1.Now(), LastTransitionTime: metav1.Now(), Reason: "ReplicaSetUpdated", Message: "progressing"}, + }, + }, + }, + pods: []*corev1.Pod{ + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-1-1", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + DeletionTimestamp: ptr.To(metav1.Now()), + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-2-1", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + { + ObjectMeta: metav1.ObjectMeta{ + Name: "apiserver-2-2", Namespace: "openshift-apiserver", + Labels: map[string]string{"foo": "bar"}, + }, + Status: corev1.PodStatus{ + Phase: corev1.PodRunning, + ContainerStatuses: []corev1.ContainerStatus{ + { + Name: "test", + Ready: true, + State: corev1.ContainerState{ + Running: &corev1.ContainerStateRunning{}, + }, + }, + }, + }, + }, + }, + validateOperatorStatus: func(actualStatus *operatorv1.OperatorStatus) error { + expectedConditions := []operatorv1.OperatorCondition{ + { + Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeAvailable), + Status: operatorv1.ConditionTrue, + Reason: "AsExpected", + Message: "", + }, + { + Type: fmt.Sprintf("%sWorkloadDegraded", defaultControllerName), + Status: operatorv1.ConditionFalse, + }, + { + Type: fmt.Sprintf("%sDeploymentDegraded", defaultControllerName), + Status: operatorv1.ConditionFalse, + Reason: "AsExpected", + Message: "", + }, + { + Type: fmt.Sprintf("%sDeployment%s", defaultControllerName, operatorv1.OperatorStatusTypeProgressing), + Status: operatorv1.ConditionTrue, + Reason: "PodsUpdating", + Message: "deployment/apiserver.openshift-apiserver: 2/3 pods have been updated to the latest generation and 2/3 pods are available", + }, + } + return areCondidtionsEqual(expectedConditions, actualStatus.Conditions) + }, + }, } for _, scenario := range scenarios {