Repository navigation
test(e2e): replace fixed timeouts with event-driven backstops - #2554
Conversation
Sweep across `hack/*.bats` and `hack/*.sh` replacing fragile `sleep N` / `timeout N until …; do sleep …; done` patterns with `kubectl wait --for=condition=…` plus a small existence backstop: until kubectl get <resource> >/dev/null 2>&1; do sleep 2; done kubectl wait <resource> --for=condition=Ready --timeout=… Existence backstop added to 33 `kubectl wait` sites across 12 bats files (redis, kafka, openbao, harbor, mongodb, qdrant, vminstance, clickhouse, mariadb, postgres, bucket, install). Without it, `kubectl wait` against a not-yet-created resource sits silently for the full timeout; with it, the test fails fast with a clear "resource never appeared" error. OpenAPI tests' fixed `sleep 5` after starting `kubectl proxy` is now a port-readiness wait. The 3 remaining genuine sleeps (LINSTOR node membership propagation, MetalLB end-to-end HTTP path, HR-count emission heuristic) are annotated with TODO + rationale so a future reader knows they are intentional. Same green-path semantics; failures now surface in seconds instead of after a 5-minute timeout. Signed-off-by: Myasnikov Daniil <[email protected]> Assisted-By: Claude <[email protected]>
github-actions
Bot
added
area/testing
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness and speed of the end-to-end test suite by replacing non-deterministic 'sleep' commands with event-driven wait patterns. By implementing existence backstops, the tests now provide immediate feedback when resources fail to materialize, significantly reducing the time spent waiting for timeouts during test failures. The changes cover a wide range of application tests and infrastructure setup scripts, ensuring more reliable CI/CD pipelines. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on Gemini (@gemini-code-assist) comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughReplace fixed sleeps in many e2e test scripts with explicit polling loops that wait for operator-created Kubernetes objects (HelmReleases, StatefulSets, Deployments, PVCs, Jobs, APIService, namespaces, BucketAccesses, etc.) to exist before running ChangesE2E readiness & existence polling
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
dosubot
Bot
added
kind/cleanup
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the robustness of E2E tests by replacing fixed sleep durations with dynamic retry loops that wait for Kubernetes resources to materialize. This change ensures that kubectl wait commands do not fail prematurely when resources are not yet present in the API. The review feedback suggests expanding these existence checks to include associated system HelmReleases in several test suites and replacing hardcoded resource names with variables to enhance consistency and maintainability.
| sleep 5 | ||
| # Wait for the operator to materialise the HelmRelease before kubectl wait | ||
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr ${name} >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
Since this test waits for both ${name} and ${name}-system HelmReleases, it is safer to include both in the existence backstop to prevent the second kubectl wait from failing immediately if the system release hasn't appeared yet.
timeout 60 sh -ec "until kubectl -n tenant-test get hr ${name} ${name}-system >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
The ${name}-system HelmRelease isn't created by the cozystack-controller — it's rendered as a kind: HelmRelease template inside the outer chart (see packages/extra/external-dns/templates/external-dns.yaml, which defines an HR named {{ .Release.Name }}-system). By the time kubectl wait hr ${name} --for=condition=ready returns, the outer chart has been fully applied, so the ${name}-system object already exists. No race here, leaving the backstop as-is.
| sleep 5 | ||
| # Wait for the operator to materialise the HelmRelease before kubectl wait | ||
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr ${name} >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
Similar to the previous test case, including both HelmReleases in the backstop ensures that the subsequent wait for the system release doesn't fail prematurely.
timeout 60 sh -ec "until kubectl -n tenant-test get hr ${name} ${name}-system >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
Same architecture — the second test case's ${name}-system HR is rendered by the outer chart at packages/extra/external-dns/templates/external-dns.yaml, so it's guaranteed to exist by the time the ${name} HR is Ready. Existing backstop is sufficient.
| sleep 5 | ||
| # Wait for the operator to materialise the HelmRelease before kubectl wait | ||
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr $release >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
The Harbor test also waits for a system HelmRelease later in the script. Adding it to the initial existence backstop provides better coverage for all resources being waited on.
timeout 60 sh -ec "until kubectl -n tenant-test get hr $release $release-system >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
harbor-${name}-system is rendered as a kind: HelmRelease template inside the outer chart at packages/apps/harbor/templates/harbor.yaml, so it's applied to the cluster as part of the outer HR's reconciliation. By the time line 44's kubectl wait hr $release --for=condition=ready returns, the -system HR already exists. The line-43 backstop is enough.
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr kafka-$name >/dev/null 2>&1; do sleep 2; done" | ||
| kubectl -n tenant-test wait hr kafka-$name --timeout=30s --for=condition=ready | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get kafkas test >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
It is better to use the $name variable here instead of the hardcoded string test for consistency with other tests and to avoid breakage if the test name is changed at the top of the file.
timeout 60 sh -ec "until kubectl -n tenant-test get kafkas $name >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
Fixed in 12ccc53 — switched the backstop to $name for consistency with every other backstop in this PR.
| sleep 5 | ||
| # Wait for the operator to materialise the HelmRelease before kubectl wait | ||
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr openbao-$name >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
Including the system HelmRelease in the existence backstop ensures that the subsequent wait for openbao-$name-system at line 25 is robust against slow resource materialization.
timeout 60 sh -ec "until kubectl -n tenant-test get hr openbao-$name openbao-$name-system >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
openbao-$name-system is rendered as a kind: HelmRelease template inside the outer chart at packages/apps/openbao/templates/openbao.yaml. Once the outer HR is Ready, the inner -system HR has already been applied — no race between the two waits, so the existing backstop is sufficient.
| sleep 5 | ||
| # Wait for the operator to materialise the HelmRelease before kubectl wait | ||
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name >/dev/null 2>&1; do sleep 2; done" |
There was a problem hiding this comment.
Adding the system HelmRelease to the backstop ensures that the wait at line 24 won't fail if the operator hasn't created the resource yet.
timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name qdrant-$name-system >/dev/null 2>&1; do sleep 2; done"
There was a problem hiding this comment.
qdrant-$name-system is rendered as a kind: HelmRelease template inside the outer chart at packages/apps/qdrant/templates/qdrant.yaml, so it's created as part of the outer HR's reconciliation. By the time line 23's --for=condition=ready returns, the -system HR already exists. No additional backstop needed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (8)
hack/e2e-apps/qdrant.bats (1)
29-29: ⚡ Quick winMove inline cleanup to a framework-level teardown.
kubectl deleteat the end of the test body is never reached when the test fails mid-run, leaving the Qdrant resource behind and potentially polluting subsequent runs. Based on learnings, cleanup in bats E2E tests underhack/e2e-apps/should be moved to a global/framework-provided teardown rather than inlined in the test body.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/qdrant.bats` at line 29, Remove the inline cleanup call "kubectl -n tenant-test delete qdrant.apps.cozystack.io $name" from the qdrant.bats test body and instead register the resource deletion in the framework-level/global teardown hook used by the E2E tests; ensure the teardown runs regardless of test failures and references the same $name variable (or capture the created resource name in a shared context) so the framework teardown performs the equivalent deletion for Qdrant resources created by the test.hack/e2e-apps/mongodb.bats (1)
41-43: ⚡ Quick winConsider moving inline cleanup to a framework-level teardown.
The
kubectl deleteat line 42 runs only if all previous steps succeed; a timeout or assertion failure earlier in the test leaves theMongoDBresource behind. The guard at line 5 (delete --ignore-not-found) covers recovery on the next run, but the leftover resource persists for the remainder of the current suite.Bats supports a
teardownfunction that runs after each test regardless of result — moving the delete there would make cleanup unconditional.+teardown() { + kubectl -n tenant-test delete mongodbs.apps.cozystack.io test --ignore-not-found --timeout=2m || true +} + `@test` "Create DB MongoDB" { name='test' kubectl -n tenant-test delete mongodbs.apps.cozystack.io $name --ignore-not-found --timeout=2m || true- # Cleanup - kubectl -n tenant-test delete mongodbs.apps.cozystack.io $name }Based on learnings: "replace inline cleanup (e.g., kubectl delete in the test body) with framework-level teardown/cleanup … Apply this pattern to all bats tests under hack/e2e-apps."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/mongodb.bats` around lines 41 - 43, Move the inline cleanup command "kubectl -n tenant-test delete mongodbs.apps.cozystack.io $name" out of the test body and into a framework-level teardown function so it runs unconditionally after each test; ensure the teardown can access the test's "name" variable (make it a global/exported variable or set it in setup) and call the same kubectl delete there (or use --ignore-not-found) to guarantee cleanup on timeouts/failures, and apply this teardown pattern to all other bats tests under hack/e2e-apps.hack/e2e-apps/foundationdb.bats (1)
119-123: ⚡ Quick winConsider moving cleanup to framework-level teardown.
The inline
kubectl delete+ wait loop at the end of the test body keeps cleanup tightly coupled to the test. If any assertion before line 120 fails, cleanup is silently skipped, leaving stale resources that can pollute subsequent runs.♻️ Sketch of framework-level teardown
+teardown() { + kubectl -n tenant-test delete foundationdb test --ignore-not-found || true + timeout 120 sh -ec "while kubectl -n tenant-test get foundationdbclusters.apps.foundationdb.org foundationdb-test 2>/dev/null; do sleep 10; done" || true +} + `@test` "Create DB FoundationDB" { name='test' ... - # Clean up - kubectl -n tenant-test delete foundationdb $name - - # Wait for cleanup to complete - timeout 120 sh -ec "while kubectl -n tenant-test get foundationdbclusters.apps.foundationdb.org foundationdb-$name 2>/dev/null; do sleep 10; done" }Based on learnings: "In E2E bats tests like hack/e2e-apps/harbor.bats, replace inline cleanup with framework-level teardown/cleanup … apply this pattern to all bats tests under hack/e2e-apps."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/foundationdb.bats` around lines 119 - 123, The test currently does inline cleanup with "kubectl -n tenant-test delete foundationdb $name" and a wait loop which will be skipped if the test fails; move this logic into the shared framework-level teardown so cleanup always runs. Remove the two lines from hack/e2e-apps/foundationdb.bats and instead register the created resource name (e.g., push $name into a shared array like CLEANUP_RESOURCES or call a helper register_cleanup in the test), then implement a teardown() in the common bats helper (or the framework harness) that iterates CLEANUP_RESOURCES and performs the kubectl -n tenant-test delete foundationdb <name> plus the same timeout/wait loop to ensure deletion; ensure the registration call is invoked where the resource is created so teardown has the correct $name even if the test fails.hack/e2e-apps/postgres.bats (1)
56-57: ⚡ Quick winMove inline cleanup to framework-level teardown.
Both
deletecalls are embedded in the test body without--ignore-not-found. If any earlier step times out, execution never reaches these lines and the resources linger in the namespace. The pre-existing cleanup on line 5 uses--ignore-not-found || true; the post-test deletes should follow the same defensiveness, and ideally be moved to a batsteardown(or equivalent framework hook) so they run regardless of test outcome.♻️ Minimal defensive fix (without restructuring)
- kubectl -n tenant-test delete postgreses.apps.cozystack.io $name - kubectl -n tenant-test delete job.batch/postgres-$name-init-job + kubectl -n tenant-test delete postgreses.apps.cozystack.io $name --ignore-not-found + kubectl -n tenant-test delete job.batch/postgres-$name-init-job --ignore-not-foundLonger-term, extract these into a bats
teardown()function so they execute even on test failure.Based on learnings: "replace inline cleanup (e.g., kubectl delete in the test body) with framework-level teardown/cleanup … move cleanup logic to a global or framework-provided teardown so the framework can clean namespaces between runs."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/postgres.bats` around lines 56 - 57, The two inline cleanup kubectl commands ("kubectl -n tenant-test delete postgreses.apps.cozystack.io $name" and "kubectl -n tenant-test delete job.batch/postgres-$name-init-job") should be moved out of the test body into a framework-level teardown (e.g., bats teardown()) so they always run even on failure, and made defensive by adding --ignore-not-found (or appending "|| true") to each delete call; update the test to remove these inline deletes and implement the teardown function that performs both deletes with --ignore-not-found to guarantee cleanup.hack/e2e-apps/vminstance.bats (1)
64-68: ⚡ Quick winConsider reordering: HR readiness check (line 68) should precede the VMI IP poll (line 67).
The current sequence is:
- Line 66 — backstop: HR exists
- Line 67 — poll: VMI has an IP address (20 s)
- Line 68 —
kubectl wait hr: HR condition=ready (5 s)Logically, HR readiness is a prerequisite for the VMI to even be created, so the readiness check at line 68 should come right after the existence backstop (line 66), before polling for the VMI IP. In practice this still passes (a running VMI implies the HR is already reconciled), but the ordering is misleading and the 5 s window at line 68 is backed only by that implication rather than an explicit guarantee.
♻️ Suggested reorder
timeout 60 sh -ec "until kubectl -n tenant-test get hr vm-instance-$name >/dev/null 2>&1; do sleep 2; done" + kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready timeout 20 sh -ec "until kubectl -n tenant-test get vmi vm-instance-$name -o jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]'; do sleep 5; done" - kubectl -n tenant-test wait hr vm-instance-$name --timeout=5s --for=condition=ready kubectl -n tenant-test wait vm vm-instance-$name --timeout=20s --for=condition=ready🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/vminstance.bats` around lines 64 - 68, The test orders the checks incorrectly: after ensuring the HelmRelease exists, move the readiness check (the kubectl wait hr --for=condition=ready invocation for hr vm-instance-$name) to run immediately after the existence backstop and before polling the VMI IP address; this guarantees the HR is reconciled before the VMI IP poll (the kubectl -n tenant-test get vmi ... jsonpath='{.status.interfaces[0].ipAddress}' | grep -q '[0-9]') runs.hack/e2e-apps/redis.bats (1)
31-31: ⚡ Quick winConsider moving inline cleanup to a
teardown()function.
clickhouse.bats,openbao.bats,kafka.bats,mariadb.bats, andredis.batsall rely on inlinekubectl deleteat the end of each test body for cleanup. If a test assertion fails mid-run, the cleanup is skipped, which can leave stale resources that pollute subsequent runs. Ateardown()hook runs unconditionally after every test.♻️ Example for redis.bats
+teardown() { + kubectl -n tenant-test delete redis.apps.cozystack.io --all --ignore-not-found 2>/dev/null || true +} + `@test` "Create Redis" { ... - kubectl -n tenant-test delete redis.apps.cozystack.io $name }Based on learnings: "In E2E bats tests like hack/e2e-apps/harbor.bats, replace inline cleanup (e.g., kubectl delete in the test body) with framework-level teardown/cleanup… Apply this pattern to all bats tests under hack/e2e-apps."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/redis.bats` at line 31, Replace the inline cleanup command "kubectl -n tenant-test delete redis.apps.cozystack.io $name" inside the redis.bats test body with a bats teardown() hook so cleanup always runs; add a teardown() function in hack/e2e-apps/redis.bats that unconditionally runs the same kubectl delete (using the same $name variable or a shared fixture variable) and remove the inline delete from the test body; apply the same pattern to clickhouse.bats, openbao.bats, kafka.bats, and mariadb.bats to ensure resources are deleted even when a test assertion fails.hack/e2e-apps/bucket.bats (1)
22-22: ⚡ Quick win
hr bucket-${name}wait also lacks an existence backstop.Line 22's
kubectl wait hr bucket-${name} --timeout=100sis the firstkubectl waitin the test and has no preceding polling loop. For consistency with the backstops added on lines 23–28, consider adding one here too.✏️ Suggested addition
+ timeout 60 sh -ec "until kubectl -n tenant-test get hr bucket-${name} >/dev/null 2>&1; do sleep 2; done" kubectl -n tenant-test wait hr bucket-${name} --timeout=100s --for=condition=ready🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/bucket.bats` at line 22, The `kubectl -n tenant-test wait hr bucket-${name} --timeout=100s --for=condition=ready` call needs an existence backstop like the other waits; add a short polling loop that retries `kubectl -n tenant-test get hr bucket-${name}` (or equivalent) with a small sleep and limited attempts until it returns success before invoking the `kubectl wait` for readiness, mirroring the backstop pattern used for the later waits in the test to avoid racing on first wait.hack/e2e-apps/harbor.bats (1)
54-70: ⚡ Quick win
$release-systemkubectl wait is missing an existence backstop.Every other
kubectl waitin this file now has a preceding polling backstop, but the$release-systemwait at line 54 does not. If the operator hasn't materialised thehr $release-systemobject yet, this will silently block for the full--timeout=600s(10 minutes) before producing a failure — exactly the problem the PR set out to fix.✏️ Suggested addition
+ timeout 60 sh -ec "until kubectl -n tenant-test get hr $release-system >/dev/null 2>&1; do sleep 2; done" kubectl -n tenant-test wait hr $release-system --timeout=600s --for=condition=ready || {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/harbor.bats` around lines 54 - 70, Add a short existence poll before the kubectl wait for hr $release-system: before the line calling "kubectl -n tenant-test wait hr $release-system --timeout=600s --for=condition=ready" add a loop that repeatedly runs "kubectl -n tenant-test get hr $release-system" (sleeping briefly between attempts) and fails after a short backstop (e.g., 30–60s) if the HelmRelease object never appears; this mirrors the other waits' pattern and ensures the script fails fast when the hr resource hasn't been created before invoking wait.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@hack/e2e-apps/kafka.bats`:
- Around line 46-47: The two commands added in hack/e2e-apps/kafka.bats
hard-code the Kafka resource name as "test"; change both occurrences to use the
existing name variable (use "$name") so they follow the PR convention and will
guard the correct resource; update the timeout loop command (until kubectl -n
tenant-test get kafkas ...) and the kubectl wait invocation (kubectl wait kafkas
-n tenant-test ...) to reference "$name" instead of the literal "test".
In `@hack/e2e-apps/qdrant.bats`:
- Line 24: Add the same existence backstop before calling kubectl wait on the
HelmRelease resource qdrant-$name-system: detect the presence of the resource
(e.g., poll kubectl -n tenant-test get hr qdrant-$name-system until it exists or
a short timeout elapses) and only then run kubectl -n tenant-test wait hr
qdrant-$name-system --for=condition=ready --timeout=120s; this mirrors the guard
used for qdrant-$name and prevents kubectl wait from hanging when the
HelmRelease does not exist.
In `@hack/e2e-install-cozystack.bats`:
- Around line 288-290: Add an existence backstop before the "kubectl wait
hr/tenant-test" call: poll for the HelmRelease resource (hr/tenant-test) to
exist (e.g. with a loop using "kubectl get hr/tenant-test >/dev/null 2>&1" and a
short sleep/retry) after the Tenant resource is applied, and only then run
"kubectl wait hr/tenant-test --for=condition=ready"; this ensures the kubectl
wait on hr/tenant-test doesn't fail immediately due to the resource not yet
being created.
---
Nitpick comments:
In `@hack/e2e-apps/bucket.bats`:
- Line 22: The `kubectl -n tenant-test wait hr bucket-${name} --timeout=100s
--for=condition=ready` call needs an existence backstop like the other waits;
add a short polling loop that retries `kubectl -n tenant-test get hr
bucket-${name}` (or equivalent) with a small sleep and limited attempts until it
returns success before invoking the `kubectl wait` for readiness, mirroring the
backstop pattern used for the later waits in the test to avoid racing on first
wait.
In `@hack/e2e-apps/foundationdb.bats`:
- Around line 119-123: The test currently does inline cleanup with "kubectl -n
tenant-test delete foundationdb $name" and a wait loop which will be skipped if
the test fails; move this logic into the shared framework-level teardown so
cleanup always runs. Remove the two lines from hack/e2e-apps/foundationdb.bats
and instead register the created resource name (e.g., push $name into a shared
array like CLEANUP_RESOURCES or call a helper register_cleanup in the test),
then implement a teardown() in the common bats helper (or the framework harness)
that iterates CLEANUP_RESOURCES and performs the kubectl -n tenant-test delete
foundationdb <name> plus the same timeout/wait loop to ensure deletion; ensure
the registration call is invoked where the resource is created so teardown has
the correct $name even if the test fails.
In `@hack/e2e-apps/harbor.bats`:
- Around line 54-70: Add a short existence poll before the kubectl wait for hr
$release-system: before the line calling "kubectl -n tenant-test wait hr
$release-system --timeout=600s --for=condition=ready" add a loop that repeatedly
runs "kubectl -n tenant-test get hr $release-system" (sleeping briefly between
attempts) and fails after a short backstop (e.g., 30–60s) if the HelmRelease
object never appears; this mirrors the other waits' pattern and ensures the
script fails fast when the hr resource hasn't been created before invoking wait.
In `@hack/e2e-apps/mongodb.bats`:
- Around line 41-43: Move the inline cleanup command "kubectl -n tenant-test
delete mongodbs.apps.cozystack.io $name" out of the test body and into a
framework-level teardown function so it runs unconditionally after each test;
ensure the teardown can access the test's "name" variable (make it a
global/exported variable or set it in setup) and call the same kubectl delete
there (or use --ignore-not-found) to guarantee cleanup on timeouts/failures, and
apply this teardown pattern to all other bats tests under hack/e2e-apps.
In `@hack/e2e-apps/postgres.bats`:
- Around line 56-57: The two inline cleanup kubectl commands ("kubectl -n
tenant-test delete postgreses.apps.cozystack.io $name" and "kubectl -n
tenant-test delete job.batch/postgres-$name-init-job") should be moved out of
the test body into a framework-level teardown (e.g., bats teardown()) so they
always run even on failure, and made defensive by adding --ignore-not-found (or
appending "|| true") to each delete call; update the test to remove these inline
deletes and implement the teardown function that performs both deletes with
--ignore-not-found to guarantee cleanup.
In `@hack/e2e-apps/qdrant.bats`:
- Line 29: Remove the inline cleanup call "kubectl -n tenant-test delete
qdrant.apps.cozystack.io $name" from the qdrant.bats test body and instead
register the resource deletion in the framework-level/global teardown hook used
by the E2E tests; ensure the teardown runs regardless of test failures and
references the same $name variable (or capture the created resource name in a
shared context) so the framework teardown performs the equivalent deletion for
Qdrant resources created by the test.
In `@hack/e2e-apps/redis.bats`:
- Line 31: Replace the inline cleanup command "kubectl -n tenant-test delete
redis.apps.cozystack.io $name" inside the redis.bats test body with a bats
teardown() hook so cleanup always runs; add a teardown() function in
hack/e2e-apps/redis.bats that unconditionally runs the same kubectl delete
(using the same $name variable or a shared fixture variable) and remove the
inline delete from the test body; apply the same pattern to clickhouse.bats,
openbao.bats, kafka.bats, and mariadb.bats to ensure resources are deleted even
when a test assertion fails.
In `@hack/e2e-apps/vminstance.bats`:
- Around line 64-68: The test orders the checks incorrectly: after ensuring the
HelmRelease exists, move the readiness check (the kubectl wait hr
--for=condition=ready invocation for hr vm-instance-$name) to run immediately
after the existence backstop and before polling the VMI IP address; this
guarantees the HR is reconciled before the VMI IP poll (the kubectl -n
tenant-test get vmi ... jsonpath='{.status.interfaces[0].ipAddress}' | grep -q
'[0-9]') runs.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 3abdaa9d-ac42-4e55-9faa-168c4dfab528
📒 Files selected for processing (16)
hack/e2e-apps/bucket.batshack/e2e-apps/clickhouse.batshack/e2e-apps/external-dns.batshack/e2e-apps/foundationdb.batshack/e2e-apps/harbor.batshack/e2e-apps/kafka.batshack/e2e-apps/mariadb.batshack/e2e-apps/mongodb.batshack/e2e-apps/openbao.batshack/e2e-apps/postgres.batshack/e2e-apps/qdrant.batshack/e2e-apps/redis.batshack/e2e-apps/run-kubernetes.shhack/e2e-apps/vminstance.batshack/e2e-install-cozystack.batshack/e2e-test-openapi.bats
| # kicks in (kubectl wait errors immediately if the object does not exist yet). | ||
| timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name >/dev/null 2>&1; do sleep 2; done" | ||
| kubectl -n tenant-test wait hr qdrant-$name --timeout=60s --for=condition=ready | ||
| kubectl -n tenant-test wait hr qdrant-$name-system --timeout=120s --for=condition=ready |
There was a problem hiding this comment.
Missing existence backstop for qdrant-$name-system HelmRelease.
Line 22 adds a polling guard before waiting on hr qdrant-$name, but line 24's kubectl wait hr qdrant-$name-system has no equivalent backstop. Per the PR's own stated rationale, kubectl wait against a non-existent resource blocks silently for the full timeout (120 s here) and then fails with an unhelpful error — the exact problem this PR is fixing everywhere else.
🛡️ Proposed fix
timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name >/dev/null 2>&1; do sleep 2; done"
kubectl -n tenant-test wait hr qdrant-$name --timeout=60s --for=condition=ready
+ timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name-system >/dev/null 2>&1; do sleep 2; done"
kubectl -n tenant-test wait hr qdrant-$name-system --timeout=120s --for=condition=ready📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| kubectl -n tenant-test wait hr qdrant-$name-system --timeout=120s --for=condition=ready | |
| timeout 60 sh -ec "until kubectl -n tenant-test get hr qdrant-$name-system >/dev/null 2>&1; do sleep 2; done" | |
| kubectl -n tenant-test wait hr qdrant-$name-system --timeout=120s --for=condition=ready |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/e2e-apps/qdrant.bats` at line 24, Add the same existence backstop before
calling kubectl wait on the HelmRelease resource qdrant-$name-system: detect the
presence of the resource (e.g., poll kubectl -n tenant-test get hr
qdrant-$name-system until it exists or a short timeout elapses) and only then
run kubectl -n tenant-test wait hr qdrant-$name-system --for=condition=ready
--timeout=120s; this mirrors the guard used for qdrant-$name and prevents
kubectl wait from hanging when the HelmRelease does not exist.
There was a problem hiding this comment.
qdrant-$name-system isn't created by the operator — it's a kind: HelmRelease template rendered inside the outer chart at packages/apps/qdrant/templates/qdrant.yaml. By the time line 23's kubectl wait hr qdrant-$name --for=condition=ready returns, the outer chart has been fully applied, so qdrant-$name-system already exists in the cluster. No race here.
There was a problem hiding this comment.
@myasnikovdaniil, thanks for the clear explanation — that makes sense. Since qdrant-$name-system is templated as a child HelmRelease inside the outer Qdrant chart (at packages/apps/qdrant/templates/qdrant.yaml), it is guaranteed to exist once the outer HelmRelease reaches ready, so the existence backstop is indeed unnecessary here. I'll retract this comment.
✏️ Learnings added
Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2554
File: hack/e2e-apps/qdrant.bats:24-24
Timestamp: 2026-05-05T08:35:27.054Z
Learning: In `hack/e2e-apps/qdrant.bats`, `hr qdrant-$name-system` does NOT need an existence backstop before `kubectl wait`. It is a `kind: HelmRelease` template rendered inside the outer Qdrant Helm chart (`packages/apps/qdrant/templates/qdrant.yaml`), not created asynchronously by the operator. By the time `kubectl wait hr qdrant-$name --for=condition=ready` completes, the outer chart is fully applied and `qdrant-$name-system` already exists in the cluster.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: lexfrei
Repo: cozystack/cozystack PR: 2058
File: hack/e2e-apps/harbor.bats:3-45
Timestamp: 2026-02-16T18:50:59.859Z
Learning: In E2E bats tests like hack/e2e-apps/harbor.bats, replace inline cleanup (e.g., kubectl delete in the test body) with framework-level teardown/cleanup. Do not rely on traps or per-test cleanup; instead, move cleanup logic to a global or framework-provided teardown so the framework can clean namespaces between runs. Apply this pattern to all bats tests under hack/e2e-apps (and similar E2E suites) to ensure consistent and reliable cleanup without embedding it in individual tests.
Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2559
File: hack/select-e2e_test.bats:75-80
Timestamp: 2026-05-02T14:46:54.942Z
Learning: In Bash Automated Testing System (Bats) tests, the `run` helper captures command output and strips the trailing newline from `$output`. Therefore, for single-line commands, exact string assertions like `[ "$output" = "redis" ]` should be safe and should not be tripped up by trailing newline/whitespace. No extra `xargs`-style trimming is needed solely to remove a trailing newline for single-line outputs.
- kafka.bats: use $name in the kafkas existence backstop, matching the variable used in the kubectl wait below it (consistency with the rest of this PR's backstops). - e2e-install-cozystack.bats: the Tenant→HelmRelease materialisation is asynchronous (cozystack-controller observes the Tenant CR and creates the HR), so guard hr/tenant-test with the same existence backstop pattern used elsewhere. Signed-off-by: Myasnikov Daniil <[email protected]>
dosubot
Bot
added
size:L
and removed
size/L
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. Solid replacement of fragile sleep N patterns with event-driven backstops across 14 sites. The shape — until kubectl get <resource> >/dev/null 2>&1; do sleep 2; done followed by kubectl wait --for=condition=… — is the canonical fix for the "kubectl wait against not-yet-created object sits silent for the full timeout" failure mode, and the inline rationale on each call site documents the why.
What's solid:
- E2E Tests is green on this HEAD — load-bearing verification that the replacements actually work in a running cluster, not just in static analysis. Replacing 14 sleep sites without breaking E2E is not trivial; the fact that CI passes is the empirical signal that the existence-backstops cover the right resources.
- Genuine sleeps preserved with
# TODO(e2e-replace-fixed-timeouts): genuine sleep …annotations explaining why no API signal would suffice (HR-count emission heuristic, LINSTOR node membership propagation, MetalLB end-to-end HTTP path). Future readers see they are intentional rather than missed during the sweep. kafka.batsfix in12ccc53d9parametrizes the existence-backstop —kubectl get kafkas test→kubectl get kafkas $name— so the backstop actually matches the resource name the test case creates.e2e-test-openapi.batsfixes thetrap "kill $!"scoping bug while at it:kubectl proxy & proxy_pid=$!; trap "kill $proxy_pid" EXITevaluates$!at trap-set time so it captures the right PID, plusnc -z localhost 21234polling replaces the fixedsleep 0.5with a real readiness signal.- Backwards-compatible: tests that pass today on the old sleep timing pass on the new event-driven path; failure mode is now seconds-to-fail with a clear "resource never appeared" error instead of 5-minute silent timeouts.
Three nit-level observations, none blocking:
- Pre-existing kafka.bats inconsistency:
kubectl wait kafkas -n tenant-test test --timeout=300s --for=condition=ready(line 47) still hardcodes the resource nametestinstead of$name. The existence-backstop one line above was correctly parametrized in12ccc53d9, but thewaititself wasn't. Not introduced by this PR — but12ccc53d9is touching the surrounding lines, so reasonably in scope to fix here as a follow-up commit. - Comment duplication: the same 2-line shebang ("Wait for the operator to materialise the HelmRelease … kubectl wait errors immediately if the object does not exist yet") appears verbatim in 14 sites. A sourced helper
wait_for_resource <ns> <kind> <name> [poll_interval]would DRY this up considerably, but that's a separate refactor. - Inconsistent poll cadence in the backstops: most use
sleep 2, kafka PVC usessleep 10, external-dns usessleep 5. Cosmetic —sleep 2would work everywhere; thesleep 10cases predate this PR. Not worth a fix-up commit.
…2554) Address lexfrei's nit on PR #2554: the pre-existing `kubectl wait kafkas -n tenant-test test ...` on line 47 hardcoded the resource name, while the existence backstop one line above was already parametrized with `$name` in 12ccc53. Switch the wait to `$name` for consistency so changing the test's resource name in one place keeps the two lines aligned. Signed-off-by: Myasnikov Daniil <[email protected]>
github-actions
Bot
added
size/L
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. Sweep is correctly scoped and the pattern is right.
The until kubectl get … >/dev/null; do sleep 2; done + kubectl wait --for=condition=… shape is the right way to cooperate with informers on resources that the operator materializes asynchronously. Without the existence backstop, kubectl wait sits silently against a NotFound for the full timeout and surfaces a useless "timed out waiting for the condition" — diagnosis-time impact is real.
14 sites covered across the per-app bats files plus the install/openapi/run-kubernetes harness — exactly the surface that flakes on cold runs. Genuine sleeps that survived (LINSTOR node membership propagation, MetalLB end-to-end HTTP, HR-count emission heuristic) are TODO-annotated with rationale, which is the right call for sleeps that aren't race-prone polling.
Vm-disk hunk in vminstance.bats co-landing here, vm-instance hunks in #2555 — clean split.
Same green-path semantics throughout, failure surface drops from 5-minute opaque timeouts to seconds with a clear "resource never appeared" error.
Ship it.
3 participants
What this PR does
Replaces fragile
sleep N/timeout N until …; do sleep …; donepatterns inhack/*.batsandhack/*.shwithkubectl wait --for=condition=…plus a small existence backstop:Why the existence backstop matters
kubectl waitagainst a not-yet-created resource sits silently for the full timeout and then fails witherror: timed out waiting for the condition— useless for diagnosis. With the backstop, the test fails fast with a clear "resource never appeared" error.Existence backstop added at 14 sites in this sweep across:
redis,kafka,openbao,harbor,mongodb,qdrant,clickhouse,mariadb,postgres,bucket,external-dns,foundationdbbatshack/e2e-install-cozystack.batshack/e2e-apps/run-kubernetes.shhack/e2e-test-openapi.bats(port-readiness wait replacing fixedsleep 5afterkubectl proxy)The vm-disk hunk in
hack/e2e-apps/vminstance.batsis included here; the vm-instance hunks land separately via thedaniil/split-vminstancePR.Genuine sleeps annotated
The 3 remaining genuine sleeps (LINSTOR node membership propagation, MetalLB end-to-end HTTP path, HR-count emission heuristic) are annotated with
# TODO+ rationale so a future reader knows they are intentional.Same green-path semantics; failures now surface in seconds instead of after a 5-minute timeout. Surfaced from #2500.
Release note
Summary by CodeRabbit
Tests
Documentation