phd: automatically fetch crucible-downstairs from Buildomat
#604
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
apparently when running PHD on CI, the `phd-runner` binary isn't in the Propolis workspace, so we need to determine the Git rev at build time, instead.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has totally checked 255 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 190 | 1 | 64 | 0 |
Click to see the invalid file list
- phd-tests/runner/build.rs
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
gjcolombo
reviewed
Jan 17, 2024
gjcolombo
approved these changes
Jan 17, 2024
hawkw
added a commit
that referenced
this pull request
Jan 21, 2024
In order to ensure that changes to `propolis` don't break instance migration from previous versions, we would like to add automated testing of migrating an instance from the current `master` branch of `propolis` to run on PR branches. This PR adds an implementation of such a test to `phd`. To implement this, I've built on top of my change from PR #604 and modified the `phd` artifact store to introduce a notion of a "base" Propolis server artifact. This artifact can then be used to test migration from the "base" Propolis version to the revision under test. I've added a new test case in `migrate.rs` that creates a source VM using the "base" Propolis artifact and attempts to migrate that instance to a target VM running on the "default" Propolis artifact (the revision being tested). In order to add the new test, I've factored out test code from the existing `migrate::smoke_test` test. How `phd` should acquire a "base" Propolis artifact is configured by several new command-line arguments. `--base-propolis-branch` takes the name of a Git branch on the `propolis` repo. If this argument is provided, PHD will download the Propolis debug artifact from the HEAD commit of that branch from Buildomat. Alternatively, the `--base-propolis-commit` argument accepts a Git commit hash to download from Buildomat. Finally, the `--base-propolis-cmd` argument takes a local path to a binary to use as the "base" Propolis. All these arguments are mutually exclusive, and if none of them are provided, the migration-from-base tests are skipped. When the "base" Propolis artifact is configured from a Git branch name (i.e. the `--base-propolis-branch` CLI argument is passed), we use the Buildomat `/public/branch/{repo}/{branch-name}` endpoint, which returns the Git hash of the HEAD commit to that branch. Then, we attempt to download an artifact from Buildomat for that commit hash. An issue here is that Buildomat's branch endpoint will return the latest commit hash for that branch as soon as it sees a commit, but the artifact for that commit may not have been published yet, so downloading it will fail. Ideally, we could resolve this sort of issue by configuring the `phd-run` job for PRs to depend on the `phd-build` job for `master`, so that the branch's test run isn't started until any commits that just merged to `master` have published artifacts. However, this isn't basely possible in Buildomat (see oxidecomputer/buildomat#46). As a temporary workaround, I've added code to the PHD artifact store to retry downloading Buildomat artifacts with an exponential backoff, for up to a configurable duration (defaulting to 20 minutes). This allows us to wait for an in-progress build to complete, with a limit on how long we'll wait for. Depends on #604
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.
Currently, in order to run the
phdtests that involve Crucible disks,the path to a local
crucible-downstairsbinary must be provided via acommand-line argument. When no
crucible-downstairsbinary path isprovided, the tests that use Crucible are skipped. This means that the
tests which use Crucible disks are not currently able to run on CI, as
no
crucible-downstairsbinary is present.This branch extends the
phd-frameworkcrate's artifact store tosupport automatically fetch a
crucible-downstairsbinary fromBuildomat when configured via the command line. This PR adds a new
--crucible-downstairs-commitCLI argument tophd-runner. If present,this argument configures PHD to fetch a
crucible-downstairsbinaryfrom Buildomat.
The
--crucible-downstairs-commitargument can either be the stringauto, or a 40-character Git hash. Whenautois passed, Git revisionto fetch from Buildomat is determined automatically based on the
propolisworkspace's Git dependency on thecruciblecrate, to ensurethat the downstairs binary matches the version of the upstairs library
that Propolis was built against. Otherwise, if a Git hash is provided,
the artifact for that commit hash is fetched from Buildomat, instead.
The behavior of the existing
--crucible-downstairs-cmdis unchanged,so a local
crucible-downstairsbinary can still be used rather thandownloading it from Buildomat. This way, users can still run
phdtestsagainst a local Crucible build, if necessary. The new
--crucible-downstairs-commitargument conflicts with--crucible-downstairs-cmd, so if both are provided,phd-runnerwillprint an error. If neither argument is provided, tests that require
Crucible are skipped, maintaining the default behavior of not
downloading anything unless explicitly configured to.
The Buildomat job for Crucible publishes the entire Illumos Crucible
zone as a tarball, rather than separate files for each artifact.
Therefore, it was necessary to extend the
phd-frameworkartifact storeto support a configuration for extracting an individual file from within
a tarball. The SHA256 used for the artifact is the SHA of the entire
tarball, since Crucible's Buildomat job publishes the tarball's SHA, but
once the tarball is downloaded, the
crucible-downstairsbinary isextracted from the tarball into the object store.