Repository navigation
fix(e2e): make kubernetes test retries effective by cleaning up stale resources - #2062
Conversation
dosubot
Bot
added
the
size/S
Summary of ChangesHello Aleksei Sviridkin (@lexfrei), 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 significantly enhances the robustness and reliability of Kubernetes end-to-end tests. By proactively clearing out potentially stale resources before test execution and extending critical wait times, it addresses common causes of flaky test failures, particularly in CI environments. These changes ensure that test retries are effective and that the test suite can better handle varying resource availability. 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. Changelog
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
📝 WalkthroughWalkthroughThis change updates the Kubernetes e2e test script to consistently quote kubeconfig references, add pre-emptive cleanup of backend and NFS resources, extend backend readiness and LoadBalancer wait timeouts, and ensure NFS resources are cleaned on failure and post-test. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 effectively addresses a source of flakiness in the E2E tests by introducing cleanup steps for stale resources before test execution. This ensures that retries start from a clean state. The increase in the deployment wait timeout is also a reasonable adjustment to account for potential resource pressure in CI environments. The added quoting for variables in kubectl commands is a good shell scripting practice that improves robustness. I have a couple of suggestions to improve the debuggability of the new cleanup commands.
| kubectl delete deployment --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | ||
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | ||
| kubectl delete service --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | ||
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true |
There was a problem hiding this comment.
For better debuggability, consider removing 2>/dev/null. The || true already ensures that the script won't exit on failure, but hiding the error message from kubectl can make it harder to diagnose issues if the cleanup fails for a reason other than the resource not existing (e.g., connection issues with the API server). Seeing the error output would be valuable for troubleshooting.
| kubectl delete deployment --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | |
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | |
| kubectl delete service --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | |
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | |
| kubectl delete deployment --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | |
| -n tenant-test --ignore-not-found --timeout=60s || true | |
| kubectl delete service --kubeconfig "tenantkubeconfig-${test_name}" "${test_name}-backend" \ | |
| -n tenant-test --ignore-not-found --timeout=60s || true |
There was a problem hiding this comment.
Done — removed 2>/dev/null from all cleanup commands in the latest force-push. || true is sufficient to prevent script termination.
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod \ | ||
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | ||
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc \ | ||
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true |
There was a problem hiding this comment.
Similar to the cleanup for backend resources, consider removing 2>/dev/null here for better debuggability. The || true already prevents script termination on failure, but showing kubectl's error output could be helpful for troubleshooting if the cleanup fails for reasons other than the resource not being found.
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod \ | |
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | |
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc \ | |
| -n tenant-test --ignore-not-found --timeout=60s 2>/dev/null || true | |
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod \ | |
| -n tenant-test --ignore-not-found --timeout=60s || true | |
| kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc \ | |
| -n tenant-test --ignore-not-found --timeout=60s || true |
There was a problem hiding this comment.
Done — same fix applied here. Removed 2>/dev/null, keeping || true for error suppression with visible diagnostics.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@hack/e2e-apps/run-kubernetes.sh`:
- Around line 224-225: The script mixes quoted and unquoted kubeconfig
arguments; standardize by quoting all occurrences of
tenantkubeconfig-${test_name} passed to kubectl (e.g., in the kubectl delete
deployment/service commands and other kubectl calls referencing --kubeconfig
tenantkubeconfig-${test_name}) so replace unquoted uses with "--kubeconfig"
"tenantkubeconfig-${test_name}" or keep the single token "--kubeconfig
tenantkubeconfig-${test_name}" but ensure the tenantkubeconfig-${test_name}
value is wrapped in quotes everywhere to match the newly modified lines and
avoid inconsistent styling.
🧹 Nitpick comments (1)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh`: - Around line 224-225: The script mixes quoted and unquoted kubeconfig arguments; standardize by quoting all occurrences of tenantkubeconfig-${test_name} passed to kubectl (e.g., in the kubectl delete deployment/service commands and other kubectl calls referencing --kubeconfig tenantkubeconfig-${test_name}) so replace unquoted uses with "--kubeconfig" "tenantkubeconfig-${test_name}" or keep the single token "--kubeconfig tenantkubeconfig-${test_name}" but ensure the tenantkubeconfig-${test_name} value is wrapped in quotes everywhere to match the newly modified lines and avoid inconsistent styling.hack/e2e-apps/run-kubernetes.sh (1)
224-225: Inconsistent kubeconfig quoting across the script.The new/modified lines properly quote
"tenantkubeconfig-${test_name}", but the unchangedkubectlcalls (e.g., lines 224-225, 250, 253, 275, 278, 287-288) still use the unquoted form. Not a bug in practice, but you could unify the style while you're here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh` around lines 224 - 225, The script mixes quoted and unquoted kubeconfig arguments; standardize by quoting all occurrences of tenantkubeconfig-${test_name} passed to kubectl (e.g., in the kubectl delete deployment/service commands and other kubectl calls referencing --kubeconfig tenantkubeconfig-${test_name}) so replace unquoted uses with "--kubeconfig" "tenantkubeconfig-${test_name}" or keep the single token "--kubeconfig tenantkubeconfig-${test_name}" but ensure the tenantkubeconfig-${test_name} value is wrapped in quotes everywhere to match the newly modified lines and avoid inconsistent styling.
… resources When the kubernetes E2E test fails at the deployment wait step, set -eu causes immediate exit before cleanup. On retry, kubectl apply outputs "unchanged" for the stuck deployment, making retries 2 and 3 guaranteed to fail against the same stuck pod. Add pre-creation cleanup of backend deployment/service and NFS test resources using --ignore-not-found, so retries start fresh. Also increase the deployment wait timeout from 90s to 300s to handle CI resource pressure, aligning with other timeouts in the same function. Co-Authored-By: Claude <[email protected]> Signed-off-by: Aleksei Sviridkin <[email protected]>
Quote all tenantkubeconfig-${test_name} references in run-kubernetes.sh
for consistent shell scripting style. The only exception is line 195
inside a sh -ec "..." double-quoted string where inner quotes would
break the outer quoting.
Co-Authored-By: Claude <[email protected]>
Signed-off-by: Aleksei Sviridkin <[email protected]>
97e6b74 to
a52da8d
Compare
dosubot
Bot
added
size/M
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@hack/e2e-apps/run-kubernetes.sh`:
- Around line 194-199: The timeout subshell calling kubectl get svc uses an
unquoted kubeconfig path (--kubeconfig tenantkubeconfig-${test_name}); update
that invocation (the timeout ... sh -ec block that runs kubectl get svc
${test_name}-backend) to quote the kubeconfig argument so it becomes
--kubeconfig "tenantkubeconfig-${test_name}" to match the rest of the script and
avoid word-splitting for test_name values with spaces or special chars.
- Around line 224-225: The success-path cleanup uses kubectl delete for the
backend deployment and service (commands invoking kubectl delete deployment
"${test_name}-backend" and kubectl delete service "${test_name}-backend"), but
unlike the pre-creation cleanup it doesn't tolerate missing resources; update
both kubectl delete commands to be defensive by adding --ignore-not-found (or
append || true) so failures won't abort the remaining NFS cleanup and final
teardown.
- Around line 287-288: Update the success-path NFS cleanup commands so failures
don't abort the script: append "|| true" (or use "--ignore-not-found") to the
kubectl delete calls for the pod and pvc—specifically modify the `kubectl
--kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod -n
tenant-test` and `kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete
pvc nfs-test-pvc -n tenant-test` commands to tolerate non-fatal failures (e.g.,
`... delete pod ... || true` and `... delete pvc ... || true`).
🧹 Nitpick comments (3)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh`: - Around line 194-199: The timeout subshell calling kubectl get svc uses an unquoted kubeconfig path (--kubeconfig tenantkubeconfig-${test_name}); update that invocation (the timeout ... sh -ec block that runs kubectl get svc ${test_name}-backend) to quote the kubeconfig argument so it becomes --kubeconfig "tenantkubeconfig-${test_name}" to match the rest of the script and avoid word-splitting for test_name values with spaces or special chars. - Around line 224-225: The success-path cleanup uses kubectl delete for the backend deployment and service (commands invoking kubectl delete deployment "${test_name}-backend" and kubectl delete service "${test_name}-backend"), but unlike the pre-creation cleanup it doesn't tolerate missing resources; update both kubectl delete commands to be defensive by adding --ignore-not-found (or append || true) so failures won't abort the remaining NFS cleanup and final teardown. - Around line 287-288: Update the success-path NFS cleanup commands so failures don't abort the script: append "|| true" (or use "--ignore-not-found") to the kubectl delete calls for the pod and pvc—specifically modify the `kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod -n tenant-test` and `kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc -n tenant-test` commands to tolerate non-fatal failures (e.g., `... delete pod ... || true` and `... delete pvc ... || true`).hack/e2e-apps/run-kubernetes.sh (3)
194-199: Unquoted kubeconfig path in LoadBalancer wait subshell.The kubeconfig path is quoted everywhere else in this PR, but this
timeoutsubshell on line 195 still uses unquoted--kubeconfig tenantkubeconfig-${test_name}. This is pre-existing, but since you're doing a quoting consistency pass it would be easy to fix here too.Suggested fix
# Wait for LoadBalancer to be provisioned (IP or hostname) timeout 90 sh -ec " - until kubectl get svc ${test_name}-backend --kubeconfig tenantkubeconfig-${test_name} -n tenant-test \ + until kubectl get svc ${test_name}-backend --kubeconfig \"tenantkubeconfig-${test_name}\" -n tenant-test \ -o jsonpath='{.status.loadBalancer.ingress[0]}' | grep -q .; do sleep 5 done🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh` around lines 194 - 199, The timeout subshell calling kubectl get svc uses an unquoted kubeconfig path (--kubeconfig tenantkubeconfig-${test_name}); update that invocation (the timeout ... sh -ec block that runs kubectl get svc ${test_name}-backend) to quote the kubeconfig argument so it becomes --kubeconfig "tenantkubeconfig-${test_name}" to match the rest of the script and avoid word-splitting for test_name values with spaces or special chars.
224-225: Same consideration applies to backend cleanup on the success path.If these
deletecommands fail, lines 227+ (NFS cleanup and final teardown) are skipped. Consider adding--ignore-not-found || truefor defensive consistency, same as the pre-creation cleanup pattern above.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh` around lines 224 - 225, The success-path cleanup uses kubectl delete for the backend deployment and service (commands invoking kubectl delete deployment "${test_name}-backend" and kubectl delete service "${test_name}-backend"), but unlike the pre-creation cleanup it doesn't tolerate missing resources; update both kubectl delete commands to be defensive by adding --ignore-not-found (or append || true) so failures won't abort the remaining NFS cleanup and final teardown.
287-288: Consider adding|| trueto success-path NFS cleanup to protect downstream cleanup.If the
delete podordelete pvccommand fails here (e.g., resource already gone, API timeout),set -euwill exit the script before reaching lines 291–299, skipping the machine deployment wait, Helm release checks, and — most importantly — the finalkubectl delete kuberneteseson line 299. Adding|| true(or--ignore-not-found) would make this consistent with the failure-path cleanup on lines 281–282 and prevent orphaned tenant resources.Suggested fix
# Cleanup NFS test resources in tenant cluster - kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod -n tenant-test --wait - kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc -n tenant-test + kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod -n tenant-test --wait --ignore-not-found || true + kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc -n tenant-test --ignore-not-found || true🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hack/e2e-apps/run-kubernetes.sh` around lines 287 - 288, Update the success-path NFS cleanup commands so failures don't abort the script: append "|| true" (or use "--ignore-not-found") to the kubectl delete calls for the pod and pvc—specifically modify the `kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pod nfs-test-pod -n tenant-test` and `kubectl --kubeconfig "tenantkubeconfig-${test_name}" delete pvc nfs-test-pvc -n tenant-test` commands to tolerate non-fatal failures (e.g., `... delete pod ... || true` and `... delete pvc ... || true`).
2 participants
Summary
run-kubernetes.sh, so E2E test retries start fresh instead of reusing stuck resources from a failed attemptContext
When
kubernetes-previous(orkubernetes-latest) E2E test fails at thekubectl wait deployment --timeout=90sstep,set -eucauses immediate exit before cleanup. On retry,kubectl applysees the stuck deployment as "unchanged", making retries 2 and 3 guaranteed to fail against the same stuck pod.This was observed in run 22076291555 where
kubernetes-previousfailed 3/3 attempts while passing in 5/6 other recent runs.Test plan
--ignore-not-foundcleanup commands are no-ops, no side effectsSummary by CodeRabbit