test: add integration tests for upgrades #2260
Conversation
matklad
force-pushed
the
matklad/uptest
branch
11 times, most recently
from
5dd20a8
to
8d44507
Compare
matklad
changed the title
matklad
changed the title
matklad
force-pushed
the
matklad/uptest
branch
5 times, most recently
from
f9d837d
to
d9d1fdf
Compare
|
Seems not too fail locally! I think this'll make our CI quite a bit slower, but I'll like to ignore that for now, and revisit some time later hollistically, with the goal of making the overall CI faster. |
matklad
force-pushed
the
matklad/uptest
branch
4 times, most recently
from
1b8ec6b
to
dcc0402
Compare
.github/workflows/ci.yml
Outdated
| @@ -31,7 +32,7 @@ jobs: | |||
| # Docker restricts io_uring by default, running unconfined then. | |||
| options: --security-opt seccomp=unconfined | |||
| steps: | |||
| - run: apk add -U git | |||
| - run: apk add -U git -U github-cli | |||
There was a problem hiding this comment.
I think the second -U is redundant.
build.zig
Outdated
| .tracer_backend = .none, | ||
| .hash_log_mode = .none, | ||
| }); | ||
| const tigerbeetle_past = download_release(b, "latest", options.target, options.mode); |
There was a problem hiding this comment.
We do use tigerbeetle_past above for the same thing, but for multiversioning naming, I think previous works better when referring to a previous release.
past internally refers to the non-current releases within a multiversion release.
There was a problem hiding this comment.
ah, I got confused thinking that _past is the right name here, thanks!
| } | ||
| // When we fetch llvm-objcopy in build.zig, there isn't an easy way to mark it as | ||
| // executable, so do it here. | ||
| try shell.file_make_executable(cli_args.llvm_objcopy); |
There was a problem hiding this comment.
Could this be done with a system command chmod +x in build_tigerbeetle_executable_get_objcopy? I'm not sure if that's better... But it feels weird to have a build step that gets you the binary and you then need to make sure it's executable outside of that.
There was a problem hiding this comment.
I think the current solution is better than chmod, because system dependencies feel like a worse problem to me. But yeah, this is pretty hacky.
The proper way to do that would be to write a custom chmoding step, but that seems relatively large amount of code for little gain, given that the end-game is to not use vendored objcopy at all.
I'll write the custom step if you think thats worthwhile though!
There was a problem hiding this comment.
I'll write the custom step if you think thats worthwhile though!
I think it's OK as-is for now! We do want to get rid of that step anyhow at some point. (and if someone else tries to use it in the meantime, figuring out the error will hopefully discourage them 😄.)
src/integration_tests.zig
Outdated
| // Against this upgrading cluster, we are running a benchmark load and check that it finishes | ||
| // with zero status. |
There was a problem hiding this comment.
| // Against this upgrading cluster, we are running a benchmark load and check that it finishes | |
| // with zero status. | |
| // Against this upgrading cluster, we are running a benchmark load and checking that it finishes | |
| // with a zero status. |
src/integration_tests.zig
Outdated
| } | ||
| } | ||
|
|
||
| // context.shell.cwd.deleteTree(context.tmp) catch {}; |
There was a problem hiding this comment.
Do we want to clear our tmp dir?
Two motivations here: - to keep integration tests as close to the real world as possible - to allow integration-testing of upgrades!
matklad
force-pushed
the
matklad/uptest
branch
from
dcc0402
to
e59bbff
Compare
|
Looks like some merge conflicts with |
matklad
force-pushed
the
matklad/uptest
branch
from
e59bbff
to
cccb185
Compare
|
should be good now! |
Adds a real multiversion binary to our integration tests, and implements a simplistic upgrade test.
Ideally, this should be moved to our upcoming system-tests, but let's have at least a smoke test in the meantime: #2270