Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions cmd/thv-operator/controllers/mcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
4 changes: 0 additions & 4 deletions cmd/thv-operator/controllers/mcpserver_pod_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,3 @@ func TestProxyRunnerStructuredLogsEnvVar(t *testing.T) {
func boolPtr(b bool) *bool {
return &b
}

func int64Ptr(i int64) *int64 {
return &i
}
14 changes: 13 additions & 1 deletion cmd/thv-operator/controllers/virtualmcpserver_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -927,14 +927,18 @@ 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
// loop will retry automatically. Kubernetes' optimistic locking prevents data loss.
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
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: MCPServer (#4364) adds a SessionStorageWarning condition when replicas > 1 without Redis session storage configured. Is the omission of an equivalent condition for VirtualMCPServer intentional?


ctxLogger.Info("Updating Deployment", "Deployment.Namespace", deployment.Namespace, "Deployment.Name", deployment.Name)
if err := r.Update(ctx, deployment); err != nil {
Expand Down Expand Up @@ -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
}

Expand Down
164 changes: 162 additions & 2 deletions cmd/thv-operator/controllers/virtualmcpserver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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)
}
34 changes: 29 additions & 5 deletions cmd/thv-operator/controllers/virtualmcpserver_deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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)
Expand All @@ -136,7 +138,7 @@ func (r *VirtualMCPServerReconciler) deploymentForVirtualMCPServer(
Annotations: deploymentAnnotations,
},
Spec: appsv1.DeploymentSpec{
Replicas: &replicas,
Replicas: vmcp.Spec.Replicas,
Selector: &metav1.LabelSelector{
MatchLabels: ls,
},
Expand All @@ -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,
Expand Down Expand Up @@ -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)
}
Expand Down Expand Up @@ -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,
},
},
}}
}

Comment on lines +353 to +373
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Does this follow the established pattern for how the proxy runner consumes the Redis password? Want to make sure the env var name and secret mounting approach are consistent.

// buildOutgoingAuthEnvVars builds environment variables for outgoing auth secrets.
func (r *VirtualMCPServerReconciler) buildOutgoingAuthEnvVars(
ctx context.Context,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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)

Expand Down Expand Up @@ -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()
Expand Down
Loading