feat(helm): worker concurrency and extraArgs support#16
Conversation
…ne worker Provider workers had a hardcoded arg list with no way to pass Celery worker options, forcing downstream overrides to replace the entire args array. Add a first-class `concurrency` value rendered as `--concurrency=<n>` and a free-form `extraArgs` list, both appended after the Celery `--` separator. Pin the esmvaltool worker to `concurrency: 1` so its Dask fan-out does not OOM-kill the node.
📝 WalkthroughWalkthroughAdds Helm chart support for configuring Celery worker ChangesWorker concurrency/extraArgs configuration
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 023137c2-25fe-4be8-b2d1-c65d5f2eb782
📒 Files selected for processing (3)
changelog/16.feature.mdhelm/templates/providers/deployment.yamlhelm/values.yaml
| concurrency: null | ||
|
|
||
| # Additional arguments appended to the Celery worker after the `--` separator, | ||
| # for options not exposed as first-class values (e.g. ["--pool=solo"]). | ||
| extraArgs: [] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify provider overrides render after defaults define the same keys.
set -eu
if ! command -v helm >/dev/null 2>&1; then
echo "helm is not installed; run this locally in an environment with Helm."
exit 0
fi
helm template verify ./helm \
--set providers.esmvaltool.concurrency=1 \
--set 'providers.esmvaltool.extraArgs[0]=--pool=solo' |
awk '
/name: .*esmvaltool/ { in_esm=1 }
in_esm && /args:/ { in_args=1 }
in_args { print }
in_args && /envFrom:/ { exit }
'
# Expect the esmvaltool container args to include:
# - --
# - --concurrency=1
# - --pool=soloRepository: Climate-REF/climate-ref-aft
Length of output: 234
🏁 Script executed:
#!/bin/bash
set -eu
printf '\n## helm/values.yaml (around the changed lines)\n'
sed -n '160,210p' helm/values.yaml
printf '\n## helm/templates/providers/deployment.yaml\n'
sed -n '1,240p' helm/templates/providers/deployment.yaml
printf '\n## any other merge/mergeOverwrite usage in helm/\n'
rg -n '\bmergeOverwrite\b|\bmerge\b' helm -g '*.yaml' -g '*.tpl' -g '*.yml'Repository: Climate-REF/climate-ref-aft
Length of output: 6153
🌐 Web query:
Sprig merge mergeOverwrite precedence destination precedence Helm docs
💡 Result:
In Helm and the underlying Sprig library, the merge and mergeOverwrite functions handle dictionary precedence differently [1][2]. Both functions perform a deep merge, meaning they recursively merge nested dictionaries, but they do not perform a deep copy; nested objects remain shared instances across the merged dictionaries [1][3]. merge The merge function gives precedence to the destination (the first) dictionary [1][4]. When merging multiple dictionaries, if a key exists in both the destination dictionary and the source dictionaries, the value in the destination dictionary is preserved [1][2]. mergeOverwrite The mergeOverwrite function gives precedence from right to left [1][4]. In this operation, values in the source dictionaries overwrite values in the destination dictionary [1][3]. This is particularly useful for overriding default configurations, as it ensures that values provided in later arguments take priority over those in earlier ones [5]. Key Differences in Behavior A notable quirk often encountered is that merge may not overwrite boolean fields set to true in the destination dictionary, whereas mergeOverwrite correctly overwrites these values [5][6]. Because these functions mutate the destination dictionary (the first argument), it is common practice to pass an empty dictionary (dict) as the first argument or to use deepCopy to avoid unintended side effects on source objects [5][7]. Example Usage To merge with overwrite precedence: {{ $newdict:= mergeOverwrite (dict) $defaultValues $userOverrides }} To ensure a deep copy during the merge: {{ $newdict:= mergeOverwrite (dict) (deepCopy $defaultValues) $userOverrides }}
Citations:
- 1: https://helm.sh/docs/chart_template_guide/function_list
- 2: http://masterminds.github.io/sprig/dicts.html
- 3: https://github.com/Masterminds/sprig/blob/bd7cb37b66a3bf86016d09188a371c36e5a63f21/docs/dicts.md
- 4: https://github.com/helm/helm-www/blob/main/docs/chart_template_guide/function_list.mdx
- 5: https://blog.nuvotex.de/helms-hidden-quirks-merging-boolean-values/
- 6: Bug? Helm
mergeORs boolean values helm/helm#13309 - 7: Helm
mergemutates the leftmost dict helm/helm#13308
Switch this merge to right-precedence.
merge (deepCopy $.Values.defaults) $spec keeps the new default concurrency: null and extraArgs: [] values, so provider overrides like providers.esmvaltool.concurrency=1 and extraArgs never make it into the worker args. Use mergeOverwrite here so provider-specific settings win.
1 participant
Summary
Provider workers were rendered with a hardcoded
celery start-workerarg list with no extension point, so tuning a worker (e.g. pinning Celery concurrency) required a downstreampostRendererskustomize patch that replaced the entireargsarray — fragile, since it silently freezes a copy of the base args and misses any future chart changes.This adds two chart values, both rendered as Celery worker options after the
--separator:concurrency(first-class) →--concurrency=<n>extraArgs(free-form list) → concatenated verbatim, for options not exposed as first-class valuesThe
--separator is only emitted when at least one worker option is set, so providers with neither render exactly as before.It also sets
providers.esmvaltool.concurrency: 1as a chart default: each esmvaltool execution already fans out through its own Dask cluster, so running multiple Celery workers per pod multiplies the memory footprint and OOM-kills the node. This upstreams a fix previously carried as a home-infra postRenderer patch, which can now be removed.Rendered output
--provider esmvaltool -- --concurrency=1concurrency: 4)--provider pmp -- --concurrency=4extraArgs: [--pool=solo])--provider ilamb -- --pool=solo--)Testing
helm lintpasses.helm templateverified for all four cases above (first-class, free-form, combined, and unset).towncrier build --draftrenders the changelog fragment.Summary by CodeRabbit
--separator where applicable.esmvaltoolworker to run with a single process per pod to help prevent out-of-memory issues.