Repository navigation
[bucket] Add resources requests/limits to s3manager container - #2624
Aleksei Sviridkin (lexfrei) merged 1 commit into
Conversation
![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 addresses a conformance warning regarding missing CPU limits in the s3manager container. By defining explicit resource requests and limits, the deployment now aligns with standard operational requirements, ensuring better resource management and compliance within the cluster. 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
|
github-actions
Bot
added
size/XS
📝 WalkthroughWalkthroughThe ChangesS3Manager Container Resource Configuration
🎯 1 (Trivial) | ⏱️ ~2 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)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/system/bucket/templates/deployment.yaml (1)
32-38: Make resources configurable viavalues.yaml.While
capi-providers-cpproviderdoes hardcode resources, hardcoding is not the established pattern for system packages. Most system packages (backup-controller, backupstrategy-controller, cert-manager, cilium, cluster-autoscaler, etc.) make resources configurable throughvalues.yaml. The bucket chart already usesvalues.yamlextensively for other configuration (bucketName, users, namespace, ingress settings). For consistency, consider moving these resource definitions tovalues.yamland referencing them via{{ .Values.resources.* }}in the template.🤖 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 `@packages/system/bucket/templates/deployment.yaml` around lines 32 - 38, The deployment template currently hardcodes CPU/memory limits and requests; move these into values.yaml and reference them from the template. Add a resources section in values.yaml (e.g. resources.limits.cpu, resources.limits.memory, resources.requests.cpu, resources.requests.memory) with sensible defaults, then replace the hardcoded block in packages/system/bucket/templates/deployment.yaml to read values from .Values.resources (using the existing Helm templating/lookup pattern such as .Values.resources.limits and .Values.resources.requests) so chart users can override resource settings. Ensure defaults exist in values.yaml to preserve current behavior if not overridden.
🤖 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.
Nitpick comments:
In `@packages/system/bucket/templates/deployment.yaml`:
- Around line 32-38: The deployment template currently hardcodes CPU/memory
limits and requests; move these into values.yaml and reference them from the
template. Add a resources section in values.yaml (e.g. resources.limits.cpu,
resources.limits.memory, resources.requests.cpu, resources.requests.memory) with
sensible defaults, then replace the hardcoded block in
packages/system/bucket/templates/deployment.yaml to read values from
.Values.resources (using the existing Helm templating/lookup pattern such as
.Values.resources.limits and .Values.resources.requests) so chart users can
override resource settings. Ensure defaults exist in values.yaml to preserve
current behavior if not overridden.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 464b049d-56c0-47eb-af48-670baa1511ec
📒 Files selected for processing (1)
packages/system/bucket/templates/deployment.yaml
![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 to the bucket deployment template. The review feedback highlights the need for a securityContext to adhere to the principle of least privilege as per the style guide. Additionally, the PR title must be corrected to follow the Conventional Commits format.
| value: {{ $endpoint | quote }} | ||
| - name: SKIP_SSL_VERIFICATION | ||
| value: "true" | ||
| resources: |
There was a problem hiding this comment.
The container is missing a securityContext. According to the repository style guide, all containers should have a securityContext defined to ensure they follow the principle of least privilege and avoid running as root without justification.
securityContext:
allowPrivilegeEscalation: false
capabilities:
drop:
- ALL
runAsNonRoot: true
seccompProfile:
type: RuntimeDefault
resources:References
- Flag missing securityContext, containers running as root without an explicit reason, and hostPath/hostNetwork usage without clear rationale. (link)
| - name: SKIP_SSL_VERIFICATION | ||
| value: "true" | ||
| resources: | ||
| limits: |
There was a problem hiding this comment.
The PR title [bucket] Add resources requests/limits to s3manager container does not follow the required Conventional Commits format. It should be prefixed with a valid type (e.g., feat, fix, chore) followed by the scope in parentheses, such as fix(bucket): add resources requests/limits to s3manager container.
References
- Each commit must follow Conventional Commits format: type(scope): brief description. (link)
Aleksei Sviridkin (lexfrei)
added
the
kind/cleanup
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
Correct fix. bucket is a standalone chart with no vendored upstream, so hardcoding the resources block directly on the s3manager container matches the existing pattern (csi-resizer, capi-providers) and the indentation lands the block as a proper sibling of env. Limits 500m/512Mi over requests 100m/128Mi are sane for a lightweight S3 browser UI. Sign-off is present; the only nit is the PR title not following Conventional Commits, which the labeler keys on.
Non-blocking: making the block configurable via values.yaml is optional — hardcoding is the established convention for these standalone charts.
The s3manager Deployment in the bucket package had no resources block, triggering a 'no CPU limit' conformance warning. Set limits 500m/512Mi and requests 100m/128Mi — comfortable headroom for the lightweight S3 browser UI. Bucket is a standalone chart (no vendored upstream), so the values are set directly in the template, matching the existing pattern in csi-resizer and capi-providers. Signed-off-by: Matthieu <[email protected]>
74cbde1 to
c8b180d
Compare
04a05f6
into
cozystack:main
## Summary The `matchbox` container in `packages/extra/bootbox/templates/matchbox/deployment.yaml` had no `resources` block, triggering a `no CPU limit` conformance warning on the `bootbox-matchbox` Deployment (PXE/iPXE provisioning service). Bootbox is a standalone chart (no vendored upstream), so the values are set directly in the template — same pattern as #2614 (csi-resizer) and #2624 (s3manager). - `limits` : `cpu: 200m`, `memory: 256Mi` - `requests` : `cpu: 50m`, `memory: 128Mi` Modest sizing since matchbox is a lightweight HTTP server serving iPXE profiles and boot assets from ConfigMaps. ## Test plan - [ ] `helm template bootbox . --namespace tenant-root --show-only templates/matchbox/deployment.yaml --set _namespace.host=example.com --set _cluster.solver=letsencrypt` from `packages/extra/bootbox/` renders the `resources` block on the `matchbox` container (verified locally) - [ ] Re-run the conformance/lint tool — the `matchbox has no CPU limit` warning should be gone - [ ] CI passes ### Release note \`\`\`release-note Add CPU and memory requests/limits to the matchbox container of the bootbox Deployment to clear the "no CPU limit" conformance warning. \`\`\` <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Chores** * Added Kubernetes resource limits and requests configuration to optimize container resource allocation and improve deployment stability. [](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2625) <!-- end of auto-generated comment: release notes by coderabbit.ai -->
](https://app.coderabbit.ai/change-stack/cozystack/cozystack/pull/2625){kind=link}
Summary
The
s3managercontainer inpackages/system/bucket/templates/deployment.yamlhad noresourcesblock, triggering ano CPU limitconformance warning on the bucket UI Deployment.The bucket package is a standalone chart (no vendored upstream), so the values are set directly in the template — same pattern as the csi-resizer fix in #2614 and the existing
capi-providers-cpprovidertemplate.limits:cpu: 500m,memory: 512Mirequests:cpu: 100m,memory: 128MiComfortable headroom for the lightweight S3 browser UI while keeping a small baseline request.
Test plan
helm template . --show-only templates/deployment.yamlfrompackages/system/bucket/renders theresourcesblock on thes3managercontainer (verified locally)s3manager has no CPU limitwarning should be goneRelease note
```release-note
Add CPU and memory requests/limits to the s3manager container of the bucket UI Deployment to clear the "no CPU limit" conformance warning.
```
Summary by CodeRabbit