Skip to content

Allow progress also when saving to docker daemon #972

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
wants to merge 1 commit into from
Closed

Allow progress also when saving to docker daemon #972

wants to merge 1 commit into from

Conversation

afbjorklund
Copy link
Contributor

@afbjorklund afbjorklund commented Mar 21, 2021 •
edited
Loading

The initial implementation only added progress to tarball

Fortunately the daemon Write uses a temporary tarball file

@codecov-io
Copy link

Codecov Report

Merging #972 (2fafd87) into main (70c58c0) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #972   +/-   ##
=======================================
  Coverage   75.19%   75.19%           
=======================================
  Files         107      107           
  Lines        4837     4837           
=======================================
  Hits         3637     3637           
  Misses        669      669           
  Partials      531      531
Impacted Files Coverage Δ
pkg/v1/daemon/write.go 46.15% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 70c58c0...2fafd87. Read the comment docs.

@afbjorklund afbjorklund mentioned this pull request Mar 21, 2021
Merged
imjasonh
imjasonh reviewed Mar 21, 2021
View reviewed changes
pkg/v1/daemon/write.go
@@ -54,15 +54,15 @@ func Tag(src, dest name.Tag) error {
}

// Write saves the image into the daemon as the given tag.
func Write(tag name.Tag, img v1.Image) (string, error) {
func Write(tag name.Tag, img v1.Image, opts ...tarball.WriteOption) (string, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a bit weird for daemon.Write to take a tarball.Options, it leaks a bit of the underlying abstraction.

Would it make sense to define a daemon Option for progress and convert it internally into the tarball Option?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that remote.Write has the same problem, due to WriteOptions being added on the wrong level

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the right thing to do would probably be to add a daemon.WithProgress, instead of tarball.WithProgress

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah exactly. #967 defines a new remote.WithProgress. In the case of daemon, it might end up using tarball.WithProgress underneath, but it feels weird to have that leak into its API.

There isn't a daemon.WriteOption type yet, but I think we could add one, and a daemon.WithProgress that implements it, that converts to a tarball.WriteOption internally.

@jonjohnsonjr wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Otherwise it seems to be working, of course we might end up using tarball anyway for Podman support.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I've stomped on this PR a little bit by making everything just daemon.Option (sorry), but it shouldn't be too hard to do this now by just translating the daemon.WithProgress into a tarball.WithProgress.

Copy link
Contributor Author

@afbjorklund afbjorklund Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I had to fork everything anyway, to make a podman matching the daemon in the API.

https://github.com/kubernetes/minikube/pull/11063/files#diff-fa213d52c815c8c5520c1a6564d404ecf1d5ea5f3acf11dfd86a406bab4a4d3a

@afbjorklund
Copy link
Contributor Author

afbjorklund commented Mar 21, 2021 •
edited
Loading

It seems like this internal "temporary archive" also breaks the digest, which is reset to nil...

Our patched go-containerregistry does allow it in the reference, but docker doesn't load it.

        // write the image in docker save format first, then load it

This means that daemon.Write behaves differently from docker pull (which does update it)

        ImageLoad(ctx context.Context, input io.Reader, quiet bool) (types.ImageLoadResponse, error)
        ImagePull(ctx context.Context, ref string, options types.ImagePullOptions) (io.ReadCloser, error)
        ImageTag(ctx context.Context, image, ref string) error

Which breaks our check, which tries to check both tag and digest. I added* an extra ImagePull

So it will first do the ImageLoad (from stdin), and then do ImagePull - only pulling the manifest

* 761f6f9

@github-actions
Copy link

This Pull Request is stale because it has been open for 90 days with
no activity. It will automatically close after 30 more days of
inactivity. Keep fresh with the 'lifecycle/frozen' label.

@github-actions github-actions bot closed this Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjasonh imjasonh imjasonh left review comments

@jonjohnsonjr jonjohnsonjr jonjohnsonjr left review comments

Assignees
No one assigned
Labels
lifecycle/stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@afbjorklund @codecov-io @imjasonh @jonjohnsonjr