avoid race conditions on multiple instances on arch specific builds#5004
Merged
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom Mar 12, 2026
Merged
avoid race conditions on multiple instances on arch specific builds#5004openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
openshift-merge-bot[bot] merged 1 commit intoopenshift:mainfrom
Conversation
Contributor
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughInjects a controller-runtime client into ManifestPusher, which now parses target image refs, fetches ImageStreamTag objects via the client, merges DockerImageManifests with source images, and updates callers to provide the client. Changes
Sequence Diagram(s)sequenceDiagram
participant Steps as Steps.handleBuilds
participant Pusher as ManifestPusher
participant Builds as Builds (data)
participant Kube as Kube API (ImageStreamTag)
participant Registry as Image Registry
Steps->>Pusher: NewManifestPusher(logger, registryURL, dockercfg, client)
Steps->>Pusher: PushImageWithManifest(builds, targetImageRef)
Pusher->>Pusher: manifestEntries(builds, targetImageRef)
Pusher->>Kube: Get ImageStreamTag(namespace/name:tag)
Kube-->>Pusher: ImageStreamTag (or NotFound)
Pusher->>Pusher: merge srcImages + DockerImageManifests -> entries
Pusher->>Registry: push manifest list for entries
Registry-->>Pusher: digest
Pusher-->>Steps: result / error
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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
droslean
force-pushed
the
bug-race
branch
2 times, most recently
from
4454d50 to
dc8096d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
pkg/manifestpusher/manifestpusher_test.go (1)
27-96: Consider expanding test coverage for edge cases.The test cases cover the happy path (merging entries) and invalid reference error, but several edge cases are missing:
- ImageStreamTag not found - Verify behavior when the IST doesn't exist yet (first-time multi-arch build)
- Empty DockerImageManifests - Verify behavior when IST exists but has no manifest list
- Empty builds array - Verify behavior when no new builds are provided
- Architecture collision - Verify behavior when a new build has the same architecture as an existing manifest
These scenarios are important for validating the race condition fix mentioned in the PR title.
💡 Example test case for IST not found
{ name: "returns only new builds when ImageStreamTag not found", targetRef: "ns/pipeline:src", builds: []buildv1.Build{ { Spec: buildv1.BuildSpec{ CommonSpec: buildv1.CommonSpec{ NodeSelector: map[string]string{nodeArchitectureLabel: "amd64"}, Output: buildv1.BuildOutput{ To: &corev1.ObjectReference{Namespace: "ns", Name: "pipeline:src-amd64"}, }, }, }, }, }, objects: []ctrlruntimeclient.Object{}, // No IST exists want: []types.ManifestEntry{ { Image: "registry/ns/pipeline:src-amd64", Platform: ocispec.Platform{ OS: "linux", Architecture: "amd64", }, }, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/manifestpusher/manifestpusher_test.go` around lines 27 - 96, Add unit tests to the testCases slice in manifestpusher_test.go to cover the missing edge cases: (1) "ImageStreamTag not found" — add a case with objects: []ctrlruntimeclient.Object{} and expect only the new build entry in want; (2) "Empty DockerImageManifests" — add an IST object with Image.DockerImageManifests: nil or empty slice and expect only new build entries in want; (3) "Empty builds array" — set builds: []buildv1.Build{} and provide an existing IST in objects, asserting want equals the existing manifest entries (or empty if none); (4) "Architecture collision" — provide an existing IST with a manifest having the same Architecture as a new build and a build in builds with the same nodeArchitectureLabel value, then assert that the resulting want deduplicates or replaces correctly per intended behavior. Use the existing field names (testCases, targetRef, builds, objects, want, wantErr) and nodeArchitectureLabel to mirror other cases so the test harness (manifestpusher_test.go) picks them up.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/manifestpusher/manifestpusher.go`:
- Around line 85-95: The current logic in the ImageStreamTag fetch
(m.client.Get) and subsequent manifest check returns nil,nil when the IST is not
found or when ist.Image.DockerImageManifests is empty, which drops the newly
built srcImages; change the branches so that on kerrors.IsNotFound(err) you
return srcImages (not nil,nil) and likewise when
len(ist.Image.DockerImageManifests) == 0 return srcImages, preserving the
existing error return for other Get failures; locate the ImageStreamTag handling
around m.client.Get, the ist variable, and the check of
ist.Image.DockerImageManifests and replace the nil,nil returns with returning
the srcImages slice so new builds are pushed.
- Around line 97-112: The merge currently appends existing manifests from
ist.Image.DockerImageManifests onto mergedEntries without filtering, causing
duplicate platform entries when srcImages already contains the same
architecture; create a set of built architectures from srcImages (inspect
srcImages[].Platform.Architecture), then when iterating
ist.Image.DockerImageManifests skip any manifest whose Architecture is present
in that set (also keep the existing checks for empty OS/Architecture/Digest)
before appending to mergedEntries so PushManifestList won't receive duplicate
platforms.
---
Nitpick comments:
In `@pkg/manifestpusher/manifestpusher_test.go`:
- Around line 27-96: Add unit tests to the testCases slice in
manifestpusher_test.go to cover the missing edge cases: (1) "ImageStreamTag not
found" — add a case with objects: []ctrlruntimeclient.Object{} and expect only
the new build entry in want; (2) "Empty DockerImageManifests" — add an IST
object with Image.DockerImageManifests: nil or empty slice and expect only new
build entries in want; (3) "Empty builds array" — set builds: []buildv1.Build{}
and provide an existing IST in objects, asserting want equals the existing
manifest entries (or empty if none); (4) "Architecture collision" — provide an
existing IST with a manifest having the same Architecture as a new build and a
build in builds with the same nodeArchitectureLabel value, then assert that the
resulting want deduplicates or replaces correctly per intended behavior. Use the
existing field names (testCases, targetRef, builds, objects, want, wantErr) and
nodeArchitectureLabel to mirror other cases so the test harness
(manifestpusher_test.go) picks them up.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 24883af4-af9d-4e25-add9-3ccd32ea50e3
📒 Files selected for processing (6)
pkg/controller/multiarchbuildconfig/multiarchbuildconfig.gopkg/manifestpusher/manifestpusher.gopkg/manifestpusher/manifestpusher_test.gopkg/steps/build_client.gopkg/steps/source.gopkg/steps/source_test.go
Member
Author
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
Signed-off-by: Nikolaos Moraitis <nmoraiti@redhat.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/manifestpusher/manifestpusher_test.go (1)
27-76: Test coverage is insufficient to catch the bugs in the implementation.The single test case only covers the happy path with non-overlapping architectures (new
amd64build + existingarm64manifest). Consider adding test cases for:
- IST not found - verifies new builds are returned (currently they're discarded)
- IST with empty
DockerImageManifests- same issue- Overlapping architecture - e.g., rebuilding
amd64when IST already hasamd64manifest (would expose the duplicate platform bug)💡 Example additional test cases
{ name: "returns srcImages when IST not found", targetRef: "ns/pipeline:src", builds: []buildv1.Build{ { Spec: buildv1.BuildSpec{ CommonSpec: buildv1.CommonSpec{ NodeSelector: map[string]string{nodeArchitectureLabel: "amd64"}, Output: buildv1.BuildOutput{ To: &corev1.ObjectReference{Namespace: "ns", Name: "pipeline:src-amd64"}, }, }, }, }, }, objects: []ctrlruntimeclient.Object{}, // No IST exists want: []types.ManifestEntry{ { Image: "registry/ns/pipeline:src-amd64", Platform: ocispec.Platform{ OS: "linux", Architecture: "amd64", }, }, }, }, { name: "rebuilding same architecture replaces existing manifest", targetRef: "ns/pipeline:src", builds: []buildv1.Build{ { Spec: buildv1.BuildSpec{ CommonSpec: buildv1.CommonSpec{ NodeSelector: map[string]string{nodeArchitectureLabel: "amd64"}, Output: buildv1.BuildOutput{ To: &corev1.ObjectReference{Namespace: "ns", Name: "pipeline:src-amd64"}, }, }, }, }, }, objects: []ctrlruntimeclient.Object{ &imagev1.ImageStreamTag{ ObjectMeta: metav1.ObjectMeta{Namespace: "ns", Name: "pipeline:src"}, Image: imagev1.Image{ DockerImageManifests: []imagev1.ImageManifest{ {Digest: "sha256:oldamd64digest", OS: "linux", Architecture: "amd64"}, }, }, }, }, want: []types.ManifestEntry{ { Image: "registry/ns/pipeline:src-amd64", Platform: ocispec.Platform{ OS: "linux", Architecture: "amd64", }, }, // Should NOT include the old amd64 manifest }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/manifestpusher/manifestpusher_test.go` around lines 27 - 76, Add more unit tests to cover IST-not-found, IST-with-empty-DockerImageManifests, and overlapping-architecture rebuilds by extending the testCases slice in manifestpusher_test.go (the existing variable testCases) with cases that: (1) set objects to an empty slice and assert the new build's ManifestEntry is returned, (2) set objects to an ImageStreamTag whose Image.DockerImageManifests is empty and assert the new build's ManifestEntry is returned, and (3) set objects to an ImageStreamTag that already contains the same Architecture (e.g., amd64) and assert the returned want slice contains the new build entry but not the old duplicate manifest; use the same targetRef/build structure as existing tests and compare against types.ManifestEntry entries to validate deduplication/replacement behavior for the function under test (the tests in this file that iterate testCases).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/manifestpusher/manifestpusher_test.go`:
- Around line 27-76: Add more unit tests to cover IST-not-found,
IST-with-empty-DockerImageManifests, and overlapping-architecture rebuilds by
extending the testCases slice in manifestpusher_test.go (the existing variable
testCases) with cases that: (1) set objects to an empty slice and assert the new
build's ManifestEntry is returned, (2) set objects to an ImageStreamTag whose
Image.DockerImageManifests is empty and assert the new build's ManifestEntry is
returned, and (3) set objects to an ImageStreamTag that already contains the
same Architecture (e.g., amd64) and assert the returned want slice contains the
new build entry but not the old duplicate manifest; use the same targetRef/build
structure as existing tests and compare against types.ManifestEntry entries to
validate deduplication/replacement behavior for the function under test (the
tests in this file that iterate testCases).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 342953ad-57a3-4198-8b19-1dadbacac471
📒 Files selected for processing (6)
pkg/controller/multiarchbuildconfig/multiarchbuildconfig.gopkg/manifestpusher/manifestpusher.gopkg/manifestpusher/manifestpusher_test.gopkg/steps/build_client.gopkg/steps/source.gopkg/steps/source_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/controller/multiarchbuildconfig/multiarchbuildconfig.go
Contributor
|
Scheduling tests matching the |
jmguzik
approved these changes
Mar 12, 2026
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: droslean, jmguzik 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 |
Member
Author
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
Contributor
|
@droslean: 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. |
openshift-merge-bot
bot
merged commit 55bbe1c
into
openshift:main
13 of 14 checks passed
4 tasks
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
3 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.
/cc @openshift/test-platform
Summary by CodeRabbit