From 03dafacc63e0de8f7e52a577cc93fc2ea795bcb8 Mon Sep 17 00:00:00 2001 From: Nicholas Yancey Date: Fri, 24 Jul 2026 10:18:50 -0400 Subject: [PATCH] added service probe --- controllers/cli_download_controller.go | 43 ++++- controllers/cli_download_controller_test.go | 174 ++++++++++++++++++++ 2 files changed, 211 insertions(+), 6 deletions(-) diff --git a/controllers/cli_download_controller.go b/controllers/cli_download_controller.go index 897dfaabc0..203d5dc1d2 100644 --- a/controllers/cli_download_controller.go +++ b/controllers/cli_download_controller.go @@ -167,11 +167,18 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe } else { desired := buildCLIServerDeployment(c.Namespace, cliServerImage) needsUpdate := false - if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe = desired.Spec.Template.Spec.Containers[0].ReadinessProbe - deployment.Spec.Template.Spec.Containers[0].LivenessProbe = desired.Spec.Template.Spec.Containers[0].LivenessProbe - needsUpdate = true + if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-cli-server"); idx != -1 { + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] + if currentContainer.ReadinessProbe == nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe + needsUpdate = true + } } if deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName { deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName @@ -187,7 +194,7 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe if err := c.Client.Update(ctx, deployment); err != nil { return fmt.Errorf("failed to update CLI server deployment: %w", err) } - c.Log.Info("Updated CLI server deployment with readiness/liveness probes and/or service account") + c.Log.Info("Updated CLI server deployment with probes and/or service account") } } @@ -389,6 +396,17 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { Protocol: corev1.ProtocolTCP, }, }, + StartupProbe: &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{ + Path: "/", + Port: intstr.FromString("http"), + }, + }, + InitialDelaySeconds: 5, + PeriodSeconds: 5, + FailureThreshold: 12, + }, ReadinessProbe: &corev1.Probe{ ProbeHandler: corev1.ProbeHandler{ HTTPGet: &corev1.HTTPGetAction{ @@ -506,3 +524,16 @@ func buildCLIServerRoute(namespace string) *routev1.Route { func int64Ptr(i int64) *int64 { return &i } + +// findContainerIndexByName returns the index of the container with the given +// name, or -1 if no such container exists. Used to avoid assuming the target +// server container is always at index 0, which may not hold if a sidecar is +// injected or the container order changes. +func findContainerIndexByName(containers []corev1.Container, name string) int { + for i := range containers { + if containers[i].Name == name { + return i + } + } + return -1 +} diff --git a/controllers/cli_download_controller_test.go b/controllers/cli_download_controller_test.go index 92b50df504..45fb355451 100644 --- a/controllers/cli_download_controller_test.go +++ b/controllers/cli_download_controller_test.go @@ -11,6 +11,7 @@ import ( corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/intstr" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" @@ -90,3 +91,176 @@ func TestReconcileCLIResources_ReconcilesExistingServiceAccount(t *testing.T) { t.Error("expected an owner reference to be added") } } + +func TestBuildCLIServerDeployment_StartupProbe(t *testing.T) { + deployment := buildCLIServerDeployment("openshift-adp", "test-image") + + if len(deployment.Spec.Template.Spec.Containers) == 0 { + t.Fatal("expected at least one container") + } + container := deployment.Spec.Template.Spec.Containers[0] + + if container.StartupProbe == nil { + t.Fatal("expected StartupProbe to be set") + } + if container.StartupProbe.HTTPGet == nil { + t.Fatal("expected StartupProbe to use HTTPGet") + } + if container.StartupProbe.HTTPGet.Path != "/" { + t.Errorf("expected StartupProbe path \"/\", got %q", container.StartupProbe.HTTPGet.Path) + } + if container.StartupProbe.HTTPGet.Port != intstr.FromString("http") { + t.Errorf("expected StartupProbe port \"http\", got %v", container.StartupProbe.HTTPGet.Port) + } + if container.StartupProbe.FailureThreshold != 12 { + t.Errorf("expected StartupProbe failureThreshold 12, got %d", container.StartupProbe.FailureThreshold) + } + if container.StartupProbe.InitialDelaySeconds != 5 { + t.Errorf("expected StartupProbe initialDelaySeconds 5, got %d", container.StartupProbe.InitialDelaySeconds) + } + if container.StartupProbe.PeriodSeconds != 5 { + t.Errorf("expected StartupProbe periodSeconds 5, got %d", container.StartupProbe.PeriodSeconds) + } + + if container.ReadinessProbe == nil { + t.Fatal("expected ReadinessProbe to remain set") + } + if container.LivenessProbe == nil { + t.Fatal("expected LivenessProbe to remain set") + } +} + +func TestReconcileCLIResources_BackfillsMissingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getCLIDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + // Simulate a Deployment created by a pre-fix version of the operator: it has + // the server container, but no probes configured at all. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + Containers: []corev1.Container{ + {Name: "oadp-cli-server", Image: "old-image"}, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestCLIRoute(namespace)). + Build() + + setup := &CLIDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileCLIResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileCLIResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: cliServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.ReadinessProbe == nil { + t.Error("expected ReadinessProbe to be backfilled") + } + if container.LivenessProbe == nil { + t.Error("expected LivenessProbe to be backfilled") + } + if container.StartupProbe == nil { + t.Error("expected StartupProbe to be backfilled") + } + // The image on an existing deployment should not be clobbered by the backfill. + if container.Image != "old-image" { + t.Errorf("expected existing image to be preserved, got %q", container.Image) + } +} + +func TestReconcileCLIResources_DoesNotOverwriteExistingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getCLIDownloadTestScheme(t) + + operatorDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: "oadp-operator", Namespace: namespace}, + } + + customProbe := &corev1.Probe{ + ProbeHandler: corev1.ProbeHandler{ + HTTPGet: &corev1.HTTPGetAction{Path: "/custom", Port: intstr.FromString("http")}, + }, + InitialDelaySeconds: 99, + PeriodSeconds: 99, + } + + // Simulate a Deployment that already has probes configured (e.g. from a + // prior reconcile, or customized by a user) to ensure the backfill logic + // doesn't clobber them. + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{Name: cliServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + ServiceAccountName: cliServerServiceAccountName, + Containers: []corev1.Container{ + { + Name: "oadp-cli-server", + Image: "old-image", + ReadinessProbe: customProbe.DeepCopy(), + LivenessProbe: customProbe.DeepCopy(), + StartupProbe: customProbe.DeepCopy(), + }, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestCLIRoute(namespace)). + Build() + + setup := &CLIDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileCLIResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileCLIResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: cliServerDeploymentName, Namespace: namespace}, updated); err != nil { + t.Fatalf("failed to get updated deployment: %v", err) + } + container := updated.Spec.Template.Spec.Containers[0] + + if container.ReadinessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing ReadinessProbe to be preserved, got InitialDelaySeconds=%d", container.ReadinessProbe.InitialDelaySeconds) + } + if container.LivenessProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing LivenessProbe to be preserved, got InitialDelaySeconds=%d", container.LivenessProbe.InitialDelaySeconds) + } + if container.StartupProbe.InitialDelaySeconds != 99 { + t.Errorf("expected existing StartupProbe to be preserved, got InitialDelaySeconds=%d", container.StartupProbe.InitialDelaySeconds) + } +}