Minimal status mutation change #4779

smarterclayton · 2015-02-24T16:53:33Z

PUT /api/v1beta3/namespaces/default/pods/foo/status
{
  "metadata": {...}, // allowed for valid values
  "spec": {}, // ignored
  "status": {...}, // allowed, except for Host
}

Exposes the simplest possibly change. Needs a slight refactoring
to RESTUpdateStrategy to split merging.

Sets the stage for #2726

googlebot · 2015-02-24T16:53:35Z

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.

fgrzadkowski · 2015-02-25T12:21:23Z

pkg/registry/pod/etcd/etcd.go


-		ReturnDeletedObject: true,
-		AfterDelete:         bindings.AfterDelete,
+	statusStore.UpdateStrategy = pod.StatusStrategy


Does it work? StatusStrategy does not implement some methods from RESTUpdateStrategy:

NamespaceScoped() bool

AllowCreateOnUpdate() bool

Forget that! I've missed that you have anonymous field Strategy.

fgrzadkowski · 2015-02-25T13:00:06Z

I have a general question. Before this PR pod status was periodically fetched by pod_cache and stored in memory. It was written to etcd only when someone updated pod via REST API. This PR combined with #4561 (thanks for doing part of the work! :) ) will result in writing status for every update. Is this expected? I imagine this can have a significant effect on performance.

smarterclayton · 2015-02-26T03:50:58Z

This doesn't necessarily make that change, but status updates can definitely ramp up the write load. We need to test the change for sure and get a feel for the cost.

On Feb 25, 2015, at 5:00 AM, Filip Grzadkowski notifications@github.com wrote:

I have a general question. Before this PR pod status was periodically fetched by pod_cache and stored in memory. It was written to etcd only when someone updated pod via REST API. This PR combined with #4561 (thanks for doing part of the work! :) ) will result in writing status for every update. Is this expected? I imagine this can have a significant effect on performance.

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

jdef · 2015-02-26T12:42:00Z

xref #4103

bgrant0607 · 2015-02-27T00:38:36Z

LGTM

bgrant0607 · 2015-02-27T00:42:59Z

@bprashanth: You'll want to use this pattern for replicationcontroller status.

smarterclayton · 2015-02-27T00:49:20Z

I'll be cleaning this up soon, then we'll do the pod-status writer in a second pull.

On Feb 26, 2015, at 7:43 PM, Brian Grant notifications@github.com wrote:

@bprashanth: You'll want to use this pattern for replicationcontroller status.

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

smarterclayton · 2015-02-27T02:52:48Z

I'll do the cleanup and other work in a follow up (since this can standalone)

smarterclayton · 2015-02-27T02:53:25Z

Ready for final review

bgrant0607 · 2015-02-27T04:06:48Z

_output/local/go/src/github.com/GoogleCloudPlatform/kubernetes/pkg/registry/etcd/etcd_test.go:44: assignment count mismatch: 2 = 3

bgrant0607 · 2015-02-27T04:09:40Z

pkg/api/validation/validation.go

+
+	allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)
+
+	// TODO: allow change when bindings are properly decoupled from pods


In what situation would we allow changes? Pods don't get rescheduled, and I'm thinking that the cleanest way to deal with rejection from Kubelet is to consider the pod dead.

If we ever wanted to allow scheduler to mutate status instead of binding, but no other reason. I'll remove the todo

On Feb 26, 2015, at 11:09 PM, Brian Grant notifications@github.com wrote:

In pkg/api/validation/validation.go:

@@ -647,6 +647,23 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
return allErrs
}

+// ValidatePodStatusUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields
+// that cannot be changed.
+func ValidatePodStatusUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {

allErrs := errs.ValidationErrorList{}

allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)

// TODO: allow change when bindings are properly decoupled from pods
In what situation would we allow changes? Pods don't get rescheduled, and I'm thinking that the cleanest way to deal with rejection from Kubelet is to consider the pod dead.

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

I'm also thinking we should get rid of host in Status, and just set the one in Spec -- the scheduler is a finalizer.

So would end users be able to set it?

On Feb 26, 2015, at 11:12 PM, Brian Grant notifications@github.com wrote:

In pkg/api/validation/validation.go:

@@ -647,6 +647,23 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
return allErrs
}

+// ValidatePodStatusUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields
+// that cannot be changed.
+func ValidatePodStatusUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {

allErrs := errs.ValidationErrorList{}

allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)

// TODO: allow change when bindings are properly decoupled from pods
I'm also thinking we should get rid of host in Status, and just set the one in Spec -- the scheduler is a finalizer.

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

Never mind, binding sets it. I keep forgetting....

On Feb 26, 2015, at 11:12 PM, Brian Grant notifications@github.com wrote:

In pkg/api/validation/validation.go:

@@ -647,6 +647,23 @@ func ValidatePodUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {
return allErrs
}

+// ValidatePodStatusUpdate tests to see if the update is legal for an end user to make. newPod is updated with fields
+// that cannot be changed.
+func ValidatePodStatusUpdate(newPod, oldPod *api.Pod) errs.ValidationErrorList {

allErrs := errs.ValidationErrorList{}

allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)

// TODO: allow change when bindings are properly decoupled from pods
I'm also thinking we should get rid of host in Status, and just set the one in Spec -- the scheduler is a finalizer.

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

bgrant0607 · 2015-02-27T04:11:50Z

LGTM, other than the test failure and TODO.

PUT /api/v1beta3/namespaces/default/pods/foo/status { "metadata": {...}, // allowed for valid values "spec": {}, // ignored "status": {...}, // allowed, except for Host } Exposes the simplest possibly change. Needs a slight refactoring to RESTUpdateStrategy to split merging which can be done in a follow up.

smarterclayton · 2015-03-02T03:40:01Z

Fixed comments and failing test.

jdef · 2015-03-02T17:04:16Z

xref d2iq-archive/kubernetes-mesos#73

Minimal status mutation change

fgrzadkowski · 2015-03-04T16:23:53Z

I synced to HEAD, but my cluster does not recognize this new handler. Is this only available in v1beta3?

fgrzadkowski · 2015-03-04T16:37:39Z

It works after kube-down/kube-up. I thought that dev-build-and-push should be enough, but apparently not.

googlebot added the cla: no label Feb 24, 2015

jdef mentioned this pull request Feb 24, 2015

zombie pods can accumulate because assignPods doesn't check for pod existence d2iq-archive/kubernetes-mesos#152

Closed

vishh assigned bgrant0607 Feb 24, 2015

fgrzadkowski reviewed Feb 25, 2015
View reviewed changes

smarterclayton force-pushed the status_endpoints branch from 4b90d1c to dd8672a Compare February 27, 2015 02:51

smarterclayton changed the title ~~WIP - Minimal status mutation change~~ Minimal status mutation change Feb 27, 2015

smarterclayton mentioned this pull request Feb 27, 2015

Properly set object status #2726

Closed

bgrant0607 reviewed Feb 27, 2015
View reviewed changes

smarterclayton force-pushed the status_endpoints branch from dd8672a to 3d29008 Compare March 2, 2015 03:35

bgrant0607 added a commit that referenced this pull request Mar 3, 2015

Merge pull request #4779 from smarterclayton/status_endpoints

fca9fd6

Minimal status mutation change

bgrant0607 merged commit fca9fd6 into kubernetes:master Mar 3, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Minimal status mutation change #4779

Minimal status mutation change #4779

smarterclayton commented Feb 24, 2015

googlebot commented Feb 24, 2015

fgrzadkowski Feb 25, 2015

fgrzadkowski Feb 25, 2015

fgrzadkowski commented Feb 25, 2015

smarterclayton commented Feb 26, 2015

jdef commented Feb 26, 2015

bgrant0607 commented Feb 27, 2015

bgrant0607 commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

bgrant0607 commented Feb 27, 2015

bgrant0607 Feb 27, 2015

smarterclayton Feb 27, 2015

bgrant0607 Feb 27, 2015

smarterclayton Mar 2, 2015

smarterclayton Mar 2, 2015

bgrant0607 commented Feb 27, 2015

smarterclayton commented Mar 2, 2015

jdef commented Mar 2, 2015

fgrzadkowski commented Mar 4, 2015

fgrzadkowski commented Mar 4, 2015


		allErrs = append(allErrs, ValidateObjectMetaUpdate(&oldPod.ObjectMeta, &newPod.ObjectMeta).Prefix("metadata")...)

		// TODO: allow change when bindings are properly decoupled from pods

Minimal status mutation change #4779

Minimal status mutation change #4779

Conversation

smarterclayton commented Feb 24, 2015

googlebot commented Feb 24, 2015

fgrzadkowski Feb 25, 2015

Choose a reason for hiding this comment

fgrzadkowski Feb 25, 2015

Choose a reason for hiding this comment

fgrzadkowski commented Feb 25, 2015

smarterclayton commented Feb 26, 2015

jdef commented Feb 26, 2015

bgrant0607 commented Feb 27, 2015

bgrant0607 commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

smarterclayton commented Feb 27, 2015

bgrant0607 commented Feb 27, 2015

bgrant0607 Feb 27, 2015

Choose a reason for hiding this comment

smarterclayton Feb 27, 2015

Choose a reason for hiding this comment

bgrant0607 Feb 27, 2015

Choose a reason for hiding this comment

smarterclayton Mar 2, 2015

Choose a reason for hiding this comment

smarterclayton Mar 2, 2015

Choose a reason for hiding this comment

bgrant0607 commented Feb 27, 2015

smarterclayton commented Mar 2, 2015

jdef commented Mar 2, 2015

fgrzadkowski commented Mar 4, 2015

fgrzadkowski commented Mar 4, 2015