Make kubelet first acknowledge time of a pod as Pod.Status.StartTime. #10066
Conversation
|
ok to test |
|
GCE e2e build/test failed for commit 1145e4b. |
| @@ -1171,7 +1171,9 @@ func (kl *Kubelet) syncPod(pod *api.Pod, mirrorPod *api.Pod, runningPod kubecont | |||
| var podStatus api.PodStatus | |||
| if updateType == SyncPodCreate { | |||
| podStatus = pod.Status | |||
| glog.V(3).Infof("Not generating pod status for new pod %v", podFullName) | |||
| podStatus.StartTime = &util.Time{start} | |||
There was a problem hiding this comment.
I think setPodStatus will already set the time if it's unset.
The impact here is extra qps, because we'll send one update for the time stamp and one update for the running in the defer call, but I'm hoping it won't be too bad.
So I actually wanted to do something a little different (just mentioning it here, not sure if you want to do it): Call SetPodStatus here with the actual pod.Status given to this method, and set the StartTime on that pod when we receive the watch notification (https://github.com/GoogleCloudPlatform/kubernetes/blob/master/pkg/kubelet/config/config.go#L264 is close enough to the entry point). That would help our e2e tests gather latency numbers, but I'm not going to push too hard for it because it might shortchange the activeDeadline feature :)
There was a problem hiding this comment.
Yes, setPodStatus does set the time if it's unset. The only reason I am doing it here is that I can reuse the initial timestamp: start. Before initializing PodStatus, kubelet does setup for pod, such as mount. Based on my previous experience, mount could be very slow operation taking many minutes under resource pressure, especially with memory.
I did consider to set StartTim on the watch notification initially. But like what you mentioned, I have concern with activeDeadline feature.
There was a problem hiding this comment.
Sounds good, guess we discussed the rest of it irl
|
LGTM just the one nit, and a little hope that we'll end up doing what i want |
|
e2e test failures are flaky tests. ok to test |
bprashanth
added
the
lgtm
|
@bprashanth - I am fine setting the StartTime when we receive the watch notification because that is literally when the Kubelet became aware of the pod on its host. As a result, I think its the most consistent solution to the spirit of |
|
ok to test |
|
GCE e2e build/test passed for commit 1145e4b. |
Make kubelet first acknowledge time of a pod as Pod.Status.StartTime.
cc/ @derekwaynecarr @bprashanth
Fix #9792