kubelet: Introduce PodInfraContainerChanged(). #6608

yifan-gu · 2015-04-09T00:33:11Z

@vmarmol @yujuhong
This new function computes in ahead whether we need to restart the pod
infra container.

Splitted from #6169 to enable smoother review.

Tests are broken, fixing...

vmarmol · 2015-04-09T00:54:53Z

pkg/kubelet/dockertools/docker.go

-	DockerPrefix          = "docker://"
+	PodInfraContainerName  = leaky.PodInfraContainerName
+	DockerPrefix           = "docker://"
+	PodInfraContainerImage = "gcr.io/google_containers/pause:0.8.0"


There is a flag for this, we should use it instead of this (the old version was unused I believe).

@vmarmol I have checked about this. The flag is stored in kubelet, I think we should move it to the DockManager?

SGTM, the PR was merged :)

yifan-gu · 2015-04-09T22:26:07Z

@vmarmol Hmm, I found by comparing the container directly, it's making the fake docker much complex since I need to construct a valid docker.Container in StartContainer and CreateContainer. What's the arguments for not using hash actually?

vmarmol · 2015-04-09T22:28:10Z

@yifan-gu I'm fine going the has route since we use that today, but it just seems out of place when we do equals other places today.

yifan-gu · 2015-04-09T22:32:46Z

@vmarmol Could you point some places where we do equals please? I didn't find one, maybe it's differently and better than me.

vmarmol · 2015-04-09T23:21:04Z

@yifan-gu we do it for PodStatus and for MirrorPod

yifan-gu · 2015-04-10T00:59:13Z

@vmarmol I see. Ideally this can be done in the future when we can reconstruct api.Container from the container runtime.

BTW, I fixed all the tests, to make it easier for you to review, I put those in the second commits :) I am running e2e now to see if this change works right.

pmorie · 2015-04-10T03:28:55Z

pkg/kubelet/kubelet_test.go

+		"list",                                 // Get pod status.
+		"create", "start", "inspect_container", // Create pod infra container.
+		"create", "start", // Create container.
+		"list", "inspect_container", "inspect_container"}) // Get pod status.


This is classy newline use; make the comments justified at the same position?

(and everywhere else) @yifan-gu

pmorie · 2015-04-10T03:34:22Z

Is PodInfraContainerChanged going to become part of the Container.Runtime interface eventually?

yifan-gu · 2015-04-10T03:55:14Z

@pmorie I don't think so, it's docker specific IMO, rkt doesn't need a pod infra container.

yifan-gu · 2015-04-10T18:01:55Z

Oops, got many e2e failures.
Debugging...

Summarizing 14 Failures:

[Fail] Services [It] should correctly serve identically named services in different namespaces on different external IP addresses 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/service.go:373

[Fail] kubectl guestbook [It] should create and stop a working application 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/util.go:304

[Fail] Density [It] should allow starting 3 pods per node 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/density.go:126

[Fail] Networking [It] should function for intra-pod communication 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/networking.go:273

[Fail] kubectl update-demo [It] should scale a replication controller 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/util.go:304

[Fail] emptyDir [It] volume on tmpfs should have the correct mode 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/util.go:325

[Fail] Pods [It] should be submitted and removed 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/pods.go:129

[Fail] ReplicationController [It] should serve a basic image on each replica with a private image 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/rc.go:126

[Fail] Shell [It] tests that services.sh passes 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/shell.go:69

[Fail] Services [It] should be able to create a functioning external load balancer 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/service.go:282

[Fail] ReplicationController [It] should serve a basic image on each replica with a public image 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/rc.go:95

[Fail] kubectl update-demo [It] should do a rolling update of a replication controller 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/util.go:304

[Fail] Pods [It] should be restarted with a /healthz http liveness probe 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/pods.go:68

[Fail] PD [It] should schedule a pod w/ a RW PD, remove it, then schedule it on another host 
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/pd.go:86

Ran 33 of 39 Specs in 1112.625 seconds
FAIL! -- 19 Passed | 14 Failed | 1 Pending | 5 Skipped F0410 10:38:47.441049   28149 driver.go:112] At least one test failed
2015/04/10 10:38:47 Error running Ginkgo tests: exit status 255
exit status 1

yifan-gu · 2015-04-10T18:02:56Z

Got a log of connection refused error, such as

INFO: After 10.313296623s failed to make proxy call to elasticsearch-logging: Get https://130.211.156.16/api/v1beta3/proxy/namespaces/default/services/elasticsearch-logging/_search?q=log%3Aes_logging_5752synthlogger&size=400: dial tcp 130.211.156.16:443: connection refused

Investigating

yifan-gu · 2015-04-10T18:47:03Z

The containers on master are keeping restarting... Sounds like a bug.

yifan-gu · 2015-04-11T01:49:38Z

Got 2 failures now:

Summarizing 2 Failures:

[Fail] Services [It] should be able to create a functioning external load balancer
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/service.go:282

[Fail] kubectl guestbook [It] should create and stop a working application
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/util.go:304

Ran 33 of 39 Specs in 909.800 seconds
FAIL! -- 31 Passed | 2 Failed | 1 Pending | 5 Skipped

yifan-gu · 2015-04-12T06:34:08Z

Fixed, it's because the target pools are not cleared.

Ran 33 of 39 Specs in 1216.063 seconds
SUCCESS! -- 33 Passed | 0 Failed | 1 Pending | 5 Skipped Shutting down test cluster in background

@vmarmol

yifan-gu · 2015-04-13T17:06:53Z

@vmarmol I think this is ready for another review?

vmarmol · 2015-04-13T17:08:48Z

Awesome, I'll take a look today @yifan-gu

yifan-gu · 2015-04-13T17:10:32Z

@vmarmol Thank you! FYI the 1st commit is the change made to source, 2nd is updating tests. I am running e2e again :)

yifan-gu · 2015-04-13T18:38:06Z

Have run for twice again, one passed all tests, one got a failure which I think is a known issue #6424

Summarizing 1 Failure:

[Fail] ReplicationController [It] should serve a basic image on each replica with a public image
/go/src/github.com/GoogleCloudPlatform/kubernetes/_output/dockerized/go/src/github.com/GoogleCloudPlatform/kubernetes/test/e2e/rc.go:151

Ran 33 of 39 Specs in 1219.849 seconds
FAIL! -- 32 Passed | 1 Failed | 1 Pending | 5 Skipped F0413 10:42:57.441241    4053 driver.go:115] At least one test failed

vmarmol · 2015-04-13T22:21:21Z

Outside of that one change (and the needed rebase), this looks good :)

This functions computes in ahead whether we need to restart the pod infra container.

Update tests.

yifan-gu · 2015-04-14T00:33:11Z

@vmarmol Rebased and ran e2e again. Looks good:

Ran 35 of 39 Specs in 1163.674 seconds
SUCCESS! -- 35 Passed | 0 Failed | 0 Pending | 4 Skipped Shutting down test cluster in background.
Bringing down cluster using provider: gce

vmarmol · 2015-04-14T00:35:26Z

LGTM, thanks for the diligence @yifan-gu! Will merge once the CIs go green.

yifan-gu · 2015-04-14T01:22:07Z

@vmarmol You're very welcome! I think cleaning up the sync logic might have potential to break the e2e, so I am trying to be more careful. I will keep an eye on that if any stuff fails in the future.

pmorie · 2015-04-14T01:37:50Z

Clean GCE e2e run? It's a miracle.

On Mon, Apr 13, 2015 at 9:22 PM, Yifan Gu notifications@github.com wrote:

@vmarmol https://github.com/vmarmol You're very welcome! I think
cleaning up the sync logic might have potential to break the e2e, so I am
trying to be more careful. I will keep an eye on that if any stuff fails in
the future.

—
Reply to this email directly or view it on GitHub
#6608 (comment)
.

kubelet: Introduce PodInfraContainerChanged().

googlebot added the cla: yes label Apr 9, 2015

yifan-gu force-pushed the infra_changed branch from 4d28d87 to 0b326d3 Compare April 9, 2015 00:35

vmarmol self-assigned this Apr 9, 2015

vmarmol reviewed Apr 9, 2015
View reviewed changes

yifan-gu force-pushed the infra_changed branch 4 times, most recently from 4cbd513 to de82552 Compare April 9, 2015 19:02

yifan-gu mentioned this pull request Apr 9, 2015

kubelet/fake_docker_client: Use self's PID instead of 42 in testing. #6653

Merged

yifan-gu force-pushed the infra_changed branch 3 times, most recently from da74906 to e1299f6 Compare April 10, 2015 00:22

pmorie reviewed Apr 10, 2015
View reviewed changes

yifan-gu force-pushed the infra_changed branch 2 times, most recently from 0b20ff7 to 6063f3a Compare April 11, 2015 01:41

yifan-gu force-pushed the infra_changed branch 2 times, most recently from ddcd5da to 7563bbe Compare April 12, 2015 05:57

yifan-gu force-pushed the infra_changed branch 3 times, most recently from ba7258c to aca5d8b Compare April 13, 2015 17:03

yifan-gu force-pushed the infra_changed branch from aca5d8b to 14fafbb Compare April 13, 2015 23:17

Yifan Gu added 2 commits April 13, 2015 16:18

kubelet: Introduce PodInfraContainerChanged().

d14cb8f

This functions computes in ahead whether we need to restart the pod infra container.

kubelet: Introduce PodInfraContainerChanged().

4cdb1bf

Update tests.

yifan-gu force-pushed the infra_changed branch from 14fafbb to 4cdb1bf Compare April 13, 2015 23:19

vmarmol added a commit that referenced this pull request Apr 14, 2015

Merge pull request #6608 from yifan-gu/infra_changed

f59a9ca

kubelet: Introduce PodInfraContainerChanged().

vmarmol merged commit f59a9ca into kubernetes:master Apr 14, 2015

yifan-gu mentioned this pull request Apr 15, 2015

kubelet: Add preparePod to check pod infra container. #6169

Closed

yifan-gu mentioned this pull request May 7, 2015

Unnecessary killing of the pod infra container. #5896

Closed

yifan-gu deleted the infra_changed branch May 7, 2015 17:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

kubelet: Introduce PodInfraContainerChanged(). #6608

kubelet: Introduce PodInfraContainerChanged(). #6608

yifan-gu commented Apr 9, 2015

vmarmol Apr 9, 2015

yifan-gu Apr 9, 2015

yifan-gu Apr 9, 2015

vmarmol Apr 9, 2015

yifan-gu commented Apr 9, 2015

vmarmol commented Apr 9, 2015

yifan-gu commented Apr 9, 2015

vmarmol commented Apr 9, 2015

yifan-gu commented Apr 10, 2015

pmorie Apr 10, 2015

pmorie Apr 10, 2015

pmorie commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 11, 2015

yifan-gu commented Apr 12, 2015

yifan-gu commented Apr 13, 2015

vmarmol commented Apr 13, 2015

yifan-gu commented Apr 13, 2015

yifan-gu commented Apr 13, 2015

vmarmol commented Apr 13, 2015

yifan-gu commented Apr 14, 2015

vmarmol commented Apr 14, 2015

yifan-gu commented Apr 14, 2015

pmorie commented Apr 14, 2015

kubelet: Introduce PodInfraContainerChanged(). #6608

kubelet: Introduce PodInfraContainerChanged(). #6608

Conversation

yifan-gu commented Apr 9, 2015

vmarmol Apr 9, 2015

Choose a reason for hiding this comment

yifan-gu Apr 9, 2015

Choose a reason for hiding this comment

yifan-gu Apr 9, 2015

Choose a reason for hiding this comment

vmarmol Apr 9, 2015

Choose a reason for hiding this comment

yifan-gu commented Apr 9, 2015

vmarmol commented Apr 9, 2015

yifan-gu commented Apr 9, 2015

vmarmol commented Apr 9, 2015

yifan-gu commented Apr 10, 2015

pmorie Apr 10, 2015

Choose a reason for hiding this comment

pmorie Apr 10, 2015

Choose a reason for hiding this comment

pmorie commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 10, 2015

yifan-gu commented Apr 11, 2015

yifan-gu commented Apr 12, 2015

yifan-gu commented Apr 13, 2015

vmarmol commented Apr 13, 2015

yifan-gu commented Apr 13, 2015

yifan-gu commented Apr 13, 2015

vmarmol commented Apr 13, 2015

yifan-gu commented Apr 14, 2015

vmarmol commented Apr 14, 2015

yifan-gu commented Apr 14, 2015

pmorie commented Apr 14, 2015