konairius/homelab
10
0
Fork 0
Code Issues 8 Pull Requests 3 Packages Projects Releases 1 Wiki Activity

fix(cilium): roll the agent DaemonSet when cilium-config changes #613

Merged
k.renner merged 1 commits from cilium-reloader into master 2026-08-26 04:37:22 +00:00
Conversation 1 Commits 1 Files Changed 1
+18
claude-bot commented 2026-08-26 00:37:55 +00:00
Member
Copy Link
Copy Source

Closes the trap that made #611 a no-op until agents were restarted by hand.

Cilium reads datapath options once, at agent start, and the chart puts no config checksum on the pod template. A ConfigMap-only change therefore leaves every agent on the previous datapath, and nothing rolls the DaemonSet.

This already cost an outage

vlan-bpf-bypass landed in cilium-config and every v30iot macvlan workload stayed broken. The merge looked entirely successful:

HelmRelease            Ready=True, "Helm upgrade succeeded for cilium.v8"
cilium-config          vlan-bpf-bypass=30
cilium DaemonSet       3/3 current, AGE 108d   <-- never rolled
cilium agents          uptime 5h20m / 134m / 84m

Three pods stayed in sandbox creation until I deleted each agent by hand, one node at a time. Everything reported healthy while nothing worked.

policyMapMax has the identical exposure — its comment says "Takes effect on agent restart", and so do the notes in both bootstrap copies. That is documentation standing in for automation, in three places.

The change

annotations:
  configmap.reloader.stakater.com/reload: cilium-config

annotations lands on the top-level cilium-agent objects including the DaemonSet, which is what Reloader watches (verified against the chart values, not assumed). Reloader is already deployed in kube-system and used across the repo; named per-ConfigMap rather than blanket, matching the existing convention. Reloader supports DaemonSets and reloads "regardless of how they are used", which matters here because Cilium mounts the ConfigMap as a config directory rather than referencing keys.

The trade, stated plainly

Any cilium-config change now rolls the CNI cluster-wide, node by node. That is disruption — but it is the same disruption a datapath change already required, and the difference is that it happens on its own instead of being forgotten. If you would rather keep the roll manual, the alternative is to make the three "takes effect on agent restart" comments impossible to miss instead; I think automating it is the better trade, but it is a judgement call and this is the PR to argue it on.

Not merged — this touches the CNI DaemonSet, so it wants someone watching the rollout rather than landing unattended.

Closes the trap that made #611 a no-op until agents were restarted by hand. Cilium reads datapath options **once, at agent start**, and the chart puts no config checksum on the pod template. A ConfigMap-only change therefore leaves every agent on the previous datapath, and nothing rolls the DaemonSet. ## This already cost an outage `vlan-bpf-bypass` landed in `cilium-config` and every v30iot macvlan workload stayed broken. The merge looked entirely successful: ``` HelmRelease Ready=True, "Helm upgrade succeeded for cilium.v8" cilium-config vlan-bpf-bypass=30 cilium DaemonSet 3/3 current, AGE 108d <-- never rolled cilium agents uptime 5h20m / 134m / 84m ``` Three pods stayed in sandbox creation until I deleted each agent by hand, one node at a time. Everything reported healthy while nothing worked. `policyMapMax` has the identical exposure — its comment says *"Takes effect on agent restart"*, and so do the notes in both bootstrap copies. That is documentation standing in for automation, in three places. ## The change ```yaml annotations: configmap.reloader.stakater.com/reload: cilium-config ``` `annotations` lands on the top-level cilium-agent objects including the DaemonSet, which is what Reloader watches (verified against the chart values, not assumed). Reloader is already deployed in `kube-system` and used across the repo; named per-ConfigMap rather than blanket, matching the existing convention. Reloader supports DaemonSets and reloads "regardless of how they are used", which matters here because Cilium mounts the ConfigMap as a config directory rather than referencing keys. ## The trade, stated plainly Any `cilium-config` change now rolls the CNI cluster-wide, node by node. That is disruption — but it is the same disruption a datapath change already required, and the difference is that it happens on its own instead of being forgotten. If you would rather keep the roll manual, the alternative is to make the three "takes effect on agent restart" comments impossible to miss instead; I think automating it is the better trade, but it is a judgement call and this is the PR to argue it on. Not merged — this touches the CNI DaemonSet, so it wants someone watching the rollout rather than landing unattended.
claude-bot added 1 commit 2026-08-26 00:37:55 +00:00
fix(cilium): roll the agent DaemonSet when cilium-config changes
ci/woodpecker/pr/ci-manifests-test Pipeline was successful
Details
ci/woodpecker/pr/ci-scripts-test Pipeline was successful
Details
ci/woodpecker/pr/pr-review Pipeline was successful
Details
e11ba99991 
Cilium reads datapath options once, at agent start, and the chart puts no
config checksum on the pod template. A changed ConfigMap therefore leaves every
agent running the previous datapath until something restarts it, and nothing
does — the DaemonSet is not rolled by a ConfigMap-only Helm change.

This already cost an outage. vlan-bpf-bypass landed in cilium-config and every
v30iot macvlan workload stayed broken until the agents were deleted by hand,
one node at a time (issue #592). The merge looked successful: HelmRelease
Ready, release upgraded, key present in the ConfigMap, and nothing working.

policyMapMax carries the same exposure. Its comment says "takes effect on agent
restart", which is documentation standing in for automation — as does the
equivalent note in the two bootstrap copies.

Reloader is already deployed and used across the repo for the same purpose.
Named per-ConfigMap rather than blanket, matching the existing convention.

The trade, stated plainly: any cilium-config change now rolls the CNI
cluster-wide, node by node. That is the same disruption a datapath change
already required; the difference is that it happens on its own rather than
being forgotten.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
review-bot left a comment
Member
Copy Link
Copy Source

🤖 Automated review — model CLI default (complexity: complex), commit e11ba99991cfc9f584276400e63e3d38976826dc.

Advisory review by the review-bot pipeline. It never approves; a human merges. Push new commits to get a fresh review.

Summary

Adds a single annotations: {configmap.reloader.stakater.com/reload: cilium-config} entry to the Cilium HelmRelease values so Stakater Reloader rolls the cilium agent DaemonSet whenever the cilium-config ConfigMap changes, closing the "config applied but agents never restarted" trap described in the PR body (and in #611/#592). The mechanism checks out and the change is small and low-risk to review; I have no blocking issues.

🟡 Suggestions

  • kubernetes/infrastructure/cilium/helmrelease.yaml (not touched by this diff, but directly relevant to it): the rendered agent DaemonSet uses updateStrategy.rollingUpdate.maxUnavailable: 2 (confirmed in kubernetes/talos/cilium.yaml:1127, same chart). On a 3-node cluster that lets a cilium-config edit roll 2 of the 3 CNI agents at once. Previously that required a human to delete pods by hand, one at a time, which incidentally throttled the blast radius; now it happens automatically the moment the ConfigMap changes. The PR body already argues this "wants someone watching the rollout" — if so, consider pinning updateStrategy.rollingUpdate.maxUnavailable: 1 for the agent DaemonSet in this same values block so an unattended/mistimed config change can't take out most of the CNI simultaneously. Non-blocking since it's pre-existing chart-default behavior this PR doesn't change, but the PR is exactly what turns it from a manual, throttled risk into an automatic, unthrottled one.
  • The bootstrap copies (kubernetes/talos/cilium.yaml, kubernetes/talos/patches/cilium-cni.yaml) are left without this annotation. That looks correct rather than an oversight — they're one-shot manifests applied before Flux and Reloader exist, so the annotation would be inert there — but since the PR body calls out keeping policyMapMax/vlanBypass "in lockstep" across all three copies, it may be worth a one-line comment (here or in those files) noting this key is deliberately not mirrored, so a future reader doesn't assume it's missing.

Tests

No automated check would catch a wrong Helm values key here: ci-manifests-test.yml's kustomize-build step only runs kubectl kustomize over each kustomization, which validates that the HelmRelease CR is well-formed YAML/CRD — it does not render the Cilium chart's templates, so a nonexistent or misplaced values key would pass CI silently. I could not reach the network from this environment to pull the upstream cilium/cilium chart source directly, so I corroborated the PR's "verified against the chart values, not assumed" claim indirectly: in kubernetes/talos/cilium.yaml (a full render of the same chart, same version line, from the bootstrap path) the cilium DaemonSet's own metadata.annotations (line 1114) is absent — consistent with .Values.annotations defaulting to empty — while its pod-template annotations (line 1131, kubectl.kubernetes.io/default-container: cilium-agent) are populated from an unrelated chart default. That is the expected fingerprint of a chart that exposes a top-level annotations value distinct from podAnnotations, applied to the DaemonSet object itself rather than its pod template — which is exactly where Reloader needs to find the annotation. I also confirmed the ConfigMap name really is cilium-config in kube-system (kubernetes/talos/cilium.yaml:230, kubernetes/talos/patches/cilium-cni.yaml:248), that Reloader is deployed cluster-wide watching all namespaces by default (kubernetes/infrastructure/reloader/helmrelease.yaml:54-56), and that the identical *.reloader.stakater.com/reload annotation pattern is already used successfully elsewhere in this repo, including on other FluxCD-managed Helm values (external-dns's deploymentAnnotations, authentik's global.podAnnotations) and on raw DaemonSet/Deployment metadata (netbird-ingress, woodpecker-agent) — so this isn't a novel pattern for the repo. I could not, however, fetch the actual upstream chart source to see the literal template line, so this is strong circumstantial confirmation, not a byte-for-byte proof. Given the PR is explicitly "not merged — wants someone watching the rollout," the one check worth doing live is: after the next cilium-config edit, confirm Reloader adds its hash annotation to spec.template.metadata.annotations on the cilium DaemonSet (not just cilium-envoy, which doesn't consume cilium-config and is correctly left unaffected) and that kubectl rollout status daemonset/cilium -n kube-system completes cleanly.

Recommendation

Looks reasonable — no blocking issues; comments above are optional.

🤖 **Automated review** — model `CLI default` (complexity: complex), commit `e11ba99991cfc9f584276400e63e3d38976826dc`. > Advisory review by the `review-bot` pipeline. It never approves; a human merges. Push new commits to get a fresh review. --- ## Summary Adds a single `annotations: {configmap.reloader.stakater.com/reload: cilium-config}` entry to the Cilium `HelmRelease` values so Stakater Reloader rolls the `cilium` agent DaemonSet whenever the `cilium-config` ConfigMap changes, closing the "config applied but agents never restarted" trap described in the PR body (and in #611/#592). The mechanism checks out and the change is small and low-risk to review; I have no blocking issues. ## 🟡 Suggestions - **`kubernetes/infrastructure/cilium/helmrelease.yaml`** (not touched by this diff, but directly relevant to it): the rendered agent DaemonSet uses `updateStrategy.rollingUpdate.maxUnavailable: 2` (confirmed in `kubernetes/talos/cilium.yaml:1127`, same chart). On a 3-node cluster that lets a `cilium-config` edit roll 2 of the 3 CNI agents at once. Previously that required a human to delete pods by hand, one at a time, which incidentally throttled the blast radius; now it happens automatically the moment the ConfigMap changes. The PR body already argues this "wants someone watching the rollout" — if so, consider pinning `updateStrategy.rollingUpdate.maxUnavailable: 1` for the agent DaemonSet in this same values block so an unattended/mistimed config change can't take out most of the CNI simultaneously. Non-blocking since it's pre-existing chart-default behavior this PR doesn't change, but the PR is exactly what turns it from a manual, throttled risk into an automatic, unthrottled one. - The bootstrap copies (`kubernetes/talos/cilium.yaml`, `kubernetes/talos/patches/cilium-cni.yaml`) are left without this annotation. That looks correct rather than an oversight — they're one-shot manifests applied before Flux and Reloader exist, so the annotation would be inert there — but since the PR body calls out keeping `policyMapMax`/`vlanBypass` "in lockstep" across all three copies, it may be worth a one-line comment (here or in those files) noting this key is deliberately *not* mirrored, so a future reader doesn't assume it's missing. ## Tests No automated check would catch a wrong Helm values key here: `ci-manifests-test.yml`'s `kustomize-build` step only runs `kubectl kustomize` over each kustomization, which validates that the `HelmRelease` CR is well-formed YAML/CRD — it does not render the Cilium chart's templates, so a nonexistent or misplaced values key would pass CI silently. I could not reach the network from this environment to pull the upstream `cilium/cilium` chart source directly, so I corroborated the PR's "verified against the chart values, not assumed" claim indirectly: in `kubernetes/talos/cilium.yaml` (a full render of the same chart, same version line, from the bootstrap path) the `cilium` DaemonSet's own `metadata.annotations` (line 1114) is absent — consistent with `.Values.annotations` defaulting to empty — while its pod-template annotations (line 1131, `kubectl.kubernetes.io/default-container: cilium-agent`) are populated from an unrelated chart default. That is the expected fingerprint of a chart that exposes a top-level `annotations` value distinct from `podAnnotations`, applied to the DaemonSet object itself rather than its pod template — which is exactly where Reloader needs to find the annotation. I also confirmed the ConfigMap name really is `cilium-config` in `kube-system` (`kubernetes/talos/cilium.yaml:230`, `kubernetes/talos/patches/cilium-cni.yaml:248`), that Reloader is deployed cluster-wide watching all namespaces by default (`kubernetes/infrastructure/reloader/helmrelease.yaml:54-56`), and that the identical `*.reloader.stakater.com/reload` annotation pattern is already used successfully elsewhere in this repo, including on other FluxCD-managed Helm values (`external-dns`'s `deploymentAnnotations`, `authentik`'s `global.podAnnotations`) and on raw DaemonSet/Deployment metadata (`netbird-ingress`, `woodpecker-agent`) — so this isn't a novel pattern for the repo. I could not, however, fetch the actual upstream chart source to see the literal template line, so this is strong circumstantial confirmation, not a byte-for-byte proof. Given the PR is explicitly "not merged — wants someone watching the rollout," the one check worth doing live is: after the next `cilium-config` edit, confirm Reloader adds its hash annotation to `spec.template.metadata.annotations` on the `cilium` DaemonSet (not just `cilium-envoy`, which doesn't consume `cilium-config` and is correctly left unaffected) and that `kubectl rollout status daemonset/cilium -n kube-system` completes cleanly. ## Recommendation Looks reasonable — no blocking issues; comments above are optional. <!-- pr-review:sha=e11ba99991cfc9f584276400e63e3d38976826dc -->
k.renner merged commit 0d8f436466 into master 2026-08-26 04:37:22 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
review-bot
Labels
Clear labels
apollo
Hermes expert: Immich, Audiobookshelf, Libation, Navidrome, Music Assistant
 atlas
Hermes expert: Talos, Flux, network, Authentik, Matrix, monitoring
 clio
Hermes expert: Paperless, wiki, ical-proxy, TRMNL, websites
 daedalus
Hermes expert: Gitea, Woodpecker, Renovate, review-bot, Hermes
 hestia
Hermes expert: Home Assistant, IoT, MQTT, ESPHome, voice
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
alert-monitor-bot claude-bot (Claude Bot) flux hermes-bot issue-fix-bot k.renner renovate-bot review-bot (Review Bot) security-bot update-audit-bot
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: konairius/homelab#613