Skip to content

feat: allow Connect content pods to run on tainted session nodes#150

Open
amdove wants to merge 2 commits into
mainfrom
connect-content-node-placement
Open

feat: allow Connect content pods to run on tainted session nodes#150
amdove wants to merge 2 commits into
mainfrom
connect-content-node-placement

Conversation

@amdove

@amdove amdove commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

Description

Extends the existing Workbench session toleration/placement capability to Posit Connect content pods. Connect content (off-host execution) pods can now be both permitted onto (via tolerations) and directed to (via a node selector) tainted Karpenter session nodes, exactly mirroring how Workbench session pods already work.

Two new optional fields are added to the Connect site spec:

  • contentTolerations — tolerations applied only to Connect content (session) pods, not the Connect server.
  • contentNodeSelector — a node selector applied only to Connect content (session) pods, not the Connect server.

This directly parallels the existing Workbench sessionTolerations mechanism, but for Connect's content pods.

Code Flow

  • api/core/v1beta1/site_types.go: adds ContentTolerations and ContentNodeSelector to InternalConnectSpec.
  • internal/controller/core/site_controller_connect.go: wires those fields into the Connect SessionConfig.Pod (product.PodConfig) as Tolerations and NodeSelector, mirroring site_controller_workbench.go which sets session Tolerations/NodeSelector for Workbench session pods.
  • No template changes are needed: api/templates/*/job.tpl already renders pod.tolerations and pod.nodeSelector.
  • Generated CRDs, deepcopy, client-go applyconfig, and the Helm chart CRD were regenerated via just mgenerate / the Helm chart generation hook.

Category of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have run the test suite (with envtest) and all tests pass
  • I have reviewed my own diff

Companion PR: posit-dev/ptd#356 — emits the spec.connect.contentTolerations / contentNodeSelector values consumed by the fields added here. These two PRs go together; merge this one first.

Extends the existing Workbench session toleration/placement mechanism to
Connect content (off-host execution) pods via new contentTolerations and
contentNodeSelector fields on the Connect site spec. These are applied only
to content pods, not the Connect server, mirroring how Workbench session
tolerations work.
@amdove
amdove marked this pull request as ready for review July 20, 2026 21:53
@claude

claude Bot commented Jul 20, 2026

Copy link
Copy Markdown

Claude finished @amdove's task in 1m 41s —— View job


Review

  • Gather context
  • Read review guidelines
  • Read changed files
  • Post review

The implementation is clean and the pattern is correct. A few observations:

Missing tests: TestSiteReconcileWithTolerations at internal/controller/core/site_test.go:466 covers the analogous Workbench SessionTolerations case — there's no equivalent test for ContentTolerations/ContentNodeSelector. Given the Workbench precedent exists, worth adding a parallel test for Connect content pods.

Asymmetry in Connect spec: Workbench has both a universal NodeSelector/Tolerations (server + sessions) and session-only SessionTolerations. Connect gains ContentNodeSelector/ContentTolerations for content pods, but its existing server-level NodeSelector (line 285 in site_types.go) has no Tolerations counterpart. This means Connect server pods can't be placed on tainted nodes. Left an inline comment on this — fine to defer if not currently needed, but worth a conscious decision.

Everything else looks good: the wiring at site_controller_connect.go:66-67 correctly mirrors the Workbench pattern at line 241, generated code (deepcopy, applyconfiguration, CRDs) is consistent, and the three CRD copies (config/, dist/chart/, internal/crdapply/) are all in sync.

@@ -284,6 +284,12 @@ type InternalConnectSpec struct {

NodeSelector map[string]string `json:"nodeSelector,omitempty"`

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Suggestion: The Workbench spec differentiates between server-level NodeSelector/Tolerations (applied universally) and session-only ones. For Connect, NodeSelector at this line applies to the server pod (line 148 in the controller), but there's no server-level Tolerations equivalent. If Connect server pods need to run on tainted nodes (e.g., to co-locate with Karpenter-provisioned infra), there's currently no way to do that. Worth noting in docs or adding a Tolerations field to mirror Workbench's server-level one — but fine to defer if not needed yet.

@amdove

amdove commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Thanks for the review. Addressed both points:

  • Missing tests — added TestSiteReconcileWithContentPlacement (commit 4022345), mirroring TestSiteReconcileWithTolerations: it sets Connect.ContentTolerations + ContentNodeSelector on the Site and asserts the child Connect CR's SessionConfig.Pod.Tolerations/NodeSelector are populated. just mtest passes.
  • Server-level Tolerations asymmetry — deferring intentionally. This change deliberately mirrors Workbench's session-only tolerations for Connect content pods; placing the Connect server on tainted nodes is out of scope here and not currently needed. Easy follow-up to add a server-level Tolerations field mirroring Workbench if a use case arises.

@Lytol Lytol left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Awesome! Same as my comment on the ptd side: we should probably do some end-to-end verification on internal infra before merging.

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.

2 participants