diff --git a/ci/plugins/mzcompose/README.md b/ci/plugins/mzcompose/README.md index b66ab43e9f5c8..486c02ea5a995 100644 --- a/ci/plugins/mzcompose/README.md +++ b/ci/plugins/mzcompose/README.md @@ -24,4 +24,31 @@ 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 main +workflow's full argument list: the step's `args` plus any `CI_EXTRA_ARGS`. The +workflow must therefore parse with `parse_known_args` and 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 kills (SIGKILL) the containers of the main +run's compose project, 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 after it has finished; the Docker teardown proper happens +afterwards. The kill is skipped under `CI_COVERAGE_ENABLED`, where the Docker +teardown is deliberately graceful. 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. + +The mzcompose-files lint does not count `ci-cleanup` as a workflow, so a +composition with a single main workflow can add it without looping over +`c.workflows`; a `default` that does loop must skip `ci-cleanup`. + [Buildkite plugin]: https://buildkite.com/docs/agent/v3/plugins diff --git a/ci/plugins/mzcompose/hooks/command b/ci/plugins/mzcompose/hooks/command index a4fa58e6eecd7..540e67184457e 100644 --- a/ci/plugins/mzcompose/hooks/command +++ b/ci/plugins/mzcompose/hooks/command @@ -142,7 +142,38 @@ cleanup() { END_TIME=$(date +%s) ELAPSED=$((END_TIME - START_TIME)) if [ $ELAPSED -ge $((BUILDKITE_TIMEOUT * 60)) ]; then - 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. Skipped under coverage: the coverage + # teardown below stops containers gracefully on purpose, and a kill here + # would defeat it. + 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\n" "$BUILDKITE_LABEL: ci-cleanup workflow failed" >> run.log + fi fi ci_unimportant_heading "Post command steps" diff --git a/ci/spec-sheet/pipeline.template.yml b/ci/spec-sheet/pipeline.template.yml index a9dac5052a1c4..0550b40081816 100644 --- a/ci/spec-sheet/pipeline.template.yml +++ b/ci/spec-sheet/pipeline.template.yml @@ -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 diff --git a/ci/test/lint-main/checks/check-mzcompose-files.sh b/ci/test/lint-main/checks/check-mzcompose-files.sh index 9045e3d8934c2..9de0f9661eecd 100755 --- a/ci/test/lint-main/checks/check-mzcompose-files.sh +++ b/ci/test/lint-main/checks/check-mzcompose-files.sh @@ -59,7 +59,8 @@ check_default_workflow_references_others() { ) for file in "${MZCOMPOSE_TEST_FILES[@]}"; do - MATCHES_COUNT=$(grep "def workflow_" "$file" -c) + # `ci-cleanup` is run by the mzcompose plugin's hook, never by `default`. + MATCHES_COUNT=$(grep "def workflow_" "$file" | grep -vc "def workflow_ci_cleanup" || true) if (( MATCHES_COUNT > 1 )); then # mzcompose file contains more than one workflow diff --git a/misc/python/materialize/cli/ci_annotate_errors.py b/misc/python/materialize/cli/ci_annotate_errors.py index 6bed1881c5ad5..3097f3f2dbc2e 100644 --- a/misc/python/materialize/cli/ci_annotate_errors.py +++ b/misc/python/materialize/cli/ci_annotate_errors.py @@ -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) diff --git a/misc/python/materialize/cli/mzcompose.py b/misc/python/materialize/cli/mzcompose.py index c438e09ee9e05..b7b314ca74cfd 100644 --- a/misc/python/materialize/cli/mzcompose.py +++ b/misc/python/materialize/cli/mzcompose.py @@ -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) @@ -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 if composition.has_testdrive_junit: # Testdrive already produced a junit.xml with detailed errors; # skip the mzcompose-level junit to avoid duplicate annotations. diff --git a/test/cluster-spec-sheet/README.md b/test/cluster-spec-sheet/README.md index 83ba8edc435a5..6328c5b57726f 100644 --- a/test/cluster-spec-sheet/README.md +++ b/test/cluster-spec-sheet/README.md @@ -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=` 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. @@ -15,11 +18,16 @@ 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_` for the pool index `` and select it with `CI_CONCURRENCY_POOL_SLOT=` (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_...`). @@ -27,7 +35,7 @@ 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 @@ -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 ``` diff --git a/test/cluster-spec-sheet/mzcompose.py b/test/cluster-spec-sheet/mzcompose.py index 0cdcbdab910c2..8192475a73be0 100644 --- a/test/cluster-spec-sheet/mzcompose.py +++ b/test/cluster-spec-sheet/mzcompose.py @@ -13,8 +13,10 @@ import argparse import csv +import functools import glob import itertools +import json import os import re import shlex @@ -104,7 +106,10 @@ def staging_credentials() -> tuple[str, str]: } +@functools.cache def staging_version() -> str: + # Cached: the version cannot change within a run, and `parse_cargo` shells + # out to cargo, whose failure must not surface inside a region retry loop. return f"{MzVersion.parse_cargo()}--pr.g{os.environ['BUILDKITE_COMMIT']}" @@ -3386,16 +3391,47 @@ def teardown(self, runner: ScenarioRunner) -> None: # TODO: We should factor the region helpers below out into a separate module. # (Similar `disable_region` functions also occur in other tests.) def disable_region(composition: Composition, hard: bool) -> None: + # `mz region disable` reports a region that does not exist as success (its + # `disable` maps the API's 404 to "Region already disabled"), so any failure + # here is a real one (auth, API error) and must surface. print("Shutting down region ...") + if hard: + composition.run("mz", "region", "disable", "--hard", rm=True) + else: + composition.run("mz", "region", "disable", rm=True) + + +def verify_region_disabled(composition: Composition, region: str) -> None: + """Fail unless `mz region list` reports `region` as disabled. + `mz region disable` returning is not proof: it can be answered by a stale + or failing API, and a region that survives a cleanup keeps costing money + and load until someone notices. "disabled" also covers a region whose + deletion is still pending, so the check proves that no enabled region is + left, not that the deletion has completed. + """ + output = composition.run( + "mz", "region", "list", "--format", "json", rm=True, capture_and_print=True + ).stdout + # `mz region list` reports a cloud provider whose lookup failed as an + # `Error: ...` line on stdout before the JSON array, so parsing the whole + # output would fail on an unrelated provider's hiccup. + lines = output.strip().splitlines() + start = next((i for i, line in enumerate(lines) if line.startswith("[")), None) try: - if hard: - composition.run("mz", "region", "disable", "--hard", rm=True) - else: - composition.run("mz", "region", "disable", rm=True) - except UIError: - # Can return: status 404 Not Found - pass + if start is None: + raise ValueError("no JSON array in the output") + statuses = { + entry["region"]: entry["status"] + for entry in json.loads("\n".join(lines[start:])) + } + except (ValueError, KeyError, TypeError) as e: + raise UIError(f"unexpected `mz region list` output: {output!r}") from e + status = statuses.get(region) + if status is None: + raise UIError(f"region {region} missing from `mz region list` output") + if status != "disabled": + raise UIError(f"region {region} is still {status} after disable") def enable_region(target: "CloudTarget", envd_cpus: int | None = None) -> None: @@ -3417,12 +3453,13 @@ def enable_region(target: "CloudTarget", envd_cpus: int | None = None) -> None: # Production Cloud forbids callers from injecting environmentd args and rejects # `--environmentd-extra-arg` with a 403 Forbidden, so this is staging-only. args += [ - "--environmentd-extra-arg=--system-parameter-default=with_0dt_caught_up_check_stability_period=0s" + "--environmentd-extra-arg=--system-parameter-default=with_0dt_caught_up_check_stability_period=0s", + # Pin the image built for this PR. Production does not accept a + # custom version. + "--version", + staging_version(), ] - if target.version is not None: - args += ["--version", target.version] - target.composition.run("mz", "region", "enable", *args, rm=True) @@ -3480,19 +3517,19 @@ def cloud_recreate_region_with_envd_cpus( soon as it has booted, in about a minute. """ for attempt in range(1, attempts + 1): - disable_region(target.composition, hard=True) try: + disable_region(target.composition, hard=True) enable_region(target, envd_cpus=envd_cpus) break except UIError as e: # A sweep does a dozen of these calls and the staging Cloud API returns the - # occasional 502, which `mz region enable` does not retry on its own. We - # disable again before retrying, so that a half-finished enable can't leave - # the region running with the previous allocation. + # occasional 502 that surfaces despite the CLI's own retries. A retry starts + # over with the disable, so that a half-finished enable can't leave the + # region running with the previous allocation. if attempt == attempts: raise print( - f"WARNING: 'mz region enable' failed (attempt {attempt}/{attempts}): {e}" + f"WARNING: recreating the region failed (attempt {attempt}/{attempts}): {e}" ) time.sleep(30) @@ -3748,16 +3785,91 @@ def log_environment_info(target: "BenchTarget") -> None: pass -def workflow_default(composition: Composition, parser: WorkflowArgumentParser) -> None: - """ - Run the bench workflow by default - """ +def add_target_arguments(parser: argparse.ArgumentParser) -> None: + """The arguments `workflow_default` and `workflow_ci_cleanup` share.""" parser.add_argument( "--cleanup", default=False, action=argparse.BooleanOptionalAction, - help="Destroy the region at the end of the workflow.", + help="Destroy the region when the workflow ends, and from CI's ci-cleanup after a cancel or timeout.", ) + # Required rather than defaulting to cloud-production: `ci-cleanup` runs + # unattended from the CI plugin's exit trap and destroys the target's + # region, so a missing or malformed target must fail loudly instead of + # quietly selecting production. Every CI step passes it explicitly. + parser.add_argument( + "--target", + required=True, + choices=["cloud-production", "cloud-staging", "docker"], + help="Target to deploy to.", + ) + + +def make_target(composition: Composition, target: str) -> tuple["BenchTarget", Mz]: + """The bench target for `--target`, with the `mz` service configured for it.""" + if target == "cloud-production": + bench_target: BenchTarget = CloudTarget( + composition, + PRODUCTION_USERNAME, + PRODUCTION_APP_PASSWORD or "", + region=PRODUCTION_REGION, + ) + mz = Mz( + region=PRODUCTION_REGION, + environment=PRODUCTION_ENVIRONMENT, + app_password=PRODUCTION_APP_PASSWORD or "", + ) + elif target == "cloud-staging": + staging_username, staging_app_password = staging_credentials() + bench_target = CloudTarget( + composition, + staging_username, + staging_app_password, + region=STAGING_REGION, + is_staging=True, + ) + mz = Mz( + region=STAGING_REGION, + environment=STAGING_ENVIRONMENT, + app_password=staging_app_password, + ) + elif target == "docker": + bench_target = DockerTarget(composition) + mz = Mz(app_password="") + else: + raise ValueError(f"Unknown target: {target}") + return bench_target, mz + + +def workflow_ci_cleanup( + composition: Composition, parser: WorkflowArgumentParser +) -> None: + """ + Destroy the Cloud region of a run that did not get to its own cleanup. + + The CI mzcompose plugin runs this workflow after `default` has exited, however + it exited, with the same arguments. A cancelled or timed-out job ends `default` + with SIGTERM, which skips its `finally` block, so for such a run this is the + only region cleanup there is. Only `--cleanup` and `--target` are read. + """ + add_target_arguments(parser) + args, _ = parser.parse_known_args() + if not args.cleanup: + print("Not destroying the region: the run was started without --cleanup") + return + target, mz = make_target(composition, args.target) + if not isinstance(target, CloudTarget): + print(f"Nothing to clean up for --target={args.target}") + return + with composition.override(mz): + target.cleanup() + + +def workflow_default(composition: Composition, parser: WorkflowArgumentParser) -> None: + """ + Run the bench workflow by default + """ + add_target_arguments(parser) parser.add_argument( "--record", default=f"results_{int(time.time())}.csv", @@ -3769,12 +3881,6 @@ def workflow_default(composition: Composition, parser: WorkflowArgumentParser) - action=argparse.BooleanOptionalAction, help="Analyze results after completing test. Dispatches to cluster-scale or envd-scale focused analyses based on the file suffix: `.cluster.csv` or `.envd.csv`.", ) - parser.add_argument( - "--target", - default="cloud-production", - choices=["cloud-production", "cloud-staging", "docker"], - help="Target to deploy to (default: cloud-production).", - ) parser.add_argument( "--max-scale", type=int, @@ -3865,34 +3971,7 @@ def workflow_default(composition: Composition, parser: WorkflowArgumentParser) - f"{', '.join(sorted(cluster_object_limits_requested))}." ) - if args.target == "cloud-production": - target: BenchTarget = CloudTarget( - composition, PRODUCTION_USERNAME, PRODUCTION_APP_PASSWORD or "" - ) - mz = Mz( - region=PRODUCTION_REGION, - environment=PRODUCTION_ENVIRONMENT, - app_password=PRODUCTION_APP_PASSWORD or "", - ) - elif args.target == "cloud-staging": - staging_username, staging_app_password = staging_credentials() - target: BenchTarget = CloudTarget( - composition, - staging_username, - staging_app_password, - is_staging=True, - version=staging_version(), - ) - mz = Mz( - region=STAGING_REGION, - environment=STAGING_ENVIRONMENT, - app_password=staging_app_password, - ) - elif args.target == "docker": - target = DockerTarget(composition) - mz = Mz(app_password="") - else: - raise ValueError(f"Unknown target: {args.target}") + target, mz = make_target(composition, args.target) with composition.override(mz): target_max = target.max_scale() @@ -3941,32 +4020,40 @@ def process(scenario_name: str) -> None: test_failed = True try: - scenarios_list = buildkite.shard_list(sorted(list(scenarios)), lambda s: s) - composition.test_parts(scenarios_list, process) - test_failed = False - finally: + try: + scenarios_list = buildkite.shard_list( + sorted(list(scenarios)), lambda s: s + ) + composition.test_parts(scenarios_list, process) + test_failed = False + finally: + for stream in streams.values(): + stream.file.close() + + # Upload, archive, and analyze each result stream uniformly. The + # cluster_object_limits stream's extra `healthy` / `failure_mode` + # columns are silently dropped on upload (CSV writer uses + # extrasaction="ignore"); to recover them, consult the artifact + # CSV directly. for stream in streams.values(): - stream.file.close() - if args.cleanup: - target.cleanup() + stream.spec.upload(composition, stream.path, not test_failed) - # Upload, archive, and analyze each result stream uniformly. The - # cluster_object_limits stream's extra `healthy` / `failure_mode` - # columns are silently dropped on upload (CSV writer uses - # extrasaction="ignore"); to recover them, consult the artifact - # CSV directly. - for stream in streams.values(): - stream.spec.upload(composition, stream.path, not test_failed) + assert not test_failed - assert not test_failed + if buildkite.is_in_buildkite(): + for stream in streams.values(): + buildkite.upload_artifact(stream.path, cwd=MZ_ROOT, quiet=True) - if buildkite.is_in_buildkite(): - for stream in streams.values(): - buildkite.upload_artifact(stream.path, cwd=MZ_ROOT, quiet=True) - - if args.analyze: - for stream in streams.values(): - stream.spec.analyze(stream.path) + if args.analyze: + for stream in streams.values(): + stream.spec.analyze(stream.path) + finally: + # Only after the results are out: the teardown can fail (a slow + # hard delete, a transient API error in its verify) and must not + # cost a multi-hour run its data. The CI plugin's `ci-cleanup` + # repeats the teardown after the workflow, however it ended. + if args.cleanup: + target.cleanup() class BenchTarget: @@ -4016,19 +4103,15 @@ def __init__( composition: Composition, username: str, app_password: str, + region: str, is_staging: bool = False, - version: str | None = None, ) -> None: self.composition = composition self.username = username + self.region = region self.app_password = app_password self.new_app_password: str | None = None self.is_staging = is_staging - # Set for staging runs so `mz region enable --version ` pins the - # exact image built for this PR. Must be None for production (production - # doesn't accept a custom version). - self.version = version - assert (version is not None) == is_staging def dbbench_connection_flags(self) -> list[str]: assert self.new_app_password is not None @@ -4083,6 +4166,7 @@ def new_connection(self) -> psycopg.Connection: def cleanup(self) -> None: disable_region(self.composition, hard=True) + verify_region_disabled(self.composition, self.region) # M.1 size with the same worker count as the {scale}00cc size. Scales # above 8 have no available M.1 equivalent.