Repository navigation
[tests] Add BATS E2E coverage for vm-disk source.disk and source.image - #2403
myasnikovdaniil wants to merge 3 commits into
Conversation
dosubot
Bot
added
the
size/M
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 enhances the testing infrastructure by introducing automated end-to-end tests for VMDisk source types. By validating the cloning paths for both images and existing disks, the changes ensure greater reliability for disk provisioning workflows within the system. 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
|
📝 WalkthroughWalkthroughAdds a new Bats end-to-end test file for the VMDisk CRD that includes a global teardown and two tests: one creating a VMDisk from Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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
the
kind/feature
![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 adds a new E2E test suite for VMDisk resources, covering both golden image and PVC-based cloning. The reviewer suggests several improvements to enhance test stability and performance, including increasing timeouts for HelmRelease readiness, ensuring cleanup commands are idempotent with the --ignore-not-found flag, and using local images for faster cloning. Additionally, the PR needs to be updated to follow the repository's commit message prefix conventions as specified in the style guide.
| @@ -0,0 +1,81 @@ | |||
| #!/usr/bin/env bats | |||
There was a problem hiding this comment.
The PR title (and commit messages) should follow the required prefix format. Based on the Repository Style Guide, the prefix for this component should be [tests] instead of tests:.
References
- Each commit must start with a component prefix in the format [component]. Valid meta prefixes include [tests]. (link)
| storageClass: replicated | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready |
There was a problem hiding this comment.
A 5-second timeout for a HelmRelease to reach the Ready state might be too short and lead to flaky tests in CI environments under load. Consider increasing this timeout to at least 60 seconds.
kubectl -n tenant-test wait hr vm-disk-$name --timeout=60s --for=condition=ready
There was a problem hiding this comment.
Following the established pattern in vminstance.bats (lines 22 and 64), which uses --timeout=5s for wait hr on the same vm-disk HelmRelease. The HelmRelease becomes ready almost immediately once the cozystack controller renders it; the long waits are on dv (250s) and pvc (200s), which is where the actual import work happens.
There was a problem hiding this comment.
Updated in b291fa6 — HelmRelease wait timeout is now 30s, which matches the pattern used in kafka.bats and absorbs controller scheduling jitter while keeping fast-fail behavior. The substantive waits remain on dv (250s) and pvc (200s).
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready | ||
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=250s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=200s --for=jsonpath='{.status.phase}'=Bound | ||
| kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name |
There was a problem hiding this comment.
Adding --ignore-not-found to the cleanup command ensures the test doesn't fail if the resource was already removed or never successfully created.
kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name --ignore-not-found
There was a problem hiding this comment.
The convention across hack/e2e-apps/*.bats is to use --ignore-not-found on idempotent setup deletes at the start of tests, but to leave the final cleanup as a bare kubectl delete. If a prior step fails, BATS exits and this cleanup line never runs anyway.
There was a problem hiding this comment.
Updated in b291fa6 — per-test cleanup was moved to a teardown() block that uses --ignore-not-found, so cleanup runs reliably even when a prior assertion fails.
| http: | ||
| url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img |
There was a problem hiding this comment.
Using the alpine-3.21 image (via source.image) for the base disk would be significantly faster than downloading the Ubuntu image via HTTP, as it leverages the local golden image collection. This would improve test efficiency and reliability.
image:
name: alpine-3.21
There was a problem hiding this comment.
The speed concern was addressed in 521f8a4 by switching the base from Ubuntu noble (~600MB) to Alpine (~50MB). Keeping source.http here on purpose: test 1 above already covers source.image directly, so using HTTP for the base disk in test 2 gives distinct coverage and avoids creating a clone-of-clone scenario.
| storageClass: replicated | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr vm-disk-$base --timeout=5s --for=condition=ready |
There was a problem hiding this comment.
Consider increasing the timeout for the HelmRelease wait to improve test robustness.
kubectl -n tenant-test wait hr vm-disk-$base --timeout=60s --for=condition=ready
There was a problem hiding this comment.
Same as the earlier timeout thread — vminstance.bats uses --timeout=5s on the same vm-disk HelmRelease. The HR is ready quickly; the substantive waits on dv (250s) and pvc (200s) cover the actual import.
There was a problem hiding this comment.
Updated in b291fa6 — HelmRelease wait timeout is now 30s, matching the pattern in kafka.bats.
| storageClass: replicated | ||
| EOF | ||
| sleep 5 | ||
| kubectl -n tenant-test wait hr vm-disk-$clone --timeout=5s --for=condition=ready |
There was a problem hiding this comment.
Consider increasing the timeout for the HelmRelease wait to improve test robustness.
kubectl -n tenant-test wait hr vm-disk-$clone --timeout=60s --for=condition=ready
There was a problem hiding this comment.
Same rationale — 5s matches the pattern in vminstance.bats for vm-disk HelmRelease readiness, with the substantive waits on dv (250s) and pvc (200s).
There was a problem hiding this comment.
Updated in b291fa6 — HelmRelease wait timeout is now 30s, matching the pattern in kafka.bats.
| kubectl -n tenant-test delete vmdisks.apps.cozystack.io $clone | ||
| kubectl -n tenant-test delete vmdisks.apps.cozystack.io $base |
There was a problem hiding this comment.
Adding --ignore-not-found to the cleanup commands ensures the test doesn't fail if the resources were already removed.
kubectl -n tenant-test delete vmdisks.apps.cozystack.io $clone --ignore-not-found
kubectl -n tenant-test delete vmdisks.apps.cozystack.io $base --ignore-not-found
There was a problem hiding this comment.
Following the repo convention — --ignore-not-found is used on setup deletes (lines 35-36) but not on final cleanup. If a prior step fails, BATS exits before reaching this line.
There was a problem hiding this comment.
Updated in b291fa6 — the per-test cleanup block was replaced with a teardown() that uses --ignore-not-found and covers both $base and $clone.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
The test file (hack/e2e-apps/vm-disk.bats) looks good and follows the existing patterns from vminstance.bats and kafka.bats. A few things to address.
Must fix
1. Cleanup is not idempotent
At the end of each test, cleanup uses bare kubectl delete without --ignore-not-found:
kubectl -n tenant-test delete vmdisks.apps.cozystack.io $nameIf the test fails before creating the resource (or the resource was already cleaned up), this delete will fail and mask the real error. The pre-test cleanup already uses --ignore-not-found correctly — the post-test cleanup should too, consistent with the pattern in kafka.bats.
2. HelmRelease wait timeout is 5 seconds
kubectl -n tenant-test wait hr vm-disk-$name --timeout=5s --for=condition=ready5 seconds is very tight for a HelmRelease to reach Ready in CI (QEMU VMs, shared resources). The existing kafka.bats uses --timeout=30s for the same wait. This will likely cause flaky failures. Increase to at least 30s.
Gemini flagged this as well — see their inline comments.
Should fix
3. source.disk test downloads from the internet
The source.disk test creates its base disk via source.http downloading an Alpine image from dl-cdn.alpinelinux.org. Since the source.image test already proves golden images work, the base disk for the source.disk test could also use source.image to avoid external network dependency and speed up the test. The commit message says "Use Alpine for source.disk base instead of Ubuntu noble" — but it still uses HTTP download, not the local golden image.
4. Consider a teardown function
Both tests have similar cleanup patterns. BATS supports teardown functions that run after each test regardless of pass/fail. This would make cleanup more reliable than inline deletes at the end of the test body.
General note
Please don't leave bot review comments (CodeRabbit, Gemini) unaddressed — either fix the issues they raised or reply explaining why they don't apply. Several of Gemini's inline comments (timeout, --ignore-not-found, using golden image for base disk) are valid and overlap with this review.
myasnikovdaniil
changed the title
|
Addressed all four points in b291fa6:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
hack/e2e-apps/vm-disk.bats (4)
38-39: Pre-test cleanup might be redundant.Similar to line 12, these delete commands ensure a clean state before the test but may be redundant given that
teardown()runs after each test and the E2E framework cleans up namespaces between runs. Based on learnings, framework-level cleanup should handle this.Consider removing if the framework guarantees clean state:
- # Ensure both resources are absent before starting - kubectl -n tenant-test delete vmdisks.apps.cozystack.io $clone --ignore-not-found --timeout=2m || true - kubectl -n tenant-test delete vmdisks.apps.cozystack.io $base --ignore-not-found --timeout=2m || true - # Create the base disk from a golden image. The assertion here is the clone🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/vm-disk.bats` around lines 38 - 39, Remove the redundant pre-test cleanup delete commands that call "kubectl -n tenant-test delete vmdisks.apps.cozystack.io $clone" and "$base": rely on the existing teardown() and the E2E framework namespace cleanup instead; delete those two kubectl delete lines (the ones referencing $clone and $base) from hack/e2e-apps/vm-disk.bats so the test setup is leaner and not duplicating teardown behavior.
27-27: Consider removing the fixed sleep delay.Modern
kubectl waithandles resources that don't exist yet and will wait for them to be created. Thesleep 5might be unnecessary unless you've observed timing issues where the wait command fails before the resource is created.If the wait commands work reliably without the sleep, consider removing it:
kubectl apply -f - <<EOF ... EOF - sleep 5 kubectl -n tenant-test wait hr vm-disk-$name --timeout=30s --for=condition=readyIf this sleep is necessary due to API server propagation delays or controller initialization, a brief comment explaining why would be helpful.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/vm-disk.bats` at line 27, Remove the fixed "sleep 5" pause in hack/e2e-apps/vm-disk.bats and rely on the subsequent kubectl wait commands to handle resource readiness; if you observed intermittent failures when wait runs before resources exist, either remove the sleep and update the wait invocations to include appropriate conditions/timeouts (e.g., kubectl wait --for=condition=Ready --timeout=60s) or keep the short pause but add a comment above the "sleep 5" explaining the observed timing issue (API server propagation/controller init) and why the hard sleep is required.
12-12: Pre-test cleanup might be redundant.The
kubectl deleteon line 12 ensures a clean state before the test, but sinceteardown()runs after each test and the E2E framework cleans up namespaces between runs, this inline cleanup may be redundant. Based on learnings, the framework-level cleanup should handle this. However, this could be defensive programming to handle interrupted test runs where teardown didn't execute.Consider removing the pre-test cleanup if the framework guarantees a clean namespace:
- kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name --ignore-not-found --timeout=2m || true kubectl apply -f - <<EOFIf you've observed cases where resources persist between test runs, the defensive cleanup is reasonable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/vm-disk.bats` at line 12, The pre-test explicit deletion using the kubectl command (kubectl -n tenant-test delete vmdisks.apps.cozystack.io $name --ignore-not-found --timeout=2m) is likely redundant because teardown() and the E2E framework already clean namespaces; remove that inline cleanup line from hack/e2e-apps/vm-disk.bats to rely on teardown()/framework cleanup, unless you've observed persistent resources after interrupted runs—if so, keep it as defensive cleanup and add a short comment explaining why.
57-57: Consider removing fixed sleep delays.Same as line 27, these
sleep 5commands might be unnecessary ifkubectl waitproperly handles resources that don't exist yet.Also applies to: 77-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/vm-disk.bats` at line 57, The script uses fixed sleep delays (sleep 5 at lines referenced) which are brittle; replace these sleeps in hack/e2e-apps/vm-disk.bats with an active wait using kubectl wait (e.g., kubectl wait --for=condition=ready or --for=condition=available with a suitable --timeout) for the specific resource(s) you're creating, or implement a short retry loop that polls kubectl get until the resource exists/ready; update the occurrences of the sleep command (and the similar one noted at line 27 and 77) to use kubectl wait against the specific pod/deployment/VM resource names instead of sleeping.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@hack/e2e-apps/vm-disk.bats`:
- Around line 38-39: Remove the redundant pre-test cleanup delete commands that
call "kubectl -n tenant-test delete vmdisks.apps.cozystack.io $clone" and
"$base": rely on the existing teardown() and the E2E framework namespace cleanup
instead; delete those two kubectl delete lines (the ones referencing $clone and
$base) from hack/e2e-apps/vm-disk.bats so the test setup is leaner and not
duplicating teardown behavior.
- Line 27: Remove the fixed "sleep 5" pause in hack/e2e-apps/vm-disk.bats and
rely on the subsequent kubectl wait commands to handle resource readiness; if
you observed intermittent failures when wait runs before resources exist, either
remove the sleep and update the wait invocations to include appropriate
conditions/timeouts (e.g., kubectl wait --for=condition=Ready --timeout=60s) or
keep the short pause but add a comment above the "sleep 5" explaining the
observed timing issue (API server propagation/controller init) and why the hard
sleep is required.
- Line 12: The pre-test explicit deletion using the kubectl command (kubectl -n
tenant-test delete vmdisks.apps.cozystack.io $name --ignore-not-found
--timeout=2m) is likely redundant because teardown() and the E2E framework
already clean namespaces; remove that inline cleanup line from
hack/e2e-apps/vm-disk.bats to rely on teardown()/framework cleanup, unless
you've observed persistent resources after interrupted runs—if so, keep it as
defensive cleanup and add a short comment explaining why.
- Line 57: The script uses fixed sleep delays (sleep 5 at lines referenced)
which are brittle; replace these sleeps in hack/e2e-apps/vm-disk.bats with an
active wait using kubectl wait (e.g., kubectl wait --for=condition=ready or
--for=condition=available with a suitable --timeout) for the specific
resource(s) you're creating, or implement a short retry loop that polls kubectl
get until the resource exists/ready; update the occurrences of the sleep command
(and the similar one noted at line 27 and 77) to use kubectl wait against the
specific pod/deployment/VM resource names instead of sleeping.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: be2734dd-1ec9-4590-bcdc-471c88cec5ac
📒 Files selected for processing (1)
hack/e2e-apps/vm-disk.bats
Follow-up to PR #2258: adds two new @test cases in hack/e2e-apps/vm-disk.bats covering the new vm-disk clone paths — source.image (golden image from cozy-public) and source.disk (PVC clone within tenant namespace). Existing source.http coverage in vminstance.bats is not touched. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
The clone step is what this test exercises, not the HTTP import. Ubuntu noble is ~600MB; Alpine is ~50MB, which cuts CI time without losing coverage. Co-Authored-By: Claude Opus 4.6 <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
- Add teardown() function for reliable cleanup after each test - Increase HelmRelease wait timeout from 5s to 30s - Switch base disk in source.disk test from source.http to source.image - Remove inline cleanup in favor of teardown Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
b291fa6 to
25537ca
Compare
github-actions
Bot
added
the
area/testing
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/vm-disk.bats`:
- Around line 27-30: Replace the fixed "sleep 5" with a short retry loop that
polls for the existence of the resources before calling kubectl wait: repeatedly
run "kubectl -n tenant-test get hr vm-disk-$name" (and similarly for dv and pvc)
with a small sleep and a total timeout, and only then invoke kubectl -n
tenant-test wait hr vm-disk-$name --for=condition=ready, kubectl -n tenant-test
wait dv vm-disk-$name --for=condition=ready and kubectl -n tenant-test wait pvc
vm-disk-$name --for=jsonpath='{.status.phase}'=Bound; apply the same replacement
for the second occurrence covering lines 57-80 so the script first ensures
resources exist before waiting for readiness.
🪄 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: 14ccb0c2-60f5-416e-a805-c39984206785
📒 Files selected for processing (1)
hack/e2e-apps/vm-disk.bats
| sleep 5 | ||
| kubectl -n tenant-test wait hr vm-disk-$name --timeout=30s --for=condition=ready | ||
| kubectl -n tenant-test wait dv vm-disk-$name --timeout=250s --for=condition=ready | ||
| kubectl -n tenant-test wait pvc vm-disk-$name --timeout=200s --for=jsonpath='{.status.phase}'=Bound |
There was a problem hiding this comment.
Replace the fixed sleep 5 with a readiness poll.
The hard-coded delay can still race on slower clusters: kubectl wait will fail if the HelmRelease/DataVolume/PVC objects have not been created yet. A retry loop or explicit existence check would make both tests much less flaky.
Also applies to: 57-80
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@hack/e2e-apps/vm-disk.bats` around lines 27 - 30, Replace the fixed "sleep 5"
with a short retry loop that polls for the existence of the resources before
calling kubectl wait: repeatedly run "kubectl -n tenant-test get hr
vm-disk-$name" (and similarly for dv and pvc) with a small sleep and a total
timeout, and only then invoke kubectl -n tenant-test wait hr vm-disk-$name
--for=condition=ready, kubectl -n tenant-test wait dv vm-disk-$name
--for=condition=ready and kubectl -n tenant-test wait pvc vm-disk-$name
--for=jsonpath='{.status.phase}'=Bound; apply the same replacement for the
second occurrence covering lines 57-80 so the script first ensures resources
exist before waiting for readiness.
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM — all four review items from the previous round are addressed:
- Cleanup is now idempotent via a top-level
teardownfunction that deletes all three resources with--ignore-not-found --timeout=2m. The inline post-test deletes are gone, so a test failing before it creates a resource no longer masks the error. - HelmRelease wait timeout bumped from 5s to 30s in all three call sites — matches
kafka.bats's pattern and should remove the flake-on-CI risk. - The
source.disktest now bootstraps its base viasource.image: alpine-3.21instead of an HTTP download, removing the external-network dependency and proving the clone is what's under test. - The
teardownblock implements the function I suggested instead of relying on inline cleanup.
Looks good to merge.
|
Closing in favour of #3482. The coverage gap this PR identified is still real, but the app E2E suites have since migrated from bats in |
2 participants
What this PR does
Follow-up to #2258 per Aleksei Sviridkin (@lexfrei)'s non-blocking review comment: adds BATS E2E coverage for the new vm-disk
source.diskandsource.imageclone paths. Existing coverage only exercisedsource.http(invminstance.bats).New file:
hack/e2e-apps/vm-disk.batswith two@testcases:source.image— creates a VMDisk cloning from thealpine-3.21golden image incozy-public(smallest image in the default collection), waits for the DataVolume to reachSucceededand the PVC to beBound, then cleans up.source.disk— first creates a base disk viasource.http, waits for it to be ready, then creates a clone viasource.disk, waits for the clone DV and PVC, and cleans up both.The file passes the
cozytest.shawk-transform +bash -nsyntax check. Full E2E requires a cluster and was not run locally.Release note
Summary by CodeRabbit