Periodically update pod status from kubelet. #5555
Conversation
|
LGTM @fgrzadkowski, but it'll need a rebase :P |
| // Create the list of pods. | ||
| podFullNames := make(map[types.UID]string) | ||
| func() { | ||
| kl.podLock.Lock() |
There was a problem hiding this comment.
You are not writing to kl.pods, so you can use GetPods() (which does RLock()) to get a copy of kl.pods.
|
@fgrzadkowski, you'll need to rebase because my PR (#5401) was just merged. I left some in-line comments to help you with that. One nit. Now that we have two locks podStatusLock and podLock, which are used in multiple places, it'd be nice to point out in the comment that some use patterns may lead to deadlocks. E.g. if you grab the locks in the order of podStatusLock and podLock, it'd cause deadlock. |
|
In fact, now that kubelet post pod statuses directly, we should be able to simply the lock usage. I'll write up another PR after this is merged, so don't worry about it. |
|
As mentioned in #5538, you should add new integration tests for pod status, and make sure that all integration tests pass. |
fgrzadkowski
force-pushed
the
sync_pod_status
branch
from
f09a32b to
1a352b7
Compare
|
Thanks for rebasing! The code LGTM. I'd like to see the integration test be fixed to reflect how we sync the pod status, but that could also be addressed in another PR. I'll leave it to @vmarmol to decide. Did the PR pass e2e? |
|
Re integration tests - I'd prefer to add them in a separate PR (I already assigned myself to that issue I filed) Re e2e tests - they pass. I checked this morning and everything worked as expected. |
|
Another PR for the tests SGTM (we know where to find you and collect on our integration test ;) ). Thanks for taking a look @yujuhong! |
Periodically update pod status from kubelet.
|
I've just noticed - that's a pretty PR number to have :D 5555 |
|
Haha you're right I didn't even notice! A good PR for that number. |
|
|
||
| // UpdateStatus takes the name of the pod and the new status. Returns the server's representation of the pod, and an error, if it occurs. | ||
| func (c *pods) UpdateStatus(name string, newStatus *api.PodStatus) (result *api.Pod, err error) { | ||
| result = &api.Pod{} |
There was a problem hiding this comment.
I don't think this is the right implementation - a client should be required to send in an api.Pod to do this.
There was a problem hiding this comment.
The reason for this is that you want to be able to force clients to deal with specifying a resource version, and this doesn't allow it (it's basically an overwrite).
There was a problem hiding this comment.
Going to submit a pull that deals with this.
There was a problem hiding this comment.
SGTM, thanks @smarterclayton
There was a problem hiding this comment.
@smarterclayton, could you hold off on the PR to fix this? UpdateStatus needs to update the status of the mirror pods too. We'll need keep the mirror pods objects and use them here. I am working on a pod manager refactoring which will take care of this.
There was a problem hiding this comment.
I wondered why the mirror status test is failing. I'll wait until after your change to merge #5738. Note my pull request changes SimpleRunKubelet so we can dial down the sync selector in the integration test.
----- Original Message -----
@@ -113,3 +114,15 @@ func (c *pods) Watch(label labels.Selector, field
fields.Selector, resourceVersi
func (c *pods) Bind(binding *api.Binding) error {
return
c.r.Post().Namespace(c.ns).Resource("pods").Name(binding.Name).SubResource("binding").Body(binding).Do().Error()
}
+
+// UpdateStatus takes the name of the pod and the new status. Returns the
server's representation of the pod, and an error, if it occurs.
+func (c *pods) UpdateStatus(name string, newStatus *api.PodStatus) (result
*api.Pod, err error) {
- result = &api.Pod{}
@smarterclayton, could you hold off on the PR to fix this? UpdateStatus needs
to update the status of the mirror pods too. We'll need keep the mirror pods
objects and use them here. I am working on a pod manager refactoring which
will take care of this.
Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5555/files#r26880558
This PR is based on #5205 with two bug fixes:
This fixes #4561