mirror of
https://github.com/cozystack/cozystack.git
synced 2026-11-04 02:57:27 +09:00
myasnikovdaniil
5d420475f4
## What this PR does
The E2E job's `if: always()` cleanup ran `rm -rf /tmp/$SANDBOX_NAME`
with no check that `SANDBOX_NAME` was ever set. `Set sandbox ID` carries
no condition, so it inherits the implicit `success()` every
unconditional step gets: any failure ahead of it — a conflicting
`pr.patch`, a `download-artifact` miss, `Checkout code`, the
release-path `curl` — or a cancellation that reaches the job after it
has started (the job condition is `!cancelled()`) skips it, and
`SANDBOX_NAME` is never written to `$GITHUB_ENV`. The `if: always()`
steps then run anyway and interpolate the empty value; `Remove
workspace` is the destructive one, expanding to `rm -rf /tmp/` and
failing the cleanup step against root-owned entries in the runner's
`/tmp`. This is not a "re-run skips the setup" bug: a re-run restarts
the job at step one and re-runs the unconditional `Set sandbox ID` — the
trigger is incomplete initialization, by any of the routes above.
The `/tmp/$SANDBOX_NAME` workspace is a vestige of the retired multi-job
self-hosted-runner design, where several `runs-on: [self-hosted]` jobs
shared no state and rendezvoused through a `sha256(REPO:WORKFLOW:REF)`
key, with a per-run `cp -r` of the checkout into `/tmp`. Since CI moved
onto the ephemeral pool (`runs-on: oracle-vm-32cpu-128gb-x86-64`) it is
a single job: there is no cross-job rendezvous and the checkout is never
reused across runs, so copying it into `/tmp` and removing it afterwards
is dead weight. (An earlier review justified the change by "wiping the
entire `/tmp` of the self-hosted runner" — that describes the topology
we retired; the hazard is real, but not for that reason.)
So this drops the workspace rather than guarding the `rm`. Both the PR
(`pull-requests.yaml`) and nightly (`nightly.yaml`) E2E jobs now run in
`$GITHUB_WORKSPACE`: the `Prepare workspace` and `Remove workspace`
steps are gone, the `cd /tmp/$SANDBOX_NAME` prefixes are dropped, and
the upload paths point at `_out/…` instead of
`/tmp/${SANDBOX_NAME}/_out/…`. That takes the empty-value expansion out
of the `cd`s, the stray `mkdir`, the upload paths and the destructive
`rm` in one move, across both workflows rather than one step of many.
`Set sandbox ID` stays: the job still reads `$SANDBOX_NAME` directly
(failure diagnostics, the chainsaw `docker cp`, teardown, and every
`make SANDBOX_NAME=…` argument) — the container needs a name, just not a
directory.
It also removes the dead code the single-runner retirement left in
`packages/core/testing/Makefile`: the world-writable `mkdir -p
/tmp/${SANDBOX_NAME}` + `chmod 777` in the `apply` target (they backed a
`-v /tmp/${SANDBOX_NAME}:/workspace/hosttmp` bind mount that was later
removed — nothing under `images/e2e-sandbox/` references
`/workspace/hosttmp`), and the `-e SANDBOX_NAME=${SANDBOX_NAME}` passed
into the container (nothing under `images/e2e-sandbox/` reads it). Every
other use of `SANDBOX_NAME` — the container name and the `make`
arguments — is kept.
Note this cannot make a job with an empty `SANDBOX_NAME` go green: such
a job already failed initialization or was cancelled, so its conclusion
is fixed before cleanup runs. It removes a second, destructive failure
the old cleanup piled on top, and it does not change whether re-runs
work in general — they do.
The diagnosis of the real trigger, the corrected blast radius, and the
scope across both workflows plus the Makefile are credited to the
maintainer review on this PR.
### Evidence
Run [30067779342 attempt
3](https://github.com/cozystack/cozystack/actions/runs/30067779342/attempts/3)
exhibits the mechanism: `Download Talos image (regular PR)` failed on
`GetSignedArtifactURL` with a 404, which skipped `Set sandbox ID`, and
the `always()` steps then ran on an empty value — `Collect chainsaw
report` created `/tmp/_out`, the uploads resolved to `/tmp//_out/…`, and
`Remove workspace` did:
```
rm: cannot remove '/tmp/systemd-private-…-systemd-timedated.service-1oADZP': Operation not permitted
rm: cannot remove '/tmp/.X11-unix': Operation not permitted
##[error]Process completed with exit code 1.
```
The step ran unprivileged in `/home/ubuntu/_work/…`, so all it could hit
was `Operation not permitted` on root-owned entries in the sticky `/tmp`
— it deleted nothing. That run is a later, independent occurrence (it
started well after this PR was opened), not this change's motivating
case. The 404 that triggered it is a separate, non-deterministic problem
— the artifact listing succeeded and only the signed-URL call failed
~190 ms in, `download-artifact` treats 404 as non-retryable, and attempt
2 of the same run fetched the artifact fine — and it deserves its own
issue rather than being folded in here.
### Screenshots
Not applicable — CI workflow change only.
### Downstream repositories
The change is confined to this repo's E2E CI
(`.github/workflows/pull-requests.yaml`,
`.github/workflows/nightly.yaml`) and its testing Makefile
(`packages/core/testing/Makefile`). No downstream repository consumes
it.
- [x] No downstream repository is affected by this change
### Release note
```release-note
refactor(ci): drop the vestigial /tmp/$SANDBOX_NAME workspace from the E2E PR and nightly jobs — they now run in $GITHUB_WORKSPACE, removing an unguarded `rm -rf /tmp/$SANDBOX_NAME` cleanup that failed the job on cleanup when SANDBOX_NAME was empty
```
<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->
## Summary by CodeRabbit
* **Bug Fixes**
* Improved end-to-end test environment setup and sandbox cleanup.
* Updated test reporting and artifact collection for more reliable
results.
* Ensured OpenAPI and full-suite tests run from the correct prepared
environment.
* **Chores**
* Simplified container and workflow configuration for sandbox-based
testing.
* Clarified checkout token handling in nightly workflows.
<!-- end of auto-generated comment: release notes by coderabbit.ai -->