feature: add pouch load functionality #1391
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1391 +/- ##
==========================================
+ Coverage 16.24% 37.3% +21.06%
==========================================
Files 200 254 +54
Lines 13572 17340 +3768
==========================================
+ Hits 2205 6469 +4264
+ Misses 11212 10042 -1170
- Partials 155 829 +674
|
apis/swagger.yml
Outdated
| schema: | ||
| $ref: "#/responses/500ErrorResponse" | ||
| parameters: | ||
| - name: "name" |
There was a problem hiding this comment.
should we add input as a body parameter? the image load api consumes application/x-tar
| return bytes.NewReader(b), nil | ||
| } | ||
|
|
||
| return nil, nil |
There was a problem hiding this comment.
If the obj is nil, shall we return an error？
If we return nil, nil, the is.Reader is nil, then we need to judge if it is nil in the caller. WDYT?
There was a problem hiding this comment.
If the obj is nil, it means that we don't need to send the data to the server. HTTP Client will handle the nil case for us. It's ok I think. How do you think?
There was a problem hiding this comment.
OK, I am OK on this, just a remind to ensure this.
daemon/mgr/image_load.go
Outdated
| // may fails to restart. | ||
| for _, img := range imgs { | ||
| if err := mgr.storeImageReference(ctx, img); err != nil { | ||
| return err |
There was a problem hiding this comment.
Shall we try to store every image?
And aggregate the errors of each store error, and then return the aggregated error?
|
Hi @allencloud and @shaloulcy , I have updated the code and please take a look. Thanks |
|
LGTM, and I would like to invite @shaloulcy to take a look as well. In addition, the missing integration test is what we need to finish ASAP. @fuweid |
pouchrobot
added
the
LGTM
|
@allencloud yes. will do |
daemon/mgr/image_load.go
Outdated
| // NOTE: in the image ocispec.v1, the org.opencontainers.image.ref.name | ||
| // annotation represents a "tag" for image. For example, an image may | ||
| // have a tag for different versions or builds of the software. | ||
| // And containerd.importer will append ":" and annotation so that we |
There was a problem hiding this comment.
":" and annotation
%s/and/to
daemon/mgr/image_utils.go
Outdated
| @@ -16,6 +16,38 @@ import ( | |||
| ocispec "github.com/opencontainers/image-spec/specs-go/v1" | |||
| ) | |||
|
|
|||
| // Multierrors contains a slice of errors. | |||
There was a problem hiding this comment.
i think Multierrors not attached to image, so we should put this code to pkg dir, maybe pkg/error, WDYT ?😆
There was a problem hiding this comment.
We can move it out to pkg if necessary. The pkg/error only contains the Multierrors. It is weird... 😈 make senses?
There was a problem hiding this comment.
I think it's ok, some cli command also need the Multierrors, so it's proper to move it to pkg, WDTY?? 😆
There was a problem hiding this comment.
Could you mind to give me a example?
There was a problem hiding this comment.
I can move it to the pkg/utils in case that some one wants to use.
Signed-off-by: Wei Fu <fhfuwei@163.com>
|
@HusterWan I have moved the |
|
LGTM |
Signed-off-by: Wei Fu fhfuwei@163.com
Ⅰ. Describe what this PR did
Allow user to load oci.v1 format image by tar stream.
Ⅱ. Does this pull request fix one issue?
NONE
Ⅲ. Describe how you did it
Use containerd image oci importer to load the image.
Ⅳ. Describe how to verify it
First one is help:
Since we don't have dump functionality for pouch, we need to verify it by
ctr.NOTE: The oci.v1 spec says that the
org.opencontainers.image.ref.nameis used as tag in common.Based on this, we don't allow user to apply any name which contains digest or tag information.
Ⅴ. Special notes for reviews
The output of
docker savedoesn't meet the oci.v1 spec and moby has the enhanced issue for this.For now, this commit have not supported
docker image layout taryet.We need to implement
docker to ocirelated tool to handle the gap between docker and pouchd.