vmcluster: support storage node pools - #2313
Conversation
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 9 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
cdeed73 to
e6a9c61
Compare
There was a problem hiding this comment.
1 issue found across 10 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
e6a9c61 to
49538a8
Compare
There was a problem hiding this comment.
2 issues found across 17 files
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
49538a8 to
1d48e0f
Compare
7016ee6 to
1131f28
Compare
1131f28 to
36b8de5
Compare
|
@vrutkovs need your feedback on this |
|
I'd like to have this merged after release is cut, but in general it looks okay |
|
sure, no rush, just wanted to get some feedback |
d24c1aa to
cfca541
Compare
| return vms.Validate() | ||
| } | ||
|
|
||
| func (vmi *VMInsert) validate() error { |
There was a problem hiding this comment.
No license verification? Seems we're only doing this for discovery, but imo its worth having for every type of vminsert
There was a problem hiding this comment.
this is needed to validate enterprise version dependent features, so in vminsert this check is redundant
| }, svc.Labels) | ||
| }}) | ||
|
|
||
| // pools: two pools with shared vminsert — pool STSes get pool label, instance label stays the cluster name, top-level vmstorage not created |
There was a problem hiding this comment.
We should be verifying the storageNode args for vminserts in all tests
| // Used as a suffix for generated resource names and as a storage group name in vmselect. | ||
| // Must be a lowercase alphanumeric DNS label; hyphens allowed in the interior. | ||
| // +kubebuilder:validation:Pattern:="^[a-z0-9]([a-z0-9-]*[a-z0-9])?$" | ||
| // +kubebuilder:validation:MaxLength=32 |
There was a problem hiding this comment.
Cluster name is unlimited, why would we limit it here? VMStorage names are very easy to overflow btw, this should be limited to 16 to make "vmstorage-" STS fit in 63 chars max
| {Verb: "Get", Kind: "VMServiceScrape", Resource: vmselectName}, | ||
| {Verb: "Create", Kind: "VMServiceScrape", Resource: vmselectName}, | ||
|
|
||
| // VMInsert |
There was a problem hiding this comment.
Lets comment here that pool logic has changed the order of reconciliation and vmselect is now checked later
| }, | ||
| }) | ||
|
|
||
| // pools: pool with dedicated vminsert — pool insert created, top-level vminsert skipped |
There was a problem hiding this comment.
Wouldn't that leave the second pool outside of the ingestion path? I think it would be more explicit to require defining vminsert for every pool
There was a problem hiding this comment.
updated tests
3e33f11 to
c271a21
Compare
c271a21 to
ff10e07
Compare
2 participants
fixes #741
Adds pools to VMCluster, allowing to run multiple isolated groups of vmstorage nodes (and optionally dedicated vminsert nodes) within a single cluster. Each pool is queried by vmselect as a named storage group, enabling multi-retention.
Each entry in spec.pools has a name, an optional vmstorage override (merges onto the top-level spec.vmstorage), and an optional vminsert override (merges onto the top-level spec.vminsert).
When any pool is defined, the top-level vmstorage StatefulSet is not deployed; pools replace it entirely.
The top-level vminsert Deployment is deployed as a shared insert only when no pool defines its own dedicated vminsert.
vmstorage.retentionPeriod inside a pool overrides the cluster-level spec.retentionPeriod for that pool, enabling per-pool retention.
Pool resources are named
vm<component>-<cluster>-<pool>(e.g. vmstorage-prod-hot).An
app.kubernetes.io/pool=<name>label is added to both the StatefulSet/Deployment selector and pod template labels, keeping per-pool selectors disjoint.