Skip to content

test(e2e): pre-pull platform images via staged-busybox DaemonSet - #2724

Merged
myasnikovdaniil merged 3 commits into
mainfrom
ci/e2e-prepull
May 26, 2026
Merged

test(e2e): pre-pull platform images via staged-busybox DaemonSet#2724
myasnikovdaniil merged 3 commits into
mainfrom
ci/e2e-prepull

Conversation

@myasnikovdaniil

@myasnikovdaniil myasnikovdaniil commented May 25, 2026

Copy link
Copy Markdown
Contributor

What this PR does

Pre-pulls every image referenced by packages/system/kubeovn, packages/system/linstor, and packages/system/cert-manager onto every node before those HelmReleases install. Cluster-member workloads (OVN raft, LINSTOR, cert-manager webhook) fail when replicas start at different times due to per-node image-pull stagger; pre-pulling means all replicas start with images already cached.

Mechanics

Renders each chart with helm template, then runs a yq filter that walks every PodSpec-shaped object and emits the images of each container — scoping the output to images the kubelet actually pulls (skips configmap fields and CRD examples that happen to contain an image: key). Each render is staged through a tmp file so a helm-template failure trips set -e cleanly (cozytest.sh runs @test bodies under /bin/sh, which is dash on Ubuntu CI — no pipefail).

The pre-pull DaemonSet itself is the hard part: distroless images (cert-manager and friends) ship no shell and no /bin/sleep, so the obvious command: ["sleep", "infinity"] fails with exec: ENOENT. The script stages a statically-linked busybox:musl into a shared emptyDir from an initContainer; every prepull container then execs /shared/sleep regardless of what the image itself ships. The glibc busybox tag is unusable — it needs /lib64/ld-linux-x86-64.so.2 which distroless lacks.

Origin

Lifted from #2619 — squash of test(e2e): pre-pull cert-manager images → POSIX rewrite → cert-manager removal (failed on distroless) → reintroduction with the busybox helper that solves the distroless problem. Final state only — the four iterative commits aren't review-friendly individually.

Verification

  • sh -n hack/e2e-prepull-images.sh clean
  • All chart-referenced images held Ready under the DaemonSet on local kind v1.33.1 single-node

Release note

NONE

Summary by CodeRabbit

  • Tests

    • Improved end-to-end test reliability for platform image pre-pulling by rendering charts separately and consolidating extracted images.
    • Simplified and consolidated readiness waits during tenant configuration for more robust application readiness checks.
  • Chores

    • Enhanced image pre-pulling to reliably handle distroless images using a staged helper binary and shared volume approach.

Review Change Stack

Cluster-member workloads (OVN raft, LINSTOR, cert-manager) fail when
replicas start at different times due to per-node image-pull stagger.
This test pre-pulls the images that those charts reference onto every
node before the platform HelmReleases install, so all replicas can
start with images already cached.

Mechanics:

- Source images directly from the rendered charts (kubeovn, linstor,
  cert-manager) via `helm template` + a `yq` filter that walks every
  PodSpec-shaped object and emits each container's image. Scopes the
  output to images the kubelet actually pulls — skips configmap fields
  and CRD examples that happen to contain an `image:` key. Each render
  is staged through a tmp file so a helm-template failure trips `set -e`
  cleanly without needing `pipefail` (cozytest.sh runs @test bodies
  under /bin/sh, which is dash on Ubuntu CI), and without capturing
  rendered YAML into a shell variable that `set -x` would expand and
  balloon the CI trace.

- The pre-pull DaemonSet is the hard part: distroless images
  (cert-manager and friends) ship no shell and no /bin/sleep, so the
  obvious `command: ["sleep", "infinity"]` fails with exec ENOENT.
  Stage a statically-linked busybox:musl into a shared emptyDir from
  an initContainer; every prepull container then execs /shared/sleep
  regardless of what the image itself ships. The glibc busybox tag is
  unusable here — it needs /lib64/ld-linux-x86-64.so.2 which distroless
  lacks. Loop is written in POSIX shell (no bash arrays) for the same
  /bin/sh-is-dash reason.

Verified locally on kind v1.33.1 single-node: every chart-referenced
image is held Ready under the DaemonSet before the platform HelmRelease
applies.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Summary of Changes

Hello, 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 introduces a pre-pulling strategy for critical system images in the Cozystack e2e test suite. By ensuring images are cached on all nodes before HelmReleases are deployed, it eliminates startup synchronization issues that previously caused intermittent installation timeouts. The implementation includes a clever workaround for distroless images by injecting a static binary via an initContainer, and improves the robustness of the test scripts by avoiding unreliable shell pipes.

Highlights

  • Image Pre-pulling: Implemented a robust pre-pulling mechanism for system images using a DaemonSet to ensure all replicas start simultaneously, preventing installation timeouts caused by staggered image pulls.
  • Distroless Support: Added an initContainer that stages a statically-linked busybox binary into a shared volume, allowing containers based on distroless images to execute a sleep command without requiring a shell or system libraries.
  • Test Reliability: Refactored the BATS test suite to improve error handling by avoiding pipe-based commands, ensuring that failures in helm template rendering are correctly caught by set -e.
New Features

🧠 You can now enable Memory (public preview) 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 Assist

The 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 /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands on the current page.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment Gemini (@gemini-code-assist) Responds in comments when explicitly tagged, both in pull request comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize the 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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. 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

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

@github-actions github-actions Bot added the size/M This PR changes 30-99 lines, ignoring generated files label May 25, 2026
@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

E2e image-prepull now renders kube-ovn, linstor, and cert-manager separately and extracts images for a prepull DaemonSet that stages a static sleep via an initContainer; tenant readiness checks are consolidated into a single kubectl wait for multiple HelmReleases (10m timeout).

Changes

E2E Testing Infrastructure

Layer / File(s) Summary
Enhanced image prepull with distroless handling
hack/e2e-install-cozystack.bats, hack/e2e-prepull-images.sh
Render kube-ovn, linstor, and cert-manager to temp files; extract container and initContainer images via yq into a combined images_list piped to hack/e2e-prepull-images.sh. Prepull script now generates a DaemonSet that uses an emptyDir at /shared and an initContainer (busybox:musl) to stage a static /shared/sleep, and runs prepull containers with /shared/sleep infinity. Comments and pod spec updated accordingly.
Consolidated tenant readiness waiting
hack/e2e-install-cozystack.bats
Replace multiple sequential kubectl wait commands with a single kubectl wait covering hr/etcd, hr/ingress, hr/monitoring, hr/seaweedfs, and hr/tenant-root using a 10m timeout.

Sequence Diagram

sequenceDiagram
  participant Test as e2e-install test
  participant YQ as yq extractor
  participant Prepull as e2e-prepull-images.sh
  participant DS as generated DaemonSet
  participant Init as busybox:musl initContainer

  Test->>Test: helm template -> temp files (kube-ovn, linstor, cert-manager)
  Test->>YQ: extract container + initContainer images
  YQ->>Prepull: write images_list
  Prepull->>DS: generate DaemonSet spec (mount /shared, run /shared/sleep)
  DS->>Init: initContainer copies /bin/busybox -> /shared/sleep
  DS->>DS: mount /shared and run /shared/sleep in containers
Loading

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • cozystack/cozystack#2615: Similar changes to image prepull rendering to use separate Helm chart temp files and consolidate yq-based image extraction piped into the prepull script.

Suggested labels

size/S

Suggested reviewers

  • lexfrei
  • androndo
  • sircthulhu
  • kvaps
  • lllamnyp
  • IvanHunters

Poem

🐰 Charts rendered tidy, three by three,

Sleep staged in volumes, distroless runs free,
A single wait now watches apps align,
Tests hum along, all checks in line —
Hooray, CI carrots for all to see! 🥕

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately captures the main change: a new e2e test helper that pre-pulls platform images using a staged-busybox DaemonSet approach, which is the primary focus of both modified files.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ci/e2e-prepull

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@dosubot dosubot Bot added the area/ci Issues or PRs related to CI workflows, GitHub Actions, automation label May 25, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds cert-manager to the pre-pulling process and updates the e2e-prepull-images.sh script to support distroless images by staging a static sleep binary via an init container. Additionally, it increases the kubectl wait timeout for the tenant-root HelmRelease to 10 minutes to accommodate serial dependencies. Feedback was provided regarding the use of pipes in the bats test script, which could mask command failures since pipefail is not supported in that environment.

Comment thread hack/e2e-install-cozystack.bats Outdated
Comment on lines +34 to +38
cat "$kubeovn_yaml" "$linstor_yaml" "$certmanager_yaml" | yq -N '
(..|select(has("containers"))|.containers[]|.image),
(..|select(has("initContainers"))|.initContainers[]|.image)
' "$kubeovn_yaml" "$linstor_yaml" > "$images_list"
hack/e2e-prepull-images.sh < "$images_list"
rm -f "$kubeovn_yaml" "$linstor_yaml" "$images_list"
' | hack/e2e-prepull-images.sh
rm -f "$kubeovn_yaml" "$linstor_yaml" "$certmanager_yaml"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

The use of a pipe here re-introduces the exit code masking issue that the comment on lines 21-26 specifically warns about. Since pipefail is not available in the bats environment (which runs under dash on Ubuntu CI), a failure in the yq command will not be caught, and the test might proceed even if image extraction fails. Additionally, cat is redundant as yq can accept multiple file arguments directly. Staging the output to a temporary file ensures that any extraction errors are caught by set -e.

  local images_list=$(mktemp)
  yq -N '
      (..|select(has("containers"))|.containers[]|.image),
      (..|select(has("initContainers"))|.initContainers[]|.image)
    ' "$kubeovn_yaml" "$linstor_yaml" "$certmanager_yaml" > "$images_list"
  hack/e2e-prepull-images.sh < "$images_list"
  rm -f "$kubeovn_yaml" "$linstor_yaml" "$certmanager_yaml" "$images_list"

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch — reverted to the tmp-file pattern that was on this file before the PR (the older comment block also explained this trap). Now yq -N ... > "$images_list" writes to a staged file and hack/e2e-prepull-images.sh < "$images_list" reads it, so set -e trips on a yq failure instead of letting prepull's empty-stdin no-op mask it. Also dropped the cat. Fixed in 2ace581.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
hack/e2e-prepull-images.sh (1)

83-84: ⚡ Quick win

Pin busybox:musl by digest to stabilize CI behavior.

Line 83-84 depends on a mutable tag for the staged binary. Pinning digest avoids unexpected breakage if upstream musl tag content changes.

Proposed fix
-      - name: stage-sleep
-        image: busybox:musl
+      - name: stage-sleep
+        image: busybox:musl@sha256:<verified-digest>
🤖 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-prepull-images.sh` around lines 83 - 84, Replace the mutable image
tag "busybox:musl" used in the pod spec that runs command ["cp", "/bin/busybox",
"/shared/sleep"] with an immutable digest-pinned reference (e.g.
busybox@sha256:<digest>) to stabilize CI; locate the occurrence of the image
string "busybox:musl" in the e2e-prepull script and update it to the
corresponding digest-pinned image (fetch the current stable digest for the musl
variant and substitute it).
🤖 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-install-cozystack.bats`:
- Around line 149-150: The grouped kubectl wait mistakenly relaxed hr/monitoring
to 10m; split the waits so hr/monitoring is waited on with a tighter timeout
(e.g., run kubectl wait hr/monitoring -n tenant-root --timeout=2m
--for=condition=ready) and remove hr/monitoring from the later grouped wait
(keep kubectl wait hr/etcd hr/ingress hr/seaweedfs hr/tenant-root -n tenant-root
--timeout=10m --for=condition=ready) so monitoring stays a strict gate.
- Around line 34-37: The pipeline that feeds yq into hack/e2e-prepull-images.sh
can hide yq failures because pipe failures aren't propagated; enable failure
propagation by turning on pipefail (e.g., add set -o pipefail near the top of
hack/e2e-install-cozystack.bats) or capture yq output to a temp file and check
yq's exit code before invoking hack/e2e-prepull-images.sh, ensuring the yq
command that extracts images (the yq -N
'(..|select(has("containers"))|.containers[]|.image),
(..|select(has("initContainers"))|.initContainers[]|.image)') must succeed
otherwise abort and fail the test.

---

Nitpick comments:
In `@hack/e2e-prepull-images.sh`:
- Around line 83-84: Replace the mutable image tag "busybox:musl" used in the
pod spec that runs command ["cp", "/bin/busybox", "/shared/sleep"] with an
immutable digest-pinned reference (e.g. busybox@sha256:<digest>) to stabilize
CI; locate the occurrence of the image string "busybox:musl" in the e2e-prepull
script and update it to the corresponding digest-pinned image (fetch the current
stable digest for the musl variant and substitute it).
🪄 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: eaf79a3f-7d80-4d4f-8e68-428496f939f5

📥 Commits

Reviewing files that changed from the base of the PR and between 1810263 and 000d516.

📒 Files selected for processing (2)
  • hack/e2e-install-cozystack.bats
  • hack/e2e-prepull-images.sh

Comment thread hack/e2e-install-cozystack.bats Outdated
Comment on lines +149 to +150
kubectl wait hr/etcd hr/ingress hr/monitoring hr/seaweedfs hr/tenant-root \
-n tenant-root --timeout=10m --for=condition=ready

@coderabbitai coderabbitai Bot May 25, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

hr/monitoring timeout got unintentionally relaxed to 10m.

Line 149-150 groups hr/monitoring into a 10m wait, which weakens the intended tight monitoring gate and may hide operator regressions.

Proposed fix
-  kubectl wait hr/etcd hr/ingress hr/monitoring hr/seaweedfs hr/tenant-root \
-    -n tenant-root --timeout=10m --for=condition=ready
+  kubectl wait hr/monitoring -n tenant-root --timeout=2m --for=condition=ready
+  kubectl wait hr/etcd hr/ingress hr/seaweedfs hr/tenant-root \
+    -n tenant-root --timeout=10m --for=condition=ready

Based on learnings: In cozystack e2e Bats tests under hack/*.bats, keep kubectl wait timeout for hr/monitoring intentionally tight and do not increase it to mask intermittent failures.

📝 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.

Suggested change
kubectl wait hr/etcd hr/ingress hr/monitoring hr/seaweedfs hr/tenant-root \
-n tenant-root --timeout=10m --for=condition=ready
kubectl wait hr/monitoring -n tenant-root --timeout=2m --for=condition=ready
kubectl wait hr/etcd hr/ingress hr/seaweedfs hr/tenant-root \
-n tenant-root --timeout=10m --for=condition=ready
🤖 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-install-cozystack.bats` around lines 149 - 150, The grouped kubectl
wait mistakenly relaxed hr/monitoring to 10m; split the waits so hr/monitoring
is waited on with a tighter timeout (e.g., run kubectl wait hr/monitoring -n
tenant-root --timeout=2m --for=condition=ready) and remove hr/monitoring from
the later grouped wait (keep kubectl wait hr/etcd hr/ingress hr/seaweedfs
hr/tenant-root -n tenant-root --timeout=10m --for=condition=ready) so monitoring
stays a strict gate.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keeping the grouped 10m intentionally. Monitoring is bundled with the other top-level tenant-root children because the parent HR only flips Ready after every child does, and the 10m bound is set by the seaweedfs serial chain (seaweedfs-dbseaweedfs-systemseaweedfs) which pushes that flip to ~5–6 min. Splitting monitoring back into a 2m wait would not change the total wall-clock the test spends here (it would still block on seaweedfs afterwards) and the value of a tight monitoring gate isn't worth the extra command in this group. We can revisit if a monitoring regression actually slips through the 10m bound.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@myasnikovdaniil, that's a well-reasoned justification — since tenant-root only flips Ready after all children do, and the seaweedfs serial chain already forces a ~5–6 min wait regardless, splitting hr/monitoring out into a 2m gate adds no real safety without changing the total blocking time. Accepted.

I'll update the long-term memory to reflect this intentional design.


✏️ Learnings added
Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2724
File: hack/e2e-install-cozystack.bats:153-154
Timestamp: 2026-05-26T11:02:28.790Z
Learning: In `hack/e2e-install-cozystack.bats` (cozystack/cozystack), the `kubectl wait` for `hr/monitoring` in the "Configure Tenant and wait for applications" test is intentionally grouped with `hr/etcd`, `hr/ingress`, `hr/seaweedfs`, and `hr/tenant-root` under a single 10m timeout. The 10m bound is driven by the seaweedfs serial install chain (seaweedfs-db → seaweedfs-system → seaweedfs, ~5–6 min), and the tenant-root parent HR only flips Ready after every child HR is Ready. Splitting hr/monitoring into a tighter separate wait would not change total wall-clock time and is not warranted. Do not flag this as a regression — the grouped timeout is the intended design.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


⛔ Learnings removed
Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2509
File: hack/e2e-install-cozystack.bats:155-158
Timestamp: 2026-05-05T06:24:47.778Z
Learning: In cozystack e2e Bats tests under hack/*.bats (including the step "Configure Tenant and wait for applications"), keep the `kubectl wait` timeout for `hr/monitoring` at the intentionally tight value (e.g., ~2m) when the test already relies on the operator-side retry behavior (e.g., `Strategy.Name=RetryOnFailure` with `RetryInterval` and an operator Helm release interval override). Do not increase the timeout to “fix” intermittent failures; a longer timeout would hide regressions/mistakes on the operator side. If the wait is failing, treat it as a signal to investigate operator behavior rather than masking it by bumping the timeout (only adjust if you also change the underlying retry/interval logic or operator behavior).

🧠 Learnings used
Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2559
File: hack/select-e2e_test.bats:75-80
Timestamp: 2026-05-02T14:46:54.942Z
Learning: In Bash Automated Testing System (Bats) tests, the `run` helper captures command output and strips the trailing newline from `$output`. Therefore, for single-line commands, exact string assertions like `[ "$output" = "redis" ]` should be safe and should not be tripped up by trailing newline/whitespace. No extra `xargs`-style trimming is needed solely to remove a trailing newline for single-line outputs.

Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2509
File: hack/e2e-install-cozystack.bats:155-158
Timestamp: 2026-05-05T06:24:47.778Z
Learning: In cozystack e2e Bats tests under hack/*.bats (including the step "Configure Tenant and wait for applications"), keep the `kubectl wait` timeout for `hr/monitoring` at the intentionally tight value (e.g., ~2m) when the test already relies on the operator-side retry behavior (e.g., `Strategy.Name=RetryOnFailure` with `RetryInterval` and an operator Helm release interval override). Do not increase the timeout to “fix” intermittent failures; a longer timeout would hide regressions/mistakes on the operator side. If the wait is failing, treat it as a signal to investigate operator behavior rather than masking it by bumping the timeout (only adjust if you also change the underlying retry/interval logic or operator behavior).

Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2509
File: hack/e2e-install-cozystack.bats:155-158
Timestamp: 2026-05-05T06:24:47.778Z
Learning: In cozystack/cozystack E2E Bats scripts under hack/*.bats, add an “existence backstop” before any kubectl wait call. Specifically, guard kubectl wait by first polling for the target resource to exist (e.g., using `timeout 60 sh -ec 'until kubectl get <resource> ... >/dev/null 2>&1; do sleep 2; done'`) so kubectl wait doesn’t fail when the resource hasn’t been created yet. Prefer the repository’s established backstop pattern rather than ad-hoc one-off variations.

Learnt from: myasnikovdaniil
Repo: cozystack/cozystack PR: 2554
File: hack/e2e-install-cozystack.bats:289-291
Timestamp: 2026-05-05T08:35:37.488Z
Learning: In cozystack e2e Bats tests, Tenant CR–backed resources (e.g., `hr/tenant-test` and the corresponding `HelmRelease` objects for other Tenant CRs) are created asynchronously by `cozystack-controller` after the Tenant CR is applied; they are not rendered by a parent Helm chart. Because of this, there’s a real race between `kubectl apply` of the Tenant and the controller materializing the `HelmRelease`/`hr` resources. Before running `kubectl wait` on those HelmReleases in tests, add an existence backstop by polling until the expected resource exists (e.g., `timeout 60 sh -ec 'until kubectl get hr/tenant-test -n tenant-root >/dev/null 2>&1; do sleep 2; done'`).

Without pipefail (dash-CI), `yq | hack/e2e-prepull-images.sh` would let a
yq failure produce empty input; prepull-images.sh exits 0 on no-input
and the test passes with pre-pull silently skipped. Stage the yq output
to a tmp file and feed it via stdin so set -e trips on a yq failure.

Assisted-By: Claude <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

♻️ Duplicate comments (1)
hack/e2e-install-cozystack.bats (1)

153-154: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Keep hr/monitoring as a strict short-timeout gate.

Line 153 currently waits hr/monitoring with the shared 10m timeout, which weakens the dedicated monitoring regression signal.

Suggested adjustment
-  kubectl wait hr/etcd hr/ingress hr/monitoring hr/seaweedfs hr/tenant-root \
-    -n tenant-root --timeout=10m --for=condition=ready
+  kubectl wait hr/monitoring -n tenant-root --timeout=2m --for=condition=ready
+  kubectl wait hr/etcd hr/ingress hr/seaweedfs hr/tenant-root \
+    -n tenant-root --timeout=10m --for=condition=ready

Based on learnings: In cozystack e2e Bats tests under hack/*.bats, keep kubectl wait timeout for hr/monitoring intentionally tight and do not increase it to mask intermittent failures.

🤖 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-install-cozystack.bats` around lines 153 - 154, The shared kubectl
wait combines hr/monitoring with a 10m timeout which dilutes the short-timeout
gate; split the waits so hr/monitoring is waited on separately with a
deliberately short timeout (e.g. 30s or whatever your regression gate requires)
while keeping the existing kubectl wait for hr/etcd hr/ingress hr/seaweedfs
hr/tenant-root with --timeout=10m; update the kubectl wait invocation(s) so the
symbol hr/monitoring appears in its own kubectl wait command and retains the
tight timeout.
🤖 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.

Duplicate comments:
In `@hack/e2e-install-cozystack.bats`:
- Around line 153-154: The shared kubectl wait combines hr/monitoring with a 10m
timeout which dilutes the short-timeout gate; split the waits so hr/monitoring
is waited on separately with a deliberately short timeout (e.g. 30s or whatever
your regression gate requires) while keeping the existing kubectl wait for
hr/etcd hr/ingress hr/seaweedfs hr/tenant-root with --timeout=10m; update the
kubectl wait invocation(s) so the symbol hr/monitoring appears in its own
kubectl wait command and retains the tight timeout.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: d9243699-2e49-4c98-9317-b985f0a187eb

📥 Commits

Reviewing files that changed from the base of the PR and between 000d516 and 2ace581.

📒 Files selected for processing (1)
  • hack/e2e-install-cozystack.bats

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Pre-pull script is correct, the bot-flagged issues are addressed in 2ace58111, and the consolidation of the tenant-root waits is defensible — with one non-blocking observation about the dropped explicit seaweedfs-system wait.

Verified:

  • packages/system/cert-manager exists as a real package directory, so the new helm template packages/system/cert-manager render is well-formed.
  • The yq-pipefail fix (2ace581117) restructures the pipeline into discrete commands (helm template ... > tmp_a, helm template ... > tmp_b, helm template ... > tmp_c, yq -N '...' a b c > images_list, hack/e2e-prepull-images.sh < images_list) — no multi-stage pipe, so set -e cleanly catches a yq failure without needing pipefail. CodeRabbit's confirmation comment matches.
  • The busybox-musl staging trick is correct: distroless images ship neither /bin/sh nor /bin/sleep, and busybox's glibc tag needs /lib64/ld-linux-x86-64.so.2 (also absent on distroless), so busybox:musl (statically linked) into a shared emptyDir via an initContainer is the standard cgroup-compatible way to keep a distroless container alive across kubectl rollout status.
  • hostNetwork: true on the prepull DaemonSet is needed because the script runs BEFORE kube-ovn installs, so CNI isn't available yet; a normal pod would stay ContainerCreating with NetworkPluginNotReady. The comment in the script explains this clearly.
  • cleanup() trap on EXIT deletes the DaemonSet on either success or failure, so a flaky run doesn't leak the DS into the next bats run.
  • mapfile -t images < <(grep -Ev '^[[:space:]]*(#|$)' | sort -u) correctly strips blanks and comments and dedupes; the early-exit on empty image list short-circuits cleanly.
  • All chart-referenced images for kubeovn/linstor/cert-manager appear in the rendered output as expected (verified by spot-checking the yq filter form (..|select(has("containers"))|.containers[]|.image), (..|select(has("initContainers"))|.initContainers[]|.image) — that's the right shape for picking real PodSpec images while skipping CRD examples or configmap fields that happen to contain an image: key).
  • CI: Build + E2E both pass on this SHA.

Bot threads — both already addressed:

  1. yq pipe pipefail trap (gemini-medium / coderabbit-major) — addressed by the 2ace58111 rewrite. CodeRabbit's follow-up explicitly acknowledges the fix.
  2. hr/monitoring relaxed to 10m (coderabbit-major) — Daniil's response explains that tenant-root parent HR only flips Ready after every child HR is Ready, the seaweedfs serial chain (seaweedfs-db → seaweedfs-system → seaweedfs wrapper) pushes the parent's Ready flip to ~5–6 min anyway, and tenant-root HR.spec.timeout is 15m so the 10m wait stays inside the budget. CodeRabbit accepted (<!-- <review_comment_addressed> -->). The chart-side comment block in the bats file documents the same reasoning inline.

Non-blocking observation:

Dropped explicit hr/seaweedfs-system wait. Pre-PR the bats waited on hr/seaweedfs-system in a separate 2m gate; post-PR it relies on the cascade from hr/seaweedfs (and from hr/tenant-root) to surface a stuck seaweedfs-system. packages/extra/seaweedfs/templates/seaweedfs.yaml:118 does render seaweedfs-system as a child HR — name: {{ .Release.Name }}-system in the same tenant-root namespace — and under Flux v2.8 kstatus-based health checks the parent hr/seaweedfs Ready cascades through children, so Daniil's framing is technically correct. But the wait change replaces an explicit assertion ("the actual SeaweedFS deployment HR is Ready") with an implicit assumption ("the wrapper's Ready means the inner HR is too"). If a future Flux change ever de-couples that cascade — or if seaweedfs-system's dependsOn: ingress-nginx-system deadlocks on a configuration the test doesn't hit — the suite would advance with seaweedfs-system not actually healthy. The trivial defense-in-depth is to add hr/seaweedfs-system to the consolidated wait list (kubectl wait hr/etcd hr/ingress hr/monitoring hr/seaweedfs hr/seaweedfs-system hr/tenant-root ...); zero extra wall time when the cascade does work, and an explicit failure when it doesn't. Not gating this PR — fine to land as-is and pin defensively in a follow-up if a seaweedfs-system regression ever sneaks past.

@myasnikovdaniil
myasnikovdaniil enabled auto-merge May 26, 2026 11:25
Empty commit to fire the Pull Request workflow, which never ran for
this commit because GitHub Actions was down during the original push.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Signed-off-by: Myasnikov Daniil <myasnikovdaniil2001@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/ci Issues or PRs related to CI workflows, GitHub Actions, automation area/testing Issues or PRs related to testing (e2e, bats, unit tests) size/M This PR changes 30-99 lines, ignoring generated files

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants