Skip to content

Fixes #6818: Harden storage Compose scripts - #7151

Open
BobSong-dev wants to merge 5 commits into
apache:masterfrom
BobSong-dev:fix/6818-storage-compose-reliability
Open

BobSong-dev wants to merge 5 commits into
apache:masterfrom
BobSong-dev:fix/6818-storage-compose-reliability

Conversation

@BobSong-dev

@BobSong-dev BobSong-dev commented Sep 21, 2026

Copy link
Copy Markdown
Contributor

Fixes #6818

Background

The storage Compose scripts ignored failures, left containers running after failures, and relied on fixed 30-second and 60-second sleeps. This could let storage initialization or service startup fail silently and made the E2E timing dependent on machine speed. The Compose files already define service healthchecks, but the scripts did not wait on them directly.

Changes

  • Enable set -euo pipefail in the four CI storage scripts and the storage matrix script;
  • Replace fixed sleeps and the invalid local healthcheck helper calls with docker compose up --wait;
  • Create the external shenyu network idempotently before startup;
  • Add EXIT cleanup traps and clean up each storage iteration in the matrix script.

Verification

  • Git Bash bash -n: all five storage scripts passed;
  • docker compose config --quiet: all four storage Compose files passed;
  • docker compose up --help: the installed CLI supports --wait;
  • git diff --check: passed;

## run e2e-test
sleep 60s

./mvnw -B -f ./shenyu-e2e/pom.xml -pl shenyu-e2e-case/shenyu-e2e-case-storage -am test

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Blocking: with set -euo pipefail, this line aborts the script on failure, so the log dumps below (lines 36-41) become unreachable.

Those two blocks are the only reason they exist - this PR's own CI run e2e-storage (shenyu-e2e-case-storage, e2e-postgres-compose) aborted here and no admin/bootstrap logs were printed to the job output. Previously the script carried on and still emitted them.

Suggestion: keep fail-fast behaviour but always dump the logs first, e.g.

if ! ./mvnw -B -f ./shenyu-e2e/pom.xml -pl shenyu-e2e-case/shenyu-e2e-case-storage -am test; then
docker compose -f "$COMPOSE_FILE" logs shenyu-admin || true
docker compose -f "$COMPOSE_FILE" logs shenyu-bootstrap || true
exit 1
fi

@Aias00 Aias00 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The direction is right. I checked the four compose files (shenyu-e2e-case/compose/storage/shenyu-storage-{h2,mysql,opengauss,postgres}.yml): all four declare healthcheck for shenyu-admin (9095) and shenyu-bootstrap (9195) with start_period: 30s, plus a healthcheck on the database service, so docker compose up --wait is a legitimate replacement for the fixed sleep 30s + sleep 60s + healthcheck.sh dance. Adding set -euo pipefail, an EXIT trap and network create ... || true are all net improvements. Two things need fixing before merge.

  1. [blocking] set -euo pipefail makes the log dumps unreachable

All five scripts now run the test with -e active, and the two docker compose logs blocks afterwards are the entire diagnostics value of these scripts. When the test fails the shell exits immediately and nothing is printed - exactly what happened in this PR's own CI: e2e-storage (shenyu-e2e-case-storage, e2e-postgres-compose) aborted and the job output contains no admin/bootstrap logs. Before this change the script continued and always emitted them.

Please keep fail-fast but guarantee the logs, e.g.

if ! ./mvnw -B -f ./shenyu-e2e/pom.xml -pl shenyu-e2e-case/shenyu-e2e-case-storage -am test; then
  docker compose -f "$COMPOSE_FILE" logs shenyu-admin || true
  docker compose -f "$COMPOSE_FILE" logs shenyu-bootstrap || true
  exit 1
fi

(in e2e-storage-compose.sh that goes inside the loop; the trailing docker compose down there already resets COMPOSE_FILE.)

  1. [blocking] bound the wait and re-run CI

docker compose up --wait has no upper timeout by default: if a service never reaches a terminal health state (starting that never resolves, or a container repeatedly restarting) the job blocks until the GitHub Actions limit instead of failing, which again loses the logs. Please add an explicit bound, e.g. --wait --wait-timeout 300.

On CI: all four e2e-storage jobs are red on this PR while the identical jobs are green on #7146 and #7153. Only e2e-postgres-compose produced a real error -

error mounting "/tmp/shenyu-e2e/postgres/schema/create-table.sql" to rootfs at
"/docker-entrypoint-initdb.d": ... not a directory: Are you trying to mount a directory
onto a file (or vice-versa)?

the other three were cancelled by fail-fast. That failure comes from k8s/script/storage/storage_init_postgres.sh (cp db/init/pg/create-table.sql ...), which this PR does not touch, so I am not attributing it to this diff. But since the postgres job died before ever reaching the --wait gate, the new gating logic is currently unverified on any of the four cases - please re-run and confirm all four are green before merge.

Non-blocking nits

  • k8s/script/healthcheck.sh is no longer invoked by the storage compose scripts (it is still used by the other modules, so keep it). Note it also looped over services-*.list, which nothing exercises for these cases any more.
  • PRGDIR=$(dirname "$curPath") is computed in every script and unused (pre-existing).
  • Fine detail: trap 'docker compose -f "$COMPOSE_FILE" down || true' EXIT in the four single-storage scripts means the stack is torn down even on success, before the explicit log dumps would run if the test passes - but the logs on success are informational anyway, so this is acceptable; just be aware the trap fires before those lines only on the set -e abort paths.

Fix items 1 and 2 and I will approve.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Task] — Storage compose scripts: no set -e, no trap, hardcoded sleeps

2 participants