refactor(agent): Make sensor DaemonSet updateStrategy configurable - #9
refactor(agent): Make sensor DaemonSet updateStrategy configurable#9infra-shinobi wants to merge 2 commits into
Conversation
WalkthroughThe Helm chart makes the agent DaemonSet updateStrategy configurable via ChangesAgent rollout strategy
🎯 2 (Simple) | ⏱️ ~8 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@charts/groundcover/templates/agent/daemonset.yaml`:
- Around line 25-29: The template directly dereferences
.Values.agent.updateStrategy.type and
.Values.agent.updateStrategy.rollingUpdate.maxUnavailable which can render <no
value> when users reuse-values; protect these lookups by assigning a safe local
(e.g. $updateStrategy := .Values.agent.updateStrategy | default dict) and use
default on the type lookup (e.g. $updateStrategy.type | default "") before
comparing to "RollingUpdate", and only render the rollingUpdate block when
$updateStrategy and $updateStrategy.rollingUpdate are present; update references
to use the guarded local ($updateStrategy) and default for maxUnavailable to
avoid empty values breaking spec.updateStrategy.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f4083e44-b9c8-49dc-b725-a3c9d6a0da94
📒 Files selected for processing (2)
charts/groundcover/templates/agent/daemonset.yamlcharts/groundcover/values.yaml
| type: {{ .Values.agent.updateStrategy.type }} | ||
| {{- if eq .Values.agent.updateStrategy.type "RollingUpdate" }} | ||
| rollingUpdate: | ||
| maxUnavailable: 100% | ||
| maxUnavailable: {{ .Values.agent.updateStrategy.rollingUpdate.maxUnavailable }} | ||
| {{- end }} |
There was a problem hiding this comment.
Guard agent.updateStrategy lookups for helm upgrade --reuse-values compatibility
In charts/groundcover/templates/agent/daemonset.yaml (lines 25-29), the template directly dereferences .Values.agent.updateStrategy.type and .Values.agent.updateStrategy.rollingUpdate.maxUnavailable. If a previous release’s reused user values don’t include agent.updateStrategy, the render can produce <no value>/empty and break spec.updateStrategy before new defaults apply.
Suggested fix
+{{- $updateStrategy := .Values.agent.updateStrategy | default (dict) -}}
+{{- $updateStrategyType := dig "type" "RollingUpdate" $updateStrategy -}}
spec:
updateStrategy:
- type: {{ .Values.agent.updateStrategy.type }}
- {{- if eq .Values.agent.updateStrategy.type "RollingUpdate" }}
+ type: {{ $updateStrategyType }}
+ {{- if eq $updateStrategyType "RollingUpdate" }}
rollingUpdate:
- maxUnavailable: {{ .Values.agent.updateStrategy.rollingUpdate.maxUnavailable }}
+ maxUnavailable: {{ dig "rollingUpdate" "maxUnavailable" "100%" $updateStrategy }}
{{- end }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@charts/groundcover/templates/agent/daemonset.yaml` around lines 25 - 29, The
template directly dereferences .Values.agent.updateStrategy.type and
.Values.agent.updateStrategy.rollingUpdate.maxUnavailable which can render <no
value> when users reuse-values; protect these lookups by assigning a safe local
(e.g. $updateStrategy := .Values.agent.updateStrategy | default dict) and use
default on the type lookup (e.g. $updateStrategy.type | default "") before
comparing to "RollingUpdate", and only render the rollingUpdate block when
$updateStrategy and $updateStrategy.rollingUpdate are present; update references
to use the guarded local ($updateStrategy) and default for maxUnavailable to
avoid empty values breaking spec.updateStrategy.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
charts/groundcover/values.yaml (1)
415-418:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftRisky default: 100% maxUnavailable causes complete monitoring blackout during updates.
Setting
maxUnavailable: 100%allows all agent pods to go down simultaneously during rollouts. This creates:
- Complete telemetry gap (metrics, logs, traces)
- Potential data loss during update window
- No gradual rollout validation
Why this matters: Observability agents should maintain coverage during updates to avoid blind spots in production monitoring.
Recommendation: Use conservative defaults like
maxUnavailable: 1or10%for controlled rollout. Reserve aggressive values for opt-in scenarios where users understand the tradeoff.🛡️ Safer default configuration
updateStrategy: type: RollingUpdate rollingUpdate: - maxUnavailable: 100% + maxUnavailable: 1Alternative if percentage is required:
updateStrategy: type: RollingUpdate rollingUpdate: - maxUnavailable: 100% + maxUnavailable: 10%🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/groundcover/values.yaml` around lines 415 - 418, The values.yaml currently sets updateStrategy.rollingUpdate.maxUnavailable to "100%", which allows all agent pods to be killed at once; change the default to a conservative value (e.g. set updateStrategy.type = RollingUpdate and updateStrategy.rollingUpdate.maxUnavailable to "1" or "10%") to ensure partial availability during rollouts, and document the tradeoff near the maxUnavailable field so operators can opt into aggressive values if desired.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@charts/groundcover/values.yaml`:
- Around line 415-418: The values.yaml currently sets
updateStrategy.rollingUpdate.maxUnavailable to "100%", which allows all agent
pods to be killed at once; change the default to a conservative value (e.g. set
updateStrategy.type = RollingUpdate and
updateStrategy.rollingUpdate.maxUnavailable to "1" or "10%") to ensure partial
availability during rollouts, and document the tradeoff near the maxUnavailable
field so operators can opt into aggressive values if desired.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3720b3c8-65d3-402c-bd77-440e9f6a8f02
📒 Files selected for processing (1)
charts/groundcover/values.yaml
2 participants
Adds support for configuring the sensor DaemonSet updateStrategy via values.yaml
Summary by CodeRabbit
New Features
Changes