My concerns are around edge cases, but if we want to really rely on this, we should sort them out.

Reviewed 5 of 18 files at r1.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @ajstorm, @healthy-pod, and @knz)

pkg/server/tenant_migration.go line 62 at r2 (raw file):

			"binary's minimum supported version %s",
			targetCV, tenantVersion.BinaryMinSupportedVersion())
		log.Warningf(ctx, "%s", msg)

I think the better thing to do here would be to construct the error using our error constructors which should do a good job with the redaction of the arguments and then pass the error to the logging function because transitively the redaction will be handled. Same for the next stanza.

pkg/server/tenant_migration.go line 69 at r2 (raw file):

		msg := fmt.Sprintf("sql pod %s is running a binary version %s which is "+
			"less than the attempted upgrade version %s",
			m.sqlServer.sqlIDContainer.SQLInstanceID().String(),

nit: don't call String here, it'll work against what I think you want to do regarding redaction.

pkg/upgrade/system_upgrade.go line 32 at r2 (raw file):

	// cluster. This is merely a convenience method and is not meant to be used
	// to infer cluster stability; for that, use UntilClusterStable.
	NumNodesOrTenantPods(ctx context.Context) (int, error)

I'm not up to date on the latest terminology. This is fine with me but a bit of a mouthful. What about NumServers= and ForEveryServer?

Code quote:

	// NumNodesOrTenantPods returns the number of nodes or tenant pods in the
	// cluster. This is merely a convenience method and is not meant to be used
	// to infer cluster stability; for that, use UntilClusterStable.
	NumNodesOrTenantPods(ctx context.Context) (int, error)

pkg/upgrade/upgradecluster/tenant_cluster.go line 132 at r2 (raw file):

	// Dialer allows for the construction of connections to other SQL pods.
	Dialer         NodeDialer
	InstanceReader *instancestorage.Reader

A delicate thing that I'm not so certain this PR deals with is the semantics of GetAllInstances. In particular, GetAllInstances uses a cache and can return stale results. What I think we want is to know the set of instances which might be live. I think this means we need a transactional mechanism to determine the set of instances. Even this isn't fundamentally safe.

0: podA starts up
1: podA begins migration protocol
2: podA determines it is the only pod, so it decides to bump the version gate
3: podB starts up and is using an old version (v1)
4: podB reads the cluster setting at v1
5: podA bumps the cluster version to v2
6: podA reads all the instances after the bump (assuming you've fixed the caching thing) and finds only itself
7: podB writes its instance entry and proceeds to serve traffic -- violating the invariant

I think that we need some way to synchronize the writing of the instance entry with the validation of the current version during startup.

One way we could deal with this would be to read the version from KV again after writing the instance row and making sure it's kosher. It's an extra RPC, but with the MR plans, it shouldn't be too painful.

pkg/upgrade/upgradecluster/tenant_cluster.go line 190 at r2 (raw file):

			conn, err := t.Dialer.Dial(ctx, roachpb.NodeID(id), rpc.DefaultClass)
			if err != nil {
				if strings.Contains(err.Error(), "failed to connect") {

this is suspicious to me, is there no more structured way to get at this condition? Do you have a test which exercises this path so we can go and poke at the error structure?

Reviewed 8 of 8 files at r13, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @healthy-pod)

pkg/cmd/roachtest/tests/multitenant_upgrade.go line 214 at r12 (raw file):

Yes, but let's have an issue open to track this followup work.

pkg/sql/sqlinstance/instancestorage/instancereader.go line 193 at r12 (raw file):

The instance ID is the primary key. This means that you can narrow down the scan in the kv batch by encoding the ID at the end of the prefix (And turn the scan into a get).

The problem I'm trying to avoid is that if we have lots of entries in the table (or, stale entries) this is going to waste time and memory.

I'm not considering this blocking here, but I do recommend this to be optimized in a later stage so you can file an issue and refer to it here.

pkg/sql/sqlinstance/instancestorage/instancereader.go line 237 at r12 (raw file):

No objection but let's have an issue to track this.

pkg/upgrade/upgradecluster/tenant_cluster.go line 197 at r12 (raw file):

ok, this raises a followup question: what happens when an instance terminates but does not clean up its instance record? Do version upgrades become completely blocked after that point? What would be the recommended action to repair from that point onwards?

pkg/upgrade/upgradecluster/tenant_cluster.go line 207 at r12 (raw file):

I see.

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

I know this about retry but this is not my question.

My question is, does this loop properly terminate when the server is requested to shut down via the stopper. Depending on which ctx is used here, it may or may not be cancelled upon quiescing the stopper. This is what we should check.

pkg/upgrade/upgrademanager/manager.go line 621 at r12 (raw file):

It's the other way around.

   err = errors.HandledWithMessage(err, "validate cluster version failed: some enant pods running on binary less than 23.1")

pkg/upgrade/upgrademanager/manager.go line 409 at r13 (raw file):

	// to every server (SQL server in the case of secondary tenants, or
	// combined KV/Storage server in the case of the storage layer) in the
	// cluster. Each node (or pod) will persist the version, bump the local

The point of my previous request was to enable you to replace all instances of "node (or pod)" by just "server' throughout the rest of this explanation.

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go line 535 at r12 (raw file):

I am not able to review this test in its current form. But I trust Andrew's review so you can go by that.

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go line 548 at r12 (raw file):

why not disable the circuit breaker throughout the entire test?

On the one hand i agree this is more work so it shouldn't block the PR.
On the other hand I am expecting the choice made here to cause hard-to-troubleshoot flakiness in the future. At the very least we should have a follow-up issue and invest into this to strengthen the test.

pkg/ccl/serverccl/server_startup_guardrails_test.go line 103 at r12 (raw file):

That seems like a bug. If you can confirm it, it's serious as it would affect our UA story.
So if confirmed please file an issue that refers to this test and calls for an investigation.
(And add a link to your issue in a comment here, so that folk do not get surprised when they see your choice)

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

require.Error would replace the code you've written already, by checking that the error message contains the string you expect.

Addressed this round. RFAL. Still working on @healthy-pod's addition.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

pkg/cmd/roachtest/tests/multitenant_upgrade.go line 214 at r12 (raw file):

#98869

pkg/sql/sqlinstance/instancestorage/instancereader.go line 193 at r12 (raw file):

#98869

pkg/sql/sqlinstance/instancestorage/instancereader.go line 237 at r12 (raw file):

#98869

pkg/upgrade/upgradecluster/tenant_cluster.go line 197 at r12 (raw file):

Yeah, it was something I was going to raise after this PR got merged. They're not completely blocked, but they'll take 10 minutes to remove failed SQL servers before they can be reattempted. This may lengthen the time required to perform upgrades in Serverless, where I'm assuming a retry loop is present. The recommended action at this point is to wait. In theory, customers could also set the cluster settings to reduce the session timeout, but that may have other complications.

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

Are you suggesting that we validate this with a new test? If so, are you thinking that this needs validation before this PR merges?

pkg/upgrade/upgrademanager/manager.go line 621 at r12 (raw file):

Done.

pkg/upgrade/upgrademanager/manager.go line 409 at r13 (raw file):

Done

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go line 535 at r12 (raw file):

Thanks.

pkg/ccl/kvccl/kvtenantccl/tenant_upgrade_test.go line 548 at r12 (raw file):

#98869

pkg/ccl/serverccl/server_startup_guardrails_test.go line 103 at r12 (raw file):

It's definitely an issue - I tested it yesterday. Annotated with the issue: #98868

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

Right, but in this case we want to accept all errors except this one. Is there a require function for that?

Third commit is now there for @healthy-pod's changes. multitenant-upgrade should now be clean 🎉.

Ready for another (hopefully final) look. 🤞

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

Reviewed 2 of 2 files at r14.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @healthy-pod)

pkg/upgrade/upgradecluster/tenant_cluster.go line 197 at r12 (raw file):

Could you propagate this explanation in the commit message. I think it's an important callout.

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

I do not think we need an automated test before it merges, but i'd like us to do a manual test.

This is similar to a change David made yesterday in a related area (retrying the allocation of instance ID forever until it succeeds). My recommended strategy for the test was to:

1. modify the code here with something inside the loop that does "if current instance ID == 3, then print a dummy log message and loop forever"
2. start a 3 or 5-node cluster with roachprod create local
3. use the create tenant / alter tenant start service shared
4. run roachprod sql -d cluster:tenantname to connect to a specific tenant and issue the version upgrade manually
5. verify visually in logs that the dummy message is produced and the loop is indeed stuck
6. run roachprod stop --sig=15 on the problem node to request a graceful shutdown
7. verify visually that the graceful shutdown completes

pkg/ccl/serverccl/server_startup_guardrails_test.go line 103 at r12 (raw file):

Thanks

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

if that is what you want the code to do, then i believe there's a mistake in the code. That if !contains(...) will fail the test in the opposite condition. Or are my eyes failing me?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @healthy-pod)

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

(The alternative is to path through the chain of calls and build confidence that this ctx gets, indeed, canceled on server shutdown. I tried and it wasn't obvious)

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

pkg/upgrade/upgradecluster/tenant_cluster.go line 197 at r12 (raw file):

Done

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

To be clear, are you suggesting that I should check to see if the SQL server's instance is quiesced? Or is it sufficient to see if the ctx has been marked Done? If it's the former, I'll need to do a bit more plumbing, as I don't have access to the SQL server's instance from in here.

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

Sorry, my earlier comment was incorrect. We want to accept this error and not all others. In that light wouldn't require.Error do the wrong thing here?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @healthy-pod)

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

I am asking to check that this loop indeed exits when the process that runs it is asked to shut down gracefully. Both standalone SQL pods and embedded secondary tenant SQL servers are subject to the same shutdown / quiescence logic, so if it works for one it will work for the other.

Right now, I do not have confidence that this loop won't keep forever running and prevent a graceful shutdown. Because i don't see logic through which the ctx would get canceled in that case.

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

ok then you can use require.ErrorContains https://github.com/stretchr/testify/blob/master/require/require.go#L328

All comments addressed. RFAL.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

OK, testing here seems to be OK. I did a modified version of what you suggested above:

1. Modified the loop so that it just retried forever and every time through the loop printed "retrying forever" to the logs.
2. Modified TestTenantUpgrade so that it created a 3 node instead of a 1 node cluster.
3. Added a go routine in TestTenantUpgrade right before performing the upgrade which logs its presence to the logs, sleeps for 15 seconds and then performs one of a few actions {stop the whole cluster, stop the node on which the tenant is running, stops the tenant (using a custom stopper), closes the DB connection in which the upgrade is running}

The findings are interesting. If we stop the cluster, node or the tenant, the loop exits. I think this is the behaviour that you were worried about, so I think the code is good as-is.

In the case where we just stop the connection that the upgrade is running in, we loop forever. I'm not sure if this is bad or good, but it does indicate that my earlier comment is true - that without that custom stopper on SQL server creation, the SQL server instance won't get shut down.

Either way, I think as far as this PR is concerned, we don't have an issue here.

pkg/ccl/serverccl/tenant_migration_test.go line 239 at r12 (raw file):

Done.

Last forced push was just a rebase.

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @healthy-pod, and @knz)

Reviewed 7 of 20 files at r21, 10 of 12 files at r22, 2 of 2 files at r23, all commit messages.
Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @ajstorm, @ajwerner, and @healthy-pod)

pkg/upgrade/upgradecluster/tenant_cluster.go line 276 at r12 (raw file):

In the case where we just stop the connection that the upgrade is running in, we loop forever. I'm not sure if this is bad or good

I believe this is expected -- once the order is given to start upgrading, the cluster will continue to attempt to perform the upgrade until it can. This has always been the case. We could build a mechanism to interrupt that process if we find it important.