From ea0c1d4e0a709e9eef7a8947f40d97f4f63cfccd Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:14:46 -0500 Subject: [PATCH 01/26] lib: derive some helpful traits for AppConfig Adds `Debug`, `Clone`, `Eq` and `PartialEq` to the `AppConfig`. --- ci-bench-runner/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index 735513d..eb0a6d0 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -43,7 +43,7 @@ struct AppState { } /// The application's configuration -#[derive(Deserialize)] +#[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct AppConfig { /// Base URL used for the GitHub API (used to mock out the API in tests) pub github_api_url_override: Option, From 0508dedd1cfba607f2f431f0ee5598e338b3c9e4 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:18:53 -0500 Subject: [PATCH 02/26] ci-bench-runner: add sqlx recommended build script --- ci-bench-runner/build.rs | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 ci-bench-runner/build.rs diff --git a/ci-bench-runner/build.rs b/ci-bench-runner/build.rs new file mode 100644 index 0000000..3026ceb --- /dev/null +++ b/ci-bench-runner/build.rs @@ -0,0 +1,7 @@ +fn main() { + // In some cases if new migrations are added without accompanying Rust src changes + // `cargo build` isn't smart enough to detect the need for recompilation to pick up + // the new embedded migrations. This build script is the recommended workaround. + // See + println!("cargo:rerun-if-changed=migrations"); +} From 518487bd438c6c8708403c385ea830def58f60a1 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:26:21 -0500 Subject: [PATCH 03/26] lib: tidy up get_cachegrind_diff url extract * Reduce duplication in error mapping. * Use StatusCode::BAD_REQUEST for all error cases. --- ci-bench-runner/src/lib.rs | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index eb0a6d0..3175656 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -142,11 +142,14 @@ async fn get_cachegrind_diff( ) -> axum::response::Result { // Extract commit hashes from URL let mut commit_parts = compared_commits.split(':'); - let baseline_commit = commit_parts.next().ok_or("malformed URL")?; - let candidate_commit = commit_parts.next().ok_or("malformed URL")?; - if commit_parts.next().is_some() { - Err((StatusCode::BAD_REQUEST, "malformed URL"))?; - } + let (baseline_commit, candidate_commit) = match ( + commit_parts.next(), + commit_parts.next(), + commit_parts.next(), + ) { + (Some(baseline), Some(candidate), None) => (baseline, candidate), + _ => Err((StatusCode::BAD_REQUEST, "malformed URL"))?, + }; let result = state .db From f846545dd00ac23347c452a01ab061178b651d25 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:28:19 -0500 Subject: [PATCH 04/26] lib: simplify result bindings in get_cachegrind_diff The `result` bindings can be inlined. --- ci-bench-runner/src/lib.rs | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index 3175656..0332d38 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -151,18 +151,15 @@ async fn get_cachegrind_diff( _ => Err((StatusCode::BAD_REQUEST, "malformed URL"))?, }; - let result = state + Ok(state .db .cachegrind_diff(baseline_commit, candidate_commit, &scenario_name) .await - .map_err(|_| "internal server error")?; - - let result = result.ok_or(( - StatusCode::NOT_FOUND, - "comparison not found for the provided commit hashes and scenario", - ))?; - - Ok(result) + .map_err(|_| "internal server error")? + .ok_or(( + StatusCode::NOT_FOUND, + "comparison not found for the provided commit hashes and scenario", + ))?) } /// Handles an incoming GitHub webhook From 48854ea659d8e609fe6a592c413ff240b6c973cb Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:32:30 -0500 Subject: [PATCH 05/26] lib: consistently include header name in trace! logs --- ci-bench-runner/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index 0332d38..3babd2b 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -181,7 +181,7 @@ async fn handle_github_webhook( }; if !verify_webhook_signature(&body, signature, &state.config.webhook_secret) { - trace!("invalid signature, ignoring event"); + trace!("{WEBHOOK_SIGNATURE_HEADER} invalid signature, ignoring event"); return StatusCode::BAD_REQUEST; } From a481c5c83ead4131ca2c338a936af45e638a612a Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:33:57 -0500 Subject: [PATCH 06/26] lib: derive helpful traits for CommitIdentifier Adds `Debug`, `Eq`, `PartialEq`. --- ci-bench-runner/src/lib.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/lib.rs b/ci-bench-runner/src/lib.rs index 3babd2b..1b7b378 100644 --- a/ci-bench-runner/src/lib.rs +++ b/ci-bench-runner/src/lib.rs @@ -222,7 +222,7 @@ pub static WEBHOOK_SIGNATURE_HEADER: &str = "X-Hub-Signature-256"; pub static WEBHOOK_EVENT_HEADER: &str = "X-GitHub-Event"; /// Identifies a specific commit in a repository -#[derive(Clone)] +#[derive(Debug, Clone, Eq, PartialEq)] pub struct CommitIdentifier { /// The URL at which the repository can be cloned pub clone_url: String, From 6bd6d2c730ad67ade182ea664db30b8522ec04f3 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:35:21 -0500 Subject: [PATCH 07/26] db: derive Debug for QueuedEvent --- ci-bench-runner/src/db.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ci-bench-runner/src/db.rs b/ci-bench-runner/src/db.rs index 7b3ab7d..93c6f60 100644 --- a/ci-bench-runner/src/db.rs +++ b/ci-bench-runner/src/db.rs @@ -12,6 +12,7 @@ use tokio::sync::Mutex; use uuid::Uuid; /// An enqueued GitHub event +#[derive(Debug)] pub struct QueuedEvent { /// An internal id for this event (i.e. not GitHub's) pub id: Uuid, From 2b06f7a3b42da020965e2470b61a3888b7054c91 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:38:45 -0500 Subject: [PATCH 08/26] db: derive Debug for BenchResult --- ci-bench-runner/src/db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/db.rs b/ci-bench-runner/src/db.rs index 93c6f60..e78e708 100644 --- a/ci-bench-runner/src/db.rs +++ b/ci-bench-runner/src/db.rs @@ -64,7 +64,7 @@ pub struct BenchJob { } /// A result for a specific benchmark scenario -#[derive(sqlx::FromRow)] +#[derive(Debug, sqlx::FromRow)] pub struct BenchResult { /// The scenario's name pub scenario_name: String, From f1674170464b7243bb9f111267779c115a714c03 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:39:26 -0500 Subject: [PATCH 09/26] db: derive Debug for ComparisonResult --- ci-bench-runner/src/db.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ci-bench-runner/src/db.rs b/ci-bench-runner/src/db.rs index e78e708..4b566ab 100644 --- a/ci-bench-runner/src/db.rs +++ b/ci-bench-runner/src/db.rs @@ -76,6 +76,7 @@ pub struct BenchResult { } /// The results of a comparison between two branches of rustls +#[derive(Debug)] pub struct ComparisonResult { /// The diffs, per scenario pub diffs: Vec, From 0761d696097966f04b87d38ea07cae8b95fe0d01 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:40:27 -0500 Subject: [PATCH 10/26] db: use Self for variant in TryFrom for ScenarioKind --- ci-bench-runner/src/db.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/db.rs b/ci-bench-runner/src/db.rs index 4b566ab..8af5503 100644 --- a/ci-bench-runner/src/db.rs +++ b/ci-bench-runner/src/db.rs @@ -125,7 +125,7 @@ impl TryFrom for ScenarioKind { fn try_from(value: i64) -> Result { match value { - 0 => Ok(ScenarioKind::Icount), + 0 => Ok(Self::Icount), kind => bail!("invalid scenario kind: {kind}"), } } From 184a5a820e193a1c9a8de2b9b4625afc373767a2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:50:28 -0500 Subject: [PATCH 11/26] event_queue: inline active_job_id in EventQueue::new This binding wasn't being used for anything and can be inlined. --- ci-bench-runner/src/event_queue.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 5481f63..4855a61 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -41,9 +41,8 @@ impl EventQueue { ) -> Self { let (worker_tx, event_enqueued_rx) = tokio::sync::mpsc::unbounded_channel(); - let active_job_id = Arc::new(Mutex::new(None)); let queue = Self { - active_job_id, + active_job_id: Arc::new(Mutex::new(None)), event_enqueued_tx: worker_tx, db, }; From 17390410f4b3b7340c05499a79bedde93ca49328 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:52:53 -0500 Subject: [PATCH 12/26] event_queue: start_and_supervise_queue_processing move and return Self This lets the caller use the call to `start_and_supervise_queue_processing` as the result from `new` directly. --- ci-bench-runner/src/event_queue.rs | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 4855a61..9f67cc6 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -52,18 +52,17 @@ impl EventQueue { config, bench_runner, octocrab, - ); - queue + ) } /// Starts and supervises the background queue processing task fn start_and_supervise_queue_processing( - &self, + self, event_enqueued_rx: UnboundedReceiver<()>, config: Arc, bench_runner: Arc, octocrab: CachedOctocrab, - ) { + ) -> Self { let active_job_id = self.active_job_id.clone(); let queue = self.clone(); let event_enqueued_rx = Arc::new(tokio::sync::Mutex::new(event_enqueued_rx)); @@ -102,6 +101,8 @@ impl EventQueue { tokio::time::sleep(Duration::from_secs(1)).await; } }); + + self } /// Spawns a tokio task to process queued events in the background. From e42a493b8bc01ef6aedf8df6fb14456437421c18 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:57:16 -0500 Subject: [PATCH 13/26] Cargo: use reqwest with rustls + webpki This avoids OpenSSL + pkg-config dev-dependencies. --- ci-bench-runner/Cargo.lock | 97 -------------------------------------- ci-bench-runner/Cargo.toml | 2 +- 2 files changed, 1 insertion(+), 98 deletions(-) diff --git a/ci-bench-runner/Cargo.lock b/ci-bench-runner/Cargo.lock index f28690e..17936f4 100644 --- a/ci-bench-runner/Cargo.lock +++ b/ci-bench-runner/Cargo.lock @@ -557,21 +557,6 @@ version = "1.0.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "3f9eec918d3f24069decb9af1554cad7c880e2da24a9afd88aca000531ab82c1" -[[package]] -name = "foreign-types" -version = "0.3.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "f6f339eb8adc052cd2ca78910fda869aefa38d22d5cb648e6485e4d3fc06f3b1" -dependencies = [ - "foreign-types-shared", -] - -[[package]] -name = "foreign-types-shared" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "00b0228411908ca8685dba7fc2cdd70ec9990a6e753e89b6ac91a84c40fbaf4b" - [[package]] name = "form_urlencoded" version = "1.2.0" @@ -945,19 +930,6 @@ dependencies = [ "tokio-io-timeout", ] -[[package]] -name = "hyper-tls" -version = "0.5.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "d6183ddfa99b85da61a140bea0efc93fdf56ceaa041b37d553518030827f9905" -dependencies = [ - "bytes", - "hyper", - "native-tls", - "tokio", - "tokio-native-tls", -] - [[package]] name = "iana-time-zone" version = "0.1.58" @@ -1188,24 +1160,6 @@ dependencies = [ "windows-sys", ] -[[package]] -name = "native-tls" -version = "0.2.11" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "07226173c32f2926027b63cce4bcd8076c3552846cbe7925f3aaffeac0a3b92e" -dependencies = [ - "lazy_static", - "libc", - "log", - "openssl", - "openssl-probe", - "openssl-sys", - "schannel", - "security-framework", - "security-framework-sys", - "tempfile", -] - [[package]] name = "nom" version = "7.1.3" @@ -1347,50 +1301,12 @@ version = "1.18.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "dd8b5dd2ae5ed71462c540258bedcb51965123ad7e7ccf4b9a8cafaa4a63576d" -[[package]] -name = "openssl" -version = "0.10.58" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a9dfc0783362704e97ef3bd24261995a699468440099ef95d869b4d9732f829a" -dependencies = [ - "bitflags 2.4.1", - "cfg-if", - "foreign-types", - "libc", - "once_cell", - "openssl-macros", - "openssl-sys", -] - -[[package]] -name = "openssl-macros" -version = "0.1.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "a948666b637a0f465e8564c73e89d4dde00d72d4d473cc972f390fc3dcee7d9c" -dependencies = [ - "proc-macro2", - "quote", - "syn 2.0.38", -] - [[package]] name = "openssl-probe" version = "0.1.5" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "ff011a302c396a5197692431fc1948019154afc178baf7d8e37367442a4601cf" -[[package]] -name = "openssl-sys" -version = "0.9.94" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "2f55da20b29f956fb01f0add8683eb26ee13ebe3ebd935e49898717c6b4b2830" -dependencies = [ - "cc", - "libc", - "pkg-config", - "vcpkg", -] - [[package]] name = "overload" version = "0.1.1" @@ -1671,12 +1587,10 @@ dependencies = [ "http-body", "hyper", "hyper-rustls", - "hyper-tls", "ipnet", "js-sys", "log", "mime", - "native-tls", "once_cell", "percent-encoding", "pin-project-lite", @@ -1687,7 +1601,6 @@ dependencies = [ "serde_urlencoded", "system-configuration", "tokio", - "tokio-native-tls", "tokio-rustls", "tower-service", "url", @@ -2526,16 +2439,6 @@ dependencies = [ "syn 2.0.38", ] -[[package]] -name = "tokio-native-tls" -version = "0.3.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "bbae76ab933c85776efabc971569dd6119c580d8f5d448769dec1764bf796ef2" -dependencies = [ - "native-tls", - "tokio", -] - [[package]] name = "tokio-rustls" version = "0.24.1" diff --git a/ci-bench-runner/Cargo.toml b/ci-bench-runner/Cargo.toml index 6e0cb48..49a6b93 100644 --- a/ci-bench-runner/Cargo.toml +++ b/ci-bench-runner/Cargo.toml @@ -27,5 +27,5 @@ uuid = { version = "1.4.1", features = ["v4", "serde"] } [dev-dependencies] ctor = "0.2.5" -reqwest = "0.11.22" +reqwest = { version = "0.11.22", default-features = false, features = ["rustls-tls-webpki-roots"] } wiremock = "0.5.19" From 4440827434022dd101dd0f861c7a810bad5a61e3 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 10:57:56 -0500 Subject: [PATCH 14/26] flake: add valgrind as a dev-dependency The unit tests for the Rust app require cg-diff be in PATH. --- flake.nix | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/flake.nix b/flake.nix index bcd7e83..1c83ad3 100644 --- a/flake.nix +++ b/flake.nix @@ -15,7 +15,7 @@ cargoToml = builtins.fromTOML (builtins.readFile "${crateDir}/Cargo.toml"); - devDeps = with pkgs; [ ansible ansible-lint ]; + devDeps = with pkgs; [ ansible ansible-lint valgrind ]; mkDevShell = rustc: pkgs.mkShell { @@ -35,9 +35,7 @@ packages.ci-bench-runner = pkgs.rustPlatform.buildRustPackage { inherit (cargoToml.package) name version; src = crateDir; - cargoLock = { - lockFile = "${crateDir}/Cargo.lock"; - }; + cargoLock = { lockFile = "${crateDir}/Cargo.lock"; }; doCheck = false; # Some tests require platform certs. }; From 9c7bae346f14428a132cccc50108ec06634c13f7 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:00:22 -0500 Subject: [PATCH 15/26] event_queue: log active_job_id in start_and_supervise_queue_processing --- ci-bench-runner/src/event_queue.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 9f67cc6..9ff4a8b 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -75,7 +75,7 @@ impl EventQueue { octocrab.clone(), ); - info!("background task started for event queue"); + info!("background task started for event queue job {active_job_id:?}"); match background_task.await { Ok(Ok(_)) => { // The task finished normally, no need to restart it @@ -85,14 +85,14 @@ impl EventQueue { // The task finished with an error error!( cause = e.to_string(), - "job queue background task errored, restarting in 1s" + "job queue background task {active_job_id:?} errored, restarting in 1s" ); } Err(e) => { // The task panicked or was cancelled error!( cause = e.to_string(), - "job queue background task crashed, restarting in 1s" + "job queue background task {active_job_id:?} crashed, restarting in 1s" ); } } From 5899707be35bb7e34bcb41ab4c06f46e65427014 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:03:45 -0500 Subject: [PATCH 16/26] event_queue: use Self in AllowedEvent::from_event_string. --- ci-bench-runner/src/event_queue.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 9ff4a8b..cd5d2c4 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -236,12 +236,12 @@ pub enum AllowedEvent { } impl AllowedEvent { - fn from_event_string(event: &str) -> Option { + fn from_event_string(event: &str) -> Option { let kind = match event { - "issue_comment" => AllowedEvent::IssueComment, - "push" => AllowedEvent::Push, - "pull_request" => AllowedEvent::PullRequest, - "pull_request_review" => AllowedEvent::PullRequestReview, + "issue_comment" => Self::IssueComment, + "push" => Self::Push, + "pull_request" => Self::PullRequest, + "pull_request_review" => Self::PullRequestReview, _ => return None, }; From 6f0a5139248d848654ac318767c782bd5d66b695 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:05:41 -0500 Subject: [PATCH 17/26] event_queue: simplify AllowedEvent::from_event_string return --- ci-bench-runner/src/event_queue.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index cd5d2c4..4282d1c 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -237,15 +237,13 @@ pub enum AllowedEvent { impl AllowedEvent { fn from_event_string(event: &str) -> Option { - let kind = match event { + Some(match event { "issue_comment" => Self::IssueComment, "push" => Self::Push, "pull_request" => Self::PullRequest, "pull_request_review" => Self::PullRequestReview, _ => return None, - }; - - Some(kind) + }) } } From 6ce45ca3c718193afb5f089d4b53302dc575f9d1 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:07:07 -0500 Subject: [PATCH 18/26] event_queue: hand-impl Debug for JobContext I suspect it might be useful to debug the event, job_id, and output dir. --- ci-bench-runner/src/event_queue.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index 4282d1c..dc60c20 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -1,3 +1,4 @@ +use std::fmt::{Debug, Formatter}; use std::path::PathBuf; use std::sync::{Arc, Mutex}; use std::time::Duration; @@ -264,6 +265,16 @@ pub struct JobContext<'a> { pub db: Db, } +impl<'a> Debug for JobContext<'a> { + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_struct("JobContext") + .field("event", &self.event) + .field("job_id", &self.job_id) + .field("job_output_dir", &self.job_output_dir) + .finish() + } +} + #[derive(Serialize, Deserialize)] pub struct JobView { #[serde(with = "time::serde::rfc3339")] From cc615a702d9b5c8cc854b54a9502c4632c4acc55 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:08:41 -0500 Subject: [PATCH 19/26] event_queue: derive Debug for JobView --- ci-bench-runner/src/event_queue.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/event_queue.rs b/ci-bench-runner/src/event_queue.rs index dc60c20..e72bbf5 100644 --- a/ci-bench-runner/src/event_queue.rs +++ b/ci-bench-runner/src/event_queue.rs @@ -275,7 +275,7 @@ impl<'a> Debug for JobContext<'a> { } } -#[derive(Serialize, Deserialize)] +#[derive(Debug, Serialize, Deserialize)] pub struct JobView { #[serde(with = "time::serde::rfc3339")] pub created_utc: OffsetDateTime, From e2ae2db26c7fcb595ddc5ef56755382397eedf12 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:11:15 -0500 Subject: [PATCH 20/26] github: derive helpful traits for api mod types --- ci-bench-runner/src/github.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/ci-bench-runner/src/github.rs b/ci-bench-runner/src/github.rs index d692a97..44d7efa 100644 --- a/ci-bench-runner/src/github.rs +++ b/ci-bench-runner/src/github.rs @@ -19,21 +19,21 @@ pub mod api { use octocrab::models::pulls::PullRequest; use serde::Deserialize; - #[derive(Deserialize)] + #[derive(Debug, Clone, PartialEq, Deserialize)] pub struct PullRequestReviewEvent { pub action: String, pub review: Review, pub pull_request: PullRequest, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Review { pub author_association: String, pub state: String, pub commit_id: String, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct PushEvent { #[serde(rename = "ref")] pub git_ref: String, @@ -42,37 +42,37 @@ pub mod api { pub deleted: bool, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct CommentEvent { pub action: String, pub comment: Comment, pub issue: Issue, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Comment { pub author_association: String, pub body: String, pub user: GitHubUser, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Issue { pub number: u64, pub pull_request: Option, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct PullRequestLite { pub url: String, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct GitHubUser { pub login: String, } - #[derive(Deserialize)] + #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct Repo { pub clone_url: String, } From d6018182ac255eb6bdd9a9cab4c1b5f8ea323cd2 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:12:07 -0500 Subject: [PATCH 21/26] github: derive Debug for CachedOctocrab --- ci-bench-runner/src/github.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ci-bench-runner/src/github.rs b/ci-bench-runner/src/github.rs index 44d7efa..29eaec2 100644 --- a/ci-bench-runner/src/github.rs +++ b/ci-bench-runner/src/github.rs @@ -79,7 +79,7 @@ pub mod api { } /// Provides access to an authenticated `Octocrab` client -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct CachedOctocrab { /// Initial GitHub client, authenticated as our GitHub App app_client: Octocrab, From 12d68f20d5cb237424a32b204caf69ad5bc370c3 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:21:55 -0500 Subject: [PATCH 22/26] github: avoid potential timing side channel in MAC verify Rather than converting the finalized MAC tag to a byte slice and comparing with standard equality check, use `mac.verify_slice`, a constant time comparison. This removes the ability to debug the computed vs actual tag but I think in practice this won't be super useful and avoiding timing side channels is more important. --- ci-bench-runner/src/github.rs | 10 +--------- 1 file changed, 1 insertion(+), 9 deletions(-) diff --git a/ci-bench-runner/src/github.rs b/ci-bench-runner/src/github.rs index 29eaec2..fb917be 100644 --- a/ci-bench-runner/src/github.rs +++ b/ci-bench-runner/src/github.rs @@ -6,7 +6,6 @@ use hmac::{Hmac, Mac}; use jsonwebtoken::EncodingKey; use octocrab::models::{InstallationId, StatusState}; use octocrab::Octocrab; -use sha2::digest::FixedOutput; use sha2::Sha256; use tracing::{error, trace, warn}; @@ -210,15 +209,8 @@ pub fn verify_webhook_signature(body: &[u8], signature: &str, secret: &str) -> b // Safe to unwrap because any key is valid let mut mac = Hmac::::new_from_slice(secret.as_bytes()).unwrap(); - mac.update(body); - let output = mac.finalize_fixed(); - trace!( - "verifying webhook signature; provided = {signature}; computed = {:?}", - hex::encode(output.as_slice()) - ); - - signature_bytes == output.as_slice() + mac.verify_slice(signature_bytes.as_slice()).is_ok() } #[cfg(test)] From 95b5252a8557802456d25ba97990885f845b9ad9 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:26:58 -0500 Subject: [PATCH 23/26] runner: in-line single use write_log helper --- ci-bench-runner/src/runner.rs | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/ci-bench-runner/src/runner.rs b/ci-bench-runner/src/runner.rs index a83c1ce..73e0a05 100644 --- a/ci-bench-runner/src/runner.rs +++ b/ci-bench-runner/src/runner.rs @@ -21,6 +21,7 @@ pub trait BenchRunner: Send + Sync { } /// A bench runner that runs benchmarks locally +#[derive(Debug)] pub struct LocalBenchRunner; impl BenchRunner for LocalBenchRunner { @@ -161,6 +162,7 @@ fn run_command(mut command: Command, logs: &mut Vec) -> anyhow::Result<()> } /// Logs for a specific command +#[derive(Debug)] pub struct Log { /// The command in question pub command: String, @@ -178,17 +180,13 @@ pub fn write_logs_for_run(s: &mut String, logs: &[Log]) { } for log in logs { - write_log(s, log); + write_log_part(s, "command", &log.command); + write_log_part(s, "cwd", &log.cwd); + write_log_part(s, "stdout", &String::from_utf8_lossy(&log.stdout)); + write_log_part(s, "stderr", &String::from_utf8_lossy(&log.stderr)); } } -fn write_log(s: &mut String, log: &Log) { - write_log_part(s, "command", &log.command); - write_log_part(s, "cwd", &log.cwd); - write_log_part(s, "stdout", &String::from_utf8_lossy(&log.stdout)); - write_log_part(s, "stderr", &String::from_utf8_lossy(&log.stderr)); -} - fn write_log_part(s: &mut String, part_name: &str, part: &str) { write!(s, "{part_name}:").ok(); if part.trim().is_empty() { From d4a3ccb8da6d7372030fcecb54e026d441efaa49 Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:30:31 -0500 Subject: [PATCH 24/26] github/job: use id to detect our own comment, not login Previously the `handle_issue_comment` logic used the GitHub login string of the user posting the comment to decide if it was a comment from the bot. This requires hardcoding the name of the application, which may be fragile. Instead, include the ID from the GitHub user model and in `handle_issue_comment` check if it matches the configured application GitHub application ID. --- ci-bench-runner/src/github.rs | 1 + ci-bench-runner/src/job/bench_pr.rs | 4 ++-- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/ci-bench-runner/src/github.rs b/ci-bench-runner/src/github.rs index fb917be..e1e1519 100644 --- a/ci-bench-runner/src/github.rs +++ b/ci-bench-runner/src/github.rs @@ -69,6 +69,7 @@ pub mod api { #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] pub struct GitHubUser { pub login: String, + pub id: u64, } #[derive(Debug, Clone, Eq, PartialEq, Deserialize)] diff --git a/ci-bench-runner/src/job/bench_pr.rs b/ci-bench-runner/src/job/bench_pr.rs index 081e0a9..12eba90 100644 --- a/ci-bench-runner/src/job/bench_pr.rs +++ b/ci-bench-runner/src/job/bench_pr.rs @@ -63,8 +63,8 @@ pub async fn handle_issue_comment(ctx: JobContext<'_>) -> anyhow::Result<()> { trace!("ignoring event for `{}` action", payload.action); return Ok(()); } - if payload.comment.user.login == "rustls-bench" { - trace!("ignoring comment from rustls-bench"); + if payload.comment.user.id == ctx.config.github_app_id { + trace!("ignoring comment from ourselves"); return Ok(()); } if !ALLOWED_AUTHOR_ASSOCIATIONS.contains(&payload.comment.author_association.as_str()) { From de1d540592e0f41c147a47a67c487776cda3610f Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:34:32 -0500 Subject: [PATCH 25/26] bench_pr: derive Debug for PrBranches, BenchPrError, BenchPrLogs --- ci-bench-runner/src/job/bench_pr.rs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/ci-bench-runner/src/job/bench_pr.rs b/ci-bench-runner/src/job/bench_pr.rs index 12eba90..40a494f 100644 --- a/ci-bench-runner/src/job/bench_pr.rs +++ b/ci-bench-runner/src/job/bench_pr.rs @@ -426,18 +426,19 @@ fn calculate_significance_thresholds(historical_results: Vec) -> Ha outlier_bounds } -#[derive(Clone)] +#[derive(Debug, Clone)] pub struct PrBranches { pub baseline: CommitIdentifier, pub candidate: CommitIdentifier, } +#[derive(Debug)] struct BenchPrError { error: anyhow::Error, logs: BenchPrLogs, } -#[derive(Default)] +#[derive(Debug, Default)] struct BenchPrLogs { base: Vec, candidate: Vec, From 61b61dd8126dafd536df6c92b579e829a06b7e3d Mon Sep 17 00:00:00 2001 From: Daniel McCarney Date: Mon, 6 Nov 2023 11:37:58 -0500 Subject: [PATCH 26/26] ci: add basic GitHub actions + dependabot setup --- .github/dependabot.yml | 14 ++++++++ .github/workflows/rust.yml | 69 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) create mode 100644 .github/dependabot.yml create mode 100644 .github/workflows/rust.yml diff --git a/.github/dependabot.yml b/.github/dependabot.yml new file mode 100644 index 0000000..a51aeec --- /dev/null +++ b/.github/dependabot.yml @@ -0,0 +1,14 @@ +version: 2 +updates: + - package-ecosystem: cargo + directory: "/" + schedule: + interval: daily + time: "07:00" + open-pull-requests-limit: 10 + - package-ecosystem: "github-actions" + directory: ".github/workflows" + schedule: + interval: "daily" + time: "07:00" + open-pull-requests-limit: 10 diff --git a/.github/workflows/rust.yml b/.github/workflows/rust.yml new file mode 100644 index 0000000..36eed5d --- /dev/null +++ b/.github/workflows/rust.yml @@ -0,0 +1,69 @@ +name: Rust + +on: + push: + pull_request: + +jobs: + format: + name: Format + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: rustfmt + - name: Check formatting + run: cargo fmt --all -- --check + + clippy: + name: Clippy + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - name: Checkout sources + uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + with: + components: clippy + - run: cargo clippy --locked -- --deny warnings + + docs: + name: Check for documentation errors + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - name: Checkout sources + uses: actions/checkout@v4 + - name: Install rust toolchain + uses: dtolnay/rust-toolchain@stable + - name: cargo doc + run: cargo doc --locked --no-deps --document-private-items + env: + RUSTDOCFLAGS: -Dwarnings + + stable: + name: Stable Rust + runs-on: ubuntu-latest + defaults: + run: + working-directory: ci-bench-runner + steps: + - uses: actions/checkout@v4 + - name: Install stable rust toolchain + uses: dtolnay/rust-toolchain@stable + - name: Install valgrind + run: sudo apt-get install -y valgrind + - name: Build + run: cargo build --locked --verbose + - name: Run tests + run: cargo test --locked --verbose