Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

clusterversion: prevent upgrades from development versions #86345

Merged
merged 2 commits into from
Aug 26, 2022

Conversation

dt
Copy link
Member

@dt dt commented Aug 17, 2022

This change defines a new "unstableVersionsAbove" point on the cluster
version line, above which any cluster versions are considered unstable
development-only versions which are still subject to change.

Performing an upgrade to a version while it is still unstable leaves a
cluster in a state where it persists a version that claims it has done
that upgrade and all prior, however those upgrades are still subject to
change by nature of being unstable. If it subsequently upgraded to a
stable version, this could result in subtle and nearly impossible to
detect issues, as being at or above a particular version is used to
assume that all subsequent version upgrades as released were run; on a
cluster that ran an earlier iteration of an upgrade this does not hold.

Thus to prevent clusters which upgrade to development versions from
subsequently upgrading to a stable version, we offset all development
versions -- those above the unstableVersionsAbove point -- into the far
future by adding one million to their major version e.g. v22.x-y becomes
1000022.x-y. This means an attempt to subsequently "upgrade" to a stable
version -- such as v22.2 -- will look like a downgrade and be forbidden.

On the release branch, prior to starting to publish upgradable releases,
the unstableVersionsAbove value should be set to invalidVersionKey to
reflect that all version upgrades in that release branch are now
considered to be stable, meaning they must be treated as immutable and
append-only.

Release note (ops change): clusters that are upgraded to an alpha or
other manual build from the development branch will not be able to be
subsequently upgraded to a release build.

Release justification: high-priority change to existing functionality,
to allow releasing alphas with known version upgrade bugs while ensuring
they do not subsequently upgrade into stable version but silently
corrupted clusters.

@dt dt requested review from ajwerner and celiala August 17, 2022 22:51
@dt dt requested a review from a team as a code owner August 17, 2022 22:51
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt changed the title clusterversion: prevent upgrades from master versions clusterversion: prevent upgrades from development versions Aug 18, 2022
@dt dt requested a review from bdarnell August 18, 2022 12:16
@dt
Copy link
Member Author

dt commented Aug 18, 2022

I went back and forth on what the flow should be on release branches: should every addition of a version -- of which there should be few / none after the mint -- adjust the unstable-above pointer to end-of-list? I didn't want to to be automatic, at least not on master, as that'd defeat the point. Should I condition on isReleaseBranch? Should release team just mint a new version for each beta/rc and be in charge of moving the pointer when they do so, e.g. so a random build from the release branch between RCs would call any new versions since the last unstable? What about the fact that we just forgot to set isReleaseBranch on a release branch for months?

I think I found something I'm happy with in the most recent revisions here though, doing away with the mistake-prone isReleaseBranch while I'm at it:
a) to serve the purpose of preventing version backports post-mint, I switched isReleaseBranch bool to finalVersion version key that, if set, must match binary version and added a check that if the binary version has no Internal - i.e. if it looks like a minted final - that final is indeed set (to prevent the forgetting part).
b) the designation of unstable versions is just a pointer into the line, and it can be unset to disable.

Thus, I believe the flow now would be:

  • master has finalVersion=invalid, unstableVersionsAbove=VFinalPrevRelease
  • cut release branch
  • on release branch ahead of beta 1, unstableVersionsAbove=invalid
  • betas, rcs, etc until we're ready to mint VFinal
  • on master we mint the final version
    • commit 1: mint VFinal, set finalVersion= VFinal
    • commit 2: mint VNextReleaseStart, set finalVersion=invalid and set unstableVersionsAbove=VFinal
  • backport to release branch mint only grabs commit 1

@dt dt requested a review from a team August 18, 2022 15:28
@dt dt force-pushed the no-upgrade-master branch 2 times, most recently from 5b0a96f to d271321 Compare August 18, 2022 17:07
@dt dt requested review from a team and stevendanna August 18, 2022 17:07
@dt dt force-pushed the no-upgrade-master branch 2 times, most recently from 9947a5b to 5a511c2 Compare August 19, 2022 20:18
Copy link
Collaborator

@celiala celiala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for putting this safeguard into place.

This makes sense to me -- :lgtm:

Reviewed 2 of 4 files at r3, 12 of 12 files at r4, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @ajwerner, @bdarnell, @dt, and @stevendanna)


pkg/clusterversion/cockroach_versions.go line 536 at r4 (raw file):

	// release, while on release branches it can be set to invalidVersionKey to
	// disable marking any versions as development versions.
	unstableVersionsAbove = V22_1

Ack. We'll update the stability period runbook accordingly, thanks.

Code quote:

	// release, while on release branches it can be set to invalidVersionKey to
	// disable marking any versions as development versions.
	unstableVersionsAbove = V22_1

@dt
Copy link
Member Author

dt commented Aug 19, 2022

@ajwerner I'm going to leave this one to you to bors if you like it and approve it, or just close if you don't (which is also ok with me; I don't have my heart set on it).

Copy link
Contributor

@ajwerner ajwerner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewed 4 of 4 files at r3, 9 of 12 files at r4.
Reviewable status: :shipit: complete! 2 of 0 LGTMs obtained (waiting on @bdarnell, @dt, and @stevendanna)

Previously this isReleaseBranch could be easily forgotten during the release process leaving
the check disabled.

Now it will fail loudly if the binary version looks like a minted final ensuring we remember to
set it.

Release note: none.

Release justification: low risk, high benefit change to make sure an existing safety check is used.
This change defines a new "unstableVersionsAbove" point on the cluster
version  line, above which any cluster versions are considered unstable
development-only versions which are still subject to change.

Performing an upgrade to a version while it is still unstable leaves a
cluster in a state where it persists a version that claims it has done
that upgrade and all prior, however those upgrades are still subject to
change by nature of being unstable. If it subsequently upgraded to a
stable version, this could result in subtle and nearly impossible to
detect issues, as being at or above a particular version is used to
assume that all subsequent version upgrades _as released_ were run; on a
cluster that ran an earlier iteration of an upgrade this does not hold.

Thus to prevent clusters which upgrade to development versions from
subsequently upgrading to a stable version, we offset all development
versions -- those above the unstableVersionsAbove point -- into the far
future by adding one million to their major version e.g. v22.x-y becomes
1000022.x-y. This means an attempt to subsequently "upgrade" to a stable
version -- such as v22.2 -- will look like a downgrade and be forbidden.

On the release branch, prior to starting to publish upgradable releases,
the unstableVersionsAbove value should be set to invalidVersionKey to
reflect that all version upgrades in that release branch are now
considered to be stable, meaning they must be treated as immutable and
append-only.

Release note (ops change): clusters that are upgraded to an alpha or
other manual build from the development branch will not be able to be
subsequently upgraded to a release build.

Release justification: high-priority change to existing functionality,
to allow releasing alphas with known version upgrade bugs while ensuring
they do not subsequently upgrade into stable version but silently
corrupted clusters.
@celiala celiala requested a review from a team August 25, 2022 21:25
@celiala
Copy link
Collaborator

celiala commented Aug 25, 2022

TFTR!

bors r=ajwerner

@craig
Copy link
Contributor

craig bot commented Aug 26, 2022

Build succeeded:

@craig craig bot merged commit 3688055 into cockroachdb:master Aug 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants