Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Build each platform concurrently #527

Merged
merged 3 commits into from
Dec 9, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 42 additions & 18 deletions pkg/build/gobuild.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"os/exec"
"path"
"path/filepath"
"runtime"
"strconv"
"strings"
"text/template"
Expand All @@ -48,6 +49,8 @@ import (
"github.com/sigstore/cosign/pkg/oci/signed"
"github.com/sigstore/cosign/pkg/oci/static"
ctypes "github.com/sigstore/cosign/pkg/types"
"golang.org/x/sync/errgroup"
"golang.org/x/sync/semaphore"
"golang.org/x/tools/go/packages"
)

Expand Down Expand Up @@ -78,6 +81,7 @@ type gobuild struct {
platformMatcher *platformMatcher
dir string
labels map[string]string
semaphore *semaphore.Weighted

cache *layerCache
}
Expand All @@ -97,6 +101,7 @@ type gobuildOpener struct {
platform string
labels map[string]string
dir string
jobs int
}

func (gbo *gobuildOpener) Open() (Interface, error) {
Expand All @@ -107,6 +112,9 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
if err != nil {
return nil, err
}
if gbo.jobs == 0 {
gbo.jobs = runtime.GOMAXPROCS(0)
}
return &gobuild{
getBase: gbo.getBase,
creationTime: gbo.creationTime,
Expand All @@ -123,6 +131,7 @@ func (gbo *gobuildOpener) Open() (Interface, error) {
buildToDiff: map[string]buildIDToDiffID{},
diffToDesc: map[string]diffIDToDescriptor{},
},
semaphore: semaphore.NewWeighted(int64(gbo.jobs)),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, do we Open() once per ko invocation or once per build? 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's once per "builder", which means the --jobs isn't shared across build configs... which is annoying, but I'm happy to fix that if someone actually complains...

}, nil
}

Expand Down Expand Up @@ -642,6 +651,11 @@ func (g *gobuild) configForImportPath(ip string) Config {
}

func (g *gobuild) buildOne(ctx context.Context, refStr string, base v1.Image, platform *v1.Platform) (oci.SignedImage, error) {
if err := g.semaphore.Acquire(ctx, 1); err != nil {
return nil, err
}
defer g.semaphore.Release(1)

ref := newRef(refStr)

cf, err := base.ConfigFile()
Expand Down Expand Up @@ -872,9 +886,12 @@ func (g *gobuild) buildAll(ctx context.Context, ref string, baseIndex v1.ImageIn
return nil, err
}

errg, ctx := errgroup.WithContext(ctx)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we want this to share it's threadpool with the --jobs pool, right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plumbed this down into buildOne


// Build an image for each child from the base and append it to a new index to produce the result.
adds := []ocimutate.IndexAddendum{}
for _, desc := range im.Manifests {
adds := make([]ocimutate.IndexAddendum, len(im.Manifests))
for i, desc := range im.Manifests {
i, desc := i, desc
// Nested index is pretty rare. We could support this in theory, but return an error for now.
if desc.MediaType != types.OCIManifestSchema1 && desc.MediaType != types.DockerManifestSchema2 {
return nil, fmt.Errorf("%q has unexpected mediaType %q in base for %q", desc.Digest, desc.MediaType, ref)
Expand All @@ -884,25 +901,32 @@ func (g *gobuild) buildAll(ctx context.Context, ref string, baseIndex v1.ImageIn
continue
}

baseImage, err := baseIndex.Image(desc.Digest)
if err != nil {
return nil, err
}
img, err := g.buildOne(ctx, ref, baseImage, desc.Platform)
if err != nil {
return nil, err
}
adds = append(adds, ocimutate.IndexAddendum{
Add: img,
Descriptor: v1.Descriptor{
URLs: desc.URLs,
MediaType: desc.MediaType,
Annotations: desc.Annotations,
Platform: desc.Platform,
},
errg.Go(func() error {
baseImage, err := baseIndex.Image(desc.Digest)
if err != nil {
return err
}
img, err := g.buildOne(ctx, ref, baseImage, desc.Platform)
if err != nil {
return err
}
adds[i] = ocimutate.IndexAddendum{
Add: img,
Descriptor: v1.Descriptor{
URLs: desc.URLs,
MediaType: desc.MediaType,
Annotations: desc.Annotations,
Platform: desc.Platform,
},
}
return nil
})
}

if err := errg.Wait(); err != nil {
return nil, err
}

baseType, err := baseIndex.MediaType()
if err != nil {
return nil, err
Expand Down
2 changes: 2 additions & 0 deletions pkg/build/limit.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ func (l *Limiter) Build(ctx context.Context, ip string) (Result, error) {
}

// NewLimiter returns a new builder that only allows n concurrent builds of b.
//
// Deprecated: Obsoleted by WithJobs option.
func NewLimiter(b Interface, n int) *Limiter {
return &Limiter{
Builder: b,
Expand Down
8 changes: 8 additions & 0 deletions pkg/build/options.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,11 @@ func withSBOMber(sbom sbomber) Option {
return nil
}
}

// WithJobs limits the number of concurrent builds.
func WithJobs(jobs int) Option {
return func(gbo *gobuildOpener) error {
gbo.jobs = jobs
return nil
}
}
7 changes: 1 addition & 6 deletions pkg/commands/resolver.go
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ import (
"log"
"os"
"path"
"runtime"
"strings"
"sync"

Expand Down Expand Up @@ -89,6 +88,7 @@ func gobuildOptions(bo *options.BuildOptions) ([]build.Option, error) {
opts := []build.Option{
build.WithBaseImages(getBaseImage(platform, bo)),
build.WithPlatforms(platform),
build.WithJobs(bo.ConcurrentBuilds),
}
if creationTime != nil {
opts = append(opts, build.WithCreationTime(*creationTime))
Expand Down Expand Up @@ -141,11 +141,6 @@ func makeBuilder(ctx context.Context, bo *options.BuildOptions) (*build.Caching,
return nil, err
}

if bo.ConcurrentBuilds == 0 {
bo.ConcurrentBuilds = runtime.GOMAXPROCS(0)
}
innerBuilder = build.NewLimiter(innerBuilder, bo.ConcurrentBuilds)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this now dead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is, but build.NewLimiter is public so I'm somewhat reluctant to just delete it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we mark it as Deprecated at least, and delete it in the future? If someone depends on it (highly unlikely) and it's not actively used by the codebase that's not a great place to be in either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.


// tl;dr Wrap builder in a caching builder.
//
// The caching builder should on Build calls:
Expand Down