Split testing job into several - #1075
Conversation
WalkthroughThis change refactors the pull request workflow by splitting a single test job into a multi-stage pipeline with explicit environment preparation, installation, testing, and cleanup phases. It introduces granular Makefile targets and new BATS scripts for end-to-end cluster and platform setup, integrating them into the CI process for improved modularity. Changes
Sequence Diagram(s)sequenceDiagram
participant GitHub Actions
participant Sandbox
participant QEMU VMs
participant Talos Cluster
participant Cozystack Installer
GitHub Actions->>Sandbox: Run prepare_env (Makefile: prepare-env)
Sandbox->>QEMU VMs: Provision Talos cluster (e2e-prepare-cluster.bats)
QEMU VMs->>Talos Cluster: Boot and configure nodes
GitHub Actions->>Sandbox: Run install_cozystack (Makefile: install-cozystack)
Sandbox->>Cozystack Installer: Deploy and validate Cozystack (e2e-install-cozystack.bats)
GitHub Actions->>Sandbox: Run test_apps
GitHub Actions->>Sandbox: Run cleanup
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
packages/core/testing/Makefile (1)
35-38: Consider extracting asset copying into a common target to reduce duplication.The asset copying logic (lines 36-37) is duplicated in the
test-clustertarget (lines 44-45). Consider extracting this into a reusable pattern.Create a common target for asset copying:
+.PHONY: copy-assets +copy-assets: + docker cp ../../../_out/assets/cozystack-installer.yaml "${SANDBOX_NAME}":/workspace/_out/assets/cozystack-installer.yaml + docker cp ../../../_out/assets/nocloud-amd64.raw.xz "${SANDBOX_NAME}":/workspace/_out/assets/nocloud-amd64.raw.xz + +prepare-cluster: copy-assets + docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && hack/cozytest.sh hack/e2e-prepare-cluster.bats' -prepare-cluster: - docker cp ../../../_out/assets/cozystack-installer.yaml "${SANDBOX_NAME}":/workspace/_out/assets/cozystack-installer.yaml - docker cp ../../../_out/assets/nocloud-amd64.raw.xz "${SANDBOX_NAME}":/workspace/_out/assets/nocloud-amd64.raw.xz - docker exec "${SANDBOX_NAME}" sh -c 'cd /workspace && hack/cozytest.sh hack/e2e-prepare-cluster.bats'.github/workflows/pull-requests.yaml (1)
82-83: Consider extracting sandbox ID calculation to reduce duplication.The sandbox ID calculation is repeated in all jobs. Consider using job outputs or artifacts to share this value.
You could modify the
prepare_envjob to output the sandbox ID and reference it in subsequent jobs:prepare_env: name: Prepare environment runs-on: [self-hosted] needs: build + outputs: + sandbox_name: ${{ steps.sandbox.outputs.name }} # Never run when the PR carries the "release" label. if: | !contains(github.event.pull_request.labels.*.name, 'release') steps: - name: Download installer uses: actions/download-artifact@v4 with: name: cozystack-installer path: _out/assets/ - name: Download Talos image uses: actions/download-artifact@v4 with: name: talos-image path: _out/assets/ - name: Set sandbox ID + id: sandbox - run: echo "SANDBOX_NAME=$(echo "${GITHUB_REPOSITORY}:${GITHUB_WORKFLOW}:${GITHUB_RUN_NUMBER}" | sha256sum | cut -c1-6)" >> $GITHUB_ENV + run: | + SANDBOX_NAME=$(echo "${GITHUB_REPOSITORY}:${GITHUB_WORKFLOW}:${GITHUB_RUN_NUMBER}" | sha256sum | cut -c1-6) + echo "SANDBOX_NAME=${SANDBOX_NAME}" >> $GITHUB_ENV + echo "name=${SANDBOX_NAME}" >> $GITHUB_OUTPUTThen in subsequent jobs:
install_cozystack: name: Install Cozystack runs-on: [self-hosted] needs: prepare_env # Never run when the PR carries the "release" label. if: | !contains(github.event.pull_request.labels.*.name, 'release') steps: - - name: Set sandbox ID - run: echo "SANDBOX_NAME=$(echo "${GITHUB_REPOSITORY}:${GITHUB_WORKFLOW}:${GITHUB_RUN_NUMBER}" | sha256sum | cut -c1-6)" >> $GITHUB_ENV - name: Install Cozystack - run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME install-cozystack + run: make -C packages/core/testing SANDBOX_NAME=${{ needs.prepare_env.outputs.sandbox_name }} install-cozystackAlso applies to: 98-99, 114-115, 130-131
hack/e2e-prepare-cluster.bats (2)
29-30: Improve error handling in VM cleanup.The current approach could fail silently if PID files don't exist.
-@test "Clean previous VMs" { - kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true - rm -rf srv1 srv2 srv3 -} +@test "Clean previous VMs" { + for i in 1 2 3; do + if [ -f "srv${i}/qemu.pid" ]; then + pid=$(cat "srv${i}/qemu.pid") + if kill -0 "$pid" 2>/dev/null; then + kill "$pid" || true + fi + fi + done + rm -rf srv1 srv2 srv3 +}
223-224: Consider verifying bootstrap success more thoroughly.The bootstrap command uses a short timeout which might not be sufficient in all environments.
Add verification after bootstrap:
@test "Bootstrap Talos cluster" { # Bootstrap etcd on the first node timeout 10 sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done' + + # Verify bootstrap was successful + if ! talosctl health -n 192.168.123.11 -e 192.168.123.11 --wait-timeout 30s; then + echo "Bootstrap verification failed" >&2 + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pull-requests.yaml(2 hunks)Makefile(1 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Build
- GitHub Check: pre-commit
🔇 Additional comments (3)
Makefile (1)
52-54: LGTM! Target follows existing patterns.The new
prepare-envtarget is well-structured and follows the established pattern in the Makefile.packages/core/testing/Makefile (1)
40-41: LGTM! Clean and focused target.The
install-cozystacktarget follows the established pattern for running BATS tests.hack/e2e-install-cozystack.bats (1)
29-31: Verify fail function availability or use standard exit.The
failfunction might not be available in all BATS versions.#!/bin/bash # Check if the fail function is used elsewhere in BATS tests rg -A 2 'fail\s*"' hack/*.bats
| - name: E2E Apps | ||
| run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME delete |
There was a problem hiding this comment.
Fix incorrect step name in cleanup job.
The step name should reflect its actual purpose.
- - name: E2E Apps
+ - name: Cleanup sandbox
run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME delete📝 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.
| - name: E2E Apps | |
| run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME delete | |
| - name: Cleanup sandbox | |
| run: make -C packages/core/testing SANDBOX_NAME=$SANDBOX_NAME delete |
🤖 Prompt for AI Agents
In .github/workflows/pull-requests.yaml at lines 132 to 133, the step name "E2E
Apps" does not accurately describe the cleanup action being performed. Rename
the step to a name that clearly reflects its purpose, such as "Cleanup Sandbox"
or "Delete Sandbox," to improve clarity and maintainability of the workflow.
| ingress_ip=$(kubectl get svc root-ingress-controller -n tenant-root -o jsonpath='{.status.loadBalancer.ingress[0].ip}') | ||
| if ! curl -sS -k "https://${ingress_ip}" -H 'Host: grafana.example.org' --max-time 30 | grep -q Found; then | ||
| echo "Failed to access Grafana via ingress at ${ingress_ip}" >&2 | ||
| exit 1 | ||
| fi |
There was a problem hiding this comment.
Improve error handling for Grafana verification.
The current approach could miss curl failures.
- if ! curl -sS -k "https://${ingress_ip}" -H 'Host: grafana.example.org' --max-time 30 | grep -q Found; then
- echo "Failed to access Grafana via ingress at ${ingress_ip}" >&2
- exit 1
- fi
+ response=$(curl -sS -k "https://${ingress_ip}" -H 'Host: grafana.example.org' --max-time 30 -w '\n%{http_code}' 2>&1) || {
+ echo "Curl failed to access Grafana via ingress at ${ingress_ip}: $?" >&2
+ exit 1
+ }
+
+ if ! echo "$response" | grep -q Found; then
+ echo "Failed to access Grafana via ingress at ${ingress_ip}" >&2
+ echo "Response: $response" >&2
+ exit 1
+ fi📝 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.
| ingress_ip=$(kubectl get svc root-ingress-controller -n tenant-root -o jsonpath='{.status.loadBalancer.ingress[0].ip}') | |
| if ! curl -sS -k "https://${ingress_ip}" -H 'Host: grafana.example.org' --max-time 30 | grep -q Found; then | |
| echo "Failed to access Grafana via ingress at ${ingress_ip}" >&2 | |
| exit 1 | |
| fi | |
| ingress_ip=$(kubectl get svc root-ingress-controller -n tenant-root -o jsonpath='{.status.loadBalancer.ingress[0].ip}') | |
| response=$(curl -sS -k "https://${ingress_ip}" -H 'Host: grafana.example.org' --max-time 30 -w '\n%{http_code}' 2>&1) || { | |
| echo "Curl failed to access Grafana via ingress at ${ingress_ip}: $?" >&2 | |
| exit 1 | |
| } | |
| if ! echo "$response" | grep -q Found; then | |
| echo "Failed to access Grafana via ingress at ${ingress_ip}" >&2 | |
| echo "Response: $response" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In hack/e2e-install-cozystack.bats around lines 145 to 149, the current code
does not properly handle curl command failures which may cause false positives.
Modify the script to check the exit status of the curl command explicitly before
grepping for the expected content. If curl fails or the expected content is not
found, print the error message and exit with status 1 to ensure robust error
handling.
87f5f83 to
11776d6
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
.github/workflows/pull-requests.yaml (5)
103-108: Duplicate checkout configuration.This checkout block is repeated from earlier jobs. Consider abstracting it via a reusable workflow or template to reduce boilerplate.
109-111: Duplicate sandbox ID setup.Repeated SANDBOX_NAME echo logic should be centralized into a composite action or a workflow-level step for maintainability.
125-133: Repeated steps intest_apps.The checkout and SANDBOX_NAME setup are identical to other jobs. DRY by using a composite action or a shared job template.
147-155: DRY duty: Cleanup setup duplication.The checkout and SANDBOX_NAME steps in the cleanup job mirror earlier jobs. Abstract these into a shared action or job template to avoid repetition.
156-157: Fix incorrect step name in cleanup job.The step
E2E Appsactually deletes the sandbox. Rename toCleanup sandboxorDelete sandboxto accurately reflect its function.
🧹 Nitpick comments (7)
.github/workflows/pull-requests.yaml (7)
59-67: Inconsistent job naming convention.The job key
prepare_envuses snake_case, while Makefile targets and other jobs lean towards kebab-case (e.g.,install-cozystack). Consider renaming the job key toprepare-envfor consistency.
69-74: DRY duty: Duplicate checkout steps.The checkout block here is identical in every job. Extracting it into a reusable workflow/composite action or a YAML anchor can reduce duplication and maintenance overhead.
87-89: DRY duty: Sandbox ID generation duplication.The SANDBOX_NAME computation is repeated in multiple jobs. Encapsulate this in a composite action or set it once as a workflow output to centralize the logic.
93-101: Job naming consistency.The job key
install_cozystackuses snake_case, whereas the Makefile target isinstall-cozystack. Align the job key toinstall-cozystackto match your naming convention.
115-123:test_appsjob naming.Similar to other jobs, the key
test_appsuses snake_case. For consistency with targets (test-apps), consider renaming it totest-apps.
134-136: Step name clarity.
- name: E2E Appsis vague. Rename toTest managed applicationsto clearly describe the step’s purpose.
137-145: Cleanup job naming consistency.The job key
cleanupis minimal; consider renaming it tocleanup-environmentordestroy-sandboxfor self-documenting clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- packages/core/testing/Makefile
- hack/e2e-prepare-cluster.bats
- hack/e2e-install-cozystack.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (2)
.github/workflows/pull-requests.yaml (2)
112-114: Validate Makefile path and target.Confirm that
packages/core/testingcontains theinstall-cozystacktarget and that the path hasn't changed. Example check:rg -n 'install-cozystack' -n packages/core/testing/Makefile
90-92: Verifyprepare-envtarget existence.Ensure the root
Makefilehas aprepare-envtarget and that it orchestrates the cluster setup correctly. You can run:rg -n '^prepare-env:' Makefile
11776d6 to
a13254d
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)
145-149: Improve error handling when probing Grafana ingress
(This repeats the earlier feedback on the same lines.)
The current check ignores curl’s exit status; a TLS/connection error yields a false “pass”.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- Makefile
- packages/core/testing/Makefile
- .github/workflows/pull-requests.yaml
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
🔇 Additional comments (1)
hack/e2e-prepare-cluster.bats (1)
55-66: Netmask mismatch may break connectivityThe VMs are configured with
/26addresses whereas the bridge is on/24.
This places the nodes in a different broadcast domain from the host gateway (192.168.123.1/24) and can lead to ARP/route surprises.Consider using the same
/24everywhere, or—if/26is required—give the bridge an address inside that /26 and add an explicit route instead of relying on L2 discovery.
| @test "Wait until Talos API port 50000 is reachable on all machines" { | ||
| timeout 60 sh -ec 'until nc -nz 192.168.123.11 50000 && nc -nz 192.168.123.12 50000 && nc -nz 192.168.123.13 50000; do sleep 1; done' | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
60 s may be too short for Talos boot
Talos often needs >60 s to bring up its API, especially on freshly-provisioned disks.
Increasing the timeout to 180 s (or making it configurable) will reduce flakiness.
🤖 Prompt for AI Agents
In hack/e2e-prepare-cluster.bats around lines 118 to 120, the timeout for
waiting until Talos API port 50000 is reachable on all machines is set to 60
seconds, which may be too short and cause flakiness. Increase the timeout value
from 60 to 180 seconds or make the timeout configurable to allow more time for
Talos to fully boot and bring up its API.
| @test "Clean previous VMs" { | ||
| kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true | ||
| rm -rf srv1 srv2 srv3 | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid invoking kill with an empty PID list
When no *.pid files exist (first run or already cleaned up) cat expands to an empty string, so kill is called without arguments and prints usage/help.
It exits with a non-zero status which, under set -e (used by Bats internally), would abort the test.
- kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true
+find srv{1..3} -name qemu.pid -exec cat {} + 2>/dev/null | \
+ xargs -r kill 2>/dev/null || true🤖 Prompt for AI Agents
In hack/e2e-prepare-cluster.bats around lines 28 to 31, the kill command is
called with potentially empty PID input, causing it to fail and abort the test
under set -e. Modify the script to first check if the PID files exist and
contain PIDs before calling kill, ensuring kill is only invoked with valid
non-empty PID arguments to prevent errors.
| timeout 10 sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done' | ||
|
|
||
| # Wait until etcd is healthy | ||
| timeout 180 sh -ec 'until talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 >/dev/null 2>&1; do sleep 1; done' | ||
| timeout 60 sh -ec 'while talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 2>&1 | grep -q "rpc error"; do sleep 1; done' | ||
|
|
There was a problem hiding this comment.
timeout 10 is insufficient for etcd bootstrap
The first talosctl bootstrap frequently takes tens of seconds; a hard 10-second cut-off will yield false negatives.
-timeout 10 sh -ec 'until talosctl bootstrap ...
+timeout 120 sh -ec 'until talosctl bootstrap ...📝 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.
| timeout 10 sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done' | |
| # Wait until etcd is healthy | |
| timeout 180 sh -ec 'until talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 >/dev/null 2>&1; do sleep 1; done' | |
| timeout 60 sh -ec 'while talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 2>&1 | grep -q "rpc error"; do sleep 1; done' | |
| timeout 120 sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done' | |
| # Wait until etcd is healthy | |
| timeout 180 sh -ec 'until talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 >/dev/null 2>&1; do sleep 1; done' | |
| timeout 60 sh -ec 'while talosctl etcd members -n 192.168.123.11,192.168.123.12,192.168.123.13 -e 192.168.123.10 2>&1 | grep -q "rpc error"; do sleep 1; done' |
🤖 Prompt for AI Agents
In hack/e2e-prepare-cluster.bats around lines 223 to 228, the timeout for the
talosctl bootstrap command is set to 10 seconds, which is too short and causes
false negatives. Increase the timeout value to a longer duration, such as 60
seconds or more, to allow enough time for the bootstrap process to complete
successfully before timing out.
| @test "Boot QEMU VMs" { | ||
| for i in 1 2 3; do | ||
| qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 8 -m 24576 \ | ||
| -device virtio-net,netdev=net0,mac=52:54:00:12:34:5${i} \ | ||
| -netdev tap,id=net0,ifname=cozy-srv${i},script=no,downscript=no \ | ||
| -drive file=srv${i}/system.img,if=virtio,format=raw \ | ||
| -drive file=srv${i}/seed.img,if=virtio,format=raw \ | ||
| -drive file=srv${i}/data.img,if=virtio,format=raw \ | ||
| -display none -daemonize -pidfile srv${i}/qemu.pid | ||
| done |
There was a problem hiding this comment.
Resource demand is unrealistic for CI runners
Spawning three QEMU VMs each with 8 vCPUs and 24 GiB RAM (total ≈ 72 GiB RAM / 24 vCPU) will exhaust most hosted runners and even many self-hosted workers, causing the job to be OOM-killed.
- qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 8 -m 24576 \
+ # Use modest defaults suitable for CI
+ qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 2 -m 4096 \Tune the numbers—or make them configurable via environment variables—to fit typical CI hardware.
📝 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.
| @test "Boot QEMU VMs" { | |
| for i in 1 2 3; do | |
| qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 8 -m 24576 \ | |
| -device virtio-net,netdev=net0,mac=52:54:00:12:34:5${i} \ | |
| -netdev tap,id=net0,ifname=cozy-srv${i},script=no,downscript=no \ | |
| -drive file=srv${i}/system.img,if=virtio,format=raw \ | |
| -drive file=srv${i}/seed.img,if=virtio,format=raw \ | |
| -drive file=srv${i}/data.img,if=virtio,format=raw \ | |
| -display none -daemonize -pidfile srv${i}/qemu.pid | |
| done | |
| @test "Boot QEMU VMs" { | |
| for i in 1 2 3; do | |
| # Use modest defaults suitable for CI | |
| qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 2 -m 4096 \ | |
| -device virtio-net,netdev=net0,mac=52:54:00:12:34:5${i} \ | |
| -netdev tap,id=net0,ifname=cozy-srv${i},script=no,downscript=no \ | |
| -drive file=srv${i}/system.img,if=virtio,format=raw \ | |
| -drive file=srv${i}/seed.img,if=virtio,format=raw \ | |
| -drive file=srv${i}/data.img,if=virtio,format=raw \ | |
| -display none -daemonize -pidfile srv${i}/qemu.pid | |
| done | |
| } |
🤖 Prompt for AI Agents
In hack/e2e-prepare-cluster.bats around lines 103 to 112, the test spawns three
QEMU VMs each with 8 vCPUs and 24 GiB RAM, which is too resource-intensive for
typical CI runners. Modify the script to reduce the number of vCPUs and RAM per
VM to more reasonable defaults, or better, make these values configurable
through environment variables so they can be adjusted based on the CI
environment's capacity.
| kubectl wait deployment/cozystack -n cozy-system --timeout=1m --for=condition=Available | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Give the installer more breathing room
kubectl wait … --timeout=1m is tight for an image pull + pod start on a fresh cluster.
Bumping to 5 min (or parameterising) will avoid avoidable failures.
🤖 Prompt for AI Agents
In hack/e2e-install-cozystack.bats around lines 20 to 21, the kubectl wait
command uses a 1 minute timeout which is too short for image pulling and pod
startup on a fresh cluster. Increase the --timeout value to 5 minutes or make it
configurable via a parameter to give the installer more time and prevent
unnecessary failures.
a13254d to
146c4aa
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/pull-requests.yaml (1)
156-157: Fix incorrect step name in cleanup job
The stepE2E Appsactually deletes resources. Rename it toCleanup sandboxorDelete environmentfor accuracy.
🧹 Nitpick comments (4)
.github/workflows/pull-requests.yaml (4)
90-92: Clarify step naming to avoid confusion
ThePrepare environmentstep name is identical to the job name. Rename the step to something likeRun prepare-env Makefile targetto distinguish the action in the logs.
87-92: Extract sandbox ID calculation into a reusable action or job output
Lines 87–92 duplicate the logic for computing and exportingSANDBOX_NAME. To DRY up the workflow, consider moving this into a composite action or computing it once inprepare_envand passing it viajobs.prepare_env.outputs.
93-113: Consolidate repetitive setup steps
Theinstall_cozystackjob (lines 93–113) repeats the checkout and sandbox‐ID setup fromprepare_env. You could streamline this by usingneeds.prepare_env.outputs.SANDBOX_NAMEand defining a reusable workflow or composite action for checkout + env setup.
134-136: Align step name with action
Intest_apps, the step is namedE2E Appsbut runsmake test-apps. For clarity, rename it toRun test-apps Makefile targetorE2E application tests.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-apps.bats(0 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
💤 Files with no reviewable changes (1)
- hack/e2e-apps.bats
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- packages/core/testing/Makefile
- hack/e2e-prepare-cluster.bats
- hack/e2e-install-cozystack.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
| cleanup: | ||
| name: Tear down environment | ||
| runs-on: [self-hosted] | ||
| needs: test_apps | ||
|
|
||
| # Never run when the PR carries the "release" label. | ||
| if: | | ||
| !contains(github.event.pull_request.labels.*.name, 'release') | ||
|
|
There was a problem hiding this comment.
Ensure teardown runs on failures
Currently cleanup only runs if test_apps succeeds, which can leak sandboxes on failures. Wrap the condition with always() to guarantee execution:
- needs: test_apps
- if: |
- !contains(github.event.pull_request.labels.*.name, 'release')
+ needs: test_apps
+ if: ${{ always() && !contains(github.event.pull_request.labels.*.name, 'release') }}📝 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.
| cleanup: | |
| name: Tear down environment | |
| runs-on: [self-hosted] | |
| needs: test_apps | |
| # Never run when the PR carries the "release" label. | |
| if: | | |
| !contains(github.event.pull_request.labels.*.name, 'release') | |
| cleanup: | |
| name: Tear down environment | |
| runs-on: [self-hosted] | |
| needs: test_apps | |
| # Never run when the PR carries the "release" label. | |
| if: ${{ always() && !contains(github.event.pull_request.labels.*.name, 'release') }} |
🤖 Prompt for AI Agents
In .github/workflows/pull-requests.yaml around lines 137 to 145, the cleanup job
only runs if the test_apps job succeeds, which can cause resource leaks on
failures. Modify the condition to wrap it with always() so that the cleanup step
runs regardless of the test_apps job outcome, ensuring teardown always executes
even on failures.
146c4aa to
de99474
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
hack/e2e-prepare-cluster.bats (4)
29-30: Still riskskillwith an empty PID list – previous feedback unaddressed
Invokingkill $(cat …)when no PID files exist expands to an empty argument list, causingkillto exit with usage and — under Bats’set -esemantics — terminate the test early.- kill $(cat srv1/qemu.pid srv2/qemu.pid srv3/qemu.pid 2>/dev/null) 2>/dev/null || true +find srv{1..3} -name qemu.pid -exec cat {} + 2>/dev/null | \ + xargs -r kill 2>/dev/null || true
105-111: QEMU sizing will OOM typical CI runners
Spawning 3 VMs with-smp 8 -m 24576(≈ 72 GiB RAM / 24 vCPU total) is far beyond what GitHub-hosted or most self-hosted runners provide, and will be killed by the kernel. Make vCPU/RAM configurable via env vars, defaulting to something modest (e.g. 2 vCPU / 4 GiB).- qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host -smp 8 -m 24576 \ + qemu-system-x86_64 -machine type=pc,accel=kvm -cpu host \ + -smp "${QEMU_VCPUS:-2}" -m "${QEMU_RAM_MB:-4096}" \
118-120: 60 s is too short for Talos API to come up – expect flaky failures
Talos frequently needs > 60 s on fresh images. Prior review suggested 180 s or making it tunable; please adjust.-timeout 60 sh -ec 'until nc -nz 192.168.123.11 50000 && nc -nz 192.168.123.12 50000 && nc -nz 192.168.123.13 50000; do sleep 1; done' +timeout "${TALOS_API_TIMEOUT:-180}" sh -ec 'until nc -nz 192.168.123.11 50000 && nc -nz 192.168.123.12 50000 && nc -nz 192.168.123.13 50000; do sleep 1; done'
223-224:timeout 10fortalosctl bootstrapis almost guaranteed to fail
Real-world bootstrap regularly takes tens of seconds. Increase to ≥ 60 s or parameterise.-timeout 10 sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done' +timeout "${TALOS_BOOTSTRAP_TIMEOUT:-120}" sh -ec 'until talosctl bootstrap -n 192.168.123.11 -e 192.168.123.11; do sleep 1; done'
🧹 Nitpick comments (1)
hack/e2e-prepare-cluster.bats (1)
201-205: Secrets file may be stale between runs
Keepingsecrets.yamlon disk and re-using it across consecutive CI invocations risks mismatching cluster secrets after a failed/aborted run. Consider generating it every time (or at least wiping it during “Clean previous VMs”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-apps.bats(8 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- Makefile
- hack/e2e-apps.bats
- hack/e2e-install-cozystack.bats
- .github/workflows/pull-requests.yaml
- packages/core/testing/Makefile
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
9ab82bd to
211f054
Compare
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
hack/e2e-prepare-cluster.bats (4)
29-30: Still risks aborting the run when no PID files exist
Earlier feedback pointed out that invokingkill $(cat …)with an empty expansion causeskillto exit 1 underset -e, aborting the test. The exact line is unchanged, so the issue persists.
103-111: Per-VM 8 vCPU / 24 GiB exceeds most CI runners
Spawning three VMs with-smp 8 -m 24576(≈ 72 GiB RAM, 24 vCPU in total) will OOM-kill or throttle on typical GitHub/self-hosted runners. Please down-size or make values overridable (e.g.VM_CPUS,VM_RAM).
118-120: 60 s Talos-API probe is still too aggressive
Talos frequently needs > 60 s to bring up port 50000 on fresh disks. Previous review suggested 180 s or a configurable timeout; unchanged here.
223-223: 10-second bootstrap timeout causes false negatives
talosctl bootstrapoften exceeds 10 s; prior comment recommended ≥ 120 s.
🧹 Nitpick comments (3)
hack/e2e-prepare-cluster.bats (1)
94-101: Add teardown for tap devices / bridge
The script creates tap interfaces and a bridge but never removes them. Subsequent CI jobs on the same runner can fail with “device exists”. Consider a final Bats test (or atrap) that deletescozy-srv*andcozy-br0.+@test "Cleanup networking" { + for i in 1 2 3; do + ip link del cozy-srv${i} 2>/dev/null || true + done + ip link del cozy-br0 2>/dev/null || true +}hack/e2e-apps.bats (2)
8-9: Consider extracting a helper for “create-if-absent” pattern
Thekubectl get … || kubectl create -f - <<EOFidiom is duplicated in every test. A small shell helper (e.g.ensure_resource()) would reduce noise and make future maintenance easier.
119-120: Aliasdvmay not exist on minimal kubectl installs
kubectl wait dv …relies on the short-namedvfordatavolumes.cdi.kubevirt.io. Short-names are cluster-registered and can be absent. Using the fully-qualified kind avoids portability issues:- kubectl -n tenant-test wait dv vm-disk-$name --timeout=150s --for=condition=ready + kubectl -n tenant-test wait datavolume.cdi.kubevirt.io/v1alpha1 vm-disk-$name --timeout=150s --for=condition=ready
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-apps.bats(9 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- packages/core/testing/Makefile
- .github/workflows/pull-requests.yaml
- hack/e2e-install-cozystack.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
This patch separates the Test job of the PR workflow into several smaller jobs: 1) create a testing sandbox and deploy Talos, 2) install Cozystack and configure it, 3) install managed applications and run e2e tests. This lets developers shorten the feedback loop if tests are merely acting flaky and aren't really broken. It's not the right way, but it's 80/20. Signed-off-by: Timofei Larkin <lllamnyp@gmail.com>
211f054 to
746641e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
hack/e2e-apps.bats (2)
8-9: Silence noisy existence-check output
kubectl getprints the full resource when it succeeds, which clutters CI logs and makes troubleshooting harder. Redirect the output to/dev/null(and stderr as well) while still relying on the exit-code for the existence check.-kubectl -n tenant-root get tenants.apps.cozystack.io test || +kubectl -n tenant-root get tenants.apps.cozystack.io test >/dev/null 2>&1 ||Apply the same redirection for every other
kubectl … get … || kubectl create …pair shown in the listed lines.Also applies to: 29-30, 102-103, 126-127, 171-172, 219-220, 270-271, 316-317
118-120: Timeouts are unbalanced: DataVolume waits 150 s, HelmRelease only 5 s
hr vm-disk-$nameoften takes longer than 5 s before the DV starts; occasional flakes are still observed.
Consider raising the HelmRelease wait to match the new DV timeout (e.g. 60 s) or polling until both are ready in a single loop to avoid false negatives.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.github/workflows/pull-requests.yaml(3 hunks)Makefile(1 hunks)hack/e2e-apps.bats(9 hunks)hack/e2e-install-cozystack.bats(1 hunks)hack/e2e-prepare-cluster.bats(1 hunks)packages/core/testing/Makefile(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/core/testing/Makefile
- Makefile
- .github/workflows/pull-requests.yaml
- hack/e2e-install-cozystack.bats
- hack/e2e-prepare-cluster.bats
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build
| kubectl -n tenant-test delete kuberneteses.apps.cozystack.io test | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure resources are fully removed before the next test step
Immediate deletion commands return as soon as the request is accepted, not when the resource is actually gone.
If the subsequent test (or Bats teardown) re-creates the same name, a race condition is possible and has bitten us in previous runs.
-kubectl -n tenant-test delete kuberneteses.apps.cozystack.io test
+kubectl -n tenant-test delete kuberneteses.apps.cozystack.io test --wait=true --timeout=2mRepeating the --wait=true --timeout=<...> (or a kubectl wait --for=delete …) for every delete in the lines above will make the script safer and more deterministic.
Also applies to: 165-166, 213-214, 265-266, 311-312
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 97 to 98, the kubectl delete command does not
wait for the resource to be fully removed, risking race conditions in subsequent
test steps. Modify the delete commands to include the --wait=true and
--timeout=<duration> flags or use kubectl wait --for=delete to ensure the
resource is completely deleted before proceeding. Apply the same fix to the
delete commands at lines 165-166, 213-214, 265-266, and 311-312 for consistent
and reliable test execution.
| timeout 180 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123 9000'; do sleep 10; done" | ||
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-0 --timeout=120s --for=jsonpath='{.status.replicas}'=1 | ||
| timeout 80 sh -ec "until kubectl -n tenant-test get endpoints chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.subsets[*].addresses[*].ip}' | grep -q '[0-9]'; do sleep 10; done" | ||
| timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[0].port}' | grep -q '9000 8123 9009'; do sleep 10; done" | ||
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.replicas}'=2 | ||
| timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[*].port}' | grep -q '9000 8123 9009'; do sleep 10; done" | ||
| timeout 80 sh -ec "until kubectl -n tenant-test get sts chi-clickhouse-$name-clickhouse-0-1 ; do sleep 10; done" | ||
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.replicas}'=1 |
There was a problem hiding this comment.
Fragile ClickHouse readiness checks
-
grep -q '8123 9000'assumes the ports appear in that exact order with a single space separator.
Different JSONPath output or extra spaces break the test. -
Waiting for
jsonpath='{.status.replicas}'=1only guarantees the StatefulSet desires one replica, not that it is running or ready. UsereadyReplicas.
-timeout 180 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123 9000'; do sleep 10; done"
+timeout 180 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123' && kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '9000'; do sleep 10; done"
@@
-kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-0 --timeout=120s --for=jsonpath='{.status.replicas}'=1
+kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-0 --timeout=120s --for=jsonpath='{.status.readyReplicas}'=1
@@
-kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.replicas}'=1
+kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.readyReplicas}'=1These tweaks make the check order-insensitive and validate that the pods are actually up.
📝 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.
| timeout 180 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123 9000'; do sleep 10; done" | |
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-0 --timeout=120s --for=jsonpath='{.status.replicas}'=1 | |
| timeout 80 sh -ec "until kubectl -n tenant-test get endpoints chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.subsets[*].addresses[*].ip}' | grep -q '[0-9]'; do sleep 10; done" | |
| timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[0].port}' | grep -q '9000 8123 9009'; do sleep 10; done" | |
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.replicas}'=2 | |
| timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[*].port}' | grep -q '9000 8123 9009'; do sleep 10; done" | |
| timeout 80 sh -ec "until kubectl -n tenant-test get sts chi-clickhouse-$name-clickhouse-0-1 ; do sleep 10; done" | |
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.replicas}'=1 | |
| timeout 180 sh -ec "until kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '8123' && kubectl -n tenant-test get svc chendpoint-clickhouse-$name -o jsonpath='{.spec.ports[*].port}' | grep -q '9000'; do sleep 10; done" | |
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-0 --timeout=120s --for=jsonpath='{.status.readyReplicas}'=1 | |
| timeout 80 sh -ec "until kubectl -n tenant-test get endpoints chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.subsets[*].addresses[*].ip}' | grep -q '[0-9]'; do sleep 10; done" | |
| timeout 100 sh -ec "until kubectl -n tenant-test get svc chi-clickhouse-$name-clickhouse-0-0 -o jsonpath='{.spec.ports[*].port}' | grep -q '9000 8123 9009'; do sleep 10; done" | |
| timeout 80 sh -ec "until kubectl -n tenant-test get sts chi-clickhouse-$name-clickhouse-0-1 ; do sleep 10; done" | |
| kubectl -n tenant-test wait statefulset.apps/chi-clickhouse-$name-clickhouse-0-1 --timeout=140s --for=jsonpath='{.status.readyReplicas}'=1 |
🤖 Prompt for AI Agents
In hack/e2e-apps.bats around lines 347 to 352, the readiness checks for
ClickHouse services are fragile because they rely on exact port order and
spacing in grep and only check desired replicas instead of ready replicas. To
fix this, modify the grep commands to be order-insensitive by matching ports
individually or using a regex that allows any order and spacing. Also, replace
the wait condition from checking jsonpath='{.status.replicas}'=1 to
jsonpath='{.status.readyReplicas}'=1 to ensure the StatefulSet pods are actually
running and ready.
This patch separates the Test job of the PR workflow into several smaller jobs: 1) create a testing sandbox and deploy Talos, 2) install Cozystack and configure it, 3) install managed applications and run e2e tests. This lets developers shorten the feedback loop if tests are merely acting flaky and aren't really broken. It's not the right way, but it's 80/20.
Summary by CodeRabbit