Repository navigation
feat(e2e): parallel post-install prep - #2557
Conversation
|
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)
📝 WalkthroughWalkthroughThe e2e Install Cozystack test now launches a background post-install preparation script while waiting for cluster-wide HelmRelease objects to be reconciled; the test waits for both HelmRelease readiness and the background script, failing if the background script exits non-zero. The new script prepares LINSTOR pools, StorageClasses, and MetalLB resources. ChangesE2E Test Initialization & Post-Install Preparation
Sequence DiagramsequenceDiagram
participant BATS as BATS Test
participant PostPrep as post-install-prep.sh
participant Cluster as Kubernetes Cluster
participant LINSTOR as LINSTOR Cluster
participant MetalLB as MetalLB
BATS->>PostPrep: Start in background (stdout → /tmp/post-install-prep.log)
par Parallel Work
BATS->>Cluster: Wait for HelmRelease objects\nkubectl wait --for=condition=ready (per HelmRelease)
and
PostPrep->>Cluster: Wait for LINSTOR HelmRelease Ready
PostPrep->>Cluster: Wait for linstor-controller Deployment Available
PostPrep->>LINSTOR: Poll until 3 nodes Online
PostPrep->>LINSTOR: Create data pools on srv1/srv2/srv3 (concurrent)
PostPrep->>Cluster: Apply StorageClass manifests (local, replicated)
PostPrep->>Cluster: Wait for MetalLB CRDs
PostPrep->>MetalLB: Apply IPAddressPool & L2Advertisement
PostPrep-->>PostPrep: Print completion message
end
BATS->>BATS: HelmRelease reconciliation done
BATS->>PostPrep: Wait for background PID
PostPrep-->>BATS: Exit (success or failure)
alt PostPrep failed
BATS->>BATS: Print /tmp/post-install-prep.log\nExit non-zero
else PostPrep succeeded
BATS->>BATS: Print /tmp/post-install-prep.log\nProceed with test
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
github-actions
Bot
added
area/platform
dosubot
Bot
removed
the
size/L
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 optimizes the Cozystack installation process by introducing configurable Helm release intervals and parallelizing post-installation tasks. These changes aim to significantly reduce the time required for E2E test suites while improving the reliability of the deployment sequence by addressing known race conditions in the LINSTOR setup. 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
|
dosubot
Bot
added
size:L
area/ci
![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 optimizes the E2E installation process by moving LINSTOR and MetalLB configuration into a background script, allowing these tasks to run in parallel with the platform reconciliation. It also introduces a configurable Helm release interval for the cozystack-operator, which is set to 30 seconds for E2E tests to accelerate the testing cycle. Feedback is provided regarding the need for explicit PID waiting in the shell script to ensure background process failures are correctly captured, and a recommendation to use the quote function in Helm templates for better value handling.
| for node in srv1 srv2 srv3; do | ||
| case $created_pools in | ||
| *" $node "*) echo " pool 'data' already exists on $node"; continue;; | ||
| esac | ||
| kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data & | ||
| done | ||
| wait |
There was a problem hiding this comment.
In POSIX sh, the wait command without arguments waits for all background processes but always returns a zero exit status, even if some background jobs failed. To ensure the E2E test fails if any of the parallel LINSTOR pool creation commands fail, you should collect the PIDs and wait for each one explicitly. Since set -e is active, the script will exit on the first failed wait.
| for node in srv1 srv2 srv3; do | |
| case $created_pools in | |
| *" $node "*) echo " pool 'data' already exists on $node"; continue;; | |
| esac | |
| kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data & | |
| done | |
| wait | |
| pids="" | |
| for node in srv1 srv2 srv3; do | |
| case $created_pools in | |
| *" $node "*) echo " pool 'data' already exists on $node"; continue;; | |
| esac | |
| kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data & | |
| pids="$pids $!" | |
| done | |
| for pid in $pids; do | |
| wait "$pid" | |
| done |
References
- Handle errors explicitly. Discarding meaningful errors is a bug. (link)
There was a problem hiding this comment.
Good catch — fixed in d8da80b. The pool-creation loop now records each background kubectl exec PID into pids, then wait "$pid" runs per job, so a failed pool creation propagates under set -e instead of being masked by the bare wait.
| - --disable-telemetry | ||
| {{- end }} | ||
| {{- if .Values.cozystackOperator.helmReleaseInterval }} | ||
| - --helmrelease-interval={{ .Values.cozystackOperator.helmReleaseInterval }} |
There was a problem hiding this comment.
Following Helm best practices and the repository style guide, string values in templates should be quoted to ensure they are correctly parsed as strings, especially when they contain units like 30s.
- --helmrelease-interval={{ .Values.cozystackOperator.helmReleaseInterval | quote }}References
- Helm template correctness: missing
quote(link)
There was a problem hiding this comment.
Not applying this one. | quote here would expand inline inside the argument string, rendering - --helmrelease-interval="30s" — i.e. the literal quote characters end up embedded in the arg passed to the operator, and flag.Duration would reject "30s". Quoting is correct on standalone YAML scalars (e.g. value: {{ ... | quote }} further down for KUBERNETES_SERVICE_PORT), but inside --flag=<value> the unquoted form is what we need.
…rallel post-install prep Two complementary changes that depend on the operator's `--helmrelease-interval` flag landing via #2509: 1. Installer chart exposes `cozystackOperator.helmReleaseInterval` (default empty -> flag not rendered -> operator default 5m applies). E2E install passes `30s`. Eliminates the 8-10 min Flux dead zone where dependency-blocked HRs (waiting on cert-manager webhooks, CRDs) are requeued only every 5 min. 2. LINSTOR / StorageClass / MetalLB setup, previously sequential `@test` cases after the 15 m platform HR wait, now run as a background prep that completes during the platform reconcile. None of them gates platform HR reconcile, so they can move out of the critical path. Helper script `hack/e2e-post-install-prep.sh` polls for its prerequisites and is awaited at the end of the `Install Cozystack` test so failures still surface deterministically. Includes the T2-race fix: wait on the LINSTOR HR being `Ready` before polling for `deploy/linstor-controller`. The old 5-min hard timeout fired during cold installs where the LINSTOR HR's dependency chain takes longer than 5 min to resolve. Production defaults are unchanged. The 30 s interval is opt-in per install command. Depends on: - #2509 (operator-side `--helmrelease-interval` flag). Signed-off-by: Myasnikov Daniil <[email protected]> Assisted-By: Claude <[email protected]>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
hack/e2e-post-install-prep.sh (1)
25-31: ⚡ Quick win
wait(no args) silently swallows background-job failures.In
/bin/sh, a barewaitafter multiple&jobs always returns 0, so a failedlinstor ps cdpon one node will be lost and the StorageClass apply proceeds against an incomplete pool topology. Capture PIDs and wait on each soset -eactually trips.♻️ Track PIDs and wait per-job
echo "[post-install-prep] creating LINSTOR storage pools (parallel across nodes)" created_pools=$(kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor sp l -s data --pastable | awk '$2 == "data" {printf " " $4} END{printf " "}') +pids="" for node in srv1 srv2 srv3; do case $created_pools in *" $node "*) echo " pool 'data' already exists on $node"; continue;; esac - kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data & + kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data & + pids="$pids $!" done -wait +for pid in $pids; do + wait "$pid" +done🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@hack/e2e-post-install-prep.sh` around lines 25 - 31, The bare wait at the end swallows background failures; change the loop that launches kubectl exec (the line with "kubectl exec -n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc --pool-name data --storage-pool data &") to capture each background PID (e.g., pids+=( $! )) and after the loop iterate over those PIDs and call "wait <pid>" for each, checking the exit status and exiting non‑zero if any wait fails so that set -e will trip; reference the loop variable "node" and the "created_pools" check when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/installer/templates/cozystack-operator.yaml`:
- Around line 71-73: The template conditionally adds the unknown CLI flag
"--helmrelease-interval" using .Values.cozystackOperator.helmReleaseInterval
which will cause the cozystack-operator to crash because the flag isn't
implemented in cmd/cozystack-operator/main.go for the pinned image (v1.3.0);
remove the conditional block that emits "- --helmrelease-interval={{
.Values.cozystackOperator.helmReleaseInterval }}" from the cozystack-operator
manifest template, or alternatively rebase/update the PR to use an operator
image that includes the new flag implementation before reinstating the
conditional.
---
Nitpick comments:
In `@hack/e2e-post-install-prep.sh`:
- Around line 25-31: The bare wait at the end swallows background failures;
change the loop that launches kubectl exec (the line with "kubectl exec -n
cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc
--pool-name data --storage-pool data &") to capture each background PID (e.g.,
pids+=( $! )) and after the loop iterate over those PIDs and call "wait <pid>"
for each, checking the exit status and exiting non‑zero if any wait fails so
that set -e will trip; reference the loop variable "node" and the
"created_pools" check when making this change.
🪄 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: ca5788af-4514-4786-a047-3293803b78b7
📒 Files selected for processing (4)
hack/e2e-install-cozystack.batshack/e2e-post-install-prep.shpackages/core/installer/templates/cozystack-operator.yamlpackages/core/installer/values.yaml
c86f5d4 to
7ed6444
Compare
github-actions
Bot
added
size/L
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@hack/e2e-post-install-prep.sh`:
- Around line 29-31: The background kubectl exec jobs started with "kubectl exec
-n cozy-linstor deploy/linstor-controller -- linstor ps cdp zfs ${node} /dev/vdc
--pool-name data --storage-pool data &" can fail silently because the single
"wait" may report success; change the loop to record each job PID (use $!) into
an array, then loop over those PIDs and run "wait <pid>" for each and test its
exit status, exiting non-zero or failing the script if any wait returns a
non-zero status; ensure you remove the single blind "wait" and replace with
per-PID waits and checks so any failed kubectl exec is propagated.
🪄 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: 02ff9b35-296a-4c41-8455-9424777d9591
📒 Files selected for processing (2)
hack/e2e-install-cozystack.batshack/e2e-post-install-prep.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/e2e-install-cozystack.bats
myasnikovdaniil
changed the title
Address review feedback on hack/e2e-post-install-prep.sh:31: POSIX `wait` without arguments returns 0 even if a backgrounded `kubectl exec` failed, which would mask partial LINSTOR prep failures and let downstream tests run on a half-provisioned cluster. Collect each background PID and `wait "$pid"` per job so `set -e` exits the script on the first failed pool creation. Assisted-By: Claude <[email protected]> Signed-off-by: Myasnikov Daniil <[email protected]>
myasnikovdaniil
removed
the
do-not-merge/work-in-progress
Andrei Kvapil (kvaps)
left a comment
Andrei Kvapil (kvaps)
left a comment
There was a problem hiding this comment.
LGTM. Three independent improvements, each well-motivated.
Parallel post-install prep: moving LINSTOR / StorageClass / MetalLB setup into a background helper that completes during the platform reconcile is the right structural fix — none of those gates platform HR reconcile, so they have no business being in the critical path. The hack/e2e-post-install-prep.sh polling for prerequisites and being awaited at the end of "Install Cozystack" preserves deterministic failure surfacing while removing the sequential wait.
T2-race fix: waiting on the LINSTOR HR Ready before polling for deploy/linstor-controller correctly orders the dependency. The previous 5-min hard timeout was racing the cert-manager → piraeus-operator-crds → piraeus-operator → linstor chain on cold installs.
helmReleaseInterval=30s for E2E: pairs cleanly with the operator-side flag from #2509 (already merged). Eliminating the 8-10 min Flux dead zone on dependency-blocked HRs is exactly what makes the parallel prep above actually parallel — without the reduced requeue interval, the prep would still get stuck behind Flux's default 5-min requeue. The operator-default stays 5m, only the e2e harness flips to 30s — right place to apply the knob.
Surfaced from #2500, three load-bearing reductions in cold-run flakiness.
Ship it.
2 participants
What this PR does
Parallel post-install prep
LINSTOR / StorageClass / MetalLB setup, previously sequential
@testcases after the 15 m platform HR wait, now run as a background prep that completes during the platform reconcile. None of them gates platform HR reconcile, so they can move out of the critical path.New helper
hack/e2e-post-install-prep.shpolls for its prerequisites and is awaited at the end of theInstall Cozystacktest so failures still surface deterministically.T2-race fix
Wait on the LINSTOR HR being
Readybefore polling fordeploy/linstor-controller. The old 5-min hard timeout fired during cold installs where the LINSTOR HR's dependency chain (cert-manager → piraeus-operator-crds → piraeus-operator → linstor) takes longer than 5 min to resolve.E2E uses helmReleaseInterval=30s
E2E install passes
--set cozystackOperator.helmReleaseInterval=30sto eliminate the 8–10 min Flux dead zone where dependency-blocked HRs are requeued only every 5 min. Operator-side flag and chart value landed in #2509; production defaults are unchanged.Surfaced from #2500.
Release note
Summary by CodeRabbit
Tests
Chores