feat: deploy CSI external health monitor sidecar, align CSI images with 2.9.2 - #2750
feat: deploy CSI external health monitor sidecar, align CSI images with 2.9.2#2750sergeyberezansky wants to merge 1 commit into
Conversation
…th 2.9.2 The csi-wekafs chart added an external health monitor sidecar, which sweeps the volume fleet and reports abnormal volumes as events on their PVC. The embedded CSI deployment had no equivalent, so operator-managed CSI got no volume health reporting. Add the sidecar to the CSI controller deployment, configurable via csi.healthMonitor (enabled/monitorInterval/timeoutSeconds) and csi.healthMonitorImage, and pass --advertisevolumehealthsupport to the driver so it only advertises VOLUME_CONDITION when something consumes it. That driver flag only exists in csi-wekafs >= v2.9.0, so bump csi.image v2.8.8 -> v2.9.2 and align the remaining sidecars with the versions the 2.9.2 chart ships. The flag sets we pass each sidecar are unchanged between the pinned and target versions (verified against the 2.9.2 chart), so the bumps are drop-in despite provisioner v5->v6 and resizer v1->v2 being major. Also grant the controller ServiceAccount "get" on events, which the health monitor needs; the rest of its RBAC was already covered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
|
Claude finished @sergeyberezansky's task in 4m 45s —— View job Review: CSI external health monitor sidecar
SummarySolid, well-scoped change. The sidecar mirrors the four existing ones faithfully —
Findings
None of these are blockers. #1 and #3 are the two I'd act on before merge. Questions
Tests: the new cases look right — presence/absence, image, args, driver flag, and hash on both toggle and retune. They mutate package-global |
There was a problem hiding this comment.
Pull request overview
Adds CSI volume health reporting to the operator-managed embedded CSI deployment by introducing the external health monitor sidecar and aligning the embedded CSI image set with the csi-wekafs chart v2.9.2 configuration.
Changes:
- Add optional
csi-external-health-monitor-controllersidecar to the CSI controller Deployment and wire driver flag--advertisevolumehealthsupportto the enablement setting. - Add env/config plumbing for health monitor image, enablement, sweep interval, timeout, and resources; include these in the controller deployment hash.
- Update Helm chart defaults (CSI sidecar image versions, new health monitor values, and RBAC event
get).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| internal/controllers/operations/csi/controller.go | Adds health monitor container creation, driver flag wiring, and includes health monitor settings in controller rollout hash. |
| internal/controllers/operations/csi/controller_hash_test.go | Adds tests asserting health monitor sidecar presence/absence, driver flag behavior, and hash changes. |
| internal/config/env.go | Introduces health monitor settings/resources in embedded CSI config and loads them from env vars. |
| charts/weka-operator/values.yaml | Bumps CSI image tags to match 2.9.2 and adds default health monitor values + resources. |
| charts/weka-operator/templates/manager.yaml | Passes new health monitor env vars (image/settings/resources) into the operator manager. |
| charts/weka-operator/templates/csi/controllerserver-rbac.yaml | Extends controller RBAC to include get on events for the health monitor sidecar. |
Suppressed comments (1)
internal/controllers/operations/csi/controller_hash_test.go:143
- This test sets global config fields (WekafsImage/HealthMonitorImage/HealthMonitor) but doesn't restore the image fields, so later tests in the same package can observe unexpected config state depending on execution order.
func TestNewCsiControllerDeploymentHealthMonitor(t *testing.T) {
config.Config.Csi.WekafsImage = "test-csi-image"
config.Config.Csi.HealthMonitorImage = "test-health-monitor-image"
t.Cleanup(func() { config.Config.Csi.HealthMonitor = config.CsiHealthMonitorSettings{} })
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| func TestGetCsiControllerDeploymentHashHealthMonitor(t *testing.T) { | ||
| config.Config.Csi.WekafsImage = "test-csi-image" | ||
| config.Config.Csi.HealthMonitorImage = "test-health-monitor-image" | ||
| config.Config.Csi.HealthMonitor = config.CsiHealthMonitorSettings{ | ||
| Enabled: true, | ||
| MonitorInterval: "5m", | ||
| TimeoutSeconds: 300, | ||
| } | ||
| t.Cleanup(func() { config.Config.Csi.HealthMonitor = config.CsiHealthMonitorSettings{} }) | ||
|
|
||
| wekaClient := &weka.WekaClient{} |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| // Tied to the health monitor sidecar: with nothing calling ControllerGetVolume, | ||
| // advertising the capability would only make the driver watch every PV for no | ||
| // consumer. The flag requires csi-wekafs >= v2.9.0. | ||
| args = append(args, fmt.Sprintf("--advertisevolumehealthsupport=%t", config.Config.Csi.HealthMonitor.Enabled)) |
There was a problem hiding this comment.
The flag is appended unconditionally, so csi.healthMonitor.enabled=false is not an escape hatch for a pinned older driver — a v2.8.x image still gets --advertisevolumehealthsupport=false and still dies on the unknown flag. The PR description says to "set csi.healthMonitor.enabled=false and pin a chart that predates this change", but the first half of that does nothing; only the chart pin helps. (The values.yaml note is accurate on this, the PR body isn't.)
If the driver's default for this flag is false when absent (likely, for a new opt-in), gating the append gives users a real escape hatch at zero cost — HealthMonitorEnabled is already in the hashable spec, so toggling still rolls the deployment:
| args = append(args, fmt.Sprintf("--advertisevolumehealthsupport=%t", config.Config.Csi.HealthMonitor.Enabled)) | |
| if config.Config.Csi.HealthMonitor.Enabled { | |
| args = append(args, "--advertisevolumehealthsupport=true") | |
| } |
If instead the driver defaults this to true, keep it explicit as written — but then please drop the misleading advice from the PR/release notes.
| // This budget covers the WHOLE paginated sweep, not one page, so it has to | ||
| // exceed the time to walk every volume. Too low and the sweep is cut off | ||
| // mid-fleet and restarts from the beginning next time, leaving the tail | ||
| // permanently unchecked. | ||
| fmt.Sprintf("--timeout=%ds", config.Config.Csi.HealthMonitor.TimeoutSeconds), | ||
| // The sidecar sweeps the fleet with ListVolumes when the driver advertises | ||
| // LIST_VOLUMES, and falls back to one ControllerGetVolume per volume | ||
| // otherwise. Those paths read different flags, so set both from the same value. | ||
| fmt.Sprintf("--monitor-interval=%s", config.Config.Csi.HealthMonitor.MonitorInterval), | ||
| fmt.Sprintf("--list-volumes-interval=%s", config.Config.Csi.HealthMonitor.MonitorInterval), |
There was a problem hiding this comment.
These two comments are in tension. The first asserts --timeout is a whole-sweep budget; the second correctly notes the sidecar has two code paths. Upstream's --timeout is the deadline for calls to the CSI driver: it wraps the whole paginated ListVolumes walk under one context (so "whole sweep" holds there), but on the ControllerGetVolume fallback path it is a per-volume RPC deadline.
That matters because 300s is 20× upstream's 15s default. If the wekafs driver does not advertise LIST_VOLUMES, then:
- a hung/slow
ControllerGetVolumepins a worker for 5 minutes instead of failing fast, and - the "raise it for larger fleets" guidance in
values.yamlis backwards for that path — fleet size doesn't enter into a per-volume deadline.
Worth confirming which capability v2.9.2 advertises, then narrowing both comments (and the values.yaml note) to the path that actually runs.
| Config.Csi.RegistrarImage = env.GetString("CSI_REGISTRAR_IMAGE", "") | ||
| Config.Csi.HealthMonitorImage = env.GetString("CSI_HEALTHMONITOR_IMAGE", "") | ||
| Config.Csi.HealthMonitor.Enabled = getBoolEnvOrDefault("CSI_HEALTH_MONITOR_ENABLED", true) | ||
| Config.Csi.HealthMonitor.MonitorInterval = getEnvOrDefault("CSI_HEALTH_MONITOR_INTERVAL", "5m") |
There was a problem hiding this comment.
MonitorInterval is taken as a free-form string and interpolated straight into a container arg, so a typo in values (monitorInterval: 300, 5min, 5 m) doesn't surface here — it ships a sidecar that flag.Duration rejects, and the CSI controller pod goes CrashLoopBackOff with the driver in it.
Note the asymmetry with the neighbouring line: getIntEnvOrDefault for TIMEOUT_SECONDS calls os.Exit(1) on an unparseable value, i.e. bad config fails fast at the operator. A time.ParseDuration check on this value would keep the two consistent:
Config.Csi.HealthMonitor.MonitorInterval = getEnvOrDefault("CSI_HEALTH_MONITOR_INTERVAL", "5m")
if _, err := time.ParseDuration(Config.Csi.HealthMonitor.MonitorInterval); err != nil {
klog.Error(fmt.Errorf("failed to parse duration %s from env var CSI_HEALTH_MONITOR_INTERVAL", Config.Csi.HealthMonitor.MonitorInterval))
os.Exit(1)
}
| func newCsiHealthMonitorContainer() corev1.Container { | ||
| return corev1.Container{ | ||
| Name: "csi-external-health-monitor-controller", | ||
| Image: config.Config.Csi.HealthMonitorImage, | ||
| Command: []string{"/shared/wait-for-leader"}, | ||
| Resources: toK8sResourceRequirements(config.Config.Csi.ControllerResources.CsiHealthMonitor), | ||
| Args: []string{ |
There was a problem hiding this comment.
Two consistency gaps vs. the four existing sidecars, both minor but easy to close now:
- Every other sidecar passes
--http-endpoint=:909xand declares a matchingContainerPort, and the pod annotationprometheus.io/port: "9090,9091,9092,9093,9095"(controller.go:223) enumerates them. This one exposes nothing, so the health monitor's sweep/error metrics are unscrapable — the one sidecar whose failures are otherwise silent (a permanently-truncated sweep looks identical to a healthy one). Adding--http-endpoint=:9094+ port +9094in the annotation would make it observable. - This container has no
SecurityContext, matching provisioner/resizer/snapshotter (onlywekafs/csi-attachersetPrivileged), so that part is fine.
Also: Resources here comes from ControllerResources.CsiHealthMonitor, which is not part of CsiControllerHashableSpec — so retuning csi.controller.resources.csiHealthMonitor won't roll the deployment. That's pre-existing behaviour for all the sidecar resource blocks, just flagging that the new knob inherits it.
| image: "quay.io/weka.io/csi-wekafs:v2.9.2" | ||
| provisionerImage: "registry.k8s.io/sig-storage/csi-provisioner:v6.3.0" | ||
| attacherImage: "registry.k8s.io/sig-storage/csi-attacher:v4.12.0" | ||
| livenessProbeImage: "registry.k8s.io/sig-storage/livenessprobe:v2.19.0" | ||
| resizerImage: "registry.k8s.io/sig-storage/csi-resizer:v2.2.1" | ||
| snapshotterImage: "registry.k8s.io/sig-storage/csi-snapshotter:v8.6.0" | ||
| registrarImage: "registry.k8s.io/sig-storage/csi-node-driver-registrar:v2.17.0" | ||
| healthMonitorImage: "registry.k8s.io/sig-storage/csi-external-health-monitor-controller:v0.18.0" |
There was a problem hiding this comment.
Blast radius of these bumps is wider than the feature itself, worth being explicit about in the release notes:
image,registrarImageandlivenessProbeImageare all inCsiNodeDaemonsetHashableSpec(daemonset.go:29-31, 78-80), so this change rolls the CSI node DaemonSet on every node in every existing install, not just the controller Deployment. That's the part users feel; the health monitor itself is additive.csi-provisionerv5→v6 andcsi-resizerv1→v2 are majors. Upstream sidecar majors usually raise the minimum supported Kubernetes, and this chart declares nokubeVersionconstraint inChart.yaml, so an under-supported cluster only finds out at pod-crash time. Worth confirming the new minimum against the oldest cluster the operator claims to support, and addingkubeVersionif it moved.- The flag-set diff against the 2.9.2 chart is the right check for these bumps — I couldn't independently verify it here (no network in this run), so I'm taking it as stated.
Graphite Automations"Add anton/matt/sergey/kristina as reviwers on operator PRs" took an action on this PR • (08/17/26)3 reviewers were added to this PR based on Anton Bykov's automation. |
What
The
csi-wekafschart (2.9.2) added an external health monitor sidecar — it sweeps the volume fleet, asks the driver for each volume's condition, and reports abnormal volumes as events on their PVC. The operator's embedded CSI deployment had no equivalent, so operator-managed CSI got no volume health reporting at all.This adds it, and pulls the CSI image set up to what the 2.9.2 chart ships.
Health monitor
csi-external-health-monitor-controllercontainer on the CSI controller Deployment, mirroring the 2.9.2 chart's container (same command/args/mounts,wait-for-leadergated like the other sidecars).csi.healthMonitorImage, andcsi.healthMonitor.{enabled,monitorInterval,timeoutSeconds}(defaultstrue/5m/300, matching the CSI chart). Resource requests/limits viacsi.controller.resources.csiHealthMonitor.--advertisevolumehealthsupport=<enabled>. It is tied to the sidecar deliberately: with nothing callingControllerGetVolume, advertisingVOLUME_CONDITIONwould only make the driver watch every PV for no consumer.Image bumps
--advertisevolumehealthsupportdoes not exist before csi-wekafs v2.9.0, so the driver has to move with the feature:Provisioner v5→v6 and resizer v1→v2 are major bumps, so I diffed the flags we pass each sidecar against the 2.9.2 chart, which runs these exact versions: the flag sets are identical (including
--prevent-volume-mode-conversion,--feature-gates=Topology=true, and resizer's--workers). No arg changes needed. All eight tags were confirmed to resolve in their registries.RBAC
The controller ServiceAccount gains
getonevents, which the health monitor needs. The rest of its requirements —persistentvolumeclaimsget/list/watch/update/patch,persistentvolumeclaims/statuspatch,nodesget/list/watch,leases— were already granted.csi.imageIf you override
csi.imagewith v2.8.x or older, the driver will now refuse to start on the unknown--advertisevolumehealthsupportflag. Setcsi.healthMonitor.enabled=falseand pin a chart that predates this change, or move the driver forward. This is called out invalues.yamlnext to the setting.Testing
go build ./...,go vet, andgo test ./internal/...all clean.controller_hash_test.go: the sidecar is present with the right image and args when enabled and absent when disabled; the driver flag flips with it; and the hash changes on both toggle and interval retune.helm lintclean; rendered the chart withcsi.installationEnabled=truein both enable states and verified every new env var and image lands.Not yet exercised against a live cluster — worth a deploy check that the sidecar reaches Ready and that volume-condition events show up on a PVC.
Related
kubectl-weka's airgapped bundler needs the health monitor image in its pull set; that side is being updated separately.🤖 Generated with Claude Code