fix: quay promotion digest resolve for ocp/4.x#5216
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
📝 WalkthroughWalkthroughThis PR refactors release image promotions for Quay by introducing explicit mirror retry helpers with randomized backoff, updating tag derivation to support consolidated OCP streams, and reorganizing promotion pod command construction to separate mirroring from resolve-and-tag operations. All test fixtures are updated to reflect the new retry-loop structure and dynamic digest resolution. ChangesQuay image promotion retry and mirror refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
openshift-ci
Bot
added
the
approved
|
/test e2e |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
pkg/steps/release/promote_test.go (2)
1113-1127: ⚡ Quick winTighten the helper-specific assertions here.
This test would still pass if the retry helper dropped the randomized backoff or the rendered mirror flags. Please assert the helper-specific contract too, e.g.
$(($RANDOM % 120)),sleep $backoff,--registry-config=..., and--max-per-registry=10.As per coding guidelines, "New or modified functionality should include test coverage. New Go functions and methods should have corresponding unit tests. Bug fixes should include a regression test that fails without the fix. Pure functions should always be tested. Prefer table-driven tests."
🤖 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 `@pkg/steps/release/promote_test.go` around lines 1113 - 1127, The test TestGetMirrorRetryShell currently checks generic substrings; tighten it to assert the helper-specific contract by verifying the randomized backoff expression and usage, the sleep call, and required flags: ensure getMirrorRetryShell(regcfg, []string{"src=dst"}, 2) output contains the substring '$(($RANDOM % 120))', the variable usage 'sleep $backoff', the registry flag '--registry-config=' followed by the provided regcfg value, and the flag '--max-per-registry=10'; update TestGetMirrorRetryShell to include these contains checks so the test fails if backoff, sleep, or mirror flags are omitted or altered.
1189-1203: ⚡ Quick winAdd boundary cases for the consolidated stream window.
The new contract is specifically
ocp/4.11throughocp/4.22, but the added cases only hit interior values plus4.23. Please add explicit4.11,4.22, and ideally4.10coverage so an off-by-one inapi.ConsolidatedQuayPromotionVersioncannot slip through.As per coding guidelines, "New or modified functionality should include test coverage. New Go functions and methods should have corresponding unit tests. Bug fixes should include a regression test that fails without the fix. Pure functions should always be tested. Prefer table-driven tests."
🤖 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 `@pkg/steps/release/promote_test.go` around lines 1189 - 1203, The table-driven test for consolidated streams is missing boundary cases around api.ConsolidatedQuayPromotionVersion; add explicit test rows for "ocp/4.10:..." (expect wantOK false), "ocp/4.11:..." (expect wantOK true), and "ocp/4.22:..." (expect wantOK true) to the test cases alongside the existing interior and 4.23 checks so off-by-one errors in ConsolidatedQuayPromotionVersion are caught; reference the existing test case entries (e.g., the rows using isTagKey like "ocp/4.13:secondary-scheduler-operator") and mirror their wantTag/wantOK expectations for the new rows.
🤖 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 `@pkg/steps/release/promote_test.go`:
- Around line 1113-1127: The test TestGetMirrorRetryShell currently checks
generic substrings; tighten it to assert the helper-specific contract by
verifying the randomized backoff expression and usage, the sleep call, and
required flags: ensure getMirrorRetryShell(regcfg, []string{"src=dst"}, 2)
output contains the substring '$(($RANDOM % 120))', the variable usage 'sleep
$backoff', the registry flag '--registry-config=' followed by the provided
regcfg value, and the flag '--max-per-registry=10'; update
TestGetMirrorRetryShell to include these contains checks so the test fails if
backoff, sleep, or mirror flags are omitted or altered.
- Around line 1189-1203: The table-driven test for consolidated streams is
missing boundary cases around api.ConsolidatedQuayPromotionVersion; add explicit
test rows for "ocp/4.10:..." (expect wantOK false), "ocp/4.11:..." (expect
wantOK true), and "ocp/4.22:..." (expect wantOK true) to the test cases
alongside the existing interior and 4.23 checks so off-by-one errors in
ConsolidatedQuayPromotionVersion are caught; reference the existing test case
entries (e.g., the rows using isTagKey like
"ocp/4.13:secondary-scheduler-operator") and mirror their wantTag/wantOK
expectations for the new rows.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 9fe3bad2-e0e6-41c4-8b2e-4d0e81a18e9e
📒 Files selected for processing (9)
pkg/steps/release/promote.gopkg/steps/release/promote_test.gopkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__arm64_only.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_basic_case__multi_architecture.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_4.12.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_multiple_tags.yamlpkg/steps/release/testdata/zz_fixture_TestGetPromotionPod_promotion_quay_non_release_namespace.yaml
|
Tests from second stage were triggered manually. Pipeline can be controlled only manually, until HEAD changes. Use command to trigger second stage. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: danilo-gemoli, deepsm007 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 |
|
@deepsm007: 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. |
2 participants
Consolidated
ocp/4.11–4.22promotion targets now resolve quay-proxy digests post-mirror viaoc image info+oc tag. Mirror retries fail the pod after 5 attempts; official digest-tag runs outsideset +e.Summary
This PR fixes image promotion for OpenShift's consolidated OCP/4.x release streams in the CI operator. Previously, promotion of container images to Quay had issues with properly resolving digest references after mirroring images between registries.
What Changed
Core Issue: The promotion logic for quay-proxy didn't correctly handle digest resolution when pushing images to consolidated
ocp/4.xstreams (e.g.,ocp/4.13,ocp/4.21). After mirroring an image to quay-proxy, the system couldn't reliably determine the resulting image digest to tag it into release streams.Solution:
oc image infoto dynamically fetch the actual image digest, then usesoc tagto apply the correct tags. This replaces the previous approach of using fixed/hardcoded digests.namespace/stream-quay:tag) and the consolidated format (ocp/4.x:tag).set +eerror suppression (ensuring proper failure detection), while template/non-digest tags use best-effort tagging with error suppression.Practical Impact
For CI operators: Image promotions to consolidated OCP release streams should now complete more reliably, with proper error handling when mirrors fail and accurate digest tracking after cross-registry operations. Failed mirroring will now properly fail the promotion pod instead of continuing with stale state.
For test fixtures: The shell scripts that execute within promotion pods now contain more structured retry logic with clearer exit/failure semantics, making promotion jobs easier to debug when issues occur.
Files Modified
pkg/steps/release/promote.go- Added mirror retry helpers, split quay-step flow for official vs. non-official targets, dynamic digest resolutionpkg/steps/release/promote_test.go- New test for mirror retry logic, expanded quay-proxy tag mapping testsoc image infofor dynamic digest resolution