Repository navigation
test(e2e): wait for the S3 gateway to accept a key before using it - #4694
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 8 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughBoth bucket Chainsaw scripts retry S3 alias setup when the gateway reports that an access key does not exist. The scripts increase their step timeouts to eight minutes and update their command-budget calculations. ChangesBucket S3 key readiness
Priority: ⬇️ Low Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Low Merge Risk: ⚪ Minimal · up to The bucket checks can wait for delayed gateway key acceptance without an identified merge-blocking issue. Normal checks remain appropriate. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 1 system. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
BucketAccess turns accessGranted as soon as the COSI driver has written the new key into the filer's IAM file. The S3 gateway loads that file later, from a filer metadata subscription, and with two filer replicas the change can reach the filer a gateway follows only after the other replica passes it on. Both bucket suites call mc alias set right after the assert, so a key the gateway has not loaded yet is rejected with InvalidAccessKeyId and the suite goes red. Probe the key through the same port-forward the writes use. Only the InvalidAccessKeyId text is waited on, for at most 60s; any other error or a timed-out attempt fails at once, and the window expiring fails with a message naming the BucketAccess. The two steps get an 8m op ceiling so the inner budgets still sit under it. Signed-off-by: Aleksei Sviridkin <[email protected]> Assisted-by: LLM
b4bcff2 to
b306abd
Compare
IvanHunters
left a comment
There was a problem hiding this comment.
LGTM
Fixes the #4580 race at the root. It waits for the S3 gateway to accept the key, bounded, rather than sleeping a fixed amount, and without needing the driver-side change the test can't make.
What I checked:
- The wait gates on the key actually being accepted. await_key returns success only when
mc alias setexits 0, so there's no other path to green and no false-green branch.mc alias setreally does contact the gateway (the two CI logs in the issue show it returning thedoes not exist in our recordserror), so a success means this gateway took the key. - The ceiling is bounded: a 60s deadline off date +%s, every attempt under
timeout 15,sleep 2between tries, ~77s worst case per call. Both step budgets recompute correctly and sit under the 8m ceiling, 409s for verify-s3-access and 441s for verify-every-bucket-takes-a-write. - It reddens on a real failure. await_key runs as a plain command under
set -eu, so the deadlinereturn 1fails the script, and chainsaw's script op fails on a non-zero exit by default (step 2 writes it out as($error == null): true, step 1 leans on the default). Pull the wait and the test drops back to the intermittent red this PR is fixing. - No inert negated assertions in here: it's a chainsaw inline /bin/sh, not a .bats under the cozytest runner. The single
! grep -qsits in anifthat leads toreturn 1, and an empty alias.log degrades to a fast fail, which is the safe direction.
Non-blocking:
- The retryable-error match keys on SeaweedFS's text
does not exist in our records. If that wording ever changes, await_key degrades to a fast fail and the flake comes back, but never to a false green. mc gives the shell no structured error code, so the text is the only signal there is; a deliberate, narrow choice. - await_key is copied verbatim into both script steps. The two copies are byte-identical today, and a single chainsaw file has no way to share a shell function across two isolated script ops, so there's no avoiding it here. The only risk is future drift.
- Cosmetic: the budget comment around the "= at most 409s. mc and kubectl both exit on SIGTERM" line has an awkward mid-sentence wrap left over from an edit.
What this PR does
The bucket e2e suite fails now and then because the S3 gateway rejects a key that COSI already reports as granted. The error is
The access key ID you provided does not exist in our records.frommc alias set. In job 109130131391 the BucketAccess assert finished at 21:23:43.57 and the firstmc alias setfailed at 21:23:44.82. In job 109396174094 cap1 and cap2 were accepted, and cap3 was rejected two seconds into the step.Granted access is not the same as a usable key. In seaweedfs-cosi-driver v0.3.1,
DriverGrantBucketAccesswrites the new key into/etc/iam/identity.jsonon the filer and returns. The S3 gateway in SeaweedFS 4.45 reloads that file later, from a filer metadata subscription (weed/s3api/auth_credentials_subscribe.go). In our chart the driver writes throughseaweedfs-filerand the two gateways followseaweedfs-filer-client, so the change may have to pass through the other filer replica first.The suite now waits until the gateway accepts the key before it writes. Every alias in both write steps goes through
await_key, which uses the same port-forward as the writes and so asks the same gateway pod. It waits only while the answer is the InvalidAccessKeyId text, for at most 60s. Any other error fails at once, and so does an attempt cut off at its 15s cap. When the window runs out, the step fails with a message that names the BucketAccess.One probe costs at most 77s, so both steps go from a 5m to an 8m op timeout. The inner budgets now sum to 409s and 441s, and the budget comments say so.
I checked the probe locally against a fake
mc:A hanging
mcfails on its first attempt after 15s with exit 124.chainsaw lint testfrom v0.2.15 accepts the file.The read-only upload check gets fixed along the way. Before, an unloaded
ro-userkey also made the upload fail, and that failure passed as the expected denial.One thing I left as it is:
await_keyis copied into both steps, because chainsaw steps share no shell, and moving it to_lib/would run every suite on each edit.Closes #4580
Screenshots
Not applicable, no UI change.
Downstream repositories
Release note