-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
roachtest: update version map and test fixtures for 22.1 #69827
roachtest: update version map and test fixtures for 22.1 #69827
Conversation
This should not be merged until after Tuesday's branch cut. |
3fc6605
to
de06502
Compare
de06502
to
a142354
Compare
a142354
to
0b85373
Compare
I'm getting errors while trying to generate fixtures (as per these instructions). I think the issue is that we're trying to fetch
|
0b85373
to
703b3e8
Compare
@@ -92,6 +92,7 @@ func runVersionUpgrade(ctx context.Context, t test.Test, c cluster.Cluster) { | |||
// the 20.2 release, this test will fail because it is missing a fixture for | |||
// 20.1; run the test (on 20.1) with the bool flipped to create the fixture. | |||
// Check it in (instructions will be logged below) and off we go. | |||
// TODO(celia) - make sure this is false before merging back! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment can be removed.
I think the problem with the fixture generation is that makeFixtureVersion
below is set to a value that assumes that 21.2.0 exists already, but it does not. You need to download beta1 to your local machine, point roachtest
at it with the --cockroach
flag, and run with makeFixtureVersion = ""
. Then we will start your local binary (as opposed to trying to download nonexistent 21.2.0), extract the version from that, and learn the predecessor from that and then make the fixture. That is what the comment on line 97-114 tries to explain, if you read that but did not draw those conclusions please reword it so that next time it will be clearer, thank you!
It's also possible that you can make it work with just using 21.2.0-beta.1
for makeFixtureVersion
which would be easier, but I don't think it will - might be worth a shot though since it's so easy to try.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still getting an error -- not sure if I'm still unclear on the exact steps to run or not. So I'll add them below. But this is the error I'm now seeing:
--- FAIL: acceptance/version-upgrade (139.78s)
test artifacts and logs in: artifacts/acceptance/version-upgrade/run_1
versionupgrade.go:480,versionupgrade.go:214,versionupgrade.go:644,versionupgrade.go:116,acceptance.go:54,acceptance.go:104,
test_runner.go:777: 1: expected version 21.1-1162, got 21.1-1124
Here's the steps I'm running:
## Download beta.1
curl https://binaries.cockroachdb.com/cockroach-v21.2.0-beta.1.darwin-10.9-amd64.tgz \
| tar -xJ && cp -i cockroach-v21.2.0-beta.1.darwin-10.9-amd64/cockroach /usr/local/bin/
## Verify version
Celia’s-MacBook-Pro:cockroach celiala$ /usr/local/bin/cockroach version
Build Tag: v21.2.0-beta.1
Build Time: 2021/09/24 15:24:05
Distribution: CCL
Platform: darwin amd64 (x86_64-apple-darwin19)
Go Version: go1.16.6
C Compiler: Clang 10.0.0
Build Commit ID: 215694e4ca58b24701d5621606aa1bfbd5783eb9
Build Type: release
Celia’s-MacBook-Pro:cockroach celiala$
## Make clean builds of CRDB, roachtest, workload, roachprod
make build # generate the full crdb binary, start at master
make bin/roachtest
make bin/workload
make bin/roachprod
## Clear out roachprod remnants, if any
bin/roachprod wipe local # just to clear out remnants, if any
bin/roachprod destroy local
## Run roachtest against beta.1 binary
bin/roachtest --local run acceptance/version-upgrade \
--debug --cockroach /usr/local/bin/cockroach
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, these steps look right. What if you bump the 30s here to something like 5 minutes? I have noticed elsewhere that the long-running migrations might take quite long to run.
err := retry.ForDuration(30*time.Second, func() error { |
69826: clusterversion: mint 21.2 cluster version r=celiala a=celiala Shortly after [Creating a release branch](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/187859111/Creating+a+release+branch) for release-21.2, we'll want to merge PRs as instructed by the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist). This PR is for Step 1a of the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist), where we add a cluster version StartX for the corresponding .0 release. Notes: - **This should NOT be merged until we've released a beta** (adding `do-not-merge` until this happens) - These are all PRs created as part of the steps of the [New major version checklist](https://cockroachlabs.atlassian.net/wiki/spaces/ENG/pages/1522270228/New+major+version+checklist): - Step 1a (you are here): #69826 - Step 1b: #69828 - Step 2a + Step 2b: #69827 - Step 2c: #69829 - Step 3: TODO Release justification: Non-production code change. Release note: None 70750: tenant: add endpoint with instant metrics r=darinpp a=darinpp Previously the tenant process was serving various metrics on `/_status/vars`. This endpoint has all the available metrics and these are updated every 10 sec. Many of the metrics show a rate that is calculated over the 10 sec interval. Some of the metrics are used by the cockroach operator to monitor the CPU workload of the tenant process and use that workload for automatic scaling. The 10 sec interval however is too long and causes a slow scaling up. The reporting of high CPU utilization can take up to 20 sec (to compute a delta). To resolve this, the PR adds a new endpoint `/_status/load` that provides an instant reading of a very small subset of the normal metrics - user and system CPU time for now. By having these be instant, the client can retrieve in quick succession, consecutive snapshots and compute a precise CPU utulization. It also allows the client to control the interval between the two pulls (as opposed to having it hard coded to 10 sec). Release note: None Co-authored-by: Celia La <celiala456@gmail.com> Co-authored-by: Darin Peshev <darinp@gmail.com>
703b3e8
to
9864415
Compare
Bumping the retry logic to 5 minutes worked, thanks! This is RFAL! |
@@ -36,7 +36,8 @@ func PredecessorVersion(buildVersion version.Version) (string, error) { | |||
// checkpoint option enabled to create the missing store directory | |||
// fixture (see runVersionUpgrade). | |||
verMap := map[string]string{ | |||
"21.2": "21.1.8", | |||
"22.1": "21.2.0-beta.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you need to rename the fixture files to checkpoint-v21.2.0-beta.2
, the snippet that moved them there isn't advanced enough to figure out the current situation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good mod the rename in my open comment. A good test would be for you to locally create the 22.1 alpha tag on this PR branch and try to run the acceptance/version-upgrade roachtest in local mode. This should then succeed and you should see it use the fixture. This indicates that it's safe to push the tag to our main repo
9864415
to
51e3879
Compare
Release justification: Non-production code change. Release note: None
51e3879
to
f8cfdae
Compare
Ack: renamed fixtures to
Ack: tests passed locally. Putting my steps here, for whoever does this next (likely me again :) )
|
TFTR! I'll merge once tests pass |
bors r+ |
Build succeeded: |
Nice, thanks for checking and glad this worked :)
(sent from mobile, please excuse my brevity)
…On Fri, Oct 1, 2021, 22:30 craig[bot] ***@***.***> wrote:
Merged #69827 <#69827> into
master.
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#69827 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZH47TONBHIQZDKHJC3UEYK47ANCNFSM5DMRMTAQ>
.
|
Shortly after Creating a release branch for release-21.2, we'll want to merge PRs as instructed by the New major version checklist.
This PR is for Step 2a + Step 2b of the New major version checklist, where we update the
PredecessorVersion
map in theroachtest
package to add an entry for the${vNEXT}
major release, as well asgenerate the fixtures for acceptance/mixed-version for
${vBRANCH_CUT}
so that the test can run on${vNEXT}
.This PR is one of the tasks detailed in #70751, which tracks all the steps relevant to creating a release branch for
${vBRANCH_CUT}
and preparing master for the${vNEXT}
major version.Release justification: Non-production code change.
Release note: None