[platform]: add vpa for cozy etcd operator #850
Conversation
Signed-off-by: kklinch0 <kklinch0@gmail.com>
WalkthroughThis update introduces Vertical Pod Autoscaler (VPA) support to the etcd-operator Helm chart. It adds new configuration options for enabling VPA and specifying resource limits for both the etcd-operator and kube-rbac-proxy containers. The deployment template is updated to conditionally omit static resource requests and limits when VPA is enabled. A new VPA manifest template is included, and the chart metadata, schema, and documentation are updated to reflect these changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Helm
participant Kubernetes API
participant etcd-operator Deployment
participant VPA Controller
User->>Helm: Install/Upgrade chart with VPA enabled
Helm->>Kubernetes API: Deploy etcd-operator and VPA manifests
Kubernetes API->>etcd-operator Deployment: Create/Update deployment
Kubernetes API->>VPA Controller: Create VPA resource
VPA Controller->>etcd-operator Deployment: Monitor and adjust resources
Possibly related PRs
Suggested reviewers
Poem
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:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
5c4627a to
e8cc444
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/system/etcd-operator/charts/etcd-operator/values.schema.json (1)
260-290: 🛠️ Refactor suggestionDuplicate VPA schema under
kubeRbacProxy
Thisvpaschema is identical to the one underetcdOperator. Please see the above suggestion about extracting a common$reffrom"$defs"to eliminate duplication and improve maintainability.
🧹 Nitpick comments (1)
packages/system/etcd-operator/charts/etcd-operator/values.schema.json (1)
333-341: Restrict and documentupdatePolicyvalues
The globalvpa.updatePolicyis currently an unrestricted string. To help users choose valid options, add an"enum"(e.g.,["Off","Auto","Initial","Recreate"]), include a"description"explaining each mode, and consider a"default"value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
packages/system/etcd-operator/charts/etcd-operator/Chart.yaml(1 hunks)packages/system/etcd-operator/charts/etcd-operator/README.md(3 hunks)packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml(2 hunks)packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml(1 hunks)packages/system/etcd-operator/charts/etcd-operator/values.schema.json(3 hunks)packages/system/etcd-operator/charts/etcd-operator/values.yaml(3 hunks)
🧰 Additional context used
🪛 YAMLlint (1.35.1)
packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
🔇 Additional comments (7)
packages/system/etcd-operator/charts/etcd-operator/Chart.yaml (1)
2-5: Chart version bump
TheappVersionand chartversionfields have been correctly incremented to0.4.2to align with the VPA support changes and release.packages/system/etcd-operator/charts/etcd-operator/templates/workload/deployment.yml (2)
48-53: Conditional resource block for etcd-operator when VPA is enabled
The Helm template now omits the staticresourcessection for theetcd-operatorcontainer ifetcdOperator.vpa.enabledis true. This aligns with standard VPA best practices and looks correct.
92-97: Conditional resource block for kube-rbac-proxy when VPA is enabled
Similarly, the template skips theresourcessection for thekube-rbac-proxycontainer underkubeRbacProxy.vpa.enabled. The logic is consistent with the etcd-operator block.packages/system/etcd-operator/charts/etcd-operator/values.yaml (3)
87-95: Verify default VPA enablement
You’ve setetcdOperator.vpa.enabled: trueby default, which will remove all static resource requests/limits unless users opt out. Confirm this is the intended backward‑compatible default.
154-162: Verify default VPA enablement for kubeRbacProxy
ThekubeRbacProxy.vpa.enabledflag is also on by default. Please verify that enabling VPA out‑of‑the‑box for the proxy is acceptable for existing deployments.
203-206: Global VPA updatePolicy default
The root‑levelvpa.updatePolicyis set to"Auto", matching the template’s default. This looks good to drive the VPA updateMode.packages/system/etcd-operator/charts/etcd-operator/templates/workload/vpa.yml (1)
1-41: New VPA manifest template
This template correctly wraps theVerticalPodAutoscalerresource in anifblock checking eitheretcdOperator.vpa.enabledorkubeRbacProxy.vpa.enabled. Resource policies for each container are injected dynamically.
Please validate the rendered YAML for proper indentation and whitespace control ({{- ... }}vs{{ ... }}) to avoid templating‑induced syntax issues. The reported YAMLlint error can be safely ignored for Helm chart templates.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
| @@ -51,4 +61,5 @@ | |||
| | serviceAccount.annotations | object | `{}` | Annotations to add to the service account | | |||
| | serviceAccount.create | bool | `true` | Specifies whether a service account should be created | | |||
| | tolerations | list | `[]` | ref: https://kubernetes.io/docs/concepts/scheduling-eviction/taint-and-toleration/ | | |||
| | vpa.updatePolicy | string | `"Auto"` | | | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Provide description for vpa.updatePolicy
The vpa.updatePolicy entry doesn’t describe allowed values or behavior. Please add text like “Controls VPA update mode (e.g., ‘Auto’, ‘Initial’, ‘Off’)”.
| | etcdOperator.vpa.enabled | bool | `true` | | | ||
| | etcdOperator.vpa.maxAllowed.cpu | string | `"1000m"` | | | ||
| | etcdOperator.vpa.maxAllowed.memory | string | `"1Gi"` | | | ||
| | etcdOperator.vpa.minAllowed.cpu | string | `"100m"` | | | ||
| | etcdOperator.vpa.minAllowed.memory | string | `"128Mi"` | | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add descriptions for etcdOperator.vpa settings
The new table entries for etcdOperator.vpa.enabled, etcdOperator.vpa.minAllowed.*, and etcdOperator.vpa.maxAllowed.* lack user-facing descriptions. Please provide concise descriptions such as “Enable VPA for etcd-operator”, “Minimum resources VPA may allocate”, and “Maximum resources VPA may allocate”.
| | kubeRbacProxy.vpa.enabled | bool | `true` | | | ||
| | kubeRbacProxy.vpa.maxAllowed.cpu | string | `"500m"` | | | ||
| | kubeRbacProxy.vpa.maxAllowed.memory | string | `"256Mi"` | | | ||
| | kubeRbacProxy.vpa.minAllowed.cpu | string | `"50m"` | | | ||
| | kubeRbacProxy.vpa.minAllowed.memory | string | `"64Mi"` | | |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add descriptions for kubeRbacProxy.vpa settings
Likewise, the kubeRbacProxy.vpa.* table entries need clear descriptions (e.g., “Enable VPA for kube-rbac-proxy”, “Min/Max resource bounds for proxy VPA”).
| }, | ||
| "vpa": { | ||
| "properties": { | ||
| "enabled": { | ||
| "type": "boolean" | ||
| }, | ||
| "maxAllowed": { | ||
| "properties": { | ||
| "cpu": { | ||
| "type": "string" | ||
| }, | ||
| "memory": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "type": "object" | ||
| }, | ||
| "minAllowed": { | ||
| "properties": { | ||
| "cpu": { | ||
| "type": "string" | ||
| }, | ||
| "memory": { | ||
| "type": "string" | ||
| } | ||
| }, | ||
| "type": "object" | ||
| } | ||
| }, | ||
| "type": "object" | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract and DRY the VPA schema for etcdOperator
The block defining "vpa" under etcdOperator duplicates the same structure used elsewhere. Consider moving this schema into a shared definition (e.g. under "$defs") and referencing it with $ref to avoid repetition and simplify future updates.
Summary by CodeRabbit