hubcio commented on code in PR #3749:
URL: https://github.com/apache/iggy/pull/3749#discussion_r3672608516
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
Review Comment:
the artifact name embeds `github.run_attempt`, and the download at line 333
uses the same expression. "re-run failed jobs" bumps run_attempt but does not
re-execute an already-successful `needs` job, so `build-rust-tests` never
uploads under the new attempt number and the partition goes looking for
`rust-nextest-archive-<run_id>-2`, which never existed. it stays broken for
attempts 3, 4 and so on - only "re-run all jobs" recovers, at the cost of a
full rebuild. with `retries = 3` in `.config/nextest.toml` a flaky partition is
routine, and re-running just that partition worked before this PR.
the fix needs three parts, not two: drop `-${{ github.run_attempt }}` from
both names, add `overwrite: true` to the upload, and raise `retention-days`
from 1 at line 327. the `overwrite` part matters - artifacts are run-scoped and
survive across attempts, so without it "re-run all jobs" hits a duplicate-name
rejection on its upload. and github allows re-runs for 30 days, so a 1-day
retention reopens the same hole from the other side.
##########
.github/workflows/_test.yml:
##########
@@ -262,3 +251,58 @@ jobs:
codecov.json
if-no-files-found: ignore
retention-days: 7
+
+ build-rust-tests:
+ if: ${{ startsWith(inputs.component, 'rust') && inputs.task == 'test' }}
+ runs-on: ubuntu-latest
+ timeout-minutes: 60
+ steps:
+ - name: Checkout code
+ uses: actions/[email protected]
+
+ - name: Build and archive Rust tests
+ uses: ./.github/actions/rust/pre-merge
+ with:
+ task: test-build
+ component: ${{ inputs.component }}
+
+ run-rust-tests:
+ if: ${{ startsWith(inputs.component, 'rust') && inputs.task == 'test' }}
+ needs: build-rust-tests
Review Comment:
worth stating this tradeoff in the PR description - the results tables only
cover the first commit, and the archive split itself is unmeasured.
`needs:` makes the gap between the build finishing and the partitions
starting structural: 82s and 113s on this PR's run. on top of that the split
adds roughly half a minute of tar and upload after the archive completes, plus
a whole extra job setup - the disk reclaim alone was 49s in the build job and
93s and 53s in the two run jobs. and it removes nothing from the critical path,
because the old two jobs already built concurrently. so wall clock can only get
worse. the job-minute saving is the real win and it is worth having.
at 2 partitions that fixed overhead is paid to dedupe a single build. at 4
it dedupes three for roughly the same cost and wins on both axes - which is the
argument for unblocking the partition count, see the note on line 388 of the
composite action.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+ compression-level: 0
+ if-no-files-found: error
+ retention-days: 1
+
+ - name: Download archived Rust tests
+ if: startsWith(inputs.task, 'test-run-')
+ uses: actions/download-artifact@v8
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+
+ - name: Run archived tests with coverage
+ if: startsWith(inputs.task, 'test-run-')
+ run: |
+ TASK="${{ inputs.task }}"
+ if [[ ! "$TASK" =~ ^test-run-([0-9]+)$ ]]; then
+ echo "::error::Invalid archived test task: $TASK"
+ exit 1
+ fi
+ PARTITION_INDEX="${BASH_REMATCH[1]}"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ NEXTEST_FILTER=$(cat "$ARTIFACT_DIR/nextest-filter.txt")
+
+ mkdir -p target/debug
+ tar -C target/debug -xf "$ARTIFACT_DIR/connector-plugins.tar"
+ tar -C target/debug -xzf "$ARTIFACT_DIR/runtime-binaries.tar.gz"
+
+ RUNTIME_BINARIES=(
+ iggy
+ iggy-server
+ iggy-server-ng
+ iggy-connectors
+ iggy-mcp
+ iggy-bench
+ )
+ CARGO_BIN_ENV=()
+ for binary in "${RUNTIME_BINARIES[@]}"; do
+ binary_path="${GITHUB_WORKSPACE}/target/debug/${binary}"
+ if [[ ! -x "$binary_path" ]]; then
+ echo "::error::Required test runtime binary not found after
extraction: ${binary_path}"
+ exit 1
+ fi
+ CARGO_BIN_ENV+=("CARGO_BIN_EXE_${binary}=${binary_path}")
+ done
+
+ source <(cargo llvm-cov show-env --sh)
+
+ if [[ "$RUNNER_OS" == "Linux" ]]; then
+ sudo sysctl -w vm.max_map_count=2000000 || true
+ eval "$(dbus-launch --sh-syntax)"
+ export DBUS_SESSION_BUS_ADDRESS
+ eval "$(echo -n "test" | gnome-keyring-daemon --unlock
--components=secrets)"
+ echo -n "warmup" | secret-tool store --label="ci-warmup" ci-test
warmup
+ fi
+
+ test_start=$(date +%s)
+ env "${CARGO_BIN_ENV[@]}" cargo nextest run \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst" \
+ --extract-to "$GITHUB_WORKSPACE" \
+ --extract-overwrite \
+ --workspace-remap "$GITHUB_WORKSPACE" \
+ --no-fail-fast \
+ --profile ci \
+ --partition "hash:${PARTITION_INDEX}/2" \
+ -E "$NEXTEST_FILTER"
+ test_end=$(date +%s)
+ test_duration=$((test_end - test_start))
+ echo "::notice::Partition ${PARTITION_INDEX}/2 executed in
${test_duration}s ($(date -ud @${test_duration} +'%M:%S'))"
cargo llvm-cov report --codecov --output-path codecov.json
Review Comment:
`cargo llvm-cov report` only ever runs here, in `test-run-*`, and the build
job has no reports-upload step - so the profraw written during `cargo build`
and `cargo nextest archive` dies with that runner. that silently drops coverage
for code which only executes at build time, i.e. proc-macro expansion inside
rustc.
measured on this PR's run against the previous flow, union of both
partitions: `core/configs_derive/src/config_env.rs` goes from 222/237 regions
hit to 0/237, and `core/configs_derive/src/lib.rs` from 4/4 to 0/4. region
totals are identical across the two runs, so it is the same code with the
counters zeroed. the control is inside the same crate family - files with real
`#[test]`s keep their coverage, only the `#[proc_macro_*]` entry points go to
zero. the new run also had a strictly larger scope, so it is not a DAG-filter
artifact.
nothing goes red, because codecov statuses here are `informational` - which
is what makes it worth fixing. also emit `cargo llvm-cov report --codecov` in
`test-build` and upload from there. don't ship the build-phase profraw inside
the artifact instead, since both partitions would merge it and double-count.
##########
.github/workflows/_test.yml:
##########
@@ -38,6 +38,7 @@ permissions:
jobs:
run:
+ if: ${{ !(startsWith(inputs.component, 'rust') && inputs.task == 'test') }}
Review Comment:
nothing fails closed on an unknown task name. every step in the composite
action is an `if:` on a string literal, and this PR takes the number of
literals that have to agree from one (`test-`) to several: `test` here and at
lines 256 and 270, `test` in `components.yml`, plus `test-build` and
`test-run-` inside the action.
rename the task in `components.yml` and this negation flips true, the
generic `run` job invokes the composite with a task matching zero steps, and
the job passes green having done nothing. the one fail-closed check the PR does
add - `::error::Invalid archived test task` - can never fire, since its only
caller is `test-run-${{ matrix.partition }}`.
a terminal step in the composite asserting `inputs.task` is in a known set
would close it.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+ compression-level: 0
+ if-no-files-found: error
+ retention-days: 1
+
+ - name: Download archived Rust tests
+ if: startsWith(inputs.task, 'test-run-')
+ uses: actions/download-artifact@v8
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+
+ - name: Run archived tests with coverage
+ if: startsWith(inputs.task, 'test-run-')
+ run: |
+ TASK="${{ inputs.task }}"
+ if [[ ! "$TASK" =~ ^test-run-([0-9]+)$ ]]; then
+ echo "::error::Invalid archived test task: $TASK"
+ exit 1
+ fi
+ PARTITION_INDEX="${BASH_REMATCH[1]}"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ NEXTEST_FILTER=$(cat "$ARTIFACT_DIR/nextest-filter.txt")
+
+ mkdir -p target/debug
+ tar -C target/debug -xf "$ARTIFACT_DIR/connector-plugins.tar"
+ tar -C target/debug -xzf "$ARTIFACT_DIR/runtime-binaries.tar.gz"
+
+ RUNTIME_BINARIES=(
+ iggy
+ iggy-server
+ iggy-server-ng
+ iggy-connectors
+ iggy-mcp
+ iggy-bench
+ )
+ CARGO_BIN_ENV=()
+ for binary in "${RUNTIME_BINARIES[@]}"; do
+ binary_path="${GITHUB_WORKSPACE}/target/debug/${binary}"
+ if [[ ! -x "$binary_path" ]]; then
+ echo "::error::Required test runtime binary not found after
extraction: ${binary_path}"
+ exit 1
+ fi
+ CARGO_BIN_ENV+=("CARGO_BIN_EXE_${binary}=${binary_path}")
+ done
+
+ source <(cargo llvm-cov show-env --sh)
+
+ if [[ "$RUNNER_OS" == "Linux" ]]; then
+ sudo sysctl -w vm.max_map_count=2000000 || true
+ eval "$(dbus-launch --sh-syntax)"
+ export DBUS_SESSION_BUS_ADDRESS
+ eval "$(echo -n "test" | gnome-keyring-daemon --unlock
--components=secrets)"
+ echo -n "warmup" | secret-tool store --label="ci-warmup" ci-test
warmup
+ fi
+
+ test_start=$(date +%s)
+ env "${CARGO_BIN_ENV[@]}" cargo nextest run \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst" \
+ --extract-to "$GITHUB_WORKSPACE" \
+ --extract-overwrite \
+ --workspace-remap "$GITHUB_WORKSPACE" \
Review Comment:
`--workspace-remap` covers less than the name suggests. compile-time
`env!("CARGO_MANIFEST_DIR")` is baked into the archived test binaries and
cannot be remapped - the flag only affects nextest's runtime cwd and the env
var, so the runtime `std::env::var` form is fine but the macro form is not.
two live users: `core/integration/src/harness/context.rs:54` and
`core/integration/tests/server/http_tls.rs:46`. this works today only because
both jobs are `ubuntu-latest` and land on the same
`/home/runner/work/iggy/iggy` path. move either to a container, a different
runner label or self-hosted and those tests break in a way the flag cannot fix.
worth a comment on both `runs-on:` lines noting the coupling.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -259,52 +239,65 @@ runs:
bins_duration=$((bins_end - bins_start))
echo "::notice::Binaries and libraries built in ${bins_duration}s
($(date -ud @${bins_duration} +'%M:%S'))"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ mkdir -p "$ARTIFACT_DIR"
+
compile_start=$(date +%s)
if [[ -n "$PACKAGE_FLAGS" ]]; then
- cargo test --locked --no-run $PACKAGE_FLAGS
+ cargo nextest archive --locked $PACKAGE_FLAGS \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst"
else
- cargo test --locked --no-run
+ cargo nextest archive --locked \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst"
fi
compile_end=$(date +%s)
compile_duration=$((compile_end - compile_start))
- echo "::notice::Tests compiled in ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
+ echo "::notice::Tests compiled and archived in ${compile_duration}s
($(date -ud @${compile_duration} +'%M:%S'))"
- # Start D-Bus and unlock keyring right before test execution to avoid
- # gnome-keyring auto-locking the collection during the build phase.
- # Previously this ran before `cargo build`, leaving a 7+ minute idle
- # window that triggered org.freedesktop.Secret.Error.IsLocked ~10% of
runs.
- if [[ "$RUNNER_OS" == "Linux" ]]; then
- eval $(dbus-launch --sh-syntax)
- export DBUS_SESSION_BUS_ADDRESS
- eval $(echo -n "test" | gnome-keyring-daemon --unlock
--components=secrets)
- echo -n "warmup" | secret-tool store --label="ci-warmup" ci-test
warmup
+ if [[ -n "$NEXTEST_FILTER" ]]; then
+ printf '%s\n' "$NEXTEST_FILTER" > "$ARTIFACT_DIR/nextest-filter.txt"
+ else
+ printf '%s\n' 'all()' > "$ARTIFACT_DIR/nextest-filter.txt"
fi
- test_start=$(date +%s)
- if command -v cargo-nextest &> /dev/null; then
- if [[ -n "$NEXTEST_FILTER" ]]; then
- cargo nextest run --locked --no-fail-fast --profile ci
$PARTITION_FLAG $PACKAGE_FLAGS -E "$NEXTEST_FILTER"
- else
- cargo nextest run --locked --no-fail-fast --profile ci
$PARTITION_FLAG
- fi
+ PLUGIN_FILES=()
+ while IFS= read -r -d '' plugin_file; do
+ PLUGIN_FILES+=("${plugin_file##*/}")
+ done < <(
+ find target/debug -maxdepth 1 -type f \
+ -name 'libiggy_connector_*.so' -print0
+ )
+ if (( ${#PLUGIN_FILES[@]} > 0 )); then
Review Comment:
this guard only catches zero plugins. 17 crates declare a cdylib, so a build
producing 3 ships a partial 601 MB tarball and the failure surfaces two jobs
later as a dlopen error inside the connector tests instead of here. the runtime
binaries just below get checked name by name; the plugins don't. comparing
against the cdylib count would make it symmetric.
worth flagging that this guard is load-bearing beyond the obvious case:
`cargo metadata ... || echo "{}"` and the `jq ... || true` above can silently
leave the package list empty, and this is one of only two places that would
notice. so don't let it get simplified away.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -259,52 +239,65 @@ runs:
bins_duration=$((bins_end - bins_start))
echo "::notice::Binaries and libraries built in ${bins_duration}s
($(date -ud @${bins_duration} +'%M:%S'))"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ mkdir -p "$ARTIFACT_DIR"
+
compile_start=$(date +%s)
if [[ -n "$PACKAGE_FLAGS" ]]; then
- cargo test --locked --no-run $PACKAGE_FLAGS
+ cargo nextest archive --locked $PACKAGE_FLAGS \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst"
else
- cargo test --locked --no-run
+ cargo nextest archive --locked \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst"
fi
compile_end=$(date +%s)
compile_duration=$((compile_end - compile_start))
- echo "::notice::Tests compiled in ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
+ echo "::notice::Tests compiled and archived in ${compile_duration}s
($(date -ud @${compile_duration} +'%M:%S'))"
- # Start D-Bus and unlock keyring right before test execution to avoid
- # gnome-keyring auto-locking the collection during the build phase.
- # Previously this ran before `cargo build`, leaving a 7+ minute idle
- # window that triggered org.freedesktop.Secret.Error.IsLocked ~10% of
runs.
- if [[ "$RUNNER_OS" == "Linux" ]]; then
- eval $(dbus-launch --sh-syntax)
- export DBUS_SESSION_BUS_ADDRESS
- eval $(echo -n "test" | gnome-keyring-daemon --unlock
--components=secrets)
- echo -n "warmup" | secret-tool store --label="ci-warmup" ci-test
warmup
+ if [[ -n "$NEXTEST_FILTER" ]]; then
+ printf '%s\n' "$NEXTEST_FILTER" > "$ARTIFACT_DIR/nextest-filter.txt"
+ else
+ printf '%s\n' 'all()' > "$ARTIFACT_DIR/nextest-filter.txt"
fi
- test_start=$(date +%s)
- if command -v cargo-nextest &> /dev/null; then
- if [[ -n "$NEXTEST_FILTER" ]]; then
- cargo nextest run --locked --no-fail-fast --profile ci
$PARTITION_FLAG $PACKAGE_FLAGS -E "$NEXTEST_FILTER"
- else
- cargo nextest run --locked --no-fail-fast --profile ci
$PARTITION_FLAG
- fi
+ PLUGIN_FILES=()
+ while IFS= read -r -d '' plugin_file; do
+ PLUGIN_FILES+=("${plugin_file##*/}")
+ done < <(
+ find target/debug -maxdepth 1 -type f \
+ -name 'libiggy_connector_*.so' -print0
+ )
+ if (( ${#PLUGIN_FILES[@]} > 0 )); then
+ tar -C target/debug -cf "$ARTIFACT_DIR/connector-plugins.tar" \
Review Comment:
this tar is uncompressed (`-cf`) while the runtime binaries below use
`-czf`, and the upload sets `compression-level: 0` - so 601 MB, roughly half
the 1.3 GB artifact, crosses the wire raw once up and twice down.
small in the scheme of things but nearly free. if you do it, use `-I 'zstd
-T0'` rather than `-z`: the build job is on the critical path, so
single-threaded gzip over 601 MB costs more there than the parallel downloads
save. note `tar --zstd` on its own is also single-threaded, and `-caf` picks
compression from the file extension, so the filename needs to change too.
##########
.github/actions/utils/setup-rust-with-cache/action.yml:
##########
@@ -19,6 +19,14 @@ name: setup-rust-with-cache
description: Setup Rust toolchain with Swatinem/rust-cache
inputs:
+ install-nextest:
+ description: "Whether to install cargo-nextest"
+ required: false
+ default: "false"
+ install-system-dependencies:
+ description: "Whether to install system packages required by Rust builds"
Review Comment:
"required by Rust builds" undersells this - it is load-bearing at run time
too. `hwlocality` links `libhwloc.so.15` dynamically (the vendored fallback is
gated on `target_env = "musl"`), and the runner image doesn't ship it: the apt
log on this PR's run shows `libhwloc15` being newly installed. so this step is
the sole provider on the `test-run-*` legs, which compile nothing.
as written the description invites the next person to switch it off for
those legs as an obvious optimisation, and break CI at process load. worth
rewording now while the input is new.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+ compression-level: 0
+ if-no-files-found: error
+ retention-days: 1
+
+ - name: Download archived Rust tests
+ if: startsWith(inputs.task, 'test-run-')
+ uses: actions/download-artifact@v8
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+
+ - name: Run archived tests with coverage
+ if: startsWith(inputs.task, 'test-run-')
+ run: |
+ TASK="${{ inputs.task }}"
+ if [[ ! "$TASK" =~ ^test-run-([0-9]+)$ ]]; then
+ echo "::error::Invalid archived test task: $TASK"
+ exit 1
+ fi
+ PARTITION_INDEX="${BASH_REMATCH[1]}"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ NEXTEST_FILTER=$(cat "$ARTIFACT_DIR/nextest-filter.txt")
+
+ mkdir -p target/debug
+ tar -C target/debug -xf "$ARTIFACT_DIR/connector-plugins.tar"
+ tar -C target/debug -xzf "$ARTIFACT_DIR/runtime-binaries.tar.gz"
+
+ RUNTIME_BINARIES=(
+ iggy
+ iggy-server
+ iggy-server-ng
+ iggy-connectors
+ iggy-mcp
+ iggy-bench
+ )
+ CARGO_BIN_ENV=()
+ for binary in "${RUNTIME_BINARIES[@]}"; do
+ binary_path="${GITHUB_WORKSPACE}/target/debug/${binary}"
+ if [[ ! -x "$binary_path" ]]; then
+ echo "::error::Required test runtime binary not found after
extraction: ${binary_path}"
+ exit 1
+ fi
+ CARGO_BIN_ENV+=("CARGO_BIN_EXE_${binary}=${binary_path}")
+ done
+
+ source <(cargo llvm-cov show-env --sh)
+
+ if [[ "$RUNNER_OS" == "Linux" ]]; then
+ sudo sysctl -w vm.max_map_count=2000000 || true
Review Comment:
moving the sysctl from the build phase to here is right - doris boots at
test time, not build time. but the six-line comment explaining why went with
it. it named the kernel constraint and pointed at the testcontainers fixture,
which is exactly what the next reader won't reconstruct from a bare `|| true`.
worth carrying over.
##########
scripts/ci/sync-python-interpreter-version.sh:
##########
@@ -1,4 +1,4 @@
-#!/bin/bash
+#!/usr/bin/env bash
Review Comment:
unrelated to either commit in this PR, and incomplete - four other scripts
under `scripts/ci` still use `#!/bin/bash`. either split it out or convert all
of them.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -45,7 +51,7 @@ runs:
# fmt/sort/machete never build into target/, so the multi-GB cache
# restore is pure overhead. Compiling legs
(check/clippy/doctest/test-*)
# keep it; a cold cache there recompiles the whole dep tree.
- read-cache: ${{ (inputs.task == 'fmt' || inputs.task == 'sort' ||
inputs.task == 'machete') && 'false' || 'true' }}
+ read-cache: ${{ (inputs.task == 'fmt' || inputs.task == 'sort' ||
inputs.task == 'machete' || startsWith(inputs.task, 'test-run-')) && 'false' ||
'true' }}
Review Comment:
the comment two lines up still says compiling legs
"(check/clippy/doctest/test-*)" keep the cache, which this line now contradicts
for `test-run-*`.
knock-on worth knowing: `Configure Cargo for CI` in `setup-rust-with-cache`
is gated on `read-cache == 'true'`, so turning the cache off here also stops
exporting `CARGO_INCREMENTAL=0` and the profile debug settings on those legs.
harmless today since nothing compiles there, but it is an accidental coupling
of two unrelated concerns rather than a decision - `read-cache` is now doing
double duty as "is this a compiling job".
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -187,44 +191,30 @@ runs:
shell: bash
- name: Install dependencies for Rust tests
- if: startsWith(inputs.task, 'test-') && runner.os == 'Linux'
+ if: startsWith(inputs.task, 'test-run-') && runner.os == 'Linux'
run: |
sudo apt-get install --yes musl-tools gnome-keyring keyutils dbus-x11
libsecret-tools
rm -f $HOME/.local/share/keyrings/*
shell: bash
- name: Install cargo-llvm-cov
- if: startsWith(inputs.task, 'test-')
+ if: inputs.task == 'test-build' || startsWith(inputs.task, 'test-run-')
uses: taiki-e/install-action@v2
Review Comment:
splitting one job into two means the build job and the run jobs now resolve
`cargo-llvm-cov` independently on separate runners, so a release landing
between them is a producer/consumer skew that could not happen before. same for
`cargo-nextest`, which `setup-rust-with-cache` curls from the floating
`get.nexte.st/latest` url.
no tool is version-pinned anywhere in this file, so the unpinned pattern is
pre-existing - it is the split that turns it into a correctness risk. `tool:
cargo-llvm-cov@<version>` in both jobs would close it.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+ compression-level: 0
+ if-no-files-found: error
+ retention-days: 1
+
+ - name: Download archived Rust tests
+ if: startsWith(inputs.task, 'test-run-')
+ uses: actions/download-artifact@v8
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+
+ - name: Run archived tests with coverage
+ if: startsWith(inputs.task, 'test-run-')
+ run: |
+ TASK="${{ inputs.task }}"
+ if [[ ! "$TASK" =~ ^test-run-([0-9]+)$ ]]; then
+ echo "::error::Invalid archived test task: $TASK"
+ exit 1
+ fi
+ PARTITION_INDEX="${BASH_REMATCH[1]}"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ NEXTEST_FILTER=$(cat "$ARTIFACT_DIR/nextest-filter.txt")
+
+ mkdir -p target/debug
+ tar -C target/debug -xf "$ARTIFACT_DIR/connector-plugins.tar"
+ tar -C target/debug -xzf "$ARTIFACT_DIR/runtime-binaries.tar.gz"
+
+ RUNTIME_BINARIES=(
+ iggy
+ iggy-server
+ iggy-server-ng
+ iggy-connectors
+ iggy-mcp
+ iggy-bench
+ )
+ CARGO_BIN_ENV=()
+ for binary in "${RUNTIME_BINARIES[@]}"; do
+ binary_path="${GITHUB_WORKSPACE}/target/debug/${binary}"
+ if [[ ! -x "$binary_path" ]]; then
+ echo "::error::Required test runtime binary not found after
extraction: ${binary_path}"
+ exit 1
+ fi
+ CARGO_BIN_ENV+=("CARGO_BIN_EXE_${binary}=${binary_path}")
+ done
+
+ source <(cargo llvm-cov show-env --sh)
+
+ if [[ "$RUNNER_OS" == "Linux" ]]; then
+ sudo sysctl -w vm.max_map_count=2000000 || true
+ eval "$(dbus-launch --sh-syntax)"
+ export DBUS_SESSION_BUS_ADDRESS
+ eval "$(echo -n "test" | gnome-keyring-daemon --unlock
--components=secrets)"
+ echo -n "warmup" | secret-tool store --label="ci-warmup" ci-test
warmup
+ fi
+
+ test_start=$(date +%s)
+ env "${CARGO_BIN_ENV[@]}" cargo nextest run \
+ --archive-file "$ARTIFACT_DIR/rust-tests.tar.zst" \
+ --extract-to "$GITHUB_WORKSPACE" \
+ --extract-overwrite \
+ --workspace-remap "$GITHUB_WORKSPACE" \
+ --no-fail-fast \
+ --profile ci \
+ --partition "hash:${PARTITION_INDEX}/2" \
Review Comment:
the partition denominator `2` is hardcoded here, again in the notice at line
392, in the matrix at `_test.yml:277`, and in the task description at line 23 -
four places that have to stay in lockstep.
growing the matrix fails loudly, since nextest rejects `hash:3/2`. shrinking
it to `[1]` fails silently: partition 2's tests simply never run and the gate
stays green. deriving the denominator from `strategy.job-total` would collapse
all four.
##########
.github/workflows/_test.yml:
##########
@@ -262,3 +251,58 @@ jobs:
codecov.json
if-no-files-found: ignore
retention-days: 7
+
+ build-rust-tests:
+ if: ${{ startsWith(inputs.component, 'rust') && inputs.task == 'test' }}
Review Comment:
this gate is `startsWith(inputs.component, 'rust')` but the artifact name in
the composite action carries no component segment. today only `rust` declares a
`test` task, so exactly one job produces it - but give any other rust-prefixed
component a `test` task and two build jobs upload the same name in one run. it
fails loudly rather than silently mixing archives, so it is latent, but it is a
one-line hardening either way: interpolate `inputs.component` into the name, or
tighten this to `== 'rust'`.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -316,12 +309,87 @@ runs:
echo "DAG scope: full workspace (${TOTAL_CRATES}
crates)"
fi
echo "All targets build: ${bins_duration}s ($(date -ud
@${bins_duration} +'%M:%S'))"
- echo "Tests compile: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
- echo "Tests execute: ${test_duration}s ($(date -ud
@${test_duration} +'%M:%S'))"
+ echo "Tests compile and archive: ${compile_duration}s ($(date -ud
@${compile_duration} +'%M:%S'))"
echo "-----------------------------------------"
echo "Total build: ${build_duration}s ($(date -ud
@${build_duration} +'%M:%S'))"
- echo "Total time: ${total_duration}s ($(date -ud
@${total_duration} +'%M:%S'))"
echo "========================================="
+ du -h "$ARTIFACT_DIR"/*
+ shell: bash
+
+ - name: Upload archived Rust tests
+ if: inputs.task == 'test-build'
+ uses: actions/upload-artifact@v7
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+ compression-level: 0
+ if-no-files-found: error
+ retention-days: 1
+
+ - name: Download archived Rust tests
+ if: startsWith(inputs.task, 'test-run-')
+ uses: actions/download-artifact@v8
+ with:
+ name: rust-nextest-archive-${{ github.run_id }}-${{ github.run_attempt
}}
+ path: ${{ runner.temp }}/rust-nextest-archive
+
+ - name: Run archived tests with coverage
+ if: startsWith(inputs.task, 'test-run-')
+ run: |
+ TASK="${{ inputs.task }}"
+ if [[ ! "$TASK" =~ ^test-run-([0-9]+)$ ]]; then
+ echo "::error::Invalid archived test task: $TASK"
+ exit 1
+ fi
+ PARTITION_INDEX="${BASH_REMATCH[1]}"
+ ARTIFACT_DIR="${RUNNER_TEMP}/rust-nextest-archive"
+ NEXTEST_FILTER=$(cat "$ARTIFACT_DIR/nextest-filter.txt")
+
+ mkdir -p target/debug
+ tar -C target/debug -xf "$ARTIFACT_DIR/connector-plugins.tar"
+ tar -C target/debug -xzf "$ARTIFACT_DIR/runtime-binaries.tar.gz"
+
+ RUNTIME_BINARIES=(
+ iggy
+ iggy-server
+ iggy-server-ng
+ iggy-connectors
+ iggy-mcp
+ iggy-bench
+ )
+ CARGO_BIN_ENV=()
+ for binary in "${RUNTIME_BINARIES[@]}"; do
+ binary_path="${GITHUB_WORKSPACE}/target/debug/${binary}"
+ if [[ ! -x "$binary_path" ]]; then
+ echo "::error::Required test runtime binary not found after
extraction: ${binary_path}"
+ exit 1
+ fi
+ CARGO_BIN_ENV+=("CARGO_BIN_EXE_${binary}=${binary_path}")
+ done
+
+ source <(cargo llvm-cov show-env --sh)
Review Comment:
`source <(cmd)` cannot fail. a failing command inside process substitution
yields an empty fifo and `source` returns 0, so `set -e` never fires and the
step continues without the env. same pattern at line 230, and in two more added
lines just below - `eval "$(dbus-launch --sh-syntax)"` at 374 and the
`gnome-keyring-daemon --unlock` eval at 376 swallow failures identically, so
`export DBUS_SESSION_BUS_ADDRESS` exports nothing and the blame lands on
`secret-tool store` at 377.
worth knowing `eval "$(cmd)"` has the same hole - it receives an empty
string and returns 0. the forms that actually propagate are `x=$(cmd)` or
redirect-then-source:
cargo llvm-cov show-env --sh > "$RUNNER_TEMP/llvm-cov-env.sh"
source "$RUNNER_TEMP/llvm-cov-env.sh"
not urgent: `cargo llvm-cov report` bails when it finds no profraw or object
files, so these end red rather than green. they just end red pointing at the
wrong step.
##########
.github/actions/rust/pre-merge/action.yml:
##########
@@ -187,44 +191,30 @@ runs:
shell: bash
- name: Install dependencies for Rust tests
- if: startsWith(inputs.task, 'test-') && runner.os == 'Linux'
+ if: startsWith(inputs.task, 'test-run-') && runner.os == 'Linux'
run: |
sudo apt-get install --yes musl-tools gnome-keyring keyutils dbus-x11
libsecret-tools
Review Comment:
`musl-tools` provides the build-time `musl-gcc` linker, and the `if:` above
narrows this step to `test-run-*`, which compiles nothing - so it is now
installed only where it cannot be used, and absent from `test-build`, where
compilation actually happens. the aarch64-musl job installs its own copy anyway
and nothing on the test path links musl, so probably just delete it rather than
move it back. the other four packages on this line are genuinely needed at test
time.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
