diff --git a/cmd/thv-operator/controllers/mcpserver_controller.go b/cmd/thv-operator/controllers/mcpserver_controller.go index 8cfe422175..013131a752 100644 --- a/cmd/thv-operator/controllers/mcpserver_controller.go +++ b/cmd/thv-operator/controllers/mcpserver_controller.go @@ -1879,6 +1879,11 @@ func int32Ptr(i int32) *int32 { return &i } +// int64Ptr returns a pointer to an int64 +func int64Ptr(i int64) *int64 { + return &i +} + // SetupWithManager sets up the controller with the Manager. func (r *MCPServerReconciler) SetupWithManager(mgr ctrl.Manager) error { // Create a handler that maps MCPExternalAuthConfig changes to MCPServer reconciliation requests diff --git a/cmd/thv-operator/controllers/mcpserver_pod_template_test.go b/cmd/thv-operator/controllers/mcpserver_pod_template_test.go index a6a03ab268..befca2d597 100644 --- a/cmd/thv-operator/controllers/mcpserver_pod_template_test.go +++ b/cmd/thv-operator/controllers/mcpserver_pod_template_test.go @@ -404,7 +404,3 @@ func TestProxyRunnerStructuredLogsEnvVar(t *testing.T) { func boolPtr(b bool) *bool { return &b } - -func int64Ptr(i int64) *int64 { - return &i -} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller.go b/cmd/thv-operator/controllers/virtualmcpserver_controller.go index 10b191b047..f1ed4a9bce 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller.go @@ -927,7 +927,8 @@ func (r *VirtualMCPServerReconciler) ensureDeployment( // - Update Spec.Template: Contains container spec, volumes, pod metadata (triggers rollout) // - Update Labels: For label selectors and queries // - Update Annotations: For metadata and tooling - // - Preserve Spec.Replicas: Allows HPA/VPA to manage scaling independently + // - Sync Spec.Replicas when spec.replicas is non-nil (operator authoritative) + // - Preserve Spec.Replicas when spec.replicas is nil (HPA or external controller manages scaling) // - Preserve ResourceVersion, UID: Required for optimistic concurrency control // // Note: If update conflicts occur due to concurrent modifications, the reconcile @@ -935,6 +936,9 @@ func (r *VirtualMCPServerReconciler) ensureDeployment( deployment.Spec.Template = newDeployment.Spec.Template deployment.Labels = newDeployment.Labels deployment.Annotations = ctrlutil.MergeAnnotations(newDeployment.Annotations, deployment.Annotations) + if newDeployment.Spec.Replicas != nil { + deployment.Spec.Replicas = newDeployment.Spec.Replicas + } ctxLogger.Info("Updating Deployment", "Deployment.Namespace", deployment.Namespace, "Deployment.Name", deployment.Name) if err := r.Update(ctx, deployment); err != nil { @@ -1077,6 +1081,14 @@ func (r *VirtualMCPServerReconciler) deploymentNeedsUpdate( return true } + // Check if spec.replicas has changed. Only compare when spec.replicas is non-nil; + // nil means hands-off mode (HPA or external controller manages replicas) and the live count is authoritative. + if vmcp.Spec.Replicas != nil { + if deployment.Spec.Replicas == nil || *deployment.Spec.Replicas != *vmcp.Spec.Replicas { + return true + } + } + return false } diff --git a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go index b43ee5a9d2..d4719aeb2c 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_controller_test.go @@ -697,8 +697,8 @@ func TestVirtualMCPServerEnsureDeployment(t *testing.T) { }, deployment) require.NoError(t, err) assert.Equal(t, vmcp.Name, deployment.Name) - assert.NotNil(t, deployment.Spec.Replicas) - assert.Equal(t, int32(1), *deployment.Spec.Replicas) + // spec.replicas is nil — nil-passthrough for HPA compatibility + assert.Nil(t, deployment.Spec.Replicas) // Verify container configuration require.Len(t, deployment.Spec.Template.Spec.Containers, 1) @@ -3147,3 +3147,163 @@ func TestVirtualMCPServerValidateEmbeddingServerRef(t *testing.T) { }) } } + +// TestVirtualMCPServerEnsureDeployment_ReplicaSync_SpecDriven verifies that when +// spec.replicas is set, ensureDeployment updates the Deployment to match. +func TestVirtualMCPServerEnsureDeployment_ReplicaSync_SpecDriven(t *testing.T) { + t.Parallel() + + specReplicas := int32(3) + vmcp := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vmcp-replica-sync", + Namespace: "default", + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: testGroupName}, + Replicas: &specReplicas, + }, + } + + mcpGroup := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: testGroupName, Namespace: "default"}, + Status: mcpv1alpha1.MCPGroupStatus{Phase: mcpv1alpha1.MCPGroupPhaseReady}, + } + + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpConfigMapName(vmcp.Name), + Namespace: "default", + Annotations: map[string]string{ + checksum.ContentChecksumAnnotation: testChecksumValue, + }, + }, + Data: map[string]string{"config.yaml": "{}"}, + } + + // Existing deployment has 1 replica — simulates a pre-existing state + existingReplicas := int32(1) + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcp.Name, + Namespace: "default", + Labels: labelsForVirtualMCPServer(vmcp.Name), + }, + Spec: appsv1.DeploymentSpec{ + Replicas: &existingReplicas, + Selector: &metav1.LabelSelector{MatchLabels: labelsForVirtualMCPServer(vmcp.Name)}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labelsForVirtualMCPServer(vmcp.Name)}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "vmcp", Image: "test:latest"}}}, + }, + }, + } + + scheme := runtime.NewScheme() + _ = mcpv1alpha1.AddToScheme(scheme) + _ = corev1.AddToScheme(scheme) + _ = appsv1.AddToScheme(scheme) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(vmcp, mcpGroup, configMap, existingDeployment). + Build() + + r := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + result, err := r.ensureDeployment(context.Background(), vmcp, []workloads.TypedWorkload{}) + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + + updated := &appsv1.Deployment{} + err = fakeClient.Get(context.Background(), types.NamespacedName{ + Name: vmcp.Name, Namespace: vmcp.Namespace, + }, updated) + require.NoError(t, err) + require.NotNil(t, updated.Spec.Replicas) + assert.Equal(t, int32(3), *updated.Spec.Replicas) +} + +// TestVirtualMCPServerEnsureDeployment_ReplicaSync_NilPassthrough verifies that when +// spec.replicas is nil, ensureDeployment does not overwrite a live replica count (HPA-managed). +func TestVirtualMCPServerEnsureDeployment_ReplicaSync_NilPassthrough(t *testing.T) { + t.Parallel() + + vmcp := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vmcp-nil-passthrough", + Namespace: "default", + }, + Spec: mcpv1alpha1.VirtualMCPServerSpec{ + Config: vmcpconfig.Config{Group: testGroupName}, + Replicas: nil, // HPA manages replicas + }, + } + + mcpGroup := &mcpv1alpha1.MCPGroup{ + ObjectMeta: metav1.ObjectMeta{Name: testGroupName, Namespace: "default"}, + Status: mcpv1alpha1.MCPGroupStatus{Phase: mcpv1alpha1.MCPGroupPhaseReady}, + } + + configMap := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcpConfigMapName(vmcp.Name), + Namespace: "default", + Annotations: map[string]string{ + checksum.ContentChecksumAnnotation: testChecksumValue, + }, + }, + Data: map[string]string{"config.yaml": "{}"}, + } + + // Existing deployment has 5 replicas — set by HPA + hpaReplicas := int32(5) + existingDeployment := &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: vmcp.Name, + Namespace: "default", + Labels: labelsForVirtualMCPServer(vmcp.Name), + }, + Spec: appsv1.DeploymentSpec{ + Replicas: &hpaReplicas, + Selector: &metav1.LabelSelector{MatchLabels: labelsForVirtualMCPServer(vmcp.Name)}, + Template: corev1.PodTemplateSpec{ + ObjectMeta: metav1.ObjectMeta{Labels: labelsForVirtualMCPServer(vmcp.Name)}, + Spec: corev1.PodSpec{Containers: []corev1.Container{{Name: "vmcp", Image: "test:latest"}}}, + }, + }, + } + + scheme := runtime.NewScheme() + _ = mcpv1alpha1.AddToScheme(scheme) + _ = corev1.AddToScheme(scheme) + _ = appsv1.AddToScheme(scheme) + + fakeClient := fake.NewClientBuilder(). + WithScheme(scheme). + WithObjects(vmcp, mcpGroup, configMap, existingDeployment). + Build() + + r := &VirtualMCPServerReconciler{ + Client: fakeClient, + Scheme: scheme, + PlatformDetector: ctrlutil.NewSharedPlatformDetector(), + } + + result, err := r.ensureDeployment(context.Background(), vmcp, []workloads.TypedWorkload{}) + require.NoError(t, err) + assert.Equal(t, ctrl.Result{}, result) + + updated := &appsv1.Deployment{} + err = fakeClient.Get(context.Background(), types.NamespacedName{ + Name: vmcp.Name, Namespace: vmcp.Namespace, + }, updated) + require.NoError(t, err) + // HPA-managed replica count must not be overwritten + require.NotNil(t, updated.Spec.Replicas) + assert.Equal(t, int32(5), *updated.Spec.Replicas) +} diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go index ae539b95a5..6e43a1152f 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment.go @@ -56,6 +56,9 @@ const ( vmcpReadinessTimeout = int32(3) // seconds - shorter timeout for faster detection vmcpReadinessFailures = int32(3) // consecutive failures before removing from service + // Graceful shutdown configuration + vmcpTerminationGracePeriodSeconds = int64(30) // seconds - allow in-flight requests to complete + // Default resource requirements for VirtualMCPServer vmcp container // These provide sensible defaults that can be overridden via PodTemplateSpec vmcpDefaultCPURequest = "100m" @@ -117,7 +120,6 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( typedWorkloads []workloads.TypedWorkload, ) *appsv1.Deployment { ls := labelsForVirtualMCPServer(vmcp.Name) - replicas := int32(1) // Build deployment components using helper functions args := r.buildContainerArgsForVmcp(vmcp) @@ -136,7 +138,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( Annotations: deploymentAnnotations, }, Spec: appsv1.DeploymentSpec{ - Replicas: &replicas, + Replicas: vmcp.Spec.Replicas, Selector: &metav1.LabelSelector{ MatchLabels: ls, }, @@ -146,7 +148,8 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer( Annotations: deploymentTemplateAnnotations, }, Spec: corev1.PodSpec{ - ServiceAccountName: serviceAccountName, + TerminationGracePeriodSeconds: int64Ptr(vmcpTerminationGracePeriodSeconds), + ServiceAccountName: serviceAccountName, Containers: []corev1.Container{{ Image: getVmcpImage(), ImagePullPolicy: corev1.PullIfNotPresent, @@ -280,8 +283,8 @@ func (r *VirtualMCPServerReconciler) buildEnvVarsForVmcp( // Always mount HMAC secret for session token binding. env = append(env, r.buildHMACSecretEnvVar(vmcp)) - // Note: Other secrets (Redis passwords, service account credentials) may be added here in the future - // following the same pattern of mounting from Kubernetes Secrets as environment variables. + // Mount Redis password secret when session storage provider is Redis. + env = append(env, r.buildRedisPasswordEnvVar(vmcp)...) return ctrlutil.EnsureRequiredEnvVars(ctx, env) } @@ -347,6 +350,27 @@ func (*VirtualMCPServerReconciler) buildHMACSecretEnvVar(vmcp *mcpv1alpha1.Virtu } } +// buildRedisPasswordEnvVar returns the THV_SESSION_REDIS_PASSWORD env var when +// sessionStorage.provider == "redis" and passwordRef is set; returns nil otherwise. +func (*VirtualMCPServerReconciler) buildRedisPasswordEnvVar(vmcp *mcpv1alpha1.VirtualMCPServer) []corev1.EnvVar { + if vmcp.Spec.SessionStorage == nil || + vmcp.Spec.SessionStorage.Provider != "redis" || + vmcp.Spec.SessionStorage.PasswordRef == nil { + return nil + } + return []corev1.EnvVar{{ + Name: "THV_SESSION_REDIS_PASSWORD", + ValueFrom: &corev1.EnvVarSource{ + SecretKeyRef: &corev1.SecretKeySelector{ + LocalObjectReference: corev1.LocalObjectReference{ + Name: vmcp.Spec.SessionStorage.PasswordRef.Name, + }, + Key: vmcp.Spec.SessionStorage.PasswordRef.Key, + }, + }, + }} +} + // buildOutgoingAuthEnvVars builds environment variables for outgoing auth secrets. func (r *VirtualMCPServerReconciler) buildOutgoingAuthEnvVars( ctx context.Context, diff --git a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go index dd95dfe9d4..e2fc4984b5 100644 --- a/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go +++ b/cmd/thv-operator/controllers/virtualmcpserver_deployment_test.go @@ -60,14 +60,18 @@ func TestDeploymentForVirtualMCPServer(t *testing.T) { require.NotNil(t, deployment) assert.Equal(t, vmcp.Name, deployment.Name) assert.Equal(t, vmcp.Namespace, deployment.Namespace) - assert.NotNil(t, deployment.Spec.Replicas) - assert.Equal(t, int32(1), *deployment.Spec.Replicas) + // spec.replicas is nil in this test — nil-passthrough for HPA compatibility + assert.Nil(t, deployment.Spec.Replicas) // Verify labels expectedLabels := labelsForVirtualMCPServer(vmcp.Name) assert.Equal(t, expectedLabels, deployment.Labels) assert.Equal(t, expectedLabels, deployment.Spec.Template.Labels) + // Verify terminationGracePeriodSeconds is always set + require.NotNil(t, deployment.Spec.Template.Spec.TerminationGracePeriodSeconds) + assert.Equal(t, vmcpTerminationGracePeriodSeconds, *deployment.Spec.Template.Spec.TerminationGracePeriodSeconds) + // Verify service account assert.Equal(t, vmcpServiceAccountName(vmcp.Name), deployment.Spec.Template.Spec.ServiceAccountName) @@ -203,6 +207,64 @@ func TestBuildEnvVarsForVmcp(t *testing.T) { assert.True(t, foundNamespace, "Should have VMCP_NAMESPACE env var") } +// TestBuildRedisPasswordEnvVar tests conditional Redis password env var injection. +func TestBuildRedisPasswordEnvVar(t *testing.T) { + t.Parallel() + + r := &VirtualMCPServerReconciler{} + + passwordRef := &mcpv1alpha1.SecretKeyRef{Name: "redis-secret", Key: "password"} + + tests := []struct { + name string + storage *mcpv1alpha1.SessionStorageConfig + expectEnVar bool + }{ + { + name: "nil sessionStorage produces no env var", + storage: nil, + expectEnVar: false, + }, + { + name: "memory provider produces no env var", + storage: &mcpv1alpha1.SessionStorageConfig{Provider: "memory"}, + expectEnVar: false, + }, + { + name: "redis without passwordRef produces no env var", + storage: &mcpv1alpha1.SessionStorageConfig{Provider: "redis", Address: "redis:6379"}, + expectEnVar: false, + }, + { + name: "redis with passwordRef produces THV_SESSION_REDIS_PASSWORD", + storage: &mcpv1alpha1.SessionStorageConfig{Provider: "redis", Address: "redis:6379", PasswordRef: passwordRef}, + expectEnVar: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + vmcp := &mcpv1alpha1.VirtualMCPServer{ + ObjectMeta: metav1.ObjectMeta{Name: "test-vmcp", Namespace: "default"}, + Spec: mcpv1alpha1.VirtualMCPServerSpec{SessionStorage: tc.storage}, + } + env := r.buildRedisPasswordEnvVar(vmcp) + if tc.expectEnVar { + require.Len(t, env, 1) + assert.Equal(t, "THV_SESSION_REDIS_PASSWORD", env[0].Name) + assert.Empty(t, env[0].Value, "must not use plaintext Value") + require.NotNil(t, env[0].ValueFrom) + require.NotNil(t, env[0].ValueFrom.SecretKeyRef) + assert.Equal(t, passwordRef.Name, env[0].ValueFrom.SecretKeyRef.Name) + assert.Equal(t, passwordRef.Key, env[0].ValueFrom.SecretKeyRef.Key) + } else { + assert.Empty(t, env) + } + }) + } +} + // TestBuildDeploymentMetadataForVmcp tests deployment metadata generation func TestBuildDeploymentMetadataForVmcp(t *testing.T) { t.Parallel()