From ed6108c9295991c6446c5987577b978366b4784c Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 17 Sep 2026 09:22:25 +0000 Subject: [PATCH 1/3] USHIFT-7500: recover metrics serving cert Requeue service-ca while the optional metrics-server serving Secret is missing or incomplete after a restart storm. Add focused unit and Robot regression coverage. --- pkg/components/metrics.go | 65 ++++++++++++++++++++++++- pkg/components/metrics_test.go | 78 ++++++++++++++++++++++++++++++ test/suites/optional/metrics.robot | 37 ++++++++++++-- 3 files changed, 174 insertions(+), 6 deletions(-) create mode 100644 pkg/components/metrics_test.go diff --git a/pkg/components/metrics.go b/pkg/components/metrics.go index 01d85b6dbd..61ea7cf031 100644 --- a/pkg/components/metrics.go +++ b/pkg/components/metrics.go @@ -17,13 +17,22 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/wait" + "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "k8s.io/utils/clock" ) const ( - metricsServerManifestPath = "/usr/lib/microshift/manifests.d/080-microshift-metrics-server" - metricsNamespace = "openshift-monitoring" + metricsServerManifestPath = "/usr/lib/microshift/manifests.d/080-microshift-metrics-server" + metricsNamespace = "openshift-monitoring" + metricsServerServiceName = "metrics-server" + metricsServerTLSResourceName = "metrics-server-tls" + + // metricsServerServingCertRecoveryAnnotation changes whenever the serving + // certificate needs recovery. The service-ca controller watches Service + // updates, so this requeues certificate generation while the expected Secret + // is absent or incomplete. + metricsServerServingCertRecoveryAnnotation = "microshift.openshift.io/service-ca-reconcile-at" ) var metricsServerEventRecorder events.Recorder = events.NewLoggingEventRecorder("microshift-metrics-server", clock.RealClock{}) @@ -50,6 +59,55 @@ func waitForNamespace(ctx context.Context, clientset kubernetes.Interface, names }) } +func metricsServerServingCertReady(secret *corev1.Secret) bool { + return secret != nil && len(secret.Data[corev1.TLSCertKey]) > 0 && len(secret.Data[corev1.TLSPrivateKeyKey]) > 0 +} + +func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientset kubernetes.Interface) error { + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + service, err := clientset.CoreV1().Services(metricsNamespace).Get(ctx, metricsServerServiceName, metav1.GetOptions{}) + if err != nil { + return err + } + + annotations := service.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[metricsServerServingCertRecoveryAnnotation] = time.Now().UTC().Format(time.RFC3339Nano) + service.SetAnnotations(annotations) + + _, err = clientset.CoreV1().Services(metricsNamespace).Update(ctx, service, metav1.UpdateOptions{}) + return err + }) +} + +// waitForMetricsServerServingCert waits for service-ca to restore the serving +// certificate used by metrics-server. While the Service remains present and +// the Secret is absent or incomplete, force a Service update to make service-ca +// re-evaluate its serving-cert annotation. +func waitForMetricsServerServingCert(ctx context.Context, clientset kubernetes.Interface) error { + return wait.PollUntilContextTimeout(ctx, 2*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { + secret, err := clientset.CoreV1().Secrets(metricsNamespace).Get(ctx, metricsServerTLSResourceName, metav1.GetOptions{}) + if err == nil && metricsServerServingCertReady(secret) { + return true, nil + } + if err != nil && !apierrors.IsNotFound(err) { + klog.Errorf("getting metrics-server serving cert secret: %v", err) + } + + if err := triggerMetricsServerServingCertReconciliation(ctx, clientset); err != nil { + if !apierrors.IsNotFound(err) { + klog.Errorf("triggering metrics-server serving cert reconciliation: %v", err) + } + return false, nil + } + + klog.V(2).Info("Waiting for service-ca to restore the metrics-server serving certificate") + return false, nil + }) +} + // ProvisionMetricsServerCerts provisions the TLS client certificate and kubelet // serving CA that metrics-server needs to authenticate to kubelet and verify its // serving certificate when scraping /metrics/resource. These are provisioned at @@ -75,6 +133,9 @@ func ProvisionMetricsServerCerts(ctx context.Context, cfg *config.Config) error if err := waitForNamespace(ctx, clientset, metricsNamespace); err != nil { return fmt.Errorf("waiting for namespace %s: %w", metricsNamespace, err) } + if err := waitForMetricsServerServingCert(ctx, clientset); err != nil { + return fmt.Errorf("waiting for metrics-server serving cert: %w", err) + } certsDir := cryptomaterial.CertsDirectory(config.DataDir) diff --git a/pkg/components/metrics_test.go b/pkg/components/metrics_test.go new file mode 100644 index 0000000000..14f94d424b --- /dev/null +++ b/pkg/components/metrics_test.go @@ -0,0 +1,78 @@ +package components + +import ( + "context" + "testing" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/client-go/kubernetes/fake" +) + +func TestMetricsServerServingCertReady(t *testing.T) { + tests := []struct { + name string + secret *corev1.Secret + ready bool + }{ + { + name: "missing secret", + ready: false, + }, + { + name: "empty secret data", + secret: &corev1.Secret{}, + ready: false, + }, + { + name: "missing private key", + secret: &corev1.Secret{Data: map[string][]byte{ + corev1.TLSCertKey: []byte("certificate"), + }}, + ready: false, + }, + { + name: "serving certificate and private key", + secret: &corev1.Secret{Data: map[string][]byte{ + corev1.TLSCertKey: []byte("certificate"), + corev1.TLSPrivateKeyKey: []byte("private-key"), + }}, + ready: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := metricsServerServingCertReady(tt.secret); got != tt.ready { + t.Errorf("metricsServerServingCertReady() = %t, want %t", got, tt.ready) + } + }) + } +} + +func TestTriggerMetricsServerServingCertReconciliation(t *testing.T) { + clientset := fake.NewSimpleClientset(&corev1.Service{ + ObjectMeta: metav1.ObjectMeta{ + Name: metricsServerServiceName, + Namespace: metricsNamespace, + Annotations: map[string]string{ + "service.beta.openshift.io/serving-cert-secret-name": metricsServerTLSResourceName, + }, + }, + }) + + if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset); err != nil { + t.Fatalf("triggering service-ca reconciliation: %v", err) + } + + service, err := clientset.CoreV1().Services(metricsNamespace).Get(context.Background(), metricsServerServiceName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting service: %v", err) + } + if got := service.Annotations[metricsServerServingCertRecoveryAnnotation]; got == "" { + t.Errorf("recovery annotation %q was not set", metricsServerServingCertRecoveryAnnotation) + } + if got := service.Annotations["service.beta.openshift.io/serving-cert-secret-name"]; got != metricsServerTLSResourceName { + t.Errorf("serving cert annotation = %q, want %q", got, metricsServerTLSResourceName) + } +} diff --git a/test/suites/optional/metrics.robot b/test/suites/optional/metrics.robot index e172c13508..92d66e40ef 100644 --- a/test/suites/optional/metrics.robot +++ b/test/suites/optional/metrics.robot @@ -7,16 +7,18 @@ Resource ../../resources/kubeconfig.resource Resource ../../resources/microshift-host.resource Resource ../../resources/oc.resource Resource ../../resources/optional-config.resource +Resource ../../resources/microshift-process.resource Suite Setup Setup Suite Teardown Teardown *** Variables *** -${METRICS_NS} openshift-monitoring -${CLIENT_CERT} /tmp/metrics-test-client.crt -${CLIENT_KEY} /tmp/metrics-test-client.key -${SERVICE_CA} /tmp/metrics-test-service-ca.crt +${METRICS_NS} openshift-monitoring +${CLIENT_CERT} /tmp/metrics-test-client.crt +${CLIENT_KEY} /tmp/metrics-test-client.key +${SERVICE_CA} /tmp/metrics-test-service-ca.crt +${RESTART_ATTEMPTS} 3 *** Test Cases *** @@ -56,6 +58,20 @@ Metrics Server Reports Node Metrics ${out}= Run With Kubeconfig oc adm top nodes --no-headers Should Match Regexp ${out} \\d+m +Metrics Server Recovers After Restart Storm + [Documentation] Service-ca must restore the metrics-server serving Secret after + ... MicroShift is interrupted repeatedly during startup. + FOR ${attempt} IN RANGE ${RESTART_ATTEMPTS} + Stop MicroShift + Start MicroShift Without Waiting For Systemd Readiness + Sleep 1s + Restart MicroShift + END + Wait Until Keyword Succeeds 5m 5s + ... Metrics Server Serving Certificate Secret Should Be Available + Named Deployment Should Be Available metrics-server ns=${METRICS_NS} + Metrics Server API Should Be Available + *** Keywords *** Setup @@ -90,6 +106,19 @@ Cleanup Metrics Client Certs [Documentation] Remove temporary client cert files from the remote host. Command Should Work rm -f ${CLIENT_CERT} ${CLIENT_KEY} ${SERVICE_CA} +Metrics Server Serving Certificate Secret Should Be Available + [Documentation] Verify service-ca has restored the TLS data mounted by metrics-server. + ${certificate}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.crt}' + Should Not Be Empty ${certificate} + ${private_key}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.key}' + Should Not Be Empty ${private_key} + +Metrics Server API Should Be Available + [Documentation] Wait until the aggregated metrics API can route to metrics-server. + Oc Wait apiservice v1beta1.metrics.k8s.io --for=condition=Available --timeout\\=120s + Metrics Endpoint Should Contain [Documentation] Scrape kube-state-metrics on the given port and assert the ... response contains the expected metric name. From e016f85a3d2714321b591ee0e35310a98abefea4 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 17 Sep 2026 09:41:44 +0000 Subject: [PATCH 2/3] USHIFT-7500: bound metrics serving cert recovery Wait for service-ca readiness, clear its retry-ceiling annotations in one conflict-safe Service update, and wait read-only for the serving Secret. Add deterministic recovery coverage without issuing serving certificates outside service-ca. --- pkg/components/metrics.go | 111 ++++++++++++++++++--- pkg/components/metrics_test.go | 155 ++++++++++++++++++++++++++--- test/suites/optional/metrics.robot | 55 +++++++++- 3 files changed, 289 insertions(+), 32 deletions(-) diff --git a/pkg/components/metrics.go b/pkg/components/metrics.go index 61ea7cf031..c0f711d02f 100644 --- a/pkg/components/metrics.go +++ b/pkg/components/metrics.go @@ -6,6 +6,7 @@ import ( "os" "time" + appsv1 "k8s.io/api/apps/v1" "k8s.io/client-go/kubernetes" "github.com/openshift/library-go/pkg/operator/events" @@ -27,14 +28,40 @@ const ( metricsNamespace = "openshift-monitoring" metricsServerServiceName = "metrics-server" metricsServerTLSResourceName = "metrics-server-tls" + serviceCANamespace = "openshift-service-ca" + serviceCADeploymentName = "service-ca" // metricsServerServingCertRecoveryAnnotation changes whenever the serving // certificate needs recovery. The service-ca controller watches Service // updates, so this requeues certificate generation while the expected Secret // is absent or incomplete. metricsServerServingCertRecoveryAnnotation = "microshift.openshift.io/service-ca-reconcile-at" + + // The pinned service-ca controller stops trying after ten failures until these + // annotations are cleared. Keep these names in sync with + // service-ca-operator/pkg/controller/api. + servingCertGenerationErrorAnnotation = "service.beta.openshift.io/serving-cert-generation-error" + servingCertGenerationErrorNumAnnotation = "service.beta.openshift.io/serving-cert-generation-error-num" + alphaServingCertGenerationErrorAnnotation = "service.alpha.openshift.io/serving-cert-generation-error" + alphaServingCertGenerationErrorNumAnnotation = "service.alpha.openshift.io/serving-cert-generation-error-num" ) +type metricsServerServingCertWaitOptions struct { + timeout time.Duration + pollInterval time.Duration + controllerRetryBackoff wait.Backoff + serviceRetryBackoff wait.Backoff +} + +var defaultMetricsServerServingCertWaitOptions = metricsServerServingCertWaitOptions{ + timeout: 5 * time.Minute, + pollInterval: 2 * time.Second, + // These discovery waits only read resources. Once service-ca is ready and + // the Service exists, exactly one reconciliation update is issued. + controllerRetryBackoff: wait.Backoff{Duration: time.Second, Factor: 2, Steps: 8, Cap: 30 * time.Second}, + serviceRetryBackoff: wait.Backoff{Duration: time.Second, Factor: 2, Steps: 8, Cap: 30 * time.Second}, +} + var metricsServerEventRecorder events.Recorder = events.NewLoggingEventRecorder("microshift-metrics-server", clock.RealClock{}) var metricsClientCARecorder events.Recorder = events.NewLoggingEventRecorder("metrics-client-ca", clock.RealClock{}) @@ -63,6 +90,44 @@ func metricsServerServingCertReady(secret *corev1.Secret) bool { return secret != nil && len(secret.Data[corev1.TLSCertKey]) > 0 && len(secret.Data[corev1.TLSPrivateKeyKey]) > 0 } +func serviceCAControllerReady(deployment *appsv1.Deployment) bool { + if deployment == nil { + return false + } + for _, condition := range deployment.Status.Conditions { + if condition.Type == appsv1.DeploymentAvailable && condition.Status == corev1.ConditionTrue { + return true + } + } + return false +} + +func waitForServiceCAController(ctx context.Context, clientset kubernetes.Interface, backoff wait.Backoff) error { + return wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) { + deployment, err := clientset.AppsV1().Deployments(serviceCANamespace).Get(ctx, serviceCADeploymentName, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("getting service-ca controller: %w", err) + } + return serviceCAControllerReady(deployment), nil + }) +} + +func waitForMetricsServerService(ctx context.Context, clientset kubernetes.Interface, backoff wait.Backoff) error { + return wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) { + _, err := clientset.CoreV1().Services(metricsNamespace).Get(ctx, metricsServerServiceName, metav1.GetOptions{}) + if apierrors.IsNotFound(err) { + return false, nil + } + if err != nil { + return false, fmt.Errorf("getting metrics-server Service: %w", err) + } + return true, nil + }) +} + func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientset kubernetes.Interface) error { return retry.RetryOnConflict(retry.DefaultRetry, func() error { service, err := clientset.CoreV1().Services(metricsNamespace).Get(ctx, metricsServerServiceName, metav1.GetOptions{}) @@ -75,6 +140,10 @@ func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientse annotations = map[string]string{} } annotations[metricsServerServingCertRecoveryAnnotation] = time.Now().UTC().Format(time.RFC3339Nano) + delete(annotations, servingCertGenerationErrorAnnotation) + delete(annotations, servingCertGenerationErrorNumAnnotation) + delete(annotations, alphaServingCertGenerationErrorAnnotation) + delete(annotations, alphaServingCertGenerationErrorNumAnnotation) service.SetAnnotations(annotations) _, err = clientset.CoreV1().Services(metricsNamespace).Update(ctx, service, metav1.UpdateOptions{}) @@ -83,24 +152,42 @@ func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientse } // waitForMetricsServerServingCert waits for service-ca to restore the serving -// certificate used by metrics-server. While the Service remains present and -// the Secret is absent or incomplete, force a Service update to make service-ca -// re-evaluate its serving-cert annotation. +// certificate used by metrics-server. It waits for the controller and Service +// with bounded exponential backoff, then makes one Service update that both +// clears service-ca's retry ceiling and requeues certificate generation. func waitForMetricsServerServingCert(ctx context.Context, clientset kubernetes.Interface) error { - return wait.PollUntilContextTimeout(ctx, 2*time.Second, 5*time.Minute, true, func(ctx context.Context) (bool, error) { + return waitForMetricsServerServingCertWithOptions(ctx, clientset, defaultMetricsServerServingCertWaitOptions) +} + +func waitForMetricsServerServingCertWithOptions(ctx context.Context, clientset kubernetes.Interface, options metricsServerServingCertWaitOptions) error { + secret, err := clientset.CoreV1().Secrets(metricsNamespace).Get(ctx, metricsServerTLSResourceName, metav1.GetOptions{}) + if err == nil && metricsServerServingCertReady(secret) { + return nil + } + if err != nil && !apierrors.IsNotFound(err) { + return fmt.Errorf("getting metrics-server serving cert secret: %w", err) + } + + waitCtx, cancel := context.WithTimeout(ctx, options.timeout) + defer cancel() + + if err := waitForServiceCAController(waitCtx, clientset, options.controllerRetryBackoff); err != nil { + return fmt.Errorf("waiting for service-ca controller: %w", err) + } + if err := waitForMetricsServerService(waitCtx, clientset, options.serviceRetryBackoff); err != nil { + return fmt.Errorf("waiting for metrics-server Service: %w", err) + } + if err := triggerMetricsServerServingCertReconciliation(waitCtx, clientset); err != nil { + return fmt.Errorf("triggering metrics-server serving cert reconciliation: %w", err) + } + + return wait.PollUntilContextTimeout(waitCtx, options.pollInterval, options.timeout, true, func(ctx context.Context) (bool, error) { secret, err := clientset.CoreV1().Secrets(metricsNamespace).Get(ctx, metricsServerTLSResourceName, metav1.GetOptions{}) if err == nil && metricsServerServingCertReady(secret) { return true, nil } if err != nil && !apierrors.IsNotFound(err) { - klog.Errorf("getting metrics-server serving cert secret: %v", err) - } - - if err := triggerMetricsServerServingCertReconciliation(ctx, clientset); err != nil { - if !apierrors.IsNotFound(err) { - klog.Errorf("triggering metrics-server serving cert reconciliation: %v", err) - } - return false, nil + return false, fmt.Errorf("getting metrics-server serving cert secret: %w", err) } klog.V(2).Info("Waiting for service-ca to restore the metrics-server serving certificate") diff --git a/pkg/components/metrics_test.go b/pkg/components/metrics_test.go index 14f94d424b..d1f20448eb 100644 --- a/pkg/components/metrics_test.go +++ b/pkg/components/metrics_test.go @@ -2,11 +2,20 @@ package components import ( "context" + "errors" + "sync/atomic" "testing" + "time" + 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/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes/fake" + k8stesting "k8s.io/client-go/testing" ) func TestMetricsServerServingCertReady(t *testing.T) { @@ -50,29 +59,145 @@ func TestMetricsServerServingCertReady(t *testing.T) { } } -func TestTriggerMetricsServerServingCertReconciliation(t *testing.T) { - clientset := fake.NewSimpleClientset(&corev1.Service{ - ObjectMeta: metav1.ObjectMeta{ - Name: metricsServerServiceName, - Namespace: metricsNamespace, - Annotations: map[string]string{ - "service.beta.openshift.io/serving-cert-secret-name": metricsServerTLSResourceName, +func TestWaitForMetricsServerServingCertRecoversMissingSecret(t *testing.T) { + service := newMetricsServerService() + service.UID = "metrics-server-service-uid" + service.Annotations[servingCertGenerationErrorAnnotation] = "service-ca was unavailable" + service.Annotations[servingCertGenerationErrorNumAnnotation] = "10" + service.Annotations[alphaServingCertGenerationErrorAnnotation] = "service-ca was unavailable" + service.Annotations[alphaServingCertGenerationErrorNumAnnotation] = "10" + clientset := fake.NewSimpleClientset(newServiceCADeployment(), service) + + var serviceUpdates atomic.Int32 + clientset.PrependReactor("update", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + serviceUpdates.Add(1) + updatedService := action.(k8stesting.UpdateAction).GetObject().(*corev1.Service) + if err := clientset.Tracker().Update(corev1.SchemeGroupVersion.WithResource("services"), updatedService, metricsNamespace); err != nil { + return true, nil, err + } + secret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: metricsServerTLSResourceName, + Namespace: metricsNamespace, + Annotations: map[string]string{ + "service.beta.openshift.io/service-name": metricsServerServiceName, + "service.beta.openshift.io/service-serving-cert-secret-name": metricsServerTLSResourceName, + }, }, - }, + Type: corev1.SecretTypeTLS, + Data: map[string][]byte{ + corev1.TLSCertKey: []byte("service-ca-certificate"), + corev1.TLSPrivateKeyKey: []byte("service-ca-private-key"), + }, + } + if err := clientset.Tracker().Create(corev1.SchemeGroupVersion.WithResource("secrets"), secret, metricsNamespace); err != nil { + return true, nil, err + } + return true, updatedService, nil }) - if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset); err != nil { - t.Fatalf("triggering service-ca reconciliation: %v", err) + if err := waitForMetricsServerServingCertWithOptions(context.Background(), clientset, testMetricsServerServingCertWaitOptions()); err != nil { + t.Fatalf("waiting for recovered serving cert: %v", err) + } + if got := serviceUpdates.Load(); got != 1 { + t.Errorf("service updates = %d, want 1", got) } - service, err := clientset.CoreV1().Services(metricsNamespace).Get(context.Background(), metricsServerServiceName, metav1.GetOptions{}) + secret, err := clientset.CoreV1().Secrets(metricsNamespace).Get(context.Background(), metricsServerTLSResourceName, metav1.GetOptions{}) if err != nil { - t.Fatalf("getting service: %v", err) + t.Fatalf("getting recreated serving cert: %v", err) + } + if !metricsServerServingCertReady(secret) { + t.Error("recreated serving cert is not ready") } - if got := service.Annotations[metricsServerServingCertRecoveryAnnotation]; got == "" { - t.Errorf("recovery annotation %q was not set", metricsServerServingCertRecoveryAnnotation) + + updatedService, err := clientset.CoreV1().Services(metricsNamespace).Get(context.Background(), metricsServerServiceName, metav1.GetOptions{}) + if err != nil { + t.Fatalf("getting updated Service: %v", err) } - if got := service.Annotations["service.beta.openshift.io/serving-cert-secret-name"]; got != metricsServerTLSResourceName { + for _, annotation := range []string{ + servingCertGenerationErrorAnnotation, + servingCertGenerationErrorNumAnnotation, + alphaServingCertGenerationErrorAnnotation, + alphaServingCertGenerationErrorNumAnnotation, + } { + if got := updatedService.Annotations[annotation]; got != "" { + t.Errorf("Service annotation %q = %q, want cleared", annotation, got) + } + } + if got := updatedService.Annotations["service.beta.openshift.io/serving-cert-secret-name"]; got != metricsServerTLSResourceName { t.Errorf("serving cert annotation = %q, want %q", got, metricsServerTLSResourceName) } } + +func TestWaitForMetricsServerServingCertMissingService(t *testing.T) { + clientset := fake.NewSimpleClientset(newServiceCADeployment()) + + err := waitForMetricsServerServingCertWithOptions(context.Background(), clientset, testMetricsServerServingCertWaitOptions()) + if !errors.Is(err, wait.ErrWaitTimeout) { + t.Fatalf("waiting for missing Service = %v, want wait timeout", err) + } + if got := countServiceUpdates(clientset.Actions()); got != 0 { + t.Errorf("service updates = %d, want 0", got) + } +} + +func TestTriggerMetricsServerServingCertReconciliationRetriesConflict(t *testing.T) { + clientset := fake.NewSimpleClientset(newMetricsServerService()) + var updates atomic.Int32 + clientset.PrependReactor("update", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + if updates.Add(1) == 1 { + return true, nil, apierrors.NewConflict(schema.GroupResource{Resource: "services"}, metricsServerServiceName, errors.New("conflict")) + } + return false, nil, nil + }) + + if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset); err != nil { + t.Fatalf("triggering service-ca reconciliation: %v", err) + } + if got := updates.Load(); got != 2 { + t.Errorf("service updates = %d, want 2 after one conflict", got) + } +} + +func testMetricsServerServingCertWaitOptions() metricsServerServingCertWaitOptions { + return metricsServerServingCertWaitOptions{ + timeout: time.Second, + pollInterval: time.Millisecond, + controllerRetryBackoff: wait.Backoff{Steps: 1}, + serviceRetryBackoff: wait.Backoff{Steps: 1}, + } +} + +func newServiceCADeployment() *appsv1.Deployment { + return &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: serviceCADeploymentName, + Namespace: serviceCANamespace, + }, + Status: appsv1.DeploymentStatus{Conditions: []appsv1.DeploymentCondition{{ + Type: appsv1.DeploymentAvailable, + Status: corev1.ConditionTrue, + }}}, + } +} + +func newMetricsServerService() *corev1.Service { + return &corev1.Service{ObjectMeta: metav1.ObjectMeta{ + Name: metricsServerServiceName, + Namespace: metricsNamespace, + Annotations: map[string]string{ + "service.beta.openshift.io/serving-cert-secret-name": metricsServerTLSResourceName, + }, + }} +} + +func countServiceUpdates(actions []k8stesting.Action) int { + updates := 0 + for _, action := range actions { + if action.GetVerb() == "update" && action.GetResource().Resource == "services" { + updates++ + } + } + return updates +} diff --git a/test/suites/optional/metrics.robot b/test/suites/optional/metrics.robot index 92d66e40ef..30dea43231 100644 --- a/test/suites/optional/metrics.robot +++ b/test/suites/optional/metrics.robot @@ -59,8 +59,10 @@ Metrics Server Reports Node Metrics Should Match Regexp ${out} \\d+m Metrics Server Recovers After Restart Storm - [Documentation] Service-ca must restore the metrics-server serving Secret after - ... MicroShift is interrupted repeatedly during startup. + [Documentation] Service-ca must recreate the deleted metrics-server serving Secret + ... after MicroShift is interrupted repeatedly during startup. The retry ceiling + ... makes the pre-fix failure deterministic without creating a certificate here. + ${old_uid}= Simulate Metrics Server Serving Certificate Retry Ceiling FOR ${attempt} IN RANGE ${RESTART_ATTEMPTS} Stop MicroShift Start MicroShift Without Waiting For Systemd Readiness @@ -68,9 +70,10 @@ Metrics Server Recovers After Restart Storm Restart MicroShift END Wait Until Keyword Succeeds 5m 5s - ... Metrics Server Serving Certificate Secret Should Be Available + ... Metrics Server Serving Certificate Secret Should Be Recreated ${old_uid} Named Deployment Should Be Available metrics-server ns=${METRICS_NS} Metrics Server API Should Be Available + [Teardown] Clear Metrics Server Serving Certificate Failure State *** Keywords *** @@ -106,14 +109,56 @@ Cleanup Metrics Client Certs [Documentation] Remove temporary client cert files from the remote host. Command Should Work rm -f ${CLIENT_CERT} ${CLIENT_KEY} ${SERVICE_CA} -Metrics Server Serving Certificate Secret Should Be Available - [Documentation] Verify service-ca has restored the TLS data mounted by metrics-server. +Simulate Metrics Server Serving Certificate Retry Ceiling + [Documentation] Delete the service-ca-managed Secret after setting the controller's + ... documented retry ceiling. The test never writes certificate data itself. + ${old_uid}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.metadata.uid}' + Should Not Be Empty ${old_uid} + Run With Kubeconfig + ... oc annotate service metrics-server -n ${METRICS_NS} --overwrite service.beta.openshift.io/serving-cert-generation-error-num=10 + Run With Kubeconfig + ... oc annotate service metrics-server -n ${METRICS_NS} --overwrite service.alpha.openshift.io/serving-cert-generation-error-num=10 + ${retry_count}= Run With Kubeconfig + ... oc get service metrics-server -n ${METRICS_NS} -o jsonpath\\='{.metadata.annotations.service\\.beta\\.openshift\\.io/serving-cert-generation-error-num}' + Should Be Equal As Integers ${retry_count} 10 + Run With Kubeconfig oc delete secret metrics-server-tls -n ${METRICS_NS} + Metrics Server Serving Certificate Secret Should Be Absent + RETURN ${old_uid} + +Metrics Server Serving Certificate Secret Should Be Absent + [Documentation] Prove the service-ca serving Secret was deleted before recovery. + ${output} ${rc}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} + ... allow_fail=${TRUE} + ... return_rc=${TRUE} + Should Not Be Equal As Integers ${rc} 0 + +Metrics Server Serving Certificate Secret Should Be Recreated + [Documentation] Verify service-ca recreated a new TLS Secret with its ownership annotation. + [Arguments] ${old_uid} + ${new_uid}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.metadata.uid}' + Should Not Be Empty ${new_uid} + Should Not Be Equal ${new_uid} ${old_uid} ${certificate}= Run With Kubeconfig ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.crt}' Should Not Be Empty ${certificate} ${private_key}= Run With Kubeconfig ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.key}' Should Not Be Empty ${private_key} + ${owner}= Run With Kubeconfig + ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.metadata.annotations.service\\.beta\\.openshift\\.io/service-name}' + Should Be Equal ${owner} metrics-server + +Clear Metrics Server Serving Certificate Failure State + [Documentation] Leave the service-ca retry bookkeeping clear if the test fails. + Run With Kubeconfig + ... oc annotate service metrics-server -n ${METRICS_NS} service.beta.openshift.io/serving-cert-generation-error- service.beta.openshift.io/serving-cert-generation-error-num- + ... allow_fail=${TRUE} + Run With Kubeconfig + ... oc annotate service metrics-server -n ${METRICS_NS} service.alpha.openshift.io/serving-cert-generation-error- service.alpha.openshift.io/serving-cert-generation-error-num- + ... allow_fail=${TRUE} Metrics Server API Should Be Available [Documentation] Wait until the aggregated metrics API can route to metrics-server. From c9586f13f3e4243f8f43064a9e0355000856d6b7 Mon Sep 17 00:00:00 2001 From: Chai Bot Date: Thu, 17 Sep 2026 12:19:07 +0000 Subject: [PATCH 3/3] USHIFT-7500: retry transient metrics API errors --- pkg/components/metrics.go | 43 ++++++-- pkg/components/metrics_test.go | 164 ++++++++++++++++++++++++++++- test/suites/optional/metrics.robot | 10 +- 3 files changed, 203 insertions(+), 14 deletions(-) diff --git a/pkg/components/metrics.go b/pkg/components/metrics.go index c0f711d02f..6a2df5cc8b 100644 --- a/pkg/components/metrics.go +++ b/pkg/components/metrics.go @@ -17,8 +17,8 @@ import ( corev1 "k8s.io/api/core/v1" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + utilnet "k8s.io/apimachinery/pkg/util/net" "k8s.io/apimachinery/pkg/util/wait" - "k8s.io/client-go/util/retry" "k8s.io/klog/v2" "k8s.io/utils/clock" ) @@ -57,7 +57,7 @@ var defaultMetricsServerServingCertWaitOptions = metricsServerServingCertWaitOpt timeout: 5 * time.Minute, pollInterval: 2 * time.Second, // These discovery waits only read resources. Once service-ca is ready and - // the Service exists, exactly one reconciliation update is issued. + // the Service exists, reconciliation retries are bounded by this backoff. controllerRetryBackoff: wait.Backoff{Duration: time.Second, Factor: 2, Steps: 8, Cap: 30 * time.Second}, serviceRetryBackoff: wait.Backoff{Duration: time.Second, Factor: 2, Steps: 8, Cap: 30 * time.Second}, } @@ -102,12 +102,29 @@ func serviceCAControllerReady(deployment *appsv1.Deployment) bool { return false } +func isTransientKubernetesAPIError(err error) bool { + return apierrors.IsInternalError(err) || + apierrors.IsServerTimeout(err) || + apierrors.IsServiceUnavailable(err) || + apierrors.IsTimeout(err) || + apierrors.IsTooManyRequests(err) || + apierrors.IsUnexpectedServerError(err) || + utilnet.IsTimeout(err) || + utilnet.IsConnectionRefused(err) || + utilnet.IsConnectionReset(err) || + utilnet.IsProbableEOF(err) || + utilnet.IsHTTP2ConnectionLost(err) +} + func waitForServiceCAController(ctx context.Context, clientset kubernetes.Interface, backoff wait.Backoff) error { return wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) { deployment, err := clientset.AppsV1().Deployments(serviceCANamespace).Get(ctx, serviceCADeploymentName, metav1.GetOptions{}) if apierrors.IsNotFound(err) { return false, nil } + if isTransientKubernetesAPIError(err) { + return false, nil + } if err != nil { return false, fmt.Errorf("getting service-ca controller: %w", err) } @@ -121,6 +138,9 @@ func waitForMetricsServerService(ctx context.Context, clientset kubernetes.Inter if apierrors.IsNotFound(err) { return false, nil } + if isTransientKubernetesAPIError(err) { + return false, nil + } if err != nil { return false, fmt.Errorf("getting metrics-server Service: %w", err) } @@ -128,11 +148,14 @@ func waitForMetricsServerService(ctx context.Context, clientset kubernetes.Inter }) } -func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientset kubernetes.Interface) error { - return retry.RetryOnConflict(retry.DefaultRetry, func() error { +func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientset kubernetes.Interface, backoff wait.Backoff) error { + return wait.ExponentialBackoffWithContext(ctx, backoff, func(ctx context.Context) (bool, error) { service, err := clientset.CoreV1().Services(metricsNamespace).Get(ctx, metricsServerServiceName, metav1.GetOptions{}) if err != nil { - return err + if isTransientKubernetesAPIError(err) { + return false, nil + } + return false, err } annotations := service.GetAnnotations() @@ -147,7 +170,13 @@ func triggerMetricsServerServingCertReconciliation(ctx context.Context, clientse service.SetAnnotations(annotations) _, err = clientset.CoreV1().Services(metricsNamespace).Update(ctx, service, metav1.UpdateOptions{}) - return err + if err == nil { + return true, nil + } + if apierrors.IsConflict(err) || isTransientKubernetesAPIError(err) { + return false, nil + } + return false, err }) } @@ -177,7 +206,7 @@ func waitForMetricsServerServingCertWithOptions(ctx context.Context, clientset k if err := waitForMetricsServerService(waitCtx, clientset, options.serviceRetryBackoff); err != nil { return fmt.Errorf("waiting for metrics-server Service: %w", err) } - if err := triggerMetricsServerServingCertReconciliation(waitCtx, clientset); err != nil { + if err := triggerMetricsServerServingCertReconciliation(waitCtx, clientset, options.serviceRetryBackoff); err != nil { return fmt.Errorf("triggering metrics-server serving cert reconciliation: %w", err) } diff --git a/pkg/components/metrics_test.go b/pkg/components/metrics_test.go index d1f20448eb..dfe7c6f97f 100644 --- a/pkg/components/metrics_test.go +++ b/pkg/components/metrics_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "sync/atomic" + "syscall" "testing" "time" @@ -59,6 +60,118 @@ func TestMetricsServerServingCertReady(t *testing.T) { } } +func TestIsTransientKubernetesAPIError(t *testing.T) { + tests := []struct { + name string + err error + transient bool + }{ + { + name: "service unavailable", + err: apierrors.NewServiceUnavailable("service unavailable"), + transient: true, + }, + { + name: "connection refused", + err: syscall.ECONNREFUSED, + transient: true, + }, + { + name: "unexpected server response", + err: apierrors.NewGenericServerResponse(502, "get", schema.GroupResource{Resource: "services"}, metricsServerServiceName, "bad gateway", 0, true), + transient: true, + }, + { + name: "unauthorized", + err: apierrors.NewUnauthorized("unauthorized"), + transient: false, + }, + { + name: "forbidden", + err: apierrors.NewForbidden(schema.GroupResource{Resource: "services"}, metricsServerServiceName, errors.New("forbidden")), + transient: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := isTransientKubernetesAPIError(tt.err); got != tt.transient { + t.Errorf("isTransientKubernetesAPIError() = %t, want %t", got, tt.transient) + } + }) + } +} + +func TestWaitForServiceCAControllerRetriesTransientReadError(t *testing.T) { + clientset := fake.NewSimpleClientset(newServiceCADeployment()) + var gets atomic.Int32 + clientset.PrependReactor("get", "deployments", func(action k8stesting.Action) (bool, runtime.Object, error) { + if gets.Add(1) == 1 { + return true, nil, apierrors.NewServiceUnavailable("service unavailable") + } + return false, nil, nil + }) + + if err := waitForServiceCAController(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()); err != nil { + t.Fatalf("waiting for service-ca controller: %v", err) + } + if got := gets.Load(); got != 2 { + t.Errorf("deployment gets = %d, want 2 after one transient error", got) + } +} + +func TestWaitForServiceCAControllerReturnsUnauthorizedError(t *testing.T) { + clientset := fake.NewSimpleClientset(newServiceCADeployment()) + var gets atomic.Int32 + clientset.PrependReactor("get", "deployments", func(action k8stesting.Action) (bool, runtime.Object, error) { + gets.Add(1) + return true, nil, apierrors.NewUnauthorized("unauthorized") + }) + + err := waitForServiceCAController(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()) + if !apierrors.IsUnauthorized(err) { + t.Fatalf("waiting for service-ca controller = %v, want unauthorized error", err) + } + if got := gets.Load(); got != 1 { + t.Errorf("deployment gets = %d, want 1 for terminal error", got) + } +} + +func TestWaitForMetricsServerServiceRetriesTransientReadError(t *testing.T) { + clientset := fake.NewSimpleClientset(newMetricsServerService()) + var gets atomic.Int32 + clientset.PrependReactor("get", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + if gets.Add(1) == 1 { + return true, nil, apierrors.NewServiceUnavailable("service unavailable") + } + return false, nil, nil + }) + + if err := waitForMetricsServerService(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()); err != nil { + t.Fatalf("waiting for metrics-server Service: %v", err) + } + if got := gets.Load(); got != 2 { + t.Errorf("service gets = %d, want 2 after one transient error", got) + } +} + +func TestWaitForMetricsServerServiceReturnsForbiddenError(t *testing.T) { + clientset := fake.NewSimpleClientset(newMetricsServerService()) + var gets atomic.Int32 + clientset.PrependReactor("get", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + gets.Add(1) + return true, nil, apierrors.NewForbidden(schema.GroupResource{Resource: "services"}, metricsServerServiceName, errors.New("forbidden")) + }) + + err := waitForMetricsServerService(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()) + if !apierrors.IsForbidden(err) { + t.Fatalf("waiting for metrics-server Service = %v, want forbidden error", err) + } + if got := gets.Load(); got != 1 { + t.Errorf("service gets = %d, want 1 for terminal error", got) + } +} + func TestWaitForMetricsServerServingCertRecoversMissingSecret(t *testing.T) { service := newMetricsServerService() service.UID = "metrics-server-service-uid" @@ -152,7 +265,7 @@ func TestTriggerMetricsServerServingCertReconciliationRetriesConflict(t *testing return false, nil, nil }) - if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset); err != nil { + if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()); err != nil { t.Fatalf("triggering service-ca reconciliation: %v", err) } if got := updates.Load(); got != 2 { @@ -160,6 +273,51 @@ func TestTriggerMetricsServerServingCertReconciliationRetriesConflict(t *testing } } +func TestTriggerMetricsServerServingCertReconciliationRetriesTransientReadAndUpdateErrors(t *testing.T) { + clientset := fake.NewSimpleClientset(newMetricsServerService()) + var gets atomic.Int32 + var updates atomic.Int32 + clientset.PrependReactor("get", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + if gets.Add(1) == 1 { + return true, nil, apierrors.NewServiceUnavailable("service unavailable") + } + return false, nil, nil + }) + clientset.PrependReactor("update", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + if updates.Add(1) == 1 { + return true, nil, apierrors.NewInternalError(errors.New("internal error")) + } + return false, nil, nil + }) + + if err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()); err != nil { + t.Fatalf("triggering service-ca reconciliation: %v", err) + } + if got := gets.Load(); got != 3 { + t.Errorf("service gets = %d, want 3 after one transient get and update error", got) + } + if got := updates.Load(); got != 2 { + t.Errorf("service updates = %d, want 2 after one transient update error", got) + } +} + +func TestTriggerMetricsServerServingCertReconciliationReturnsForbiddenUpdateError(t *testing.T) { + clientset := fake.NewSimpleClientset(newMetricsServerService()) + var updates atomic.Int32 + clientset.PrependReactor("update", "services", func(action k8stesting.Action) (bool, runtime.Object, error) { + updates.Add(1) + return true, nil, apierrors.NewForbidden(schema.GroupResource{Resource: "services"}, metricsServerServiceName, errors.New("forbidden")) + }) + + err := triggerMetricsServerServingCertReconciliation(context.Background(), clientset, testMetricsServerServingCertRetryBackoff()) + if !apierrors.IsForbidden(err) { + t.Fatalf("triggering service-ca reconciliation = %v, want forbidden error", err) + } + if got := updates.Load(); got != 1 { + t.Errorf("service updates = %d, want 1 for terminal error", got) + } +} + func testMetricsServerServingCertWaitOptions() metricsServerServingCertWaitOptions { return metricsServerServingCertWaitOptions{ timeout: time.Second, @@ -169,6 +327,10 @@ func testMetricsServerServingCertWaitOptions() metricsServerServingCertWaitOptio } } +func testMetricsServerServingCertRetryBackoff() wait.Backoff { + return wait.Backoff{Steps: 3, Duration: time.Millisecond, Factor: 1} +} + func newServiceCADeployment() *appsv1.Deployment { return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ diff --git a/test/suites/optional/metrics.robot b/test/suites/optional/metrics.robot index 30dea43231..61dc096a91 100644 --- a/test/suites/optional/metrics.robot +++ b/test/suites/optional/metrics.robot @@ -141,12 +141,10 @@ Metrics Server Serving Certificate Secret Should Be Recreated ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.metadata.uid}' Should Not Be Empty ${new_uid} Should Not Be Equal ${new_uid} ${old_uid} - ${certificate}= Run With Kubeconfig - ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.crt}' - Should Not Be Empty ${certificate} - ${private_key}= Run With Kubeconfig - ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.key}' - Should Not Be Empty ${private_key} + Run With Kubeconfig + ... test "$(oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.crt}' | base64 -d | wc -c)" -gt 0 + Run With Kubeconfig + ... test "$(oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.data.tls\\.key}' | base64 -d | wc -c)" -gt 0 ${owner}= Run With Kubeconfig ... oc get secret metrics-server-tls -n ${METRICS_NS} -o jsonpath\\='{.metadata.annotations.service\\.beta\\.openshift\\.io/service-name}' Should Be Equal ${owner} metrics-server