Skip to content

feat(nodeset): roll pods when mounted Secrets/ConfigMaps change - #204

Open
giuliocalzo wants to merge 1 commit into
SlinkyProject:mainfrom
giuliocalzo:feat/nodeset-config-rollout
Open

feat(nodeset): roll pods when mounted Secrets/ConfigMaps change#204
giuliocalzo wants to merge 1 commit into
SlinkyProject:mainfrom
giuliocalzo:feat/nodeset-config-rollout

Conversation

@giuliocalzo

Copy link
Copy Markdown
Contributor

Summary

Adds opt-in tracking of the Secrets and ConfigMaps mounted by a NodeSet's worker pods so that a content change automatically rolls the pods and records an event.

  • Opt in per-NodeSet via the annotation slinky.slurm.net/reload-on-change: "true".
  • Tracks Secrets/ConfigMaps referenced through volumes (incl. projected sources), envFrom, and env[].valueFrom.
  • Computes a per-resource checksum (key-aware: hashes both keys and values) and stamps it on the in-memory pod template (spec.template.metadata.annotations) during reconcile, before revision computation. Because getPatch folds spec.template into the ControllerRevision, a changed checksum yields a new revision and triggers the existing Slurm-aware rolling update (drain + maxUnavailable + updateStrategy).
  • Adds a ConfigMap watch, extends the Secret watch, and adds a NodeSet field index so a changed resource enqueues only the opted-in NodeSets that mount it.
  • Missing resources resolve to a deterministic sentinel; transient read errors requeue without creating spurious revisions.

Why

NodeSet worker pods routinely mount configuration and credentials from Secrets/ConfigMaps, but Kubernetes does not roll a workload when the content of a mounted Secret/ConfigMap changes — only when the pod template itself changes. For most workloads a kubelet-side projected-volume refresh is enough, but Slurm workers are different: many of these files are read once at container start (or by an init hook), so a live update silently diverges from what the running slurmd actually loaded. Operators are then forced to manually delete pods to pick up the new config, which bypasses Slurm-aware draining.

Concrete production scenarios this addresses:

  • Container runtime / enroot + hooks.d config. A cluster ships its enroot configuration and a set of hooks.d scripts (e.g. enroot/pyxis hooks, NCCL/topology tuning, mount setup) via a mounted ConfigMap. These are consumed at container/job startup, so updating the ConfigMap has no effect on already-running workers. With opt-in tracking, editing the hooks.d ConfigMap rolls the NodeSet through a proper Slurm drain so every worker comes up with the new hooks.
  • Credentials for internal enterprise services. Workers often mount Secrets to authenticate to internal services — private container/artifact registries, package mirrors, object storage, a license server, or an enterprise API. When such a Secret is rotated, the old credentials stay live in the running pods until they are recreated, causing auth failures or stale tokens. Tracking the Secret triggers an automatic, controlled rollout on rotation.
  • Multiple mounts on one NodeSet. A single worker template commonly mounts several of these at once (enroot config + hooks + a registry pull-credential Secret + an internal-CA bundle). The per-resource checksums and the <kind>/<name>-keyed status.configHashes map make it clear which resource changed, and the ConfigHashChanged event gives operators an audit trail of what triggered each rollout.

The behavior is opt-in per NodeSet precisely because rolling on every config change is not always desired; clusters enable it only for the workers whose mounted config must be consistent with what slurmd loaded.

Change detection & events

  • The observed checksums are persisted in a new status.configHashes map keyed by <kind>/<name> (e.g. configmap/my-config) on the NodeSet. The full slinky.slurm.net/... annotation key is used only for the pod-template stamp.
  • On each reconcile, applyConfigHashes compares the freshly computed checksums against status.configHashes and emits a Normal ConfigHashChanged event against the NodeSet when a tracked resource's content differs. syncNodeSetStatus refreshes the map from the checksums stamped on the pod template.
  • No event fires on first observation (creation/opt-in), only on subsequent changes.
  • The status map is the single source of truth for "previously observed" hashes — no ControllerRevision reads and no watch-time old/new diffing. The Secret/ConfigMap watch handlers are enqueue-only; they just trigger a reconcile so the controller can recompute and compare against persisted state.

Implementation notes

  • New package internal/utils/confighash (reference discovery, length-safe annotation keys, key-aware hashing).
  • New status.configHashes field on NodeSetStatus (CRDs + deepcopy regenerated).
  • applyConfigHashes reconciler step wired into Sync; status persistence in syncNodeSetStatus.
  • Operator-injected resources (Slurm auth key, SSH config) are intentionally out of scope; they are handled separately.

Test Plan

  • go build ./..., go vet ./internal/... ./api/...
  • Unit tests: internal/utils/confighash, .../nodeset, .../nodeset/indexes, .../nodeset/eventhandler (event on status mismatch, no event when status matches, no event on first observation)
  • Manual: rebuilt operator into kind, edited a mounted ConfigMap/Secret on an opted-in NodeSet, observed a new ControllerRevision and a ConfigHashChanged event
  • Live verification: go test -tags configrollout ./test/configrollout/ against a kind cluster (Go port of the former hack/verify-config-rollout.sh)

Adds opt-in tracking of the Secrets and ConfigMaps mounted by a NodeSet's
worker pods so a content change automatically rolls the pods and records a
ConfigHashChanged event.

- Opt in per-NodeSet via the "slinky.slurm.net/reload-on-change" annotation.
- Discovers Secrets/ConfigMaps referenced via volumes (incl. projected
  sources), envFrom, and env[].valueFrom (new internal/utils/confighash).
- Computes a key-aware checksum per resource and stamps it on the pod
  template before revision computation, so a change yields a new
  ControllerRevision and the existing Slurm-aware rolling update.
- Persists the observed checksums in a new status.configHashes map keyed by
  "<kind>/<name>"; the reconciler compares freshly computed checksums against
  it and emits a ConfigHashChanged event on change. No event fires on first
  observation. The status map is the sole source of truth for change
  detection (no ControllerRevision reads, no watch-time diffing).
- Watches ConfigMaps, extends the Secret watch, and indexes opted-in
  NodeSets by mounted config refs so a changed resource enqueues only the
  NodeSets that mount it.
- Missing resources resolve to a deterministic sentinel; transient read
  errors requeue without creating spurious revisions.
- Adds a live-cluster verification test (build-tagged test/configrollout,
  "make test-config-rollout") that asserts on status.configHashes, a new
  ControllerRevision, and the ConfigHashChanged event.
@vivian-hafener

Copy link
Copy Markdown
Contributor

Good afternoon @giuliocalzo,

I'm reading through this PR now. Out of curiosity, why did you use an annotation to contain this configuration value instead of the NodeSet CRD itself? In my opinion, a NodeSet CRD field would be more consistent with the rest of our NodeSet configuration pattern.

Best,
Vivian Hafener

@giuliocalzo

Copy link
Copy Markdown
Contributor Author

Good afternoon @giuliocalzo,

I'm reading through this PR now. Out of curiosity, why did you use an annotation to contain this configuration value instead of the NodeSet CRD itself? In my opinion, a NodeSet CRD field would be more consistent with the rest of our NodeSet configuration pattern.

Best, Vivian Hafener

hi @vivian-hafener I did not want to overload the CRD of flags/parameters, using an annotation feels more correct as opt-in feature, just let me know I can convert as CRD parameter if you think is better

@vivian-hafener

Copy link
Copy Markdown
Contributor

I think that it would make the most sense to use a tool like Reloader for this. I've reached out to the Reloader team to see if they would be open to a contribution that would enable that tool to reload NodeSet pods: stakater/Reloader#1192

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