From 0275079d429fa669bb497b78405840eb59ac910a Mon Sep 17 00:00:00 2001 From: Nikhil Benesch Date: Fri, 10 Jun 2022 14:25:51 -0400 Subject: [PATCH] ci: pull cargo test out of the build CI job Building the ci-cargo-test Docker image is one of the slowest parts of the CI build job because it requires shipping around a 1.5GiB image that's only used by one other job. Instead, use a third builder agent to run `cargo nextest` directly. This is a bit more wasteful in terms of overall CPU usage (we have to recompile the world from scratch twice), but should make CI *much* faster because the main build job will complete much faster, which lets the rest of the tests get started much sooner. Fix #13010. --- .config/nextest.toml | 12 +++ bin/lint | 1 + ci/builder/Dockerfile | 2 +- ci/test/cargo-test/image/.gitignore | 8 -- ci/test/cargo-test/image/Dockerfile | 28 ------- ci/test/cargo-test/image/mzbuild.yml | 19 ----- ci/test/cargo-test/image/run-tests | 38 --------- ci/test/cargo-test/mzcompose.py | 80 ++++++++++--------- ci/test/pipeline.template.yml | 5 +- doc/developer/mzbuild.md | 4 - misc/python/materialize/mzbuild.py | 72 ----------------- misc/python/materialize/mzcompose/services.py | 4 +- src/environmentd/tests/auth.rs | 10 +++ src/environmentd/tests/util.rs | 4 +- 14 files changed, 74 insertions(+), 213 deletions(-) create mode 100644 .config/nextest.toml delete mode 100644 ci/test/cargo-test/image/.gitignore delete mode 100644 ci/test/cargo-test/image/Dockerfile delete mode 100644 ci/test/cargo-test/image/mzbuild.yml delete mode 100755 ci/test/cargo-test/image/run-tests diff --git a/.config/nextest.toml b/.config/nextest.toml new file mode 100644 index 000000000000..beb8dd35abbf --- /dev/null +++ b/.config/nextest.toml @@ -0,0 +1,12 @@ +[profile.default] +slow-timeout = { period = "60s", terminate-after = 2 } + +[[profile.default.overrides]] +filter = "package(mz-environmentd)" +threads-required = 8 +slow-timeout = { period = "120s", terminate-after = 2 } + +[profile.ci] +junit = { path = "junit_cargo-test.xml" } +fail-fast = false +failure-output = "immediate-final" diff --git a/bin/lint b/bin/lint index a406d7043e47..f4f576c0639f 100755 --- a/bin/lint +++ b/bin/lint @@ -49,6 +49,7 @@ copyright_files=$(grep -vE \ -e '^netlify\.toml$' \ -e '^rustfmt\.toml$' \ -e '^clippy\.toml$' \ + -e '^\.config/nextest\.toml$' \ -e '(^|/)yarn\.lock$' \ -e '(^|/)requirements.*\.txt$' \ -e '\.(md|json|asc|png|jpe?g|svg|avro|avsc|pb|ico|html|so)$' \ diff --git a/ci/builder/Dockerfile b/ci/builder/Dockerfile index 93991cc66aea..7371ae41d5cc 100644 --- a/ci/builder/Dockerfile +++ b/ci/builder/Dockerfile @@ -153,7 +153,7 @@ RUN mkdir rust \ && rm -rf rust \ && cargo install --root /usr/local --version "=1.40.5" cargo-deb \ && cargo install --root /usr/local --version "=0.12.2" cargo-deny \ - && cargo install --root /usr/local --version "=0.1.12" cargo2junit \ + && cargo install --root /usr/local --version "=0.9.44" cargo-nextest \ && cargo install --root /usr/local --version "=0.1.34" cargo-udeps --features=vendored-openssl \ && cargo install --root /usr/local --version "=0.2.15" --no-default-features --features=s3,openssl/vendored sccache diff --git a/ci/test/cargo-test/image/.gitignore b/ci/test/cargo-test/image/.gitignore deleted file mode 100644 index f216f9797b5a..000000000000 --- a/ci/test/cargo-test/image/.gitignore +++ /dev/null @@ -1,8 +0,0 @@ -/computed -/storaged -/environmentd -/protobuf-install -/protoc -/shlib -/test-binaries.json -/tests diff --git a/ci/test/cargo-test/image/Dockerfile b/ci/test/cargo-test/image/Dockerfile deleted file mode 100644 index a74166bc889e..000000000000 --- a/ci/test/cargo-test/image/Dockerfile +++ /dev/null @@ -1,28 +0,0 @@ -# Copyright Materialize, Inc. and contributors. All rights reserved. -# -# Use of this software is governed by the Business Source License -# included in the LICENSE file at the root of this repository. -# -# As of the Change Date specified in that file, in accordance with -# the Business Source License, use of this software will be governed -# by the Apache License, Version 2.0. - -MZFROM ubuntu-base - -RUN apt-get update && apt-get -qy install \ - ca-certificates \ - jq \ - psmisc - -COPY tests environmentd /tests/ -COPY storaged computed / -COPY run-tests /usr/local/bin - -# Install the Protobuf compiler from protobuf-src for use by mz_protoc's tests. -COPY protobuf-install /usr/local/ -ENV PROTOC /usr/local/bin/protoc - -# Ensure any Rust binaries that crash print a backtrace. -ENV RUST_BACKTRACE=1 - -WORKDIR /workdir diff --git a/ci/test/cargo-test/image/mzbuild.yml b/ci/test/cargo-test/image/mzbuild.yml deleted file mode 100644 index 91f878c2a5b9..000000000000 --- a/ci/test/cargo-test/image/mzbuild.yml +++ /dev/null @@ -1,19 +0,0 @@ -# Copyright Materialize, Inc. and contributors. All rights reserved. -# -# Use of this software is governed by the Business Source License -# included in the LICENSE file at the root of this repository. -# -# As of the Change Date specified in that file, in accordance with -# the Business Source License, use of this software will be governed -# by the Apache License, Version 2.0. - -name: ci-cargo-test -pre-image: - - type: cargo-build - bin: testdrive - extract: - protobuf-src: - install: protobuf-install - - type: cargo-build - bin: [storaged, computed, environmentd] - - type: cargo-test diff --git a/ci/test/cargo-test/image/run-tests b/ci/test/cargo-test/image/run-tests deleted file mode 100755 index 12954e2326e4..000000000000 --- a/ci/test/cargo-test/image/run-tests +++ /dev/null @@ -1,38 +0,0 @@ -#!/usr/bin/env bash - -# Copyright Materialize, Inc. and contributors. All rights reserved. -# -# Use of this software is governed by the Business Source License -# included in the LICENSE file at the root of this repository. -# -# As of the Change Date specified in that file, in accordance with -# the Business Source License, use of this software will be governed -# by the Apache License, Version 2.0. - -set -euo pipefail - -passed=0 -total=0 - -while read -r binary package cwd; do - echo "--- $binary" - killall -9 storaged computed materialized &> /dev/null || true - # Run tests, generating a JSON event stream. The jq in the pipeline below - # prefixes each event's name field, if present, with the package name, so - # that we can distinguish tests with the same names in different packages. - if (cd "$cwd" && "/tests/$binary" -Z unstable-options --format json --report-time) \ - | jq -c --arg package "$package" 'if .name then .name = "\($package)::\(.name)" else . end' \ - | tee -a results.json - then - ((++passed)) - else - echo "^^^ +++" - fi - ((++total)) -done < /tests/manifest - -echo "+++ Status report" -echo "$passed/$total commands passed" -if ((passed != total)); then - exit 1 -fi diff --git a/ci/test/cargo-test/mzcompose.py b/ci/test/cargo-test/mzcompose.py index 203a3dd7be80..920076371ae7 100644 --- a/ci/test/cargo-test/mzcompose.py +++ b/ci/test/cargo-test/mzcompose.py @@ -7,50 +7,58 @@ # the Business Source License, use of this software will be governed # by the Apache License, Version 2.0. -from materialize import ROOT, ci_util, spawn -from materialize.mzcompose import Composition, Service +import os + +from materialize import spawn +from materialize.mzcompose import Composition, WorkflowArgumentParser from materialize.mzcompose.services import Kafka, Postgres, SchemaRegistry, Zookeeper SERVICES = [ Zookeeper(), - Kafka(), + Kafka( + # We need a stable port to advertise, so pick one that is unlikely to + # conflict with a Kafka cluster running on the local machine. + port="30123:30123", + allow_host_ports=True, + extra_environment=[ + f"KAFKA_ADVERTISED_LISTENERS=HOST://localhost:30123,PLAINTEXT://kafka:9092", + "KAFKA_LISTENER_SECURITY_PROTOCOL_MAP=HOST:PLAINTEXT,PLAINTEXT:PLAINTEXT", + ], + ), SchemaRegistry(), Postgres(image="postgres:14.2"), - Service( - name="ci-cargo-test", - config={ - "mzbuild": "ci-cargo-test", - "environment": [ - "ZOOKEEPER_ADDR=zookeeper:2181", - "KAFKA_ADDRS=kafka:9092", - "SCHEMA_REGISTRY_URL=http://schema-registry:8081", - "POSTGRES_URL=postgres://postgres:postgres@postgres", - "MZ_SOFT_ASSERTIONS=1", - "MZ_PERSIST_EXTERNAL_STORAGE_TEST_S3_BUCKET=mtlz-test-persist-1d-lifecycle-delete", - "MZ_PERSIST_EXTERNAL_STORAGE_TEST_POSTGRES_URL=postgres://postgres:postgres@postgres", - "AWS_DEFAULT_REGION", - "AWS_ACCESS_KEY_ID", - "AWS_SECRET_ACCESS_KEY", - "AWS_SESSION_TOKEN", - ], - "volumes": ["../../../:/workdir"], - "ulimits": { - "core": 0, - }, - }, - ), ] -def workflow_default(c: Composition) -> None: +def workflow_default(c: Composition, parser: WorkflowArgumentParser) -> None: + parser.add_argument("args", nargs="*") + args = parser.parse_args() c.start_and_wait_for_tcp(["zookeeper", "kafka", "schema-registry", "postgres"]) - try: - c.run("ci-cargo-test", "run-tests") - finally: - junit_report = ci_util.junit_report_filename("cargo-test") - spawn.runv( - ["cargo2junit"], - stdin=(ROOT / "results.json").open("rb"), - stdout=junit_report.open("wb"), + # Heads up: this intentionally runs on the host rather than in a Docker + # image. See #13010. + postgres_url = ( + f"postgres://postgres:postgres@localhost:{c.default_port('postgres')}" + ) + spawn.runv( + [ + "cargo", + "build", + "--bin", + "storaged", + "--bin", + "computed", + ] + ) + spawn.runv( + ["cargo", "nextest", "run", "--profile=ci", *args.args], + env=dict( + os.environ, + ZOOKEEPER_ADDR=f"localhost:{c.default_port('zookeeper')}", + KAFKA_ADDRS=f"localhost:30123", + SCHEMA_REGISTRY_URL=f"http://localhost:{c.default_port('schema-registry')}", + POSTGRES_URL=postgres_url, + MZ_SOFT_ASSERTIONS="1", + MZ_PERSIST_EXTERNAL_STORAGE_TEST_S3_BUCKET="mtlz-test-persist-1d-lifecycle-delete", + MZ_PERSIST_EXTERNAL_STORAGE_TEST_POSTGRES_URL=postgres_url, ), - ci_util.upload_junit_report("cargo-test", junit_report) + ) diff --git a/ci/test/pipeline.template.yml b/ci/test/pipeline.template.yml index 43d3ce70074d..cea57b5c4dfb 100644 --- a/ci/test/pipeline.template.yml +++ b/ci/test/pipeline.template.yml @@ -113,9 +113,8 @@ steps: - id: cargo-test label: Cargo test - depends_on: build-x86_64 timeout_in_minutes: 30 - artifact_paths: junit_cargo-test_*.xml + artifact_paths: target/nextest/ci/junit_cargo-test.xml env: AWS_DEFAULT_REGION: "us-east-2" plugins: @@ -123,7 +122,7 @@ steps: - ./ci/plugins/mzcompose: composition: cargo-test agents: - queue: linux-x86_64 + queue: builder-linux-x86_64 - id: miri-test label: Miri test diff --git a/doc/developer/mzbuild.md b/doc/developer/mzbuild.md index e84adf926302..05df39367e56 100644 --- a/doc/developer/mzbuild.md +++ b/doc/developer/mzbuild.md @@ -428,10 +428,6 @@ publish: true has a custom Cargo build script, as Rust crates without a build script do not have a build directory. - * `type: cargo-test` builds a special image that simulates `cargo test`. This - plugin is very special-cased at the moment, and unlikely to be generally - useful. - * `publish` (bool) specifies whether the image should be automatically published to Docker Hub by CI. Non-publishable images can still be *used* by users and CI, but they must always be built from source. Use sparingly. The default is diff --git a/misc/python/materialize/mzbuild.py b/misc/python/materialize/mzbuild.py index fe8a87dbaf1f..e944cee773c6 100644 --- a/misc/python/materialize/mzbuild.py +++ b/misc/python/materialize/mzbuild.py @@ -341,76 +341,6 @@ def inputs(self) -> Set[str]: return super().inputs() | set(inp for dep in deps for inp in dep.inputs()) -# TODO(benesch): make this less hardcoded and custom. -class CargoTest(CargoPreImage): - """A pre-image action that builds all test binaries in the Cargo workspace. - - .. todo:: This action does not generalize well. - Its implementation currently hardcodes many details about the - dependencies of various Rust crates. Ideally these dependencies would - instead be captured by configuration in `mzbuild.yml`. - """ - - def __init__(self, rd: RepositoryDetails, path: Path, config: Dict[str, Any]): - super().__init__(rd, path) - - def run(self) -> None: - super().run() - - # NOTE(benesch): The two invocations of `cargo test --no-run` here - # deserve some explanation. The first invocation prints error messages - # to stdout in a human readable form. If that succeeds, the second - # invocation instructs Cargo to dump the locations of the test binaries - # it built in a machine readable form. Without the first invocation, the - # error messages would also be sent to the output file in JSON, and the - # user would only see a vague "could not compile " error. - args = [*self.rd.cargo("test", rustflags=[]), "--locked", "--no-run"] - spawn.runv(args, cwd=self.rd.root) - output = spawn.capture(args + ["--message-format=json"], cwd=self.rd.root) - - tests = [] - for line in output.split("\n"): - if line.strip() == "": - continue - message = json.loads(line) - if message.get("profile", {}).get("test", False): - crate_name = message["package_id"].split()[0] - target_kind = "".join(message["target"]["kind"]) - if target_kind == "proc-macro": - continue - slug = crate_name + "." + target_kind - if target_kind != "lib": - slug += "." + message["target"]["name"] - crate_path_match = re.search( - r"\(path\+file://(.*)\)", message["package_id"] - ) - if not crate_path_match: - raise ValueError(f'invalid package_id: {message["package_id"]}') - crate_path = Path(crate_path_match.group(1)).relative_to( - self.rd.root.resolve() - ) - executable = self.rd.rewrite_builder_path_for_host( - Path(message["executable"]) - ) - tests.append((executable, slug, crate_path)) - - os.makedirs(self.path / "tests" / "examples") - with open(self.path / "tests" / "manifest", "w") as manifest: - for (executable, slug, crate_path) in tests: - shutil.copy(executable, self.path / "tests" / slug) - spawn.runv( - [*self.rd.tool("strip"), self.path / "tests" / slug], - cwd=self.rd.root, - ) - package = slug.replace(".", "::") - manifest.write(f"{slug} {package} {crate_path}\n") - shutil.copytree(self.rd.root / "misc" / "shlib", self.path / "shlib") - - def inputs(self) -> Set[str]: - crates = self.rd.cargo_workspace.crates.values() - return super().inputs() | set(inp for crate in crates for inp in crate.inputs()) - - class Image: """A Docker image whose build and dependencies are managed by mzbuild. @@ -442,8 +372,6 @@ def __init__(self, rd: RepositoryDetails, path: Path): typ = pre_image.pop("type", None) if typ == "cargo-build": self.pre_images.append(CargoBuild(self.rd, self.path, pre_image)) - elif typ == "cargo-test": - self.pre_images.append(CargoTest(self.rd, self.path, pre_image)) elif typ == "copy": self.pre_images.append(Copy(self.rd, self.path, pre_image)) else: diff --git a/misc/python/materialize/mzcompose/services.py b/misc/python/materialize/mzcompose/services.py index 839a12a48559..578661e4f533 100644 --- a/misc/python/materialize/mzcompose/services.py +++ b/misc/python/materialize/mzcompose/services.py @@ -315,7 +315,8 @@ def __init__( name: str = "kafka", image: str = "confluentinc/cp-kafka", tag: str = DEFAULT_CONFLUENT_PLATFORM_VERSION, - port: int = 9092, + port: Union[str, int] = 9092, + allow_host_ports: bool = False, auto_create_topics: bool = False, broker_id: int = 1, offsets_topic_replication_factor: int = 1, @@ -343,6 +344,7 @@ def __init__( config: ServiceConfig = { "image": f"{image}:{tag}", "ports": [port], + "allow_host_ports": allow_host_ports, "environment": [ *environment, f"KAFKA_AUTO_CREATE_TOPICS_ENABLE={auto_create_topics}", diff --git a/src/environmentd/tests/auth.rs b/src/environmentd/tests/auth.rs index ed31f17c5eb2..2db43e616d00 100644 --- a/src/environmentd/tests/auth.rs +++ b/src/environmentd/tests/auth.rs @@ -599,6 +599,8 @@ fn test_auth_expiry() { #[allow(clippy::unit_arg)] #[test] fn test_auth() { + mz_ore::test::init_logging(); + let ca = Ca::new_root("test ca").unwrap(); let (server_cert, server_key) = ca .request_cert("server", vec![IpAddr::V4(Ipv4Addr::LOCALHOST)]) @@ -795,6 +797,7 @@ fn test_auth() { }, ], ); + drop(server); // Test connecting to a server that requires TLS and uses Materialize Cloud for // authentication. @@ -1045,6 +1048,7 @@ fn test_auth() { }, ], ); + drop(server); // Test TLS modes with a server that does not support TLS. let server = util::start_server(util::Config::default()).unwrap(); @@ -1113,6 +1117,7 @@ fn test_auth() { }, ], ); + drop(server); // Test TLS modes with a server that requires TLS. let config = util::Config::default().with_tls(TlsMode::Require, &server_cert, &server_key); @@ -1199,6 +1204,7 @@ fn test_auth() { }, ], ); + drop(server); // Test connecting to a server that verifies client certificates. let config = util::Config::default().with_tls( @@ -1358,6 +1364,7 @@ fn test_auth() { }, ], ); + drop(server); // Test connecting to a server that both verifies client certificates and // verifies that the CN matches the pgwire user name. @@ -1564,6 +1571,7 @@ fn test_auth() { }, ], ); + drop(server); } #[test] @@ -1622,6 +1630,7 @@ fn test_auth_intermediate_ca() { }, ], ); + drop(server); // When the server is configured to present the entire certificate chain, // the client should be able to verify the chain even though it only knows @@ -1649,4 +1658,5 @@ fn test_auth_intermediate_ca() { }, ], ); + drop(server); } diff --git a/src/environmentd/tests/util.rs b/src/environmentd/tests/util.rs index 7ab4cf28807a..a64bb791f1f8 100644 --- a/src/environmentd/tests/util.rs +++ b/src/environmentd/tests/util.rs @@ -182,9 +182,7 @@ pub fn start_server(config: Config) -> Result { .parent() .unwrap() .to_path_buf(), - // NOTE(benesch): would be nice to not have to do this, but - // the subprocess output wreaks havoc on cargo2junit. - suppress_output: true, + suppress_output: false, environment_id: environment_id.clone(), secrets_dir: data_directory.join("secrets"), command_wrapper: vec![],