From 1a9b49219dd360ab9f5dae84ddfec2ea5de134f9 Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Tue, 4 Apr 2023 17:03:07 -0400 Subject: [PATCH 1/3] Fix TestClientTLS: Wait for old router pods to be cleaned up before testing new mTLS config --- test/e2e/client_tls_test.go | 4 ++-- test/e2e/operator_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 8b85b4264c..96306be1ea 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -349,8 +349,8 @@ func TestClientTLS(t *testing.T) { if err := waitForDeploymentEnvVar(t, kclient, deployment, 1*time.Minute, "ROUTER_MUTUAL_TLS_AUTH", "required"); err != nil { t.Fatalf("expected updated deployment to have ROUTER_MUTUAL_TLS_AUTH=required: %v", err) } - if err := waitForDeploymentComplete(t, kclient, deployment, 3*time.Minute); err != nil { - t.Fatalf("failed to observe expected conditions: %v", err) + if err := waitForDeploymentCompleteWithCleanup(t, kclient, deploymentName, 3*time.Minute); err != nil { + t.Fatalf("timed out waiting for old router generation to be cleaned up: %v", err) } requiredPolicyTestCases := []struct { diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 72f8937fde..994af8d88e 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -3579,6 +3579,36 @@ func waitForDeploymentEnvVar(t *testing.T, cl client.Client, deployment *appsv1. return err } +// waitForDeploymentCompleteWithCleanup waits for a deployment to complete its rollout, then waits for the old +// generation's pods to finish terminating. +func waitForDeploymentCompleteWithCleanup(t *testing.T, cl client.Client, deploymentName types.NamespacedName, timeout time.Duration) error { + t.Helper() + deployment := &appsv1.Deployment{} + if err := cl.Get(context.TODO(), deploymentName, deployment); err != nil { + return fmt.Errorf("failed to get deployment %s: %w", deploymentName.Name, err) + } + + if deployment.Generation != deployment.Status.ObservedGeneration { + if err := waitForDeploymentComplete(t, cl, deployment, timeout); err != nil { + return fmt.Errorf("timed out waiting for deployment %s to complete rollout: %w", deploymentName.Name, err) + } + } + + selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) + if err != nil { + return fmt.Errorf("deployment %s has invalid spec.selector: %w", deploymentName.Name, err) + } + + return wait.PollImmediate(2*time.Second, timeout, func() (bool, error) { + pods := &corev1.PodList{} + if err := cl.List(context.TODO(), pods, client.MatchingLabelsSelector{Selector: selector}); err != nil { + t.Logf("failed to list pods for deployment %q: %v", deploymentName.Name, err) + return false, nil + } + return len(pods.Items) == int(*deployment.Spec.Replicas), nil + }) +} + func clusterOperatorConditionMap(conditions ...configv1.ClusterOperatorStatusCondition) map[string]string { conds := map[string]string{} for _, cond := range conditions { From 20e4e387782c610b7bc73ca3b08c3f79bef98e28 Mon Sep 17 00:00:00 2001 From: Ryan Fredette Date: Wed, 19 Apr 2023 13:14:26 -0400 Subject: [PATCH 2/3] Rename waitForDeploymentCompleteWithCleanup to waitForDeploymentCompleteWithOldPodTermination Also: - Rename pods to podList - When checking for old pod termination, only count the currently ready pods, instead of all pods --- test/e2e/client_tls_test.go | 2 +- test/e2e/operator_test.go | 27 +++++++++++++++++++-------- 2 files changed, 20 insertions(+), 9 deletions(-) diff --git a/test/e2e/client_tls_test.go b/test/e2e/client_tls_test.go index 96306be1ea..12766fa586 100644 --- a/test/e2e/client_tls_test.go +++ b/test/e2e/client_tls_test.go @@ -349,7 +349,7 @@ func TestClientTLS(t *testing.T) { if err := waitForDeploymentEnvVar(t, kclient, deployment, 1*time.Minute, "ROUTER_MUTUAL_TLS_AUTH", "required"); err != nil { t.Fatalf("expected updated deployment to have ROUTER_MUTUAL_TLS_AUTH=required: %v", err) } - if err := waitForDeploymentCompleteWithCleanup(t, kclient, deploymentName, 3*time.Minute); err != nil { + if err := waitForDeploymentCompleteWithOldPodTermination(t, kclient, deploymentName, 3*time.Minute); err != nil { t.Fatalf("timed out waiting for old router generation to be cleaned up: %v", err) } diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 994af8d88e..751a5bce4d 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -3581,17 +3581,15 @@ func waitForDeploymentEnvVar(t *testing.T, cl client.Client, deployment *appsv1. // waitForDeploymentCompleteWithCleanup waits for a deployment to complete its rollout, then waits for the old // generation's pods to finish terminating. -func waitForDeploymentCompleteWithCleanup(t *testing.T, cl client.Client, deploymentName types.NamespacedName, timeout time.Duration) error { +func waitForDeploymentCompleteWithOldPodTermination(t *testing.T, cl client.Client, deploymentName types.NamespacedName, timeout time.Duration) error { t.Helper() deployment := &appsv1.Deployment{} if err := cl.Get(context.TODO(), deploymentName, deployment); err != nil { return fmt.Errorf("failed to get deployment %s: %w", deploymentName.Name, err) } - if deployment.Generation != deployment.Status.ObservedGeneration { - if err := waitForDeploymentComplete(t, cl, deployment, timeout); err != nil { - return fmt.Errorf("timed out waiting for deployment %s to complete rollout: %w", deploymentName.Name, err) - } + if err := waitForDeploymentComplete(t, cl, deployment, timeout); err != nil { + return fmt.Errorf("timed out waiting for deployment %s to complete rollout: %w", deploymentName.Name, err) } selector, err := metav1.LabelSelectorAsSelector(deployment.Spec.Selector) @@ -3599,13 +3597,26 @@ func waitForDeploymentCompleteWithCleanup(t *testing.T, cl client.Client, deploy return fmt.Errorf("deployment %s has invalid spec.selector: %w", deploymentName.Name, err) } + expectedReplicas := 1 + if deployment.Spec.Replicas != nil && int(*deployment.Spec.Replicas) != 0 { + expectedReplicas = int(*deployment.Spec.Replicas) + } + return wait.PollImmediate(2*time.Second, timeout, func() (bool, error) { - pods := &corev1.PodList{} - if err := cl.List(context.TODO(), pods, client.MatchingLabelsSelector{Selector: selector}); err != nil { + podList := &corev1.PodList{} + if err := cl.List(context.TODO(), podList, client.MatchingLabelsSelector{Selector: selector}); err != nil { t.Logf("failed to list pods for deployment %q: %v", deploymentName.Name, err) return false, nil } - return len(pods.Items) == int(*deployment.Spec.Replicas), nil + readyPods := 0 + for _, pod := range podList.Items { + for _, condition := range pod.Status.Conditions { + if condition.Type == corev1.PodReady && condition.Status == corev1.ConditionTrue { + readyPods++ + } + } + } + return readyPods == expectedReplicas, nil }) } From 5cd2a01c1144e680e8d8c1c67862ea622ea844be Mon Sep 17 00:00:00 2001 From: Miciah Masters Date: Wed, 26 Apr 2023 11:33:12 -0400 Subject: [PATCH 3/3] waitForDeploymentCompleteWithOldPodTermination fix Follow-up to commit 20e4e387782c610b7bc73ca3b08c3f79bef98e28. * test/e2e/operator_test.go (waitForDeploymentCompleteWithOldPodTermination): Correct the function name in the godoc. Use "k8s.io/utils/pointer".Int32Deref, and respect the value in spec.replicas even if it is set explicitly to 0. --- test/e2e/operator_test.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/test/e2e/operator_test.go b/test/e2e/operator_test.go index 751a5bce4d..eec3d18f4f 100644 --- a/test/e2e/operator_test.go +++ b/test/e2e/operator_test.go @@ -53,6 +53,7 @@ import ( "k8s.io/apimachinery/pkg/types" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/utils/pointer" "k8s.io/apiserver/pkg/storage/names" @@ -3579,8 +3580,9 @@ func waitForDeploymentEnvVar(t *testing.T, cl client.Client, deployment *appsv1. return err } -// waitForDeploymentCompleteWithCleanup waits for a deployment to complete its rollout, then waits for the old -// generation's pods to finish terminating. +// waitForDeploymentCompleteWithOldPodTermination waits for a deployment to +// complete its rollout, then waits for the old generation's pods to finish +// terminating. func waitForDeploymentCompleteWithOldPodTermination(t *testing.T, cl client.Client, deploymentName types.NamespacedName, timeout time.Duration) error { t.Helper() deployment := &appsv1.Deployment{} @@ -3597,10 +3599,8 @@ func waitForDeploymentCompleteWithOldPodTermination(t *testing.T, cl client.Clie return fmt.Errorf("deployment %s has invalid spec.selector: %w", deploymentName.Name, err) } - expectedReplicas := 1 - if deployment.Spec.Replicas != nil && int(*deployment.Spec.Replicas) != 0 { - expectedReplicas = int(*deployment.Spec.Replicas) - } + // If spec.replicas is null, the default value is 1, per the API spec. + expectedReplicas := int(pointer.Int32Deref(deployment.Spec.Replicas, 1)) return wait.PollImmediate(2*time.Second, timeout, func() (bool, error) { podList := &corev1.PodList{}