test(e2e): improve reliability, parallelism, and CI caching - #995
Open
bennyz wants to merge 6 commits into
Open
Conversation
bennyz
commented
Aug 10, 2026
bennyz
commented
Member
- Pin nip.io hostnames in /etc/hosts so DNS flakes don't burn the connect budget
- Fix kubectl query helpers to not treat stderr as output (was ending Eventually polls early)
- Add retry, continue-on-failure, and broader hook failure matching to reduce spurious failures
- Cut fixture overhead: single kubectl apply for pagination, reuse hooks exporter when config matches
- Enable ginkgo --procs=2: graceful SIGTERM lifecycle, argv-scoped orphan sweep, --client instead of config client use
- Cache Go modules, controller tools, and load kind images concurrently
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR updates CI caching and E2E execution controls. It adds retries and parallelism, improves environment setup and fixture isolation, strengthens Kubernetes polling, and adds tracked exporter process cleanup. ChangesE2E reliability and CI execution
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant E2ETest
participant ProcessTracker
participant KubectlQuery
participant Exporter
E2ETest->>ProcessTracker: Start or reuse exporter
ProcessTracker->>Exporter: Track process identity and PID
E2ETest->>KubectlQuery: Check exporter status and lease
KubectlQuery-->>E2ETest: Return readiness or offline state
E2ETest->>ProcessTracker: Stop exporter
ProcessTracker->>Exporter: Send SIGTERM, then SIGKILL if required
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/test/utils.go (1)
353-362: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReturn only stdout from
MustKubectlApply.
MustKubectlApplywrites stdout and stderr into one buffer and returns that buffer. Any kubectl warning on stderr becomes part of the returned value. This is the same failure mode that theKubectlQuerycomment below describes. Keep the streams separate, return stdout, and put stderr in the failure message.♻️ Proposed refactor
func MustKubectlApply(manifest string) string { cmd := exec.Command("kubectl", "-n", Namespace(), "apply", "-f", "-") cmd.Stdin = strings.NewReader(manifest) - var out bytes.Buffer + var out, errOut bytes.Buffer cmd.Stdout = &out - cmd.Stderr = &out + cmd.Stderr = &errOut err := cmd.Run() - ExpectWithOffset(1, err).NotTo(HaveOccurred(), "kubectl apply failed: %s", out.String()) + ExpectWithOffset(1, err).NotTo(HaveOccurred(), + "kubectl apply failed: %s%s", out.String(), errOut.String()) return strings.TrimSpace(out.String()) }🤖 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 `@e2e/test/utils.go` around lines 353 - 362, Update MustKubectlApply to capture stdout and stderr in separate buffers, return only trimmed stdout, and include the trimmed stderr alongside the existing kubectl apply failure message when cmd.Run fails.
🤖 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 `@e2e/setup-e2e.sh`:
- Around line 297-300: Update the /etc/hosts guard in the setup function to
verify grpc.${basedomain}, router.${basedomain}, and login.${basedomain} are all
present before returning. If any required alias is missing, append only the
missing entries while preserving the existing log and success behavior when all
are available.
In `@e2e/test/hooks_test.go`:
- Around line 50-68: Update startHooksExporter so runningConfig is cleared
before tracker.StopAll() when replacing the exporter, ensuring failed startup or
readiness waits cannot leave the previous configuration marked as active; keep
the existing reuse branch and assign the new config only after WaitForExporter
succeeds.
In `@e2e/test/utils.go`:
- Around line 470-488: Add a mutex field to ProcessTracker and use it to
synchronize all accesses to pids and specs: lock track while appending, guard
the reads in TrackedPIDs and sweepOrphans, and lock around StopAll’s pids read
and clear. Ensure each critical section is protected consistently without
holding the lock across unrelated process operations.
---
Nitpick comments:
In `@e2e/test/utils.go`:
- Around line 353-362: Update MustKubectlApply to capture stdout and stderr in
separate buffers, return only trimmed stdout, and include the trimmed stderr
alongside the existing kubectl apply failure message when cmd.Run fails.
🪄 Autofix
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f51af15d-2e02-417b-bc32-11419274aed6
📒 Files selected for processing (17)
.github/workflows/build-images.yaml.github/workflows/e2e.yaml.github/workflows/lint.yaml.github/workflows/release-operator-installer.yamlcontroller/hack/deploy_with_operator.she2e/lib/common.she2e/setup-e2e.she2e/test/auth_logging_test.goe2e/test/compat_old_client_test.goe2e/test/compat_old_controller_test.goe2e/test/direct_listener_test.goe2e/test/dut_network_test.goe2e/test/e2e_test.goe2e/test/exit_on_lease_end_test.goe2e/test/exporterset_qemu_test.goe2e/test/hooks_test.goe2e/test/utils.go
bennyz
force-pushed
the
e2e-reliability
branch
from
August 10, 2026 11:00
796aed7 to
52dc410
Compare
bkhizgiy
reviewed
Aug 12, 2026
| # Retry a failed spec once before failing the job. Retries are reported as | ||
| # flakes in the ginkgo summary, so this hides nothing; it only stops a single | ||
| # infrastructure hiccup from failing an unrelated PR. | ||
| E2E_FLAKE_ATTEMPTS: "2" |
Comment on lines
+347
to
+349
| env: | ||
| KIND_VERSION: v0.27.0 | ||
| KUSTOMIZE_VERSION: v5.4.1 |
Member
There was a problem hiding this comment.
can we somehow define this once in the file?, we have some duplication of it and it will be better in case the version changes to do it once insted track all places.
Member
Author
There was a problem hiding this comment.
added Makefile extraction
| return 0 | ||
| fi | ||
|
|
||
| if grep -q "grpc.${basedomain}" /etc/hosts 2>/dev/null; then |
Member
There was a problem hiding this comment.
Since we're matching a literal hostname here, mayve we should we use grep -Fq instead? -F avoids treating characters like . as regex wildcards and makes the match more precise.
| # nothing to do with the code under test (a slow DNS answer, a pod scheduled | ||
| # late, a router connection dropped). A retried spec is still reported as | ||
| # flaky in the summary, so genuine instability stays visible. | ||
| local flake_attempts="${E2E_FLAKE_ATTEMPTS:-1}" |
Member
There was a problem hiding this comment.
I think we should set it to 2, since ginkgo treats it as the total number of runs rather than the number of retries, otherwise we'll end up with the same behavior as before.
Member
Author
There was a problem hiding this comment.
we have it set to 2 in the workflow yaml, it will default to 1 only for local testing and such
Comment on lines
+761
to
+762
| if proc, err := os.FindProcess(pid); err == nil { | ||
| go func() { _, _ = proc.Wait() }() |
Member
There was a problem hiding this comment.
Do we need another Wait() here? It looks like StartExporter* already waits for these processes, so we might end up with two goroutines trying to reap the same process. Could we rely on the existing one instead?
Member
Author
There was a problem hiding this comment.
it's addressed in the comment on L#756
Host-side clients (jmp, curl) resolve <prefix>.jumpstarter.<IP>.nip.io against a public resolver on every invocation. A slow or rate-limited lookup burns the client's entire connect budget and surfaces as "Timeout connecting to grpc....:8082", which reads like a server fault. The IP is already embedded in the nip.io name, so serve the same answer locally. Mirrors the existing dex /etc/hosts handling. Non-nip.io base domains are left alone. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
RunCmd folds stderr into the returned string. For a value polled inside
Eventually that is wrong: `-o jsonpath={.items[0].metadata.name}` against
an empty list exits non-zero and prints "array index out of bounds", so
Eventually(...).ShouldNot(BeEmpty()) accepts the error text as a result
and stops waiting on the first attempt. A five-minute wait satisfied
itself in 122ms, and the captured error string was then passed to
`kubectl wait exporters.jumpstarter.dev/<error text>`, turning a clean
RBAC denial into an unreadable failure.
Add KubectlQuery, which returns "" when kubectl fails, and use it for the
polled queries. Guard WaitForExporter against being handed a non-name.
Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Retry a failed spec once before failing the job (E2E_FLAKE_ATTEMPTS), add ContinueOnFailure to independent suites so a single failure does not skip all remaining specs, and accept every known beforeLease hook failure outcome to stop spurious failures from race conditions. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Create pagination fixtures with one kubectl apply instead of ten spawned jmp processes, and reuse the hooks exporter when the overlay config is unchanged instead of restarting it every spec. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
…ifecycle Send SIGTERM instead of SIGKILL so the exporter child unregisters from the controller. Scope the orphan sweep to argv-matched processes so parallel ginkgo workers don't reap each other's exporters. Remove the Serial marker from the core suite by using --client instead of config client use. Enable E2E_PROCS=2 in CI. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
Point setup-go cache-dependency-path at the actual go.sum files so module downloads are restored. Cache controller/bin (kind, kustomize, grpcurl) so go-install-tool skips recompilation on cache hit. Load all container images into the kind cluster concurrently. Signed-off-by: Benny Zlotnik <bzlotnik@redhat.com>
bennyz
force-pushed
the
e2e-reliability
branch
from
August 12, 2026 14:11
52dc410 to
f33d4fa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.