Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add label flags to ctr import #9476

Merged
merged 1 commit into from Jan 8, 2024

Conversation

roman-kiselenko
Copy link
Contributor

@roman-kiselenko roman-kiselenko commented Dec 6, 2023

This PR introduces an option for assigning labels at the import step.

The current implementation provides a way to label images at the pull(fetch) step, and labeling images at the import step reduces additional steps.

related

Fixes #9505

@k8s-ci-robot
Copy link

Hi @roman-kiselenko. 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.

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.

@roman-kiselenko
Copy link
Contributor Author

roman-kiselenko commented Dec 6, 2023

I've doubts about my implementation, and it's better to move that logic in the image.Store method 🤔

Also, I guess some integration tests are required.

@roman-kiselenko
Copy link
Contributor Author

/kind feature

@roman-kiselenko
Copy link
Contributor Author

/cc @ruiwen-zhao @samuelkarp

Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

@roman-kiselenko Thanks for adding this, although adding the labels after import is doable using ctr images label still its a good addition to have consistency with the pull command. Going to try it out locally

client/import.go Outdated Show resolved Hide resolved
@roman-kiselenko
Copy link
Contributor Author

@adisky, @fuweid, @samuelkarp, thanks for your review. 🙏
To address your review
The current implementation of image importing doesn't inherit base image labels.

FROM busybox

RUN echo -e '#!/bin/sh\nwhile :; do date; sleep 1; done' > /entrypoint && \
    chmod 750 /entrypoint

RUN ["echo", "bar"]
ENTRYPOINT [ "/entrypoint" ]
# build image
docker build -t image-with-label --label some=label .
docker save -o with-label.tar docker.io/library/image-with-label
# in air-gapped environment
cat with-label.tar | ./bin/ctr images import -
unpacking docker.io/library/image-with-label:latest (sha256:ceccd3c9687d1e5d72432405278443a6d1c505dc0eb899b20414aec8d7759cf4)...done
./bin/ctr images ls
REF                                       TYPE                                                 DIGEST                                                                  SIZE    PLATFORMS   LABELS 
docker.io/library/image-with-label:latest application/vnd.docker.distribution.manifest.v2+json sha256:ceccd3c9687d1e5d72432405278443a6d1c505dc0eb899b20414aec8d7759cf4 4.1 MiB linux/arm64 -      

With --label options:

cat with-label.tar | ./bin/ctr images import --label=bar=bar -
./bin/ctr images ls
REF                                       TYPE                                                 DIGEST                                                                  SIZE    PLATFORMS   LABELS  
docker.io/library/image-with-label:latest application/vnd.docker.distribution.manifest.v2+json sha256:ceccd3c9687d1e5d72432405278443a6d1c505dc0eb899b20414aec8d7759cf4 4.1 MiB linux/arm64 bar=bar

Proposed changes complement the implementation of image pinning in air-gapped environments

client/import.go Outdated Show resolved Hide resolved
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

But please squash three commits into one. Thanks!

@roman-kiselenko roman-kiselenko force-pushed the label-images-on-import branch 2 times, most recently from e33ffd1 to 1ada509 Compare December 21, 2023 06:31
Copy link
Contributor

@adisky adisky left a comment

Choose a reason for hiding this comment

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

Thanks @roman-kiselenko LGTM

@ruiwen-zhao
Copy link
Member

Thank you @roman-kiselenko !

@fuweid
Copy link
Member

fuweid commented Jan 4, 2024

Hi @roman-kiselenko would you please rebase this one? We have two LGTMs. I can merge it when it's ready. Thanks

@mxpv mxpv added the status/needs-update Awaiting contributor update label Jan 4, 2024
Signed-off-by: roman-kiselenko <roman.kiselenko.dev@gmail.com>
@roman-kiselenko
Copy link
Contributor Author

Hi @roman-kiselenko would you please rebase this one? We have two LGTMs. I can merge it when it's ready. Thanks

@fuweid Thank you! Rebased 👍

Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

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

LGTM

@fuweid fuweid removed the status/needs-update Awaiting contributor update label Jan 8, 2024
@fuweid fuweid added this pull request to the merge queue Jan 8, 2024
Merged via the queue into containerd:main with commit 781d027 Jan 8, 2024
43 checks passed
@roman-kiselenko roman-kiselenko deleted the label-images-on-import branch January 8, 2024 11:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Enhance ctr images import CLI with '--label' Option for Improved Image Metadata Assignment
6 participants