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

dockershim: Return Labels as Info in ImageStatus. #58036

Merged
merged 1 commit into from Feb 28, 2018

Conversation

shlevy
Copy link

@shlevy shlevy commented Jan 10, 2018

c6ddc74 added an Info field to
ImageStatusResponse when Verbose is true. This makes the image's
Labels available in that field, rather than unconditionally returning
an empty map.

What this PR does / why we need it:

This PR exposes an image's Labels through the CRI. In particular, I want this so I can write an ImageService wrapper that delegates all operations to a real ImageService but also, when the right Labels, ensures any needed nix store paths are present on the system when an image is pulled, enabling users to use nix for package distribution while still using containers for isolation and kubernetes for orchestration. In general, though, this should be useful for anything that wants to know about an image's Labels

Special notes for your reviewer:

I'd prefer to put this change into the Image protobuf type instead of putting it into Info (gated by Verbose or not, available in other requests like ListImages or not), but that would be a change to the protocol and it seems Info was introduced exactly for this purpose. If it's acceptable to put this into Image, I'll rework this.

If this change is acceptable, I will also do the work for cri-o, rktlet, frakti, and cri-containerd where applicable.

I have started the process for my employer to sign on to the CLA. I don't have reason to expect it to take long, but because there is more work to do if this change is desired I'd prefer if we can start review before that is completed.

Release note:

dockershim now makes an Image's Labels available in the Info field of ImageStatusResponse

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jan 10, 2018
@k8s-ci-robot
Copy link
Contributor

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.


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. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jan 10, 2018
@cblecker
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jan 10, 2018
@cblecker
Copy link
Member

@shlevy Please review and sign the CLA: #58036 (comment)

@shlevy
Copy link
Author

shlevy commented Jan 10, 2018

@cblecker I am waiting on my employer's guidelines for signing as an individual or as a corporation

@shlevy
Copy link
Author

shlevy commented Jan 10, 2018

OK, the process is started by my employer and the CLA should be signed within a week or so. In the mean time, though, if this work is desirable there's more work I'd need to do for the other CRI server implementations, so if possible a high level review (taking into account my "notes for reviewer") would be appreciated. I understand if everything needs to wait for the CLA though!

@mtaufen
Copy link
Contributor

mtaufen commented Jan 10, 2018

/unassign
/assign @yujuhong

@k8s-ci-robot k8s-ci-robot assigned yujuhong and unassigned mtaufen Jan 10, 2018
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 20, 2018
@shlevy
Copy link
Author

shlevy commented Feb 7, 2018

OK, CLA finally came through. Rebasing now.

@shlevy
Copy link
Author

shlevy commented Feb 8, 2018

/retest

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 8, 2018
@shlevy
Copy link
Author

shlevy commented Feb 8, 2018

@cblecker How do I get the CLA check to run again?

@cblecker
Copy link
Member

cblecker commented Feb 8, 2018

@shlevy The CLA check runs whenever the PR is updated. It's still reporting that you don't have a signed CLA. You may wish to check the top comment one more time to confirm you've followed all the needed steps.

@shlevy shlevy force-pushed the cri-ImageStatus-info branch 2 times, most recently from 132489d to 1609490 Compare February 23, 2018 12:45
c6ddc74 added an Info field to
ImageStatusResponse when Verbose is true. This makes the image's
Labels available in that field, rather than unconditionally returning
an empty map.
@shlevy
Copy link
Author

shlevy commented Feb 23, 2018

OK, I think CLA is finally all covered 😅

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Feb 23, 2018
@shlevy
Copy link
Author

shlevy commented Feb 23, 2018

Hmm, I don't really understand what's going on in that test failure, is that really related to this PR?

@cblecker
Copy link
Member

/test pull-kubernetes-unit, flake #59426

@cblecker
Copy link
Member

/retest

@yujuhong
Copy link
Contributor

I'm okay with the change since it's gated by "verbose".
Each runtime implements the verbose response differently, so they can determine whether this is useful for them.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 28, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shlevy, yujuhong

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 28, 2018
@shlevy
Copy link
Author

shlevy commented Feb 28, 2018

@yujuhong Any thoughts on this?

I'd prefer to put this change into the Image protobuf type instead of putting it into Info (gated by Verbose or not, available in other requests like ListImages or not), but that would be a change to the protocol and it seems Info was introduced exactly for this purpose. If it's acceptable to put this into Image, I'll rework this.

@Random-Liu
Copy link
Member

Random-Liu commented Feb 28, 2018

@yujuhong This seems fine to me. :)

@shlevy I think we can consider adding label as official CRI fields when we have more use cases. The current use case seems more like an implementation detail for a specific use case. :)

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 58171, 58036, 60540). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit b63fab3 into kubernetes:master Feb 28, 2018
@ironhouzi
Copy link

@Random-Liu This feature is sorely needed by my use case:

We use containers for blackboxing trusted algorithms or data processors basically. We store definitions of inputs, how they map to commandline arguments to the executable and what outputs the processor is expected to produce. It makes a lot of sense to have this metadata as image labels instead of files inside the image, since the metadata is only needed by the container scheduler and not by the container runtime. Having the metadata and image bundled together is very nice in terms of portability.

@shlevy
Copy link
Author

shlevy commented Mar 14, 2018

@Random-Liu What would be the appropriate process for proposing adding this as an official field of the Image protobuf type?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants