Skip to content

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

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

Jump to bottom
Closed
thaJeztah wants to merge 1 commit into from
Closed

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

thaJeztah wants to merge 1 commit into from

Conversation

@thaJeztah
Copy link
Member

@thaJeztah thaJeztah commented Jul 15, 2022

- Description for the changelog

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

@thaJeztah thaJeztah added this to the v-next milestone Jul 15, 2022
@thaJeztah
Copy link
Member Author

thaJeztah commented Jul 15, 2022

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
	^

@thaJeztah thaJeztah force-pushed the containerd_progress branch from f99d612 to da7afb6 Compare July 15, 2022 14:14
AkihiroSuda
AkihiroSuda reviewed Jul 16, 2022
View reviewed changes
daemon/containerd/service.go Outdated
return err
}

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

@AkihiroSuda AkihiroSuda Jul 16, 2022

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)

AkihiroSuda
AkihiroSuda reviewed Jul 16, 2022
View reviewed changes
daemon/containerd/service.go Outdated
_, 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

@AkihiroSuda AkihiroSuda Jul 16, 2022

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?

@ndeloof @thaJeztah
produce progress events polling ctrd's content.Store
11e6176 
Signed-off-by: Nicolas De Loof <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>
@thaJeztah thaJeztah force-pushed the containerd_progress branch from da7afb6 to 11e6176 Compare July 18, 2022 18:46
cpuguy83
cpuguy83 requested changes Jul 18, 2022
View reviewed changes
daemon/containerd/image_pull.go
return err
}
}
stop <- struct{}{}
Copy link
Member

@cpuguy83 cpuguy83 Jul 18, 2022

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

@cpuguy83 cpuguy83 Jul 18, 2022

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

@ndeloof ndeloof Jul 25, 2022

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

thaJeztah commented Aug 8, 2022

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

This was referenced Sep 1, 2022
Merged
Merged
Merged
Merged
Merged
Merged
@rumpl
Copy link
Member

rumpl commented Jan 10, 2023

This one can be closed, superseeded by #44756

@cpuguy83 cpuguy83 closed this Jan 10, 2023
@thaJeztah thaJeztah deleted the containerd_progress branch May 8, 2023 07:29
@thaJeztah thaJeztah removed area/distribution Image 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

Reviewers

@AkihiroSuda AkihiroSuda AkihiroSuda left review comments

@cpuguy83 cpuguy83 cpuguy83 requested changes

+1 more reviewer

@ndeloof ndeloof ndeloof left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@thaJeztah @rumpl @ndeloof @cpuguy83 @AkihiroSuda