Allow multiple tablet pools of the same type - #800
Conversation
|
I didn't use table driven tests because I didn't see them used elsewhere and the coverage is only rudimentary, but the tests passed for me and I've deployed this fork into our internal Vitess clusters. In our environments, I have also leveraged the |
There was a problem hiding this comment.
Pull request overview
This PR updates tablet UID generation so multiple tablet pools can share the same (cell, type) without requiring ExternalDatastore, and introduces a compatibility flag (--default_pool_names) to preserve existing tablet UIDs during migrations.
Changes:
- Add
--default_pool_namesoperator flag and helpers for treating certain pool names as “unnamed” during UID generation. - Generate name-aware tablet UIDs for non-default named pools, independent of
ExternalDatastore. - Add unit tests covering UID behavior for default vs non-default pool names and mixed pools in the same (cell, type).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| pkg/operator/environment/environment.go | Adds --default_pool_names flag and helpers to control backward-compatible UID generation. |
| pkg/controller/vitessshard/reconcile_tablets.go | Updates UID generation logic to use pool names (except configured defaults) and adjusts pool-name labeling behavior. |
| pkg/controller/vitessshard/reconcile_tablets_test.go | Adds tests validating UID stability for default pool names and uniqueness across multiple pools. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if 0 < len(pool.Name) { | ||
| labels[planetscalev2.TabletPoolNameLabel] = pool.Name | ||
| } |
| // If TabletPools has multiple pools within the same (cell,type) pair, we need to add a pool name to the UID generator. | ||
| if pool.ExternalDatastore != nil && 0 < len(pool.Name) { | ||
| // Pool names listed in --default_pool_names are treated as unnamed to preserve existing tablet UIDs. | ||
| if 0 < len(pool.Name) && !environment.IsDefaultPoolName(pool.Name) { |
| // tabletsForPool returns the tablet specs belonging to the named pool, using | ||
| // the pool-name label that vttabletSpecs stamps on every tablet. |
| func SetDefaultPoolNames(names []string) { | ||
| defaultPoolNames = names | ||
| } |
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
Thanks, @absolutejam ! ❤️ There are a few things that I think we should try and address before merging (sometimes it can be resolved in a discussion vs code changes).
I think that it's worth guarding against default-pool UID collisions in pkg/controller/vitessshard/reconcile_tablets.go:286. The new --default_pool_names behavior can make two pools in the same (cell,type) both use the legacy unnamed UID path. For example, an unnamed pool plus a default-named pool, or two named pools both listed in --default_pool_names, will generate the same tablet UIDs for the same indexes. That seems like it can make the desired tablet map overwrite one pool with the other, reusing the same pod/PVC/tablet alias identity. I think we should reject or otherwise prevent more than one “legacy UID” pool per (cell,type), and add a regression test for that case.
I think that we should Include the pool name in “same pool” anti-affinity labels in pkg/operator/vttablet/spec.go:67. Named local pools can now represent distinct pools with the same cell/type, but poolLabels() still does not include TabletPoolNameLabel. That means the default anti-affinity that is described as applying to the “same exact pool” still groups separate named pools together. I think we should include the pool name label there when present so scheduling behavior matches the new pool identity model.
We will need to update the API/CRD wording for named local pools
in pkg/apis/planetscale/v2/vitessshard_types.go:147
pkg/apis/planetscale/v2/labels.go:39. The Name field comment still says a pool name requires ExternalDatastore, and the pool-name label comment still says it applies to Vitess-unmanaged keyspaces. Since this PR intentionally supports named local pools, I think we should update those comments and regenerate any CRD descriptions derived from them so users do not think the new configuration is unsupported.
Side note, the DCO is currently failing on the PR. If you click on that failed workflow it will give you precise instructions for how to fix it.
65c5463 to
3c031ad
Compare
|
Thanks for the thoughtful comments @mattlord - I think adding a guard for UID collisions is the right move. I've updated the documentation now and restricted |
3c031ad to
2a78a15
Compare
Signed-off-by: James Booth <james@absolutejam.co.uk>
2a78a15 to
fb9f668
Compare
mattlord
left a comment
mattlord
left a comment
There was a problem hiding this comment.
-
Blocking: I think that we should return immediately when
vttabletSpecs()fails. Inpkg/controller/vitessshard/reconcile_tablets.go:85, the error is recorded but reconciliation continues with an empty desired set.ReconcileObjectSet()can then begin draining and deleting existing non-primary tablets, followed by their PVCs, because they all appear unwanted. A UID collision should fail safely without touching existing resources. I think we should add a reconciliation-level regression test that verifies no existing Pods or PVCs are modified or deleted. -
Blocking: I think that
--default_pool_nameshould only select the legacy UID path for a named, operator-managed pool. Inpkg/controller/vitessshard/reconcile_tablets.go:295, setting this flag also makes unnamed pools useUIDWithPoolName(..., "")and makes a matching namedExternalDatastorepool stop using its existing name-aware UID. Enabling the compatibility flag for one pool can therefore churn unrelated tablet and PVC identities across the operator’s entire fleet. Empty names should always retainUID(), named external pools should always retainUIDWithPoolName(), and the override should apply only to named managed pools. I think a table-driven compatibility matrix would help pin this down. -
Blocking: I think that disk reconciliation also needs to include the pool name.
pkg/controller/vitessshard/reconcile_disk.go:59finds tablets using only(cell,type), so each same-type managed pool checks every other pool’s PVC against its own requested size. When those pools use different disk sizes, one pool will wait forever for another pool’s PVC spec to match the wrong size, preventing the resize rollout from completing. I think we should filter usingTabletPoolNameLabeland add a regression test with two same-type pools using different storage sizes. -
Non-blocking: The PR description still refers to
--default_pool_namesand multiple “specified pools,” while the implementation provides the singular--default_pool_name. I think we should align that wording and clarify that the flag must remain configured while the named pool needs to retain its legacy UIDs.
There was a problem hiding this comment.
🟡 Changes recommended
UID error handling can delete existing tablets, and migration and disk-reconciliation behavior remain incomplete.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 6
- Review effort level: Balanced
| if err != nil { | ||
| resultBuilder.Error(err) | ||
| } |
| func IsDefaultPoolName(name string) bool { | ||
| return name == defaultPoolName | ||
| } |
| // This field is optional, and defaults to an empty string. | ||
| // Assigning different names to this field enables the existence of multiple pools with a specific tablet type in a given cell. |
| keyspaceName := vts.Labels[planetscalev2.KeyspaceLabel] | ||
|
|
||
| var tablets []*vttablet.Spec | ||
| seenUIDs := make(map[uint32]string) |
| if pool.ExternalDatastore != nil { | ||
| labels[planetscalev2.TabletPoolNameLabel] = pool.Name | ||
| } | ||
| labels[planetscalev2.TabletPoolNameLabel] = pool.Name |
| operatorFlagSet := pflag.NewFlagSet("operator", pflag.ExitOnError) | ||
|
|
||
| operatorFlagSet.DurationVar(&reconcileTimeout, "reconcile_timeout", 10*time.Minute, "Maximum time that any controller will spend trying to reconcile a single object before giving up.") | ||
| operatorFlagSet.StringVar(&defaultPoolName, "default_pool_name", "", "Pool name to treat as unnamed (equivalent to \"\") for UID generation. Use this when migrating a named pool that previously had no name-aware UID generation.") |
Removes the external data store requirement for multiple tabletpools.
Also adds
--default_pool_namesflag which can optionally be supplied to keep backwards compatible behaviour for the specified pools.