feat: direct Connect content pods to Karpenter session node pool#356
feat: direct Connect content pods to Karpenter session node pool#356amdove wants to merge 2 commits into
Conversation
Extends the existing Workbench sessionTolerations wiring so Connect content pods receive the same toleration plus a nodeSelector onto the session node pool, via new spec.connect.contentTolerations / contentNodeSelector fields.
|
Claude finished @amdove's task in 1m 50s —— View job PR Review
The implementation is clean and follows existing patterns well. The Two notes:
No correctness or security issues found. |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Karpenter content placement: allow and direct Connect content pods onto the session node pool. | ||
| if tolerations := sessionTolerations(clusterCfg.KarpenterConfig); len(tolerations) > 0 { | ||
| connectSpec := map[string]interface{}{ | ||
| "contentTolerations": tolerations, | ||
| } | ||
| if pool := sessionPoolName(clusterCfg.KarpenterConfig); pool != "" { | ||
| connectSpec["contentNodeSelector"] = map[string]string{ | ||
| "karpenter.sh/nodepool": pool, | ||
| } | ||
| } | ||
| spec["connect"] = connectSpec | ||
| } |
There was a problem hiding this comment.
Important: sessionTolerations is called twice here with the same argument — once for the workbench block above and once here. Since both blocks are gated on the same condition, they can share a single call. Consider merging the two if blocks:
if tolerations := sessionTolerations(clusterCfg.KarpenterConfig); len(tolerations) > 0 {
spec["workbench"] = map[string]interface{}{
"sessionTolerations": tolerations,
}
connectSpec := map[string]interface{}{
"contentTolerations": tolerations,
}
if pool := sessionPoolName(clusterCfg.KarpenterConfig); pool != "" {
connectSpec["contentNodeSelector"] = map[string]string{
"karpenter.sh/nodepool": pool,
}
}
spec["connect"] = connectSpec
}
| // sessionPoolName returns the Name of the first node pool that has | ||
| // session_taints=true, or an empty string if none. This mirrors the | ||
| // selection logic in sessionTolerations. | ||
| func sessionPoolName(kc *types.KarpenterConfig) string { | ||
| if kc == nil { | ||
| return "" | ||
| } | ||
| for _, pool := range kc.NodePools { | ||
| if pool.SessionTaints { | ||
| return pool.Name | ||
| } | ||
| } | ||
| return "" | ||
| } |
There was a problem hiding this comment.
Suggestion: sessionPoolName iterates kc.NodePools independently, but sessionTolerations already does the same scan to find the first session-tainted pool. If those two functions are always called together (as they are in buildAWSSiteSpec), a single helper that returns both the tolerations and the pool name would eliminate duplicate traversal and keep the "find first session-tainted pool" logic in one place. That said, the slice is expected to be short so this is minor — flag it only if you anticipate this pattern growing.
|
Thanks for the review.
|
Lytol
left a comment
Lytol
left a comment
There was a problem hiding this comment.
Awesome, thanks for adding this! I'd make sure to test on an internal workload before merging by the way.
2 participants
Summary
Extends the existing Workbench session-pod placement capability to Posit Connect content pods.
Today PTD writes
spec.workbench.sessionTolerationsinto the Site CR so Workbench session pods can schedule onto Karpenter session-tainted nodes. This PR adds a parallelspec.connectblock so Connect content pods are both:contentTolerations(the sameworkload-type=session:NoScheduletoleration used for Workbench), andcontentNodeSelector(karpenter.sh/nodepool: <session pool name>).The node pool name is resolved at runtime from config (the first Karpenter node pool with
session_taints=true); nothing is hardcoded. This is AWS-only, inbuildAWSSiteSpec, which is correct since session taints are a Karpenter/AWS feature.Pairs with a team-operator change adding
contentTolerationsandcontentNodeSelectorfields to the Connect site spec; the JSON key names here match those CRD tags exactly.Changes
lib/steps/sites.go: newsessionPoolNamehelper (mirrorssessionTolerations);buildAWSSiteSpecnow emitsspec.connect.contentTolerations/contentNodeSelectorwhen a session-tainted pool exists.lib/steps/sites_test.go: unit tests forsessionPoolNameand for the new connect block (present with session taints, absent without).Testing
just test-libpasses.Companion PR: posit-dev/team-operator#150 — adds the
contentTolerations/contentNodeSelectorCRD fields this PR populates. These two PRs go together; merge team-operator#150 first.