CLID-625, CLID-626: Tests for --parallel-images and --parallel-layers flags#1415
Conversation
…allel-layers flags
openshift-ci-robot
added
the
jira/valid-reference
|
@adolfo-ab: This pull request references CLID-625 which is a valid jira issue. This pull request references CLID-626 which is a valid jira issue. DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
WalkthroughThis PR adds integration tests for oc-mirror's parallel copy feature, a multilayer test image and build tooling, YAML test inputs for image- and layer-parallel scenarios, and a small change to retry logic treating HTTP 500–504 as retryable. ChangesParallel copy integration tests
Mirror retry policy
🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (11 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 |
![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
🤖 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 `@tests/integration/image-builders/additional_images/multilayers/Makefile`:
- Line 19: The Makefile's digest target uses an invalid Go-template expression;
replace the incorrect use of the index function with a direct field access. Edit
the command that runs $(RUNTIME) inspect $(IMAGE_REF) --format '{{index
.Digest}}' to use '{{.Digest}}' instead so the template accesses the Digest
field directly (references: RUNTIME, IMAGE_REF, the inspect --format invocation
in the digest target).
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 271c83ca-8b7a-4983-9d5c-fcc0170306aa
📒 Files selected for processing (7)
tests/integration/image-builders/additional_images/README.mdtests/integration/image-builders/additional_images/multilayers/Dockerfiletests/integration/image-builders/additional_images/multilayers/Makefiletests/integration/image-builders/additional_images/multilayers/README.mdtests/integration/parallel_copy_test.gotests/integration/testdata/imagesetconfigs/parallel_copy/isc-multilayers.yamltests/integration/testdata/imagesetconfigs/parallel_copy/isc-parallel-images.yaml
aguidirh
left a comment
aguidirh
left a comment
There was a problem hiding this comment.
Thanks for the PR. I added few comments.
Needs to check why the integration test is failing, is it because of some kind of race condition when images share the same blob on few layers?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/pkg/mirror/mirror.go (1)
240-242: ⚡ Quick winConsider excluding HTTP 501 from the retry range.
The current condition retries all HTTP status codes 500–504. However, HTTP 501 (Not Implemented) is not a transient error and should not be retried—if a server lacks the functionality, repeated attempts will continue to fail.
Consider explicitly excluding 501 to avoid unnecessary retry attempts.
♻️ Proposed fix to exclude HTTP 501
case errors.As(err, &httpError): - // Retry on 500-504 server errors, they appear to be quite common in the field + // Retry on transient 5xx server errors (500, 502-504), they appear to be quite common in the field // We duplicate this here because older versions of oc-mirror cannot bump containers/common given Golang version restrictions - if httpError.StatusCode >= http.StatusInternalServerError && httpError.StatusCode <= http.StatusGatewayTimeout { + if (httpError.StatusCode >= http.StatusInternalServerError && httpError.StatusCode <= http.StatusGatewayTimeout) && httpError.StatusCode != http.StatusNotImplemented { return true }🤖 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 `@internal/pkg/mirror/mirror.go` around lines 240 - 242, The retry condition currently treats all status codes from 500 to 504 as transient; change the check around httpError.StatusCode (in mirror.go) to exclude 501 (HTTP StatusNotImplemented) so it is not retried—update the if that reads "httpError.StatusCode >= http.StatusInternalServerError && httpError.StatusCode <= http.StatusGatewayTimeout" to also ensure httpError.StatusCode != http.StatusNotImplemented (i.e., only retry 500, 502–504).
🤖 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.
Nitpick comments:
In `@internal/pkg/mirror/mirror.go`:
- Around line 240-242: The retry condition currently treats all status codes
from 500 to 504 as transient; change the check around httpError.StatusCode (in
mirror.go) to exclude 501 (HTTP StatusNotImplemented) so it is not
retried—update the if that reads "httpError.StatusCode >=
http.StatusInternalServerError && httpError.StatusCode <=
http.StatusGatewayTimeout" to also ensure httpError.StatusCode !=
http.StatusNotImplemented (i.e., only retry 500, 502–504).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: a3210d62-fb44-4f23-8987-427502e9d4e3
📒 Files selected for processing (1)
internal/pkg/mirror/mirror.go
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/integration/parallel_copy_test.go`:
- Around line 30-36: The It blocks call runner.MirrorToDisk with
"--parallel-layers"/"--parallel-images" but only assert success, so the test
doesn't verify the flags actually changed concurrency; update the tests in
parallel_copy_test.go to either (A) observe an external concurrency signal from
the operation (e.g., inject or mock a layer fetcher in runner.MirrorToDisk to
record concurrent goroutine counts and assert >1 when "--parallel-layers" is
set, or check timestamps of mocked layer downloads to ensure overlapping
execution) and add assertions on that signal, or (B) if such instrumentation is
not available, rename the It descriptions to "accepts
--parallel-layers/--parallel-images flag" and keep only success assertions;
refer to runner.MirrorToDisk and the existing It/By blocks to locate where to
add the concurrency probe or rename the test.
🪄 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: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3df9f5d-8204-47f9-a8b2-c7c0fc491413
📒 Files selected for processing (1)
tests/integration/parallel_copy_test.go
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: adolfo-ab, aguidirh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
|
/assign @nidangavali For learning purposes only. |
|
/verified bypass Since there is no production code being changed, there is no need to verify. |
openshift-ci-robot
added
the
verified
|
@aguidirh: The DetailsIn response to this:
Instructions 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 openshift-eng/jira-lifecycle-plugin repository. |
|
@adolfo-ab: all tests passed! 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. |
5 participants
Description
Adds tests for --parallel-images and --parallel-layers flags.
Replaces manual tests OCP-83188 and OCP-83189.
Github / Jira issue:
CLID-625, CLID-626
How Has This Been Tested?
By running the tests
Summary by CodeRabbit