Skip to content

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

Merged
dmcgowan merged 1 commit intocontainerd:masterfrom
katiewasnothere:ctr_image_chainid
Jan 22, 2021
Merged

Add a new flag to ctr "images pull" to print the image's chainID#4848
dmcgowan merged 1 commit intocontainerd:masterfrom
katiewasnothere:ctr_image_chainid

Conversation

@katiewasnothere
Copy link
Copy Markdown

@katiewasnothere katiewasnothere commented Dec 15, 2020

This PR is the follow up to discussion in #4797.

This PR adds the option to print the pulled image's chain id in a ctr "images pull" call. This can be used to feed into a call to "snapshots prepare/view" for creating new snapshots for the user.

From the previous PR:

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
Copy link
Copy Markdown

k8s-ci-robot commented Dec 15, 2020

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.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 15, 2020

Build succeeded.

mxpv
mxpv reviewed Dec 15, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@mxpv mxpv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much shorter :)
One minor nit, otherwise LGTM

Comment thread cmd/ctr/commands/images/pull.go Outdated
@estesp
Copy link
Copy Markdown
Member

estesp commented Dec 15, 2020

Is the change to the snapshot prepare sub-command (removing target and adding mounts) expected to be a part of this PR? It's not mentioned in the commit message or PR text, so wondering if that was a mistake? If nothing else it might be better as it's own PR clearly describing the reason/intention.

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Dec 15, 2020

Is the change to the snapshot prepare sub-command (removing target and adding mounts) expected to be a part of this PR? It's not mentioned in the commit message or PR text, so wondering if that was a mistake? If nothing else it might be better as it's own PR clearly describing the reason/intention.

@estesp That was intentional since moving to print the mounts as json (so as not to use the linux specific helper function for printing mounts) would effectively make the target option do nothing. Does that seem okay?

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 15, 2020

Build succeeded.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Dec 16, 2020

Build succeeded.

@AkihiroSuda
Copy link
Copy Markdown
Member

AkihiroSuda commented Dec 28, 2020 •
edited
Loading

Is the change to the snapshot prepare sub-command (removing target and adding mounts) expected to be a part of this PR? It's not mentioned in the commit message or PR text, so wondering if that was a mistake? If nothing else it might be better as it's own PR clearly describing the reason/intention.

@estesp That was intentional since moving to print the mounts as json (so as not to use the linux specific helper function for printing mounts) would effectively make the target option do nothing. Does that seem okay?

I think this should be another PR or at least another commit

@dmcgowan
Copy link
Copy Markdown
Member

dmcgowan commented Dec 30, 2020

I agree with @AkihiroSuda, I agree about adding the new option, but not necessarily about removing the target option for Linux.

@dmcgowan dmcgowan added status/needs-update Awaiting contributor update and removed needs-ok-to-test labels Jan 12, 2021
@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Jan 12, 2021 •
edited
Loading

I agree with @AkihiroSuda, I agree about adding the new option, but not necessarily about removing the target option for Linux.

@dmcgowan That seems fair to me. I've updated to add back the target option.

@theopenlab-ci
Copy link
Copy Markdown

theopenlab-ci Bot commented Jan 12, 2021

Build succeeded.

@katiewasnothere
Copy link
Copy Markdown
Author

katiewasnothere commented Jan 21, 2021

@AkihiroSuda or @dmcgowan could you PTAL when you get the chance?

estesp
estesp approved these changes Jan 21, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@estesp estesp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@estesp estesp removed the status/needs-update Awaiting contributor update label Jan 21, 2021
dmcgowan
dmcgowan approved these changes Jan 22, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@dmcgowan dmcgowan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dmcgowan dmcgowan merged commit 392b285 into containerd:master Jan 22, 2021
@katiewasnothere katiewasnothere mentioned this pull request Feb 1, 2021
Closed
@katiewasnothere katiewasnothere mentioned this pull request Apr 14, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@dmcgowan dmcgowan dmcgowan approved these changes

@mxpv mxpv mxpv approved these changes

@estesp estesp estesp approved these changes

@dcantah dcantah dcantah approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@katiewasnothere @k8s-ci-robot @estesp @AkihiroSuda @dmcgowan @mxpv @dcantah