fix: reject nodeSelector on sandbox agents instead of silently dropping it - #2348
Open
younsl wants to merge 3 commits into
Open
fix: reject nodeSelector on sandbox agents instead of silently dropping it#2348younsl wants to merge 3 commits into
younsl wants to merge 3 commits into
Conversation
…ng it Substrate ActorTemplates carry no node placement (actors are scheduled onto WorkerPool workers), so a per-agent deployment.nodeSelector on a SandboxAgent was silently discarded by the sandbox backend and the workload could land on an unintended node. Reject the configuration in ValidateSubstrateSandboxAgentSpec so the API server, reconciler, and compiler all surface an explicit error pointing at the WorkerPool's nodeSelector as the supported alternative. Fixes kagent-dev#2306 Signed-off-by: younsl <cysl@kakao.com>
EItanya
reviewed
Jul 28, 2026
EItanya
left a comment
EItanya
left a comment
Contributor
There was a problem hiding this comment.
What about using CEL to reject the whole deployment spec on SandboxAgent
younsl
force-pushed
the
fix/sandbox-nodeselector-reject
branch
from
July 28, 2026 14:26
11176e1 to
8f60459
Compare
Per review feedback, reject deployment.nodeSelector on SandboxAgent via CEL XValidation rules on SandboxAgentSpec so the API server rejects the config at admission instead of the controller discovering it at reconcile time. Covers both declarative and BYO deployments; an empty map is still accepted, matching the Go check. The Go-side ValidateSubstrateSandboxAgentSpec check is kept (mirroring the existing spec.skills pattern) for objects created before the rules shipped and callers that bypass the API server. Adds an envtest-backed CEL test pinning the rules against the shipped CRD YAML. Signed-off-by: younsl <cysl@kakao.com>
younsl
force-pushed
the
fix/sandbox-nodeselector-reject
branch
from
July 28, 2026 14:27
8f60459 to
cfa8906
Compare
Contributor
Author
|
@EItanya Agreed, CEL is the right place for this. Done in cfa8906. I scoped the CEL rules to deployment.nodeSelector instead of rejecting the whole deployment spec. BYO agents require deployment.image and deployment.cmd, and substrate honors deployment.env when rendering ActorTemplates, so rejecting the entire block would break supported configuration. The Go-side check is kept alongside the CEL rules, following the existing spec.skills pattern, so objects created before the rules shipped are still caught at reconcile time. Also added an envtest backed test that pins the rules against the shipped CRD YAML. |
…failure Signed-off-by: younsl <cysl@kakao.com>
Contributor
Author
|
@EItanya CI is clean now. PTAL |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2306
For sandbox agents, a configured
deployment.nodeSelectorwas silently dropped: the translator resolves it into the sharedPodTemplateSpec, but the substrate backend builds itsActorTemplateSpeconly from the kagent container and worker pool and never readspodTemplate.Spec.NodeSelector. The sandbox workload could therefore be scheduled onto an unintended node with no signal that the config was ignored.ActorTemplateSpechas no node-placement field at all — substrate schedules actors onto WorkerPool workers, and node placement is governed by the WorkerPool's ownnodeSelector. So a per-agent selector cannot be honored by mapping it through; the issue's second suggested option (explicit rejection) is implemented instead:ValidateSubstrateSandboxAgentSpecnow rejects a non-emptydeployment.nodeSelector(declarative and BYO) with an error pointing at the WorkerPool'snodeSelectoras the supported alternative.Notes for reviewers
--default-agent-node-selectorflag is intentionally not rejected here: it is not part of the agent spec, and hard-failing every sandbox agent in clusters that set a global default would be a breaking regression. The sandbox backend simply does not consume it (placement is the WorkerPool's job).tolerations/affinityare also silently ignored by the sandbox backend today; left out of scope to keep this PR focused on the reported bug — can follow up if maintainers want the same treatment.Testing
TestValidateSubstrateSandboxAgentSpec: declarative selector rejected, BYO selector rejected, empty selector map still allowed.go test ./...ingo/api, plusgo/corecontroller/translator/sandboxbackend packages — pass.make -C go lint— 0 issues.