Update pod status only when it changes. #5714

fgrzadkowski · 2015-03-20T16:42:51Z

Update pod status only when it changes.
Refactor syncing logic into a separate struct

This PR is ready for review, but I haven't run e2e tests. I'm sending this now so that I can address comments on Monday CET time.

This fixes #5624 and #5693

I plan to apply the same logic (update upon change) for node status updates.

@vmarmol @bgrant0607 @dchen1107 @smarterclayton

vmarmol · 2015-03-20T16:51:53Z

Shippable is complaining about gofmt.

vmarmol · 2015-03-20T16:56:04Z

pkg/kubelet/kubelet.go

 }

-func (kl *Kubelet) generatePodStatus(podFullName string, uid types.UID) (api.PodStatus, error) {
+func (kl *Kubelet) generatePodStatus(podFullName string) (api.PodStatus, error) {


Eventually we should subsume this into the new status object too. Not for this PR though :) there is some more refactoring needed before that I think.

fgrzadkowski · 2015-03-23T12:11:51Z

e2e tests pass (liveness.sh flaked - successful 3/4 times)

vmarmol · 2015-03-23T16:06:18Z

pkg/kubelet/status_manager.go

+	status      api.PodStatus
+}
+
+type statusManager struct {


Can we comment that the implementation is thread-safe?

vmarmol · 2015-03-23T16:26:58Z

Looks good, just two comments. Also needs a rebase it seems.

yujuhong · 2015-03-23T17:04:06Z

pkg/kubelet/status_manager.go

+			}
+			_, err = s.kubeClient.Pods(namespace).UpdateStatus(name, &status)
+			if err != nil {
+				glog.Warningf("Error updating status for pod %q: %v", name, err)


If the apiserver request fails, would we lose this status transition completely?

That's a good point, we should retry on failure. Easy way is to remove the status from our map so that SetStatus() will re-send it. We won't be able to lock around the send in SetStatus() though.

Wow! That's a good catch! Thanks :) Fixed. I'm deleting cached value to make sure it is retried next time.

fgrzadkowski · 2015-03-23T19:42:08Z

Rebased.

fgrzadkowski · 2015-03-23T20:38:07Z

For some reason integration test is flaky. I'm still not sure whether it's related to this PR. I'll investigate tomorrow.

smarterclayton · 2015-03-23T20:51:46Z

It's fixed in #5775

We were always terminating the podRunning loop early for mirror pods.

----- Original Message -----

For some reason integration test is flaky. I'm still not sure whether it's
related to this PR. I'll investigate tomorrow.

Reply to this email directly or view it on GitHub:
#5714 (comment)

fgrzadkowski · 2015-03-23T21:47:38Z

Thanks. This helped!

dchen1107 · 2015-03-23T22:12:10Z

pkg/kubelet/kubelet.go

 	if found {
 		glog.V(3).Infof("Returning cached status for %q", podFullName)
 		return cachedPodStatus, nil
 	}
-	return kl.generatePodStatus(podFullName, uid)
+	return kl.generatePodStatus(podFullName)


Kubelet failed to retrieve cached PodStatus from statusManager, regenerate it from scratch. Shouldn't this update the cached one in statusManager too?

dchen1107 · 2015-03-23T22:14:57Z

LGTM overall except one small nit / question.

fgrzadkowski · 2015-03-23T23:05:55Z

It seems that after my last rebase, e2e stopped working. It's pretty late here, so I'll investigate tomorrow.

fgrzadkowski · 2015-03-23T23:06:16Z

FYI - they were passing before the rebase.

vmarmol · 2015-03-24T04:28:40Z

There were issues with the e2e tests, may be worth re-running after a rebase.

dchen1107 · 2015-03-24T05:44:15Z

You have to rebase first before merge.

dchen1107 · 2015-03-24T06:46:40Z

I thought I proposed having podWorker periodically update PodStatus before, and we agreed to have that in a separate PR later. But now with introducing statusManager to populate PodStatus for all pods, it looks like we take a different approach. Why we prefer this way?

fgrzadkowski · 2015-03-24T08:22:43Z

@dchen1107 I don't see how is it different. The only thing that has changed is that syncing logic is outside of kubelet itself. We still populate pod status from pod worker via SyncPod method that is called in PodWorker.
Initially the logic was pretty simple - periodically update status. Now that we want to update it only upon change it gets slightly more complicated so I thought it's worth a separate class for this.

fgrzadkowski · 2015-03-24T09:26:47Z

Rebased. It helped for for e2e tests that pass now.

@vmarmol Let's merge, before I need to rebase again :)

vmarmol · 2015-03-24T15:07:08Z

Oooh man, somehow it seems we'll need to rebase again! sorry @fgrzadkowski :( I'm here waiting by the merge button

fgrzadkowski · 2015-03-24T15:38:14Z

Rebased :)

* Refactor syncing logic into a separate struct

fgrzadkowski · 2015-03-24T15:42:38Z

And rebased again. This time looks OK.

dchen1107 · 2015-03-24T15:47:30Z

@fgrzadkowski Here is the reason on why: Currently PodWorker create and run containers which might fail due to all kinds of reasons. Sync Status logic uses docker inspect to retrieve the reason for failure, but there is no easy way to find on why a container failed on creation. If PodWorker does both, we could propogate the creation failures much cleaner.

I am ok with what we are having here for now.

vmarmol · 2015-03-24T15:52:34Z

@dchen1107 I think we do what you'd expect today. We set the pod status in the syncWorker but we actually send it in an async thread. We will propagate the creation failure.

vmarmol · 2015-03-24T15:52:59Z

LGTM, will merge on green. Thanks @fgrzadkowski! You get the "most rebased" award :)

Update pod status only when it changes.

dchen1107 · 2015-03-24T16:28:28Z

@fgrzadkowski and @vmarmol ahh, I misread that code since the code is outside both PodWorker and Kubelet, but actually setter logic is invoked by PodWorker. Thanks! LGTM

googlebot added the cla: yes label Mar 20, 2015

vmarmol self-assigned this Mar 20, 2015

vmarmol reviewed Mar 20, 2015
View reviewed changes

fgrzadkowski force-pushed the update_status_on_change branch from d48fa4e to 415733d Compare March 20, 2015 18:40

fgrzadkowski mentioned this pull request Mar 23, 2015

after a pod is scheduled, it should take < 5 seconds for apiserver to show it as running #3952

Closed

fgrzadkowski force-pushed the update_status_on_change branch from 415733d to f068d61 Compare March 23, 2015 11:05

wojtek-t mentioned this pull request Mar 23, 2015

Kubelet config sources should use the provided hostname, not lookup os.Hostname() #5775

Merged

fgrzadkowski force-pushed the update_status_on_change branch from f068d61 to 6af068a Compare March 23, 2015 14:49

vmarmol reviewed Mar 23, 2015
View reviewed changes

yujuhong reviewed Mar 23, 2015
View reviewed changes

fgrzadkowski force-pushed the update_status_on_change branch from 6af068a to f0615c1 Compare March 23, 2015 19:42

fgrzadkowski force-pushed the update_status_on_change branch from f0615c1 to c88c703 Compare March 23, 2015 21:40

dchen1107 reviewed Mar 23, 2015
View reviewed changes

yujuhong mentioned this pull request Mar 24, 2015

Clients should not check conditions, UpdateStatus() is inconsistent #5738

Merged

wojtek-t mentioned this pull request Mar 24, 2015

Scheduling plateaus due to POD killing #5654

Closed

fgrzadkowski force-pushed the update_status_on_change branch from c88c703 to 0dd7743 Compare March 24, 2015 15:37

* Update pod status only when it changes.

632ca50

* Refactor syncing logic into a separate struct

fgrzadkowski force-pushed the update_status_on_change branch from 0dd7743 to 632ca50 Compare March 24, 2015 15:41

vmarmol added a commit that referenced this pull request Mar 24, 2015

Merge pull request #5714 from fgrzadkowski/update_status_on_change

a2e2fea

Update pod status only when it changes.

vmarmol merged commit a2e2fea into kubernetes:master Mar 24, 2015

fabioy mentioned this pull request Mar 24, 2015

Nodes seem to be updating its status way too frequently #5864

Closed

This was referenced Mar 24, 2015

Delete pod_cache and rely on updating pod status by kublet. #5854

Merged

Improve replication controller manager performance #5884

Closed

dchen1107 mentioned this pull request Mar 27, 2015

Potential pod scheduling bug #6107

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update pod status only when it changes. #5714

Update pod status only when it changes. #5714

fgrzadkowski commented Mar 20, 2015

vmarmol commented Mar 20, 2015

vmarmol Mar 20, 2015

fgrzadkowski commented Mar 23, 2015

vmarmol Mar 23, 2015

fgrzadkowski Mar 23, 2015

vmarmol commented Mar 23, 2015

yujuhong Mar 23, 2015

vmarmol Mar 23, 2015

fgrzadkowski Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

smarterclayton commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

dchen1107 Mar 23, 2015

dchen1107 commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

vmarmol commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

vmarmol commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

vmarmol commented Mar 24, 2015

vmarmol commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

Update pod status only when it changes. #5714

Update pod status only when it changes. #5714

Conversation

fgrzadkowski commented Mar 20, 2015

vmarmol commented Mar 20, 2015

vmarmol Mar 20, 2015

Choose a reason for hiding this comment

fgrzadkowski commented Mar 23, 2015

vmarmol Mar 23, 2015

Choose a reason for hiding this comment

fgrzadkowski Mar 23, 2015

Choose a reason for hiding this comment

vmarmol commented Mar 23, 2015

yujuhong Mar 23, 2015

Choose a reason for hiding this comment

vmarmol Mar 23, 2015

Choose a reason for hiding this comment

fgrzadkowski Mar 23, 2015

Choose a reason for hiding this comment

fgrzadkowski commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

smarterclayton commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

dchen1107 Mar 23, 2015

Choose a reason for hiding this comment

dchen1107 commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

fgrzadkowski commented Mar 23, 2015

vmarmol commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

vmarmol commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

fgrzadkowski commented Mar 24, 2015

dchen1107 commented Mar 24, 2015

vmarmol commented Mar 24, 2015

vmarmol commented Mar 24, 2015

dchen1107 commented Mar 24, 2015