Repository navigation
feat(charts): add nodeSelector / tolerations / affinity support to workload charts - #321
Conversation
…harts
Wires the three standard pod-scheduling knobs into every Deployment and
StatefulSet pod spec across 24 charts (34 workload templates), with empty
defaults in values.yaml so unset renders nothing and existing releases are
unaffected. This makes dedicated / tainted node-pool placement expressible
through Helm instead of an out-of-band kubectl patch that a helm upgrade
silently erases.
Two fixes were needed for affinity to work at all:
- kafka and zookeeper already read .Values.affinity but rendered it as
`{{ toYaml ... | indent 8 }}` on an already-indented line, double-indenting
the block. Setting affinity produced invalid YAML and helm template failed.
Switched to nindent.
- cockroachdb, dgraph and mariadb hardcoded a podAntiAffinity with no
override. Wrapped it in the same if/else kafka and zookeeper use, so
affinity replaces it. Their READMEs note the replace-not-merge behaviour.
uptime-monitoring is the only chart with additionalProperties: false, so its
values.schema.json needed the three properties or installs failed validation.
Subcharts are scheduled separately: kafka->zookeeper, litellm->postgres,
localai->postgres/nats, opentsdb->zookeeper and superset->postgres/redis pull
pinned published versions, so their pods stay unpinnable until those
dependency versions are bumped. Documented in each README.
mosquitto was never published at v0.0.1, so it is released at v0.0.1 rather
than bumped to v0.0.2.
Verified with helm lint (24/24), a defaults render diffed against main
(byte-identical apart from generated passwords), and a structural check that
all 34 pod specs deep-equal the supplied scheduling values.
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Review — PR #321, nodeSelector / tolerations / affinity across 24 workload charts
Reviewed on a worktree of review/pr-321 against origin/main.
What I verified, and what I only read. Verified by running: helm dependency update + helm lint
on all 24 charts (24/24 clean, 0 failures); rendering all 24 at defaults and with all three keys set,
parsing the YAML and asserting each pod spec; verify-index.py against origin/main; every subchart
pass-through claim below; and a cluster test on local minikube — cockroachdb and postgres
installed onto a node labelled workload=stateful and tainted workload=stateful:NoSchedule, which
is the exact configuration the feature is for. The first finding comes from that install, not from reading.
The change is well built. Defaults render nothing, so existing releases are untouched; the
kafka / zookeeper indent 8 → nindent 8 fix is real (the old form double-indented and made
affinity unrenderable); the if/else wrapping of the hardcoded podAntiAffinity in cockroachdb,
dgraph and mariadb is applied consistently, and the "setting affinity replaces the default
anti-affinity" caveat appears in exactly those five READMEs and nowhere else — which is correct.
docs/index.yaml was genuinely regenerated (0/183 created: timestamps carried over), 24 added,
0 removed, 0 changed. No committed subchart tarballs, no tabs, branch name compliant.
Nothing fails CI. But the first finding below produces a release that reports deployed and is silently
broken, so I'd take that one before merge; the rest are about the feature's reach rather than its
correctness.
1. [Should fix — verified on a cluster] A chart's own init Jobs were skipped, and the release then deploys silently broken
charts/cockroachdb/templates/job.init.yamlcharts/postgres/templates/init-script-config-map.yaml(the db-init Job)charts/scylladb/templates/user-job.yamlcharts/superset/templates/init-job.yaml
The StatefulSets in these charts take the scheduling block; the Jobs sitting beside them in the same
chart do not. That is the whole bug: a toleration exists precisely so a pod can land on a tainted
pool, so a workload where some of the pods get the toleration is worse than one where none do — the
release comes up looking healthy and is not.
Installed on minikube with the node labelled workload=stateful and tainted
workload=stateful:NoSchedule, values setting nodeSelector + tolerations to match.
cockroachdb — the StatefulSet honours both keys and schedules; the init Job cannot:
r321-cockroachdb-0 0/1 Running 0 restarts 5m57s
r321-cockroachdb-1 0/1 Running 0 restarts 5m57s
r321-cockroachdb-2 0/1 Running 0 restarts 5m57s
r321-cockroachdb-init-jpd8h 0/1 Pending 0 restarts 5m57s
FailedScheduling: 0/1 nodes are available: 1 node(s) had untolerated taint {workload: stateful}
Unhealthy (x62): Readiness probe failed: HTTP probe failed with statuscode: 503
helm install reported STATUS: deployed. Six minutes later no pod had ever become Ready and none
ever would: cockroach init lives in that Job, so the cluster is never initialised. Zero restarts —
nothing crash-loops, nothing logs an error, the release just never works.
postgres — same shape, and the damage is quieter:
pgt-postgres-0 2/2 Running 0 restarts
postgres-pgt-app-init-02b8899b-vsx6s 0/1 Pending 0 restarts
$ psql -U postgres -tAc "SELECT datname FROM pg_database"
postgres template0 template1 # 'appdb' was never created
Postgres reports 2/2 Running, helm reports deployed, and the database the Job exists to create is
simply absent. Whatever consumes it fails later, far from the cause.
One correction to what the PR body implies about --wait: plain --wait does not cover Jobs, which
is why the install above returned success. It is --wait-for-jobs that would block — and the comment
in init-script-config-map.yaml:80 anticipates exactly that ("--wait-for-jobs would otherwise sit
on it"), where the Job would now hang to its activeDeadlineSeconds: 1800.
Extending the same {{- with }} block to those four Job pod templates closes it. The PR body
enumerates the subchart gap but not this one.
2. [Should fix] 22 of 24 charts add three top-level values keys with no values.schema.json entry
charts/<name>/values.schema.json — every touched chart except uptime-monitoring (and zookeeper,
which has no schema at all).
values.yaml gains nodeSelector, tolerations and affinity, but no schema property describes
them. In this repo the schema is not only validation — it drives the zop.dev configuration form, so a
top-level key with no property is both unvalidated and invisible in the UI. The result is that
the feature this PR exists to deliver is reachable only through -f values.yaml, not through the
product surface where most users would set it.
The tell that this was an omission rather than a decision: uptime-monitoring did get the three
properties — because it is the one chart with additionalProperties: false at the root, so skipping
them would have failed schema validation. The lint gate caught the one chart it could see.
Fix — add to each schema, following charts/litellm/values.schema.json (storeModelInDB) for style:
"nodeSelector": {
"category": "advanced",
"type": "object",
"description": "Node labels the pod must match, for pinning the workload to a node pool",
"default": {},
"mutable": true
},
"tolerations": { "category": "advanced", "type": "array", "description": "...", "default": [], "mutable": true },
"affinity": { "category": "advanced", "type": "object", "description": "...", "default": {}, "mutable": true }advanced is the right category here (it is where postgres / externalDatabase subchart blocks
live), and it is one of the four legal values — no new category is needed.
3. [Should fix] The three properties that were added omit description, default and mutable
charts/uptime-monitoring/values.schema.json:66-80
"nodeSelector": { "category": "advanced", "type": "object" },Every sibling property in that same file carries description and default; 28 of the repo's 29
schemas use mutable. verify-values.py flags it:
[should] 3 leaf field(s) without description: nodeSelector, tolerations, affinity
Without mutable: true, CONTRIBUTING's "user-editable fields marked mutable" is unmet and the field
may render read-only in the config form — the same outcome as finding 2, by a different route.
(The [BLOCKING] root has "additionalProperties": false that the same script reports on this chart is
pre-existing on main, not yours.)
4. [Should fix] The README's subchart advice tells the reader to do something that silently does nothing
charts/kafka/README.md, charts/opentsdb/README.md, charts/superset/README.md,
charts/localai/README.md, charts/litellm/README.md — the Pod Scheduling section:
Bundled subcharts (zookeeper) are scheduled separately — set the same keys under their own value
prefix if they should follow the parent.
The second half is not true at the pinned dependency versions. Every one of those subcharts is pinned
to a published version that predates this feature, so the values are accepted and ignored:
$ helm template t charts/kafka --set zookeeper.nodeSelector.workload=stateful
t-zookeeper nodeSelector = None # zookeeper pinned 0.0.1; support lands in v0.0.3, released by this PR
Same result, verified individually, for litellm→postgres 0.0.14, superset→postgres 0.0.12 and
redis 0.0.1, opentsdb→zookeeper v0.0.2, and localai→nats 2.14.4 (upstream nats uses a different
values path, so the advice misses there too — all report nodeSelector = None).
The PR description states this correctly — "those pods stay unpinnable until the dependency versions
are bumped". The READMEs say the opposite to the person actually reading them, and a user following
that line gets a silent no-op with no error to explain it. Reword to say the subchart pods cannot
currently be pinned, and name the version the dependency must reach.
5. [Note] mosquitto is published for the first time here, and it is missing its listing metadata
docs/mosquitto-v0.0.1.tgz is an add — mosquitto does not appear in origin/main's
docs/index.yaml. So this PR, whose subject is scheduling, is also mosquitto's first release. That
is fine in itself (no live install can break; the version correctly stays 0.0.1), but the chart
arrives on helm.zop.dev without the metadata that makes it visible:
charts/mosquitto/Chart.yamlhas noicon:— it will render logo-less- it has no
annotations.type: application|datasource— it lands in neither zop.dev listing - it is absent from the Available Charts table in the root
README.md
None of this is caused by your change, and only the last is an unwritten convention (CONTRIBUTING's
six steps stop at the index). But this PR is what makes the chart public, so it is the moment to
either add the three things or hold mosquitto's tarball back for its own PR.
Also checked and clean: no duplicate keys in any rendered pod spec; outline / wordpress pin their
service and mysql dependencies, so the service v0.0.33 bump does not cascade into them; all 24
READMEs document the new values; all 24 values.yaml carry all three keys; no hard tabs; no
committed subchart tarballs. The Chromadb-v0.0.1.tgz URL-casing warning from verify-index.py is
pre-existing on main.
On the cluster test I also confirmed the feature does what it claims where it is wired: the
cockroachdb and postgres StatefulSet pods carried exactly the supplied nodeSelector and
tolerations and scheduled onto the tainted node, which they could not have done before this PR.
1. Init Jobs took no scheduling block. The StatefulSets in cockroachdb, postgres, scylladb and superset honoured nodeSelector/tolerations while the Jobs beside them did not, so on a tainted pool the Job stayed Pending and the release reported `deployed` while never working: cockroachdb never ran `cockroach init`, postgres never created the database. Extended the same guarded block to all four Job pod templates. The postgres Job sits inside a range over .Values.services, so it reads $.Values. 2. 22 charts gained the three top-level values keys with no schema property, leaving them unvalidated and invisible in the zop.dev config form. Added nodeSelector/tolerations/affinity to every touched values.schema.json under category "advanced" (zookeeper has no schema and is unchanged). 3. The three properties already added to uptime-monitoring were bare stubs. All 23 now carry description, default and mutable: true, matching the style of their sibling properties. 4. The README subchart advice told the reader to set the keys under the subchart prefix, which is a silent no-op at the pinned dependency versions. Reworded to say those pods cannot currently be pinned, naming the version each dependency must reach. 5. mosquitto is first published by this PR, so it gets the listing metadata it was missing: annotations.type, an icon, and a row in the root README. The icon is the generic zop.dev logo, as scylladb uses — a real logo should replace it. Charts repackaged and docs/index.yaml regenerated. Verified: helm lint 24/24; 38/38 pod specs (34 workloads + 4 Jobs) deep-equal the supplied scheduling values; defaults still render no scheduling block and diff clean against main apart from chart-version labels; 0 index entries lost, no digest drift.
|
Thanks @arunesh-j — the cluster test caught something template-level checks could not have, and finding 1 was a real bug. All five addressed in 35ce411. 1. Init Jobs — fixedYou're right that this is worse than not supporting scheduling at all: a partially-tolerating release reports
The postgres Job sits inside a I swept for others: I also extended the verifier to Jobs and CronJobs. It now asserts 38 pod specs (34 workloads + 4 Jobs) deep-equal the supplied values — 38/38 pass, and reverting any one template makes it report Taking the correction on 2 & 3. Schema entries — fixedAdded You diagnosed my reasoning correctly: I'd taken For the affinity 4. README subchart advice — fixedReworded in all five to say those pods cannot currently be pinned and to name the target version, e.g. kafka:
Same for litellm→postgres I've also added a line to the cockroachdb / postgres / scylladb / superset READMEs noting the init Job is covered, with the failure mode from finding 1. 5. mosquitto — metadata addedAdded One thing needs your call: the icon is Re-verificationCharts repackaged and
|
Removes the icon, annotations.type and root-README row. mosquitto keeps the scheduling change that is this PR's subject, and is still packaged at v0.0.1. Repackaged and reindexed so docs/mosquitto-v0.0.1.tgz matches source again.
arunesh-j
left a comment
arunesh-j
left a comment
There was a problem hiding this comment.
Approving — findings 1–4 are fixed, and I re-verified each on 7d5a721 rather than taking the reply
for it.
Finding 1, re-tested on the same cluster scenario that failed before (node labelled
workload=stateful, tainted workload=stateful:NoSchedule, all three keys set):
r321r2-cockroachdb-0 1/1 Running 0 restarts 20s
r321r2-cockroachdb-1 1/1 Running 0 restarts 20s
r321r2-cockroachdb-2 1/1 Running 0 restarts 20s
r321r2-cockroachdb-init-z8rpb 0/1 Completed 0 restarts 20s
Ready in 20 seconds, against three pods that never became Ready at all last time. postgres with
--wait --wait-for-jobs — the flag that would actually have hung on the old code — completes, and
appdb is now present where it was absent:
job.batch/postgres-pgt3-app-init-9f90b0d2 Complete 1/1 24s
$ psql -U postgres -tAc "SELECT datname FROM pg_database"
appdb postgres template0 template1
The Job pod carries all three keys including affinity, and $.Values inside the range renders
correctly — that was the subtle one. My own sweep for kind: Job|CronJob across the 24 charts in
scope returns exactly your four templates, all four fixed.
Findings 2 & 3: all 69 new schema properties checked mechanically — category: advanced on every
one (no fifth category), each with type, description, default and mutable: true, defaults
matching the shipped values. The affinity caveat variant is on exactly the four anti-affinity charts
that have a schema. The verify-values.py blocking items on postgres/service/cockroachdb/
uptime-monitoring are pre-existing — identical counts on origin/main, none introduced here.
Finding 4: all five READMEs now say the subchart pods cannot currently be pinned and name the
target version; the old wording is gone from the tree entirely.
Also re-verified: helm lint 24/24 clean; 32/32 own-chart pod specs carry all three keys; docs/index.yaml
regenerated with 24 added, 0 removed, 0 changed; and all 24 published tarballs byte-match their
source templates/, values.yaml and values.schema.json — the fixes are in the .tgz files, not
just the tree.
Two things to pick up, neither blocking:
-
charts/scylladb/README.mdis the one of the four that did not get the init-Job line — cockroachdb,
postgres and superset have it.user-job.yamldid get the fix, so this is docs only. -
7d5a721reverted finding 5, so mosquitto ships from this PR still withouticon:,
withoutannotations.typeand without its root-README row. Noting it for the record rather than
holding the PR: to answer the question in your comment,https://zop.dev/logo.pngis the right
icon and what I'd have picked — it's the sanctioned fallback thatscylladb,zookeeperand
uptime-monitoringall use, and a real logo is a bucket upload away later.
2 participants
Closes #320
Description
None of the workload charts exposed pod-scheduling controls, so a chart-managed workload could not be placed on a dedicated / tainted node pool through Helm. Pods that had to land on such a pool were pinned out-of-band (
kubectl patch), which ahelm upgradesilently erases by re-rendering the pod template without the scheduling block.This wires
nodeSelector,tolerationsandaffinityinto every Deployment and StatefulSet pod spec — 24 charts, 34 workload templates — with empty defaults in eachvalues.yaml, using the usual guarded blocks so an unset value renders nothing.Charts covered:
cassandra,chromadb,clickhouse,cockroachdb,dgraph(zero + alpha),holmesgpt,kafka,litellm,localai(standalone + frontend + worker),mariadb(master + slave),mosquitto,mysql,opentsdb,postgres(primary + slaves),qdrant,redis,redisdistributed(master + slave),scylladb,service,solr,superset(node/worker/beat/flower),surrealdb,uptime-monitoring(prometheus + blackbox),zookeeper.Two fixes needed for
affinityto work at allkafkaandzookeeperhad a broken.Values.affinity. Both already read it, but rendered it as{{ toYaml ... | indent 8 }}on a line that was already indented 8 spaces — double-indenting the block. Settingaffinityproduced invalid YAML andhelm templatefailed outright. Switched to{{- ... | nindent 8 }}.cockroachdb,dgraphandmariadbhardcoded apodAntiAffinitywith no way to override it. Wrapped it in the sameif/elsepatternkafka/zookeeperuse, soaffinityreplaces it. Those five READMEs call out that settingaffinityreplaces rather than merges with the chart's default anti-affinity.Other notes
uptime-monitoringis the only chart withadditionalProperties: false, so itsvalues.schema.jsonneeded the three properties added or installs failed schema validation.kafka→zookeeper,litellm→postgres,localai→postgres/nats,opentsdb→zookeeper andsuperset→postgres/redis pull pinned published dependency versions, so those pods stay unpinnable until the dependency versions are bumped to the ones released here. Documented in each README — flagging it as the one part of the issue not fully closed by this PR alone.mosquittowas never published atv0.0.1, so it is released atv0.0.1rather than bumped tov0.0.2.jupyterhub,karpenter-gcp,solr-operator,zookeeper-operator(vendored upstream charts);openobserve-standalonealready had all three keys.Type of Change
kafka/zookeeperaffinity rendered invalid YAML)Verification
No real cluster was used — everything below is template-level.
helm lint— 24/24 charts clean.origin/mainworktree. Output is byte-identical apart from randomly generated passwords. None of the three keys appears anywhere with defaults, so existing releases are unaffected.nodeSelector/tolerations/affinitydeep-equal the supplied values, enablingreplication.enabled,distributed.enabledandsupersetCeleryBeat/Flowerto reach the conditional workloads. Result: 34 ok, 0 failed.MISMATCH ... nodeSelector=None, confirming it pins the boundary rather than passing vacuously.docs/*.tgzplus a regenerateddocs/index.yaml. Verified 0 previously-published versions lost and no digest or URL drift on existing entries; the only change to old entries is thecreatedtimestamp, inherent tohelm repo indexregeneration. Packaged tarballs were re-rendered to confirm they aren't stale.Checklist
helm lintpasses without errorsdocs/anddocs/index.yamlregenerated