Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions .github/workflows/e2e-k8s.yml
Original file line number Diff line number Diff line change
Expand Up @@ -364,9 +364,8 @@ jobs:
echo "=== Operator logs ===" && \
kubectl logs -n vmafx-e2e-test deployment/vmafx-operator \
--tail=200 2>/dev/null || true
# Select the server Pods by the chart Service's labels: the server
# Deployment's own selector also matches the operator Pod, so
# `kubectl logs deployment/vmafx` can print the operator's logs.
# Select the server Pods by the chart Service's labels so every
# server Pod's log is printed; `deployment/vmafx` picks one Pod.
echo "=== Server logs ===" && \
kubectl logs -n vmafx-e2e-test \
-l app.kubernetes.io/instance=vmafx,app.kubernetes.io/component=server \
Expand Down
18 changes: 18 additions & 0 deletions .github/workflows/helm-chart.yml
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,24 @@ jobs:
helm template vmafx deploy/helm/vmafx \
--set pushgateway.enabled=true > /dev/null

# ADR-1353: the server Deployment selected every Pod of the release, the
# operator's and node's included. Render each server workload with every
# other component on and require each selector to pick only its own Pods.
- name: Workload selector isolation
run: |
set -euo pipefail
python3 -B -m unittest discover -s scripts/ci/tests \
-p 'test_check_helm_selector_isolation.py'
for workload in Deployment StatefulSet Job; do
rendered="$RUNNER_TEMP/selectors-${workload}.yaml"
helm template vmafx deploy/helm/vmafx \
--set "workload=${workload}" \
--set operator.enabled=true \
--set node.enabled=true \
--set podDisruptionBudget.enabled=true > "$rendered"
python3 scripts/ci/check-helm-selector-isolation.py "$rendered"
done

# ADR-1119 removed the pre-fx operator flags. A plain Helm render stayed
# syntactically valid while Kubernetes probed the now-unused :8082 port,
# so pin the semantic env/port contract as well as YAML validity.
Expand Down
16 changes: 16 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,22 @@
limit; each still publishes one signed, attested multi-arch image.


- **Helm: the server Deployment no longer selects the operator and node Pods.**
The chart's server Deployment and StatefulSet selected only the release
labels, so they also matched the operator, node and `helm test` Pods, and
`kubectl logs deployment/vmafx` could print the operator's log. Both now
also select `app.kubernetes.io/component: server` (ADR-1353). Scoring
traffic was not affected: the Services already selected the server Pods
only. **Upgrade note for v1.0.0-rc.1 installs:** a workload's selector
cannot be changed in place, so `helm upgrade` fails with
`spec.selector: ... field is immutable`. Delete the server workload first;
`--cascade=orphan` keeps its Pods serving until the upgrade replaces them:
`kubectl delete deployment,statefulset -n <namespace> --cascade=orphan -l app.kubernetes.io/instance=<release>,app.kubernetes.io/component=server`,
then run `helm upgrade` as usual. Uninstalling and installing again also
works. See "Upgrading from 1.0.0-rc.1" in
`docs/development/k8s-deployment.md`.


- A published release's container images can be recovered after a build
recipe fix (ADR-1347). A `workflow_dispatch` of the image publish workflows
on the default branch builds the release tag's source with that commit's
Expand Down
14 changes: 14 additions & 0 deletions changelog.d/fixed/helm-server-selector.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,14 @@
- **Helm: the server Deployment no longer selects the operator and node Pods.**
The chart's server Deployment and StatefulSet selected only the release
labels, so they also matched the operator, node and `helm test` Pods, and
`kubectl logs deployment/vmafx` could print the operator's log. Both now
also select `app.kubernetes.io/component: server` (ADR-1353). Scoring
traffic was not affected: the Services already selected the server Pods
only. **Upgrade note for v1.0.0-rc.1 installs:** a workload's selector
cannot be changed in place, so `helm upgrade` fails with
`spec.selector: ... field is immutable`. Delete the server workload first;
`--cascade=orphan` keeps its Pods serving until the upgrade replaces them:
`kubectl delete deployment,statefulset -n <namespace> --cascade=orphan -l app.kubernetes.io/instance=<release>,app.kubernetes.io/component=server`,
then run `helm upgrade` as usual. Uninstalling and installing again also
works. See "Upgrading from 1.0.0-rc.1" in
`docs/development/k8s-deployment.md`.
13 changes: 10 additions & 3 deletions deploy/helm/vmafx/AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -117,9 +117,15 @@ adds its metrics port 8080 as endpoint behind scoring Service, producing
nondeterministic HTTP 404 responses. Keep headless Service, main PDB, HTTP
NetworkPolicy, and ServiceMonitor selectors aligned with server label.

Never add label to existing Deployment/StatefulSet `spec.selector` in
patch release: those fields are immutable for installed workloads. Pod
template label plus consumer selectors provides upgrade-safe routing isolation.
Server Deployment + StatefulSet `spec.selector` also carry
`component: server` (ADR-1353, 1.0.0-rc.2). rc.1 selector (release labels
only) matched operator, node, `helm test` Pods. Keep label. Workload selectors
immutable: any later
`spec.selector` change needs ADR + documented delete-and-upgrade path, like
`docs/development/k8s-deployment.md#upgrading-from-100-rc1`.
`scripts/ci/check-helm-selector-isolation.py` (`helm-chart.yml`) fails when
any workload selector matches another component's Pods. New component: own
`component` label in both selector and Pod template.

## Active GPU backends

Expand All @@ -141,3 +147,4 @@ scheduling documentation when rebasing older chart work.
- [ADR-1119](../../../docs/adr/1119-golusoris-go-framework-adoption.md) — env-only fx migration
- [ADR-1129](../../../docs/adr/1129-release-container-runtime-alignment.md) — release image/runtime alignment
- [ADR-0726](../../../docs/adr/0726-drop-vulkan-backend.md) — Vulkan backend removal
- [ADR-1353](../../../docs/adr/1353-helm-server-component-selector.md) — server workload component selector, rc.1 upgrade path
1 change: 1 addition & 0 deletions deploy/helm/vmafx/templates/deployment.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ spec:
selector:
matchLabels:
{{- include "vmafx.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: server
strategy:
{{- toYaml .Values.deployment.strategy | nindent 4 }}
template:
Expand Down
1 change: 1 addition & 0 deletions deploy/helm/vmafx/templates/statefulset.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ spec:
selector:
matchLabels:
{{- include "vmafx.selectorLabels" . | nindent 6 }}
app.kubernetes.io/component: server
updateStrategy:
{{- toYaml .Values.statefulSet.updateStrategy | nindent 4 }}
# minReadySeconds: 0 by default; set via .Values.minReadySeconds if needed.
Expand Down
43 changes: 43 additions & 0 deletions docs/adr/1353-helm-server-component-selector.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,43 @@
<!-- markdownlint-disable MD013 MD060 -->
# ADR-1353: Give the Helm server workload its own component selector

- **Status**: Accepted
- **Date**: 2026-09-28
- **Deciders**: lusoris
- **Tags**: helm, kubernetes, release

## Context

The chart's server Deployment (`deploy/helm/vmafx/templates/deployment.yaml`) selected only the release labels, `app.kubernetes.io/name` and `app.kubernetes.io/instance`. The operator and node Deployments select the same two labels plus their own `app.kubernetes.io/component`. The server selector therefore matched the operator and node Pods too, and the `helm test` Pod. Kubernetes documents overlapping controller selectors as unsupported. In the Kubernetes E2E run, `kubectl logs deployment/vmafx` printed the operator's log (`T-HELM-SERVER-DEPLOYMENT-SELECTOR-OVERLAP-2026-09-27`). The StatefulSet variant of the server workload (`statefulset.yaml`) had the same selector.

Scoring traffic was never affected. #1181 added `app.kubernetes.io/component: server` to the server Pod templates and to the Service, headless Service, ServiceMonitor, PodDisruptionBudget and HTTP NetworkPolicy selectors. It deliberately left the workload selectors alone, because `spec.selector` of a Deployment or StatefulSet is immutable: a `helm upgrade` that changes it fails with `spec.selector: Invalid value: ...: field is immutable`.

v1.0.0-rc.1 shipped with the overlapping selectors. Fixing them means that `helm upgrade` from an rc.1 release fails once, whatever else changes.

## Decision

The server Deployment and StatefulSet select `app.kubernetes.io/component: server` in addition to the release labels, so each chart component selects only its own Pods. The change ships in 1.0.0-rc.2 without a chart major version. The upgrade path for an rc.1 install is documented in the [Kubernetes deployment guide](../development/k8s-deployment.md#upgrading-from-100-rc1) and in the changelog: delete the server workload with `--cascade=orphan` and run `helm upgrade` (the new workload adopts the running Pods), or uninstall and install again. The chart `version:` stays `0.1.0`: it is packaging-only and deliberately not coordinated with releases ([ADR-1127](1127-single-semver-release-stream.md)). `scripts/ci/check-helm-selector-isolation.py` checks `helm template` output in the Helm Chart workflow and fails when any workload selector matches another component's Pods.

## Alternatives considered

| Option | Pros | Cons | Why not chosen |
|---|---|---|---|
| Change the selectors in rc.2 with a documented delete-and-upgrade (chosen) | Fixes the overlap before 1.0.0, while only release-candidate testers have installs; the orphan-delete path keeps the Pods serving | Every rc.1 install needs one manual step before its first upgrade | Maintainer decision, 2026-09-27 |
| Change the selectors with a chart major version bump | Signals the break through the chart version | Nothing publishes the chart version, so the bump would warn nobody; it would still need the same manual step | Rejected by the maintainer |
| Keep the overlapping selectors (status quo since #1181) | No upgrade step | Unsupported controller overlap stays in 1.0.0 and becomes harder to remove after it | Leaves a known defect in the final release |
| Fix only the Deployment, not the StatefulSet | Smaller change | The StatefulSet has the same overlap and would need a second breaking upgrade later | One migration costs less than two |
| A chart `lookup` guard that fails the upgrade with instructions | Clearer message than Kubernetes' immutable-field error | Adds template logic that `helm template` and GitOps renderers cannot exercise, for a one-time migration | The documented error text and upgrade section cover it |

## Consequences

- **Positive**: `kubectl logs deployment/<release>` and any query by the workload's selector see only server Pods, and no two chart controllers compete for a Pod. The check keeps any new component from reintroducing an overlap.
- **Negative**: `helm upgrade` from a v1.0.0-rc.1 release fails until the server Deployment or StatefulSet is deleted. Argo CD and Flux report the same error.
- **Neutral / follow-ups**: The Kubernetes E2E creates a fresh kind cluster for every run, so it installs the chart and never upgrades it. The upgrade path was exercised by hand on kind v1.37.0 with Helm 4.2.4, from the v1.0.0-rc.1 chart and images: the plain upgrade failed for both workloads; after `kubectl delete --cascade=orphan` the upgrade succeeded, the new Deployment adopted the running ReplicaSet and Pod without a restart and then rolled a template change normally; the StatefulSet adopted its Pod and kept its PVC; delete without `--cascade=orphan` followed by the upgrade also succeeded.

## References

- req: "ship in RC2 with an upgrade note telling rc.1 testers to delete and reinstall (NOT a chart major bump)" (maintainer decision, 2026-09-27, recorded in the `docs/state.md` row).
- `T-HELM-SERVER-DEPLOYMENT-SELECTOR-OVERLAP-2026-09-27`; `T-E2E-K8S-FIXTURE-BELOW-DEFAULT-MODEL-2026-09-27` (where the overlap was found).
- [Research: E2E Kubernetes runtime contract](../research/e2e-k8s-runtime-contract-2026-08-31.md) (#1181, the Pod-template and Service selector fix).
- [ADR-1127](1127-single-semver-release-stream.md) (chart packaging version is independent).
- Kubernetes documentation, Deployments, "Label selector updates" and "Selector": overlapping selectors between controllers are not supported.
1 change: 1 addition & 0 deletions docs/adr/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -1153,4 +1153,5 @@ public authority; documentation never links into either local root.
| [ADR-1350](1350-recovery-overlay-ffmpeg-patches.md) | A release image recovery run also takes `ffmpeg-patches/` (the patch series for the bundled FFmpeg) from the dispatching commit, so the rc.1 node image can build with patch 0019's aarch64 warning fix; all VMAFx and libvmaf code stays the tag's. | Accepted | release, ci, container, ffmpeg |
| [ADR-1352](1352-rc-phase-shift-plus-one.md) | Shift the first-release candidate mapping by one so that phase numbers match tags: `v1.0.0-rc.2` (RC2) is a stabilisation candidate that carries the dependency and fix train under the RC1 exit bar, benchmarks, profiling and tuning move to `v1.0.0-rc.3` (RC3), and the one-shot real retrain moves to `v1.0.0-rc.4` (RC4). Amends ADR-1341's tag mapping only. | Accepted | release, rc |
| [ADR-1355](1355-cli-option-value-backslashes.md) | Backslashes in `--model` / `--feature` values are data, except in a run that directly precedes `:` / `=` or ends the value, which is read in pairs; keys keep the ADR-1190 escape set, so `..\`, `\server` and `\.cache` paths survive | Accepted | cli, parser, windows, upstream, bug |
| [ADR-1353](1353-helm-server-component-selector.md) | The Helm server Deployment and StatefulSet select `app.kubernetes.io/component: server` so they no longer match the operator, node and test Pods; ships in 1.0.0-rc.2 without a chart major bump, and an rc.1 install deletes the server workload (`--cascade=orphan` keeps its Pods) before `helm upgrade`. | Accepted | helm, kubernetes, release |
| [ADR-1247](1247-scorecard-exact-head-gates.md) | Bind Scorecard gates to their measured source and scope | Accepted | ci, security, supply-chain |
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
| [ADR-1353](1353-helm-server-component-selector.md) | The Helm server Deployment and StatefulSet select `app.kubernetes.io/component: server` so they no longer match the operator, node and test Pods; ships in 1.0.0-rc.2 without a chart major bump, and an rc.1 install deletes the server workload (`--cascade=orphan` keeps its Pods) before `helm upgrade`. | Accepted | helm, kubernetes, release |
1 change: 1 addition & 0 deletions docs/adr/_index_fragments/_order.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1061,3 +1061,4 @@
1350-recovery-overlay-ffmpeg-patches
1352-rc-phase-shift-plus-one
1355-cli-option-value-backslashes
1353-helm-server-component-selector
3 changes: 2 additions & 1 deletion docs/adr/by-tag/helm.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

Auto-generated by `scripts/docs/generate-adr-by-tag.sh`. Edit ADR `Tags:` lines to update.

8 ADR(s) carry this tag.
9 ADR(s) carry this tag.

| ID | Title |
|----|-------|
Expand All @@ -15,3 +15,4 @@ Auto-generated by `scripts/docs/generate-adr-by-tag.sh`. Edit ADR `Tags:` lines
| [ADR-1058](../1058-helm-chart-security-hardening.md) | Helm chart security hardening — PDB, RBAC split, metrics NetworkPolicy, schema tightening |
| [ADR-1074](../1074-helm-values-completeness.md) | Helm chart values completeness — missing knobs and schema gaps |
| [ADR-1094](../1094-helm-rolling-update-correctness.md) | Helm chart rolling-update correctness — node strategy, PDB default, probe fix, grace period |
| [ADR-1353](../1353-helm-server-component-selector.md) | Give the Helm server workload its own component selector |
6 changes: 3 additions & 3 deletions docs/adr/by-tag/index.md
Original file line number Diff line number Diff line change
Expand Up @@ -263,7 +263,7 @@ Auto-generated by `scripts/docs/generate-adr-by-tag.sh` from each ADR's `Tags:`
| [hbd](hbd.md) | 1 |
| [hdr](hdr.md) | 18 |
| [headers](headers.md) | 1 |
| [helm](helm.md) | 8 |
| [helm](helm.md) | 9 |
| [hfr](hfr.md) | 2 |
| [hip](hip.md) | 99 |
| [hooks](hooks.md) | 5 |
Expand Down Expand Up @@ -299,7 +299,7 @@ Auto-generated by `scripts/docs/generate-adr-by-tag.sh` from each ADR's `Tags:`
| [knob-sweep](knob-sweep.md) | 1 |
| [konvid](konvid.md) | 5 |
| [konvid-1k](konvid-1k.md) | 1 |
| [kubernetes](kubernetes.md) | 4 |
| [kubernetes](kubernetes.md) | 5 |
| [ladder](ladder.md) | 6 |
| [language-modernization](language-modernization.md) | 3 |
| [language-policy](language-policy.md) | 1 |
Expand Down Expand Up @@ -475,7 +475,7 @@ Auto-generated by `scripts/docs/generate-adr-by-tag.sh` from each ADR's `Tags:`
| [regression-gate](regression-gate.md) | 2 |
| [regression-guard](regression-guard.md) | 2 |
| [regression-recovery](regression-recovery.md) | 2 |
| [release](release.md) | 30 |
| [release](release.md) | 31 |
| [release-please](release-please.md) | 1 |
| [reliability](reliability.md) | 4 |
| [rename](rename.md) | 1 |
Expand Down
3 changes: 2 additions & 1 deletion docs/adr/by-tag/kubernetes.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@

Auto-generated by `scripts/docs/generate-adr-by-tag.sh`. Edit ADR `Tags:` lines to update.

4 ADR(s) carry this tag.
5 ADR(s) carry this tag.

| ID | Title |
|----|-------|
| [ADR-0699](../0699-vmafx-helm-chart-k8s.md) | VMAFX Helm Chart and Kubernetes Manifests with 3-Vendor GPU Device-Plugin Support |
| [ADR-0930](../0930-helm-networkpolicy-pss.md) | Ship NetworkPolicy default-deny + Pod Security Standards "restricted" in the VMAFX Helm chart |
| [ADR-0969](../0969-helm-seccomp-default-plus-node-image-helper.md) | Helm chart — add seccompProfile default and fix node-deployment image helper (Round 26 audit B.1 + B.3) |
| [ADR-1094](../1094-helm-rolling-update-correctness.md) | Helm chart rolling-update correctness — node strategy, PDB default, probe fix, grace period |
| [ADR-1353](../1353-helm-server-component-selector.md) | Give the Helm server workload its own component selector |
3 changes: 2 additions & 1 deletion docs/adr/by-tag/release.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

Auto-generated by `scripts/docs/generate-adr-by-tag.sh`. Edit ADR `Tags:` lines to update.

30 ADR(s) carry this tag.
31 ADR(s) carry this tag.

| ID | Title |
|----|-------|
Expand Down Expand Up @@ -37,3 +37,4 @@ Auto-generated by `scripts/docs/generate-adr-by-tag.sh`. Edit ADR `Tags:` lines
| [ADR-1349](../1349-native-arch-node-image-build.md) | Build the vmafx-node image per architecture on native runners |
| [ADR-1350](../1350-recovery-overlay-ffmpeg-patches.md) | Include the FFmpeg patch series in the image recovery recipe |
| [ADR-1352](../1352-rc-phase-shift-plus-one.md) | Shift the first-release candidate mapping by one |
| [ADR-1353](../1353-helm-server-component-selector.md) | Give the Helm server workload its own component selector |
Loading
Loading