-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
✨CreateOrPatch #850
✨CreateOrPatch #850
Conversation
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.
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess it doesn't really matter since we're diffing the object after mutation and deriving the patch based on that, though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the difference between
OperationResultUpdatedStatus
andOperationResultUpdated
? does it make sense to expose this distinction inCreateOrUpdate
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm what if both the object and the status were updated? Then we basically need to return two results or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
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? |
Hi @alexeldeib, First, thank you for the review. I appreciate it :)
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 I remember prior to unstructured data literally setting the // 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. |
// 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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@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? |
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/approve
/assign @alexeldeib
for final lgtm
[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:
Approvers can indicate their approval by writing |
return result, err | ||
} | ||
if result == OperationResultUpdated { | ||
result = OperationResultUpdatedStatus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for illustrative purposes:
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 |
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. |
/hold |
My mistake, scanned through the existing comments quickly. Agree with your comment as well though, would be nice to have clarity :) |
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! |
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 |
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. |
@@ -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" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Uhm what if both the object and the status were updated? Then we basically need to return two results or not?
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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will always fail if the status subresource is not enabled or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So update to gracefully handle the resources that lack a status sub resource?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you referring to the if condition above?
Issues go stale after 90d of inactivity. If this issue is safe to close now please do so with Send feedback to sig-testing, kubernetes/test-infra and/or fejta. |
/remove-lifecycle stale |
I'm fine merging this as-is 👍 and improve the comments above later |
Sgtm. |
/hold cancel |
This patch introduces a variation on the
controllerutil.CreateOrUpdate
function namedCreateOrPatch
. 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 thepatch.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 updatedOperationResultUpdatedStatusOnly
- only the status was updatedThere are tests implemented for
CreateOrPatch
that follow those implemented forCreateOrUpdate
with the addition of two, new tests that validate the new result possibilities.