tarexport: Plumb ctx, add OTEL spans, handle cancellation

[vvoland]

Pass context.Context through tarexport.Load and tarexport.Save. Create OTEL spans for the most time consuming operations.

Also, handle context cancellations to actually end saving/loading when the operation is cancelled - before this PR the daemon would still be performing the operation even though the user already cancelled it.

- How to verify it

- Description for the changelog

Fix `docker save` and `docker load` not ending on the daemon side when the operation was cancelled (eg. Ctrl+C)

- A picture of a cute animal (not mandatory but encouraged)

[cpuguy83]

Should we check this down at L581 as well? (sorry GH won't let me add a comment there)

[vvoland]

Not sure - it's calling system.Chtimes on two paths. Doesn't look like it has a potential to take a noticeable amount of time.

[cpuguy83]

Need a doc string here.

Also I think something like an io.PipeReader would be a better for the use case which you can call CloseWithError(ctx.Err()) on and handle cancellation a little more completely (rather than be potentially blocked on reading the underlying reader).

e.g.

pr, pw := io.Pipe()

go func() {
  <-ctx.Done()
  pr.CloseWithError(ctx.Err())
}()

// ...

Or we can forgo the pipe and just close the reader normally when the context is cancelled and check after the io.Copy if it was cancelled.

[vvoland]

I'm not sure if the pipe actually solves anything though.

If the underlying reader Read method blocks indefinitely, then there's actually no way of stopping it, other than calling Close on the reader if it's actually also an io.Closer and hoping that ends all pending Reads.

I would prefer to avoid doing any Close inside the wrapper, because of the defer closer.Close() being actually the most common way of handling calling close. Doing that could potentially result in Close being called twice (first in the wrapper Read call, and then in the defer), which is implementation specific and can lead to subtle bugs like #46418.

[vvoland]

Added a CopyCtx as a replacement for the io.Copy as a best-effort recovery in a situation where Read blocks completely - in this case the calling goroutine won't be blocked, only the child goroutine that does the io.Copy.

[thaJeztah]

This is the only part I have some questions about; not necessarily a blocker, but just cautious where we extend anything in pkg/;

• Do we expect these functions to be used externally (i.e., will we need these in, e.g. buildkit or elsewhere)?
• ☝️ if not (or at least not currently) perhaps we should consider moving this to an internal/ package
• ☝️ if we DO need those externally; is pkg/ioutils the correct package for this, or should it be a separate package? (e.g. contextreader / contextreader.New() (I'd like to be sure we don't end up making ioutils too much of a "utils" package, and it to become a "grab bag" or only "somewhat" related functionality).

[vvoland]

Hmm right, I don't expect it to be used outside of moby, so I'm definitely +1 on making it internal.

We can always make it public if we need to, but going the reverse is always harder.

Let me move it to internal.

[thaJeztah]

Thanks! Yes, trying to be more conservative a bit on expanding things in pkg/ ❤️

[thaJeztah]

I was curious as well if we checked if the Go team is considering something along these lines (and/or if there's proposals), and if we in that case could align to whatever design may be worked on.

I did find this proposal, but haven't read in-depth if there's any progress / good proposal on design there;

• proposal: io: add Context parameter to Reader, etc. golang/go#20280

[laurazard]

LGTM

[thaJeztah]

LGTM