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

smarterclayton · 2015-03-20T22:24:30Z

Corrects some of the cargo culting that has crept into the API.

Fixes some client issues that have crept in over time. Makes UpdateStatus
consistent with its intended use. Makes integration tests have a shorter sync
interval so they can be tested.

@yujuhong please comment here when you have fixed mirror pods status

googlebot · 2015-03-20T22:24:31Z

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

smarterclayton · 2015-03-20T22:27:28Z

The integration test for mirror status pods is failing, waiting for @yujuhong to let this proceed

yujuhong · 2015-03-20T22:34:47Z

Thanks for fixing this! I will update once the refactoring is done.

smarterclayton · 2015-03-22T23:00:03Z

As a note on this change - the server must be able to handle all of the errors that are not related to a client object properly querying the server for a request, so that clients in other languages do not also need to add these checks (namespace equality, validation of values, update consistency). Only checks required to build a proper URL (namespace, name, and type) should be checked, and they should be checked consistently.

smarterclayton · 2015-03-22T23:22:20Z

The integration failure was actually #5757 - this has no blockers except @yujuhong 's hold

nikhiljindal · 2015-03-23T21:08:52Z

I looked at the "Clients should not check conditions, UpdateStatus() is inconsistent" commit. LGTM.
Would love to see this merged soon.
Needs a rebase.

smarterclayton · 2015-03-23T22:08:32Z

Rebased

nikhiljindal · 2015-03-23T22:15:27Z

Thanks, LGTM.
Ready to merge whenever @yujuhong 's hold is lifted.

yujuhong · 2015-03-24T00:52:52Z

#5826 refactors pod manager to allow mirror pod lookup. Once that's in, I could do a quick fix for the mirror pods. Sorry for the the wait and the rebasing.

Also, #5714 is touching the same part of the code, so there might be some contention and some more rebasing.

smarterclayton · 2015-03-24T00:54:30Z

That's fine - just let me know when you're ready.

On Mar 23, 2015, at 8:53 PM, Yu-Ju Hong notifications@github.com wrote:

#5826 refactors pod manager to allow mirror pod lookup. Once that's in, I could do a quick fix for the mirror pods. Sorry for the the wait and the rebasing.

Also, #5714 is touching the same part of the code, so there might be some contention and some more rebasing.

—
Reply to this email directly or view it on GitHub.

Pod status update should include the ObjectMeta of the pod. This change is required for kubernetes#5738 to merge.

smarterclayton · 2015-03-25T18:52:15Z

Rebased on top of the latest code

yujuhong · 2015-03-25T20:07:06Z

pkg/kubelet/status_manager.go

+			}
+			// TODO: make me easier to express from client code
+			// TODO: resource version on a pod should be saved when dealing with mirror pods so that
+			// subsequent updates don't require a GET.


TODO can be removed. Mirror pod has already been passed as pod here.

There is also no need to GET pod below.

The Get is needed - update status should not be calling get (that was removed). You have to Get to get the latest version so you can overwrite.

I don't quite understand what makes mirror pods different from regular pods in this case...

Also, do we always want to overwrite the status even though kubelet was syncing to an older pod spec?

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

@@ -104,7 +104,22 @@ func (s *statusManager) SyncBatch() {
podFullName := kubecontainer.GetPodFullName(pod)
status := syncRequest.status
glog.V(3).Infof("Syncing status for %s", podFullName)

_, err := s.kubeClient.Pods(pod.Namespace).UpdateStatus(pod.Name,
&status)
+

var err error

statusPod := &api.Pod{

ObjectMeta: pod.ObjectMeta,

}

// TODO: make me easier to express from client code

// TODO: resource version on a pod should be saved when dealing with
mirror pods so that

// subsequent updates don't require a GET.

I don't quite understand what makes mirror pods different from regular pods
in this case...

Regular pods are getting updates from other sources, so the Kubelet's watch on the master will report changes to them and fill an updated resource version, but it might lag (so the resource version the kubelet knows is less likely to be the most recent version). Mirror pods start from the Kubelet, so the resource version of a mirror pod update is known almost all the time. It's not a huge distinction.

In general, the resource version the kubelet has last seen should be used instead of doing the GET here. Done right, most status updates would be a single operation (a PUT) without conflict. However, if there is a race, the kubelet needs to retry the status update. We've discussed letting the server allow a server-overwrite for certain actions, which would put the logic on the server side, but that isn't implemented. In general, avoiding the GET would be ideal, but I'm not convinced the Kubelet will be the only client that is setting status either (network management plugins?). So we will eventually need to intelligently merge the status on the Kubelet side during an update.

Also, do we always want to overwrite the status even though kubelet was
syncing to an older pod spec?

I'm sure there's some unpleasant race conditions and timing problems here. The Kubelet may wish to delay updating status when its pod spec doesn't match the master's. Also, there is a general problem that a 409 should return the resource as it exists on the server so that a client doesn't need to re-get. That would reduce the complexity for clients that wish to implement overwrite or atomic retry. I have a todo to open an issue on it.

Reply to this email directly or view it on GitHub:
https://github.com/GoogleCloudPlatform/kubernetes/pull/5738/files#r27162095

Corrects some of the cargo culting that has crept into the API.

yujuhong · 2015-03-25T20:30:35Z

pkg/kubelet/status_manager.go

-			_, err := s.kubeClient.Pods(pod.Namespace).UpdateStatus(pod.Name, &status)
+
+			var err error
+			statusPod := &api.Pod{


statusPod is always overwritten in line 115, what's the point of defining it here?

There has to be a scope variable for the pod that we read and then write. I'm fine with it being var statusPod *api.Pod

yujuhong · 2015-03-25T20:52:15Z

Ok. I think I may have misunderstood your original statement. If we just want to GET the pod from the apiserver before sending the update, there should not be any difference between regular and mirror pods. (and we don't need the actual pod passed the SetPodStatus() at all)

smarterclayton · 2015-03-25T20:53:19Z

Correct. But eventually having more info about the Kubelet's view of the world vs the master's would be good.

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

Ok. I think I may have misunderstood your original statement. If we just want
to GET the pod from the apiserver before sending the update, there should
not be any difference between regular and mirror pods. (and we don't need
the actual pod passed the SetPodStatus() at all)

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

yujuhong · 2015-03-25T20:57:42Z

LGTM. shippable is not happy, but tranvis passed. I think it's safe to merge.

Clients should not check conditions, UpdateStatus() is inconsistent

Pod status update should include the ObjectMeta of the pod. This change is required for kubernetes#5738 to merge.

googlebot added the cla: no label Mar 20, 2015

smarterclayton force-pushed the cleanup_clients branch from ced1119 to f1d272a Compare March 20, 2015 22:28

dchen1107 assigned yujuhong Mar 20, 2015

smarterclayton force-pushed the cleanup_clients branch from f1d272a to 26d4a5d Compare March 20, 2015 22:37

mbforbes added cla: yes and removed cla: no labels Mar 21, 2015

googlebot added cla: no and removed cla: yes labels Mar 22, 2015

smarterclayton force-pushed the cleanup_clients branch from 26d4a5d to 54aa5a2 Compare March 22, 2015 23:02

smarterclayton force-pushed the cleanup_clients branch from 54aa5a2 to 0f0d93b Compare March 23, 2015 03:08

smarterclayton force-pushed the cleanup_clients branch 2 times, most recently from 2bf5eef to ba1f581 Compare March 23, 2015 04:19

smarterclayton force-pushed the cleanup_clients branch from ba1f581 to a09e4a4 Compare March 23, 2015 22:08

yujuhong mentioned this pull request Mar 25, 2015

Kubelet: pass the acutal pod for status update #5928

Merged

yujuhong added a commit to yujuhong/kubernetes that referenced this pull request Mar 25, 2015

Kubelet: pass the acutal pod for status update

b4b0bc7

Pod status update should include the ObjectMeta of the pod. This change is required for kubernetes#5738 to merge.

smarterclayton force-pushed the cleanup_clients branch from a09e4a4 to 740a6c7 Compare March 25, 2015 18:51

yujuhong reviewed Mar 25, 2015
View reviewed changes

smarterclayton force-pushed the cleanup_clients branch from 740a6c7 to 8f3d913 Compare March 25, 2015 20:24

Clients should not check conditions, UpdateStatus() is inconsistent

b1ab143

Corrects some of the cargo culting that has crept into the API.

smarterclayton force-pushed the cleanup_clients branch from 8f3d913 to b1ab143 Compare March 25, 2015 20:25

yujuhong reviewed Mar 25, 2015
View reviewed changes

yujuhong added a commit that referenced this pull request Mar 25, 2015

Merge pull request #5738 from smarterclayton/cleanup_clients

6145b3b

Clients should not check conditions, UpdateStatus() is inconsistent

yujuhong merged commit 6145b3b into kubernetes:master Mar 25, 2015

akram pushed a commit to akram/kubernetes that referenced this pull request Apr 7, 2015

Kubelet: pass the acutal pod for status update

b69d7ea

Pod status update should include the ObjectMeta of the pod. This change is required for kubernetes#5738 to merge.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

smarterclayton commented Mar 20, 2015

googlebot commented Mar 20, 2015

smarterclayton commented Mar 20, 2015

yujuhong commented Mar 20, 2015

smarterclayton commented Mar 22, 2015

smarterclayton commented Mar 22, 2015

nikhiljindal commented Mar 23, 2015

smarterclayton commented Mar 23, 2015

nikhiljindal commented Mar 23, 2015

yujuhong commented Mar 24, 2015

smarterclayton commented Mar 24, 2015

smarterclayton commented Mar 25, 2015

yujuhong Mar 25, 2015

smarterclayton Mar 25, 2015

yujuhong Mar 25, 2015

smarterclayton Mar 25, 2015

yujuhong Mar 25, 2015

smarterclayton Mar 25, 2015

yujuhong commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

yujuhong commented Mar 25, 2015

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

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

Conversation

smarterclayton commented Mar 20, 2015

googlebot commented Mar 20, 2015

smarterclayton commented Mar 20, 2015

yujuhong commented Mar 20, 2015

smarterclayton commented Mar 22, 2015

smarterclayton commented Mar 22, 2015

nikhiljindal commented Mar 23, 2015

smarterclayton commented Mar 23, 2015

nikhiljindal commented Mar 23, 2015

yujuhong commented Mar 24, 2015

smarterclayton commented Mar 24, 2015

smarterclayton commented Mar 25, 2015

yujuhong Mar 25, 2015

Choose a reason for hiding this comment

smarterclayton Mar 25, 2015

Choose a reason for hiding this comment

yujuhong Mar 25, 2015

Choose a reason for hiding this comment

smarterclayton Mar 25, 2015

Choose a reason for hiding this comment

yujuhong Mar 25, 2015

Choose a reason for hiding this comment

smarterclayton Mar 25, 2015

Choose a reason for hiding this comment

yujuhong commented Mar 25, 2015

smarterclayton commented Mar 25, 2015

yujuhong commented Mar 25, 2015