Let users specify CPU requests in VCPUs #972
Conversation
|
Warning Rate limit exceeded@kvaps has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 6 minutes and 36 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (7)
WalkthroughThis change updates the ClickHouse Helm chart to explicitly declare a dependency on the internal Changes
Sequence Diagram(s)sequenceDiagram
participant Helm as Helm Chart Renderer
participant ClickHouse as ClickHouse Chart
participant CozyLib as cozy-lib Templates
participant K8s as Kubernetes API
Helm->>ClickHouse: Render ClickHouse chart
ClickHouse->>CozyLib: Call resources.sanitize/preset with [values, context]
CozyLib->>CozyLib: Check input type (checkInput)
CozyLib->>CozyLib: Load cozyConfig if missing (loadCozyConfig)
CozyLib->>CozyLib: Sanitize/merge resources, apply CPU allocation ratio
CozyLib->>ClickHouse: Return sanitized resource YAML
ClickHouse->>K8s: Deploy with computed resources
Possibly related PRs
Suggested reviewers
Poem
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
efd7ccb to
abf9652
Compare
6095caa to
3ebf2e2
Compare
860d18c to
6f702b5
Compare
3ebf2e2 to
3e8d115
Compare
6f702b5 to
9592f7f
Compare
3e8d115 to
99d4ec5
Compare
99d4ec5 to
a1fbed1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
56-83:⚠️ Potential issueAdd validation for CPU allocation ratio and error handling.
The sanitize function needs protection against edge cases:
- Division by zero if
$cpuAllocationRatiois 0- Invalid CPU values that can't be converted to float
Add validation before the division:
{{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" dict | float64 }} +{{- if le $cpuAllocationRatio 0.0 }} +{{- fail "CPU allocation ratio must be greater than 0" }} +{{- end }}Consider adding error handling for the float conversion:
{{- $vcpuRequestF64 := (include "cozy-lib.resources.toFloat" $v) | float64 }} +{{- if eq $vcpuRequestF64 0.0 }} +{{- fail (printf "Invalid CPU value: %s" $v) }} +{{- end }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/apps/clickhouse/Chart.yaml(1 hunks)packages/library/cozy-lib/00-init.tpl(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(3 hunks)packages/library/versions_map(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Test
🔇 Additional comments (4)
packages/library/cozy-lib/Chart.yaml (1)
18-18: Version bump looks appropriate for new features.The version increment from 0.1.0 to 0.2.0 correctly follows semantic versioning for the new CPU allocation ratio functionality.
packages/library/versions_map (1)
1-2: Version mapping correctly tracks releases.Good practice to pin the previous version to a specific commit while the new version tracks HEAD.
packages/apps/clickhouse/Chart.yaml (1)
27-30: Dependency declaration is correctly configured.The addition of
cozy-libas a dependency is properly formatted with the correct internal repository URL.packages/library/cozy-lib/templates/_resources.tpl (1)
13-27: Well-implemented resource quantity converter.The
toFloatfunction correctly handles both base-2 and base-10 units with appropriate multipliers. Good use of hex float literals for precise base-2 values.
a1fbed1 to
0839b4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
5-13: Variable scoping issue appears to be resolved.The previous review comment about
$cozyConfigscoping has been addressed. The template now callsinclude "cozy-lib.loadCozyConfig" .and accesses the config viaindex . 1 "cozyConfig", which suggests the config is properly loaded into the context.
🧹 Nitpick comments (3)
packages/library/cozy-lib/templates/_cozyconfig.tpl (1)
1-7: Consider adding error handling for missing ConfigMap.The template correctly loads the ConfigMap, but doesn't handle the case where the
cozystackConfigMap might not exist in thecozy-systemnamespace. This could lead to silent failures.Consider adding a check:
{{- define "cozy-lib.loadCozyConfig" }} {{- include "cozy-lib.checkInput" . }} {{- if not (hasKey (index . 1) "cozyConfig") }} {{- $cozyConfig := lookup "v1" "ConfigMap" "cozy-system" "cozystack" }} +{{- if not $cozyConfig }} +{{- fail "ConfigMap 'cozystack' not found in namespace 'cozy-system'" }} +{{- end }} {{- $_ := set (index . 1) "cozyConfig" $cozyConfig }} {{- end }} {{- end }}packages/library/cozy-lib/templates/_resources.tpl (2)
15-29: Add validation for edge cases in unit conversion.The
toFloatfunction handles unit conversion well, but consider adding validation for edge cases:
- Empty strings or invalid numeric values
- Unknown units that might not be caught by the suffix matching
Consider adding basic validation:
{{- define "cozy-lib.resources.toFloat" -}} {{- $value := . -}} + {{- if not . -}} + {{- fail "Empty value provided to toFloat" -}} + {{- end -}} {{- $unit := 1.0 -}} {{- if typeIs "string" . -}} {{- $base2 := dict "Ki" 0x1p10 "Mi" 0x1p20 "Gi" 0x1p30 "Ti" 0x1p40 "Pi" 0x1p50 "Ei" 0x1p60 -}} {{- $base10 := dict "m" 1e-3 "k" 1e3 "M" 1e6 "G" 1e9 "T" 1e12 "P" 1e15 "E" 1e18 -}} {{- range $k, $v := merge $base2 $base10 -}} {{- if hasSuffix $k $ -}} {{- $value = trimSuffix $k $ -}} {{- $unit = $v -}} {{- end -}} {{- end -}} {{- end -}} + {{- if not (kindIs "float64" (float64 $value)) -}} + {{- fail (printf "Invalid numeric value: %s" $value) -}} + {{- end -}} {{- mulf (float64 $value) $unit | toString -}} {{- end -}}
85-87: CPU allocation calculation aligns with PR objectives.The CPU allocation logic correctly implements the PR objective of allowing users to specify CPU requests in vCPUs. The vCPU value is divided by the allocation ratio to determine the actual CPU request, which aligns with KubeVirt's CPU allocation behavior.
However, consider potential precision issues when converting between float64 and string representations.
To improve precision handling, consider using a more controlled string formatting:
-{{- $_ := set $output.requests $k ($cpuRequestF64 | toString) }} +{{- $_ := set $output.requests $k (printf "%.3f" $cpuRequestF64) }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/templates/_checkinput.tpl(1 hunks)packages/library/cozy-lib/templates/_cozyconfig.tpl(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/versions_map(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/library/versions_map
- packages/library/cozy-lib/Chart.yaml
🔇 Additional comments (5)
packages/library/cozy-lib/templates/_checkinput.tpl (1)
1-5: LGTM! Clean input validation helper.The template provides clear validation for the expected input format and helpful error messages.
packages/apps/clickhouse/templates/clickhouse.yaml (1)
124-128: LGTM! Template calls correctly updated for new argument format.The changes properly adapt to the new cozy-lib template signatures by passing arguments as lists with context.
packages/library/cozy-lib/templates/_resourcepresets.tpl (2)
17-42: Good refactoring to separate CPU and memory resources.The separation of CPU and memory resources into distinct dictionaries improves maintainability. The dynamic CPU limit calculation based on the allocation ratio is a nice enhancement that aligns with the PR objective of standardizing CPU resource allocation.
14-14:❌ Incorrect review comment
Verify the existence of required helper templates.
The template references
cozy-lib.resources.cpuAllocationRatioandcozy-lib.resources.toFloatwhich are not visible in the provided files.Also applies to: 37-37
🏁 Script executed:
#!/bin/bash # Description: Search for the missing helper templates that are referenced but not provided # Search for cpuAllocationRatio template definition echo "Searching for cpuAllocationRatio template..." rg -A 10 'define\s+"cozy-lib\.resources\.cpuAllocationRatio"' packages/library/cozy-lib/ # Search for toFloat template definition echo "Searching for toFloat template..." rg -A 10 'define\s+"cozy-lib\.resources\.toFloat"' packages/library/cozy-lib/ # Check if there's a _resources.tpl file that might contain these echo "Checking for _resources.tpl file..." fd -t f "_resources.tpl" packages/library/cozy-lib/Length of output: 2616
Helper templates verified in
_resources.tplThe referenced templates are already defined in
packages/library/cozy-lib/templates/_resources.tpl:
cozy-lib.resources.cpuAllocationRatiocozy-lib.resources.toFloatNo missing helpers—no changes required.
Likely an incorrect or invalid review comment.
packages/library/cozy-lib/templates/_resources.tpl (1)
63-78:✅ Verification successful
Parameter structure change may impact existing consumers.
The template now expects arguments at
index . 0instead of direct access to.. This is a breaking change that could affect existing consumers of this template.Verify that all consumers of
cozy-lib.resources.sanitizehave been updated to pass arguments in the new format.
🏁 Script executed:
#!/bin/bash # Description: Find all usages of cozy-lib.resources.sanitize to verify parameter format # Expected: All calls should pass context with resources at index 0 echo "Searching for all usages of cozy-lib.resources.sanitize:" rg -A 3 -B 1 "cozy-lib\.resources\.sanitize" echo -e "\nSearching for potential old-style calls:" rg -A 3 -B 1 "include.*cozy-lib\.resources\.sanitize.*\.Values\.resources"Length of output: 2867
All consumers updated to new
cozy-lib.resources.sanitizesignatureI searched the repo for calls to
cozy-lib.resources.sanitizeand confirmed that:
packages/apps/clickhouse/templates/clickhouse.yamlnow invokes it with(list .Values.resources $).- The only remaining “old-style” invocation is the example comment in
_resources.tplitself, which isn’t executed by consumers.No breaking impact—no changes required.
| */}} | ||
| {{- define "cozy-lib.resources.sanitize" }} | ||
| {{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" . | float64 }} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for CPU allocation ratio.
The CPU allocation ratio should be validated to ensure it's positive and non-zero to prevent division by zero errors.
Add validation after retrieving the ratio:
{{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" . | float64 }}
+{{- if le $cpuAllocationRatio 0.0 }}
+{{- fail "CPU allocation ratio must be positive and non-zero" }}
+{{- end }}📝 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.
| {{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" . | float64 }} | |
| {{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" . | float64 }} | |
| {{- if le $cpuAllocationRatio 0.0 }} | |
| {{- fail "CPU allocation ratio must be positive and non-zero" }} | |
| {{- end }} |
🤖 Prompt for AI Agents
In packages/library/cozy-lib/templates/_resources.tpl at line 61, add validation
for the CPU allocation ratio after retrieving it to ensure it is positive and
non-zero. Check if the value is less than or equal to zero, and if so, handle
the error appropriately (e.g., set a default positive value or raise an error)
to prevent division by zero errors later in the code.
There was a problem hiding this comment.
@NickVolynkin You're absolutely right! The validation would be crucial here since the CPU allocation ratio is used in division operations. Without proper validation, invalid configuration values (like 0 or negative numbers) could lead to runtime failures or unexpected resource calculations.
The suggested validation would catch these issues early and provide clear error messages, making the template more robust and easier to debug when misconfigured.
/| /|
( :v: )
|(_)|
ba4aa36 to
37b95e1
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
61-61: 🛠️ Refactor suggestionAdd validation for CPU allocation ratio to prevent runtime errors.
The CPU allocation ratio should be validated to ensure it's positive and non-zero to prevent division by zero errors in the CPU request calculation.
Add validation after retrieving the ratio:
{{- $cpuAllocationRatio := include "cozy-lib.resources.cpuAllocationRatio" . | float64 }} +{{- if le $cpuAllocationRatio 0.0 }} +{{- fail "CPU allocation ratio must be positive and non-zero" }} +{{- end }}
🧹 Nitpick comments (1)
packages/library/cozy-lib/templates/_resources.tpl (1)
85-87: Consider precision handling in CPU request calculation.The conversion from float64 back to string may result in precision loss or unexpected formatting (e.g., scientific notation for very small values). Consider formatting the output to maintain appropriate precision for Kubernetes resource specifications.
Consider using a more controlled string formatting:
-{{- $_ := set $output.requests $k ($cpuRequestF64 | toString) }} +{{- $cpuRequestStr := printf "%.3f" $cpuRequestF64 }} +{{- $_ := set $output.requests $k $cpuRequestStr }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/apps/clickhouse/templates/clickhouse.yaml(1 hunks)packages/library/cozy-lib/Chart.yaml(1 hunks)packages/library/cozy-lib/templates/_checkinput.tpl(1 hunks)packages/library/cozy-lib/templates/_cozyconfig.tpl(1 hunks)packages/library/cozy-lib/templates/_resourcepresets.tpl(1 hunks)packages/library/cozy-lib/templates/_resources.tpl(1 hunks)packages/library/versions_map(0 hunks)
💤 Files with no reviewable changes (1)
- packages/library/versions_map
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/library/cozy-lib/Chart.yaml
- packages/library/cozy-lib/templates/_checkinput.tpl
- packages/library/cozy-lib/templates/_cozyconfig.tpl
- packages/apps/clickhouse/templates/clickhouse.yaml
- packages/library/cozy-lib/templates/_resourcepresets.tpl
🔇 Additional comments (5)
packages/library/cozy-lib/templates/_resources.tpl (5)
1-3: LGTM: Simple default value template.The default CPU allocation ratio of 10 is clearly defined and follows good templating practices.
5-13: LGTM: CPU allocation ratio retrieval with proper fallback.The template correctly loads the cozy configuration and provides a fallback to the default value when the ConfigMap is not available. The use of
digfor safe nested key access is appropriate.
15-29: LGTM: Comprehensive unit conversion implementation.The resource quantity to float conversion properly handles both binary (Ki, Mi, Gi, etc.) and decimal (m, k, M, etc.) unit prefixes, which aligns with Kubernetes resource quantity standards.
74-78: LGTM: Correct precedence handling for resource merging.The logic correctly implements the documented precedence where top-level resource keys take priority over requests and limits sections. This provides flexibility while maintaining predictable behavior.
80-90: LGTM: Appropriate CPU vs non-CPU resource handling.The differentiated handling correctly implements the PR objective: CPU resources are scaled by the allocation ratio for requests while maintaining the original limit, and non-CPU resources have equal requests and limits. This aligns with the goal of allowing vCPU specifications while maintaining consistent resource allocation.
37b95e1 to
50aed6a
Compare
This patch introduces reusable library charts that provide backward-compatibility for users that specify their resources as explicit requests and limits for cpu, however this input is processed so that limits are set equal to requests except for CPU which only gets requests. Users can now embrace the new form by directly specifying resources in the first level of nesting (e.g. resources.cpu=100m instead of .resources.requests.cpu=100m). The order of precedence is top-level, then requests, then limits, ensuring that nothing will break in terms of scheduling, however workloads that specified limits much higher than requests might get a performance hit, now that they cannot use all this excess capacity. This should only affect memory-hungry workloads in low-contention environments. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
With this change a request for a virtual machine with 3 vCPUs will
reserve exactly the same amount of physical compute, as a request for a
Clickhouse instance with `{"resources": {"cpu": "3"}}` in its values,
with the scaling factor being KubeVirt's CPU allocation ratio.
Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
50aed6a to
7023abd
Compare
| to have equal requests and limits, except CPU, that has only requests. The | ||
| template expects to receive a dict {"requests":{...}, "limits":{...}} as | ||
| input, e.g. {{ include "cozy-lib.resources.sanitize" .Values.resources }}. | ||
| to have equal requests and limits, except CPU, where the limit is increased |
There was a problem hiding this comment.
This is ambiguous, and can be understood in two ways:
- All resources use the same (equal) request and limit preset
- In each given resource's default preset, requests equal limits
There was a problem hiding this comment.
Re your first point, I don't think anyone can imagine having all requests/limits equal among each other across different resources. 1Mi (1048576) of memory (unit of bytes implied) and 1048576 of cpu (unit of seconds implied) would be pretty wild.
Re your second point -- there aren't any presets here, what do you mean?
Anyway, I'm open to a better wording if you'd like to suggest something.
| Example input: | ||
| ============== | ||
| limits: | ||
| cpu: 100m | ||
| cpu: "1" |
There was a problem hiding this comment.
Limit of 1 and request of 2 — how does it work? Shouldn't the limit be more or equal to the request?
There was a problem hiding this comment.
Yeah, hence the name sanitize. The user can input any garbage he or she desires and the function will coerce it to something reasonable and reasonably backwards-compatible. See the comments of the example output bellow.
With this change a request for a virtual machine with 3 vCPUs will reserve exactly the same amount of physical compute, as a request for a Clickhouse instance with
{"resources": {"cpu": "3"}}in its values, with the scaling factor being KubeVirt's CPU allocation ratio.Summary by CodeRabbit
New Features
Bug Fixes
Chores