Skip to content

Commit 3148520

Browse files
rikatzrikatz
committed
OCPBUGS-53432: deflake TestIngressControllerCustomEndpoints by proper waiting for CCM to be ready
1 parent bbfcb94 commit 3148520

File tree

2 files changed

+160
-111
lines changed

2 files changed

+160
-111
lines changed

‎test/e2e/operator_test.go‎

Lines changed: 76 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -125,6 +125,7 @@ var (
125125
operandNamespace = operatorcontroller.DefaultOperandNamespace
126126
defaultName = types.NamespacedName{Namespace: operatorNamespace, Name: manifests.DefaultIngressControllerName}
127127
clusterConfigName = types.NamespacedName{Namespace: operatorNamespace, Name: manifests.ClusterIngressConfigName}
128+
ccmDeploymentName = types.NamespacedName{Namespace: "openshift-cloud-controller-manager", Name: "aws-cloud-controller-manager"}
128129

129130
// Platforms that need a DNS "warmup" period for internal (inside the test cluster) DNS resolution.
130131
// The warmup period is a period of delay before the first query is executed to avoid negative caching.
@@ -142,6 +143,13 @@ const (
142143
// cluster (which may be on different platforms) and any negative caching along the way. As of writing this, AWS
143144
// typically resolves within ~1 minute (see OCPBUGS-14966), while IBMCloud takes ~7 minutes (see OCPBUGS-48780).
144145
dnsResolutionTimeout = 10 * time.Minute
146+
147+
// ccmConfigAnnotation is an annotation that exists on CloudControllerManager
148+
// and contains a hash of the current infrastructure configuration.
149+
// It is used to signal to CCM deployment a configuration change that needs
150+
// a rollout update, and can be used by tests to also detect changes on CCM
151+
// deployment
152+
ccmConfigAnnotation = "operator.openshift.io/config-hash"
145153
)
146154

147155
func init() {
@@ -2596,110 +2604,84 @@ func TestContainerLoggingMinLength(t *testing.T) {
25962604
}
25972605

25982606
func TestIngressControllerCustomEndpoints(t *testing.T) {
2607+
if infraConfig.Status.ControlPlaneTopology == configv1.ExternalTopologyMode {
2608+
t.Skipf("skipping TestIngressControllerCustomEndpoints test due to controlplanetopology: %v", infraConfig.Status.ControlPlaneTopology)
2609+
}
25992610
platform := infraConfig.Status.PlatformStatus
26002611
if platform == nil {
26012612
t.Fatalf("platform status is missing for infrastructure %s", infraConfig.Name)
26022613
}
2603-
switch platform.Type {
2604-
case configv1.AWSPlatformType:
2605-
switch {
2606-
case platform.AWS == nil:
2607-
t.Fatalf("aws platform status is missing for infrastructure %s", infraConfig.Name)
2608-
case len(platform.AWS.ServiceEndpoints) != 0:
2609-
t.Skipf("custom endpoints detected for infrastructure %s, skipping TestIngressControllerCustomEndpoints",
2610-
infraConfig.Name)
2611-
case len(platform.AWS.Region) == 0:
2612-
t.Fatalf("region is missing from aws platform status for infrastructure %s", infraConfig.Name)
2613-
case platform.AWS.Region == endpoints.CnNorth1RegionID || platform.AWS.Region == endpoints.CnNorthwest1RegionID:
2614-
t.Skipf("region %s or %s detected for infrastructure %s, skipping TestIngressControllerCustomEndpoints",
2615-
endpoints.CnNorth1RegionID, endpoints.CnNorthwest1RegionID, infraConfig.Name)
2616-
}
2617-
route53Endpoint := configv1.AWSServiceEndpoint{
2618-
Name: "route53",
2619-
// AWS Route 53 is a non-regionalized service, so the endpoint URL
2620-
// does not include a region.
2621-
URL: "https://route53.amazonaws.com",
2622-
}
2623-
taggingEndpoint := configv1.AWSServiceEndpoint{
2624-
Name: "tagging",
2625-
// us-east-1 region is required to get hosted zone resources
2626-
// since route 53 is a non-regionalized service.
2627-
URL: "https://tagging.us-east-1.amazonaws.com",
2628-
}
2629-
elbEndpoint := configv1.AWSServiceEndpoint{
2630-
Name: "elasticloadbalancing",
2631-
URL: fmt.Sprintf("https://elasticloadbalancing.%s.amazonaws.com", platform.AWS.Region),
2632-
}
2633-
2634-
ccmDeploymentName := types.NamespacedName{Namespace: "openshift-cloud-controller-manager", Name: "aws-cloud-controller-manager"}
2635-
oldCCMDeployment, err := retrieveDeployment(ccmDeploymentName)
2636-
if err != nil {
2637-
t.Fatalf("error getting CCM deployment: %v\n", err)
2638-
}
2639-
oldConfigHash := retrieveCCMConfigurationHash(oldCCMDeployment)
2640-
if oldConfigHash == "" {
2641-
t.Fatalf("CCM does not have a config hash, this should never happen: %v\n", oldCCMDeployment)
2642-
}
26432614

2644-
if err := updateInfrastructureConfigSpecWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) {
2645-
spec.PlatformSpec.AWS = &configv1.AWSPlatformSpec{
2646-
ServiceEndpoints: []configv1.AWSServiceEndpoint{
2647-
route53Endpoint,
2648-
taggingEndpoint,
2649-
elbEndpoint,
2650-
},
2651-
}
2615+
if platform.Type != configv1.AWSPlatformType {
2616+
t.Skipf("skipping TestIngressControllerCustomEndpoints test due to platform type: %s", platform.Type)
2617+
}
2618+
2619+
switch {
2620+
case platform.AWS == nil:
2621+
t.Fatalf("aws platform status is missing for infrastructure %s", infraConfig.Name)
2622+
case len(platform.AWS.ServiceEndpoints) != 0:
2623+
t.Skipf("custom endpoints detected for infrastructure %s, skipping TestIngressControllerCustomEndpoints",
2624+
infraConfig.Name)
2625+
case len(platform.AWS.Region) == 0:
2626+
t.Fatalf("region is missing from aws platform status for infrastructure %s", infraConfig.Name)
2627+
case platform.AWS.Region == endpoints.CnNorth1RegionID || platform.AWS.Region == endpoints.CnNorthwest1RegionID:
2628+
t.Skipf("region %s or %s detected for infrastructure %s, skipping TestIngressControllerCustomEndpoints",
2629+
endpoints.CnNorth1RegionID, endpoints.CnNorthwest1RegionID, infraConfig.Name)
2630+
}
2631+
2632+
route53Endpoint := configv1.AWSServiceEndpoint{
2633+
Name: "route53",
2634+
// AWS Route 53 is a non-regionalized service, so the endpoint URL
2635+
// does not include a region.
2636+
URL: "https://route53.amazonaws.com",
2637+
}
2638+
taggingEndpoint := configv1.AWSServiceEndpoint{
2639+
Name: "tagging",
2640+
// us-east-1 region is required to get hosted zone resources
2641+
// since route 53 is a non-regionalized service.
2642+
URL: "https://tagging.us-east-1.amazonaws.com",
2643+
}
2644+
elbEndpoint := configv1.AWSServiceEndpoint{
2645+
Name: "elasticloadbalancing",
2646+
URL: fmt.Sprintf("https://elasticloadbalancing.%s.amazonaws.com", platform.AWS.Region),
2647+
}
2648+
2649+
oldCCMDeployment := waitForCCMReadiness(t, 5*time.Minute, "")
2650+
oldConfigHash := retrieveCCMConfigurationHash(oldCCMDeployment)
2651+
if oldConfigHash == "" {
2652+
t.Fatalf("CCM does not have a config hash, this should never happen: %v", oldCCMDeployment)
2653+
}
2654+
2655+
if err := updateInfrastructureConfigWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) {
2656+
spec.PlatformSpec.AWS = &configv1.AWSPlatformSpec{
2657+
ServiceEndpoints: []configv1.AWSServiceEndpoint{
2658+
route53Endpoint,
2659+
taggingEndpoint,
2660+
elbEndpoint,
2661+
},
2662+
}
2663+
}); err != nil {
2664+
t.Fatalf("failed to update infrastructure config: %v", err)
2665+
}
2666+
defer func() {
2667+
// Remove the custom endpoints from the infrastructure config.
2668+
if err := updateInfrastructureConfigWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) {
2669+
spec.PlatformSpec.AWS = nil
26522670
}); err != nil {
2653-
t.Fatalf("failed to update infrastructure config: %v\n", err)
2671+
t.Fatalf("failed to update infrastructure config: %v", err)
26542672
}
2655-
defer func() {
2656-
// Remove the custom endpoints from the infrastructure config.
2657-
if err := updateInfrastructureConfigSpecWithRetryOnConflict(t, types.NamespacedName{Name: "cluster"}, timeout, func(spec *configv1.InfrastructureSpec) {
2658-
spec.PlatformSpec.AWS = nil
2659-
}); err != nil {
2660-
t.Fatalf("failed to update infrastructure config: %v", err)
2661-
}
2662-
}()
2663-
2664-
// Wait for infrastructure status to update with custom endpoints,
2665-
// for CCM deploy to get a new configuration hash and for pods to be scheduled
2666-
// and available
2667-
// This configuration change and replicas available may take up to 5 minutes, depending on
2668-
// the environment load
2669-
if err := wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, 5*time.Minute, false, func(ctx context.Context) (done bool, err error) {
2670-
if err := kclient.Get(ctx, types.NamespacedName{Name: "cluster"}, &infraConfig); err != nil {
2671-
t.Logf("failed to get infrastructure config: %v\n", err)
2672-
return false, err
2673-
}
2674-
if len(infraConfig.Status.PlatformStatus.AWS.ServiceEndpoints) == 0 {
2675-
return false, nil
2676-
}
2673+
}()
26772674

2678-
currentCCMDeployment, err := retrieveDeployment(ccmDeploymentName)
2679-
if err != nil {
2680-
t.Logf("error getting CCM deployment: %v\n", err)
2681-
return false, err
2682-
}
2683-
currentConfigHash := retrieveCCMConfigurationHash(currentCCMDeployment)
2684-
if oldConfigHash == currentConfigHash {
2685-
t.Log("new config hash not yet applied to CCM deployment")
2686-
return false, nil
2687-
}
2675+
// Wait for infrastructure status to update with custom endpoints,
2676+
// for CCM deploy to get a new configuration hash and for pods to be scheduled
2677+
// and available
2678+
// This configuration change and replicas available may take up to 5 minutes, depending on
2679+
// the environment load
26882680

2689-
if currentCCMDeployment.Generation == currentCCMDeployment.Status.ObservedGeneration &&
2690-
currentCCMDeployment.Status.Replicas == currentCCMDeployment.Status.ReadyReplicas &&
2691-
currentCCMDeployment.Status.Replicas == currentCCMDeployment.Status.UpdatedReplicas &&
2692-
currentCCMDeployment.Status.UnavailableReplicas == 0 {
2693-
return true, nil
2694-
}
2695-
return false, nil
2681+
// Wait for CCM to be ready with the new configuration, so we can guarantee that
2682+
// the LoadBalancer will be created correctly
2683+
_ = waitForCCMReadiness(t, 5*time.Minute, oldConfigHash)
26962684

2697-
}); err != nil {
2698-
t.Fatalf("failed to observe status update for infrastructure config %s", infraConfig.Name)
2699-
}
2700-
default:
2701-
t.Skipf("skipping TestIngressControllerCustomEndpoints test due to platform type: %s", platform.Type)
2702-
}
27032685
// The default ingresscontroller should surface the expected status conditions.
27042686
if err := waitForIngressControllerCondition(t, kclient, 30*time.Second, defaultName, availableConditionsForIngressControllerWithLoadBalancer...); err != nil {
27052687
t.Fatalf("did not get expected ingress controller conditions: %v", err)

‎test/e2e/util_test.go‎

Lines changed: 84 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import (
1111
"fmt"
1212
"net"
1313
"net/http"
14+
"slices"
1415
"strings"
1516
"testing"
1617
"time"
@@ -622,49 +623,115 @@ func updateIngressConfigSpecWithRetryOnConflict(t *testing.T, name types.Namespa
622623
})
623624
}
624625

625-
// updateInfrastructureConfigSpecWithRetryOnConflict gets a fresh copy
626+
// updateInfrastructureConfigWithRetryOnConflict gets a fresh copy
626627
// of the named infrastructure object, calls updateSpecFn() where
627628
// callers can modify fields of the spec, and then updates the infrastructure
628629
// config object. If there is a conflict error on update then the
629630
// complete operation of get, mutate, and update is retried until
630631
// timeout is reached.
631-
func updateInfrastructureConfigSpecWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(*configv1.InfrastructureSpec)) error {
632+
// After updating, it will check if the desired spec.Endpoints where applied to
633+
// the infraconfig
634+
func updateInfrastructureConfigWithRetryOnConflict(t *testing.T, name types.NamespacedName, timeout time.Duration, mutateSpecFn func(*configv1.InfrastructureSpec)) error {
632635
infraConfig := configv1.Infrastructure{}
633-
return wait.PollImmediate(1*time.Second, timeout, func() (bool, error) {
634-
if err := kclient.Get(context.TODO(), name, &infraConfig); err != nil {
636+
// First we apply the configuration
637+
err := wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, timeout, false, func(ctx context.Context) (done bool, err error) {
638+
if err := kclient.Get(ctx, name, &infraConfig); err != nil {
635639
t.Logf("error getting infrastructure config %v: %v, retrying...", name, err)
636640
return false, nil
637641
}
638642
mutateSpecFn(&infraConfig.Spec)
639643
if err := kclient.Update(context.TODO(), &infraConfig); err != nil {
640-
if errors.IsConflict(err) {
641-
t.Logf("conflict when updating infrastructure config %v: %v, retrying...", name, err)
644+
t.Logf("error updating infrastructure config %v: %v, retrying...", name, err)
645+
return false, nil
646+
}
647+
return true, nil
648+
})
649+
if err != nil {
650+
return err
651+
}
652+
// Then we get the applied configuration, and check if the serviceendpoints match
653+
return wait.PollUntilContextTimeout(context.TODO(), 1*time.Second, timeout, false, func(ctx context.Context) (done bool, err error) {
654+
if err := kclient.Get(ctx, name, &infraConfig); err != nil {
655+
t.Logf("error getting infrastructure config %v: %v, retrying...", name, err)
656+
return false, nil
657+
}
658+
659+
// Early return if we don't have any specific service endpoint
660+
if infraConfig.Spec.PlatformSpec.AWS == nil || len(infraConfig.Spec.PlatformSpec.AWS.ServiceEndpoints) == 0 {
661+
return true, nil
662+
}
663+
// In case we have a desired ServiceEndpoints, we need to check once it has been reflected correctly
664+
if infraConfig.Status.PlatformStatus == nil ||
665+
infraConfig.Status.PlatformStatus.AWS == nil ||
666+
len(infraConfig.Status.PlatformStatus.AWS.ServiceEndpoints) != len(infraConfig.Spec.PlatformSpec.AWS.ServiceEndpoints) {
667+
t.Logf("infrastructure config status does not contain endpoints yet %v: %v, retrying...", name, infraConfig.Status)
668+
return false, nil
669+
}
670+
671+
for _, endpoint := range infraConfig.Spec.PlatformSpec.AWS.ServiceEndpoints {
672+
if !slices.ContainsFunc(infraConfig.Status.PlatformStatus.AWS.ServiceEndpoints, func(e configv1.AWSServiceEndpoint) bool {
673+
return e.Name == endpoint.Name && e.URL == endpoint.URL
674+
}) {
675+
t.Logf("infrastructure config status does not contain the desired endpoints, retrying %v: %v, retrying...", name, endpoint)
642676
return false, nil
643677
}
644-
return false, err
645678
}
646679
return true, nil
647680
})
648681
}
649682

650-
// retrieveDeployment is used to retrieve a full deployment resource
651-
func retrieveDeployment(name types.NamespacedName) (appsv1.Deployment, error) {
683+
// waitForCCMReadiness waits for Cloud Controller Manager to be ready.
684+
//
685+
// for infrastructure status to update with custom endpoints,
686+
//
687+
// for CCM deploy to get a new configuration hash and for pods to be scheduled
688+
// and available
689+
// This configuration change and replicas available may take up to 5 minutes, depending on
690+
// the environment load
691+
// If oldConfigurationHash is not empty it will additionally verify that the configuration hash
692+
// is different between the deployments
693+
func waitForCCMReadiness(t *testing.T, timeout time.Duration, oldConfigurationHash string) appsv1.Deployment {
694+
// The pooler below will never return an error, and instead it will log and try until it fails.
695+
// This avoids errors that are network errors to cause a failure on the test, and instead retries until it
696+
// is successful
652697
ccmDeployment := appsv1.Deployment{}
653-
err := kclient.Get(context.TODO(), name, &ccmDeployment)
654-
return ccmDeployment, err
698+
if err := wait.PollUntilContextTimeout(context.TODO(), 5*time.Second, timeout, false, func(ctx context.Context) (done bool, err error) {
699+
if err := kclient.Get(context.TODO(), ccmDeploymentName, &ccmDeployment); err != nil {
700+
t.Logf("error getting CCM deployment: %v", err)
701+
return false, nil
702+
}
703+
704+
if oldConfigurationHash != "" {
705+
currentConfigHash := retrieveCCMConfigurationHash(ccmDeployment)
706+
if oldConfigurationHash == currentConfigHash {
707+
t.Log("new config hash not yet applied to CCM deployment")
708+
return false, nil
709+
}
710+
}
711+
712+
if ccmDeployment.Generation != ccmDeployment.Status.ObservedGeneration ||
713+
ccmDeployment.Status.Replicas != ccmDeployment.Status.ReadyReplicas ||
714+
ccmDeployment.Status.Replicas != ccmDeployment.Status.UpdatedReplicas ||
715+
ccmDeployment.Status.UnavailableReplicas != 0 {
716+
t.Log("CCM replicas are not ready, will retry")
717+
return false, nil
718+
}
719+
720+
return true, nil
721+
722+
}); err != nil {
723+
t.Fatalf("failed to observe status update for infrastructure config %s", infraConfig.Name)
724+
}
725+
726+
return ccmDeployment
655727
}
656728

657729
// retrieveCCMConfigurationHash fetches the config-hash used on CCM deployment
658730
// and present on the annotation operator.openshift.io/config-hash
659731
// this information can be used to signal that a required infrastructure change
660732
// was successfully applied on CCM and forces CCM to do a rollout update.
661733
func retrieveCCMConfigurationHash(ccmDeployment appsv1.Deployment) string {
662-
ccmConfigAnnotation := "operator.openshift.io/config-hash"
663-
annotations := ccmDeployment.Spec.Template.GetAnnotations()
664-
if cfghash, ok := annotations[ccmConfigAnnotation]; ok {
665-
return cfghash
666-
}
667-
return ""
734+
return ccmDeployment.Spec.Template.Annotations[ccmConfigAnnotation]
668735
}
669736

670737
// updateInfrastructureStatus updates the Infrastructure status by applying

0 commit comments

Comments
 (0)