fix(affinity): update affinity on server and aggregator to be automatically set - #898
fix(affinity): update affinity on server and aggregator to be automatically set#898pinkneyj wants to merge 4 commits into
Conversation
Greptile SummaryThe PR relocates the webhook anti-affinity default into chart values and adds equivalent automatic defaults for server and aggregator workloads.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; the previously reported regression-coverage gap is addressed by focused tests for both affected workloads. Important Files Changed
Reviews (2): Last reviewed commit: "update default affinities" | Re-trigger Greptile |
| {{- $podAntiAffinity := dict "podAntiAffinity" (dict "preferredDuringSchedulingIgnoredDuringExecution" (list (dict "weight" 100 "podAffinityTerm" (dict "labelSelector" (dict "matchLabels" (dict "app.kubernetes.io/name" "server")) "topologyKey" "kubernetes.io/hostname")))) }} | ||
| {{- $userAffinity := deepCopy (.Values.server.affinity | default (dict)) }} | ||
| {{- include "cloudzero-agent.generateAffinity" (dict "default" .Values.defaults.affinity "affinity" (merge $userAffinity $podAntiAffinity)) | nindent 6 }} |
There was a problem hiding this comment.
Missing anti-affinity regression coverage
The new server and aggregator anti-affinity rendering lacks focused Helm tests for the generated selector, weight, topology key, and user override behavior, so regressions in this scheduling policy can pass without detection.
Context Used: CLAUDE.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: helm/templates/agent-deploy.yaml
Line: 350-352
Comment:
**Missing anti-affinity regression coverage**
The new server and aggregator anti-affinity rendering lacks focused Helm tests for the generated selector, weight, topology key, and user override behavior, so regressions in this scheduling policy can pass without detection.
**Context Used:** CLAUDE.md ([source](https://github.com/cloudzero/cloudzero-agent/blob/main/CLAUDE.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Hi @pinkneyj , thank you again for another contribution! This is definitely a good change and I would certainly like to get this behavior merged in, one way or another. The anti-affinity term looks good to me, and you've followed [the pattern that already exists in the webhook-server template](https://github.com/Cloudzero/cloudzero-agent/blob/develop/helm/templates/webhook-deploy.yaml#L106-L120). But, your change has surfaced an opportunity for improvement in the chart generally. In the existing templates and in the proposed change, we're baking in an opinionated setting for pod affinity that cannot be removed or overridden. It's a reasonable default behavior, but we also want to allow users the flexibility to choose. What I would propose instead is to move this default - for the agent-deploy, the aggregator-deploy, and the webhook-server deploy - into the values.yaml file, declared on each component's own affinity key. This would mean that:
Let me know what you think. If you're up for the change in this PR, then that's great. Otherwise, we can handle it on our side. Thank you again for the PR! |
Hey @dmepham thanks for this response. I've made some changes based on your suggestions, do have a look when is convenient to do so. Thanks in advance! |
Why?
What
How Tested