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

containerd integration: produce progress events polling ctrd's content.Store #43819

Closed
wants to merge 1 commit into from

Conversation

thaJeztah
Copy link
Member

- Description for the changelog

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

@thaJeztah
Copy link
Member Author

Minor fix needed;

daemon/containerd/progress.go:17: File is not `goimports`-ed (goimports)
	"github.com/opencontainers/image-spec/specs-go/v1"
daemon/containerd/progress.go:100:2: `name` is unused (structcheck)
	name     string
	^

return err
}

unpacked, err := img.IsUnpacked(ctx, containerd.DefaultSnapshotter)
Copy link
Member

Choose a reason for hiding this comment

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

(the snapshotter has to be eventually configurable, in a follow-up PR)

_, err = cs.client.Pull(ctx, ref.String(), opts...)
jobs := newJobs()
h := containerdimages.HandlerFunc(func(ctx context.Context, desc ocispec.Descriptor) ([]ocispec.Descriptor, error) {
if desc.MediaType != containerdimages.MediaTypeDockerSchema1Manifest {
Copy link
Member

Choose a reason for hiding this comment

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

Should we return an error for schema1?

Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
return err
}
}
stop <- struct{}{}
Copy link
Member

Choose a reason for hiding this comment

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

What's the purpose of this? Seems like this races with your goroutine above.

Copy link
Member

Choose a reason for hiding this comment

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

Seems like we can close the channel (from the goroutine) rather than sending?

Copy link
Contributor

Choose a reason for hiding this comment

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

closing channel would be a valid alternative, indeed

@thaJeztah thaJeztah added the containerd-integration Issues and PRs related to containerd integration label Jul 21, 2022
@thaJeztah
Copy link
Member Author

Discussing; we can include rumpl#16 here, but also need to have a look at

moby/plugin/fetch_linux.go

Lines 198 to 285 in f068067

func withFetchProgress(cs content.Store, out progress.Output, ref reference.Named) images.HandlerFunc {
return func(ctx context.Context, desc specs.Descriptor) ([]specs.Descriptor, error) {
switch desc.MediaType {
case specs.MediaTypeImageManifest, images.MediaTypeDockerSchema2Manifest:
tn := reference.TagNameOnly(ref)
tagged := tn.(reference.Tagged)
progress.Messagef(out, tagged.Tag(), "Pulling from %s", reference.FamiliarName(ref))
progress.Messagef(out, "", "Digest: %s", desc.Digest.String())
return nil, nil
case
images.MediaTypeDockerSchema2LayerGzip,
images.MediaTypeDockerSchema2Layer,
specs.MediaTypeImageLayer,
specs.MediaTypeImageLayerGzip:
default:
return nil, nil
}
id := stringid.TruncateID(desc.Digest.String())
if _, err := cs.Info(ctx, desc.Digest); err == nil {
out.WriteProgress(progress.Progress{ID: id, Action: "Already exists", LastUpdate: true})
return nil, nil
}
progress.Update(out, id, "Waiting")
key := remotes.MakeRefKey(ctx, desc)
go func() {
timer := time.NewTimer(100 * time.Millisecond)
if !timer.Stop() {
<-timer.C
}
defer timer.Stop()
var pulling bool
var ctxErr error
for {
timer.Reset(100 * time.Millisecond)
select {
case <-ctx.Done():
ctxErr = ctx.Err()
// make sure we can still fetch from the content store
// TODO: Might need to add some sort of timeout
ctx = context.Background()
case <-timer.C:
}
s, err := cs.Status(ctx, key)
if err != nil {
if !c8derrdefs.IsNotFound(err) {
logrus.WithError(err).WithField("layerDigest", desc.Digest.String()).Error("Error looking up status of plugin layer pull")
progress.Update(out, id, err.Error())
return
}
if _, err := cs.Info(ctx, desc.Digest); err == nil {
progress.Update(out, id, "Download complete")
return
}
if ctxErr != nil {
progress.Update(out, id, ctxErr.Error())
return
}
continue
}
if !pulling {
progress.Update(out, id, "Pulling fs layer")
pulling = true
}
if s.Offset == s.Total {
out.WriteProgress(progress.Progress{ID: id, Action: "Download complete", Current: s.Offset, LastUpdate: true})
return
}
out.WriteProgress(progress.Progress{ID: id, Action: "Downloading", Current: s.Offset, Total: s.Total})
}
}()
return nil, nil
}
}
to see if we can share the existing implementation

@rumpl
Copy link
Member

rumpl commented Jan 10, 2023

This one can be closed, superseeded by #44756

@cpuguy83 cpuguy83 closed this Jan 10, 2023
containerd integration automation moved this from In progress to Done Jan 10, 2023
@thaJeztah thaJeztah deleted the containerd_progress branch May 8, 2023 07:29
@thaJeztah thaJeztah removed area/distribution status/2-code-review containerd-integration Issues and PRs related to containerd integration labels May 8, 2023
@thaJeztah thaJeztah removed this from the 24.0.0 milestone May 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

5 participants