upgrades: flatten permanent upgrades#119142
Conversation
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
RaduBerinde
left a comment
There was a problem hiding this comment.
So much simpler, thanks!
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @dt and @stevendanna)
pkg/upgrade/upgrades/upgrades.go line 70 at r1 (raw file):
upgrade.NewPermanentSystemUpgrade( "populate initial version cluster setting table entry", clusterversion.VPrimordial3.Version(),
We should remove all the cluster settings that are no longer used (and remove the commentary around Permanent upgrades in cockroach_versions.go).
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/upgrades.go line 70 at r1 (raw file):
Previously, RaduBerinde wrote…
We should remove all the cluster settings that are no longer used (and remove the commentary around Permanent upgrades in
cockroach_versions.go).
Which cluster settings are no longer used?
|
Previously, dt (David Taylor) wrote…
Bleh, I meant cluster versions |
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/upgrades.go line 70 at r1 (raw file):
Previously, RaduBerinde wrote…
Bleh, I meant cluster versions
Ah, makes sense, removed. Just glad to know there aren't some extra cluster settings relating to upgrade execution.
stevendanna
left a comment
There was a problem hiding this comment.
This looks good. I wonder if we want to add a documentation comment somewhere explaining what you need to do when you think you need to modified the bootstrap migrations.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1, 1 of 1 files at r5, 4 of 6 files at r7, 1 of 3 files at r8, 4 of 5 files at r10, 2 of 2 files at r12, all commit messages.
Reviewable status:
pkg/clusterversion/cockroach_versions.go line 173 at r10 (raw file):
// VBootstrap versions are associated with bootstrap steps; no new bootstrap // versions should be added, as the two bootstrap functions are extended as // needed in-place.
[nit] maybe add a pointer to where the bootstrap steps are defined
pkg/upgrade/upgrades/permanent_upgrades.go line 36 at r7 (raw file):
ctx context.Context, cv clusterversion.ClusterVersion, deps upgrade.SystemDeps, ) error { for _, fn := range []upgrade.SystemUpgradeFunc{
[nit] I would make this a global variable and add a comment explaining when it is appropriate to add something here.
pkg/upgrade/upgrades/permanent_upgrades.go line 57 at r7 (raw file):
) error { for _, u := range []bootstrapStep{ {"add users and roles", addRootUser},
[nit] ditto, and what's with the unused strings?
Release note: none. Epic: none.
Release note: none. Epic: none.
Release note: none. Epic: none.
This switches the permanent migrations system from allowing any migration to be considered permanent to having exactly two 'bootstrap' migrations, one for system tenants specifically and one for all clusters. This single bootstrap job can be edited in place to add addition bootstrap steps, such as creating a new singleton job, as needed, _along side_ a regular migration that performs the same task during upgrades, typically by calling the same function in both places. This should simplify bootstrap by having only one or two jobs to run, instead of some unbounded number. Release note: none. Epic: none.
Release note: none. Epic: none.
Release note: none. Epic: none.
Release note: none. Epic: none.
Release note: none. Epic: none.
dt
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/permanent_upgrades.go line 36 at r7 (raw file):
Previously, RaduBerinde wrote…
[nit] I would make this a global variable and add a comment explaining when it is appropriate to add something here.
I tried pulling this into its own var and commenting on it, but the commentary ended up duplicative with the comment on the functions that ran through the list. I think I want comments on the functions, so I went back to inline lists but with the function comments speaking to the lists they contain. WDYT?
pkg/upgrade/upgrades/permanent_upgrades.go line 57 at r7 (raw file):
Previously, RaduBerinde wrote…
[nit] ditto, and what's with the unused strings?
We had these string names before that documented what each step was doing and I didn't want to lose them, just in case we later decide to e.g. update the job status between steps to reflect which step is running at any given time.
I'll log.Info them for now so they're not unused.
Release note: none. Epic: none.
RaduBerinde
left a comment
There was a problem hiding this comment.
Reviewable status:
pkg/upgrade/upgrades/permanent_upgrades.go line 36 at r7 (raw file):
Previously, dt (David Taylor) wrote…
I tried pulling this into its own var and commenting on it, but the commentary ended up duplicative with the comment on the functions that ran through the list. I think I want comments on the functions, so I went back to inline lists but with the function comments speaking to the lists they contain. WDYT?
Looks great, thanks!
|
TFTR! bors r+ |
|
Build succeeded: |
This partially reverts commit 129d62a. It also fixes the roachtest that was added in the previous commit. In PR cockroachdb#119142, the permanent upgrades were flattened so that they all have a version of 0.0-x. That means that the last element of the permanentUpgrades slice has a version of 0.0-4. Clusters that were bootstrapped with a version from v23.1.0 or later will have rows in system.migrations for versions 0.0-x, since that is the version where the permanent upgrades framework was introduced (in PR cockroachdb#119142). However, clusters that are were bootstrapped from a version earlier than v23.1 will not yet have these rows in system.migrations for versions 0.0-x, and those migrations will only be present under the legacy startupmigrations key. Release note (bug fix): Fixed a bug that could cause the password for the root user to be deleted while upgrading to v24.1. This bug only affects clusters that were initially created with a version of v22.2 or earlier. The same bug could also cause the `defaultdb` and `postgres` databases to be recreated during the upgrade to v24.1 if they had been previously deleted.
This partially reverts commit 129d62a. It also fixes the roachtest that was added in the previous commit. In PR cockroachdb#119142, the permanent upgrades were flattened so that they all have a version of 0.0-x. That means that the last element of the permanentUpgrades slice has a version of 0.0-4. Clusters that were bootstrapped with a version from v23.1.0 or later will have rows in system.migrations for versions 0.0-x, since that is the version where the permanent upgrades framework was introduced (in PR cockroachdb#91627). However, clusters that are were bootstrapped from a version earlier than v23.1 will not yet have these rows in system.migrations for versions 0.0-x, and those migrations will only be present under the legacy startupmigrations key. Release note (bug fix): Fixed a bug that could cause the password for the root user to be deleted while upgrading to v24.1. This bug only affects clusters that were initially created with a version of v22.2 or earlier. The same bug could also cause the `defaultdb` and `postgres` databases to be recreated during the upgrade to v24.1 if they had been previously deleted.
This switches the permanent migrations system from allowing
any migration to be considered permanent to having exactly two
'bootstrap' migrations, one for system tenants specifically and
one for all clusters.
This single bootstrap job can be edited in place to add addition bootstrap
steps, such as creating a new singleton job, as needed, along side a
regular migration that performs the same task during upgrades, typically by
calling the same function in both places. This should simplify bootstrap by
having only one or two jobs to run, instead of some unbounded number.
Release note: none.
Epic: none.