CRI: Image pullable support in dockershim #34380

Random-Liu · 2016-10-08T05:02:14Z

The new test ImageID should be set to the manifest digest (from RepoDigests) when available introduced in #33014 is failing, because:

docker-pullable:// conversion is not supported in dockershim;
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:

Change kuberuntime.IsImagePresent to use runtime.ImageStatus and dockershim.InspectImage instead. _Notice an API change: ImageStatus should return (nil, nil) for non-existing image._
Add docker-pullable:// support.
Fix RemoveImage in dockershim Removing images with multiple tags #29316.

I've tried myself, the test can pass now.

@yujuhong @feiskyer @yifan-gu
/cc @kubernetes/sig-node

This change is

k8s-ci-robot · 2016-10-08T05:20:28Z

Jenkins Kubemark GCE e2e failed for commit bb7e3e9. 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 · 2016-10-08T05:32:29Z

Jenkins GCI GCE e2e failed for commit bb7e3e9. 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.

Random-Liu · 2016-10-08T05:51:17Z

@k8s-bot cri test this.

feiskyer · 2016-10-09T06:30:07Z

@Random-Liu nit: #33014 has been reverted.

yifan-gu · 2016-10-11T01:45:25Z

pkg/kubelet/kuberuntime/kuberuntime_image.go

-			Image: &image.Image,
-		},
-	})
+	status, err := m.imageService.ImageStatus(&runtimeApi.ImageSpec{Image: &image.Image})


Can we return an error if the image is not found?

We could, we just need to define different types of error then.

@Random-Liu I suggest we define and return an error then.
BTW, it seems the dockershim will return nil, error when the image doesn't exist.

@Random-Liu Alternatively, can't dockershim support filtering the images through digest?

BTW, it seems the dockershim will return nil, error when the image doesn't exist.

@yifan-gu dockershim returns nil, nil https://github.com/kubernetes/kubernetes/pull/34380/files#diff-99802aef301353226cc4233749846c68R58

@Random-Liu Alternatively, can't dockershim support filtering the images through digest?

@yifan-gu Docker itself doesn't support filter image by digest when listing images.
We can implement it ourselves, but I feel like it is simpler and makes more sense to use ImageStatus.

@Random-Liu OK, but I still suggest return not found as an error, which is somehow a convention, e.g. os.Stat().

Random-Liu · 2016-10-11T22:02:00Z

@feiskyer @yujuhong Rebased the PR.

Random-Liu · 2016-10-11T22:02:08Z

@k8s-bot cri test this.

Random-Liu · 2016-10-11T23:01:44Z

@yujuhong The test passed! :) Do you want to take a look.

feiskyer · 2016-10-12T00:18:57Z

LGTM

yujuhong · 2016-10-12T15:48:07Z

pkg/kubelet/api/v1alpha1/runtime/api.proto

@@ -46,7 +46,8 @@ service RuntimeService {
 service ImageService {
    // ListImages lists existing images.
    rpc ListImages(ListImagesRequest) returns (ListImagesResponse) {}
-    // ImageStatus returns the status of the image.
+    // ImageStatus returns the status of the image. If the image doesn't


s/doesn't/is not

yujuhong · 2016-10-12T15:50:36Z

pkg/kubelet/dockershim/docker_image.go

@@ -51,16 +50,16 @@ func (ds *dockerService) ListImages(filter *runtimeApi.ImageFilter) ([]*runtimeA
 	return result, nil
 }

-// ImageStatus returns the status of the image.
+// ImageStatus returns the status of the image, returns nil if the image doesn't present.


s/doesn't/is not

yujuhong · 2016-10-12T15:51:31Z

pkg/kubelet/dockershim/docker_image.go

-	_, err := ds.client.RemoveImage(image.GetImage(), dockertypes.ImageRemoveOptions{PruneChildren: true})
+	// If the image has multiple tags, we need to remove all the tags
+	// TODO: We assume image.Image is image ID here, which is true in the current implementation
+	// of kubelet, but we should still clarify this kind of thing in CRI.


nit: remove kind of thing

yujuhong · 2016-10-12T15:53:16Z

pkg/kubelet/dockershim/docker_image.go

@@ -79,6 +78,19 @@ func (ds *dockerService) PullImage(image *runtimeApi.ImageSpec, auth *runtimeApi

 // RemoveImage removes the image.
 func (ds *dockerService) RemoveImage(image *runtimeApi.ImageSpec) error {
-	_, err := ds.client.RemoveImage(image.GetImage(), dockertypes.ImageRemoveOptions{PruneChildren: true})
+	// If the image has multiple tags, we need to remove all the tags
+	// TODO: We assume image.Image is image ID here, which is true in the current implementation


Is the assumption necessary?
If we want to make the assumption, maybe we should switch to using ImageRef.

Is the assumption necessary?

No, the comment is only for this PR. I'll send another PR to change the API a bit.

If we want to make the assumption, maybe we should switch to using ImageRef.

Yeah, I think so.

yujuhong · 2016-10-12T15:56:46Z

@Random-Liu we talked about using ImageRef for all but the PullImage operation, would that be done in a separate POR?

Random-Liu · 2016-10-12T16:09:40Z

@Random-Liu we talked about using ImageRef for all but the PullImage operation, would that be done in a separate POR?

I tried to change the code and found that we also use ImageSpec for ImageStatus. Before pulling an image, we use ImageStatus to check whether the image is present. At that time, we only have ImageSpec, because image is not pulled yet.

If so, it seems that only RemoveImage needs ImageRef.

* Fix inspect image bug. * Fix remove image bug.

Random-Liu · 2016-10-12T16:21:37Z

@yujuhong Addressed comments.

yujuhong · 2016-10-12T16:53:59Z

LGTM. Let's get this in first to stop the test failures. We can talk about changing the API later.

k8s-github-robot · 2016-10-12T17:54:32Z

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

k8s-ci-robot · 2016-10-12T19:25:18Z

Jenkins GCI GKE smoke e2e failed for commit afa3414. 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.

k8s-github-robot · 2016-10-12T19:25:37Z

Automatic merge from submit-queue

Random-Liu added area/docker area/kubelet area/kubelet-api sig/node Categorizes an issue or PR as relevant to SIG Node. labels Oct 8, 2016

Random-Liu added this to the 1.2 milestone Oct 8, 2016

Random-Liu assigned feiskyer and yujuhong Oct 8, 2016

googlebot added the cla: yes label Oct 8, 2016

Random-Liu added the release-note-none Denotes a PR that doesn't merit a release note. label Oct 8, 2016

k8s-github-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Oct 8, 2016

Random-Liu removed this from the 1.2 milestone Oct 8, 2016

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 8, 2016

yifan-gu reviewed Oct 11, 2016

View reviewed changes

Random-Liu force-pushed the fix-cri-image branch from bb7e3e9 to 3941cf3 Compare October 11, 2016 22:01

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 11, 2016

yujuhong mentioned this pull request Oct 12, 2016

CRI implemention in kubelet (with in-process docker integration) #29478

Closed

25 tasks

yujuhong reviewed Oct 12, 2016

View reviewed changes

* Add docker pullable support.

afa3414

* Fix inspect image bug. * Fix remove image bug.

Random-Liu force-pushed the fix-cri-image branch from 3941cf3 to afa3414 Compare October 12, 2016 16:21

yujuhong added lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. labels Oct 12, 2016

k8s-github-robot merged commit b99a909 into kubernetes:master Oct 12, 2016

Random-Liu deleted the fix-cri-image branch October 12, 2016 21:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRI: Image pullable support in dockershim #34380

CRI: Image pullable support in dockershim #34380

Random-Liu commented Oct 8, 2016 •

edited

Loading

k8s-ci-robot commented Oct 8, 2016

k8s-ci-robot commented Oct 8, 2016

Random-Liu commented Oct 8, 2016

feiskyer commented Oct 9, 2016

yifan-gu Oct 11, 2016

Random-Liu Oct 11, 2016 •

edited

Loading

yifan-gu Oct 11, 2016

yifan-gu Oct 11, 2016 •

edited

Loading

Random-Liu Oct 11, 2016 •

edited

Loading

yifan-gu Oct 11, 2016

Random-Liu commented Oct 11, 2016

Random-Liu commented Oct 11, 2016

Random-Liu commented Oct 11, 2016

feiskyer commented Oct 12, 2016

yujuhong Oct 12, 2016

yujuhong Oct 12, 2016

Random-Liu Oct 12, 2016

yujuhong Oct 12, 2016

Random-Liu Oct 12, 2016

yujuhong Oct 12, 2016

Random-Liu Oct 12, 2016 •

edited

Loading

yujuhong commented Oct 12, 2016

Random-Liu commented Oct 12, 2016 •

edited

Loading

Random-Liu commented Oct 12, 2016

yujuhong commented Oct 12, 2016

k8s-github-robot commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

k8s-github-robot commented Oct 12, 2016

CRI: Image pullable support in dockershim #34380

CRI: Image pullable support in dockershim #34380

Conversation

Random-Liu commented Oct 8, 2016 • edited Loading

k8s-ci-robot commented Oct 8, 2016

k8s-ci-robot commented Oct 8, 2016

Random-Liu commented Oct 8, 2016

feiskyer commented Oct 9, 2016

Choose a reason for hiding this comment

Random-Liu Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yifan-gu Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Random-Liu Oct 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu commented Oct 11, 2016

Random-Liu commented Oct 11, 2016

Random-Liu commented Oct 11, 2016

feiskyer commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Random-Liu Oct 12, 2016 • edited Loading

Choose a reason for hiding this comment

yujuhong commented Oct 12, 2016

Random-Liu commented Oct 12, 2016 • edited Loading

Random-Liu commented Oct 12, 2016

yujuhong commented Oct 12, 2016

k8s-github-robot commented Oct 12, 2016

k8s-ci-robot commented Oct 12, 2016

k8s-github-robot commented Oct 12, 2016

Random-Liu commented Oct 8, 2016 •

edited

Loading

Random-Liu Oct 11, 2016 •

edited

Loading

yifan-gu Oct 11, 2016 •

edited

Loading

Random-Liu Oct 11, 2016 •

edited

Loading

Random-Liu Oct 12, 2016 •

edited

Loading

Random-Liu commented Oct 12, 2016 •

edited

Loading