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

llbsolver: create single temp lease for exports for performance #4947

Merged
merged 1 commit into from
May 27, 2024
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
2 changes: 1 addition & 1 deletion cache/remote.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ func (sr *immutableRef) GetRemotes(ctx context.Context, createIfNeeded bool, ref
}

// Search all available remotes that has the topmost blob with the specified
// compression with all combination of copmressions
// compression with all combination of compressions
res := []*solver.Remote{remote}
topmost, parentChain := remote.Descriptors[len(remote.Descriptors)-1], remote.Descriptors[:len(remote.Descriptors)-1]
vDesc, err := getBlobWithCompression(ctx, sr.cm.ContentStore, topmost, refCfg.Compression.Type)
Expand Down
4 changes: 2 additions & 2 deletions solver/llbsolver/history.go
Original file line number Diff line number Diff line change
Expand Up @@ -366,11 +366,11 @@ func (h *HistoryQueue) addResource(ctx context.Context, l leases.Lease, desc *co
}
if _, err := h.hContentStore.Info(ctx, desc.Digest); err != nil {
if errdefs.IsNotFound(err) {
ctx, release, err := leaseutil.WithLease(ctx, h.hLeaseManager, leases.WithID("history_migration_"+identity.NewID()), leaseutil.MakeTemporary)
lr, ctx, err := leaseutil.NewLease(ctx, h.hLeaseManager, leases.WithID("history_migration_"+identity.NewID()), leaseutil.MakeTemporary)
Copy link
Member

Choose a reason for hiding this comment

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

Is this ctx intended to be used by the call on 383 as well?

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess lease on a context doesn't do anything for a AddResource call. Technically the lease created here is on the right namespace for that leasemanger but if this if statement is not reached I don't think we need to add lease for only the AddResource call.

if err != nil {
return err
}
defer release(ctx)
defer lr.Discard()
ok, err := h.migrateBlobV2(ctx, string(desc.Digest), detectSkipLayers)
if err != nil {
return err
Expand Down
33 changes: 29 additions & 4 deletions solver/llbsolver/solver.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/moby/buildkit/util/compression"
"github.com/moby/buildkit/util/entitlements"
"github.com/moby/buildkit/util/grpcerrors"
"github.com/moby/buildkit/util/leaseutil"
"github.com/moby/buildkit/util/progress"
"github.com/moby/buildkit/util/tracing/detect"
"github.com/moby/buildkit/worker"
Expand Down Expand Up @@ -156,7 +157,7 @@ func (s *Solver) Bridge(b solver.Builder) frontend.FrontendLLBBridge {
return s.bridge(b)
}

func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend.SolveRequest, exp ExporterRequest, j *solver.Job, usage *resources.SysSampler) (func(*Result, []exporter.DescriptorReference, error) error, error) {
func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend.SolveRequest, exp ExporterRequest, j *solver.Job, usage *resources.SysSampler) (func(context.Context, *Result, []exporter.DescriptorReference, error) error, error) {
stopTrace, err := detect.Recorder.Record(ctx)
if err != nil {
return nil, err
Expand Down Expand Up @@ -184,7 +185,7 @@ func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend
return nil, err
}

return func(res *Result, descrefs []exporter.DescriptorReference, err error) error {
return func(ctx context.Context, res *Result, descrefs []exporter.DescriptorReference, err error) error {
en := time.Now()
rec.CompletedAt = &en

Expand All @@ -197,7 +198,7 @@ func (s *Solver) recordBuildHistory(ctx context.Context, id string, req frontend
}
}

ctx, cancel := context.WithCancelCause(context.Background())
ctx, cancel := context.WithCancelCause(ctx)
ctx, _ = context.WithTimeoutCause(ctx, 300*time.Second, errors.WithStack(context.DeadlineExceeded))
defer cancel(errors.WithStack(context.Canceled))

Expand Down Expand Up @@ -509,7 +510,7 @@ func (s *Solver) Solve(ctx context.Context, id string, sessionID string, req fro
return nil, err1
}
defer func() {
err = rec(resProv, descrefs, err)
err = rec(context.WithoutCancel(ctx), resProv, descrefs, err)
}()
}

Expand Down Expand Up @@ -585,6 +586,22 @@ func (s *Solver) Solve(ctx context.Context, id string, sessionID string, req fro
return nil, err
}

// Functions that create new objects in containerd (eg. content blobs) need to have a lease to ensure
// that the object is not garbage collected immediately. This is protected by the indivual components,
// but because creating a lease is not cheap and requires a disk write, we create a single lease here
// early and let all the exporters, cache export and provenance creation use the same one.
lm, err := s.leaseManager()
if err != nil {
return nil, err
}
ctx, done, err := leaseutil.WithLease(ctx, lm, leaseutil.MakeTemporary)
if err != nil {
return nil, err
}
releasers = append(releasers, func() {
done(context.WithoutCancel(ctx))
})

cacheExporters, inlineCacheExporter := splitCacheExporters(exp.CacheExporters)

var exporterResponse map[string]string
Expand Down Expand Up @@ -747,6 +764,14 @@ func (s *Solver) runExporters(ctx context.Context, exporters []exporter.Exporter
return exporterResponse, descs, nil
}

func (s *Solver) leaseManager() (*leaseutil.Manager, error) {
w, err := defaultResolver(s.workerController)()
if err != nil {
return nil, err
}
return w.LeaseManager(), nil
}

func splitCacheExporters(exporters []RemoteCacheExporter) (rest []RemoteCacheExporter, inline inlineCacheExporter) {
rest = make([]RemoteCacheExporter, 0, len(exporters))
for _, exp := range exporters {
Expand Down
Loading