✨CreateOrPatch #850

akutz · 2020-03-10T20:51:19Z

This patch introduces a variation on the controllerutil.CreateOrUpdate function named CreateOrPatch. Instead of issuing update calls, the new function uses a patch to perform a more surgical update to the remote data. Additionally, the implementation relies on logic similar to the patch.Helper mechanics from Cluster API. The resource is converted to unstructured data first in order to patch the resource and any potential status separately.

Two new OperationResult values were added:

OperationResultUpdatedStatus - the resource and its status were updated
OperationResultUpdatedStatusOnly - only the status was updated

There are tests implemented for CreateOrPatch that follow those implemented for CreateOrUpdate with the addition of two, new tests that validate the new result possibilities.

This patch introduces a variation on the controllerutil.CreateOrUpdate function named CreateOrPatch. Instead of issuing update calls, the new function uses a patch to perform a more surgical update to the remote data. Additionally, the implementation relies on logic similar to the PatchHelper mechanics in the Cluster API util/patch package. The resource is converted to unstructured data first in order to patch the resource and any potential status separately. Two new OperationResult values were added: 1. OperationResultUpdatedStatus - the resource and its status were updated 2. OperationResultUpdatedStatusOnly - only the status was updated

This patch adds a test to assert that the resource passed into CreateOrPatch is updated as part of the CreateOrPatch operation.

pkg/controller/controllerutil/controllerutil.go

alexeldeib · 2020-03-14T21:16:37Z

pkg/controller/controllerutil/controllerutil.go

@@ -180,6 +182,10 @@ const ( // They should complete the sentence "Deployment default/foo has been ..
 	OperationResultCreated OperationResult = "created"
 	// OperationResultUpdated means that an existing resource is updated
 	OperationResultUpdated OperationResult = "updated"
+	// OperationResultUpdatedStatus means that an existing resource and its status is updated
+	OperationResultUpdatedStatus OperationResult = "updatedStatus"


These are currently only used for patch operations. The naming may be confusing since an update sets the whole object, while a patch does not. Consider OperationResultPatched + OperationResultPatchedStatus?

I guess it doesn't really matter since we're diffing the object after mutation and deriving the patch based on that, though.

Hi @alexeldeib,

These are currently only used for patch operations. The naming may be confusing since an update sets the whole object, while a patch does not. Consider OperationResultPatched + OperationResultPatchedStatus?

The reason for the current approach is to be consistent with the existing constants.

what's the difference between OperationResultUpdatedStatus and OperationResultUpdated? does it make sense to expose this distinction in CreateOrUpdate as well? If not, does it make sense to remove one?

what's the difference between OperationResultUpdatedStatus and OperationResultUpdated? does it make sense to expose this distinction in CreateOrUpdate as well? If not, does it make sense to remove one?

Hi @alexeldeib,

I suppose if we do not create Patched variants, then there is no real distinction between OperationResultUpdated and OperationResultUpdatedStatus. Good catch.

To me Updated means the result, not the method used to achieve it. This is another reason I did not create the variants. But I have no real strong opinions here.

What is important to me is to have a result that indicates the status sub-resource was updated, but not the primary resource itself.

Beyond that, I do not have any real strong thoughts on how the result codes are indicated.

(edit: weird timing -- you managed to reply before I posted this and saw it after :) )

my initial reaction was because of a race condition like this:

thread a - createOrPatch

get object

mutate and extract patch

send patch to API server

thread b - racer

get obj

mutate

send to apiserver

semantically I would want to tell the user their patch was successfully applied but I cannot guarantee that it was applied against the same object they provided (i.e., b3 occurred before a3). This is a slightly less strong guarantee than OperationResultUpdated (which would imply either a3 or b3 would fail), so I would have matched the HTTP methods and reached for OperationResultPatched.

To me OperationResultUpdatedStatus means what I think OperationResultUpdatedStatusOnly is supposed to mean -- that a patch was applied but only the status was updated. Neither of them actually means an update operation was what was executed, vs the naming of OperationResultCreated, OperationResultUpdated

Uhm what if both the object and the status were updated? Then we basically need to return two results or not?

If both were updated, then OperationResultUpdated would imply both were updated.

I didn't change the names of the results to Patched because I was trying to preserve compatibility with the existing result values. I'm happy to adjust the names.

But the fact that the CreateOrPatch tries to update the Status whereas CreateOrUpdate does not is just an implementation detail and not inherent to doing patch vs update or am I missing something?

May I ask for the reason you need to differentiate if the status resource was updated? I am asking because I believe that whatever we do here should also be added to CreateOrUpdate and I know ppl use the result sometimes to determine how to proceed. If we extend this we might break them which is why maybe returning a []result may be better (but nor completely sure).

alexeldeib · 2020-03-14T21:19:27Z

Separating out the status diverges from the existing CreateOrUpdate implementation. I'm not opposed to differentiating spec/status updates, but I think it would be cleaner to be consistent.

WDYT? Do you think it makes sense to add that logic in CreateOrUpdate too?

akutz · 2020-03-14T22:31:45Z

Hi @alexeldeib,

First, thank you for the review. I appreciate it :)

Separating out the status diverges from the existing CreateOrUpdate implementation. I'm not opposed to differentiating spec/status updates, but I think it would be cleaner to be consistent.

WDYT? Do you think it makes sense to add that logic in CreateOrUpdate too?

I am going to ping @detiber and @vincepri to help add context on the reason for the current approach, but if I recall correctly, it is because we all ran into trouble last year when issuing Patch against a resource whose status was not yet present. That is why we ended up removing the status pre-patch and then adding it back for the secondary status patch call.

I remember prior to unstructured data literally setting the .status = Status{} (or whatever the resource's status struct was) to zero it out. Ah, here is what I mean:

	// Make sure the status isn't part of the JSON patch.
	newStatus := c.Cluster.Status.DeepCopy()
	c.Cluster.Status = clusterv1.ClusterStatus{}
	c.ClusterCopy.Status.DeepCopyInto(&c.Cluster.Status)

Anyway, that is why the status is handled the way it is.

negz · 2020-03-18T01:34:18Z

pkg/controller/controllerutil/controllerutil.go

+// The MutateFn is called regardless of creating or updating an object.
+//
+// It returns the executed operation and an error.
+func CreateOrPatch(ctx context.Context, c client.Client, obj runtime.Object, f MutateFn) (OperationResult, error) {


https://github.com/crossplane/crossplane-runtime/blob/c092201a/pkg/resource/resource.go#L274

We have a similar function that we use in various Crossplane controllers. We've found that passing a function that takes the existing and the desired resource can be useful, for example to ensure that the existing resource has the same controller reference as the desired resource before we mutate it.

vincepri · 2020-04-24T14:40:02Z

@alexeldeib @akutz What's the status of this PR? Regarding having Status patched, it's a really nice to have, not sure if it's worth to do it separately though.

akutz · 2020-04-28T18:43:12Z

@alexeldeib @akutz What's the status of this PR? Regarding having Status patched, it's a really nice to have, not sure if it's worth to do it separately though.

Hi @vincepri,

As far as I am concerned this PR is fine as-is. There was a healthy discussion above, but nothing that resulted in me wanting to make any major changes to the PR. I do agree that perhaps the operation results could be cleaned up, but I am also unsure to what. Do you have any feedback here?

akutz · 2020-04-28T18:45:24Z

Hi @vincepri,

I had pinged both you and @detiber for your feedback on March 14th #850 (comment) thoughts on this? I remember the reason we separated the status was because at the time the Patch operation could not patch the status with the primary resource as it was a sub-resource. IMO to separate it into a distinct function is orthogonal to the behavior of CreateOrUpdate.

vincepri

/approve
/assign @alexeldeib
for final lgtm

k8s-ci-robot · 2020-06-05T21:50:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: akutz, vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [vincepri]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

alexeldeib · 2020-06-05T22:14:19Z

pkg/controller/controllerutil/controllerutil.go

+			return result, err
+		}
+		if result == OperationResultUpdated {
+			result = OperationResultUpdatedStatus


@vincepri fyi since this was one we wanted more 👀 on

IMO my comment in https://github.com/kubernetes-sigs/controller-runtime/pull/850/files#r392628297 is still accurate, either OperationResultUpdatedStatus should be removed or there should be real differentiation between OperationResultUpdatedStatus and OperationResultUpdated.

Yeah that makes sense, I'd suggest to just keep one for now and revisit in a future version. +1 removing the updated status one

alexeldeib · 2020-06-05T22:21:48Z

pkg/controller/controllerutil/controllerutil.go

+	result := OperationResultNone
+
+	if !reflect.DeepEqual(before, after) {
+		// Only issue a Patch if the before and after resources (minus status) differ
+		if err := c.Patch(ctx, obj, objPatch); err != nil {
+			return result, err
+		}
+		result = OperationResultUpdated
+	}
+
+	if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {
+		// Only issue a Status Patch if the resource has a status and the beforeStatus
+		// and afterStatus copies differ
+		if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {
+			return result, err
+		}
+		if result == OperationResultUpdated {
+			result = OperationResultUpdatedStatus
+		} else {
+			result = OperationResultUpdatedStatusOnly
+		}
+	}
+
+	return result, nil


for illustrative purposes:

Suggested change

result := OperationResultNone

if !reflect.DeepEqual(before, after) {

// Only issue a Patch if the before and after resources (minus status) differ

if err := c.Patch(ctx, obj, objPatch); err != nil {

return result, err

}

result = OperationResultUpdated

}

if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {

// Only issue a Status Patch if the resource has a status and the beforeStatus

// and afterStatus copies differ

if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {

return result, err

}

if result == OperationResultUpdated {

result = OperationResultUpdatedStatus

} else {

result = OperationResultUpdatedStatusOnly

}

}

return result, nil

result := OperationResultNone

if !reflect.DeepEqual(before, after) {

// Only issue a Patch if the before and after resources (minus status) differ

if err := c.Patch(ctx, obj, objPatch); err != nil {

return result, err

}

result = OperationResultUpdatedSpec

}

if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {

// Only issue a Status Patch if the resource has a status and the beforeStatus

// and afterStatus copies differ

if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {

return result, err

}

if result == OperationResultUpdatedSpec {

result = OperationResultUpdated

} else {

result = OperationResultUpdatedStatus

}

}

return result, nil

alexeldeib · 2020-06-05T22:22:47Z

If no one else is bothered by the naming, it's fine with me. I don't think users should depend on the return values here anyway, but if we provide it publicly we should make it as correct as possible.

akutz · 2020-06-05T22:38:12Z

If no one else is bothered by the naming, it's fine with me. I don't think users should depend on the return values here anyway, but if we provide it publicly we should make it as correct as possible.

Hi @alexeldeib,

I am totally fine changing the naming. I pinged Vince because of the order logic question. If you would like to suggest the preferred naming, I'll update the PR.

akutz · 2020-06-05T22:38:39Z

/hold

alexeldeib · 2020-06-05T22:48:24Z

My mistake, scanned through the existing comments quickly. Agree with your comment as well though, would be nice to have clarity :)

akutz · 2020-06-22T17:06:00Z

Hi @alexeldeib,

Is there a time when we could hop on a Zoom and discuss the result values in a high-bandwidth medium? I want to unblock this PR, and I am still unsure what to do about those return values. Thanks!

alexeldeib · 2020-06-22T17:52:14Z

TBH, I'm inclined to merge as-is and fix forward. I think this is useful functionality and my only objections are nits. For the CreateOrUpdate case, we can add the same differentiation if we find it's actually useful in practice.

@alvaroaleman @DirectXMan12 any thoughts from either of you here?

adding lgtm based on ^ leaving hold for one of those fine folks (perhaps with lazy consensus?). i'll be free this afternoon after 2ish pacific and tomorrow in the middle of the day from 11:30-1ish if you still want to chat about it, but I'd rather get us unstuck

/lgtm

akutz · 2020-06-22T19:25:38Z

Hi @alexeldeib,

If everyone is happy with this as-is, then I'm more than happy to remove the hold (since I am the one that added it). It already has two lgtms, so I'm not sure we need to wait any longer if the plan is to iterate anyway.

To be clear, I'm also fine waiting. I was just trying to push future discussion into additional iterations of the logic since we all agree it's a desirable feature.

alvaroaleman · 2020-06-22T19:33:11Z

pkg/controller/controllerutil/controllerutil.go

@@ -180,6 +182,10 @@ const ( // They should complete the sentence "Deployment default/foo has been ..
 	OperationResultCreated OperationResult = "created"
 	// OperationResultUpdated means that an existing resource is updated
 	OperationResultUpdated OperationResult = "updated"
+	// OperationResultUpdatedStatus means that an existing resource and its status is updated
+	OperationResultUpdatedStatus OperationResult = "updatedStatus"


Uhm what if both the object and the status were updated? Then we basically need to return two results or not?

alvaroaleman · 2020-06-22T19:33:36Z

pkg/controller/controllerutil/controllerutil.go

+	if (hasBeforeStatus || hasAfterStatus) && !reflect.DeepEqual(beforeStatus, afterStatus) {
+		// Only issue a Status Patch if the resource has a status and the beforeStatus
+		// and afterStatus copies differ
+		if err := c.Status().Patch(ctx, obj, statusPatch); err != nil {


This will always fail if the status subresource is not enabled or not?

So update to gracefully handle the resources that lack a status sub resource?

So update to gracefully handle the resources that lack a status sub resource?

Sorry I don't understand what you mean by this comment?

We could probably handle this by just optimistically patching status and handling the error we get in case its not enabled (I believe its a 404 but might be wrong)?

Are you referring to the if condition above?

fejta-bot · 2020-09-20T20:10:22Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

akutz · 2020-09-25T13:56:30Z

/remove-lifecycle stale

vincepri · 2020-09-25T14:24:03Z

I'm fine merging this as-is 👍 and improve the comments above later

akutz · 2020-09-25T15:21:23Z

Sgtm.

akutz · 2020-09-25T15:21:45Z

/hold cancel

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Mar 10, 2020

k8s-ci-robot requested review from DirectXMan12 and droot March 10, 2020 20:52

Add test to assert local copy updated post patch

196055a

This patch adds a test to assert that the resource passed into CreateOrPatch is updated as part of the CreateOrPatch operation.

alexeldeib reviewed Mar 14, 2020

View reviewed changes

pkg/controller/controllerutil/controllerutil.go Show resolved Hide resolved

alexeldeib reviewed Mar 14, 2020

View reviewed changes

pkg/controller/controllerutil/controllerutil.go Show resolved Hide resolved

alexeldeib reviewed Mar 14, 2020

View reviewed changes

negz reviewed Mar 18, 2020

View reviewed changes

vincepri approved these changes Jun 5, 2020

View reviewed changes

k8s-ci-robot assigned alexeldeib Jun 5, 2020

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 5, 2020

vincepri added this to the v0.6.x milestone Jun 5, 2020

alexeldeib reviewed Jun 5, 2020

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 5, 2020

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 22, 2020

alvaroaleman reviewed Jun 22, 2020

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Sep 20, 2020

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2020

k8s-ci-robot merged commit fdc6658 into kubernetes-sigs:master Sep 25, 2020

This was referenced Dec 13, 2020

migrate to controller-runtime v0.7.0 metal-stack/duros-controller#6

Closed

migrate to controller-runtime v0.7.0 fi-ts/postgreslet#27

Closed

timebertt mentioned this pull request Jan 29, 2021

☂️ -Issue for graduating CachedRuntimeClients to beta gardener/gardener#2822

Closed

12 tasks

✨CreateOrPatch #850

✨CreateOrPatch #850

Conversation

akutz commented Mar 10, 2020

alexeldeib Mar 14, 2020 • edited

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

akutz Mar 14, 2020 • edited

Choose a reason for hiding this comment

alexeldeib Mar 14, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akutz Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Mar 14, 2020

akutz commented Mar 14, 2020 • edited

Choose a reason for hiding this comment

vincepri commented Apr 24, 2020

akutz commented Apr 28, 2020

akutz commented Apr 28, 2020 • edited

vincepri left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Jun 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexeldeib commented Jun 5, 2020

akutz commented Jun 5, 2020

akutz commented Jun 5, 2020

alexeldeib commented Jun 5, 2020

akutz commented Jun 22, 2020

alexeldeib commented Jun 22, 2020 • edited

akutz commented Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alvaroaleman Jun 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fejta-bot commented Sep 20, 2020

akutz commented Sep 25, 2020

vincepri commented Sep 25, 2020

akutz commented Sep 25, 2020

akutz commented Sep 25, 2020

alexeldeib Mar 14, 2020 •

edited

akutz Mar 14, 2020 •

edited

alexeldeib Mar 14, 2020 •

edited

akutz Jun 22, 2020 •

edited

akutz commented Mar 14, 2020 •

edited

akutz commented Apr 28, 2020 •

edited

alexeldeib commented Jun 22, 2020 •

edited

akutz commented Jun 22, 2020 •

edited

alvaroaleman Jun 22, 2020 •

edited