WIP: Split multi-arch source pipeline into arch-independent scr + arch-specific src#4995
WIP: Split multi-arch source pipeline into arch-independent scr + arch-specific src#4995joepvd wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
Pipeline controller notification For optional jobs, comment This repository is configured in: automatic mode |
WalkthroughAdds SCR (scratch-source) support: new SrcAssemblyStepConfiguration and srcAssemblyStep that produce architecture-specific source images, refactors source step emission to return multiple steps, converts source builds to a scratch-based multi-stage Dockerfile, and updates defaults, validation, tests, and handleBuild call sites. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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)
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 |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: joepvd The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
joepvd
force-pushed
the
multi-arch-scr-from-scratch
branch
from
faa5edd to
3dfa5f4
Compare
![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/src_assembly.go (1)
58-72: Consider simplifying the refs filtering logic.The refs filtering logic correctly handles both primary refs and extra refs, filtering by
config.Refwhen specified. However, the pattern of buildingorgRepoand comparing it could be extracted to a helper for consistency with similar logic elsewhere.♻️ Optional: Extract orgRepo matching to a helper
+func matchesRef(r prowv1.Refs, configRef string) bool { + if configRef == "" { + return true + } + return fmt.Sprintf("%s.%s", r.Org, r.Repo) == configRef +} + func (s *srcAssemblyStep) run(ctx context.Context) error { var refs []prowv1.Refs if s.jobSpec.Refs != nil { - r := *s.jobSpec.Refs - orgRepo := fmt.Sprintf("%s.%s", r.Org, r.Repo) - if s.config.Ref == "" || orgRepo == s.config.Ref { + if matchesRef(*s.jobSpec.Refs, s.config.Ref) { refs = append(refs, r) } } for _, r := range s.jobSpec.ExtraRefs { - orgRepo := fmt.Sprintf("%s.%s", r.Org, r.Repo) - if s.config.Ref == "" || orgRepo == s.config.Ref { + if matchesRef(r, s.config.Ref) { refs = append(refs, r) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/src_assembly.go` around lines 58 - 72, The refs filtering in srcAssemblyStep.run repeats building orgRepo and comparing to s.config.Ref for both s.jobSpec.Refs and each entry in s.jobSpec.ExtraRefs; extract that logic into a small helper function (e.g., func matchesRef(r prowv1.Refs, ref string) bool) that constructs fmt.Sprintf("%s.%s", r.Org, r.Repo) and returns true when ref=="" or orgRepo==ref, then call this helper when deciding to append for the pointer case (jobSpec.Refs) and the loop over jobSpec.ExtraRefs to remove duplication and keep behavior identical.pkg/steps/source.go (1)
168-177: Consider removing unusedmetricsAgentfield.The
metricsAgentfield is passed to the struct via theSourceStepfactory function (line 885, 895) but is not used anywhere insourceStep's methods. The metrics are now handled internally byhandleBuildviaclient.MetricsAgent().This appears to be dead code after the refactor. Consider removing both the field and the corresponding parameter from the
SourceStepfactory function.♻️ Proposed cleanup
type sourceStep struct { config api.SourceStepConfiguration resources api.ResourceConfiguration client BuildClient podClient kubernetes.PodClient jobSpec *api.JobSpec cloneAuthConfig *CloneAuthConfig pullSecret *corev1.Secret - metricsAgent *metrics.MetricsAgent }And update the factory function:
func SourceStep( config api.SourceStepConfiguration, resources api.ResourceConfiguration, buildClient BuildClient, podClient kubernetes.PodClient, jobSpec *api.JobSpec, cloneAuthConfig *CloneAuthConfig, pullSecret *corev1.Secret, - metricsAgent *metrics.MetricsAgent, ) api.Step { return &sourceStep{ config: config, resources: resources, client: buildClient, podClient: podClient, jobSpec: jobSpec, cloneAuthConfig: cloneAuthConfig, pullSecret: pullSecret, - metricsAgent: metricsAgent, } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/steps/source.go` around lines 168 - 177, The sourceStep struct contains an unused field metricsAgent and the SourceStep factory function still accepts/passes that parameter; remove the metricsAgent field from the sourceStep struct and remove the corresponding parameter from the SourceStep factory function signature and its internal assignment/argument passing (search for sourceStep{... metricsAgent: ...} and the factory function that constructs sourceStep) and update all call sites to stop providing that argument so the code compiles; ensure no remaining references to metricsAgent exist in methods on sourceStep and remove any related imports if they become unused.
🤖 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/steps/source.go`:
- Around line 168-177: The sourceStep struct contains an unused field
metricsAgent and the SourceStep factory function still accepts/passes that
parameter; remove the metricsAgent field from the sourceStep struct and remove
the corresponding parameter from the SourceStep factory function signature and
its internal assignment/argument passing (search for sourceStep{...
metricsAgent: ...} and the factory function that constructs sourceStep) and
update all call sites to stop providing that argument so the code compiles;
ensure no remaining references to metricsAgent exist in methods on sourceStep
and remove any related imports if they become unused.
In `@pkg/steps/src_assembly.go`:
- Around line 58-72: The refs filtering in srcAssemblyStep.run repeats building
orgRepo and comparing to s.config.Ref for both s.jobSpec.Refs and each entry in
s.jobSpec.ExtraRefs; extract that logic into a small helper function (e.g., func
matchesRef(r prowv1.Refs, ref string) bool) that constructs fmt.Sprintf("%s.%s",
r.Org, r.Repo) and returns true when ref=="" or orgRepo==ref, then call this
helper when deciding to append for the pointer case (jobSpec.Refs) and the loop
over jobSpec.ExtraRefs to remove duplication and keep behavior identical.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5aa17fc-6323-4d92-92fd-2fd7f7de33ee
⛔ Files ignored due to path filters (1)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*
📒 Files selected for processing (15)
pkg/api/types.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/source.gopkg/steps/source_test.gopkg/steps/src_assembly.gopkg/steps/testdata/zz_fixture_TestCreateScrBuild_basic_options_for_a_presubmit.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_title_in_pull_gets_squashed.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_OAuth_token.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_a_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_a_pull_secret.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_extra_refs.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_extra_refs_setting_workdir_and_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScrBuild_with_ssh_key.yamlpkg/validation/graph.go
joepvd
force-pushed
the
multi-arch-scr-from-scratch
branch
from
3dfa5f4 to
b97b312
Compare
joepvd
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/defaults/defaults_test.go`:
- Around line 2315-2327: The comparator used in defaults_test.go (the anonymous
sort function that computes refA/refB from a.SourceStepConfiguration /
a.SrcAssemblyStepConfiguration and similarly for b) is not a strict total order
because different step variants can share the same Ref; update the comparator so
if refA == refB it returns a deterministic secondary ordering based on step kind
(e.g., derive kindA/kindB from which configuration is non-nil or an existing
Kind field and compare those strings/values), ensuring the comparator always
returns true or false strictly for any pair so cmpopts.SortSlices remains
deterministic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2a643e86-aad0-44f8-b3a5-12419d9bde2d
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (17)
pkg/api/types.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/bundle_source.gopkg/steps/project_image.gopkg/steps/source.gopkg/steps/source_test.gopkg/steps/src_assembly.gopkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_basic_options_for_a_presubmit.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_title_in_pull_gets_squashed.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_OAuth_token.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_a_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_a_pull_secret.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_extra_refs.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_extra_refs_setting_workdir_and_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_ssh_key.yamlpkg/validation/graph.go
🚧 Files skipped from review as they are similar to previous changes (1)
- pkg/steps/source_test.go
| refA := "" | ||
| if a.SourceStepConfiguration != nil { | ||
| refA = a.SourceStepConfiguration.Ref | ||
| } else if a.SrcAssemblyStepConfiguration != nil { | ||
| refA = a.SrcAssemblyStepConfiguration.Ref | ||
| } | ||
| refB := "" | ||
| if b.SourceStepConfiguration != nil { | ||
| refB = b.SourceStepConfiguration.Ref | ||
| } else if b.SrcAssemblyStepConfiguration != nil { | ||
| refB = b.SrcAssemblyStepConfiguration.Ref | ||
| } | ||
| return refA < refB |
There was a problem hiding this comment.
Use a strict sort key here.
Both step variants for the same repo now share the same Ref, so this comparator returns false in both directions for those pairs. cmpopts.SortSlices expects a strict ordering; add a secondary key for the step kind so the helper stays deterministic.
Suggested fix
less := func(a, b api.StepConfiguration) bool {
- refA := ""
+ refA, kindA := "", ""
if a.SourceStepConfiguration != nil {
refA = a.SourceStepConfiguration.Ref
+ kindA = "source"
} else if a.SrcAssemblyStepConfiguration != nil {
refA = a.SrcAssemblyStepConfiguration.Ref
+ kindA = "src-assembly"
}
- refB := ""
+ refB, kindB := "", ""
if b.SourceStepConfiguration != nil {
refB = b.SourceStepConfiguration.Ref
+ kindB = "source"
} else if b.SrcAssemblyStepConfiguration != nil {
refB = b.SrcAssemblyStepConfiguration.Ref
+ kindB = "src-assembly"
}
- return refA < refB
+ if refA != refB {
+ return refA < refB
+ }
+ return kindA < kindB
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| refA := "" | |
| if a.SourceStepConfiguration != nil { | |
| refA = a.SourceStepConfiguration.Ref | |
| } else if a.SrcAssemblyStepConfiguration != nil { | |
| refA = a.SrcAssemblyStepConfiguration.Ref | |
| } | |
| refB := "" | |
| if b.SourceStepConfiguration != nil { | |
| refB = b.SourceStepConfiguration.Ref | |
| } else if b.SrcAssemblyStepConfiguration != nil { | |
| refB = b.SrcAssemblyStepConfiguration.Ref | |
| } | |
| return refA < refB | |
| less := func(a, b api.StepConfiguration) bool { | |
| refA, kindA := "", "" | |
| if a.SourceStepConfiguration != nil { | |
| refA = a.SourceStepConfiguration.Ref | |
| kindA = "source" | |
| } else if a.SrcAssemblyStepConfiguration != nil { | |
| refA = a.SrcAssemblyStepConfiguration.Ref | |
| kindA = "src-assembly" | |
| } | |
| refB, kindB := "", "" | |
| if b.SourceStepConfiguration != nil { | |
| refB = b.SourceStepConfiguration.Ref | |
| kindB = "source" | |
| } else if b.SrcAssemblyStepConfiguration != nil { | |
| refB = b.SrcAssemblyStepConfiguration.Ref | |
| kindB = "src-assembly" | |
| } | |
| if refA != refB { | |
| return refA < refB | |
| } | |
| return kindA < kindB | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/defaults/defaults_test.go` around lines 2315 - 2327, The comparator used
in defaults_test.go (the anonymous sort function that computes refA/refB from
a.SourceStepConfiguration / a.SrcAssemblyStepConfiguration and similarly for b)
is not a strict total order because different step variants can share the same
Ref; update the comparator so if refA == refB it returns a deterministic
secondary ordering based on step kind (e.g., derive kindA/kindB from which
configuration is non-nil or an existing Kind field and compare those
strings/values), ensuring the comparator always returns true or false strictly
for any pair so cmpopts.SortSlices remains deterministic.
… + arch-specific src Fixes the race condition (OCPBUGS-65845) where multiple ci-operator instances building different architectures concurrently would conflict on the shared pipeline ImageStream, causing "builds are using older src image" errors. The source pipeline is now two steps: - scratch-source: arch-independent FROM-scratch image with cloned repo - src assembly: per-architecture step copying scratch-source onto root This eliminates the critical section where the race occurred and removes the 5-minute sleep workaround along with the hardcoded multiArchRepos list. Made-with: Cursor
joepvd
force-pushed
the
multi-arch-scr-from-scratch
branch
from
b97b312 to
af9afbf
Compare
|
/test images |
There was a problem hiding this comment.
♻️ Duplicate comments (2)
pkg/defaults/defaults_test.go (1)
2314-2327:⚠️ Potential issue | 🟡 MinorUse a strict secondary key in this comparator.
SourceStepConfigurationandSrcAssemblyStepConfigurationcan now share the sameRef, so thislessfunction returnsfalsein both directions for those pairs.cmpopts.SortSlicesexpects a strict ordering, and this can make the diff unstable.♻️ Suggested fix
less := func(a, b api.StepConfiguration) bool { - refA := "" + refA, kindA := "", "" if a.SourceStepConfiguration != nil { refA = a.SourceStepConfiguration.Ref + kindA = "source" } else if a.SrcAssemblyStepConfiguration != nil { refA = a.SrcAssemblyStepConfiguration.Ref + kindA = "src-assembly" } - refB := "" + refB, kindB := "", "" if b.SourceStepConfiguration != nil { refB = b.SourceStepConfiguration.Ref + kindB = "source" } else if b.SrcAssemblyStepConfiguration != nil { refB = b.SrcAssemblyStepConfiguration.Ref + kindB = "src-assembly" } - return refA < refB + if refA != refB { + return refA < refB + } + return kindA < kindB }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults_test.go` around lines 2314 - 2327, The comparator function less (comparing api.StepConfiguration) isn't strict when SourceStepConfiguration.Ref equals SrcAssemblyStepConfiguration.Ref; update less to add a deterministic secondary key so it establishes a strict total order: first compute refA/refB as now, then if refA != refB return refA < refB; otherwise break ties by comparing a deterministic discriminator such as whether SourceStepConfiguration is non-nil vs SrcAssemblyStepConfiguration (e.g., treat SourceStepConfiguration < SrcAssemblyStepConfiguration), or compare another stable field (like a Name/Path/ID) from the non-nil config, ensuring the comparator always returns either true or false for any pair so cmpopts.SortSlices gets a strict ordering.pkg/defaults/defaults.go (1)
1123-1148:⚠️ Potential issue | 🟠 MajorKeep
LOCAL_IMAGE_SRCshort-circuiting both source stages.Line 1123 now emits
scratch-sourceandsrcas separate steps. BecausefromConfig()later appliescheckForFullyQualifiedStep()per step, setting onlyLOCAL_IMAGE_SRCskips the assembly step but still runs the clone step. That changes the existing override contract and can still fail on clone auth/root import even when the finalsrcimage is already supplied externally. Please suppress the pairedSourceStepConfigurationin that case and add a regression test for it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/defaults/defaults.go` around lines 1123 - 1148, The sourceStepsForRef function currently emits two steps (SourceStepConfiguration and SrcAssemblyStepConfiguration) which allows LOCAL_IMAGE_SRC to skip only the assembly step; update sourceStepsForRef so that when the LOCAL_IMAGE_SRC override is present it short-circuits both steps by omitting the SourceStepConfiguration entirely (i.e., only emit the SrcAssemblyStepConfiguration or suppress both as appropriate) so checkForFullyQualifiedStep cannot accidentally run the clone stage; change behavior around constructing orgRepo/root/scratchSource/source inside sourceStepsForRef and ensure the logic checks for the LOCAL_IMAGE_SRC env var before returning the slice, and add a regression test that sets LOCAL_IMAGE_SRC and asserts the clone (SourceStepConfiguration) is not present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@pkg/defaults/defaults_test.go`:
- Around line 2314-2327: The comparator function less (comparing
api.StepConfiguration) isn't strict when SourceStepConfiguration.Ref equals
SrcAssemblyStepConfiguration.Ref; update less to add a deterministic secondary
key so it establishes a strict total order: first compute refA/refB as now, then
if refA != refB return refA < refB; otherwise break ties by comparing a
deterministic discriminator such as whether SourceStepConfiguration is non-nil
vs SrcAssemblyStepConfiguration (e.g., treat SourceStepConfiguration <
SrcAssemblyStepConfiguration), or compare another stable field (like a
Name/Path/ID) from the non-nil config, ensuring the comparator always returns
either true or false for any pair so cmpopts.SortSlices gets a strict ordering.
In `@pkg/defaults/defaults.go`:
- Around line 1123-1148: The sourceStepsForRef function currently emits two
steps (SourceStepConfiguration and SrcAssemblyStepConfiguration) which allows
LOCAL_IMAGE_SRC to skip only the assembly step; update sourceStepsForRef so that
when the LOCAL_IMAGE_SRC override is present it short-circuits both steps by
omitting the SourceStepConfiguration entirely (i.e., only emit the
SrcAssemblyStepConfiguration or suppress both as appropriate) so
checkForFullyQualifiedStep cannot accidentally run the clone stage; change
behavior around constructing orgRepo/root/scratchSource/source inside
sourceStepsForRef and ensure the logic checks for the LOCAL_IMAGE_SRC env var
before returning the slice, and add a regression test that sets LOCAL_IMAGE_SRC
and asserts the clone (SourceStepConfiguration) is not present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0a17647-2903-4836-b87c-65b2644203b1
⛔ Files ignored due to path filters (2)
pkg/api/zz_generated.deepcopy.gois excluded by!**/zz_generated*pkg/webreg/zz_generated.ci_operator_reference.gois excluded by!**/zz_generated*
📒 Files selected for processing (17)
pkg/api/types.gopkg/defaults/defaults.gopkg/defaults/defaults_test.gopkg/steps/bundle_source.gopkg/steps/project_image.gopkg/steps/source.gopkg/steps/source_test.gopkg/steps/src_assembly.gopkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_basic_options_for_a_presubmit.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_title_in_pull_gets_squashed.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_OAuth_token.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_a_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_a_pull_secret.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_extra_refs.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_extra_refs_setting_workdir_and_path_alias.yamlpkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_ssh_key.yamlpkg/validation/graph.go
🚧 Files skipped from review as they are similar to previous changes (3)
- pkg/steps/source_test.go
- pkg/steps/testdata/zz_fixture_TestCreateScratchSourceBuild_with_extra_refs_setting_workdir_and_path_alias.yaml
- pkg/validation/graph.go
|
/test images |
|
/test e2e |
|
@joepvd: The following tests 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. |
2 participants
Summary
scrimage (FROM scratch) that clones source once, eliminating redundant git clones in multi-arch buildssrcAssemblyStepthat layers the cloned source fromscronto each arch-specific build root to produce per-architecturesrcimagesbin,test-bin, tests, images) continue to depend onsrcunchangedMotivation
In multi-arch CI builds, source code is architecture-independent but was being cloned separately for each architecture (
src-amd64,src-arm64). This is wasteful — the git clone is identical regardless of CPU architecture.Design
The single
sourceStep(producingsrc) is split into two steps:scr— Built once on a single architecture using a multi-stage Dockerfile. First stage clones source using the build root + clonerefs, second stage (FROM scratch) contains only/go/src. Does not implementMultiArchStep.src—srcAssemblyStepcombines the arch-specificrootwith source fromscrviaCOPY --from=scr. ImplementsMultiArchStep, producingsrc-amd64,src-arm64etc. with manifest lists as before.Test plan
pkg/api,pkg/steps,pkg/defaults,pkg/validationscris built once andsrc-{arch}variants are assembled correctlyCOPY --from=scrworks (amd64-builtscrused on arm64 build node)Made with Cursor
Summary by CodeRabbit
New Features
Tests