CRI: Enable custom infra container image #33488

resouer · 2016-09-26T14:53:59Z

A minor fix to enable custom infra container image ref #29478

Need to address:
Not sure how do deal with infra image credential, leave it as it is today. Should we allow user to specify credentials in pod yaml?

This change is

resouer · 2016-09-27T02:47:24Z

@k8s-bot e2e test this please #33175

k8s-ci-robot · 2016-09-28T04:53:20Z

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

Random-Liu · 2016-09-28T17:31:31Z

pkg/kubelet/api/v1alpha1/runtime/api.pb.go

-	XXX_unrecognized []byte                 `json:"-"`
+	Linux *LinuxPodSandboxConfig `protobuf:"bytes,8,opt,name=linux" json:"linux,omitempty"`
+	// Optional custom image of pod's infra container
+	PodInfraContainerImage *string `protobuf:"bytes,9,opt,name=podInfraContainerImage" json:"podInfraContainerImage,omitempty"`


I'm not sure whether we want to pass this through CRI. At least, currently infra container is a docker specific thing.

IIRC, we can only specify infra container image per-node not per-container, if so I feel like we should pass this to dockershim when creating docker service instead of passing through CRI.

/cc @yujuhong @feiskyer

Yes, "infra" image is a docker-specific implementation detail. I think we'll be better off passing this to the internal DockerService for now, and eventually this should be configured directly for the CRI shim (either through flags or a configuartion file).

hmm, only passing PodInfraContainerImage to dockershim is enough. It shouldn't be part of CRI.

Random-Liu · 2016-09-28T17:41:36Z

Not sure how do deal with infra image credential, leave it as it is today. Should we allow user to specify credentials in pod yaml?

We have an issue discussing this before #22344.

vishh · 2016-09-28T22:12:20Z

I don't see why the infra container needs to be in kubelet. It is a runtime specific implementation detail. I'd also not recommend making custom infra containers a legit thing, because we intend to make infra containers init daemons in the future.

yujuhong · 2016-09-28T22:27:15Z

I don't see why the infra container needs to be in kubelet. It is a runtime specific implementation detail. I'd also not recommend making custom infra containers a legit thing, because we intend to make infra containers init daemons in the future.

As @Random-Liu mentioned above, infra container is a docker-specific implementation detail. The issue was to support the current behavior in the CRI shim for docker, not via the API.

As for "custom" infra containers, this is already a thing since we have a kubelet flag for it, and there are users relying on it. I don't see how we can suddently stop supporting it.
If infra container is a runtime implementation detail, I don't think we can make infra containers init daemons.

feiskyer · 2016-09-29T03:38:20Z

Agree with @yujuhong, the infra container image feature shouldn't be dropped, but it also shouldn't be part of CRI. I think pass the image to dockershim directly in kubelet is ok.

resouer · 2016-09-30T01:09:59Z

Will use http://kubernetes.io/docs/user-guide/images/#configuring-nodes-to-authenticate-to-a-private-repository to deal with credentials, and move commits to DockerService

k8s-ci-robot · 2016-10-02T21:51:21Z

Jenkins GCE Node e2e failed for commit b8013d2. 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 · 2016-10-02T22:48:01Z

Jenkins Kubemark GCE e2e failed for commit b8013d2. 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.

resouer · 2016-10-03T18:19:03Z

The result of Jenkins Kubemark has no clue for what's failing ...

yujuhong · 2016-10-03T18:21:25Z

@resouer this is #33867. I don't think the kubemark suite is blocking, so just ignore the failure for now.

Random-Liu · 2016-10-03T18:35:26Z

pkg/kubelet/dockershim/docker_sandbox.go

-	// (with credentials)?
-	image := defaultSandboxImage
+	var (
+		image string


Why not var image string?

Random-Liu · 2016-10-03T18:44:24Z

LGTM with one nit.

yujuhong · 2016-10-03T19:41:04Z

pkg/kubelet/dockershim/docker_sandbox.go

+	var (
+		image string
+	)
+	infraContainerImage := ds.podInfraContainerImage


You can simplify line 48 to 56 by doing

image := defaultSandboxImage if len(infraContainerImage) != 0 { image = infraContainerImage }

yujuhong · 2016-10-03T19:44:25Z

pkg/kubelet/dockershim/docker_sandbox.go

+		image = defaultSandboxImage
+	}
+
+	// NOTE(harryz) Assume user to handle custom pod infra container image pulling by following:


Is this a TODO or just a regular comment? If it's the latter, there is no need to add your name here (as there is no action item).

nit: Change it to To use a custom sandbox image in a private repository, users need to configure the nodes with credentials properly. http://kubernetes.io/docs/user-guide/images/#configuring-nodes-to-authenticate-to-a-private-repository

yujuhong · 2016-10-03T19:45:06Z

pkg/kubelet/dockershim/docker_service.go

@@ -76,7 +77,8 @@ type DockerLegacyService interface {
 }

 type dockerService struct {
-	client dockertools.DockerInterface
+	client                 dockertools.DockerInterface
+	podInfraContainerImage string


Rename the field to podSandboxImage. We should not use the "infra container" name anymore in this package.

Modify api protoc for infra

resouer · 2016-10-04T04:08:33Z

rebased and nit fixed in latest commit

Random-Liu · 2016-10-04T04:31:57Z

LGTM. Wait for @yujuhong to approve because she had change request before.

yujuhong · 2016-10-04T15:39:43Z

LGTM. Thanks for the PR!

k8s-github-robot · 2016-10-04T17:26:39Z

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

k8s-github-robot · 2016-10-04T18:11:29Z

Automatic merge from submit-queue

googlebot added the cla: yes label Sep 26, 2016

k8s-github-robot assigned vishh Sep 26, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 26, 2016

resouer force-pushed the infra-image branch from 7d1d050 to 0db3893 Compare September 26, 2016 15:01

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

resouer mentioned this pull request Sep 27, 2016

CRI: Support pod sandbox env in kuberuntime #33561

Closed

resouer force-pushed the infra-image branch from 0db3893 to 555f957 Compare September 28, 2016 04:09

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

Random-Liu reviewed Sep 28, 2016

View reviewed changes

Random-Liu added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note-label-needed labels Sep 28, 2016

Random-Liu assigned Random-Liu and yujuhong and unassigned vishh Sep 28, 2016

yujuhong mentioned this pull request Sep 29, 2016

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

Closed

25 tasks

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

resouer force-pushed the infra-image branch 2 times, most recently from d6275a2 to b8013d2 Compare October 2, 2016 21:33

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 2, 2016

Random-Liu reviewed Oct 3, 2016

View reviewed changes

yujuhong suggested changes Oct 3, 2016

View reviewed changes

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

Enabel custom infracontainer image

29c6a3b

Modify api protoc for infra

resouer force-pushed the infra-image branch from b8013d2 to 29c6a3b Compare October 4, 2016 04:07

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

yujuhong approved these changes Oct 4, 2016

View reviewed changes

yujuhong added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 4, 2016

yujuhong added this to the v1.5 milestone Oct 4, 2016

k8s-github-robot merged commit d26b4ca into kubernetes:master Oct 4, 2016

k8s-github-robot mentioned this pull request Oct 6, 2016

[k8s.io] Downward API volume should update annotations on modification [Conformance] {Kubernetes e2e suite} #34014

Closed

resouer deleted the infra-image branch February 15, 2017 12:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CRI: Enable custom infra container image #33488

CRI: Enable custom infra container image #33488

resouer commented Sep 26, 2016 •

edited

Loading

resouer commented Sep 27, 2016

k8s-ci-robot commented Sep 28, 2016

Random-Liu Sep 28, 2016 •

edited

Loading

Random-Liu Sep 28, 2016

yujuhong Sep 28, 2016

feiskyer Sep 29, 2016

Random-Liu commented Sep 28, 2016

vishh commented Sep 28, 2016

yujuhong commented Sep 28, 2016

feiskyer commented Sep 29, 2016

resouer commented Sep 30, 2016

k8s-ci-robot commented Oct 2, 2016

k8s-ci-robot commented Oct 2, 2016

resouer commented Oct 3, 2016

yujuhong commented Oct 3, 2016

Random-Liu Oct 3, 2016

Random-Liu commented Oct 3, 2016

yujuhong Oct 3, 2016

yujuhong Oct 3, 2016

yujuhong Oct 3, 2016

resouer commented Oct 4, 2016

Random-Liu commented Oct 4, 2016

yujuhong commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

CRI: Enable custom infra container image #33488

CRI: Enable custom infra container image #33488

Conversation

resouer commented Sep 26, 2016 • edited Loading

resouer commented Sep 27, 2016

k8s-ci-robot commented Sep 28, 2016

Random-Liu Sep 28, 2016 • edited Loading

Choose a reason for hiding this comment

Random-Liu Sep 28, 2016

Choose a reason for hiding this comment

yujuhong Sep 28, 2016

Choose a reason for hiding this comment

feiskyer Sep 29, 2016

Choose a reason for hiding this comment

Random-Liu commented Sep 28, 2016

vishh commented Sep 28, 2016

yujuhong commented Sep 28, 2016

feiskyer commented Sep 29, 2016

resouer commented Sep 30, 2016

k8s-ci-robot commented Oct 2, 2016

k8s-ci-robot commented Oct 2, 2016

resouer commented Oct 3, 2016

yujuhong commented Oct 3, 2016

Random-Liu Oct 3, 2016

Choose a reason for hiding this comment

Random-Liu commented Oct 3, 2016

yujuhong Oct 3, 2016

Choose a reason for hiding this comment

yujuhong Oct 3, 2016

Choose a reason for hiding this comment

yujuhong Oct 3, 2016

Choose a reason for hiding this comment

resouer commented Oct 4, 2016

Random-Liu commented Oct 4, 2016

yujuhong commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

k8s-github-robot commented Oct 4, 2016

resouer commented Sep 26, 2016 •

edited

Loading

Random-Liu Sep 28, 2016 •

edited

Loading