[oadp-1.6] OADP-8483: Add startup probe to CLI and VMDP download servers

internal/controller/cli_download_controller.go

@@ -171,11 +171,21 @@ func (c *CLIDownloadSetup) reconcileCLIResources(ctx context.Context, operatorDe
 		// Deployment exists from a version before probes/service account were added; backfill them.
 		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 && 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 deployment.Spec.Template.Spec.ServiceAccountName != cliServerServiceAccountName {
 			deployment.Spec.Template.Spec.ServiceAccountName = desired.Spec.Template.Spec.ServiceAccountName
@@ -191,7 +201,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")
 		}
 	}
 
@@ -444,6 +454,17 @@ func buildCLIServerDeployment(namespace, image string) *appsv1.Deployment {
 								InitialDelaySeconds: 15,
 								PeriodSeconds:       20,
 							},
+							StartupProbe: &corev1.Probe{
+								ProbeHandler: corev1.ProbeHandler{
+									HTTPGet: &corev1.HTTPGetAction{
+										Path: "/",
+										Port: intstr.FromString("http"),
+									},
+								},
+								InitialDelaySeconds: 5,
+								PeriodSeconds:       5,
+								FailureThreshold:    12,
+							},
 						},
 					},
 					TerminationGracePeriodSeconds: int64Ptr(10),
@@ -524,3 +545,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
+}

internal/controller/cli_download_controller_test.go

@@ -5,14 +5,41 @@ import (
 	"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/types"
+	"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_ServiceAccount(t *testing.T) {
 	deployment := buildCLIServerDeployment("openshift-adp", "test-image")
 	podSpec := deployment.Spec.Template.Spec
@@ -51,14 +78,14 @@ func TestBuildCLIServerServiceAccount(t *testing.T) {
 // ignored: by that point the ServiceAccount step (step 1) has already run
 // and persisted its result, which is what these tests verify.
 func newCLITestScheme(t *testing.T) *runtime.Scheme {
-	scheme := runtime.NewScheme()
-	if err := corev1.AddToScheme(scheme); err != nil {
+	testScheme := runtime.NewScheme()
+	if err := corev1.AddToScheme(testScheme); err != nil {
 		t.Fatal(err)
 	}
-	if err := appsv1.AddToScheme(scheme); err != nil {
+	if err := appsv1.AddToScheme(testScheme); err != nil {
 		t.Fatal(err)
 	}
-	return scheme
+	return testScheme
 }
 
 func newCLITestOperatorDeployment() *appsv1.Deployment {
@@ -75,9 +102,9 @@ func newCLITestOperatorDeployment() *appsv1.Deployment {
 // ServiceAccount is created with the desired state when it doesn't exist.
 func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) {
 	const ns = "openshift-adp"
-	scheme := newCLITestScheme(t)
+	testScheme := newCLITestScheme(t)
 	operatorDeploy := newCLITestOperatorDeployment()
-	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy).Build()
+	fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy).Build()
 
 	setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()}
 	// Expect an error once reconcileCLIResources reaches the unregistered
@@ -108,7 +135,7 @@ func TestReconcileCLIResources_CreatesServiceAccountWhenMissing(t *testing.T) {
 func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) {
 	const ns = "openshift-adp"
 	trueVal := true
-	scheme := newCLITestScheme(t)
+	testScheme := newCLITestScheme(t)
 	operatorDeploy := newCLITestOperatorDeployment()
 
 	existing := &corev1.ServiceAccount{
@@ -120,7 +147,7 @@ func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) {
 		AutomountServiceAccountToken: &trueVal, // wrong: should be false
 	}
 
-	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build()
+	fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build()
 
 	setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()}
 	if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil {
@@ -159,7 +186,7 @@ func TestReconcileCLIResources_FixesExistingServiceAccountDrift(t *testing.T) {
 func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) {
 	const ns = "openshift-adp"
 	falseVal := false
-	scheme := newCLITestScheme(t)
+	testScheme := newCLITestScheme(t)
 	operatorDeploy := newCLITestOperatorDeployment()
 
 	existing := &corev1.ServiceAccount{
@@ -176,7 +203,7 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) {
 		AutomountServiceAccountToken: &falseVal,
 	}
 
-	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, existing).Build()
+	fakeClient := fake.NewClientBuilder().WithScheme(testScheme).WithObjects(operatorDeploy, existing).Build()
 
 	setup := &CLIDownloadSetup{Client: fakeClient, Namespace: ns, Log: logr.Discard()}
 	if err := setup.reconcileCLIResources(context.Background(), operatorDeploy, "test-image"); err != nil {
@@ -205,15 +232,15 @@ func TestReconcileCLIResources_FixesMissingOwnerReferenceOnly(t *testing.T) {
 // updated (no ResourceVersion bump).
 func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T) {
 	const ns = "openshift-adp"
-	scheme := newCLITestScheme(t)
+	testScheme := newCLITestScheme(t)
 	operatorDeploy := newCLITestOperatorDeployment()
 
 	desired := buildCLIServerServiceAccount(ns)
 	desired.OwnerReferences = []metav1.OwnerReference{
 		{UID: operatorDeploy.UID, Name: operatorDeploy.Name, Kind: "Deployment", APIVersion: "apps/v1"},
 	}
 
-	fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithObjects(operatorDeploy, desired.DeepCopy()).Build()
+	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 {
@@ -234,3 +261,174 @@ func TestReconcileCLIResources_NoopWhenServiceAccountAlreadyCorrect(t *testing.T
 		t.Errorf("expected no update (ResourceVersion unchanged), got before=%s after=%s", before.ResourceVersion, after.ResourceVersion)
 	}
 }
+
+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 := 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)
+	}
+}