Skip to content

feat(charts): expose extra* values and podSpec/container patches on the plane charts - #314

Open
lemaitre-aneo wants to merge 1 commit into
feat/revamp-helmfrom
fl/helm-spec-patch
Open

lemaitre-aneo wants to merge 1 commit into
feat/revamp-helmfrom
fl/helm-spec-patch

Conversation

@lemaitre-aneo

@lemaitre-aneo lemaitre-aneo commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Motivation

The plane charts enumerate pod and container fields one by one, so anything not enumerated is unreachable: a topologySpreadConstraint, a hostAlias, a sidecar, a securityContext the chart got wrong. Every such need is currently a PR against this repo and a wait for a release.

Description

Three mechanisms, with one rule dividing them:

extra* owns lists the chart builds. *Patch owns scalars and maps. Anything else is a post-renderer.

extra* (additive): extraEnv, extraEnvFrom, extraVolumes, extraVolumeMounts, extraContainers, extraInitContainers. Appended to what the chart builds, never replacing it. Extras go last so container 0 stays the chart's own, which is what kubectl logs picks by default.

podSpecPatch / containerPatch (overriding): a map merged over the rendered fragment, last and winning. Names mirror the API types (you patch a PodSpec, and a Container, which has no ContainerSpec), matching Argo Workflows' podSpecPatch. On the compute plane they sit on partitionCommon, so per-partition override works through the existing merge with no new machinery.

podSpecPatch targets spec.template.spec, never spec.template: pod labels have to stay a superset of the immutable spec.selector.matchLabels.

Three limits follow from armonik.utils.merge, the first enforced by the new armonik.utils.patch:

  • A list replaces, it does not merge by key. A patch may introduce a list key the fragment does not build, but replacing one it does is a render error naming the additive value instead. The check reads the fragment rather than a hand-kept deny list, so a list is protected the day the chart starts building it. command and args are exempt.
  • A patch cannot delete a key, null and "" being absent to the merge.
  • Anything beyond this is helm --post-renderer or ArgoCD's kustomize post-render, which give real strategic-merge semantics. Documented in charts/best-practices.md rather than reimplemented.

armonik.utils.patch parses what it patches and printed text cannot be patched, so each patchable object moved into a define of literal YAML in a new _deployment.tpl, leaving the Deployment envelope in place. armonik-compute-plane/templates/deployment.yaml goes from 215 lines to 88.

Fixes carried along, all in lines this PR rewrites:

  • affinity shipped in armonik-control-plane/values.yaml three times and no template read any of them; armonik-compute-plane shipped none at all. Now rendered on all five workloads across both charts.
  • metricsExporter.imagePullSecrets and init.imagePullSecrets were likewise dead. Now wired, layered most-specific-first.
  • global.imagePullSecrets rendered invalid YAML: armonik.utils.index returns a string, so toYaml re-encoded it as a scalar (imagePullSecrets: '- name: cred'). Replaced by a plain concat of both inputs.
  • armonik-compute-plane/templates/deployment.yaml:113 had {{- . toYaml }} (missing pipe), so any non-empty agent.readinessProbe aborted the render. Unreachable because the value ships {}.

Testing

./test/unittest.sh, ./test/matrix.sh (with kubeconform), helm lint on all 11 charts, helm-docs and pre-commit all pass locally.

The refactor is byte-identical to the previous render on every existing fixture: compute-plane defaults and both ci/ fixtures, the fluent-bit sidecar path, control-plane defaults and both fixtures, and the full umbrella against ci/minimal-values.yaml. Verified by rendering baseline charts built from feat/revamp-helm and diffing document by document after normalising key order.

Added: armonik.utils.patch contract suite in test/harness/common-harness (8 tests), per-chart patch suites (11 + 9), a compute-plane scheduling suite for the affinity and pull-secret fixes (5), two control-plane tests for the same, one pinning the readinessProbe fix, two ci/patch-values.yaml render fixtures, and two expected-failure fixtures with matrix.sh cases for the list guard.

Impact

  • Behaviour change: chart-level and global imagePullSecrets now both apply rather than one shadowing the other. Intended: the kubelet tries each in turn, so a global registry credential no longer silently discards a chart-level one. Anyone relying on the old shadowing gets an extra secret in the list, which is harmless.
  • toYaml sorts keys, so rendered manifests are now alphabetical rather than name-first. Cosmetic, and what kubectl get -o yaml shows anyway, but helm template diffs against the current output will be noisy once.
  • No default render changes. No new dependencies. New values are all empty by default.
  • charts/best-practices.md gains an "Exposing arbitrary parameters" section stating the convention and how to add a patch point.

Additional Information

  • Sequencing: no file overlap with feat: custom/shared issuer support #313, and only two values files with feat: add network policies #303 (different hunks, several hundred lines apart). Both are mergeable against the same base. Suggest landing those two first and rebasing this, since it moves ~150 lines of deployment.yaml and the diff reads better against a settled base.
  • Two interactions with feat: add network policies #303 worth a look from its author: that PR derives its NetworkPolicy port from partitionCommon.agent.ports.containerPort, which the list guard already protects from being moved by a patch, and its pod selector uses armonik.selectorLabels, which podSpecPatch cannot reach.
  • hostNetwork is deliberately allowed. An earlier revision refused it because it escapes the NetworkPolicies feat: add network policies #303 adds; that was paternalistic. The guard's job is to stop a patch silently destroying what the chart built, not to second-guess a field the user set on purpose. A test pins the pass-through.
  • extraEnv intentionally has no collision check. Setting a name the conf layers already set emits it twice. Kubernetes accepts that (validation was proposed in Do not allow duplicate environment variable definitions kubernetes/kubernetes#59593 and closed unmerged over backward compatibility), but name is the patchMergeKey, so removing one duplicate later removes both. Noted in best-practices.md; not enforced, since envFrom can collide the same way with no way to check.
  • Not covered, deliberately: armonik-ingress, the metrics-exporter Deployment and the init Jobs have no patch points yet. The first is where feat: custom/shared issuer support #313 and feat: add network policies #303 both touch the pod spec; the others are straightforward follow-ups.

Checklist

  • My code adheres to the coding and style guidelines of the project.
  • I have performed a self-review of my code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • I have thoroughly tested my modifications and added tests when necessary.
  • Tests pass locally and in the CI.
  • I have assessed the performance impact of my modifications.

… charts

Also wires affinity and per-workload imagePullSecrets, which shipped as values
no template read; chart and global imagePullSecrets now both apply instead of
one shadowing the other.
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.

1 participant