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
🐛 controllerutil.CreateOrPatch doesn't update not empty Status fields if Spec fields are specified #1403
Conversation
Hi @dmvolod. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/ok-to-test |
@@ -309,6 +309,20 @@ func CreateOrPatch(ctx context.Context, c client.Client, obj client.Object, f Mu | |||
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 result == OperationResultUpdated { | |||
// If Status was replaced by Patch before, set it to afterStatus | |||
objectAfterPatch, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&obj) |
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.
Should this be ToUnstructured(obj)
(no address operator) like L282?
I see that L257 makes a deep copy while L282 does not. Is it possible that Patch()
on L303 has manipulated afterStatus
as well? I think a test that calls CreateOrPatch()
with an *unstructured.Unstructured
will confirm.
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.
Probably should use obj
, not its address, thanks @cbandy, fixed. But tests both variants, it seems to be working fine.
… if Spec fields are specified
/milestone v0.9.x |
@vincepri is there any chance to merge this PR? |
/lgtm |
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
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alvaroaleman, dmvolod 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 |
If we would like to patch custom resource with some
Spec
fields andStatus
which are not empty, the controllerutil.CreateOrPatch patchesSpec
only fields, but anyStatus
keep unchanged,result
returnsOperationResultUpdatedStatus
which means that bothSpec
andStatus
where updatedThe reason why this happens is that the original
Status
of the object returned when thePatch
procedure is exited and, as a result, noStatus
fields are updatedFixes #1392