diff --git a/pkg/steps/bundle_source.go b/pkg/steps/bundle_source.go index e41d2256b4..d2edf7f5ab 100644 --- a/pkg/steps/bundle_source.go +++ b/pkg/steps/bundle_source.go @@ -80,8 +80,9 @@ func (s *bundleSourceStep) run(ctx context.Context) error { ) // Bundle images are not multi-arch by design. Here we build it without creating a manifest-listed image. - // Note that we are not configuring a node selector here, so the build will be scheduled on any available - // node no matter the architecture. + // The build must still run on a node whose architecture can resolve its manifest-listed inputs + // (e.g. pipeline:src) — see pinBuildToSingleArchNode. + pinBuildToSingleArchNode(build) return handleBuild(ctx, s.client, s.podClient, *build) } diff --git a/pkg/steps/bundle_source_test.go b/pkg/steps/bundle_source_test.go index d30e610598..7e4d6fd42a 100644 --- a/pkg/steps/bundle_source_test.go +++ b/pkg/steps/bundle_source_test.go @@ -2,20 +2,31 @@ package steps import ( "bytes" + "context" + "encoding/json" + "fmt" "os" "os/exec" "path/filepath" "runtime" "strings" "testing" + "time" + coreapi "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + k8sruntime "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/util/sets" + ctrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client" fakectrlruntimeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + buildapi "github.com/openshift/api/build/v1" + "github.com/openshift/api/image/docker10" imagev1 "github.com/openshift/api/image/v1" "github.com/openshift/ci-tools/pkg/api" "github.com/openshift/ci-tools/pkg/steps/loggingclient" + testhelper_kube "github.com/openshift/ci-tools/pkg/testhelper/kubernetes" ) var subs = []api.PullSpecSubstitution{ @@ -163,3 +174,143 @@ RUN find . -type f -regex ".*\.\(yaml\|yml\)" -exec sed -i s?quay.io/openshift/o t.Errorf("Generated bundle source dockerfile does not equal expected; generated dockerfile: %s", generatedDockerfile) } } + +// pendingOnCreateBuildClient stamps newly created builds as Pending before they're persisted, +// mimicking what a real build controller does immediately on admission. Without this, a build +// created by the fake client sits at its zero-value phase forever (nothing ever transitions it), +// so waitForBuild's pending-check — gated on seeing phase New/Pending on the *first* watched +// event — never fires and the wait blocks until the context is done. +type pendingOnCreateBuildClient struct { + BuildClient +} + +func (c pendingOnCreateBuildClient) Create(ctx context.Context, obj ctrlruntimeclient.Object, opts ...ctrlruntimeclient.CreateOption) error { + if b, ok := obj.(*buildapi.Build); ok { + b.Status.Phase = buildapi.BuildPhasePending + } + return c.BuildClient.Create(ctx, obj, opts...) +} + +// TestBundleSourceStepRunPinsBuildToSingleArchNode is a direct unit test of the code path +// pinBuildToSingleArchNode is called from: it exercises bundleSourceStep.Run and inspects the +// actual build object submitted to the API, rather than the invariants +// TestSingleArchBuildPinMatchesPipelineArchGuarantees checks in isolation. +func TestBundleSourceStepRunPinsBuildToSingleArchNode(t *testing.T) { + const namespace = "target-namespace" + metadata, err := json.Marshal(docker10.DockerImage{Config: &docker10.DockerConfig{WorkingDir: "/go/src"}}) + if err != nil { + t.Fatalf("failed to marshal image metadata: %v", err) + } + underlying := &buildClient{LoggingClient: loggingclient.New(fakectrlruntimeclient.NewClientBuilder().WithRuntimeObjects( + &imagev1.ImageStreamTag{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: namespace, + Name: fmt.Sprintf("%s:%s", api.PipelineImageStream, api.PipelineImageStreamTagReferenceSource), + }, + Image: imagev1.Image{ + ObjectMeta: metav1.ObjectMeta{Name: "source-digest"}, + DockerImageMetadata: k8sruntime.RawExtension{Raw: metadata}, + }, + }, + ).Build(), nil)} + client := pendingOnCreateBuildClient{BuildClient: underlying} + podClient := &testhelper_kube.FakePodClient{ + FakePodExecutor: &testhelper_kube.FakePodExecutor{LoggingClient: client}, + PendingTimeout: 0, + } + + s := &bundleSourceStep{ + jobSpec: &api.JobSpec{}, + client: client, + podClient: podClient, + } + s.jobSpec.SetNamespace(namespace) + + ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) + defer cancel() + err = s.Run(ctx) + if err == nil || !strings.Contains(err.Error(), "didn't start running") { + t.Fatalf("expected a pending-timeout error proving the build was created and watched, got: %v", err) + } + + created := &buildapi.Build{} + key := ctrlruntimeclient.ObjectKey{Namespace: namespace, Name: string(api.PipelineImageStreamTagReferenceBundleSource)} + if err := client.Get(context.Background(), key, created); err != nil { + t.Fatalf("failed to get created build: %v", err) + } + if arch := created.Spec.NodeSelector[coreapi.LabelArchStable]; arch != string(api.NodeArchitectureAMD64) { + t.Errorf("Run() submitted a build with %s=%q, want %q", coreapi.LabelArchStable, arch, api.NodeArchitectureAMD64) + } +} + +// TestSingleArchBuildPinMatchesPipelineArchGuarantees guards the cross-file invariant behind +// pinBuildToSingleArchNode (used for bundle builds here and in project_image.go): pinning bundle +// builds to one architecture is only safe while every pipeline manifest list is guaranteed to +// contain that architecture. If any assertion below fails, that guarantee changed — update +// pinBuildToSingleArchNode together with the change that broke it. +func TestSingleArchBuildPinMatchesPipelineArchGuarantees(t *testing.T) { + pinned := &buildapi.Build{} + pinBuildToSingleArchNode(pinned) + pinnedArch, ok := pinned.Spec.NodeSelector[coreapi.LabelArchStable] + if !ok { + t.Fatalf("pinBuildToSingleArchNode did not set a %s node selector", coreapi.LabelArchStable) + } + + // This subtest never calls pinBuildToSingleArchNode: it asserts the pipeline-side + // invariant the pin depends on (default builds target the pinned arch), so a change + // to constructMultiArchBuilds that breaks that invariant fails here first. + t.Run("default pipeline build is the pinned architecture", func(t *testing.T) { + builds := constructMultiArchBuilds(buildapi.Build{}, nil) + if len(builds) != 1 { + t.Fatalf("expected exactly one default build, got %d", len(builds)) + } + if got := builds[0].Spec.NodeSelector[coreapi.LabelArchStable]; got != pinnedArch { + t.Errorf("default pipeline build targets %q, but bundle builds pin to %q", got, pinnedArch) + } + }) + + // Also does not call pinBuildToSingleArchNode: guards ResolveMultiArch() itself, since a + // change there could drop the pinned arch from the resolved set without touching the pin. + t.Run("image steps resolve the pinned architecture unconditionally", func(t *testing.T) { + step := &projectDirectoryImageBuildStep{ + config: api.ProjectDirectoryImageBuildStepConfiguration{ + AdditionalArchitectures: []string{"arm64"}, + }, + architectures: sets.New[string](), + } + if resolved := step.ResolveMultiArch(); !resolved.Has(pinnedArch) { + t.Errorf("ResolveMultiArch() = %v does not contain %q, so a pipeline manifest list without a %q entry is now possible", sets.List(resolved), pinnedArch, pinnedArch) + } + }) + + // Also does not call pinBuildToSingleArchNode: guards constructMultiArchBuilds() across + // multiple configured architectures, the case pinning a bundle build actually relies on. + t.Run("multi-arch pipelines still build the pinned architecture", func(t *testing.T) { + step := &projectDirectoryImageBuildStep{ + config: api.ProjectDirectoryImageBuildStepConfiguration{ + AdditionalArchitectures: []string{"arm64", "ppc64le"}, + }, + architectures: sets.New[string](), + } + found := false + for _, b := range constructMultiArchBuilds(buildapi.Build{}, sets.List(step.ResolveMultiArch())) { + if b.Spec.NodeSelector[coreapi.LabelArchStable] == pinnedArch { + found = true + } + } + if !found { + t.Errorf("no %q build produced for a multi-arch pipeline, so its manifest list would lack the entry bundle builds pin to", pinnedArch) + } + }) + + t.Run("pinning merges into an existing node selector", func(t *testing.T) { + build := &buildapi.Build{Spec: buildapi.BuildSpec{CommonSpec: buildapi.CommonSpec{NodeSelector: map[string]string{"foo": "bar"}}}} + pinBuildToSingleArchNode(build) + if build.Spec.NodeSelector["foo"] != "bar" { + t.Error("pinBuildToSingleArchNode replaced the pre-existing node selector instead of merging into it") + } + if build.Spec.NodeSelector[coreapi.LabelArchStable] != pinnedArch { + t.Errorf("pinBuildToSingleArchNode did not set %s=%s", coreapi.LabelArchStable, pinnedArch) + } + }) +} diff --git a/pkg/steps/project_image.go b/pkg/steps/project_image.go index bf605388cf..a062d6f54a 100644 --- a/pkg/steps/project_image.go +++ b/pkg/steps/project_image.go @@ -71,7 +71,10 @@ func (s *projectDirectoryImageBuildStep) run(ctx context.Context) error { ) // Bundle images are non multi-arch by design. No manifest list is needed. Here we spawn a single build. + // The build must still run on a node whose architecture can resolve its manifest-listed inputs — + // see pinBuildToSingleArchNode. if s.config.IsBundleImage() { + pinBuildToSingleArchNode(build) return handleBuild(ctx, s.client, s.podClient, *build) } diff --git a/pkg/steps/source.go b/pkg/steps/source.go index d4ea4aa4a4..0b24bc6d86 100644 --- a/pkg/steps/source.go +++ b/pkg/steps/source.go @@ -525,6 +525,20 @@ func handleBuilds(ctx context.Context, buildClient BuildClient, podClient kubern return utilerrors.NewAggregate(errs) } +// pinBuildToSingleArchNode pins a non-manifest-listed build (bundle images) to nodes of the one +// architecture every pipeline manifest list is guaranteed to contain: constructMultiArchBuilds +// builds NodeArchitectureAMD64 by default and AdditionalArchitectures/Capabilities are additive +// (see ProjectDirectoryImageBuildStepConfiguration.AllCapabilities and ResolveMultiArch), so amd64 +// nodes can always resolve the build's inputs. Selector keys other than the architecture label are +// merged rather than replaced so a selector set earlier on the build survives; any pre-existing +// architecture value is intentionally overwritten with the pinned architecture. +func pinBuildToSingleArchNode(build *buildapi.Build) { + if build.Spec.NodeSelector == nil { + build.Spec.NodeSelector = map[string]string{} + } + build.Spec.NodeSelector[corev1.LabelArchStable] = string(api.NodeArchitectureAMD64) +} + // constructMultiArchBuilds gets a specific build and constructs multiple builds for each architecture. // The name and the output image of the build is suffixed with the architecture name and it will include the nodeSelector for the specific architecture. // e.x if the build name is "foo" and the architectures are "amd64,arm64", the new builds will be "foo-amd64" and "foo-arm64".