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.

Sign up for GitHub

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

Jump to bottom

CRI: Rename 'RepoTags' to 'Names', and 'RepoDigests' to 'Digests'. #33971

Closed
wants to merge 1 commit into from
Closed

CRI: Rename 'RepoTags' to 'Names', and 'RepoDigests' to 'Digests'. #33971

wants to merge 1 commit into from

Conversation

yifan-gu
Copy link
Contributor

@yifan-gu yifan-gu commented Oct 3, 2016 •
edited by k8s-oncall
Loading

This makes the field name for generic.

/cc @kubernetes/sig-node @kubernetes/sig-rktnetes

Will have a merge conflict with #33970

This change is Reviewable

@yifan-gu yifan-gu added area/kubelet-api sig/node Categorizes an issue or PR as relevant to SIG Node. release-note-none Denotes a PR that doesn't merit a release note. labels Oct 3, 2016
@yifan-gu yifan-gu added this to the v1.5 milestone Oct 3, 2016
@k8s-ci-robot
Copy link
Contributor

Jenkins GCE Node e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot node e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins unit/integration failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GKE smoke e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCE e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins Kubemark GCE e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot kubemark e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GCE e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gce e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

@k8s-ci-robot
Copy link
Contributor

Jenkins GCI GKE smoke e2e failed for commit e5bf458. Full PR test history.

The magic incantation to run this job again is @k8s-bot gci gke e2e test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

yujuhong
yujuhong reviewed Oct 3, 2016
View reviewed changes
pkg/kubelet/api/v1alpha1/runtime/api.proto
@@ -628,9 +628,9 @@ message Image {
// ID of the image.
optional string id = 1;
// Other names by which this image is known.
repeated string repo_tags = 2;
repeated string names = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Does name include digests?

Copy link
Contributor Author

@yifan-gu yifan-gu Oct 3, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Are you asking what's the value of these two when someone specifies the image name as digest in the container spec when creating the container?

IIUC, for the return value of docker inspect, the names are in the format of ubuntu:latest, digests are ubuntu@sha256:45b23dee08af5e43a7fea6c4cf9c25ccf269ee113168c19722f87876677c5cb2

Both digests and names can be used to reference and pull the image. Digests also act as a content hash of the image.

Copy link
Contributor

Choose a reason for hiding this comment

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

I am asking because the definition could be ambiguous in some cases, and it'd be better to clarify this in the comment. RepoTags clearly would not contain the digest, but I am not sure if it's still so straightforward withNames. For example, the image Names included in the node status may include both tags and digests.

Copy link
Member

Choose a reason for hiding this comment

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

still think repo_tags is more clear, it clearly implies the format should be image_name:tag.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@feiskyer We can comment and require the Name to have format like image_name:tag as well. RepoTags just sounds very docker specific. Same as RepoDigests.

@yujuhong I don't see why the Names in node status should contain digest at first place? Shouldn't we add a field about the digest / hash of the image?

Copy link
Member

Choose a reason for hiding this comment

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

@feiskyer We can comment and require the Name to have format like image_name:tag as well. RepoTags just sounds very docker specific. Same as RepoDigests.

OK. It sounds docker specific just because only docker image is supported today. Does ACI also supports image:tag and image@digest?

pkg/kubelet/api/v1alpha1/runtime/api.proto
// Digests by which this image is known.
repeated string repo_digests = 3;
repeated string digests = 3;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should also amend the description of ImageSpec to be either names or digests

Copy link
Member

Choose a reason for hiding this comment

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

And same with repo_digests, since image name is also expected in this field.

@k8s-github-robot k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 3, 2016
@yifan-gu
Copy link
Contributor Author

yifan-gu commented Oct 4, 2016 •
edited
Loading

It seems we have a different view on what's the name of the image.
IMO, I think we should define the name as a human readable (also pullable) string, in the format like repo:tag, which corresponds to the one in the k8s API.

I don't see why we should think the repoDigest is an image name, especially it's intended to be read by human. It's unfortunate we haven't a separate field in k8s API for that, so we reuse the image name field in order to pull the image.

@yujuhong
Copy link
Contributor

yujuhong commented Oct 4, 2016

@yifan-gu I am ok with the name and digest change, but just wanted some clarification written down in the api as to what each term refers to. I think the PR as it is may create more confusion than necessary.

@k8s-github-robot
Copy link

@yifan-gu PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 5, 2016
@yifan-gu
Copy link
Contributor Author

Closing this, now I agree repoTags and repoDigests are clean for now since docker images are the only supported format today.

@yifan-gu yifan-gu closed this Oct 27, 2016
@yifan-gu yifan-gu deleted the image_names branch October 27, 2016 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feiskyer feiskyer feiskyer left review comments

@yujuhong yujuhong yujuhong left review comments

Assignees

@feiskyer feiskyer

@euank euank

@yujuhong yujuhong

Labels
area/kubelet-api needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v1.5
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@yifan-gu @k8s-ci-robot @yujuhong @k8s-github-robot @feiskyer @euank @googlebot