Increase VMSelect default cpu limit #660
Conversation
dosubot
bot
added
the
size:M
WalkthroughThis update simplifies the resource configurations within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/extra/monitoring/templates/vm/vmcluster.yaml (2)
14-17: Clarify vminsert CPU Limit Default Behavior
The CPU limit for vminsert is included conditionally using thedigfunction with a default value ofnil, while the memory has a default of"1000Mi". If the intention is to allow the operator to apply its default (or leave it unset), this is fine. However, if a specific CPU default (such as"500m") is expected for vminsert, please consider adjusting the call or adding documentation to clarify the design decision.
51-54: Clarify vmstorage CPU Limit Default Handling
Similar to vminsert, vmstorage’s CPU limit is conditionally rendered via thedigfunction with a default ofnil. Ensure that this behavior is intentional—i.e. that the operator should provide or omit a CPU limit if not specified. Documenting this behavior or considering a default similar to requests might help avoid confusion in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/extra/monitoring/templates/vm/vmcluster.yaml(2 hunks)
🔇 Additional comments (5)
packages/extra/monitoring/templates/vm/vmcluster.yaml (5)
19-20: Verify Default Resource Requests for vminsert
The resource requests for vminsert correctly default to"500m"for CPU and"500Mi"for memory using thedigfunction. These values appear well balanced for typical usage.
25-29: Increase and Document vmselect CPU Limit
The vmselect section now sets the CPU limit to"2000m", which meets the PR objective to enhance performance. The accompanying comments clearly explain that omitting this would have led to the operator defaulting to"500m", which is insufficient. This clear documentation enables future maintainers to understand the rationale behind the increased value.
31-32: Confirm vmselect Resource Requests
The CPU and memory requests for vmselect are defined with defaults"500m"and"500Mi"respectively. These defaults provide a balanced baseline compared to the increased CPU limit and appear to be in line with the intended resource allocation strategy.
56-57: Approve vmstorage Resource Requests Defaults
The resource requests for vmstorage, with defaults"100m"for CPU and"500Mi"for memory, are explicitly provided. These settings appear consistent with the desired resource allocation strategy for this component.
1-67: Overall Template Simplification and Resource Allocation Defaults
The revised template effectively streamlines resource configuration by replacing previous conditional logic with direct calls to thedigfunction. The defaults for memory and requests are clearly defined, and the increased default CPU limit for vmselect ("2000m") aligns well with the performance enhancement goals outlined in the PR.🧰 Tools
🪛 YAMLlint (1.35.1)
[error] 1-1: syntax error: expected the node content, but found '-'
(syntax)
|
Here is a result (in case no resource changes requested by user): --- vmcluster-before.yaml 2025-03-03 19:36:37.538818989 +0100
+++ vmcluster-after.yaml 2025-03-03 19:36:42.464828766 +0100
@@ -1,49 +1,65 @@
apiVersion: operator.victoriametrics.com/v1beta1
kind: VMCluster
metadata:
annotations:
meta.helm.sh/release-name: monitoring
meta.helm.sh/release-namespace: tenant-root
creationTimestamp: "2025-02-03T23:15:17Z"
finalizers:
- apps.victoriametrics.com/finalizer
- generation: 1
+ generation: 2
labels:
app.kubernetes.io/managed-by: Helm
helm.toolkit.fluxcd.io/name: monitoring
helm.toolkit.fluxcd.io/namespace: tenant-root
name: shortterm
namespace: tenant-root
uid: 711b0177-b931-4441-9353-0aa072d9e586
spec:
replicationFactor: 2
retentionPeriod: 3d
vminsert:
replicaCount: 2
- resources: {}
+ resources:
+ limits:
+ memory: 1000Mi
+ requests:
+ cpu: 500m
+ memory: 500Mi
vmselect:
cacheMountPath: /select-cache
extraArgs:
dedup.minScrapeInterval: 15s
search.maxUniqueTimeseries: "600000"
vmalert.proxyURL: http://vmalert-shortterm.tenant-root.svc:8080
replicaCount: 2
- resources: {}
+ resources:
+ limits:
+ cpu: 2000m
+ memory: 1000Mi
+ requests:
+ cpu: 500m
+ memory: 500Mi
storage:
volumeClaimTemplate:
spec:
resources:
requests:
storage: 2Gi
vmstorage:
replicaCount: 2
- resources: {}
+ resources:
+ limits:
+ memory: 2048Mi
+ requests:
+ cpu: 100m
+ memory: 500Mi
storage:
volumeClaimTemplate:
spec:
resources:
requests:
storage: 10Gi
storageDataPath: /vm-data
status:
clusterStatus: operational |
2 participants
resourcesdefinitions are forwarded to the actual VMCluster if user hasn't specified any.Summary by CodeRabbit