Repository navigation
test(tests): pre-pull timing-sensitive platform images before install - #2556
Conversation
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 reliability of end-to-end installations by addressing image-pull stagger across nodes. By ensuring that all required images are cached on every node before the main workload deployment, it prevents race conditions and timeouts in distributed systems that are sensitive to replica startup synchronization. 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
|
github-actions
Bot
added
area/uncategorized
📝 WalkthroughWalkthroughAdds an e2e pre-pull step: a new Bats test renders ChangesImage Pre-caching for E2E Tests
Sequence DiagramsequenceDiagram
participant Bats as Bats Test
participant Helm as Helm/YQ
participant Script as Pre-pull Script
participant K8s as Kubernetes API
participant Node as Cluster Node
Bats->>Helm: Render charts (`kubeovn`, `linstor`)
Helm-->>Bats: YAML with containers & initContainers
Bats->>Helm: Extract `.image` fields (yq)
Helm-->>Bats: Deduplicated image list
Bats->>Script: Pipe image list to stdin
Script->>Script: Parse, dedupe, build DaemonSet manifest
Script->>K8s: kubectl apply DaemonSet
K8s->>Node: Schedule Pod per node (sleep containers)
Node->>Node: Pull images (IfNotPresent)
K8s-->>Script: DaemonSet ready (rollout)
Script->>K8s: Delete DaemonSet (cleanup)
Script-->>Bats: Pre-pull complete
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
the
area/testing
![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 introduces an image pre-pulling mechanism for E2E tests to prevent timeouts in peer-sensitive workloads like OVN and LINSTOR. It adds a new test case to extract images from Helm charts and a bash script that deploys a temporary DaemonSet to cache these images across all nodes. Feedback includes improving the script's robustness by using a shell trap for cleanup, refining the yq extraction logic, and ensuring the commit scope adheres to the repository's style guide.
| # status blocks until all pods are Ready (= all containers running = all | ||
| # images cached on every node), then we delete the DaemonSet. | ||
|
|
||
| set -euo pipefail |
There was a problem hiding this comment.
To ensure the e2e-image-prepuller DaemonSet is always cleaned up, even if the script fails or times out during the rollout status check, it's recommended to use a trap on EXIT.
| set -euo pipefail | |
| set -euo pipefail | |
| cleanup() { | |
| kubectl delete daemonset e2e-image-prepuller -n kube-system --ignore-not-found | |
| } | |
| trap cleanup EXIT |
There was a problem hiding this comment.
Good catch — confirmed. Under set -euo pipefail, a kubectl rollout status timeout aborted the script before line 80 and leaked the e2e-image-prepuller DaemonSet until the next CI run overwrote it. Moved the delete into a cleanup() registered with trap … EXIT so it fires on every exit path (success, rollout timeout, signal). Fixed in 9df5950.
| fi | ||
| } | ||
|
|
||
| @test "Pre-pull platform images" { |
There was a problem hiding this comment.
The PR title uses the scope e2e, which is not listed as a valid scope in the repository style guide. Please consider using tests or hack instead to adhere to the Conventional Commits requirements.
References
- Valid scopes for commits are defined in the repository style guide (lines 40-45).
e2eis not among them. (link)
There was a problem hiding this comment.
You're right — e2e is neither a Conventional Commits type nor one of the scopes listed in .gemini/styleguide.md (Meta scopes are api, hack, tests, ci, docs). Renamed the commit subject and the PR title to test(tests): pre-pull timing-sensitive platform images before install, which matches the repo's convention for tests-only changes. Fixed in 9df5950 (force-push).
| } | yq -N ' | ||
| (..|select(has("containers"))|.containers[]|.image), | ||
| (..|select(has("initContainers"))|.initContainers[]|.image) | ||
| ' | hack/e2e-prepull-images.sh |
There was a problem hiding this comment.
The yq query can be simplified and made more robust by using optional chaining and filtering out null values. This ensures that if any container or initContainer is missing an image field, it won't cause issues in the downstream script.
} | yq -N '.. | (.containers, .initContainers) | .[]? | .image | select(.)' | hack/e2e-prepull-images.sh
There was a problem hiding this comment.
The longer form is intentional: each branch of the union (containers, initContainers) lines up with the comment block above it that explains "walk only PodSpec-shaped objects, take container images." The proposed .[]? does add safety against a missing image: field, but helm template of these charts always emits a fully-populated image: per container, so there's nothing to guard against today. Keeping the current shape for readability.
|
|
||
| set -euo pipefail | ||
|
|
||
| mapfile -t images < <(grep -Ev '^[[:space:]]*(#|$)' | sort -u) |
There was a problem hiding this comment.
Adding --- to the grep filter ensures that any YAML document separators produced by yq (which can happen with multi-document input) are ignored.
| mapfile -t images < <(grep -Ev '^[[:space:]]*(#|$)' | sort -u) | |
| mapfile -t images < <(grep -Ev '^[[:space:]]*(#|$|---)' | sort -u) |
There was a problem hiding this comment.
yq -N (--no-doc) is upstream of this grep and explicitly suppresses YAML document separators by design — yq cannot emit --- with that flag set. Adding --- to the filter would be dead code, and worse, would silently mask a real upstream bug if -N ever got dropped. Prefer to keep the script erroring loudly on unexpected input.
| echo "Waiting for e2e-image-prepuller DaemonSet to become ready..." | ||
| kubectl rollout status daemonset/e2e-image-prepuller -n kube-system --timeout=10m | ||
|
|
||
| kubectl delete daemonset e2e-image-prepuller -n kube-system --ignore-not-found |
There was a problem hiding this comment.
With the implementation of a trap for cleanup, this explicit delete call is redundant and can be removed.
There was a problem hiding this comment.
Removed — the delete now runs from the EXIT trap installed at the top of the script (see the related thread on line 18). Fixed in 9df5950.
![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-prepull-images.sh (1)
77-80: ⚡ Quick winOrphaned DaemonSet left in
kube-systemon rollout timeout.Because
set -euo pipefailis active, akubectl rollout statustimeout causes the script to abort at line 78 and never reach thekubectl deleteon line 80. Thee2e-image-prepullerDaemonSet (and its sleeping pods) stays running until the next CI run overwrites it viakubectl apply.Add a cleanup trap immediately after the apply so it fires on every exit path:
🛡️ Proposed fix
kubectl apply -f - <<EOF ... EOF + +trap 'kubectl delete daemonset e2e-image-prepuller -n kube-system --ignore-not-found' EXIT echo "Waiting for e2e-image-prepuller DaemonSet to become ready..." kubectl rollout status daemonset/e2e-image-prepuller -n kube-system --timeout=10m -kubectl delete daemonset e2e-image-prepuller -n kube-system --ignore-not-found echo "Image pre-pull complete."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-prepull-images.sh` around lines 77 - 80, After applying the e2e-image-prepuller DaemonSet, add a cleanup function and install a trap so the DaemonSet is deleted on every exit path (including rollout timeout); implement a small function (e.g., cleanup_prepuller) that runs kubectl delete daemonset e2e-image-prepuller -n kube-system --ignore-not-found and register it with trap 'cleanup_prepuller' EXIT (and optionally INT TERM) immediately after the kubectl apply, so the existing kubectl rollout status timeout cannot leave orphaned pods.
🤖 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-install-cozystack.bats`:
- Around line 21-27: The pipeline silently swallows failures from the two helm
template commands; to fix it, capture each helm template output into variables
(e.g., render_kubeovn and render_linstor) by assigning their stdout to vars
before piping, so Bash's set -e will fail the test if a helm template fails,
then feed those variables into yq and finally into hack/e2e-prepull-images.sh;
update the block that runs "helm template packages/system/kubeovn" and "helm
template packages/system/linstor" to perform assignments and only proceed to the
yq | hack/e2e-prepull-images.sh pipeline when both assignments succeed.
---
Nitpick comments:
In `@hack/e2e-prepull-images.sh`:
- Around line 77-80: After applying the e2e-image-prepuller DaemonSet, add a
cleanup function and install a trap so the DaemonSet is deleted on every exit
path (including rollout timeout); implement a small function (e.g.,
cleanup_prepuller) that runs kubectl delete daemonset e2e-image-prepuller -n
kube-system --ignore-not-found and register it with trap 'cleanup_prepuller'
EXIT (and optionally INT TERM) immediately after the kubectl apply, so the
existing kubectl rollout status timeout cannot leave orphaned pods.
🪄 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: 6e53384a-2faa-4dda-83c4-3996c8038901
📒 Files selected for processing (2)
hack/e2e-install-cozystack.batshack/e2e-prepull-images.sh
Some workloads (kube-ovn raft, LINSTOR controller / piraeus-server) fail when replicas start at materially different times due to image-pull stagger across nodes. Add a DaemonSet-based pre-pull step that runs before `helm install`, ensuring every node has the images cached so replicas start within milliseconds of each other. The script accepts image refs on stdin and creates one container per image so pulls run in parallel — total time = max(any single image) rather than sum. The bats test sources image refs directly from the rendered platform charts via `yq`, walking only PodSpec-shaped objects, so version bumps stay in sync without a hardcoded list. Signed-off-by: Myasnikov Daniil <[email protected]> Assisted-By: Claude <[email protected]>
0d53f13 to
9df5950
Compare
dosubot
Bot
added
size:L
and removed
size/L
myasnikovdaniil
changed the title
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-prepull-images.sh`:
- Around line 37-43: The DaemonSet currently injects command:
["sleep","infinity"] into each container (see the containers+="... name:
pull-${i} ... command: [\"sleep\",\"infinity\"] ..." block), which makes
readiness depend on the image providing sleep; remove the command and related
readiness dependence and instead detect success by verifying each pulled
container has an imageID (status.containerStatuses[].imageID). Update both
places where the containers block is constructed (the pull-${i} block and the
similar block later) to omit the command and imagePullPolicy logic as needed,
and change the rollout/wait logic to poll kubectl -n kube-system get pods -l
app=e2e-image-prepuller -o json and confirm every pod's
.status.containerStatuses[].imageID is non-empty (or use the jq snippet from the
comment inverted to assert no missing imageIDs) before considering the DaemonSet
ready.
🪄 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: 2df58e3e-f7fa-4127-b8b8-8e20fae3042e
📒 Files selected for processing (2)
hack/e2e-install-cozystack.batshack/e2e-prepull-images.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- hack/e2e-install-cozystack.bats
Aleksei Sviridkin (lexfrei)
left a comment
Aleksei Sviridkin (lexfrei)
left a comment
There was a problem hiding this comment.
LGTM. All bot findings folded into 9df5950:
- Cleanup moved to a
trap … EXITso thee2e-image-prepullerDaemonSet is removed on every exit path (success, rollout timeout, signal) instead of only the success path underset -euo pipefail. - PR title renamed from
e2escope totest(tests):per.gemini/styleguide.md. - The
helm template … | yq … | prepullpipeline now captures each render into a variable first (kubeovn_manifests=$(helm template …)) soset -efires on a render failure — bats deliberately doesn't enablepipefail, so the previous brace-group form would have masked ahelm templateerror behind yq's exit code and silently skipped pre-pull while the test passed. sleep infinitytradeoff is reasonable for the current image set (all three are Debian/Ubuntu-based —linstor-csiondebian:trixie-slim,piraeus-serverondebian:bookworm,kubeovnon Ubuntu — all shipsleepfrom coreutils). If a distroless image ever joins the list,kubectl rollout statustimes out loudly rather than silently passing — acceptable safety net.---filter in the grep dedup step would be dead code given upstreamyq -N(--no-doc) and would mask the bug if-Never got dropped — keeping current shape is correct.
Substance:
- The peer-sensitive workload diagnosis matches the OVN raft and LINSTOR controller behavior — replicas that lose their startup window because of pull stagger consistently hit raft/connection-retry exhaustion. This class of flake also shows up in cnpg-webhook bootstrap and similar peer-sensitive paths. The DaemonSet approach (one container per image so pulls go in parallel — total time = max image, not sum) is the right shape and avoids deps like kube-fledged.
hostNetwork: trueis correct: pre-pull runs before kube-ovn installs, so a normal pod staysContainerCreatingwithNetworkPluginNotReadyand never reaches image-pull. Mirroring the cozystack-operator's pattern.tolerations: [operator: Exists]reaches every node including control-plane and nodes still coming up — required because pre-pull happens during cluster bootstrap when nodes still carry not-ready taints.- Image discovery via
helm template … | yq '.. | select(has("containers")) | .containers[].image'keeps version bumps automatically synced — no hardcoded list to drift againstmake update. automountServiceAccountToken: falseon the DaemonSet's pod spec — good hygiene for a pod that just sleeps.
Two non-blocking observations, neither requiring a change:
- The script's
cpu: 1m / memory: 1Mirequests with no limits are fine for a sleeping process. Worth a one-line comment in the YAML so a future maintainer doesn't pattern-match this to "unset limits = bug" and add limits that don't help. kubectl rollout status daemonset/… --timeout=10mis generous; in practice the timeout is bounded by the slowest registry. If oracle-runner GHCR pulls regress further, a--timeout=15mbump might be needed before this test starts flaking on its own. Leave alone for now.
github-actions
Bot
added
the
size/L
2 participants
What this PR does
Some workloads (kube-ovn raft, LINSTOR controller / piraeus-server) fail when replicas start at materially different times due to image-pull stagger across nodes. Adds a DaemonSet-based pre-pull step that runs before
helm install, ensuring every node has the images cached so replicas start within milliseconds of each other.hack/e2e-prepull-images.shaccepts image refs on stdin and creates one container per image so pulls run in parallel — total time = max(any single image) rather than sum. The bats test sources image refs directly from the rendered platform charts viayq, walking only PodSpec-shaped objects, so version bumps stay in sync without a hardcoded list.Surfaced from #2500.
Release note
Summary by CodeRabbit
Tests
Chores