containerd integration: image pull#44756
Merged
neersighted merged 4 commits intomoby:masterfrom Jan 11, 2023
Merged
Conversation
rumpl
requested review from
corhere,
cpuguy83,
neersighted,
thaJeztah and
vvoland
rumpl
force-pushed
the
containerd-image-pull
branch
from
96c03ec to
af4507f
Compare
rumpl
added
the
containerd-integration
thaJeztah
added
area/distribution
rumpl
force-pushed
the
containerd-image-pull
branch
3 times, most recently
from
b4cf150 to
8ee91f3
Compare
corhere
requested changes
Jan 6, 2023
Contributor
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
The big picture looks good, but some more work is needed in the details.
| func (f httpFallback) RoundTrip(r *http.Request) (*http.Response, error) { | ||
| resp, err := f.super.RoundTrip(r) | ||
| if err != nil { | ||
| if strings.Contains(err.Error(), "http: server gave HTTP response to HTTPS client") { |
Contributor
There was a problem hiding this comment.
Lame, but there's no alternative. The standard library returns an errors.New error for this case. Anyone wanna write a Go language proposal?
Member
Author
There was a problem hiding this comment.
I'll leave that one to you :)
rumpl
force-pushed
the
containerd-image-pull
branch
from
8ee91f3 to
86028fd
Compare
corhere
reviewed
Jan 9, 2023
daemon/containerd/progress.go
Outdated
|
|
||
| type updateProgressFunc func(ctx context.Context, ongoing *jobs, output progress.Output, start time.Time) error | ||
|
|
||
| func showProgress(ctx context.Context, ongoing *jobs, out progress.Output, updateFunc updateProgressFunc) func() { |
Contributor
There was a problem hiding this comment.
Since jobs is only useful in conjunction with showProgress and vice versa, I feel that association should be formalized.
Suggested change
| func showProgress(ctx context.Context, ongoing *jobs, out progress.Output, updateFunc updateProgressFunc) func() { | |
| func (j *jobs) showProgress(ctx context.Context, out progress.Output, updateFunc updateProgressFunc) func() { |
rumpl
force-pushed
the
containerd-image-pull
branch
from
86028fd to
30af35f
Compare
Member
Author
|
CI failures look unrelated |
corhere
reviewed
Jan 10, 2023
Contributor
corhere
left a comment
corhere
left a comment
There was a problem hiding this comment.
LGTM, aside from one thing I missed in my earlier reviews. My bad.
cpuguy83
approved these changes
Jan 10, 2023
Signed-off-by: Nicolas De Loof <[email protected]> Signed-off-by: Sebastiaan van Stijn <[email protected]> containerd: Push progress Signed-off-by: Paweł Gronowski <[email protected]>
We pass WithPullUnpack anyway Signed-off-by: Paweł Gronowski <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]> Signed-off-by: Nicolas De Loof <[email protected]>
Signed-off-by: Paweł Gronowski <[email protected]>
rumpl
force-pushed
the
containerd-image-pull
branch
from
30af35f to
9032e67
Compare
vvoland
approved these changes
Jan 11, 2023
corhere
approved these changes
Jan 11, 2023
Member
|
Going to bring this one in as it has reached acceptance. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
- What I did
Upstreaming:
c8d/pull: Add options for stargz/nydus snapshotters rumpl/moby#42Implements
docker pullwith containerd.Also includes stargz and nydus snapshotter handling, these two are lazy ones and need additional handlers during pull(pre-emptively removing this from the changeset to avoid too much comments, will add this one later)Limiting concurrent downloads is not yet implemented, see here for the discussion, the way containerd handles limits is different than what moby does, I think we can implement limiting in a followup
Note: I will squash the commits once it's reviewed, it's easier to review with different commits