From d36db7a0824fdde7ba8d0cd3e3574cd50f6e2b35 Mon Sep 17 00:00:00 2001 From: Xuanwo Date: Mon, 16 Jan 2023 16:27:37 +0800 Subject: [PATCH] fix: Sccache dist tests broken after bump to tokio 1.21 and later Signed-off-by: Xuanwo --- .github/workflows/ci.yml | 108 +++++++++++++------------- Cargo.lock | 23 ++---- Cargo.toml | 1 - src/dist/http.rs | 23 ++++-- src/server.rs | 54 ++++++++----- src/test/mock_storage.rs | 2 +- tests/harness/Dockerfile.sccache-dist | 15 +--- tests/harness/mod.rs | 53 ++++--------- 8 files changed, 129 insertions(+), 150 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 7ef3c1f308..5af42c5b1f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -44,7 +44,7 @@ jobs: include: - os: ubuntu-20.04 rustc: 1.60.0 # Oldest supported version, keep in sync with README.md - - os: ubuntu-18.04 + - os: ubuntu-22.04 rustc: 1.60.0 extra_desc: dist-server extra_args: --no-default-features --features=dist-tests test_dist_ -- --test-threads 1 @@ -182,10 +182,10 @@ jobs: extra_args: --features=unstable - os: macOS-11 rustc: nightly -# Disable on Windows for now as it fails with: -# found invalid metadata files for crate `vte_generate_state_changes` -# - os: windows-2019 -# rustc: nightly + # Disable on Windows for now as it fails with: + # found invalid metadata files for crate `vte_generate_state_changes` + # - os: windows-2019 + # rustc: nightly env: RUST_BACKTRACE: 1 steps: @@ -207,9 +207,9 @@ jobs: - name: Execute tests run: cargo test --no-fail-fast --locked --all-targets --verbose ${{ matrix.extra_args }} env: - CARGO_INCREMENTAL: '0' - RUSTC_WRAPPER: '' - RUSTFLAGS: '-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off' + CARGO_INCREMENTAL: "0" + RUSTC_WRAPPER: "" + RUSTFLAGS: "-Zprofile -Ccodegen-units=1 -Copt-level=0 -Clink-dead-code -Coverflow-checks=off" - name: Generate coverage data (via `grcov`) id: coverage @@ -245,53 +245,53 @@ jobs: matrix: job: - { os: macos-12 } - release: [ "13.1" ] + release: ["13.1"] steps: - - uses: actions/checkout@v3 - - name: Prepare, build and test - uses: vmactions/freebsd-vm@v0 - with: - mem: 8192 - usesh: true - copyback: false - prepare: pkg install -y ca_root_nss curl gmake gtar pot sudo - run: | - ##################################################################################### - ### Prepare, build, and test - ##################################################################################### - ### based on ref: - ### and on ref: - ### * NOTE: All steps need to be run in this block, otherwise, we are operating back - ### on the mac host. - set -exo pipefail - # - ### Basic user setup ################################################################ - TEST_USER=tester - TEST_USER_HOME="/opt/$TEST_USER" - REPO_NAME=${GITHUB_WORKSPACE##*/} - WORKSPACE_PARENT="/Users/runner/work/${REPO_NAME}" - WORKSPACE="${WORKSPACE_PARENT}/${REPO_NAME}" - export WORKSPACE - # - mkdir -p "$TEST_USER_HOME" - pw adduser -n "$TEST_USER" -d "$TEST_USER_HOME" -c "Tester" -h - - chown -R "$TEST_USER":"$TEST_USER" "$TEST_USER_HOME" - chown -R "$TEST_USER":"$TEST_USER" "/$WORKSPACE_PARENT"/ - cat > /usr/local/etc/sudoers.d/wheel< + ### and on ref: + ### * NOTE: All steps need to be run in this block, otherwise, we are operating back + ### on the mac host. + set -exo pipefail + # + ### Basic user setup ################################################################ + TEST_USER=tester + TEST_USER_HOME="/opt/$TEST_USER" + REPO_NAME=${GITHUB_WORKSPACE##*/} + WORKSPACE_PARENT="/Users/runner/work/${REPO_NAME}" + WORKSPACE="${WORKSPACE_PARENT}/${REPO_NAME}" + export WORKSPACE + # + mkdir -p "$TEST_USER_HOME" + pw adduser -n "$TEST_USER" -d "$TEST_USER_HOME" -c "Tester" -h - + chown -R "$TEST_USER":"$TEST_USER" "$TEST_USER_HOME" + chown -R "$TEST_USER":"$TEST_USER" "/$WORKSPACE_PARENT"/ + cat > /usr/local/etc/sudoers.d/wheel< { - let certs = server_certificates.lock().unwrap(); - let (cert_digest, cert_pem) = try_or_500_log!(req_id, certs.get(&server_id) + let certs = { + let guard = server_certificates.lock().unwrap(); + guard.get(&server_id).map(|v|v.to_owned()) + }; + + let (cert_digest, cert_pem) = try_or_500_log!(req_id, certs .context("server cert not available")); let res = ServerCertificateHttpResponse { - cert_digest: cert_digest.clone(), - cert_pem: cert_pem.clone(), + cert_digest, + cert_pem, }; prepare_response(request, &res) }, @@ -1100,6 +1110,9 @@ mod client { let client = reqwest::ClientBuilder::new() .timeout(timeout) .connect_timeout(connect_timeout) + // Disable connection pool to avoid broken connection + // between runtime + .pool_max_idle_per_host(0) .build() .context("failed to create an async HTTP client")?; let client_toolchains = diff --git a/src/server.rs b/src/server.rs index edb7a84d6d..cdb4a34b78 100644 --- a/src/server.rs +++ b/src/server.rs @@ -32,7 +32,6 @@ use filetime::FileTime; use futures::channel::mpsc; use futures::future::FutureExt; use futures::{future, stream, Sink, SinkExt, Stream, StreamExt, TryFutureExt}; -use futures_locks::RwLock; use number_prefix::NumberPrefix; use std::collections::HashMap; use std::env; @@ -48,12 +47,13 @@ use std::path::PathBuf; use std::pin::Pin; use std::process::{ExitStatus, Output}; use std::sync::Arc; -use std::sync::Mutex; use std::task::{Context, Poll, Waker}; use std::time::Duration; #[cfg(feature = "dist-client")] use std::time::Instant; use std::u64; +use tokio::sync::Mutex; +use tokio::sync::RwLock; use tokio::{ io::{AsyncRead, AsyncWrite}, net::TcpListener, @@ -683,7 +683,7 @@ where C: Send, { /// Server statistics. - stats: Arc>, + stats: Arc>, /// Distributed sccache client dist_client: Arc, @@ -760,7 +760,7 @@ where match req.into_inner() { Request::Compile(compile) => { debug!("handle_client: compile"); - me.stats.write().await.compile_requests += 1; + me.stats.lock().await.compile_requests += 1; me.handle_compile(compile).await } Request::GetStats => { @@ -825,11 +825,11 @@ where info: ActiveInfo, ) -> SccacheService { SccacheService { - stats: Arc::new(RwLock::new(ServerStats::default())), + stats: Arc::default(), dist_client: Arc::new(dist_client), storage, - compilers: Arc::new(RwLock::new(HashMap::new())), - compiler_proxies: Arc::new(RwLock::new(HashMap::new())), + compilers: Arc::default(), + compiler_proxies: Arc::default(), rt, creator: C::new(client), tx, @@ -887,7 +887,7 @@ where /// Get info and stats about the cache. async fn get_info(&self) -> Result { - let stats = self.stats.read().await.clone(); + let stats = self.stats.lock().await.clone(); let cache_location = self.storage.location(); futures::try_join!(self.storage.current_size(), self.storage.max_size()).map( move |(cache_size, max_cache_size)| ServerInfo { @@ -901,7 +901,7 @@ where /// Zero stats about the cache. async fn zero_stats(&self) { - *self.stats.write().await = ServerStats::default(); + *self.stats.lock().await = ServerStats::default(); } /// Handle a compile request from a client. @@ -1090,11 +1090,10 @@ where cwd: PathBuf, env_vars: Vec<(OsString, OsString)>, ) -> SccacheResponse { - let mut stats = self.stats.write().await; match compiler { Err(e) => { debug!("check_compiler: Unsupported compiler: {}", e.to_string()); - stats.requests_unsupported_compiler += 1; + self.stats.lock().await.requests_unsupported_compiler += 1; return Message::WithoutBody(Response::Compile( CompileResponse::UnsupportedCompiler(OsString::from(e.to_string())), )); @@ -1106,7 +1105,7 @@ where match c.parse_arguments(&cmd, &cwd, &env_vars) { CompilerArguments::Ok(hasher) => { debug!("parse_arguments: Ok: {:?}", cmd); - stats.requests_executed += 1; + self.stats.lock().await.requests_executed += 1; let (tx, rx) = Body::pair(); self.start_compile_task(c, hasher, cmd, cwd, env_vars, tx); let res = CompileResponse::CompileStarted; @@ -1121,12 +1120,13 @@ where } else { debug!("parse_arguments: CannotCache({}): {:?}", why, cmd) } + let mut stats = self.stats.lock().await; stats.requests_not_cacheable += 1; *stats.not_cached.entry(why.to_string()).or_insert(0) += 1; } CompilerArguments::NotCompilation => { debug!("parse_arguments: NotCompilation: {:?}", cmd); - stats.requests_not_compile += 1; + self.stats.lock().await.requests_not_compile += 1; } } } @@ -1190,16 +1190,22 @@ where }; match result { Ok((compiled, out)) => { - let mut stats = me.stats.write().await; + let mut stats = me.stats.lock().await; match compiled { CompileResult::Error => { + debug!("compile result: cache error"); + stats.cache_errors.increment(&kind); } CompileResult::CacheHit(duration) => { + debug!("compile result: cache hit"); + stats.cache_hits.increment(&kind); stats.cache_read_hit_duration += duration; } CompileResult::CacheMiss(miss_type, dist_type, duration, future) => { + debug!("compile result: cache miss"); + match dist_type { DistType::NoDist => {} DistType::Ok(id) => { @@ -1224,16 +1230,24 @@ where } stats.cache_misses.increment(&kind); stats.compiler_write_duration += duration; + debug!("stats after compile result: {stats:?}"); cache_write = Some(future); } CompileResult::NotCacheable => { + debug!("compile result: not cacheable"); + stats.cache_misses.increment(&kind); stats.non_cacheable_compilations += 1; } CompileResult::CompileFailed => { + debug!("compile result: compile failed"); + stats.compile_fails += 1; } }; + // Make sure the write guard has been dropped ASAP. + drop(stats); + let Output { status, stdout, @@ -1248,7 +1262,7 @@ where res.stderr = stderr; } Err(err) => { - let mut stats = me.stats.write().await; + let mut stats = me.stats.lock().await; match err.downcast::() { Ok(ProcessError(output)) => { debug!("Compilation failed: {:?}", output); @@ -1299,7 +1313,7 @@ where match cache_write.await { Err(e) => { debug!("Error executing cache write: {}", e); - me.stats.write().await.cache_write_errors += 1; + me.stats.lock().await.cache_write_errors += 1; } //TODO: save cache stats! Ok(info) => { @@ -1308,7 +1322,7 @@ where info.object_file_pretty, util::fmt_duration_as_secs(&info.duration) ); - let mut stats = me.stats.write().await; + let mut stats = me.stats.lock().await; stats.cache_writes += 1; stats.cache_write_duration += info.duration; } @@ -1794,13 +1808,13 @@ impl Future for ShutdownOrInactive { /// Helper future which tracks the `ActiveInfo` below. This future will resolve /// once all instances of `ActiveInfo` have been dropped. struct WaitUntilZero { - info: std::sync::Weak>, + info: std::sync::Weak>, } #[derive(Clone)] #[allow(dead_code)] struct ActiveInfo { - info: Arc>, + info: Arc>, } struct Info { @@ -1818,7 +1832,7 @@ impl Drop for Info { impl WaitUntilZero { #[rustfmt::skip] fn new() -> (WaitUntilZero, ActiveInfo) { - let info = Arc::new(Mutex::new(Info { waker: None })); + let info = Arc::new(std::sync::Mutex::new(Info { waker: None })); (WaitUntilZero { info: Arc::downgrade(&info) }, ActiveInfo { info }) } diff --git a/src/test/mock_storage.rs b/src/test/mock_storage.rs index 2c22f9b93d..58c2be5320 100644 --- a/src/test/mock_storage.rs +++ b/src/test/mock_storage.rs @@ -15,9 +15,9 @@ use crate::cache::{Cache, CacheWrite, Storage}; use crate::errors::*; use futures::channel::mpsc; -use futures_locks::Mutex; use std::sync::Arc; use std::time::Duration; +use tokio::sync::Mutex; /// A mock `Storage` implementation. pub struct MockStorage { diff --git a/tests/harness/Dockerfile.sccache-dist b/tests/harness/Dockerfile.sccache-dist index 64be2f7a35..8eb2c11cc0 100644 --- a/tests/harness/Dockerfile.sccache-dist +++ b/tests/harness/Dockerfile.sccache-dist @@ -1,15 +1,4 @@ -FROM ubuntu:18.04 as bwrap-build +FROM ubuntu:22.04 RUN apt-get update && \ - apt-get install -y wget xz-utils gcc libcap-dev make && \ + apt-get install -y libcap2 bubblewrap && \ apt-get clean -RUN wget -q -O - https://github.com/projectatomic/bubblewrap/releases/download/v0.3.1/bubblewrap-0.3.1.tar.xz | \ - tar -xJ -RUN cd /bubblewrap-0.3.1 && \ - ./configure --disable-man && \ - make - -FROM aidanhs/ubuntu-docker:18.04-17.03.2-ce -RUN apt-get update && \ - apt-get install libcap2 libssl1.1 && \ - apt-get clean -COPY --from=bwrap-build /bubblewrap-0.3.1/bwrap /bwrap diff --git a/tests/harness/mod.rs b/tests/harness/mod.rs index d8b5e11c07..e5da248f35 100644 --- a/tests/harness/mod.rs +++ b/tests/harness/mod.rs @@ -8,7 +8,7 @@ use std::io::Write; use std::net::{self, IpAddr, SocketAddr}; use std::path::{Path, PathBuf}; use std::process::{Command, Output, Stdio}; -use std::str; +use std::str::{self, FromStr}; use std::thread; use std::time::{Duration, Instant}; @@ -38,7 +38,7 @@ macro_rules! matches { const CONTAINER_NAME_PREFIX: &str = "sccache_dist_test"; const DIST_IMAGE: &str = "sccache_dist_test_image"; const DIST_DOCKERFILE: &str = include_str!("Dockerfile.sccache-dist"); -const DIST_IMAGE_BWRAP_PATH: &str = "/bwrap"; +const DIST_IMAGE_BWRAP_PATH: &str = "/usr/bin/bwrap"; const MAX_STARTUP_WAIT: Duration = Duration::from_secs(5); const DIST_SERVER_TOKEN: &str = "THIS IS THE TEST TOKEN"; @@ -55,6 +55,9 @@ pub fn start_local_daemon(cfg_path: &Path, cached_cfg_path: &Path) { // will hang because the internal server process is not detached. let _ = sccache_command() .arg("--start-server") + // Uncomment following lines to debug locally. + // .env("SCCACHE_LOG", "debug") + // .env("SCCACHE_ERROR_LOG", "/tmp/sccache_log.txt") .env("SCCACHE_CONF", cfg_path) .env("SCCACHE_CACHED_CONF", cached_cfg_path) .status() @@ -79,7 +82,9 @@ pub fn get_stats(f: F) { .success() .stdout(predicate::function(move |output: &[u8]| { let s = str::from_utf8(output).expect("Output not UTF-8"); - f(serde_json::from_str(s).expect("Failed to parse JSON stats")); + let stats = serde_json::from_str(s).expect("Failed to parse JSON stats"); + eprintln!("get server stats: {stats:?}"); + f(stats); true })); } @@ -271,6 +276,8 @@ impl DistSystem { "SCCACHE_LOG=sccache=trace", "-e", "RUST_BACKTRACE=1", + "--network", + "host", "-v", &format!("{}:/sccache-dist", self.sccache_dist.to_str().unwrap()), "-v", @@ -337,6 +344,8 @@ impl DistSystem { "SCCACHE_LOG=sccache=trace", "-e", "RUST_BACKTRACE=1", + "--network", + "host", "-v", &format!("{}:/sccache-dist", self.sccache_dist.to_str().unwrap()), "-v", @@ -364,7 +373,7 @@ impl DistSystem { check_output(&output); - let server_ip = self.container_ip(&server_name); + let server_ip = IpAddr::from_str("127.0.0.1").unwrap(); let server_cfg = sccache_server_cfg(&self.tmpdir, self.scheduler_url(), server_ip); fs::File::create(&server_cfg_path) .unwrap() @@ -388,7 +397,7 @@ impl DistSystem { handler: S, ) -> ServerHandle { let server_addr = { - let ip = self.host_interface_ip(); + let ip = IpAddr::from_str("127.0.0.1").unwrap(); let listener = net::TcpListener::bind(SocketAddr::from((ip, 0))).unwrap(); listener.local_addr().unwrap() }; @@ -461,8 +470,7 @@ impl DistSystem { } pub fn scheduler_url(&self) -> HTTPUrl { - let ip = self.container_ip(self.scheduler_name.as_ref().unwrap()); - let url = format!("http://{}:{}", ip, SCHEDULER_PORT); + let url = format!("http://127.0.0.1:{}", SCHEDULER_PORT); HTTPUrl::from_url(reqwest::Url::parse(&url).unwrap()) } @@ -474,37 +482,6 @@ impl DistSystem { assert!(res.status().is_success()); bincode::deserialize_from(res).unwrap() } - - fn container_ip(&self, name: &str) -> IpAddr { - let output = Command::new("docker") - .args(&[ - "inspect", - "--format", - "{{ .NetworkSettings.IPAddress }}", - name, - ]) - .output() - .unwrap(); - check_output(&output); - let stdout = String::from_utf8(output.stdout).unwrap(); - stdout.trim().to_owned().parse().unwrap() - } - - // The interface that the host sees on the docker network (typically 'docker0') - fn host_interface_ip(&self) -> IpAddr { - let output = Command::new("docker") - .args(&[ - "inspect", - "--format", - "{{ .NetworkSettings.Gateway }}", - self.scheduler_name.as_ref().unwrap(), - ]) - .output() - .unwrap(); - check_output(&output); - let stdout = String::from_utf8(output.stdout).unwrap(); - stdout.trim().to_owned().parse().unwrap() - } } // If you want containers to hang around (e.g. for debugging), comment out the "rm -f" lines