Allow update without resource version #10074

nikhiljindal · 2015-06-19T01:22:38Z

Updating code to populate the user given object with the resource version of the latest etcd object, if the user given object does not have a resource version.

nikhiljindal · 2015-06-19T01:23:54Z

pkg/tools/etcd_helper.go

 // Pass an EtcdUpdateFunc to EtcdHelper.GuaranteedUpdate to make an etcd update that is guaranteed to succeed.
 // See the comment for GuaranteedUpdate for more detail.
-type EtcdUpdateFunc func(input runtime.Object, res ResponseMeta) (output runtime.Object, ttl *uint64, err error)
+type EtcdUpdateFunc func(input runtime.Object, node *etcd.Node) (output runtime.Object, ttl *uint64, err error)


Replacing ResponseMeta with etcd.Node should be fine as per #8724 (comment)

But we didn't want to do that for coupling reasons....

Dan convinced me it was a bad idea and I agreed

Conversation lost to the mists of github history, but basically: introduces coupling and encourages bad patterns - generic etcd should be generally isolated from exact etcd objects for test ability, and also callers should not be encouraged to accidentally (or intentionally) start using the node value directly

We could add ModifiedIndex to ResponseMeta, then (named ResourceVersion?).

Yes. I can add node.Expiration and node.ModifiedIndex to ResponseMeta and then change e.Helper.Versioner.UpdateObject() to take modified index and expiration instead of the whole node, if this is a concern. Based on that discussion, I thought @smarterclayton had convinced @lavalamp that its fine to pass node, but looks like thats not the case.

Yeah the other way around. I fought but then conceded because he was right - pass what you need, not what you think you'll want. :)

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

// Pass an EtcdUpdateFunc to EtcdHelper.GuaranteedUpdate to make an etcd
update that is guaranteed to succeed.
// See the comment for GuaranteedUpdate for more detail.
-type EtcdUpdateFunc func(input runtime.Object, res ResponseMeta) (output
runtime.Object, ttl *uint64, err error)
+type EtcdUpdateFunc func(input runtime.Object, node *etcd.Node) (output
runtime.Object, ttl *uint64, err error)

Yes. I can add node.Expiration and node.ModifiedIndex to ResponseMeta and
then change e.Helper.Versioner.UpdateObject() to take modified index and
expiration instead of the whole node, if this is a concern. Based on that
discussion, I thought @smarterclayton had convinced @lavalamp that its fine
to pass node, but looks like thats not the case.

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

Please keep response meta. You don't need to change this at all, the latest resource version is embedded in the object passed to the SimpleEtcdUpdateFunc.

Reverted that change, so that this still takes a ResponseMeta and not etcd.Node

nikhiljindal · 2015-06-19T01:24:22Z

cc @bgrant0607 @smarterclayton @lavalamp

smarterclayton · 2015-06-19T01:31:26Z

pkg/registry/generic/etcd/etcd.go

@@ -271,10 +272,21 @@ func (e *Etcd) Update(ctx api.Context, obj runtime.Object) (runtime.Object, bool
 	if err != nil {
 		return nil, false, err
 	}
+	// If the object specified by the user does not have a resource version,


I would want this feature to be optional on a type by type basis - opt in. We have a few objects where I'd want to discourage this pattern.

I don't think that we ever want to encourage this pattern, but it needs to exist, and I'd prefer that it be implemented in the server rather than via a RMW loop in the client. We could create an UpdateOption, but that would be more work, and then we'd need to generate a better error message for the case where resourceVersion == 0 but no forceUpdate option was specified, and we might still need to do something urgently to mitigate false conflicts with status updates.

Sorry, I meant specifically in generic.Etcd as an implementor you have to enable this function. Prevents it being enabled by accident on new resources - I'd rather this be a deliberate decision.

I can add a AllowUpdateWithoutResourceVersion() like AllowCreateOnUpdate() (https://github.com/GoogleCloudPlatform/kubernetes/blob/5520386b180d3ddc4fa7b7dfe6f52642cc0c25f3/pkg/api/rest/update.go#L35) in RESTUpdateStrategy?

Yeah, something like that. That allows us to add something to resttest that you indeed fail if that is false.

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

@@ -271,10 +272,21 @@ func (e *Etcd) Update(ctx api.Context, obj
runtime.Object) (runtime.Object, bool
if err != nil {
return nil, false, err
}

// If the object specified by the user does not have a resource version,

I can add a AllowUpdateWithoutResourceVersion() like AllowCreateOnUpdate()
(https://github.com/GoogleCloudPlatform/kubernetes/blob/5520386b180d3ddc4fa7b7dfe6f52642cc0c25f3/pkg/api/rest/update.go#L35)
in RESTUpdateStrategy?

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

Added an AllowUnconditionalUpdate() and using that here

k8s-bot · 2015-06-19T01:39:50Z

GCE e2e build/test passed for commit 13ca0cf73f20b926021e370132e84604deed81b6.

bgrant0607 · 2015-06-19T05:11:46Z

pkg/registry/generic/etcd/etcd.go

+	// If the object specified by the user does not have a resource version,
+	// then populete it with the latest version.
+	// Else, we check that the version specified by the user matches the version of latest etcd object.
+	useLatestResourceVersion := false


I would call this unsafeUpdate, unconditionalUpdate, forceUpdate, or something like that.

allowUnsafeUpdates?

er, reading more closely-- unconditionalUpdate or clobber might be better.

renamed to allowUnconditionalUpdate

lavalamp · 2015-06-19T22:59:52Z

pkg/registry/generic/etcd/etcd.go

+	// If AllowUnconditionalUpdate() is true and the object specified by the user does not have a resource version,
+	// then we populate it with the latest version.
+	// Else, we check that the version specified by the user matches the version of latest etcd object.
+	allowUnconditionalUpdate := false


Suggest:

doUnconditionalUpdate := resourceVersion == 0 && e.UpdateStrategy.AllowUnconditionalUpdate()

You're not just allowing it, you're doing it...

lavalamp · 2015-06-19T23:06:08Z

Just a few nits, otherwise looks good.

nikhiljindal · 2015-06-20T01:29:33Z

Updated code as per comments.
@lavalamp PTAL.

k8s-bot · 2015-06-20T03:05:49Z

GCE e2e build/test passed for commit ed56655e3a10992b63b4361ead24885c869c2352.

lavalamp · 2015-06-22T18:27:15Z

LGTM

nikhiljindal · 2015-06-22T18:47:12Z

Thanks! Added a test case for unconditional update to pkg/registry/generic/etcd/etcd_test.go.

k8s-bot · 2015-06-22T19:18:23Z

GCE e2e build/test passed for commit 5aeb1bc558aaacdb76513b2e81c1815308a6404f.

j3ffml · 2015-06-22T23:35:18Z

A test that was causing Shippable timeouts is disabled at head. Should be green after rebase.

nikhiljindal · 2015-06-23T07:16:38Z

Rebased

k8s-bot · 2015-06-23T07:34:50Z

GCE e2e build/test passed for commit 221ae4d.

j3ffml · 2015-06-23T16:23:04Z

Per new merge policy, please add a brief risk assessment.

nikhiljindal · 2015-06-23T16:56:51Z

Risk assessment: medium-low.
This change only affects the UPDATE operation on apiserver.

nikhiljindal · 2015-06-24T00:49:49Z

@brendandburns or @davidopp for another LGTM.

davidopp · 2015-06-24T05:12:20Z

pkg/registry/generic/etcd/etcd.go

+			// Update the object's resource version to match the latest etcd object's resource version.
+			err = e.Helper.Versioner.UpdateObject(obj, res.Expiration, res.ResourceVersion)
+			if err != nil {
+				return nil, nil, err


is there some reason why you don't return obj, nil, err here?

oh sorry, never mind, this is an error case.

davidopp · 2015-06-24T05:13:10Z

LGTM

Allow update without resource version

bgrant0607 · 2015-06-24T18:08:47Z

Thanks. It looks like the API documentation already assumed we were doing this:
https://github.com/GoogleCloudPlatform/kubernetes/blob/master/docs/api-conventions.md#concurrency-control-and-consistency

googlebot added the cla: yes label Jun 19, 2015

nikhiljindal reviewed Jun 19, 2015
View reviewed changes

smarterclayton reviewed Jun 19, 2015
View reviewed changes

bgrant0607 self-assigned this Jun 19, 2015

bgrant0607 added this to the v1.0 milestone Jun 19, 2015

bgrant0607 reviewed Jun 19, 2015
View reviewed changes

nikhiljindal force-pushed the retryUpdate branch from 13ca0cf to 0916d31 Compare June 19, 2015 22:54

bgrant0607 assigned lavalamp and unassigned bgrant0607 Jun 19, 2015

lavalamp reviewed Jun 19, 2015
View reviewed changes

nikhiljindal force-pushed the retryUpdate branch 2 times, most recently from e13bafd to ed56655 Compare June 20, 2015 01:28

lavalamp added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2015

nikhiljindal force-pushed the retryUpdate branch from ed56655 to 5aeb1bc Compare June 22, 2015 18:45

nikhiljindal closed this Jun 22, 2015

nikhiljindal reopened this Jun 22, 2015

nikhiljindal mentioned this pull request Jun 22, 2015

Unable to set node unschedulable #9873

Closed

Allow update without resource version

221ae4d

nikhiljindal force-pushed the retryUpdate branch from 5aeb1bc to 221ae4d Compare June 23, 2015 07:16

nikhiljindal closed this Jun 23, 2015

nikhiljindal reopened this Jun 23, 2015

j3ffml assigned bgrant0607 and unassigned lavalamp Jun 23, 2015

davidopp reviewed Jun 24, 2015
View reviewed changes

davidopp added the ok-to-merge label Jun 24, 2015

mbforbes added a commit that referenced this pull request Jun 24, 2015

Merge pull request #10074 from nikhiljindal/retryUpdate

11f9fd1

Allow update without resource version

mbforbes merged commit 11f9fd1 into kubernetes:master Jun 24, 2015

nikhiljindal mentioned this pull request Jun 26, 2015

Clean up PUT semantics #2114

Closed

nikhiljindal mentioned this pull request Feb 16, 2016

AllowUnconditionalUpdate is very frightening #21330

Closed

Allow update without resource version #10074

Allow update without resource version #10074

Conversation

nikhiljindal commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nikhiljindal commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-bot commented Jun 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lavalamp commented Jun 19, 2015

nikhiljindal commented Jun 20, 2015

k8s-bot commented Jun 20, 2015

lavalamp commented Jun 22, 2015

nikhiljindal commented Jun 22, 2015

k8s-bot commented Jun 22, 2015

j3ffml commented Jun 22, 2015

nikhiljindal commented Jun 23, 2015

k8s-bot commented Jun 23, 2015

j3ffml commented Jun 23, 2015

nikhiljindal commented Jun 23, 2015

nikhiljindal commented Jun 24, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davidopp commented Jun 24, 2015

bgrant0607 commented Jun 24, 2015