Avoid trying to generate the pod status of a new pod #9549
Conversation
|
EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded. |
|
Added unittest and this passed e2e, so it should be good for review. Will squash commits before checkin. |
|
EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded. |
|
EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded. |
|
EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded. |
|
EXPERIMENTAL JENKINS PR BUILDER: e2e build succeeded. |
|
GCE e2e build/test failed for commit d4bbfe39f6cfc6d02b3de40f125a9517283dbc2e. |
|
e2e failure is unrelated and fixed with #9647 |
| @@ -2789,7 +2789,7 @@ func TestCreateMirrorPod(t *testing.T) { | |||
| } | |||
| pods := []*api.Pod{pod} | |||
| kl.podManager.SetPods(pods) | |||
| err := kl.syncPod(pod, nil, container.Pod{}) | |||
There was a problem hiding this comment.
Some test for SyncPodCreate could be useful.
|
I don't like using metrics.SyncPodType (i.e. some monitoring type) in "ordinary" production code. If it is needed we should have it as a first class citizen in Kubelet codebase and use this in monitoring, not the other way around. |
|
Thinking about kubelet restart |
| if updateType == metrics.SyncPodCreate { | ||
| podStatus = pod.Status | ||
| glog.V(3).Infof("Not generating pod status for new pod %v", podFullName) | ||
| } else { |
There was a problem hiding this comment.
Let's kill the else, e.g.:
var podStatus api.PodStatus
var err error
if updateType != metrics.SyncPodCreate {
podStatus, err = kl.generatePodStatus(pod)
...
}There was a problem hiding this comment.
I wanted the logging so I kept the else
bprashanth
force-pushed
the
generate_fix
branch
3 times, most recently
from
e10188a
to
a53a86b
Compare
|
PTAL @dchen1107 would appreciate if you could also take a quick look |
|
GCE e2e build/test passed for commit b74ed284e7491672831f5da39d11c7b1f938ead2. |
|
GCE e2e build/test passed for commit e10188a79067dd2b994a1653184ee5072f03e196. |
|
GCE e2e build/test passed for commit a53a86b90a17af99061bac9f28411b920c468ac5. |
| @@ -131,6 +142,7 @@ func (p *podWorkers) UpdatePod(pod *api.Pod, mirrorPod *api.Pod, updateComplete | |||
| // the channel is empty, so buffer of size 1 is enough. | |||
| podUpdates = make(chan workUpdate, 1) | |||
| p.podUpdates[uid] = podUpdates | |||
| updateType = SyncPodCreate | |||
There was a problem hiding this comment.
More comments here to explain why reset this updateType here to Create?
There was a problem hiding this comment.
I'm assuming that if you created a pod worker you're willing to believe the status in the pod without having generateStatus again, either because it is a brand new pod or the kubelet restarted. I'll add this as a comment.
|
LGTM except one small nit. |
|
Comment added, PTAL |
|
GCE e2e build/test failed for commit 8ad79322f68db825849447cfbb4fe5debbbe7871. |
|
GCE e2e build/test failed for commit b5ed0e9. |
gmarek
added
the
lgtm
|
LGTM. We'll need to figure out proper way of handling Kubelet restarts in the future. |
Avoid trying to generate the pod status of a new pod
The approach in the first commit is cleaner but won't work if someone runs the kubelet with PodConfigNotificationSnapshots. The second is a little hacky but scopes the application of updateType=create to only new pod workers.
still e2e-ing, and thinking about some edge cases, after which I'll add some tests.
/ref #9429 @dchen1107 @yujuhong