--transport docker-daemon support #1776
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for the docker-daemon transport. The changes are generally moving in the right direction, but there's a critical issue regarding the inconsistent handling of the "docker" transport string. It's defined with two different meanings in different parts of the code, which could lead to confusion and bugs. I've provided a suggestion to resolve this inconsistency. Additionally, there's a minor code redundancy that could be cleaned up.
ericcurtin
force-pushed
the
master
branch
2 times, most recently
from
b8e0810 to
4e00c6e
Compare
ericcurtin
changed the title
ericcurtin
force-pushed
the
master
branch
4 times, most recently
from
71f2c85 to
471709d
Compare
|
Some updates made, now we are hitting this issue: $ sudo bootc switch --transport docker-daemon localhost/bootc:latest |
ericcurtin
force-pushed
the
master
branch
3 times, most recently
from
0c4b515 to
87247f0
Compare
|
It works now: |
|
@cgwalters @jmarrero PTAL |
So we can do things like: sudo bootc switch --transport docker-daemon localhost/bootc:latest Signed-off-by: Eric Curtin <eric.curtin@docker.com>
jmarrero
left a comment
jmarrero
left a comment
There was a problem hiding this comment.
this looks good and straight forward to me.
| /// Even though this indicates gzip compression, docker-daemon actually stores | ||
| /// the layers uncompressed, so we need to treat this as uncompressed. |
There was a problem hiding this comment.
This seems surprising to me on two levels.
First, in theory the current skopeo proxy should be translating Docker media types to OCI unconditionally.
(Though honestly, I want to get away from supporting docker v2s2 at all, we should emit a deprecation warning or somethign)
Second, yeah the compression thing. Although that may just be really fallout from us not translating from v2s2 to OCI correctly.
Can we instead special case this workaround in the layer_info processing we do only if the input transport is Transport::DockerDaemon?
Also worth at least trying to spawn an agent to cross check all of this.
There was a problem hiding this comment.
I added another commit. Do you prefer the approach in the new commit?
There was a problem hiding this comment.
Yes though I suspect there's still an underlying bug in the skopeo or containers-image-proxy logic here and note that because composefs goes through a different path it'd likely need a similar workaround right now.
To handle this is a different location Signed-off-by: Eric Curtin <eric.curtin@docker.com>
|
Also while I'm OK letting this slide really in general changes like this should come with a test. I know doing that would be slightly involved at the moment though. |
3 participants
So we can do things like:
sudo bootc switch --transport docker-daemon localhost/bootc:latest