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.

Sign up for GitHub

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

Jump to bottom

clusterversion: set binaryMinSupportedVersion to v21_1 #67281

Merged
merged 3 commits into from
Aug 3, 2021
Merged

clusterversion: set binaryMinSupportedVersion to v21_1 #67281

merged 3 commits into from
Aug 3, 2021

Conversation

ajwerner
Copy link
Contributor

@ajwerner ajwerner commented Jul 6, 2021

This probably should have been done when Start21_2 was added.

Fixes #67277.

Release note: None

@ajwerner ajwerner requested a review from a team as a code owner July 6, 2021 17:39
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from 1b94f6e to 721798e Compare July 6, 2021 17:39
@rafiss
Copy link
Collaborator

rafiss commented Jul 21, 2021

Just stumbled into this; some process Qs:

  1. do we need to prioritize this specific issue within one of the engineering teams?
  2. do we need to add something to a process somewhere so some team (i guess release-eng?) does this for future releases?

@knz
Copy link
Contributor

knz commented Jul 29, 2021

does this need help?

@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from 721798e to 965ef75 Compare July 29, 2021 14:57
@ajwerner ajwerner requested a review from a team as a code owner July 29, 2021 14:57
@ajwerner ajwerner requested a review from a team July 29, 2021 14:57
@ajwerner
Copy link
Contributor Author

This has been rebased and most of the failures removed because of #67822.

I added another commit to fix a real problem. I am left somewhat confused: why are we running the migration before bumping to the fence version? @irfansharif am I missing something or is this migration call in the wrong place?

@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from 965ef75 to 8ee9951 Compare July 29, 2021 15:39
@ajwerner
Copy link
Contributor Author

@irfansharif am I missing something or is this migration call in the wrong place?

I think I've got it sorted. We do it to make sure the version gating for new nodes joining is sound

@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from 8ee9951 to a1e4df7 Compare July 29, 2021 15:45
@ajwerner
Copy link
Contributor Author

I would like very much to backport the 2nd commit. Who needs to look at it and what additional testing does it need?

@yuzefovich yuzefovich removed the request for review from a team July 29, 2021 16:53
knz
knz reviewed Aug 2, 2021
View reviewed changes
Copy link
Contributor

@knz knz left a comment

Choose a reason for hiding this comment

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

1st, 3rd and 4th commit LGTM

Would like a review from @irfansharif on the 2nd commit too

Reviewed 5 of 5 files at r4, 2 of 3 files at r5, 2 of 2 files at r7, 1 of 1 files at r8.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @ajwerner)

pkg/migration/migrationmanager/manager.go, line 295 at r7 (raw file):

		alreadyCompleted, err = migrationjob.CheckIfMigrationCompleted(ctx, txn, m.ie, version)
		if alreadyCompleted || err != nil {
			log.Infof(ctx, "already completed or error %v", err)

maybe split the cases and use log.Errorf where appropriate?

@ajwerner
migrations: fix TruncatedStateMigrationTest to inject the appropriate…
ad450b1 
… version

The test was only working because we allowed the minBinarySupportedVersion to be
quite old.

Release note: None
@ajwerner
migration/migrationmanager: validate the cluster version before migra…
e41dd01 
…tion

Before this change, the code would run a migration, then validate that it
can bump the cluster version. This is a little bit wacky and can lead to
lots of jobs being created, jobs which may ultimately fail. This change
is good in that it both prevents jobs from being created and gives us an
error before we bump the cluster version to the fence version.

Release note (bug fix): Fixed a bug where migration jobs might run and
update a cluster version before the cluster was ready for the upgrade.
The bug could result in many extra failed migration jobs.
@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from a1e4df7 to b64dc88 Compare August 2, 2021 18:09
ajwerner
ajwerner commented Aug 3, 2021
View reviewed changes
Copy link
Contributor Author

@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.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)

pkg/migration/migrationmanager/manager.go, line 295 at r7 (raw file):

Previously, knz (kena) wrote…

maybe split the cases and use log.Errorf where appropriate?

Done.

@irfansharif irfansharif self-requested a review August 3, 2021 17:55
irfansharif
irfansharif approved these changes Aug 3, 2021
View reviewed changes
Copy link
Contributor

@irfansharif irfansharif left a comment

Choose a reason for hiding this comment

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

LGTM.

why are we running the migration before bumping to the fence version? @irfansharif am I missing something or is this migration call in the wrong place?

We do it to make sure the version gating for new nodes joining is sound

Yup. The other thing to note is that the release demarcation versions (Start21_2, Start21_1, etc.) don't have any migrations attached to them. The migration manager will then:

runMigration(..., Start21_2)
bumpClusterVersion(..., fenceVersionFor(Start21_2))
runMigration(..., <first real version in 21.2>)

As I say this I realize we don't have panics/asserts making sure we don't accidentally attach a migration on one of these special StartXY_Z versions. We could document/assert in this PR if you feel it necessary.

pkg/migration/migrations/truncated_state_external_test.go Outdated
// We want to exercise manual control over the upgrade process.
DisableAutomaticVersionUpgrade: 1,
makeArgs := func() (args base.TestServerArgs) {
minVersion := minVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have these?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Detritus. I think at some point for some reason there were some pointers involved. Removed.

@irfansharif
Copy link
Contributor

(Also sorry for the review lag, only now crawling out of my inbox after a hectic L2 rotation + a long weekend.)

@ajwerner
clusterversion: set binaryMinSupportedVersion to v21_1
42e952d 
This probably should have been done when `Start21_2` was added.

Fixes cockroachdb#67277.

Release note: None
@ajwerner ajwerner force-pushed the ajwerner/move-binaryMinSupportedVersion-to-2v1_1 branch from b64dc88 to 42e952d Compare August 3, 2021 19:33
@ajwerner
Copy link
Contributor Author

ajwerner commented Aug 3, 2021

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Aug 3, 2021

Build succeeded:

@craig craig bot merged commit 9758387 into cockroachdb:master Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knz knz knz approved these changes

@irfansharif irfansharif irfansharif approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

clusterversion: set binaryMinSupportedVersion on master to v21_1
5 participants
@ajwerner @cockroach-teamcity @rafiss @knz @irfansharif