ROX-29771: Use prefetched images in non-groovy tests#17354
Conversation
|
Skipping CI for Draft Pull Request. |
|
Images are ready for the commit at 4d0bb44. To use with deploy scripts, first |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #17354 +/- ##
=======================================
Coverage 48.71% 48.72%
=======================================
Files 2724 2724
Lines 203031 202980 -51
=======================================
- Hits 98899 98893 -6
+ Misses 96367 96334 -33
+ Partials 7765 7753 -12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider extracting the default namespace string into a constant to avoid hardcoding "default" in all setupDeploymentInNamespace calls.
- The createDeploymentViaAPI function is quite long—splitting it into smaller helpers (e.g. spec construction, error handling, logging) would improve readability.
- The waitForDeployment* functions mix *testing.T and testutils.T; unifying on a single test interface would make the API more consistent.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider extracting the default namespace string into a constant to avoid hardcoding "default" in all setupDeploymentInNamespace calls.
- The createDeploymentViaAPI function is quite long—splitting it into smaller helpers (e.g. spec construction, error handling, logging) would improve readability.
- The waitForDeployment* functions mix *testing.T and testutils.T; unifying on a single test interface would make the API more consistent.
## Individual Comments
### Comment 1
<location> `scripts/ci/lib.sh:893-895` </location>
<code_context>
qa-e2e)
cp "$SCRIPTS_ROOT/qa-tests-backend/scripts/images-to-prefetch.txt" "$image_list"
;;
+ qa-nongroovy-e2e)
+ cp "$SCRIPTS_ROOT/tests/images-to-prefetch.txt" "$image_list"
+ ;;
*)
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Check for existence of images-to-prefetch.txt before copying.
Add a check to verify the file exists before copying, or handle missing file errors to prevent runtime failures.
```suggestion
qa-nongroovy-e2e)
if [ -f "$SCRIPTS_ROOT/tests/images-to-prefetch.txt" ]; then
cp "$SCRIPTS_ROOT/tests/images-to-prefetch.txt" "$image_list"
else
echo "WARNING: $SCRIPTS_ROOT/tests/images-to-prefetch.txt not found, skipping image prefetch for qa-nongroovy-e2e."
fi
;;
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
9a43c43 to
5a405dd
Compare
|
I ❤️ |
This PR is already an extraction from #17216, which was planned to be a quick look into a flake 😂 |
dc2675a to
4d0bb44
Compare
|
/retest |
2 similar comments
|
/retest |
|
/retest |
|
gke-nongroovy-e2e-tests failed with |
|
Images are ready for the commit at 3109e90. To use with deploy scripts, first |
|
This is currently blocked by #17374 |
|
/retest |
|
/test gke-nongroovy-e2e-tests |
3 similar comments
|
/test gke-nongroovy-e2e-tests |
|
/test gke-nongroovy-e2e-tests |
|
/test gke-nongroovy-e2e-tests |
|
Images are ready for the commit at f5043c0. To use with deploy scripts, first |
|
/test gke-nongroovy-e2e-tests |
1 similar comment
|
/test gke-nongroovy-e2e-tests |
|
I see that I enabled log collection for the default namespace and the affected pod is doing well apparently: pod logs: I think this is another issue unrelated to this work - the pod seem to start after the timeout for stopping it. The log line is caused by changes introduced here in this PR and will be fixed before merging. |
933d3fc to
360782e
Compare
|
/test gke-nongroovy-e2e-tests |
360782e to
db54343
Compare
|
/test gke-nongroovy-e2e-tests |
…e2e tests Use prefetched quay.io images instead of docker.io to avoid rate limits and unauthorized access errors. Introduce createDeploymentViaAPI to create K8s deployments via API client instead of kubectl, enabling proper imagePullPolicy configuration for prefetched images. Key changes: - Add createDeploymentViaAPI with security context to satisfy PodSecurity policies and ensure graceful pod termination (fixes teardown timeouts) - Apply IMAGE_PULL_POLICY_FOR_QUAY_IO only to actual quay.io/ images, not mirrored images (icsp.invalid, idms.invalid, itms.invalid) used in mirror tests - Collect logs from "default" namespace in delegated scanning tests for debugging deployment issues - Update test images to use quay.io mirrors and add them to prefetch list Fixes TestDelegatedScanning teardown timeout caused by missing security context fields preventing graceful pod termination. AI-generated: Initial createDeploymentViaAPI structure, security context fields, and log collection additions User-corrected: imagePullPolicy logic to exclude mirrors, security context values to match PodSecurity requirements, and verified all deployment creation call paths
…iaAPI Test images (nginx, etc.) run as root by default. The RunAsNonRoot constraint was causing pod creation failures with "container has runAsNonRoot and image will run as root" error in TestBackup and other tests. Keep other security context fields (AllowPrivilegeEscalation=false, drop all capabilities, seccomp profile) to help ensure graceful termination without breaking root containers. AI-generated: Initial approach with full security constraints User-corrected: Removed RunAsNonRoot after identifying root container requirement from test failure
db54343 to
a689c34
Compare
|
@vikin91: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
|
🛑 blocked by ROX-29771: Refactor common nongroovy test code #17374Description
This commit addresses flakiness in e2e tests by improving deployment management and test reliability:
Fixed TestExcludedScopes (excluded_scopes_test.go):
Updated related tests for consistency:
All changes maintain backward compatibility while improving debuggability and reducing flakiness in CI environments.
Partially AI-generated.
For the
tests/active_vuln_test.go, I have downloaded images from docker.io and uploaded them to quay.io as follows:User-facing documentation
Testing and quality
Automated testing
How I validated my change
gke-nongroovy-e2e-testswill tell the truth