From 45823389f4f414915207f356b99f79dc003381d3 Mon Sep 17 00:00:00 2001 From: Nicholas Yancey Date: Wed, 22 Jul 2026 11:16:49 -0400 Subject: [PATCH 1/3] Add startup probe to CLI and VMDP download servers --- .../controller/cli_download_controller.go | 41 +++++++++++++++---- .../cli_download_controller_test.go | 39 ++++++++++++++++++ .../controller/vmdp_download_controller.go | 41 +++++++++++++++---- .../vmdp_download_controller_test.go | 39 ++++++++++++++++++ 4 files changed, 144 insertions(+), 16 deletions(-) create mode 100644 internal/controller/cli_download_controller_test.go create mode 100644 internal/controller/vmdp_download_controller_test.go diff --git a/internal/controller/cli_download_controller.go b/internal/controller/cli_download_controller.go index 8313950baa..2ad393886f 100644 --- a/internal/controller/cli_download_controller.go +++ b/internal/controller/cli_download_controller.go @@ -95,16 +95,30 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe c.Log.Info("Created CLI server deployment", "image", cliServerImage) } else if err != nil { return fmt.Errorf("failed to get CLI server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - // Deployment exists from a version before probes were added; backfill them. + } else if len(deployment.Spec.Template.Spec.Containers) > 0 { + // Deployment exists from a version before probes were added; backfill any missing ones. desired := buildCLIServerDeployment(c.Namespace, cliServerImage) - 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 - if err := c.Client.Update(ctx, deployment); err != nil { - return fmt.Errorf("failed to update CLI server deployment with probes: %w", err) + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[0] + needsUpdate := false + if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + needsUpdate = true + } + if currentContainer.LivenessProbe == nil && desiredContainer.LivenessProbe != nil { + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil && desiredContainer.StartupProbe != nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe + needsUpdate = true + } + if needsUpdate { + if err := c.Client.Update(ctx, deployment); err != nil { + return fmt.Errorf("failed to update CLI server deployment with probes: %w", err) + } + c.Log.Info("Updated CLI server deployment with probes") } - c.Log.Info("Updated CLI server deployment with readiness/liveness probes") } // 2. Create or update the service @@ -333,6 +347,17 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment { }, ReadOnlyRootFilesystem: &readOnlyRootFilesystem, }, + 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{ diff --git a/internal/controller/cli_download_controller_test.go b/internal/controller/cli_download_controller_test.go new file mode 100644 index 0000000000..a9bc230600 --- /dev/null +++ b/internal/controller/cli_download_controller_test.go @@ -0,0 +1,39 @@ +package controller + +import ( + "testing" + + "k8s.io/apimachinery/pkg/util/intstr" +) + +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.ReadinessProbe == nil { + t.Fatal("expected ReadinessProbe to remain set") + } + if container.LivenessProbe == nil { + t.Fatal("expected LivenessProbe to remain set") + } +} diff --git a/internal/controller/vmdp_download_controller.go b/internal/controller/vmdp_download_controller.go index 74c5c2302e..4b0c28ea47 100644 --- a/internal/controller/vmdp_download_controller.go +++ b/internal/controller/vmdp_download_controller.go @@ -90,16 +90,30 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator v.Log.Info("Created VMDP server deployment", "image", vmdpServerImage) } else if err != nil { return fmt.Errorf("failed to get VMDP server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 && - deployment.Spec.Template.Spec.Containers[0].ReadinessProbe == nil { - // Deployment exists from a version before probes were added; backfill them. + } else if len(deployment.Spec.Template.Spec.Containers) > 0 { + // Deployment exists from a version before probes were added; backfill any missing ones. desired := buildVMDPServerDeployment(v.Namespace, vmdpServerImage) - 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 - if err := v.Client.Update(ctx, deployment); err != nil { - return fmt.Errorf("failed to update VMDP server deployment with probes: %w", err) + desiredContainer := desired.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[0] + needsUpdate := false + if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { + currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe + needsUpdate = true + } + if currentContainer.LivenessProbe == nil && desiredContainer.LivenessProbe != nil { + currentContainer.LivenessProbe = desiredContainer.LivenessProbe + needsUpdate = true + } + if currentContainer.StartupProbe == nil && desiredContainer.StartupProbe != nil { + currentContainer.StartupProbe = desiredContainer.StartupProbe + needsUpdate = true + } + if needsUpdate { + if err := v.Client.Update(ctx, deployment); err != nil { + return fmt.Errorf("failed to update VMDP server deployment with probes: %w", err) + } + v.Log.Info("Updated VMDP server deployment with probes") } - v.Log.Info("Updated VMDP server deployment with readiness/liveness probes") } // 2. Create or update the service @@ -315,6 +329,17 @@ func buildVMDPServerDeployment(namespace, image string) *appsv1.Deployment { }, ReadOnlyRootFilesystem: &readOnlyRootFilesystem, }, + 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{ diff --git a/internal/controller/vmdp_download_controller_test.go b/internal/controller/vmdp_download_controller_test.go new file mode 100644 index 0000000000..aef4eb0e84 --- /dev/null +++ b/internal/controller/vmdp_download_controller_test.go @@ -0,0 +1,39 @@ +package controller + +import ( + "testing" + + "k8s.io/apimachinery/pkg/util/intstr" +) + +func TestBuildVMDPServerDeployment_StartupProbe(t *testing.T) { + deployment := buildVMDPServerDeployment("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.ReadinessProbe == nil { + t.Fatal("expected ReadinessProbe to remain set") + } + if container.LivenessProbe == nil { + t.Fatal("expected LivenessProbe to remain set") + } +} From 54895822b4e34c55028d6c139a044e63c0d454b7 Mon Sep 17 00:00:00 2001 From: Nicholas Yancey Date: Wed, 22 Jul 2026 13:19:37 -0400 Subject: [PATCH 2/3] Addressed CodeRabbit review comments --- .../controller/cli_download_controller.go | 17 +- .../cli_download_controller_test.go | 171 ++++++++++++++++++ .../controller/vmdp_download_controller.go | 4 +- .../vmdp_download_controller_test.go | 156 ++++++++++++++++ 4 files changed, 344 insertions(+), 4 deletions(-) diff --git a/internal/controller/cli_download_controller.go b/internal/controller/cli_download_controller.go index 2ad393886f..11ff5e6339 100644 --- a/internal/controller/cli_download_controller.go +++ b/internal/controller/cli_download_controller.go @@ -95,11 +95,11 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe c.Log.Info("Created CLI server deployment", "image", cliServerImage) } else if err != nil { return fmt.Errorf("failed to get CLI server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 { + } else if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-cli-server"); idx != -1 { // Deployment exists from a version before probes were added; backfill any missing ones. desired := buildCLIServerDeployment(c.Namespace, cliServerImage) desiredContainer := desired.Spec.Template.Spec.Containers[0] - currentContainer := &deployment.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] needsUpdate := false if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe @@ -443,3 +443,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/internal/controller/cli_download_controller_test.go b/internal/controller/cli_download_controller_test.go index a9bc230600..b59196d6c9 100644 --- a/internal/controller/cli_download_controller_test.go +++ b/internal/controller/cli_download_controller_test.go @@ -1,11 +1,43 @@ package controller import ( + "context" "testing" + "github.com/go-logr/logr" + consolev1 "github.com/openshift/api/console/v1" + routev1 "github.com/openshift/api/route/v1" + appsv1 "k8s.io/api/apps/v1" + 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" ) +// getDownloadTestScheme returns a scheme with the console/route API groups +// registered, suitable for use with a fake client in CLI/VMDP reconciliation tests. +func getDownloadTestScheme(t *testing.T) *runtime.Scheme { + t.Helper() + if err := consolev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add consolev1 to scheme: %v", err) + } + if err := routev1.AddToScheme(scheme.Scheme); err != nil { + t.Fatalf("failed to add routev1 to scheme: %v", err) + } + return scheme.Scheme +} + +// newTestCLIRoute returns a CLI server route with a hostname already assigned, +// so reconcileCLIResources doesn't hit its hostname-assignment retry/backoff path. +func newTestCLIRoute(namespace string) *routev1.Route { + route := buildCLIServerRoute(namespace) + route.Spec.Host = "cli.example.com" + return route +} + func TestBuildCLIServerDeployment_StartupProbe(t *testing.T) { deployment := buildCLIServerDeployment("openshift-adp", "test-image") @@ -29,6 +61,12 @@ func TestBuildCLIServerDeployment_StartupProbe(t *testing.T) { 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") @@ -37,3 +75,136 @@ func TestBuildCLIServerDeployment_StartupProbe(t *testing.T) { t.Fatal("expected LivenessProbe to remain set") } } + +func TestReconcileCLIResources_BackfillsMissingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(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{ + 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 := getDownloadTestScheme(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{ + 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) + } +} diff --git a/internal/controller/vmdp_download_controller.go b/internal/controller/vmdp_download_controller.go index 4b0c28ea47..0d72561a26 100644 --- a/internal/controller/vmdp_download_controller.go +++ b/internal/controller/vmdp_download_controller.go @@ -90,11 +90,11 @@ func (v *VMDPDownloadSetup) reconcileVMDPResources(ctx context.Context, operator v.Log.Info("Created VMDP server deployment", "image", vmdpServerImage) } else if err != nil { return fmt.Errorf("failed to get VMDP server deployment: %w", err) - } else if len(deployment.Spec.Template.Spec.Containers) > 0 { + } else if idx := findContainerIndexByName(deployment.Spec.Template.Spec.Containers, "oadp-vmdp-server"); idx != -1 { // Deployment exists from a version before probes were added; backfill any missing ones. desired := buildVMDPServerDeployment(v.Namespace, vmdpServerImage) desiredContainer := desired.Spec.Template.Spec.Containers[0] - currentContainer := &deployment.Spec.Template.Spec.Containers[0] + currentContainer := &deployment.Spec.Template.Spec.Containers[idx] needsUpdate := false if currentContainer.ReadinessProbe == nil && desiredContainer.ReadinessProbe != nil { currentContainer.ReadinessProbe = desiredContainer.ReadinessProbe diff --git a/internal/controller/vmdp_download_controller_test.go b/internal/controller/vmdp_download_controller_test.go index aef4eb0e84..8f277ce390 100644 --- a/internal/controller/vmdp_download_controller_test.go +++ b/internal/controller/vmdp_download_controller_test.go @@ -1,11 +1,28 @@ package controller import ( + "context" "testing" + "github.com/go-logr/logr" + routev1 "github.com/openshift/api/route/v1" + appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/intstr" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" ) +// newTestVMDPRoute returns a VMDP server route with a hostname already +// assigned, so reconcileVMDPResources doesn't hit its hostname-assignment +// retry/backoff path. +func newTestVMDPRoute(namespace string) *routev1.Route { + route := buildVMDPServerRoute(namespace) + route.Spec.Host = "vmdp.example.com" + return route +} + func TestBuildVMDPServerDeployment_StartupProbe(t *testing.T) { deployment := buildVMDPServerDeployment("openshift-adp", "test-image") @@ -29,6 +46,12 @@ func TestBuildVMDPServerDeployment_StartupProbe(t *testing.T) { 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") @@ -37,3 +60,136 @@ func TestBuildVMDPServerDeployment_StartupProbe(t *testing.T) { t.Fatal("expected LivenessProbe to remain set") } } + +func TestReconcileVMDPResources_BackfillsMissingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(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: vmdpServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + {Name: "oadp-vmdp-server", Image: "old-image"}, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestVMDPRoute(namespace)). + Build() + + setup := &VMDPDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileVMDPResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileVMDPResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: vmdpServerDeploymentName, 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 TestReconcileVMDPResources_DoesNotOverwriteExistingProbes(t *testing.T) { + namespace := "openshift-adp" + testScheme := getDownloadTestScheme(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: vmdpServerDeploymentName, Namespace: namespace}, + Spec: appsv1.DeploymentSpec{ + Template: corev1.PodTemplateSpec{ + Spec: corev1.PodSpec{ + Containers: []corev1.Container{ + { + Name: "oadp-vmdp-server", + Image: "old-image", + ReadinessProbe: customProbe.DeepCopy(), + LivenessProbe: customProbe.DeepCopy(), + StartupProbe: customProbe.DeepCopy(), + }, + }, + }, + }, + }, + } + + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(existingDeployment, newTestVMDPRoute(namespace)). + Build() + + setup := &VMDPDownloadSetup{ + Client: fakeClient, + Namespace: namespace, + OperatorName: "oadp-operator", + OperatorNamespace: namespace, + Log: logr.Discard(), + } + + if err := setup.reconcileVMDPResources(context.Background(), operatorDeployment, "test-image"); err != nil { + t.Fatalf("reconcileVMDPResources returned error: %v", err) + } + + updated := &appsv1.Deployment{} + if err := fakeClient.Get(context.Background(), client.ObjectKey{Name: vmdpServerDeploymentName, 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) + } +} From 526dc4aa6e7cd7889fda0f11e24313e32fb8f582 Mon Sep 17 00:00:00 2001 From: Nicholas Yancey Date: Wed, 22 Jul 2026 22:15:07 -0400 Subject: [PATCH 3/3] fixed error statements to be better --- .../cli_download_controller_test.go | 56 +++++++++++++------ .../vmdp_download_controller_test.go | 41 ++++++++------ 2 files changed, 61 insertions(+), 36 deletions(-) diff --git a/internal/controller/cli_download_controller_test.go b/internal/controller/cli_download_controller_test.go index 56963d10d6..a73b643e39 100644 --- a/internal/controller/cli_download_controller_test.go +++ b/internal/controller/cli_download_controller_test.go @@ -2,6 +2,7 @@ package controller import ( "context" + "strings" "testing" "github.com/go-logr/logr" @@ -16,6 +17,7 @@ import ( "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) // getDownloadTestScheme returns a scheme with the console/route API groups @@ -258,6 +260,20 @@ func newCLITestScheme(t *testing.T) *runtime.Scheme { return testScheme } +// assertExpectedRouteSchemeError fails the test unless err is the intentional +// error caused by newCLITestScheme omitting routev1/consolev1: reconcile is +// expected to fail once it reaches the Route step, but only after the +// ServiceAccount/Deployment steps under test have already run and persisted +// their results. Any other error indicates a real regression in an earlier +// step and must not be silently treated as "expected". +func assertExpectedRouteSchemeError(t *testing.T, err error) { + t.Helper() + if !strings.Contains(err.Error(), "no kind is registered for the type v1.Route") { + t.Fatalf("expected error from unregistered Route scheme, got unexpected error: %v", err) + } + t.Logf("got expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) +} + func newCLITestOperatorDeployment() *appsv1.Deployment { return &appsv1.Deployment{ ObjectMeta: metav1.ObjectMeta{ @@ -280,7 +296,7 @@ func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) { // Expect an error once reconcileCLIResources reaches the unregistered // Route/ConsoleCLIDownload steps — irrelevant to this test. if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -321,7 +337,7 @@ func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) { setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -377,7 +393,7 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -399,7 +415,11 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) { // TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect verifies a // ServiceAccount already matching the desired state is not unnecessarily -// updated (no ResourceVersion bump). +// updated. Asserts on the number of Update calls the fake client actually +// received (via an interceptor) rather than comparing ResourceVersion +// before/after, since the fake client's ResourceVersion bookkeeping is only +// an approximation of a real API server's and isn't a reliable signal that +// no update occurred. func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { const ns = "openshift-adp" testScheme := newCLITestScheme(t) @@ -410,24 +430,24 @@ func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, } - fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() - - before := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, before); err != nil { - t.Fatalf("failed to get SA: %v", err) - } + updateCalls := 0 + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(operatorDeploy, desired.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updateCalls++ + return c.Update(ctx, obj, opts...) + }, + }). + Build() setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileCLIResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) - } - - after := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: cliServerServiceAccountName, Namespace: ns}, after); err != nil { - t.Fatalf("failed to get SA: %v", err) + assertExpectedRouteSchemeError(t, err) } - if before.ResourceVersion != after.ResourceVersion { - t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + if updateCalls != 0 { + t.Errorf("expected no Update calls when ServiceAccount already matches desired state, got %d", updateCalls) } } diff --git a/internal/controller/vmdp_download_controller_test.go b/internal/controller/vmdp_download_controller_test.go index afbfd2c198..52265d78cb 100644 --- a/internal/controller/vmdp_download_controller_test.go +++ b/internal/controller/vmdp_download_controller_test.go @@ -13,6 +13,7 @@ import ( "k8s.io/apimachinery/pkg/util/intstr" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" ) // newTestVMDPRoute returns a VMDP server route with a hostname already @@ -241,7 +242,7 @@ func TestReconcileVMDPResources_CreatesServiceAccountWhenMissing(t *testing.T) { setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -282,7 +283,7 @@ func TestReconcileVMDPResources_FixesExistingServiceAccountDrift(t *testing.T) { setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -338,7 +339,7 @@ func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) + assertExpectedRouteSchemeError(t, err) } got := &corev1.ServiceAccount{} @@ -360,7 +361,11 @@ func TestReconcileVMDPResources_FixesMissingOwnerReferenceOnly(t *testing.T) { // TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect verifies a // ServiceAccount already matching the desired state is not unnecessarily -// updated (no ResourceVersion bump). +// updated. Asserts on the number of Update calls the fake client actually +// received (via an interceptor) rather than comparing ResourceVersion +// before/after, since the fake client's ResourceVersion bookkeeping is only +// an approximation of a real API server's and isn't a reliable signal that +// no update occurred. func TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) { const ns = "openshift-adp" testScheme := newCLITestScheme(t) @@ -371,24 +376,24 @@ func TestReconcileVMDPResources_NoopWhenServiceAccountAlreadyCorrect(t *testing. {UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"}, } - fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build() - - before := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, before); err != nil { - t.Fatalf("failed to get SA: %v", err) - } + updateCalls := 0 + fakeClient := fake.NewClientBuilder(). + WithScheme(testScheme). + WithObjects(operatorDeploy, desired.DeepCopy()). + WithInterceptorFuncs(interceptor.Funcs{ + Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error { + updateCalls++ + return c.Update(ctx, obj, opts...) + }, + }). + Build() setup := &VMDPDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()} if err := setup.reconcileVMDPResources(context.Background(), operatorDeploy, "test-image"); err != nil { - t.Logf("reconcileVMDPResources returned expected error (Route/ConsoleCLIDownload step unregistered in test scheme): %v", err) - } - - after := &corev1.ServiceAccount{} - if err := fakeClient.Get(context.Background(), types.NamespacedName{Name: vmdpServerServiceAccountName, Namespace: ns}, after); err != nil { - t.Fatalf("failed to get SA: %v", err) + assertExpectedRouteSchemeError(t, err) } - if before.ResourceVersion != after.ResourceVersion { - t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion) + if updateCalls != 0 { + t.Errorf("expected no Update calls when ServiceAccount already matches desired state, got %d", updateCalls) } }