fix etcd topologySpreadConstraints #1331
Conversation
Signed-off-by: kklinch0 <kklinch0@gmail.com>
WalkthroughVersion bump of the etcd Helm chart to 2.9.1, introduction of configurable topologySpreadConstraints in the etcd StatefulSet via an optional ConfigMap, and updates to versions_map to pin 2.9.0 to a commit and add 2.9.1 at HEAD. Changes
Sequence Diagram(s)sequenceDiagram
participant User as Helm install/upgrade
participant Helm as Etcd chart template
participant K8s as Kubernetes API
Note over Helm: Render etcd-cluster.yaml
Helm->>K8s: lookup ConfigMap cozy-system/cozystack-scheduling
alt ConfigMap has key globalAppTopologySpreadConstraints (non-empty)
Helm->>Helm: Parse YAML constraints
Helm-->>User: Render with provided topologySpreadConstraints
else
Helm-->>User: Render with hard-coded topologySpreadConstraints
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Suggested reviewers
Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Summary of Changes
Hello @klinch0, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
I've implemented a fix for etcd's topologySpreadConstraints by making them configurable. This change allows the etcd cluster to dynamically fetch its topology spread constraints from a central ConfigMap, providing greater flexibility in deployment configurations. If no custom constraints are defined, it falls back to the default behavior.
Highlights
- Dynamic Topology Spread Constraints: I've updated the etcd cluster deployment to dynamically retrieve topology spread constraints from the
cozystack-schedulingConfigMap. This enables centralized management of pod distribution policies for etcd. - Fallback Mechanism: If the specified ConfigMap or the
globalAppTopologySpreadConstraintskey is not found, the etcd deployment will default to its original, hardcoded topology spread constraints to ensure continued functionality. - Version Update: I've incremented the etcd chart version to 2.9.1 and updated the
versions_mapto reflect this change, along with the associated commit hash.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request updates the etcd chart to allow for configurable topologySpreadConstraints from a global ConfigMap, falling back to a default if not provided. The chart version is also bumped. My review includes a suggestion to make the Helm template logic for fetching the constraints more concise and robust.
| {{- $configMap := lookup "v1" "ConfigMap" "cozy-system" "cozystack-scheduling" }} | ||
| {{- $rawConstraints := "" }} | ||
| {{- if $configMap }} | ||
| {{- $rawConstraints = get $configMap.data "globalAppTopologySpreadConstraints" }} | ||
| {{- end }} |
There was a problem hiding this comment.
The logic to fetch globalAppTopologySpreadConstraints from the ConfigMap can be made more concise and idiomatic by using the with action. This avoids declaring a temporary variable for the ConfigMap and makes the template cleaner. Using default "" also adds robustness in case the key is not found or the data map is nil.
{{- $rawConstraints := "" }}
{{- with lookup "v1" "ConfigMap" "cozy-system" "cozystack-scheduling" }}
{{- $rawConstraints = get .data "globalAppTopologySpreadConstraints" | default "" }}
{{- end }}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/extra/etcd/templates/etcd-cluster.yaml (1)
52-56: Be cautious with lookup: determinism and RBAC. Consider gating or documenting.Using
lookupagainstcozy-system/cozystack-schedulingintroduces environment-dependent rendering:
- helm template (no cluster access) will take the fallback path; helm install/upgrade (with cluster access) might render differently.
- If the executing identity lacks
geton configmaps incozy-system, rendering may fail (depending on Helm version/behavior).Options:
- Gate behind a chart value (e.g.,
.Values.enableGlobalSchedulingConfig) or document the RBAC requirement and rendering variance.- Keep as-is if this is an intentional global override mechanism in your deployment model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/extra/etcd/Chart.yaml(1 hunks)packages/extra/etcd/templates/etcd-cluster.yaml(1 hunks)packages/extra/versions_map(1 hunks)
🔇 Additional comments (3)
packages/extra/versions_map (1)
16-17: versions_map entries verified and consistentAll verification steps have passed without issue:
- Commit
8ddbe32eexists.packages/extra/etcd/Chart.yamlat that commit is version 2.9.0.- Current
packages/extra/etcd/Chart.yamlis version 2.9.1.packages/extra/versions_mapincludes theetcd 2.9.1 HEADentry at lines 16–17.No further action required.
packages/extra/etcd/Chart.yaml (1)
6-6: Version bump to 2.9.1 LGTM.packages/extra/etcd/templates/etcd-cluster.yaml (1)
60-66: Default fallback retained; verify the label selector matches actual pod labels.The fallback constraint selects on
app.kubernetes.io/instance: etcd. Ensure etcd pods carry this label (operator-managed labels sometimes differ). If they don’t, the constraint won’t be applied.
| {{- if $rawConstraints }} | ||
| {{- $rawConstraints | fromYaml | toYaml | nindent 6 }} | ||
| {{- else }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle empty/whitespace values and list-vs-map inputs to avoid emitting 'null' or malformed YAML.
If the ConfigMap key exists but is empty/whitespace, fromYaml may yield nil and toYaml can emit null, breaking the manifest. Also, if the provided YAML is only a list (not including the topologySpreadConstraints: key), it will be emitted at the wrong level.
Proposed robust handling:
- Trim whitespace before testing truthiness.
- Parse once; if it's a map with
topologySpreadConstraints, emit it as-is; otherwise wrap the parsed content undertopologySpreadConstraints:.
Apply:
- {{- $configMap := lookup "v1" "ConfigMap" "cozy-system" "cozystack-scheduling" }}
- {{- $rawConstraints := "" }}
- {{- if $configMap }}
- {{- $rawConstraints = get $configMap.data "globalAppTopologySpreadConstraints" }}
- {{- end }}
- {{- if $rawConstraints }}
- {{- $rawConstraints | fromYaml | toYaml | nindent 6 }}
- {{- else }}
+ {{- $configMap := lookup "v1" "ConfigMap" "cozy-system" "cozystack-scheduling" }}
+ {{- $rawConstraints := "" }}
+ {{- if $configMap }}
+ {{- $rawConstraints = get $configMap.data "globalAppTopologySpreadConstraints" }}
+ {{- end }}
+ {{- $rawTrim := trim $rawConstraints }}
+ {{- if $rawTrim }}
+ {{- $parsed := fromYaml $rawTrim }}
+ {{- if and (kindIs "map" $parsed) (hasKey $parsed "topologySpreadConstraints") }}
+{{ $parsed | toYaml | nindent 6 }}
+ {{- else }}
+ topologySpreadConstraints:
+{{ $parsed | toYaml | nindent 8 }}
+ {{- end }}
+ {{- else }}
topologySpreadConstraints:
- maxSkew: 1
topologyKey: "kubernetes.io/hostname"
whenUnsatisfiable: ScheduleAnyway
labelSelector:
matchLabels:
app.kubernetes.io/instance: etcd
- {{- end }}
+ {{- end }}Also applies to: 67-67
🤖 Prompt for AI Agents
In packages/extra/etcd/templates/etcd-cluster.yaml around lines 57-59 (and
likewise at line 67), the template currently pipes $rawConstraints directly
through fromYaml/toYaml which can emit "null" for empty/whitespace values and
misplace list-only inputs; trim whitespace first, bail out if the trimmed value
is empty, then parse once into a variable, test if the parsed value is a map
that already contains the topologySpreadConstraints key and emit it as-is,
otherwise wrap the parsed value under topologySpreadConstraints: before
rendering so lists become the value of that key.
|
Successfully created backport PR for |
# Description Backport of #1331 to `release-0.34`.
What this PR does
Release note
Summary by CodeRabbit
New Features
Chores