-
Notifications
You must be signed in to change notification settings - Fork 39.4k
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
Kubelet: Use RepoDigest for ImageID when available #33014
Kubelet: Use RepoDigest for ImageID when available #33014
Conversation
d374914
to
ffee60b
Compare
This is an implementation of the short-term solution I discussed in #32159. cc @kubernetes/sig-node @derekwaynecarr @ncdc |
cc @dchen1107 |
/cc @deads2k |
@@ -153,6 +154,14 @@ func filterHTTPError(err error, image string) error { | |||
|
|||
// Check if the inspected image matches what we are looking for | |||
func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool { | |||
// NB: the code below this check checks for validity of `image` as a pull spec. | |||
// However, it can be useful to also inspect images by ID (config digest, e.g. | |||
// when determining the manifest digestof the image in use by a container). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/digestof/digest of/
@@ -202,6 +211,8 @@ func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool { | |||
} | |||
if digest.Digest().Algorithm().String() == id.Algorithm().String() && digest.Digest().Hex() == id.Hex() { | |||
return true | |||
} else { | |||
glog.Infof("DIGEST: %v != %v", digest.Digest().Hex(), id.Hex()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This probably needs to be V(4), and it could be a bit more verbose :-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, I think that suck in from debugging.
@@ -153,6 +154,14 @@ func filterHTTPError(err error, image string) error { | |||
|
|||
// Check if the inspected image matches what we are looking for | |||
func matchImageTagOrSHA(inspected dockertypes.ImageInspect, image string) bool { | |||
// NB: the code below this check checks for validity of `image` as a pull spec. | |||
// However, it can be useful to also inspect images by ID (config digest, e.g. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'd ever expect this to happen. You would have to pull the image by tag or digest to a node, then run a pod whose container's image is the value for config.digest
, and that pod would have to be scheduled to the same node. Right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I'm understanding you here, but when you do container-inspect, you get a field called ImageID
, which we currently use to populate the ImageID
. In order to turn that into a manifest digest, you have to do an image-inspect on that config digest to get the information for the image (which includes the RepoDigests
field).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The purpose of this function is to determine if the image
parameter matches the inspected
image, either by tag or digest. Because there are 3 ways a user can reference an image (tag, digest, id aka config.digest), we have to make sure that the inspected image does not match id/config.digest.
Because inspected.ID
is the value of config.digest
, the only way for it to equal image
is if the user specified the config.digest
for containers[i].image
in the pod spec. But you can't pull by config.digest
, which means that the only way you can successfully run a container whose image is config.digest
is if that image already exists on the node. So you would have to:
- Run a pod whose image is e.g.
foo/bar:latest
- Find out what that image's
config.digest
is - Run another pod, specifying the
config.digest
value for the image, and make sure the pod is scheduled to the same node from step 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, the only way to get into the docker InspectImage
API is through the InspectImage
method on DockerInterface
, which calls this. So the alternative is to add an InspectImageUnchecked
to DockerInterface
which doesn't verify the inspected info after pulling (or add a check
argument to the InspectImage
method).
I see what you mean here, though. The intent of the method was not entirely clear based on the godoc above :-/.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, switched over to adding a checked
argument to InspectImage
which optionally skips the check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like the checked
arg added to the function. If the verification is not always needed, why not just move it up to somewhere else? E.g., IsImagePresent
might be a good candidate.
@@ -45,6 +45,7 @@ import ( | |||
const ( | |||
PodInfraContainerName = leaky.PodInfraContainerName | |||
DockerPrefix = "docker://" | |||
DockerPullablePrefix = "docker-pullable://" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason we need a different prefix?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It lets us tell which value (config digest vs manifest digest/spec) we're getting at a glance. Otherwise, there's not an easy way to tell, except by trying to parse and/or inspect the value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed upon introducing a new field called CanonicalImageID to distinguish manifest digest from config digest, and leaving the existing field as is. Why we change that direction?
// default to the image ID, but try and inspect for the RepoDigests | ||
imageID := DockerPrefix + iResult.Image | ||
imgInspectResult, err := dm.client.InspectImage(iResult.Image) | ||
if err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think this could be cleaner as a switch block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... like
switch {
case err != nil:
utilruntime.HandleError(fmt.Errorf("unable to inspect docker image %q while inspecting docker container %q: %v", containerName, iResult.Image, err))
case len(imgInspectResult.RepoDigests) > 1:
glog.V(4).Infof("Container %q had more than one associated RepoDigest (%v), only using the first", containerName, imgInspectResult.RepoDigests)
fallthrough
case len(imgInspectResult.RepoDigests) > 0:
imageID = xyz
Maybe? I could also just move the logging if statement out a bit (it doesn't need to be nested like it is). I'm not convinced that a switch is better than that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, something like that. I was trying to get rid of the nesting if possible.
FYI @smarterclayton |
d7a291f
to
801f385
Compare
801f385
to
5d3a966
Compare
GCE e2e build/test passed for commit 5d3a9669e80d0dbfe33dd7b7c8ed6730ded23426. |
Thanks for reusing the Although last time I heard from @deads2k I thought we weren't going to change the prefix ( |
@dchen1107 -- are you able to review this? @DirectXMan12 -- can you make the red x's go away? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am ok with the change in general, especially it only expose the image ID without changing any behavior for now.
@@ -45,6 +45,7 @@ import ( | |||
const ( | |||
PodInfraContainerName = leaky.PodInfraContainerName | |||
DockerPrefix = "docker://" | |||
DockerPullablePrefix = "docker-pullable://" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought we agreed upon introducing a new field called CanonicalImageID to distinguish manifest digest from config digest, and leaving the existing field as is. Why we change that direction?
@dchen1107 in the long run, we want to have two separate fields, but we were thinking that we could start here, since we've never explicitly guaranteed the meaning of the field (and you'll be able to distinguish between the old and new meanings using the prefix change). |
This PR effecitvely changes the meaning of the |
ImageID was only meant for debugging. Will this change be necessary if the |
@yujuhong This is my concern too. I understand the motivation why @DirectXMan12 want to start from here. But once we build the automated system and logic based on top of this, especially relying on parsing the certain string, it is difficult to change later. My concern might not valid in this case. @vishh I think there is a need to expose the right / canonical image id, at least at the node level. Given the current situation, before settling down the generic structure to represent different image types, I don't think we can simply introducing digest to PodSpec. What @DirectXMan12 proposed here is a simple and less intrusive way to expose the necessary information, and this change enables building an out-of-band image / package management system at the cluster level for them. What they learnt from that system eventually can benefit OSS kubernetes to define a generic structure for other image types, and build a generic package / image management system at cluster level. |
cc @euank @jonboulle |
@DirectXMan12 Can you fix the failing tests? |
Previously, we used the docker config digest (also called "image ID" by Docker) for the value of the `ImageID` field in the container status. This was not particularly useful, since the config manifest is not what's used to identify the image in a registry, which uses the manifest digest instead. Docker 1.12+ always populates the RepoDigests field with the manifest digests, and Docker 1.10 and 1.11 populate it when images are pulled by digest. This commit changes `ImageID` to point to the the manifest digest when available, using the prefix `docker-pullable://` (instead of `docker://`)
@derekwaynecarr My understanding was that we'd add a new one. Sorry for my misunderstanding and getting to this conversation a bit late; my bad! My concern here is more trying to "keep us honest" to our own standards than practical, and if we want to assert that we don't need to follow the rules for this sort of mostly-useless-field case I'm fine with being overruled. |
@k8s-bot kubemark e2e test this |
LGTM |
Jenkins Kubemark GCE e2e failed for commit 01b0b5e. Full PR test history. The magic incantation to run this job again is |
@k8s-bot kubemark e2e test this |
@dchen1107 -- looks like all the check marks are green... ptal |
@euank ImageID will always contain The reason the value in that field contains a prefix is so that we could add other types of image IDs, but we expected to change it upon request only, such as for other kinds of images, like OCI. @smarterclayton @euank @dchen1107 Why is the current field useless? Feel free to point me at an earlier comment. |
@bgrant0607 It's useless because the current image id it presents, This change is about making it so it can whenever possible display a value that is not node-local, but rather a uniquely identifying and reference-able in external repositories. The new value will not always be present iiuc, and the Since the original spirit of including the prefix was for users to parse and switch on it with the expectation of more, I withdraw my concern. My bad for confusion drawing this out, sorry! |
After this change, if an image digest is available, kubele will report |
@bgrant0607 -- it sounds like there are no more remaining concerns? Objections to merging? |
@derekwaynecarr I discussed the change with @yujuhong and it's ok with me from an API-compatibility point of view. She'll make a final pass over the PR. |
LGTM. |
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue |
This PR broke all kubemark suites. I'm reverting it. |
Automatic merge from submit-queue CRI: Image pullable support in dockershim For #33189. The new test `ImageID should be set to the manifest digest (from RepoDigests) when available` introduced in #33014 is failing, because: 1) `docker-pullable://` conversion is not supported in dockershim; 2) `kuberuntime` and `dockershim` is using `ListImages with image name filter` to check whether image presents. However, `ListImages` doesn't support filter with `digest`. This PR: 1) Change `kuberuntime.IsImagePresent` to use `runtime.ImageStatus` and `dockershim.InspectImage` instead. ***Notice an API change: `ImageStatus` should return `(nil, nil)` for non-existing image.*** 2) Add `docker-pullable://` support. 3) Fix `RemoveImage` in dockershim #29316. I've tried myself, the test can pass now. @yujuhong @feiskyer @yifan-gu /cc @kubernetes/sig-node
Release note:
Previously, we used the docker config digest (also called "image ID"
by Docker) for the value of the
ImageID
field in the container status.This was not particularly useful, since the config manifest is not
what's used to identify the image in a registry, which uses the manifest
digest instead. Docker 1.12+ always populates the RepoDigests field
with the manifest digests, and Docker 1.10 and 1.11 populate it when
images are pulled by digest.
This commit changes
ImageID
to point to the the manifest digest whenavailable, using the prefix
docker-pullable://
(instead ofdocker://
)Related to #32159
This change is