Add new snapshots command in ctr for getting an image's snapshot layer mounts by katiewasnothere · Pull Request #4797 · containerd/containerd

katiewasnothere · 2020-12-03T02:35:02Z

This PR adds a new command to ctr's snapshot commands for retrieving the snapshot mounts of an image from a given snapshotter on a given platform. Adding this utility to the snapshot commands makes it clear what the purpose is and what the caller's responsibilities are (iow cleaning up the snapshot).

This new command is helpful for functional/unit tests in shims because it allows users to retrieve the layer mounts that would be used for creating a new container. Users can then manipulate or use these layer mounts as needed to test specific scenarios.

As an example, in https://github.com/microsoft/hcsshim we have a functional tests package for unit testing here. However, in order for us to unit test our shim, we must manually start a container or VM with an image's layer mounts. We're currently relying on an older docker command as seen here to retrieve those. This PR would replace the out of date docker command.

Signed-off-by: Kathryn Baldauf [email protected]

k8s-ci-robot · 2020-12-03T02:35:10Z

Hi @katiewasnothere. Thanks for your PR.

I'm waiting for a containerd member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

katiewasnothere · 2020-12-03T02:35:17Z

@kevpar @ambarve @dcantah @anmaxvl fyi

theopenlab-ci · 2020-12-03T02:42:35Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 59s (non-voting)

theopenlab-ci · 2020-12-03T19:03:37Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 48s (non-voting)

ambarve · 2020-12-04T19:04:52Z

I think having two commands with such similar names but different purpose can be confusing and also doesn't follow the convention. For example, with ctr snapshotter mount and mounts are just aliases for the same command. I think the better approach is to add a new --list-only flag to the image mount command. What do you think?

cpuguy83 · 2020-12-04T20:33:56Z

Agree here, --list-only or --dry-run on the mount command. Or a combination like --show --dry-run... or always show the mounts and add --dry-run.

theopenlab-ci · 2020-12-04T23:21:14Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 58s (non-voting)

theopenlab-ci · 2020-12-08T20:19:00Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 36s (non-voting)

theopenlab-ci · 2020-12-09T23:30:47Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 49s (non-voting)

katiewasnothere · 2020-12-09T23:34:24Z

@ambarve @cpuguy83 I reworked this PR to instead make a new command in the list of snapshots commands. I've updated the title and the description. @ambarve and I were discussing the previous design and we decided that what the goal of my changes were just weren't very compatible with that route. Particularly around the "dry-run" and side effects. This new design makes it more clear what's going on and what work the caller needs to do to clean up or not. It also avoids any complications with the Mount.All command on different platforms (ex: getting the snapshots for a linux image on windows).

theopenlab-ci · 2020-12-09T23:38:10Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 50s (non-voting)

ambarve · 2020-12-09T23:46:26Z

I think it might be okay to make this a required parameter just like the prepare command. Unless there is a specific use case where we don't care about the key with which the snapshot is created.

Yeah that might be a good idea so that we can make sure people know what key to remove later

ambarve · 2020-12-09T23:48:33Z

I think we should provide the option of creating a read-only (View) snapshot too. We can add a --read-only flag for the command.

Signed-off-by: Kathryn Baldauf <[email protected]>

theopenlab-ci · 2020-12-11T01:23:05Z

Build succeeded.

containerd-build-arm64 : SUCCESS in 5m 56s (non-voting)

mxpv · 2020-12-11T19:26:28Z

+	ArgsUsage: "[flags] <key> <image name>",
+	Description: `Creates a new snapshot from a specified image.
+
+When you're done, use the remove command.


We already have prepareCommand and viewCommand for creating snapshots. Would it make sense to extend those instead? This one seems like does the same, but offers more CLI flags.

katiewasnothere · 2020-12-11T21:35:22Z

We already have prepareCommand and viewCommand for creating snapshots. Would it make sense to extend those instead? This one seems like does the same, but offers more CLI flags.

I thought about that too, but I'm not sure how much we're okay with changing the expectations for those commands. The goal of this new command is really to take an image name, like "docker.io/library/alpine:latest", and get a new snapshot created with the mounts printed to stdout. Prepare and View both start out with image key and optionally the chain ID of the parent. I think there could be some refactoring done, but I'm not sure if it would make sense to try to merge them together. Thoughts?

mxpv · 2020-12-11T21:59:56Z

My thinking is that the proposed function is basically a superset of pull + prepare/view.

Pull already fetches/unpacks the image with respect to platform, the only missing piece is --print-chainid
That chain id can be then passed to prepare/view as a parent, mounts are already printed as well.

katiewasnothere · 2020-12-14T23:44:27Z

@mxpv that sounds reasonable to me. In that case, I'll close this PR and open a new one with the suggested change instead.

k8s-ci-robot added the needs-ok-to-test label Dec 3, 2020

katiewasnothere force-pushed the image_layer_mounts branch from df14d6d to 5dcc015 Compare December 3, 2020 18:56

anmaxvl reviewed Dec 4, 2020

View reviewed changes

Comment thread cmd/ctr/commands/images/mounts.go Outdated

dcantah reviewed Dec 4, 2020

View reviewed changes

Comment thread cmd/ctr/commands/images/mounts.go Outdated

katiewasnothere force-pushed the image_layer_mounts branch from 5dcc015 to 33a0aea Compare December 4, 2020 23:13

ambarve reviewed Dec 5, 2020

View reviewed changes

Comment thread cmd/ctr/commands/images/mount.go Outdated

katiewasnothere force-pushed the image_layer_mounts branch from 33a0aea to d4a1e80 Compare December 8, 2020 20:11

katiewasnothere force-pushed the image_layer_mounts branch from d4a1e80 to 7703068 Compare December 9, 2020 23:23

katiewasnothere changed the title ~~Add new images command in ctr for getting an image's layer mounts~~ Add new snapshots command in ctr for getting an image's snapshot layer mounts Dec 9, 2020

katiewasnothere force-pushed the image_layer_mounts branch from 7703068 to 8dba036 Compare December 9, 2020 23:30

ambarve reviewed Dec 9, 2020

View reviewed changes

Add new images command in ctr for getting an image's layer mounts

42eb81a

Signed-off-by: Kathryn Baldauf <[email protected]>

katiewasnothere force-pushed the image_layer_mounts branch from 8dba036 to 42eb81a Compare December 11, 2020 01:15

mxpv reviewed Dec 11, 2020

View reviewed changes

katiewasnothere closed this Dec 14, 2020

katiewasnothere deleted the image_layer_mounts branch December 14, 2020 23:45

katiewasnothere mentioned this pull request Dec 15, 2020

Add a new flag to ctr "images pull" to print the image's chainID #4848

Merged

Conversation

katiewasnothere commented Dec 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Dec 3, 2020

Uh oh!

katiewasnothere commented Dec 3, 2020

Uh oh!

theopenlab-ci Bot commented Dec 3, 2020

Uh oh!

theopenlab-ci Bot commented Dec 3, 2020

Uh oh!

Uh oh!

ambarve commented Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cpuguy83 commented Dec 4, 2020

Uh oh!

Uh oh!

theopenlab-ci Bot commented Dec 4, 2020

Uh oh!

Uh oh!

theopenlab-ci Bot commented Dec 8, 2020

Uh oh!

theopenlab-ci Bot commented Dec 9, 2020

Uh oh!

katiewasnothere commented Dec 9, 2020

Uh oh!

theopenlab-ci Bot commented Dec 9, 2020

Uh oh!

ambarve Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

katiewasnothere Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

katiewasnothere Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

ambarve Dec 9, 2020

Choose a reason for hiding this comment

Uh oh!

katiewasnothere Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

theopenlab-ci Bot commented Dec 11, 2020

Uh oh!

mxpv Dec 11, 2020

Choose a reason for hiding this comment

Uh oh!

katiewasnothere commented Dec 11, 2020

Uh oh!

mxpv commented Dec 11, 2020

Uh oh!

katiewasnothere commented Dec 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

katiewasnothere commented Dec 3, 2020 •

edited

Loading

ambarve commented Dec 4, 2020 •

edited

Loading