Skip to content

fix dropped pull progress output due to canceled context #2254

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.

Sign up for GitHub

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

Merged
tonistiigi merged 1 commit into from
Jul 15, 2021
Merged

fix dropped pull progress output due to canceled context #2254

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
6 changes: 5 additions & 1 deletion source/containerimage/pull.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,10 @@ func mainManifestKey(ctx context.Context, desc specs.Descriptor, platform specs.
func (p *puller) CacheKey(ctx context.Context, g session.Group, index int) (cacheKey string, cacheOpts solver.CacheOpts, cacheDone bool, err error) {
p.Puller.Resolver = resolver.DefaultPool.GetResolver(p.RegistryHosts, p.Ref, "pull", p.SessionManager, g).WithImageStore(p.ImageStore, p.id.ResolveMode)

// progressFactory needs the outer context, the context in `p.g.Do` will
// be canceled before the progress output is complete
progressFactory := progress.FromContext(ctx)
Copy link
Copy Markdown
Member

@tonistiigi tonistiigi Jul 14, 2021

Choose a reason for hiding this comment

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

looking at FromContext implementation it only uses context for the ctx.Value(contextKey) call and later uses another context coming with the call. So why does it matter if this context gets canceled. Also, it is possible that this context also gets canceled before the blobs are actually pulled.

Copy link
Copy Markdown
Collaborator Author

@coryb coryb Jul 14, 2021

Choose a reason for hiding this comment

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

yeah, it was not obvious to me either.

The progress factory is not used until later when we call the controller Start

buildkit/util/progress/controller/controller.go

Line 30 in 699121c

c.writer, _, _ = c.WriterFactory(ctx)

Start is eventually called here when we need to Unlazy the ref:

buildkit/cache/remote.go

Line 201 in 699121c

ctx, stopProgress = p.dh.Progress.Start(ctx)

By the time we call Start, the original context from the p.g.Do has already been canceled, the new writer immediately gets ignored via:

buildkit/util/progress/progress.go

Lines 171 to 172 in 699121c

case <-pr.ctx.Done():
return

Copy link
Copy Markdown
Collaborator Author

@coryb coryb Jul 14, 2021

Choose a reason for hiding this comment

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

As discussed in slack, it seems the reader is what is getting associated with the canceled context via the flightcontrol on g.Do:

buildkit/util/flightcontrol/flightcontrol.go

Lines 105 to 110 in 699121c

ctx := newContext(c) // newSharedContext
pr, pctx, closeProgressWriter := progress.NewContext(context.Background())
c.progressCtx = pctx
c.ctx = ctx
c.closeProgressWriter = closeProgressWriter

So this fix is really about using the reader associated with the CacheKey ctx


_, err = p.g.Do(ctx, "", func(ctx context.Context) (_ interface{}, err error) {
if p.cacheKeyErr != nil || p.cacheKeyDone == true {
return nil, p.cacheKeyErr
Expand Down Expand Up @@ -201,7 +205,7 @@ func (p *puller) CacheKey(ctx context.Context, g session.Group, index int) (cach

if len(p.manifest.Descriptors) > 0 {
progressController := &controller.Controller{
WriterFactory: progress.FromContext(ctx),
WriterFactory: progressFactory,
}
if p.vtx != nil {
progressController.Digest = p.vtx.Digest()
Expand Down