-
Notifications
You must be signed in to change notification settings - Fork 18.9k
[WIP] Support OCI image layout in docker save/load #26369
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
Conversation
ed5ccca to
75b88f4
Compare
|
@jhowardmsft do you know why the following test fails with: The test fails at: Is there anything I could do to fix this? |
75b88f4 to
955312d
Compare
|
Yes, docker save is currently broken on Windows. @jstarks |
@jhowardmsft it doesn't seem to fail when piping docker save to tar though |
|
@runcom Are you sure it's not just piping whatever comes out of stderr? AFAIK the issue is known, just not fixed, and @jstarks had a fix in the pipeline. You would see something like the following if you attempt to |
|
@jhowardmsft I was referring to the test I've added here for instance https://github.com/docker/docker/pull/26369/files#diff-00a3ffcc2e471d5f664f602aac761d7bR441. I cannot see how this test passes if the output of the command pipeline is just an error since it's doing some check on the output itself. And all the other tests that do |
|
Oh sorry, you're referring to Windows to Linux, not Windows to Windows. In which case, I don't know. |
@jhowardmsft yeah, it may be something with |
|
This is what I see logged onto one of the Windows boxes used in Win2Lin CI. From that, it looks like -x -O and -f are valid options. |
|
@jhowardmsft the error in #26369 (comment) shows something about not being able to resolve C: - no clue here |
|
Could that be that it's a posix utility and expecting /c/ rather than Windows-style drive letters? |
|
It could be, though, I don't have any way to test it since I don't have windows vm around. I guess I'll re-push and see what will happen. (or any way you can test this out quickly? :)) |
|
I don't unfortunately have an easy way to test Windows to Linux outside of CI. It would take some setting up to get me (back) to being able to do that. Note you should be able to create a free tiny Windows VM instance on Azure. Not sure how long that lasts for, or what other terms there are though. eg https://azure.microsoft.com/en-us/free/. Alternately, you could download a trial of Server 2016 TP5 and install that in a VM locally. eg https://www.microsoft.com/en-us/evalcenter/evaluate-windows-server-essentials-technical-preview |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are called "MediaTypes".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is vendored code right? I have a fix for those consts though, to be imported from docker/distribution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@runcom I understand, but this is confusing. You may want to fix these upstream. They should all be prefixed with MediaTypeDocker. Both OCI and docker specs use the term "Media Type", over mime type.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevvooe I'm making a patch but I'm not following why this is important in this PR review since those aren't even used :/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stevvooe opened containers/image#78 though...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@runcom Just commenting on what I saw, I'll provide a more thorough review in the coming days.
955312d to
d18a928
Compare
5e4738b to
86fd0f2
Compare
|
@jhowardmsft I figured - on windows the tar command needs However, it's ugly to mix both path separators when extracting so I'm using pipelines as the other tests are doing right (and work on both platform, expecting unix paths). |
9410755 to
fa6d45b
Compare
|
@stevvooe took care of your comment about Media Type(s) |
client/transport/tlsconfig_clone.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curiosity: are they really adding this in go1.8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
Thanks @stevvooe |
|
@stevvooe when is rc5 expected to be tagged so this can be moved along? |
|
@mlaventure I'm going to put up a vote for it today. So, in a week or less. |
|
@vbatts RC5 won't have the name change, so we'll have to wait for RC6. Let's get that name change merged. |
|
Rebasing this one and adapting code for image-spec RC5 |
dcb443e to
b54a08e
Compare
9aea97b to
5735ad4
Compare
|
thanks @runcom ! |
Signed-off-by: Antonio Murdaca <[email protected]>
Signed-off-by: Antonio Murdaca <[email protected]>
5735ad4 to
5483189
Compare
|
I'm lost on this one; what's the status? I see an LGTM from @stevvooe, code reviews after. Should we close this if this needs more discussion? |
|
I believe we are waiting on image spec changes. |
|
OK, @runcom let me close this PR for now, while we wait for the image specs; you should be able to reopen when we're ready to move foreward again ❤️ 👍 |
|
Last status we that @runcom had rebased on the latest image-spec RC. I don't know of any blockers for the review of this PR. |
Just going by @stevvooe's direction here. |
|
@thaJeztah Let's re-open this (not sure why I can't). |
|
Hm can't reopen either, possibly due to the repo move (seen it on other PR's) @runcom can you open a new PR from this branch? |
|
Yup, I'll open another PR, rebase and update to image-spec RC6 when it goes out |
|
@runcom Thanks! |
Closes #25779
This PR tries to tackle #25779 by supporting the OCI image layout when saving and loading Docker images.
Right now, this PR contains just the code for
docker save --format ocibecause I found various aspects that still need to be discussed (I left many comments around in the code, please review them).I'm also opening this in RFC because I didn't want to go that far and finding out that what I've been doing was completely wrong, so I'm here to gather feedback and comments to go forward (with
docker load --format oci)Changes:
docker save(plus I guessed on something likedocker save --format oci name@digestThis PR vendors containers/image andthe image spec to be able to deal with the OCI image layout and w/o the need to re-implement everything in the Docker code base.docker savenow...)TODO:
docker load --format ocidocker loadand combineddocker save --format oci | docker load -Alright, here's the net result as a simple example of
docker save, please do review the integration tests to have a picture of how this looks like in other scenarios:/cc @stevvooe @aaronlehmann @vdemeester @vbatts @rhatdan @cpuguy83 @duglin @icecrime @estesp @tonistiigi