client: Add helper function which checks if an image is unpacked #1668

jessvalarezo · 2017-10-20T18:15:49Z

Check if an image has been unpacked. (see last column)

REF                             TYPE                                                      DIGEST                                                                  STATUS           SIZE                UNPACKED
docker.io/library/mysql:latest  application/vnd.docker.distribution.manifest.list.v2+json sha256:eb9a3bca059ee178b5a69a2443462d156ff5b3d3f739c516b62d9d902ba49132 complete (12/12) 137.5 MiB/137.5 MiB true
docker.io/library/redis:latest  application/vnd.docker.distribution.manifest.list.v2+json sha256:07e7b6cb753f8d06a894e22af30f94e04844461ab6cb002c688841873e5e5116 complete (7/7)   37.6 MiB/37.6 MiB   false

# ctr rootfs unpack sha256:07e7b6cb753f8d06a894e22af30f94e04844461ab6cb002c688841873e5e5116
unpacking sha256:07e7b6cb753f8d06a894e22af30f94e04844461ab6cb002c688841873e5e5116 (application/vnd.docker.distribution.manifest.list.v2+json)...done

# ctr images check
REF                             TYPE                                                      DIGEST                                                                  STATUS           SIZE                UNPACKED
docker.io/library/mysql:latest  application/vnd.docker.distribution.manifest.list.v2+json sha256:eb9a3bca059ee178b5a69a2443462d156ff5b3d3f739c516b62d9d902ba49132 complete (12/12) 137.5 MiB/137.5 MiB true
docker.io/library/redis:latest  application/vnd.docker.distribution.manifest.list.v2+json sha256:07e7b6cb753f8d06a894e22af30f94e04844461ab6cb002c688841873e5e5116 complete (7/7)   37.6 MiB/37.6 MiB   true

Closes #1572.

Signed-off-by: Jess Valarezo valarezo.jessica@gmail.com

crosbymichael · 2017-10-20T18:23:11Z

You wanna add an integration tests for this client function? I don't think it would be too hard to test with our setup.

codecov-io · 2017-10-20T18:24:10Z

Codecov Report

Merging #1668 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #1668   +/-   ##
=======================================
  Coverage   49.03%   49.03%           
=======================================
  Files          27       27           
  Lines        4087     4087           
=======================================
  Hits         2004     2004           
  Misses       1664     1664           
  Partials      419      419

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 e2f9fbf...1552ee2. Read the comment docs.

mlaventure · 2017-10-20T18:25:00Z

image.go

@@ -63,6 +66,26 @@ func (i *image) Config(ctx context.Context) (ocispec.Descriptor, error) {
 	return i.i.Config(ctx, provider, platforms.Default())
 }

+func (i *image) IsUnpacked(ctx context.Context, snapshotterName string) (bool, error) {
+	sn := i.client.SnapshotService(snapshotterName)


should we move this logic to images.Check?

If so, may have to create a return type for it

/cc @stevvooe

Check is a really busy method already with all those return results.

could have it return a type, like I originally suggested. But it kinda feel that Check should give me that too.

I can update the Check return results to be a type - would we want that in this same PR?

Can be a separate PR

This needs to be added as a separate helper function for the image type in the images package. Once we review that, we can add the sugar in the containerd client.

I'm still not thrilled with this never ending interface model -- it doesn't scale as functionality changes. See the go-grpc package for a reference as to where this leads. We need to writing helpers that are a function of the type, rather than adding methods to the type itself, unless it operates on a core or hidden abstraction.

This needs to be added as a separate helper function for the image type in the images package. Once we review that, we can add the sugar in the containerd client.

The current unpack logic is not put into the images package and I think it is better that way so that we don't introduce the snapshot package interfaces to the images package. This would also introduce the naming patterns used by the clients for referencing snapshots, which is more opinionated than what should be included in the images package. If we want to keep the implementations for these helpers out of the root, we could probably move Unpack and this new function to rootfs.

I'm still not thrilled with this never ending interface model

I agree the Image interface does not seem to going in the right direction. Is there a need for this interface as all, why don't we just export image and use *Image from the client as well. Then we could be more flexible in adding methods or helpers for that type.

Just chatted with @dmcgowan about this. Proposed solution to have the IsUnpacked function take in a SnapshotterService and containerd.Image - that way it is no longer a member function or part of the interface. Thoughts?

stevvooe · 2017-10-23T07:04:15Z

The implementation looks good but need to add the helper function in the images package first.

mlaventure · 2017-10-24T23:47:39Z

No objections. Do we still want an interface for images at all though?

On Wed, Oct 25, 2017, 10:15 AM Jessica Valarezo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In image.go <#1668 (comment)> : > @@ -63,6 +66,26 @@ func (i *image) Config(ctx context.Context) (ocispec.Descriptor, error) { return i.i.Config(ctx, provider, platforms.Default()) } +func (i *image) IsUnpacked(ctx context.Context, snapshotterName string) (bool, error) { + sn := i.client.SnapshotService(snapshotterName) Just chatted with @dmcgowan <https://github.com/dmcgowan> about this. Proposed solution to have the Unpack function take in a SnapshotterService and containerd.Image - that way it is no longer a member function. Thoughts? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#1668 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/APUifur81Pi-0mMLGHJFpTq0OxnNuIAFks5svm-XgaJpZM4QBCdZ> .

-- Sent from my phone.

stevvooe · 2017-10-30T21:24:53Z

@jessvalarezo Let's go ahead and get this rebased, as is. We'll merge and make tweaks on the backend as needed. The current interface is fine and meets the current requirements.

Signed-off-by: Jess Valarezo <valarezo.jessica@gmail.com>

estesp

LGTM

mlaventure reviewed Oct 20, 2017

View reviewed changes

jessvalarezo force-pushed the image-unpack-check branch 2 times, most recently from 94aec33 to f40287c Compare October 20, 2017 23:32

Random-Liu mentioned this pull request Oct 26, 2017

Use image.IsUnpacked in containerd. containerd/cri#361

Closed

client: Add helper function which checks if an image is unpacked

1552ee2

Signed-off-by: Jess Valarezo <valarezo.jessica@gmail.com>

jessvalarezo force-pushed the image-unpack-check branch from f40287c to 1552ee2 Compare October 30, 2017 23:34

AkihiroSuda approved these changes Oct 31, 2017

View reviewed changes

estesp approved these changes Oct 31, 2017

View reviewed changes

estesp merged commit ff90484 into containerd:master Oct 31, 2017

jessvalarezo deleted the image-unpack-check branch November 7, 2017 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

client: Add helper function which checks if an image is unpacked #1668

client: Add helper function which checks if an image is unpacked #1668

jessvalarezo commented Oct 20, 2017 •

edited

crosbymichael commented Oct 20, 2017

codecov-io commented Oct 20, 2017 •

edited

mlaventure Oct 20, 2017

mlaventure Oct 20, 2017

crosbymichael Oct 20, 2017

mlaventure Oct 20, 2017

jessvalarezo Oct 20, 2017

mlaventure Oct 21, 2017

stevvooe Oct 23, 2017

dmcgowan Oct 23, 2017

jessvalarezo Oct 24, 2017 •

edited

stevvooe commented Oct 23, 2017

mlaventure commented Oct 24, 2017 via email

stevvooe commented Oct 30, 2017

estesp left a comment

client: Add helper function which checks if an image is unpacked #1668

client: Add helper function which checks if an image is unpacked #1668

Conversation

jessvalarezo commented Oct 20, 2017 • edited

crosbymichael commented Oct 20, 2017

codecov-io commented Oct 20, 2017 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jessvalarezo Oct 24, 2017 • edited

Choose a reason for hiding this comment

stevvooe commented Oct 23, 2017

mlaventure commented Oct 24, 2017 via email

stevvooe commented Oct 30, 2017

estesp left a comment

Choose a reason for hiding this comment

jessvalarezo commented Oct 20, 2017 •

edited

codecov-io commented Oct 20, 2017 •

edited

jessvalarezo Oct 24, 2017 •

edited