Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 14 additions & 0 deletions ci/plugins/mzcompose/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,18 @@ at the last occurrence of the marker. Logs without the marker are scanned in
full. This is useful for workflows that exercise historical binaries before
testing the current build.

## Cleaning up resources outside of Docker

A composition that creates resources outside of Docker, such as a Cloud
region, can define a workflow named `ci-cleanup`. The command hook runs it
after the main workflow has exited, however it exited, and passes it the same
`args`. Cancelling or timing out a job ends the main workflow with SIGTERM,
which does not run Python `finally` blocks, so a composition must not rely on
its own cleanup path for those cases. Before the workflow runs, the hook kills
the main workflow's containers, so nothing left over from the main run can
race the cleanup; the Docker teardown proper happens afterwards. The workflow
must be idempotent: it also runs after a successful run that already cleaned
up. It writes no JUnit report, so the main workflow's report survives. Its
failure is recorded in the error annotation and fails an otherwise green job.
Comment on lines +29 to +39

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The section is missing the three things the next adopter cannot infer from the hook. test/mz-e2e and test/cloud-canary, which the description names as follow-ups, would hit all three on their first attempt:

  1. The workflow receives the main workflow's full args: list plus CI_EXTRA_ARGS, so it must use parse_known_args. With parse_args it exits 2 on every run, the marker fires, and every green job of that composition turns red.
  2. check-mzcompose-files.sh fails a test/ composition with more than one def workflow_ unless default loops over c.workflows, and both of those compositions have exactly one workflow today and are not on its exclusion list. The remedy the lint prints would make default run ci-cleanup mid-run. Worth fixing the lint to skip workflow_ci_cleanup in this PR (see the review body), and saying so here either way.
  3. "kills" is docker kill, so SIGKILL, and it runs on green jobs too. Under CI_COVERAGE_ENABLED the hook deliberately uses a graceful mzcompose down so the .profraw files get written at exit; the kill would lose them silently. The 15m budget is also only in a hook comment.
Suggested change
A composition that creates resources outside of Docker, such as a Cloud
region, can define a workflow named `ci-cleanup`. The command hook runs it
after the main workflow has exited, however it exited, and passes it the same
`args`. Cancelling or timing out a job ends the main workflow with SIGTERM,
which does not run Python `finally` blocks, so a composition must not rely on
its own cleanup path for those cases. Before the workflow runs, the hook kills
the main workflow's containers, so nothing left over from the main run can
race the cleanup; the Docker teardown proper happens afterwards. The workflow
must be idempotent: it also runs after a successful run that already cleaned
up. It writes no JUnit report, so the main workflow's report survives. Its
failure is recorded in the error annotation and fails an otherwise green job.
A composition that creates resources outside of Docker, such as a Cloud
region, can define a workflow named `ci-cleanup`. The command hook runs it
after the main workflow has exited, however it exited, and passes it the main
workflow's full argument list (the step's `args`, plus any `CI_EXTRA_ARGS`),
so the workflow must parse with `parse_known_args` and must be able to find
its target from those arguments alone. Cancelling or timing out a job ends
the main workflow with SIGTERM, which does not run Python `finally` blocks,
so a composition must not rely on its own cleanup path for those cases.
Before the workflow runs, the hook stops the main run's compose-managed
containers with `docker kill` (SIGKILL), so that a command still in flight
from the main run, such as an `mz region enable` that outlived the cancelled
process, cannot undo the cleanup once it has finished; the Docker teardown
proper happens afterwards. The kill is skipped
under `CI_COVERAGE_ENABLED`, since a killed process writes no `.profraw`. The
workflow must be idempotent: it also runs after a successful run that already
cleaned up. It has 15 minutes, so that a hung cleanup cannot eat the cancel
grace period before the artifacts are uploaded. It writes no JUnit report, so
the main workflow's report survives. Its failure is recorded in the error
annotation and fails an otherwise green job. A composition whose `default`
loops over `c.workflows` must skip `ci-cleanup` in that loop.


[Buildkite plugin]: https://buildkite.com/docs/agent/v3/plugins
27 changes: 27 additions & 0 deletions ci/plugins/mzcompose/hooks/command
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,33 @@ cleanup() {
printf "\n%s" "$BUILDKITE_LABEL: test timed out" >> run.log

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: no trailing newline, and the cleanup's tee -a run.log below now appends straight after this, so on a timed-out job the marker line becomes <label>: test timed out--- <first cleanup line>. ERROR_RE still matches (.* $), so the annotation fires, but its text carries whatever the cleanup printed first.

Suggested change
printf "\n%s" "$BUILDKITE_LABEL: test timed out" >> run.log
printf "\n%s\n" "$BUILDKITE_LABEL: test timed out" >> run.log

fi

# A composition that holds resources outside of Docker, such as a Cloud
# region, can declare a `ci-cleanup` workflow. It runs here, before the Docker
# teardown, with the arguments of the main workflow. A cancelled or timed-out
# job reaches this trap through SIGTERM, which ends the mzcompose process
# without running the composition's own cleanup path, so this is the only
# cleanup such a run gets. A failure goes to run.log, where ci-annotate-errors
# turns it into an error annotation and fails an otherwise green job.
if echo "$workflows" | grep -x "ci-cleanup" > /dev/null; then
ci_unimportant_heading ":docker: Running the composition's ci-cleanup workflow"
# The main workflow's containers can outlive its process: SIGTERM ends
# mzcompose, but a `docker compose run` container it started keeps going,
# and an in-flight `mz region enable` could re-create the region after the
# cleanup deleted it. Only this composition's compose project (named after
# the composition): other jobs' containers, and containers a composition
# drives outside compose, such as kind nodes, keep running for the log
# collection below. Killed containers keep their logs for services.log;
# `mzcompose down` removes them.
docker ps --quiet --filter "label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" | xargs --no-run-if-empty docker kill > /dev/null || true
Comment on lines +164 to +165

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem. docker kill is SIGKILL, and this runs on every job of a composition that defines ci-cleanup, successful ones included. Under CI_COVERAGE_ENABLED the hook's teardown is deliberately graceful (mzcompose down --volumes, line 289) because the LLVM runtime writes .profraw only at normal process exit. A SIGKILL before that point discards the job's coverage data, and nothing reports it: the job stays green, there is just no .profraw to collect.

Scope. cluster-spec-sheet does not run under coverage, so nothing breaks today. This is for the next composition that adopts ci-cleanup.

Fix. Skip the kill when CI_COVERAGE_ENABLED is set; the graceful down then handles the containers, as it does for every other coverage job. The race the kill guards against (an mz region enable outliving the cancelled process) is a Cloud-target concern, and coverage jobs do not run against Cloud, so nothing is lost.

Suggested change
# `mzcompose down` removes them.
docker ps --quiet --filter "label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" | xargs --no-run-if-empty docker kill > /dev/null || true
# `mzcompose down` removes them. Skipped under coverage: SIGKILL loses the
# .profraw files written at exit, and the coverage teardown is graceful for
# the same reason.
if [ -z "${CI_COVERAGE_ENABLED:-}" ]; then
docker ps --quiet --filter "label=com.docker.compose.project=$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" | xargs --no-run-if-empty docker kill > /dev/null || true
fi

# 15m keeps a hung cleanup from eating the agents' 40-minute cancel grace
# period before the artifacts and the error annotation. Normal disables
# take about 30 s. The output also goes to run.log so that the artifact
# shows why a cleanup failed.
if ! bin/ci-builder run "$builder" timeout --signal=TERM --kill-after=30s 15m bin/mzcompose --find "$BUILDKITE_PLUGIN_MZCOMPOSE_COMPOSITION" run ci-cleanup "${run_args[@]:1}" |& sed -u -r 's/\x1B\[[0-9;]*[A-Za-z]//g' | tee -a run.log; then
printf "\n%s" "$BUILDKITE_LABEL: ci-cleanup workflow failed" >> run.log
fi
fi

ci_unimportant_heading "Post command steps"
# Run before potential "run down" in coverage
docker ps --all --quiet | xargs --no-run-if-empty docker inspect | jq '
Expand Down
2 changes: 1 addition & 1 deletion ci/spec-sheet/pipeline.template.yml
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,7 @@ steps:
queue: linux-aarch64-small

- id: cluster-spec-sheet-staging
label: "Cluster spec sheet: Staging"
label: "Cluster spec sheet: envd Scalability + Cluster Object Count Limits (against Staging)"
timeout_in_minutes: 3600
depends_on: devel-docker-tags
parallelism: 7
Expand Down
1 change: 1 addition & 0 deletions misc/python/materialize/cli/ci_annotate_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@
| SUMMARY:\ .*Sanitizer
| primary\ source\ \w+\ seemingly\ dropped\ before\ subsource
| :\ test\ timed\ out
| :\ ci-cleanup\ workflow\ failed
| very\ slow\ coordinator\ message
# Only notifying on unexpected failures. INT, TRAP, BUS, FPE, SEGV, PIPE
| \ ANOM_ABEND\ .*\ sig=(2|5|7|8|11|13)
Expand Down
12 changes: 10 additions & 2 deletions misc/python/materialize/cli/mzcompose.py
Original file line number Diff line number Diff line change
Expand Up @@ -869,7 +869,7 @@ def handle_composition(
workflow_name, *args.unknown_subargs[1:], *extra_args
)

if self.shall_generate_junit_report(args.find, composition):
if self.shall_generate_junit_report(args.find, workflow_name, composition):
junit_suite = self.generate_junit_suite(composition)
self.write_junit_report_to_file(junit_suite)

Expand All @@ -880,8 +880,16 @@ def handle_composition(
raise UIError("at least one test case failed")

def shall_generate_junit_report(
self, composition_name: str | None, composition: Composition
self,
composition_name: str | None,
workflow_name: str | None,
composition: Composition,
) -> bool:
if workflow_name == "ci-cleanup":
# Runs from the CI plugin's exit trap after the main workflow, under
# the same BUILDKITE_JOB_ID, so its report would overwrite the main
# run's. Its outcome is reported through run.log instead.
return False
Comment on lines +888 to +892

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Design point rather than a defect: the reason for the suppression is a property of how the plugin invokes the workflow (same BUILDKITE_JOB_ID, hence the same junit_mzcompose_<job id>.xml), but the policy lives here, keyed on a workflow name the CLI otherwise knows nothing about. Two consequences: a developer who runs bin/mzcompose run ci-cleanup by hand, or a composition whose default calls c.workflow("ci-cleanup"), silently gets no report; and a composition that names its cleanup workflow anything else gets no suppression. An env var or --no-junit-report set by the hook on this one invocation would keep the decision where the knowledge is. Fine to defer if you would rather not widen the CLI surface now.

if composition.has_testdrive_junit:
# Testdrive already produced a junit.xml with detailed errors;
# skip the mzcompose-level junit to avoid duplicate annotations.
Expand Down
20 changes: 14 additions & 6 deletions test/cluster-spec-sheet/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,12 @@ Reproduce data for the cluster spec sheet effort.

# Usage

`bin/mzcompose --find cluster-spec-sheet run default`
`bin/mzcompose --find cluster-spec-sheet run default --target=<target>`

This will run all scenarios currently defined for the cluster spec sheet.
`--target` is required: `cloud-production`, `cloud-staging`, or `docker`.
There is deliberately no default, because the CI cleanup below destroys the
target's region unattended.

Pass `--cleanup` to disable the region after the test.

Expand All @@ -15,19 +18,24 @@ Pass `--cleanup` to disable the region after the test.

## Running via Buildkite

The workload runs as part of the release qualification pipeline in Buildkite.
The workload runs in the `spec-sheet` Buildkite pipeline. After a CI job
ends, however it ends, the mzcompose plugin runs the `ci-cleanup` workflow
with the job's arguments; for a Cloud target started with `--cleanup` it
disables the region. A canceled or timed-out job never reaches the
composition's own cleanup, so this is what keeps canceled runs from leaving
regions behind.

## Running manually in Cloud

To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, you need to set the environment variables `NIGHTLY_CANARY_USERNAME` and `NIGHTLY_CANARY_APP_PASSWORD`.
To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, the run uses one account from the E2E database pool: set `E2E_STAGING_TEST_FRONTEGG_DATABASE_APP_PASSWORD_<n>` for the pool index `<n>` and select it with `CI_CONCURRENCY_POOL_SLOT=<n>` (outside CI, index 0 is used when the slot is unset); the username is derived from the index.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A manual staging run also needs BUILDKITE_COMMIT, which staging_version() reads unconditionally.

Suggested change
To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, the run uses one account from the E2E database pool: set `E2E_STAGING_TEST_FRONTEGG_DATABASE_APP_PASSWORD_<n>` for the pool index `<n>` and select it with `CI_CONCURRENCY_POOL_SLOT=<n>` (outside CI, index 0 is used when the slot is unset); the username is derived from the index.
To run the cloud canary test manually, you can specify either `--target=cloud-production` (which is hardcoded to aws/us-east-1) or `--target=cloud-staging` (which is hardcoded to aws/eu-west-1). For production, you need to set the environment variables `NIGHTLY_MZ_USERNAME` and `MZ_CLI_APP_PASSWORD`. For staging, the run uses one account from the E2E database pool: set `E2E_STAGING_TEST_FRONTEGG_DATABASE_APP_PASSWORD_<n>` for the pool index `<n>` and select it with `CI_CONCURRENCY_POOL_SLOT=<n>` (outside CI, index 0 is used when the slot is unset); the username is derived from the index. Staging runs also need `BUILDKITE_COMMIT`, which selects the image version to enable.


The username is an email address, the app password is a password generated in the cloud console (something like `mzp_...`).

Once the environment variables have been set, you can run:

```
cd test/cluster-spec-sheet
./mzcompose run default
./mzcompose run default --target=cloud-production
```

## Running in Docker
Expand Down Expand Up @@ -55,11 +63,11 @@ bin/mzcompose --find cluster-spec-sheet run default envd_qps_scalability --targ
```
or
```
bin/mzcompose --find cluster-spec-sheet run default cluster
bin/mzcompose --find cluster-spec-sheet run default cluster --target=cloud-production
```
or
```
bin/mzcompose --find cluster-spec-sheet run default envd_objects_scalability
bin/mzcompose --find cluster-spec-sheet run default envd_objects_scalability --target=cloud-production
```
or
```
Expand Down
Loading
Loading