Feat/tests with resource quota - #1389
Conversation
WalkthroughIncreased VM e2e timeouts; tenant install test now sets explicit ResourceQuota ( Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant T as Test Runner
participant H as Helm chart (tenant)
participant K as Kubernetes API
participant RQ as ResourceQuota
participant LR as LimitRange
T->>H: Install tenant chart with resourceQuotas (cpu:60, memory:128Gi, storage:100Gi)
H->>K: Apply ResourceQuota manifest
K-->>RQ: ResourceQuota created/updated
H->>K: Apply LimitRange manifest
K-->>LR: LimitRange created/updated
T->>K: Poll ResourceQuota in tenant ns
K-->>T: Returns memory≈128Gi, storage≈100Gi
T->>K: Inspect LimitRange defaults
K-->>T: Returns container default/defaultRequest values
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks (1 passed, 2 inconclusive)❌ Failed checks (2 inconclusive)
✅ Passed checks (1 passed)
✨ 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. Comment |
There was a problem hiding this comment.
Summary of Changes
Hello IvanHunters, 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 introduces new end-to-end tests to validate resource quota functionality within the system. It also refines the underlying resource allocation logic to allow for more flexible resource limit definitions, and adjusts existing test timeouts to ensure stability with the new resource constraints.
Highlights
- Resource Quota Testing: Introduced end-to-end tests for resource quotas by configuring CPU, memory, and storage limits for a test tenant.
- Resource Allocation Logic Refinement: Updated the resource template logic to no longer automatically set resource limits equal to requests for all resource types, providing more granular control over resource definitions.
- E2E Test Stability Improvements: Increased wait timeouts in VM instance tests to accommodate potential delays introduced by new resource constraints and ensure test stability.
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 (@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 (@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.
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 introduces resource quota support for tenants and adds corresponding end-to-end tests. The changes include updating test scripts to configure tenants with resource quotas and adjusting timeouts, as well as modifying Helm templates to generate ResourceQuota objects. My review identified a high-severity issue in the cozy-lib.resources.sanitize helper template where resource limits for ephemeral-storage and extended resources (like GPUs) are no longer being set. This could lead to resource over-commitment and instability. I've provided a detailed comment with a suggested code change to rectify this while still correctly handling quota keys that do not have limits, such as storage.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
49-51: Fix unit parsing in toFloat (incorrect use of$context)
hasSuffix $k $andtrimSuffix $k $are wrong;$is the root context, not the input string. This breaks parsing like "256Mi" and "200m".- {{- if hasSuffix $k $ -}} - {{- $value = trimSuffix $k $ -}} + {{- if hasSuffix $value $k -}} + {{- $value = trimSuffix $k $value -}}
🧹 Nitpick comments (3)
hack/e2e-apps/vminstance.bats (1)
20-22: Align HR wait with longer DV/PVC waits to reduce flakesThe HR wait at 5s is likely too short compared to the extended DV/PVC waits.
- kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready + kubectl -n tenant-test wait hr vm-disk-$name --timeout=60s --for=condition=readypackages/library/cozy-lib/templates/_resources.tpl (1)
174-184: Confirm callers expect YAML, not a map, from flatten
flattenemits YAML (toYaml) rather than returning a map. Verify all call sites treat the include as YAML text; otherwise, return the map and let callers serialize.Option if you want
flattento return a map:-{{- $out | toYaml }} +{{- $out }}hack/e2e-install-cozystack.bats (1)
192-195: Add a check that ResourceQuota objects are created/enforced for the tenantGreat to specify quotas; add a sanity check that a ResourceQuota appears in tenant-test to catch regressions.
kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + # Verify quota objects surfaced in the tenant namespace + timeout 60 sh -ec 'until kubectl -n tenant-test get resourcequota >/dev/null 2>&1; do sleep 1; done' + kubectl -n tenant-test get resourcequota
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
hack/e2e-apps/vminstance.bats(1 hunks)hack/e2e-install-cozystack.bats(2 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
hack/e2e-install-cozystack.bats (1)
192-195: Confirm Tenant CRD’sspec.resourceQuotaskeys
Check the Tenant CRD (e.g. under config/crd/bases/) or its Go API types to ensurespec.resourceQuotasis defined as a map accepting exactlycpu,memory, andstorage. If the operator actually expects keys likeephemeral-storageorrequests.storage, update the test or spec accordingly.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
126-126: Remove stray blank line for consistency.Keep comments tight to the command block.
-
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
| resourceQuotas: | ||
| cpu: "10000m" | ||
| memory: "16Gi" | ||
| storage: "100Gi" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Structured resourceQuotas look correct; add assertions and confirm CRD schema.
Good move to explicit cpu/memory/storage. Please 1) assert these materialize into a ResourceQuota in tenant-test, and 2) verify the CRD/schema reflects the structured fields.
Add a post-check (after the namespace becomes Active) to ensure requests.* are set as intended:
@@
kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active
+
+ # Assert ResourceQuota requests match expected values
+ kubectl get resourcequota -n tenant-test \
+ -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \
+ | grep -qx '10000m 16Gi 100Gi'Optionally bump this wait to reduce flakiness under quota-enforced clusters:
- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready
+ kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=readyRun this to verify the new API shape is wired through the repo:
🏁 Script executed:
#!/bin/bash
# Verify CRD/schema and templates use structured quotas and requests.*
set -euo pipefail
echo "Searching for structured resourceQuotas fields in CRDs or types..."
rg -n -C3 -e 'resourceQuotas:' -e '\bcpu:\b' -e '\bmemory:\b' -e '\bstorage:\b' --type yaml --type go || true
echo "Confirm cozy-lib emits requests.* (limits removed)..."
rg -n -C2 -e 'requests\.(cpu|memory|storage)' packages/library/cozy-lib || trueLength of output: 2084
Add ResourceQuota assertion and verify CRD schema
- In hack/e2e-install-cozystack.bats (after the namespace wait at ~line 192), insert:
# Assert ResourceQuota requests match expected values kubectl get resourcequota -n tenant-test \ -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ | grep -qx '10000m 16Gi 100Gi'
- Ensure the Tenant CRD schema and Go types include the structured resourceQuotas fields (e.g. in config/crd/**/.yaml and the corresponding api//types.go).
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
192-200: Assert the rendered ResourceQuota and LimitRange match expectations.Add explicit checks so the new structured quotas and defaults are validated.
kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + + # Assert ResourceQuota requests match expected values + kubectl get resourcequota tenant-quota -n tenant-test \ + -o jsonpath='{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}' \ + | grep -qx '16 20Gi 100Gi' + + # Assert LimitRange defaults + kubectl get limitrange tenant-range-limits -n tenant-test \ + -o jsonpath='{range .spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ + | grep -qx '500m 512Mi 100m 128Mi'
🧹 Nitpick comments (3)
packages/apps/tenant/templates/quota.yaml (2)
10-24: Parameterize LimitRange defaults to avoid hard-coding.Expose these via values so tests/clusters can tune without editing templates.
limits: - default: - cpu: "500m" - memory: "512Mi" + cpu: {{ .Values.limitRange.defaults.cpu | default "500m" | quote }} + memory: {{ .Values.limitRange.defaults.memory | default "512Mi" | quote }} defaultRequest: - cpu: "100m" - memory: "128Mi" + cpu: {{ .Values.limitRange.defaultRequest.cpu | default "100m" | quote }} + memory: {{ .Values.limitRange.defaultRequest.memory | default "128Mi" | quote }} type: Container
10-24: Consider decoupling LimitRange from the ResourceQuota toggle.Teams may want defaults even when quotas are off. Gate with a dedicated flag like
.Values.limitRange.enabledinstead of.Values.resourceQuotas.Example (outside this hunk):
{{- if .Values.resourceQuotas }} # ResourceQuota... {{- end }} {{- if .Values.limitRange.enabled }} # LimitRange... {{- end }}hack/e2e-install-cozystack.bats (1)
198-198: Increase wait timeout for tenant HR to reduce flakiness under quotas.- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/apps/tenant/templates/quota.yaml (1)
10-24: Drop the ResourceQuota guard suggestion: this template defines a LimitRange, not a ResourceQuota, and there is no.Values.resourceQuotasin use.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
102-113: Restore limits for non-CPU resources
ephemeral-storage and all other generic resources currently only set requests, leaving limits unset (allowing unbounded usage). Add matchingset $output.limitscalls:{{- else if eq $k "ephemeral-storage" }} $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) + $_ := set $output.limits $k ($v | toString) {{- else }} $_ := set $output.requests $k $v + $_ := set $output.limits $k ($v | toString) {{- end }}
🧹 Nitpick comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
58-86: Update docstring to match behavior (memory/ephemeral requests reduced by ratios).The comment claims only CPU requests are reduced; code also reduces memory and ephemeral-storage requests. Align docs or code.
Proposed wording: “All resources set limits from input values. Requests are equal to limits, except cpu, memory, and ephemeral-storage, whose requests are reduced by their respective allocation ratios.”
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/library/cozy-lib/templates/_resources.tpl(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
174-184: flatten() wrapper removal safe; only one call site
Only oneinclude "cozy-lib.resources.flatten"found, and its consumer underspec.hardexpects flat entries.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
192-200: Assert ResourceQuota materializes (requests.*) and give HR more time.Great to switch to structured quotas. Please 1) wait longer for tenant HR, and 2) assert the rendered ResourceQuota matches the expected requests.* values; optionally verify a LimitRange exists. This was requested earlier as well.
Apply:
- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + + # Assert ResourceQuota requests match expected values + kubectl get resourcequota -n tenant-test \ + -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ + | grep -qx '18000m 32Gi 100Gi' + + # Ensure a LimitRange exists (added alongside ResourceQuota) + kubectl get limitrange -n tenant-test -o jsonpath='{.items[0].metadata.name}' | grep -q .To double-check the API shape through the repo, run:
#!/bin/bash # Verify structured quotas and requests.* wiring set -euo pipefail echo "[1/3] Tenant CRD/types use structured resourceQuotas..." rg -n -C3 -e 'resourceQuotas:' -e '\bcpu:\b' -e '\bmemory:\b' -e '\bstorage:\b' --type yaml --type go || true echo "[2/3] Cozy-lib emits requests.* (no limits for memory/storage)..." rg -n -C2 -e 'requests\.(cpu|memory|storage)' packages/library || true echo "[3/3] Ensure we did not reintroduce hard.limits.* inadvertently..." rg -n -C2 -e '\.spec\.hard\.limits\.' -e 'limits\.(memory|storage)' --type yaml packages || true
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
126-141: Reduce flakiness: increase HR readiness timeouts for root tenant components.etcd/ingress/monitoring/seaweedfs can take longer on CI; current 2–4m waits are tight.
Apply:
- kubectl wait hr/etcd hr/ingress hr/tenant-root hr/seaweedfs -n tenant-root --timeout=4m --for=condition=ready + kubectl wait hr/etcd hr/ingress hr/tenant-root hr/seaweedfs -n tenant-root --timeout=10m --for=condition=ready @@ - if ! kubectl wait hr/monitoring -n tenant-root --timeout=2m --for=condition=ready; then + if ! kubectl wait hr/monitoring -n tenant-root --timeout=5m --for=condition=ready; then @@ - if ! kubectl wait hr/seaweedfs-system -n tenant-root --timeout=2m --for=condition=ready; then + if ! kubectl wait hr/seaweedfs-system -n tenant-root --timeout=5m --for=condition=ready; then
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats(2 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
42-56: Fix: toFloat uses root$instead of local$value, breaking unit parsing.This prevents suffix detection (e.g., "Mi", "m") and will fail on quantities like "256Mi" or "200m".
Apply:
- {{- range $k, $v := merge $base2 $base10 -}} - {{- if hasSuffix $k $ -}} - {{- $value = trimSuffix $k $ -}} - {{- $unit = $v -}} - {{- end -}} - {{- end -}} + {{- range $k, $v := merge $base2 $base10 -}} + {{- if hasSuffix $value $k -}} + {{- $value = trimSuffix $k $value -}} + {{- $unit = $v -}} + {{- end -}} + {{- end -}}
58-86: Docs drift: sanitize contract/examples don’t match implementation.Text says only CPU requests are reduced and all non-CPU equal limits; code reduces memory/ephemeral requests and omits some limits. Update docs or implementation for consistency.
Apply minimal doc fix (if current behavior is intended):
- All resources are returned with equal **requests** and **limits**, except for - **cpu**, whose *request* is reduced by the CPU-allocation ratio obtained from - `cozy-lib.resources.cpuAllocationRatio`. + Limits mirror input values. Requests may be reduced by allocation ratios: + - cpu: reduced by `cozy-lib.resources.cpuAllocationRatio` + - memory: reduced by `cozy-lib.resources.memoryAllocationRatio` + - ephemeral-storage: reduced by `cozy-lib.resources.ephemeralStorageAllocationRatio` + Other resources default to requests == limits.And update the example accordingly (show reduced memory/ephemeral and both limits/requests present).
Also applies to: 132-149
♻️ Duplicate comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
97-101: Fix regression: hardcoded CPU downscale ignores cpu-allocation-ratio.Dividing by 3 bypasses the configured ratio and breaks the contract.
Apply:
-{{- $cpuRequestF64 := divf $vcpuRequestF64 3 }} +{{- $cpuRequestF64 := divf $vcpuRequestF64 $cpuAllocationRatio }}
🧹 Nitpick comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
102-106: Memory request format/rounding: consider keeping units consistent with limits.Requests are emitted as raw bytes (int), limits keep original units (e.g., Mi). Optional: normalize requests to Mi to match limits.
Apply:
-{{- $_ := set $output.requests $k ($memoryRequestF64 | int | toString ) }} +{{- $memBytes := $memoryRequestF64 | int -}} +{{- $memMi := div $memBytes 0x1p20 -}} +{{- $_ := set $output.requests $k (printf "%dMi" $memMi) }}
76-86: Nit: use canonical GPU resource name in docs.Replace
devices.com/nvidiawithnvidia.com/gpufor accuracy.Apply:
- devices.com/nvidia: "1" + nvidia.com/gpu: "1"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/apps/tenant/templates/quota.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps/tenant/templates/quota.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
174-184: Ignore wrapper concern: flatten change is safe—no callers expect a top-levelresourceQuotas:key
Onlypackages/apps/tenant/templates/quota.yamlusescozy-lib.resources.flattenand correctly indents the plain map; no other templates include or rely on aresourceQuotas:wrapper.Likely an incorrect or invalid review comment.
107-113: Restore limits for non-CPU resources
Thecpuandmemorybranches correctly set bothoutput.requestsandoutput.limits, but theephemeral-storageand fallback (else) branches only populateoutput.requests. Mirror the other branches by adding{{- $_ := set $output.limits $k ($v | toString) }}in both the
ephemeral-storageandelseblocks.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
111-113: Extended resources (e.g., nvidia.com/gpu) must be set in limits, not requests.Currently all non-special keys go to requests only. For extended resources, Kubernetes expects limits (requests are implicitly set). As-is, GPUs and similar won’t be scheduled.
Apply:
-{{- else }} -{{- $_ := set $output.requests $k $v }} +{{- else }} +{{- if contains "/" $k }} +{{- /* extended resource: limits only */}} +{{- $_ := set $output.limits $k ($v | toString) }} +{{- else }} +{{- /* regular scalar resource: mirror to both */}} +{{- $_ := set $output.requests $k ($v | toString) }} +{{- $_ := set $output.limits $k ($v | toString) }} +{{- end }}
🧹 Nitpick comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
58-86: Comment drift vs implementation.The header comment claims only CPU requests are reduced and all others mirror limits; the code also scales memory/ephemeral and omits limits for some keys. Update the docblock or align the code.
Happy to draft a precise, up-to-date example block once behavior is confirmed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/library/cozy-lib/templates/_resources.tpl(2 hunks)
🔇 Additional comments (4)
packages/library/cozy-lib/templates/_resources.tpl (4)
102-106: Clarify intent: memory requests are scaled by memoryAllocationRatio.Docs say only CPU requests are reduced, but code reduces memory as well. If intentional, update comments and release notes; if not, set memory requests to the limit.
Option to match docs:
-{{- $vMemoryRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} -{{- $memoryRequestF64 := divf $vMemoryRequestF64 $memoryAllocationRatio }} -{{- $_ := set $output.requests $k ($memoryRequestF64 | int | toString ) }} +{{- $_ := set $output.requests $k ($v | toString) }} {{- $_ := set $output.limits $k ($v | toString) }}
107-111: Ephemeral-storage limit is dropped.Only requests are emitted; this removes the limit and may allow unbounded ephemeral usage. If limits are desired (common), also set limits to the input value.
Apply:
{{- else if eq $k "ephemeral-storage" }} {{- $vEphemeralStorageRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} {{- $ephemeralStorageRequestF64 := divf $vEphemeralStorageRequestF64 $ephemeralStorageAllocationRatio }} {{- $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) }} +{{- $_ := set $output.limits $k ($v | toString) }}
151-157: Preset merge precedence likely inverted.deepCopy $resources | mergeOverwrite $presetMap causes preset to override user input; expected is “user overrides preset”. Consider flipping.
Apply if confirmed:
-{{- $mergedMap := deepCopy $resources | mergeOverwrite $presetMap }} +{{- $mergedMap := deepCopy $presetMap | mergeOverwrite $resources }}
174-184: Flatten usage verified in quota template — the call in packages/apps/tenant/templates/quota.yaml now emits top-level requests.* and limits.* under spec.hard, matching the Kubernetes ResourceQuota schema.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
packages/apps/tenant/templates/quota.yaml (3)
10-24: Parameterize LimitRange defaults; consider deriving from quota.Hard-coded defaults (cpu=4, memory=4Gi, requests=100m/128Mi) may not fit small tenants. Make them configurable and optionally clamp to a fraction of .Values.resourceQuotas.
apiVersion: v1 kind: LimitRange metadata: name: tenant-range-limits namespace: {{ include "tenant.name" . }} spec: limits: - - default: - cpu: "4" - memory: "4Gi" - defaultRequest: - cpu: "100m" - memory: "128Mi" + - default: + cpu: {{ default "4" .Values.limitRange.default.cpu | quote }} + memory: {{ default "4Gi" .Values.limitRange.default.memory | quote }} + defaultRequest: + cpu: {{ default "100m" .Values.limitRange.defaultRequest.cpu | quote }} + memory: {{ default "128Mi" .Values.limitRange.defaultRequest.memory | quote }} type: Container
16-24: Add optional min/max to prevent extremes.Adding min and/or max guards helps avoid accidental oversized or undersized Pods when users omit resources.
spec: limits: - default: cpu: {{ default "4" .Values.limitRange.default.cpu | quote }} memory: {{ default "4Gi" .Values.limitRange.default.memory | quote }} defaultRequest: cpu: {{ default "100m" .Values.limitRange.defaultRequest.cpu | quote }} memory: {{ default "128Mi" .Values.limitRange.defaultRequest.memory | quote }} + max: + cpu: {{ default "" .Values.limitRange.max.cpu | quote }} + memory: {{ default "" .Values.limitRange.max.memory | quote }} + min: + cpu: {{ default "" .Values.limitRange.min.cpu | quote }} + memory: {{ default "" .Values.limitRange.min.memory | quote }} type: Container
16-24: Consider ephemeral-storage defaults.Many workloads write to emptyDir; defaultRequest for ephemeral-storage improves scheduling predictability.
defaultRequest: cpu: {{ default "100m" .Values.limitRange.defaultRequest.cpu | quote }} memory: {{ default "128Mi" .Values.limitRange.defaultRequest.memory | quote }} + ephemeral-storage: {{ default "64Mi" .Values.limitRange.defaultRequest.ephemeralStorage | quote }}packages/library/cozy-lib/templates/_resources.tpl (4)
97-101: Normalize CPU request to milli-cores for readability.Using decimals (e.g., "0.2") is valid but less consistent with kubectl output and common practice. Emit “m”.
-{{- $_ := set $output.requests $k ($cpuRequestF64 | toString) }} +{{- $cpuMilli := mulf $cpuRequestF64 1000 | int }} +{{- $_ := set $output.requests $k (printf "%dm" $cpuMilli) }}
102-107: Preserve memory units in requests; avoid raw bytes.Plain integers are allowed but hard to read and brittle for tests. Emit Mi to mirror limits.
-{{- $vMemoryRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} -{{- $memoryRequestF64 := divf $vMemoryRequestF64 $memoryAllocationRatio }} -{{- $_ := set $output.requests $k ($memoryRequestF64 | int | toString ) }} +{{- $vMemoryRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} +{{- $memoryRequestF64 := divf $vMemoryRequestF64 $memoryAllocationRatio }} +{{- $memMi := divf $memoryRequestF64 1048576 | floor | int }} +{{- $_ := set $output.requests $k (printf "%dMi" $memMi) }}
111-113: Ensure non-core resources serialize as strings.If $v is numeric, requests.* may become numbers. Force string for consistency.
-{{- $_ := set $output.requests $k $v }} +{{- $_ := set $output.requests $k ($v | toString) }}
174-184: Deterministic key order in flatten output.Map iteration order is random. Sorting improves reproducibility and test stability.
-{{- range $section, $values := $res }} - {{- range $k, $v := $values }} - {{- $key := printf "%s.%s" $section $k }} - {{- $_ := set $out $key $v }} - {{- end }} -{{- end }} +{{- $sections := list "limits" "requests" }} +{{- range $section := $sections }} + {{- with index $res $section }} + {{- range $k := keys . | sortAlpha }} + {{- $key := printf "%s.%s" $section $k }} + {{- $_ := set $out $key (index . $k) }} + {{- end }} + {{- end }} +{{- end }} {{- $out | toYaml }}hack/e2e-install-cozystack.bats (3)
192-197: Add assertions for ResourceQuota and LimitRange.Validate that quotas and defaults materialize as intended to catch regressions.
monitoring: false resourceQuotas: cpu: "20" memory: "32Gi" storage: "100Gi" seaweedfs: false EOF - kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + + # Assert ResourceQuota keys exist + kubectl get resourcequota -n tenant-test tenant-quota -o jsonpath='{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}' + + # Assert LimitRange defaults exist + kubectl get limitrange -n tenant-test tenant-range-limits -o jsonpath='{.spec.limits[0].defaultRequest.cpu}{" "}{.spec.limits[0].defaultRequest.memory}{" "}{.spec.limits[0].default.cpu}{" "}{.spec.limits[0].default.memory}{"\n"}'If you want me to pin expected values precisely (accounting for allocation ratios and units), say the word and I’ll wire them in.
198-200: Bump readiness timeout to reduce flakes.HelmRelease convergence for a fresh tenant can exceed 1m on CI.
- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready
124-131: Optional: wait for all root apps uniformly.Use a single loop to wait for expected HRs and unify timeouts.
- timeout 60 sh -ec 'until kubectl get hr -n tenant-root etcd ingress monitoring seaweedfs tenant-root >/dev/null 2>&1; do sleep 1; done' - kubectl wait hr/etcd hr/ingress hr/tenant-root hr/seaweedfs -n tenant-root --timeout=4m --for=condition=ready + timeout 120 sh -ec 'until kubectl get hr -n tenant-root etcd ingress monitoring seaweedfs tenant-root seaweedfs-system >/dev/null 2>&1; do sleep 1; done' + kubectl get hr -n tenant-root etcd ingress monitoring seaweedfs seaweedfs-system tenant-root \ + -o name | xargs -r -n1 -I{} kubectl wait -n tenant-root --timeout=5m --for=condition=ready {}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/library/cozy-lib/templates/_resources.tpl (3)
104-107: Ephemeral-storage limits dropped; add back to keep requests == limits policy (scaled request).Only requests are set; limits are omitted. This likely breaks downstream expectations and diverges from the examples.
Apply:
{{- else if eq $k "ephemeral-storage" }} {{- $vEphemeralStorageRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} {{- $ephemeralStorageRequestF64 := divf $vEphemeralStorageRequestF64 $ephemeralStorageAllocationRatio }} -{{- $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) }} +{{- $ephReqBytes := $ephemeralStorageRequestF64 | int }} +{{- $ephReqMi := div $ephReqBytes 1048576 }} +{{- $_ := set $output.requests $k (printf "%dMi" $ephReqMi) }} +{{- $_ := set $output.limits $k ($v | toString) }}
151-154: Preset merge order overrides user values; reverse it.The preset currently overwrites explicit .Values.resources. Users’ overrides should win.
Apply:
{{- $presetMap := include "cozy-lib.resources.unsanitizedPreset" $preset | fromYaml }} -{{- $mergedMap := deepCopy $resources | mergeOverwrite $presetMap }} +{{- $mergedMap := deepCopy $presetMap | mergeOverwrite $resources }} {{- include "cozy-lib.resources.sanitize" (list $mergedMap $global) }}
42-56: toFloat uses wrong variables/order in hasSuffix/trimSuffix.It checks/trim on
$(root) and reverses arg order; unit parsing will be wrong in many contexts.Apply:
- {{- range $k, $v := merge $base2 $base10 -}} - {{- if hasSuffix $k $ -}} - {{- $value = trimSuffix $k $ -}} - {{- $unit = $v -}} - {{- end -}} - {{- end -}} + {{- range $k, $v := merge $base2 $base10 -}} + {{- if hasSuffix $value $k -}} + {{- $value = trimSuffix $k $value -}} + {{- $unit = $v -}} + {{- end -}} + {{- end -}}
♻️ Duplicate comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
97-99: Restore CPU requests (regression vs. contract).CPU branch writes only limits; requests are missing and should be scaled by cpuAllocationRatio. This reintroduces the issue previously flagged and breaks consumers expecting requests.cpu.
Apply:
-{{- if eq $k "cpu" }} -{{- $_ := set $output.limits $k ($v | toString) }} +{{- if eq $k "cpu" }} +{{- $vcpuRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} +{{- $cpuRequestF64 := divf $vcpuRequestF64 $cpuAllocationRatio }} +{{- $cpuMilli := mulf $cpuRequestF64 1000 | int }} +{{- $_ := set $output.requests $k (printf "%dm" $cpuMilli) }} +{{- $_ := set $output.limits $k ($v | toString) }}
🧹 Nitpick comments (3)
packages/library/cozy-lib/templates/_resources.tpl (3)
100-103: Memory request formatting: avoid raw bytes; preserve units when possible.Current code emits byte counts for requests (hard to read). If ratio is 1, pass limit through; otherwise, format to Mi.
Apply:
{{- $vMemoryRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} {{- $memoryRequestF64 := divf $vMemoryRequestF64 $memoryAllocationRatio }} -{{- $_ := set $output.requests $k ($memoryRequestF64 | int | toString ) }} -{{- $_ := set $output.limits $k ($v | toString) }} +{{- if eq $memoryAllocationRatio 1.0 }} +{{- $_ := set $output.requests $k ($v | toString) }} +{{- else }} +{{- $memReqBytes := $memoryRequestF64 | int }} +{{- $memReqMi := div $memReqBytes 1048576 }} +{{- $_ := set $output.requests $k (printf "%dMi" $memReqMi) }} +{{- end }} +{{- $_ := set $output.limits $k ($v | toString) }}
42-56: Optional: iterate suffixes in deterministic order to avoid ambiguity.Maps are unordered; prefer an ordered list of suffixes (Ki..Ei, then E..m) to avoid inconsistent matches.
Sketch:
- {{- range $k, $v := merge $base2 $base10 -}} + {{- $units := merge $base2 $base10 -}} + {{- $ordered := list "Ki" "Mi" "Gi" "Ti" "Pi" "Ei" "E" "P" "T" "G" "M" "k" "m" -}} + {{- range $ordered -}} + {{- $k := . -}} + {{- $v := index $units $k -}} {{- if hasSuffix $value $k -}} {{- $value = trimSuffix $k $value -}} {{- $unit = $v -}} {{- end -}} {{- end -}}
58-86: Docstring currently contradicts implementation; align after code fix.After restoring CPU requests and limits for non-CPU, the comment will be correct except for memory/ephemeral ratios; if you intend to keep those ratios configurable, add a brief note that requests for memory/ephemeral-storage may be reduced by their allocation ratios (defaults: 1 and 40).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
180-181: LGTM: flatten now emits a simple flattened map.Change is clear and aligns with the stated “flattened map” behavior.
108-110: Remove explicit limit assignment for extended resources
Kubernetes permits setting only a request for an extended resource; limits aren’t mandatory, and if you set only a limit the request is automatically copied to match. Requests and limits must be equal only when both are explicitly defined — no change needed here.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
197-199: Increase tenant HR wait to 5m to reduce flakiness under quota-enabled clusters.Same suggestion as earlier review; quotas/limitranges can slow reconciliation.
- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
192-194: Consider adding a CPU quota to avoid unbounded CPU requests.If the intent is to enforce CPU as well, include it; otherwise pods can request CPU without namespace-level accounting.
resourceQuotas: + cpu: "10000m" memory: "32Gi" storage: "100Gi"To confirm whether the template maps CPU into requests.* or limits.* (recent cozy-lib changes), run:
#!/bin/bash # Inspect quota template emission for CPU vs requests.* rg -n -C3 -e 'kind:\s*ResourceQuota' -e 'requests\.(cpu|memory|storage)' -e 'limits\.cpu' packages apps config api 2>/dev/null || true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
hack/e2e-install-cozystack.bats (1)
126-126: No-op whitespace change — OK to keep or squash later.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/library/cozy-lib/templates/_resources.tpl (3)
97-98: CPU dropped from sanitized output (likely regression).The
if eq $k "cpu"branch is empty, so CPU is omitted from both requests and limits. This contradicts the contract in this file and likely breaks consumers (e.g., flatten won’t emit requests.cpu/limits.cpu).Proposed fix (keep old behavior: limit=$v, request scaled by CPU allocation ratio):
{{- range $k, $v := $args }} {{- if eq $k "cpu" }} +{{- $vcpuRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} +{{- $cpuRequestF64 := divf $vcpuRequestF64 $cpuAllocationRatio }} +{{- $cpuMilli := mulf $cpuRequestF64 1000 | int }} +{{- $_ := set $output.requests $k (printf "%dm" $cpuMilli) }} +{{- $_ := set $output.limits $k ($v | toString) }} {{- else if eq $k "memory" }}
49-53: Unit parsing bug: hasSuffix arguments reversed; quantity parsing will fail.
hasSuffix $k $checks if the unit (e.g., "Mi") ends with the value (e.g., "256Mi"); it should be the other way around. As written,toFloatwon’t strip units andfloat64on values like "256Mi" will error.Apply:
- {{- if hasSuffix $k $ -}} + {{- if hasSuffix $ $k -}} {{- $value = trimSuffix $k $ -}} {{- $unit = $v -}} {{- end -}}
151-152: Preset merge order overrides user values (likely wrong precedence).
mergeOverwritewith$resourcesfirst and$presetMapsecond makes presets override user inputs. Usually presets should fill only missing keys, and user values should win.Apply:
-{{- $mergedMap := deepCopy $resources | mergeOverwrite $presetMap }} +{{- $mergedMap := mergeOverwrite (deepCopy $presetMap) $resources }}
🧹 Nitpick comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
103-110: Non-memory resources only set requests; limits missing; clarify intent or align behavior.ephemeral-storage (and the generic else path) populate only requests. If intentional (move away from limits), update docs and ensure all consumers expect requests-only for these keys. If not, set limits=$v to preserve the documented contract.
If you want to set limits as well:
{{- else if eq $k "ephemeral-storage" }} {{- $vEphemeralStorageRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} {{- $ephemeralStorageRequestF64 := divf $vEphemeralStorageRequestF64 $ephemeralStorageAllocationRatio }} {{- $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) }} + {{- $_ := set $output.limits $k ($v | toString) }} {{- else }} -{{- $_ := set $output.requests $k $v }} +{{- $_ := set $output.requests $k $v }} +{{- $_ := set $output.limits $k $v }} {{- end }}
58-86: Docs out of sync with behavior.Comment block promises “equal requests and limits (except cpu)”. Current code omits CPU entirely and doesn’t set limits for several resources. Update examples and narrative to match actual behavior, or adjust code to meet the documented contract.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
170-180: Verifyflattencall sites: Automated searches returned no references tocozy-lib.resources.flattenin.tplor.yamlfiles—please manually confirm there are no dynamic or indirect usages that expect a map rather than a YAML string and update them accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hack/e2e-install-cozystack.bats (1)
34-38: HR readiness failures don’t fail the test.
The branch echoes but never exits non‑zero, so the test can pass with unready HRs.# Fail the test if any HelmRelease is not Ready if kubectl get hr -A | grep -v " True " | grep -v NAME; then kubectl get hr -A echo "Some HelmReleases failed to reconcile" >&2 + exit 1 fi
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
192-194: Assert ResourceQuota materializes with expected requests. values.*
Right now the test sets quotas but never verifies them. Add a post‑namespace wait that asserts requests.memory=64Gi and requests.storage=100Gi exist in tenant-test.Apply after the namespace wait:
kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + + # Wait for ResourceQuota to appear and assert values + timeout 60 sh -ec 'until [ "$(kubectl get resourcequota -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' + kubectl get resourcequota -n tenant-test \ + -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ + | grep -qx '64Gi 100Gi'
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
197-199: Increase waits to reduce e2e flakiness under quotas.
1m/20s is tight once quota controllers and namespace init kick in.- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready - kubectl wait namespace tenant-test --timeout=20s --for=jsonpath='{.status.phase}'=Active + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready + kubectl wait namespace tenant-test --timeout=2m --for=jsonpath='{.status.phase}'=Active
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats(2 hunks)
🔇 Additional comments (2)
hack/e2e-install-cozystack.bats (2)
126-126: No-op whitespace change.
No action needed.
192-194: Confirm CRD/types support structured resourceQuotas
The e2e test at hack/e2e-install-cozystack.bats (192–194) now assertsrequests.memoryandrequests.storageunderresourceQuotas, but no matching schema appears in your CRD (e.g. config/crd/bases/…) nor in the Go TenantSpec type (pkg/apis/.../tenant_types.go). Verify and extend both the CRD schema and API types to includeresourceQuotas.requests.memoryandresourceQuotas.requests.storage.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/library/cozy-lib/templates/_resources.tpl (2)
111-113: Ensure values are serialized as strings for non-CPU resources.ResourceQuota expects resource.Quantity strings. If a user supplies an unquoted integer (e.g.,
nvidia.com/gpu: 1), this will render as a YAML number and may be rejected by the API. Cast to string.-{{- $_ := set $output.requests $k $v }} +{{- $_ := set $output.requests $k ($v | toString) }}
151-156: Flip mergeOverwrite arguments to apply user values over presets.Sprig’s
mergeOverwritegives precedence to keys in later arguments, sodeepCopy $resources | mergeOverwrite $presetMapcurrently lets preset values override user inputs (docs.pydio.com). Swap the order to merge defaults first, then overlay user data:
-{{- $mergedMap := deepCopy $resources | mergeOverwrite $presetMap }} +{{- $mergedMap := deepCopy $presetMap | mergeOverwrite $resources }}
🧹 Nitpick comments (5)
packages/apps/tenant/templates/quota.yaml (2)
10-24: Make defaults configurable (avoid hardcoding).Expose these defaults via chart values so tenants can tune without forking.
metadata: - name: tenant-range-limits + name: tenant-range-limits namespace: {{ include "tenant.name" . }} spec: limits: - default: - cpu: "250m" - memory: "1Gi" + cpu: {{ .Values.limitRange.defaults.cpu | default "250m" | quote }} + memory: {{ .Values.limitRange.defaults.memory | default "1Gi" | quote }} defaultRequest: - cpu: "100m" - memory: "128Mi" + cpu: {{ .Values.limitRange.defaultRequests.cpu | default "100m" | quote }} + memory: {{ .Values.limitRange.defaultRequests.memory | default "128Mi" | quote }} type: Container
10-24: Optional: default ephemeral-storage when quota is set.If you plan to quota requests.ephemeral-storage, add a defaultRequest to prevent zero-request pods from bypassing quota.
- default: # … defaultRequest: cpu: {{ .Values.limitRange.defaultRequests.cpu | default "100m" | quote }} memory: {{ .Values.limitRange.defaultRequests.memory | default "128Mi" | quote }} + {{- if hasKey .Values.resourceQuotas "ephemeral-storage" }} + ephemeral-storage: {{ .Values.limitRange.defaultRequests.ephemeralStorage | default "128Mi" | quote }} + {{- end }} type: Containerpackages/library/cozy-lib/templates/_resources.tpl (3)
98-101: Optional: prefer millicores formatting to avoid float artifacts.
0.3333333333vs333m. Millicores tend to be clearer and avoid scientific notation risks.-{{- $_ := set $output.requests $k ($cpuRequestF64 | toString) }} +{{- $cpuMilli := mulf $cpuRequestF64 1000 | int }} +{{- $_ := set $output.requests $k (printf "%dm" $cpuMilli) }}
103-110: Optional: normalize memory/ephemeral units for readability.You downcast to raw bytes (
int) which is valid but hard to read in manifests. Consider rounding to Mi (or preserving input unit) for requests to keep human-friendly values.Example approach:
-{{- $_ := set $output.requests $k ($memoryRequestF64 | int | toString ) }} +{{- $reqMi := divf $memoryRequestF64 0x1p20 | ceil | int }} +{{- $_ := set $output.requests $k (printf "%dMi" $reqMi) }}And similarly for ephemeral-storage.
118-149: Docs drift in example.The example still shows
limits.ephemeral-storage, but the implementation no longer sets limits for non-CPU resources. Update the comment to match behavior to prevent confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/e2e-install-cozystack.bats
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/apps/tenant/templates/quota.yaml (1)
10-24: LimitRange defaults: good addition.Adds sane default requests/limits so ResourceQuota accounting works reliably even when workloads omit resources. Looks consistent with the new flattened quota map.
packages/library/cozy-lib/templates/_resources.tpl (1)
183-183: No remaining call sites depend onresourceQuotas:wrapper. flatten is only used inpackages/apps/tenant/templates/quota.yaml, which correctly ingests the bare map.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
hack/e2e-install-cozystack.bats (2)
200-205: LGTM: ResourceQuota assertions added as previously requestedThe wait + jsonpath + grep check for memory/storage is in place and matches earlier review guidance.
206-209: LGTM: LimitRange defaults assertion addedThe defaults/requests check matches the intended values.
🧹 Nitpick comments (2)
hack/e2e-install-cozystack.bats (2)
200-205: Also assert CPU and make the check resilient to unit variantsAdd CPU to the assertion and accept both 60 and 60000m to avoid flakes.
- kubectl get quota -n tenant-test \ - -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ - | grep -qx '64Gi 100Gi' + # Assert requests.{cpu,memory,storage}; accept 60 or 60000m for CPU + kubectl get quota -n tenant-test \ + -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ + | egrep -qx '60 64Gi 100Gi|60000m 64Gi 100Gi'
206-209: Avoid race: wait for LimitRange to exist before asserting fieldsA brief existence wait reduces flakiness on slower clusters.
+ # Wait for LimitRange to appear + timeout 60 sh -ec 'until [ "$(kubectl get limitrange -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' # Assert LimitRange defaults for containers kubectl get limitrange -n tenant-test \ -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ | grep -qx '250m 1Gi 100m 128Mi'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/apps/tenant/templates/quota.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
hack/e2e-install-cozystack.bats (2)
126-126: No-op whitespace changeNo functional impact.
192-195: Structured resourceQuotas look good; confirm CPU value representationKubernetes may surface requests.cpu as either “60” or “60000m”. Make sure downstream assertions tolerate both, or add an explicit CPU check accordingly.
| {{- $_ := set $output.limits $k ($v | toString) }} | ||
| {{- else }} | ||
| {{- $_ := set $output.requests $k $v }} | ||
| {{- $_ := set $output.limits $k $v }} |
There was a problem hiding this comment.
Is there any reason for removing limits?
44fd306 to
ae8c804
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
93-95: sanitize() now rejects nestedrequests/limitssections
Audit everyinclude "cozy-lib.resources.sanitize"anddefaultingSanitizecall and ensure you’re passing a flat map (wrap nested.Values.resourceswithcozy-lib.resources.flattenor equivalent) to avoid hard failures.
e.g. packages/system/keycloak/templates/sts.yaml:71 still passes.Values.resources.limitsdirectly.
♻️ Duplicate comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
107-113: Missing limits for non-CPU resources (breaks extended resources semantics).In sanitize(), you don’t set limits for "ephemeral-storage" and the generic else branch. For extended resources (e.g., GPUs), Kubernetes requires requests == limits (integers). Emitting only requests can cause admission failures and scheduling bugs. Also re-add the ephemeral-storage limit to align with the documented contract and examples.
Apply:
{{- else if eq $k "ephemeral-storage" }} {{- $vEphemeralStorageRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} {{- $ephemeralStorageRequestF64 := divf $vEphemeralStorageRequestF64 $ephemeralStorageAllocationRatio }} -{{- $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) }} +{{- $_ := set $output.requests $k ($ephemeralStorageRequestF64 | int | toString) }} +{{- $_ := set $output.limits $k ($v | toString) }} {{- else }} -{{- $_ := set $output.requests $k $v }} +{{- /* For extended resources and others, requests must equal limits */}} +{{- $_ := set $output.requests $k ($v | toString) }} +{{- $_ := set $output.limits $k ($v | toString) }} {{- end }}
🧹 Nitpick comments (9)
packages/library/cozy-lib/templates/_resources.tpl (2)
58-86: Docs and examples no longer match behavior.Header comment promises limits for all resources except CPU, but current code omits limits for some keys. Either restore limits (preferred; see previous comment) or update docs/examples for consistency.
98-101: Optional: normalize CPU requests to millicores for readability.Kubernetes accepts decimals (e.g., 0.2), but “m” is the common convention and avoids float formatting surprises.
-{{- $_ := set $output.requests $k ($cpuRequestF64 | toString) }} +{{- $cpuMilli := mulf $cpuRequestF64 1000 | int }} +{{- $_ := set $output.requests $k (printf "%dm" $cpuMilli) }}hack/e2e-install-cozystack.bats (3)
200-205: Assert CPU quota too (handle either decimals or millicores).Currently you only validate memory/storage. Add CPU to catch regressions and format differences.
kubectl get quota -n tenant-test \ -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ | grep -qx '68719476736 100Gi' + + # Assert CPU requests quota (accept decimals or millicores) + kubectl get quota -n tenant-test \ + -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{"\n"}{end}' \ + | grep -Eq '^(6(\.0+)?|6000m)$'
206-209: Also assert ephemeral-storage defaultRequest from LimitRange.This ensures the new LimitRange covers ephemeral storage as intended.
kubectl get limitrange -n tenant-test \ -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ | grep -qx '250m 1Gi 100m 128Mi' + + # Ephemeral-storage defaultRequest should also be present + kubectl get limitrange -n tenant-test \ + -o jsonpath='{range .items[*].spec.limits[*]}{.defaultRequest.ephemeral-storage}{"\n"}{end}' \ + | grep -qx '128Mi'
198-199: Consider a longer wait for tenant HelmRelease to reduce flakes.1 minute can be tight under load. Suggest 5m to match other waits.
- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=readypackages/apps/tenant/templates/quota.yaml (4)
18-24: Parameterize hardcoded defaults via chart values.Expose these defaults under .Values.limitRange.{default,defaultRequest} with fallbacks to retain current behavior. Keeps tests stable while allowing operators to tune per-cluster.
Apply this diff:
- - default: - cpu: "250m" - memory: "1Gi" + - default: + cpu: {{ default "250m" (.Values.limitRange.default.cpu | default "") | quote }} + memory: {{ default "1Gi" (.Values.limitRange.default.memory | default "") | quote }} defaultRequest: - cpu: "100m" - memory: "128Mi" - ephemeral-storage: "128Mi" + cpu: {{ default "100m" (.Values.limitRange.defaultRequest.cpu | default "") | quote }} + memory: {{ default "128Mi" (.Values.limitRange.defaultRequest.memory | default "") | quote }} + ephemeral-storage: {{ default "128Mi" (.Values.limitRange.defaultRequest.ephemeralStorage | default "") | quote }}
18-25: Consider adding an ephemeral-storage default limit (not only request).If a ResourceQuota tracks limits.ephemeral-storage, Pods without a limit may be rejected. Setting default to match the request is a safe baseline.
Apply this diff:
- default: cpu: "250m" memory: "1Gi" + ephemeral-storage: "128Mi"If quotas won’t include limits.ephemeral-storage, feel free to skip; otherwise this avoids admission failures.
13-16: Add standard labels for discoverability.Labels help ops tooling and debugging (e.g., kubectl, dashboards).
Apply this diff:
metadata: name: tenant-range-limits namespace: {{ include "tenant.name" . }} + labels: + app.kubernetes.io/name: tenant + app.kubernetes.io/part-of: cozystack
10-25: Confirm scoping: LimitRange is gated by .Values.resourceQuotas.Placing LimitRange inside the same if means it won’t render when resourceQuotas is disabled. Is that intentional? Many clusters want defaults even without a quota.
If unintended, move the LimitRange into its own template or guard it with a separate flag (e.g., .Values.limitRange.enabled) decoupled from .Values.resourceQuotas.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hack/e2e-apps/vminstance.bats(1 hunks)hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (3)
packages/library/cozy-lib/templates/_resources.tpl (1)
174-184: No consumers expect aresourceQuotaswrapper
cozy-lib.resources.flattenis only used inpackages/apps/tenant/templates/quota.yaml, which injects its output directly underspec.hard. No other templates or CRDs reference a top-levelresourceQuotaskey—removing the wrapper has no impact.Likely an incorrect or invalid review comment.
hack/e2e-apps/vminstance.bats (1)
21-22: LGTM: longer waits reduce flakiness.Increasing DV/PVC timeouts is reasonable for slower clusters.
packages/apps/tenant/templates/quota.yaml (1)
10-25: Solid addition: LimitRange defaults align with sane container quotas.This second manifest is well-formed (multi-doc with ---), apiVersion/kind are correct, and values are reasonable. Should help enforce requests/limits for Pods created in the tenant namespace.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
200-205: Make RQ assertions robust: include CPU and tolerate canonical unit variants.
Kubernetes may surface quantities as “128Gi” or canonical bytes; match both and include CPU to fully validate the quota.Apply this diff:
- kubectl get quota -n tenant-test \ - -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ - | grep -qx '137438953472 100Gi' + kubectl get quota -n tenant-test \ + -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ + | grep -Eq '^(60|60000m) (128Gi|137438953472) 100Gi$'Additionally, consider bumping the HR wait to reduce flakiness under load (outside this hunk):
# line ~198 kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
hack/e2e-install-cozystack.bats(2 hunks)
🔇 Additional comments (2)
hack/e2e-install-cozystack.bats (2)
126-128: LGTM: root tenant patch broadened (incl. seaweedfs) aligns with later waits.
No issues spotted with the patch payload vs subsequent HR waits.
192-197: Add CPU assertion and verify resourceQuotas schema wiring
- In hack/e2e-install-cozystack.bats, assert the CPU quota (e.g. check .spec.hard.requests.cpu) alongside memory and storage.
- Confirm Tenant API (api/v1alpha1/cozystackresourcedefinitions_types.go) and CRD schema expose the new resourceQuotas fields (cpu, memory, storage).
| # Assert LimitRange defaults for containers | ||
| kubectl get limitrange -n tenant-test \ | ||
| -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ | ||
| | grep -qx '250m 128Mi 25m 128Mi' |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wait for LimitRange and filter by type to avoid multi-item/ordering flakiness.
Ensure the resource exists before reading it and select only .type=="Container". Loosen grep to -Eq to match any line.
Apply this diff:
- # Assert LimitRange defaults for containers
- kubectl get limitrange -n tenant-test \
- -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \
- | grep -qx '250m 128Mi 25m 128Mi'
+ # Wait for and assert LimitRange defaults for containers
+ timeout 60 sh -ec 'until [ "$(kubectl get limitrange -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done'
+ kubectl get limitrange -n tenant-test \
+ -o jsonpath='{range .items[*].spec.limits[?(@.type=="Container")]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \
+ | grep -Eq '^250m 128Mi 25m 128Mi$'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Assert LimitRange defaults for containers | |
| kubectl get limitrange -n tenant-test \ | |
| -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ | |
| | grep -qx '250m 128Mi 25m 128Mi' | |
| # Wait for and assert LimitRange defaults for containers | |
| timeout 60 sh -ec 'until [ "$(kubectl get limitrange -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' | |
| kubectl get limitrange -n tenant-test \ | |
| -o jsonpath='{range .items[*].spec.limits[?(@.type=="Container")]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ | |
| | grep -Eq '^250m 128Mi 25m 128Mi$' |
🤖 Prompt for AI Agents
In hack/e2e-install-cozystack.bats around lines 206-209, the test reads
LimitRange items directly which can be flaky if the resource isn't created yet
or multiple types are present; change it to first wait/poll until a LimitRange
exists in namespace tenant-test, then use a jsonpath filter to select only
entries with .type=="Container" (e.g. with a jsonpath clause that filters by
type) and finally use grep -Eq to match the expected values so the assertion
tolerates ordering/extra lines; implement a short retry loop or polling command
that times out to ensure the resource exists before asserting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
packages/apps/tenant/templates/quota.yaml (4)
10-30: Scope the LimitRange with its own flag and parameterize defaults.Currently the LimitRange only renders when
.Values.resourceQuotasis set, and the defaults are hardcoded. Recommend:
- Add a dedicated toggle (e.g.,
.Values.limitRange.enabled) so teams can enable LimitRange independently of ResourceQuota.- Parameterize default/defaultRequest/maxLimitRequestRatio via values to avoid chart edits for tuning.
Diff within this block:
--- -apiVersion: v1 -kind: LimitRange -metadata: - name: tenant-range-limits - namespace: {{ include "tenant.name" . }} -spec: - limits: - - default: - cpu: "250m" - memory: "128Mi" - ephemeral-storage: "2Gi" - defaultRequest: - cpu: "25m" - memory: "128Mi" - ephemeral-storage: "50Mi" - maxLimitRequestRatio: - cpu: 10 - memory: 4 - ephemeral-storage: 50 - type: Container +{{- if .Values.limitRange.enabled | default true }} +apiVersion: v1 +kind: LimitRange +metadata: + name: {{ default "tenant-range-limits" .Values.limitRange.name }} + namespace: {{ include "tenant.name" . }} +spec: + limits: + - default: + cpu: {{ default "250m" .Values.limitRange.default.cpu | quote }} + memory: {{ default "128Mi" .Values.limitRange.default.memory | quote }} + ephemeral-storage: {{ default "2Gi" .Values.limitRange.default.ephemeralStorage | quote }} + defaultRequest: + cpu: {{ default "25m" .Values.limitRange.defaultRequest.cpu | quote }} + memory: {{ default "128Mi" .Values.limitRange.defaultRequest.memory | quote }} + ephemeral-storage: {{ default "50Mi" .Values.limitRange.defaultRequest.ephemeralStorage | quote }} + maxLimitRequestRatio: + cpu: {{ default 10 .Values.limitRange.maxLimitRequestRatio.cpu }} + memory: {{ default 4 .Values.limitRange.maxLimitRequestRatio.memory }} + ephemeral-storage: {{ default 50 .Values.limitRange.maxLimitRequestRatio.ephemeralStorage }} + type: Container +{{- end }}
11-15: Consider name scoping for future-proofing.Static name
tenant-range-limitsis fine per-namespace, but if namespace strategy changes later this could collide. Consider prefixing with tenant/chart identifiers.
18-25: Confirm intent: memory request equals memory limit.
defaultRequest.memory==default.memory(128Mi) while CPU request (25m) < limit (250m). This yields Burstable QoS, but fixes memory at limit==request. If the goal is headroom, consider a lower memory request (e.g., 64Mi).
16-30: Optional: add PVC storage constraints via LimitRange item.If you also want to govern PVC sizes, add a
type: PersistentVolumeClaimitem withmin/maxforstorage(defaults aren’t supported for PVCs).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/apps/tenant/templates/quota.yaml(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
packages/apps/tenant/templates/quota.yaml (2)
26-29: Validate maxLimitRequestRatio values, especially for ephemeral-storage.Ratios look generous (cpu:10, memory:4, ephemeral-storage:50). Please confirm these align with your multi-tenant isolation goals and node capacity; ephemeral 50x can allow large spikes.
10-30: Overall LGTM.Adding LimitRange alongside ResourceQuota is a solid step to enforce sane defaults and prevent zero-requests pods.
Signed-off-by: IvanHunters <xorokhotnikov@gmail.com>
d8e9c20 to
3e03b1b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
hack/e2e-install-cozystack.bats (2)
198-199: Increase readiness timeout for tenant-test to reduce flakes.
Tenant HRs can exceed 1m under load; bump to 5m.- kubectl wait hr/tenant-test -n tenant-root --timeout=1m --for=condition=ready + kubectl wait hr/tenant-test -n tenant-root --timeout=5m --for=condition=ready
206-209: Wait for LimitRange and filter Container type to avoid multi-item/ordering issues.
Prevents flakes when multiple LimitRanges/types exist.- # Assert LimitRange defaults for containers - kubectl get limitrange -n tenant-test \ - -o jsonpath='{range .items[*].spec.limits[*]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ - | grep -qx '250m 128Mi 25m 128Mi' + # Wait for and assert LimitRange defaults for containers + timeout 60 sh -ec 'until [ "$(kubectl get limitrange -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' + kubectl get limitrange -n tenant-test \ + -o jsonpath='{range .items[*].spec.limits[?(@.type=="Container")]}{.default.cpu}{" "}{.default.memory}{" "}{.defaultRequest.cpu}{" "}{.defaultRequest.memory}{"\n"}{end}' \ + | grep -Eq '^250m 128Mi 25m 128Mi$'
🧹 Nitpick comments (1)
hack/e2e-install-cozystack.bats (1)
132-136: Typo in comment.
“unvailability” → “unavailability”.- # TODO: Workaround ingress unvailability issue + # TODO: Workaround ingress unavailability issue
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
hack/e2e-apps/vminstance.bats(1 hunks)hack/e2e-install-cozystack.bats(2 hunks)packages/apps/tenant/templates/quota.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/library/cozy-lib/templates/_resources.tpl
- hack/e2e-apps/vminstance.bats
- packages/apps/tenant/templates/quota.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build
🔇 Additional comments (2)
hack/e2e-install-cozystack.bats (2)
126-126: Comment change looks fine.
No functional impact here.
192-197: Structured resourceQuotas in spec look good.
Matches intended cpu/memory/storage shape.
| # Wait for ResourceQuota to appear and assert values | ||
| timeout 60 sh -ec 'until [ "$(kubectl get quota -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' | ||
| kubectl get quota -n tenant-test \ | ||
| -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ | ||
| | grep -qx '137438953472 100Gi' | ||
|
|
There was a problem hiding this comment.
Fix ResourceQuota assertion: include CPU and tolerate unit normalization.
K8s may render cpu as 60 or 60000m, memory as 128Gi or 137438953472. Current check will spuriously fail.
# Wait for ResourceQuota to appear and assert values
timeout 60 sh -ec 'until [ "$(kubectl get quota -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done'
- kubectl get quota -n tenant-test \
- -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \
- | grep -qx '137438953472 100Gi'
+ kubectl get quota -n tenant-test \
+ -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \
+ | grep -Eq '^(60|60000m) (128Gi|137438953472) 100Gi$'📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Wait for ResourceQuota to appear and assert values | |
| timeout 60 sh -ec 'until [ "$(kubectl get quota -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' | |
| kubectl get quota -n tenant-test \ | |
| -o jsonpath='{range .items[*]}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ | |
| | grep -qx '137438953472 100Gi' | |
| # Wait for ResourceQuota to appear and assert values | |
| timeout 60 sh -ec 'until [ "$(kubectl get quota -n tenant-test --no-headers 2>/dev/null | wc -l)" -ge 1 ]; do sleep 1; done' | |
| kubectl get quota -n tenant-test \ | |
| -o jsonpath='{range .items[*]}{.spec.hard.requests\.cpu}{" "}{.spec.hard.requests\.memory}{" "}{.spec.hard.requests\.storage}{"\n"}{end}' \ | |
| | grep -Eq '^(60|60000m) (128Gi|137438953472) 100Gi$' |
🤖 Prompt for AI Agents
In hack/e2e-install-cozystack.bats around lines 200 to 205, the ResourceQuota
assertion only checks memory and storage and fails when K8s renders CPU and
memory in different units; update the jsonpath to also emit cpu (requests.cpu)
and change the assertion to tolerate unit normalization by either (a)
normalizing cpu to millicores (treat plain numbers as cores ×1000) and memory to
a canonical form (convert Gi to bytes or vice‑versa) before comparison, or (b)
use a flexible regex/alternate-value match that accepts both formats (e.g., 60
or 60000m for CPU, and 128Gi or 137438953472 for memory); ensure the final check
asserts cpu, memory and storage values in a normalized/regex-tolerant way to
avoid spurious failures.
What this PR does
Release note
Summary by CodeRabbit
New Features
Behavior Changes
Tests