-
Notifications
You must be signed in to change notification settings - Fork 22
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
phd: add basic "migration-from-base" tests + machinery (#609)
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
- Loading branch information
Showing
12 changed files
with
834 additions
and
246 deletions.
There are no files selected for viewing
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
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Oops, something went wrong.
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,272 @@ | ||
| // This Source Code Form is subject to the terms of the Mozilla Public | ||
| // License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| // file, You can obtain one at https://mozilla.org/MPL/2.0/. | ||
| use super::DownloadConfig; | ||
| use anyhow::Context; | ||
| use camino::Utf8Path; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::{borrow::Cow, fmt, str::FromStr, time::Duration}; | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub(super) struct Repo(Cow<'static, str>); | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Eq, PartialEq)] | ||
| #[serde(transparent)] | ||
| pub struct Commit(String); | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| #[serde(transparent)] | ||
| pub(super) struct Series(Cow<'static, str>); | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize)] | ||
| pub struct BuildomatArtifact { | ||
| pub(super) repo: Repo, | ||
| pub(super) series: Series, | ||
| pub(super) commit: Commit, | ||
| pub(super) sha256: String, | ||
| } | ||
|
|
||
| const BASE_URI: &str = "https://buildomat.eng.oxide.computer/public"; | ||
|
|
||
| impl Repo { | ||
| pub(super) const fn from_static(s: &'static str) -> Self { | ||
| Self(Cow::Borrowed(s)) | ||
| } | ||
|
|
||
| pub(super) fn artifact_for_commit( | ||
| self, | ||
| series: Series, | ||
| commit: Commit, | ||
| filename: impl AsRef<Utf8Path>, | ||
| downloader: &DownloadConfig, | ||
| ) -> anyhow::Result<BuildomatArtifact> { | ||
| let filename = filename.as_ref(); | ||
| let sha256 = self.get_sha256(&series, &commit, filename, downloader)?; | ||
|
|
||
| Ok(BuildomatArtifact { repo: self, series, commit, sha256 }) | ||
| } | ||
|
|
||
| pub(super) fn get_branch_head( | ||
| &self, | ||
| branch: &str, | ||
| ) -> anyhow::Result<Commit> { | ||
| (|| { | ||
| let uri = format!("{BASE_URI}/branch/{self}/{branch}"); | ||
| let client = reqwest::blocking::ClientBuilder::new() | ||
| .timeout(Duration::from_secs(5)) | ||
| .build()?; | ||
| let req = client.get(uri).build()?; | ||
| let rsp = client.execute(req)?; | ||
| let status = rsp.status(); | ||
| anyhow::ensure!(status.is_success(), "HTTP status: {status}"); | ||
| let bytes = rsp.bytes()?; | ||
| str_from_bytes(&bytes)?.parse::<Commit>() | ||
| })() | ||
| .with_context(|| { | ||
| format!("Failed to determine HEAD commit for {self}@{branch}") | ||
| }) | ||
| } | ||
|
|
||
| fn get_sha256( | ||
| &self, | ||
| series: &Series, | ||
| commit: &Commit, | ||
| filename: &Utf8Path, | ||
| downloader: &DownloadConfig, | ||
| ) -> anyhow::Result<String> { | ||
| (|| { | ||
| let filename = filename | ||
| .file_name() | ||
| .ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Buildomat filename has no filename: {filename:?}" | ||
| ) | ||
| })? | ||
| // Strip the file extension, if any. | ||
| // | ||
| // Note: we use `Utf8PathBuf::file_name` and then split on '.'s | ||
| // rather than using `Utf8PathBuf::file_stem`, because the latter | ||
| // only strips off the rightmost file extension, rather than all | ||
| // extensions. So, "foo.tar.gz" has a `file_stem()` of "foo.tar", | ||
| // rather than "foo". | ||
| // | ||
| // TODO(eliza): `std::path::Path` has an unstable `file_prefix()` | ||
| // method, which does exactly what we would want here (see | ||
| // https://github.com/rust-lang/rust/issues/86319). If this is | ||
| // stabilized, and `camino` adds a `file_prefix()` method wrapping | ||
| // it, this code can be replaced with just `filename.file_prefix()`. | ||
| .split('.') | ||
| .next() | ||
| .ok_or_else(|| { | ||
| anyhow::anyhow!( | ||
| "Buildomat filename has no filename prefix: {filename:?}" | ||
| ) | ||
| })?; | ||
| let uri = format!("{BASE_URI}/file/{self}/{series}/{commit}/{filename}.sha256.txt"); | ||
| let bytes = downloader.download_buildomat_uri(&uri)?; | ||
| str_from_bytes(&bytes).map(String::from) | ||
| })().with_context(|| { | ||
| format!("Failed to get SHA256 for {self}@{commit}, series: {series}, file: {filename})") | ||
| }) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for Repo { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| self.0.fmt(f) | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for Commit { | ||
| type Err = anyhow::Error; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let s = s.trim(); | ||
|
|
||
| // Ensure this looks like a valid Git commit. | ||
| anyhow::ensure!( | ||
| s.len() == 40, | ||
| "Buildomat requires full (40-character) Git commit hashes" | ||
| ); | ||
|
|
||
| for c in s.chars() { | ||
| if !c.is_ascii_hexdigit() { | ||
| anyhow::bail!( | ||
| "'{c}' is not a valid hexadecimal digit; Git \ | ||
| commit hashes should consist of the characters \ | ||
| [0-9, a-f, A-F]" | ||
| ); | ||
| } | ||
| } | ||
|
|
||
| Ok(Self(s.to_string())) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for Commit { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| f.write_str(&self.0) | ||
| } | ||
| } | ||
|
|
||
| impl<'de> Deserialize<'de> for Commit { | ||
| fn deserialize<D>(deserializer: D) -> Result<Self, D::Error> | ||
| where | ||
| D: serde::Deserializer<'de>, | ||
| { | ||
| let s = String::deserialize(deserializer)?; | ||
| FromStr::from_str(&s).map_err(serde::de::Error::custom) | ||
| } | ||
| } | ||
|
|
||
| impl Series { | ||
| pub(super) const fn from_static(s: &'static str) -> Self { | ||
| Self(Cow::Borrowed(s)) | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for Series { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| self.0.fmt(f) | ||
| } | ||
| } | ||
|
|
||
| impl BuildomatArtifact { | ||
| pub(super) fn uri(&self, filename: impl AsRef<Utf8Path>) -> String { | ||
| let Self { | ||
| repo: Repo(ref repo), | ||
| series: Series(ref series), | ||
| commit: Commit(ref commit), | ||
| .. | ||
| } = self; | ||
| let filename = filename.as_ref(); | ||
| format!("{BASE_URI}/file/{repo}/{series}/{commit}/{filename}") | ||
| } | ||
| } | ||
|
|
||
| impl fmt::Display for BuildomatArtifact { | ||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | ||
| let Self { | ||
| repo: Repo(ref repo), | ||
| series: Series(ref series), | ||
| commit: Commit(ref commit), | ||
| .. | ||
| } = self; | ||
| write!(f, "Buildomat {repo}/{series}@{commit}") | ||
| } | ||
| } | ||
|
|
||
| impl super::DownloadConfig { | ||
| /// Download a file from the provided Buildomat URI. | ||
| /// | ||
| /// This method will retry the download if Buildomat returns an error that | ||
| /// indicates a file does not yet exist, for up to the configurable maximum | ||
| /// retry duration. This retry logic serves as a mechanism for PHD to wait | ||
| /// for an artifact we expect to exist to be published, when the build that | ||
| /// publishes that artifact is still in progress. | ||
| pub(super) fn download_buildomat_uri( | ||
| &self, | ||
| uri: &str, | ||
| ) -> anyhow::Result<bytes::Bytes> { | ||
| tracing::info!( | ||
| timeout = ?self.timeout, | ||
| %uri, | ||
| "Downloading file from Buildomat...", | ||
| ); | ||
| let client = reqwest::blocking::ClientBuilder::new() | ||
| .timeout(self.timeout) | ||
| .build()?; | ||
| let try_download = || { | ||
| let request = client | ||
| .get(uri) | ||
| .build() | ||
| // failing to build the request is a permanent (non-retryable) | ||
| // error, because any retries will use the same URI and request | ||
| // configuration, so they'd fail as well. | ||
| .map_err(|e| backoff::Error::permanent(e.into()))?; | ||
|
|
||
| let response = client | ||
| .execute(request) | ||
| .map_err(|e| backoff::Error::transient(e.into()))?; | ||
| if !response.status().is_success() { | ||
| // when downloading a file from buildomat, we currently retry | ||
| // all errors, since buildomat returns 500s when an artifact | ||
| // doesn't exist. hopefully, this will be fixed upstream soon: | ||
| // https://github.com/oxidecomputer/buildomat/pull/48 | ||
| let err = anyhow::anyhow!( | ||
| "Buildomat returned HTTP error {}", | ||
| response.status() | ||
| ); | ||
| return Err(backoff::Error::transient(err)); | ||
| } | ||
| Ok(response) | ||
| }; | ||
|
|
||
| let log_retry = |error, wait| { | ||
| tracing::info!( | ||
| %error, | ||
| %uri, | ||
| "Buildomat download failed, trying again in {wait:?}..." | ||
| ); | ||
| }; | ||
|
|
||
| let bytes = backoff::retry_notify( | ||
| self.buildomat_backoff.clone(), | ||
| try_download, | ||
| log_retry, | ||
| ) | ||
| .map_err(|e| match e { | ||
| backoff::Error::Permanent(e) => e, | ||
| backoff::Error::Transient { err, .. } => err, | ||
| }) | ||
| .with_context(|| format!("Failed to download '{uri}' from Buildomat"))? | ||
| .bytes()?; | ||
|
|
||
| Ok(bytes) | ||
| } | ||
| } | ||
|
|
||
| fn str_from_bytes(bytes: &bytes::Bytes) -> anyhow::Result<&str> { | ||
| Ok(std::str::from_utf8(bytes.as_ref())?.trim()) | ||
| } |
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
Oops, something went wrong.