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

Projects
None yet
9 participants
@shlevy
Contributor

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

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 10, 2018

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.

@cblecker

This comment has been minimized.

Member

cblecker commented Jan 10, 2018

/ok-to-test

@cblecker

This comment has been minimized.

Member

cblecker commented Jan 10, 2018

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

@shlevy

This comment has been minimized.

Contributor

shlevy commented Jan 10, 2018

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

@shlevy

This comment has been minimized.

Contributor

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

This comment has been minimized.

Contributor

mtaufen commented Jan 10, 2018

/unassign
/assign @yujuhong

@shlevy

This comment has been minimized.

Contributor

shlevy commented Feb 7, 2018

OK, CLA finally came through. Rebasing now.

@shlevy

This comment has been minimized.

Contributor

shlevy commented Feb 8, 2018

/retest

@k8s-ci-robot k8s-ci-robot added size/XS and removed size/M labels Feb 8, 2018

@shlevy

This comment has been minimized.

Contributor

shlevy commented Feb 8, 2018

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

@cblecker

This comment has been minimized.

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.

dockershim: Return Labels as Info in ImageStatus.
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

This comment has been minimized.

Contributor

shlevy commented Feb 23, 2018

OK, I think CLA is finally all covered 😅

@shlevy

This comment has been minimized.

Contributor

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

This comment has been minimized.

Member

cblecker commented Feb 23, 2018

/test pull-kubernetes-unit, flake #59426

@cblecker

This comment has been minimized.

Member

cblecker commented Feb 23, 2018

/retest

@yujuhong

This comment has been minimized.

Contributor

yujuhong commented Feb 28, 2018

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 label Feb 28, 2018

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Feb 28, 2018

[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

@shlevy

This comment has been minimized.

Contributor

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

This comment has been minimized.

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. :)

@jdumars jdumars added this to the v1.10 milestone Feb 28, 2018

@k8s-merge-robot

This comment has been minimized.

Contributor

k8s-merge-robot commented Feb 28, 2018

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-merge-robot k8s-merge-robot merged commit b63fab3 into kubernetes:master Feb 28, 2018

13 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation shlevy authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-unit Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
@ironhouzi

This comment has been minimized.

ironhouzi commented Mar 14, 2018

@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

This comment has been minimized.

Contributor

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