Multiple tabletpools of same type not supported

[james-at-upmind]

Use case

Creating distinct tablet pools of the same type, to create distinct configurations for tablets in the same Keyspace - ie. migrating from one volumeClaimTemplate to another.

Issue

I have a keyspace with 2 tablet pools - default (replica) and reporting (rdonly). This works fine.

I added a third tablet pool local, of type replica again (different volumeClaimTemplate and some labels distinct to the tablet pool) with a single replica (default x3, local x1) but when the operator applies the change, it instead scaled down a tablet from the default pool, leaving me with default x2, local x1. This happened again when I tried to scale up to local x2 resulting in default x2, local x2.

Root cause

It looks like the pool's name is only factored into the generation of the tablet UID when ExternalDatastore = true. This means that the 2x tablet pools' replicas are getting the same UIDs across the pools (based on their replica index), and as such are overwriting one-another.

vitess-operator/pkg/controller/vitessshard/reconcile_tablets.go

Lines 287 to 290 in 2d43a0d

	// 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) {
	tabletAlias.Uid = vttablet.UIDWithPoolName(pool.Cell, keyspaceName, vts.Spec.KeyRange, pool.Type, uint32(tabletIndex), pool.Name)
	}

However, the docs say There must be at most one pool in this list for each (cell,type,name), which leads me to think that the if ExternalDatastore != nil condition can be removed?

Version

Operator: v2.14.0
Vitess: v21.0.3

Looking at the source, this is still the same in main

I didn't see an issue template so I tried to include what I though what was relevant - Happy to provide more info here or on Slack.

[james-at-upmind]

I'm testing a fork that removes the if pool.ExternalDatastore condition, but I have hit the snag (in a staging cluster) that out current rdonly pool is called default, which will cause a UID regeneration. This performs a rather aggressive rollout of all tablets, which leads to new PVs (restored from backup, etc.).

I'm just testing what this looks like if we rename our tablet pool to "" at the same time.

But either way, this is a fairly disruptive change for anyone using a non-empty tablet pool name without some kind of migration path or config value 🤔

[jdoupe]

That condition was added as part of #428 ... but I can't really wrap my head around why.

[james-at-upmind]

The real issue is the backwards compatibility - I have had to add a --default-pool-names flag that lets me treat a legacy pool name (in my case default) as "" to stop regenerating UIDs.

I know it's not fully breaking regenerating the UID, but I don't want the overhead of redeploying all of the tablets, provisioning new PVCs, etc.

[mattlord]

The initial use case for the issue and PR were "external tablets", so I suspect the noted if pool.ExternalDatastore was meant to scope the work to that use case. The problem, I think, is that it logically opened the door to what you want to do here... but it doesn't work. :-)

Do you think I'm misunderstanding anything? If not... then I'm also not sure why we would not just remove that condition. We can at least see if any tests break if you are interested in opening a PR?

[james-at-upmind]

I can push a PR from my fork in the next day or so - and I propose the --default_pool_names flag so operators can opt to treat a pool as "" (in my case name: default) for backwards compatibility - Does that sounds good?

I've also added some tests explicitly around this but there wasn't anything existing about the tablet reconciliation logic.

[absolutejam]

#800