Simplify A/B-Test orchestration #4879
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
roypat
requested review from
xmarcalx,
kalyazin,
pb8o and
Manciukic
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #4879 +/- ##
=======================================
Coverage 84.10% 84.10%
=======================================
Files 251 251
Lines 28080 28080
=======================================
Hits 23616 23616
Misses 4464 4464
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
roypat
force-pushed
the
ab-simplification
branch
3 times, most recently
from
8afc130 to
8381766
Compare
roypat
added
the
Status: Awaiting review
pb8o
reviewed
Oct 29, 2024
roypat
force-pushed
the
ab-simplification
branch
5 times, most recently
from
8e5e6cf to
f154498
Compare
roypat
force-pushed
the
ab-simplification
branch
from
d3c6591 to
8e7bd10
Compare
Eliminate the local variable and the for-break-else construct by simply returning early. Signed-off-by: Patrick Roy <[email protected]>
Our docker container uses Python 3.12 these days, so follow the comment the says "once we upgrade past 3.11, use the stdlib". Signed-off-by: Patrick Roy <[email protected]>
`None` is an invalid value, and this caused A/B-Tests to be impossible to run locally. Signed-off-by: Patrick Roy <[email protected]>
The output of the spectre/meltdown checker, and the vulnerability files on the host will not be influenced by the local checkout of the firecracker repository. Thus these A/B-tests were noops. Fix this by only doing the non-PR assertion in the nightly pipeline. Signed-off-by: Patrick Roy <[email protected]>
Currently, we have two types of pre-PR A/B-Tests: Those that depend on the repository as a whole (e.g. `cargo audit`, which checks properties of Cargo.toml), and those that depend on the pre-compiled firecracker binaries. At the moment, these tests can use the same infrastructure, because our buildkite shared build step will pre-compile binaries and then upload the entire repository as an artifact, and so both types of A/B-tests will find what they're looking for in this artifact and end up being happy. However, this model makes it very difficult to manually run A/B-tests locally (one will need to setup clones of the git repository at specific locations and then pre-compile firecracker with specific flags inside them). This patch series has the goal to simplify these manual A/B-tests, but the cost is that the pre-PR A/B tests will require different infrastructure. Thus, prepare by providing different functions for each of these types of A/B-test to use. Signed-off-by: Patrick Roy <[email protected]>
The tools/ab_test.py script relies on precompiled binaries existing in well-defined locations, and refuses to compile firecracker itself if they're missing. It derives these locations from commit SHAs, so conceptually, it makes more sense to cut out this middle step and just directly pass in directories instead of SHAs (with the expectation that the directories contain firecracker and jailer binaries). However, the commit SHAs were still used to print the commit ranges over which A/B-tests were done. It turns out that this was not working in the vast majority of cases though, as the commit log printing logic did not contain all the resolution logic that goes into compilation step (e.g. for parsing revisions and such). So just remove that part. In the EMF metrics, we now tag each report with the directory path instead of the commit SHA. For the post-merge runs, this makes no difference, as the SHA is part of the path. Signed-off-by: Patrick Roy <[email protected]>
Passing `--rev` to `tools/devtool build` will do a checkout of the specified revision into a temporary directory, compile firecracker into it, and then copy over the final binaries into build/$revision. This has advantages over our current approach for compiling A/B revisions, as by only copying the final binaries into build/$revision we save a lot of bandwidth when uploading artifacts for transfer between different buildkite steps (as we no longer include gigabytes of compilation artifacts). This now means that all revisions are compiled in the same docker container, however due to the rust-toolchain.toml this should not have an impact on cross-toolchain testability, and has the advantage that the cargo cache is shared (meaning we're hitting crates.io for downloads less). Signed-off-by: Patrick Roy <[email protected]>
Update the documentation to reflect that the script now takes directories with binaries. The old documentation was wrong anyway, since ab_test.py would fail if it couldn't find binaries in directories named matching the passed revisions after the very specific naming scheme that our buildkite infra uses. Signed-off-by: Patrick Roy <[email protected]>
In the pre-PR A/B-tests we were compiling Firecracker. Instead, explicitly rely on the precompiled binaries that we set up in the shared buildkite build step. This has the slight downside that it makes it harder to run these tests locally, as now you need to explicitly pre-compile the binaries, but arguably running these vulnerability A/B-tests locally doesnt make sense anyway, because they specifically test the security configuration on our supported .metals. While we're at it, rename the ab_* utility functions to make it obvious that they expect pre-compiled binaries. Signed-off-by: Patrick Roy <[email protected]>
Otherwise, they clash with the directories used for the pre-compiled binaries (as the `build`-subdirectories now no longer contain checked out repositories, but instead only binaries). Since each pipeline only uses git_ab_test in precisely one test, loosing the re-use is not a big deal. Signed-off-by: Patrick Roy <[email protected]>
These tags were removed in 07e564c, so the logic serves no purpose anymore (and it makes pytest print very unhelpful error messages if a test function has no doc string). Signed-off-by: Patrick Roy <[email protected]>
It's shorter and gives nicer error messages than `assert False` in a `try` block. Signed-off-by: Patrick Roy <[email protected]>
roypat
force-pushed
the
ab-simplification
branch
from
a8e998a to
1836edf
Compare
pb8o
reviewed
Oct 31, 2024
pb8o
approved these changes
Oct 31, 2024
zulinx86
approved these changes
Nov 4, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Make the A/B-Test script take two directories with firecracker/jailer binaries as arguments, instead of two git revisions
Reason
Since we extracted the build step in our buildkite infra, we refuse to compile firecracker inside of pytest. This means that running A/B-Tests outside the buildkite infra was essentially impossible, as the two revisions were resolved to a directory path as it would be created by the buildkite infra. If this directory did not exist, the script would fail. Additionally, the few git operations that the script did do on the revisions were very brittle and did not match what the extracted build step did for resolving git objects (so it could be that the build step passes, but then ab_test.py doesn't understand the git objects anymore).
License Acceptance
By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache 2.0 license. For more information on following Developer
Certificate of Origin and signing off your commits, please check
CONTRIBUTING.md.PR Checklist
PR.
CHANGELOG.md.TODOs link to an issue.contribution quality standards.
rust-vmm.