[snapshot-controller] Set resources requests/limits on controller and webhook - #2633
Matthieu ROBIN (matthieu-robin) wants to merge 1 commit into
Conversation
… webhook
The piraeusdatastore snapshot-controller chart (which deploys
kubernetes-csi/external-snapshotter) ships with resources: {} for both
the controller and the validation webhook, triggering 'no CPU limit'
conformance warnings. The upstream manifests don't ship recommendations
either, so size based on the production reference at
oneuptime.com/blog/2026-02-09-csi-snapshot-controller:
snapshot-controller limits 500m/512Mi, requests 100m/128Mi
snapshot-validation-webhook limits 100m/128Mi, requests 10m/32Mi
Webhook stays light (HTTP admission, matches the cert-manager-webhook
sizing in cozystack#2616); controller gets the production-recommended envelope.
Signed-off-by: Matthieu <[email protected]>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Summary of ChangesHello, 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! This pull request updates the Helm chart configuration for the snapshot-controller to include resource requests and limits. By defining these values for both the controller and the validation webhook, the changes ensure better resource management and satisfy cluster conformance requirements regarding CPU limits. Highlights
New Features🧠 You can now enable Memory (public preview) to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
Customization To customize the 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 Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis pull request adds Kubernetes resource constraints to the snapshot-controller Helm values. CPU and memory limits and requests are specified for both the controller component and the webhook component, with the webhook receiving lower resource allocations than the controller. ChangesSnapshot Controller Resource Configuration
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Poem
🚥 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)
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. Comment |
github-actions
Bot
added
area/uncategorized
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds resource requests and limits for the snapshot-controller's controller and webhook components. Feedback includes the need to update the values.schema.json file via make generate, ensuring the PR title and commits adhere to Conventional Commits and sign-off requirements, and adding securityContext configurations for both components to align with security standards.
| resources: | ||
| limits: | ||
| cpu: 500m | ||
| memory: 512Mi | ||
| requests: | ||
| cpu: 100m | ||
| memory: 128Mi |
There was a problem hiding this comment.
When adding or modifying fields in values.yaml, the values.schema.json file should be updated to ensure the dashboard UI and input validation are in sync. Please run make generate and include the resulting changes in this pull request.
References
- values.schema.json is used for dashboard UI and input validation and is regenerated by make generate.
| controller: | ||
| replicaCount: 2 | ||
| revisionHistoryLimit: 10 | ||
| resources: |
There was a problem hiding this comment.
The PR title and the release note do not follow the Conventional Commits format required by the repository style guide. Additionally, ensure all commits include a Signed-off-by: trailer. The PR title should be in the format type(scope): description (e.g., feat(snapshot-controller): set resources requests/limits), and the release note should also follow the type(scope): description format.
References
- Each commit must follow Conventional Commits format: type(scope): brief description.
- PR body must contain a release note block following the type(scope): human-readable changelog entry format.
| controller: | ||
| replicaCount: 2 | ||
| revisionHistoryLimit: 10 | ||
| resources: |
There was a problem hiding this comment.
The repository style guide recommends flagging missing securityContext configurations. Consider defining podSecurityContext and containerSecurityContext for the controller to enhance the security posture of the deployment.
References
- Flag missing securityContext, containers running as root without justification, and other security-related omissions.
| certManagerIssuerRef: | ||
| name: selfsigned-cluster-issuer | ||
| kind: ClusterIssuer | ||
| resources: |
There was a problem hiding this comment.
Similar to the controller, the webhook deployment is missing a securityContext. It is recommended to add one to adhere to the project's security standards.
References
- Flag missing securityContext, containers running as root without justification, and other security-related omissions.
Aleksei Sviridkin (lexfrei)
added
kind/cleanup
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Both resource blocks are placed correctly under the existing controller and webhook keys, the webhook/controller split is sensible, and E2E passes. The automated make generate / schema reminder doesn't apply — this package has no values.schema.json and no cozyvalues-gen step, and pre-commit is green — and the commit is already signed off. The remaining item is the Conventional Commits title; please retitle to something like chore(snapshot-controller): set resource requests/limits on controller and webhook. The securityContext suggestion is out of scope for a resources-only change and can be a separate PR.
|
I would take the memory half of this. The CPU limits I would rather drop, and the webhook's 100m specifically. That path is synchronous admission, so a ceiling that binds there turns into snapshot creation timeouts rather than into a slow controller. The controller half does not scale with anything so its ceiling is harmless, but I would keep the two consistent rather than explain the difference later. Drop the two |
|
Matthieu ROBIN (@matthieu-robin) myasnikovdaniil asked on 2026-08-15 to drop the two |
3 participants
Summary
The piraeusdatastore snapshot-controller chart (which deploys kubernetes-csi/external-snapshotter) ships with
resources: {}for both the controller and the validation webhook, triggeringno CPU limitconformance warnings on the two Deployments incozy-snapshot-controller.Upstream doesn't publish official sizing either — manifests in the kubernetes-csi repo and the OpenShift fork both leave resources unset. Sized based on a production reference:
snapshot-controllersnapshot-validation-webhookWebhook stays light (HTTP admission validation, same sizing as cert-manager-webhook in #2616); controller gets the production-recommended envelope.
Test plan
helm template . --namespace cozy-snapshot-controllerfrompackages/system/snapshot-controller/rendersresourcesblocks on bothsnapshot-controllerandsnapshot-validation-webhookDeployments (verified locally)no CPU limitwarnings should be goneRelease note
```release-note
Set CPU/memory requests and limits on the snapshot-controller and snapshot-validation-webhook Deployments to clear "no CPU limit" conformance warnings.
```
Summary by CodeRabbit