Skip to content

feat(operator): allow pod scheduling on the proxy Deployment via resourceOverrides - #6183

Open
talshechanovitz wants to merge 3 commits into
stacklok:mainfrom
talshechanovitz:talsh/proxy-deployment-scheduling-overrides
Open

feat(operator): allow pod scheduling on the proxy Deployment via resourceOverrides#6183
talshechanovitz wants to merge 3 commits into
stacklok:mainfrom
talshechanovitz:talsh/proxy-deployment-scheduling-overrides

Conversation

@talshechanovitz

Copy link
Copy Markdown

Closes #5879.

What

Adds nodeSelector, tolerations and affinity to resourceOverrides.proxyDeployment and applies them to the proxy pod, so the proxy can be steered onto the same nodes as the MCP server pod.

Without it the proxy is repelled by a dedicated pre-warmed pool's taint, lands on a cold general node, and the ~40s node boot is paid anyway — the server is warm but the proxy isn't.

Diff shape: +335 hand-written, +7,827 generated. All the generated volume is corev1.Affinity's schema expanding into four CRD files; regenerating on a clean tree produces no churn, so it's all attributable here.

Scope — please sanity-check this call

@ChrisJBurns asked to keep this to MCPServer. That turns out not to be cleanly possible: ProxyDeploymentOverrides is reached via ResourceOverrides, which MCPRemoteProxy also embeds (mcpremoteproxy_types.go:153), so the fields land on both CRDs regardless.

This PR wires both, rather than leaving MCPRemoteProxy accepting fields it ignores.

One wrinkle worth knowing before you weigh in — the two resources aren't symmetric:

Can it already schedule its proxy? Effect of these fields
MCPServer NopodTemplateSpec is marshalled into a runtime patch for the MCP pod and never reaches the proxy PodSpec the actual fix
MCPRemoteProxy YespodTemplateSpec is applied to its proxy Deployment directly a second route to the same field

And where both are set on MCPRemoteProxy, podTemplateSpec wins silently (verified: nodeSelector from podTemplateSpec overwrites the override).

So there's a case for splitting the type instead, giving MCPServer its own overrides type so scheduling never appears on MCPRemoteProxy. I didn't, for one reason: it needs two new API types plus retyping ~24 ResourceOverrides literals across ~10 files, which makes the review considerably larger than the alternative you were trying to avoid. Happy to do it if you'd rather have the clean separation — just say and I'll push it.

How

  • Three fields on ProxyDeploymentOverrides (mcpserver_types.go)
  • proxyDeploymentScheduling(*ResourceOverrides) — shared helper, so both controllers behave identically from one implementation
  • Applied to the proxy PodSpec in deploymentForMCPServer and deploymentForMCPRemoteProxy
  • proxySchedulingNeedsUpdate — shared drift predicate, used by both deploymentNeedsUpdate paths
  • task operator-generate + task operator-manifests

Simpler than I sketched on the issue: both operators build their proxy PodSpec directly, so no runtime or applyPodTemplatePatch change is needed.

The overrides are authoritative, not merged — neither controller sets proxy scheduling of its own today (every NodeSelector/Tolerations/Affinity match under cmd/thv-operator is SessionAffinity, a Service field).

Drift detection

Worth calling out because the first version of this PR got it wrong: deploymentNeedsUpdate compared no scheduling fields, so editing the overrides on an existing CR silently did nothing until an unrelated field changed. Create worked, update didn't.

Both controllers now compare, following the ImagePullSecrets precedent (equality.Semantic.DeepEqual, so nil ≡ empty and an unset override isn't perpetual drift).

The comparison has to differ per resource, which TestMCPRemoteProxyPodTemplateSpecDriftDetection caught immediately: for MCPRemoteProxy scheduling may come from podTemplateSpec, so it compares against the rebuilt Deployment; comparing against the overrides alone reported false drift — an infinite reconcile loop. MCPServer needs no equivalent, since its podTemplateSpec never touches the proxy PodSpec.

Tests

  • TestDeploymentForMCPServer_ProxyDeploymentScheduling — each field alone, all three together, and no overrides asserting scheduling stays unset so the default path is provably unchanged
  • TestMCPServerDeploymentNeedsUpdate_ProxySchedulingDrift — spec-gains-value, spec-cleared-with-stale-deployment, nil-vs-empty-is-not-drift
  • TestMCPRemoteProxySchedulingOverridesAndPodTemplateSpec — overrides applied and stable, clearing them detected, and podTemplateSpec-supplied scheduling not mistaken for drift

go test ./cmd/thv-operator/... — 21 packages pass. The test-integration/... suites fail locally on missing envtest binaries (kubebuilder/bin/etcd): 23 failures both with this change and on a clean main, so environmental here. I couldn't run golangci-lint locally either — the installed build targets go1.23 and the repo is on 1.26 — so CI will be the first real lint pass; gofmt is clean.

Follow-up

VirtualMCPServer, as agreed on the issue.

…urceOverrides

Adds nodeSelector, tolerations and affinity to resourceOverrides.proxyDeployment
and applies them to the proxy pod spec, matching the scheduling control
spec.podTemplateSpec already gives the MCP server pod.

Without this the proxy is repelled by a dedicated pre-warmed pool's taint and
lands on a cold node, so the server is warm but the proxy is not and the
cold-start cost is paid anyway.

The operator sets no proxy scheduling of its own, so the overrides are
authoritative rather than merged.

Refs stacklok#5879
…ling drift

The helper had been inserted between deploymentForMCPServer's doc comment and
the function, hijacking both the docstring and the nolint:gocyclo directive.

deploymentNeedsUpdate now compares the proxy pod's scheduling fields against
the shared proxyDeploymentScheduling helper (equality.Semantic, matching the
ImagePullSecrets precedent) — without this, editing the overrides on an
existing MCPServer silently changed nothing. Adds a drift test mirroring
TestMCPServerDeploymentNeedsUpdate_ImagePullSecretsDrift.
MCPServer and MCPRemoteProxy share the ResourceOverrides type, so the three
scheduling fields already appeared on both CRDs. Wiring only MCPServer left
MCPRemoteProxy accepting them and silently ignoring them, so this honours them
there as well (issue stacklok#5879, option 2).

proxyDeploymentScheduling now takes ResourceOverrides directly and a shared
proxySchedulingNeedsUpdate predicate serves both controllers' drift checks.

MCPRemoteProxy's podTemplateSpec is applied to the proxy Deployment itself, so
in that branch scheduling is compared against the rebuilt Deployment rather
than the overrides alone — comparing against overrides reported false drift on
a podTemplateSpec-supplied nodeSelector. MCPServer needs no equivalent: its
podTemplateSpec becomes a runtime patch for the MCP pod and never touches the
proxy PodSpec.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Operator: allow pod scheduling (nodeSelector/tolerations/affinity) on the proxy Deployment via MCPServer resourceOverrides

2 participants