CreateOrUpdate falsely claiming its updating resources

[mcristina422]

While using CreateOrUpdate with resources that get defaulted it can return OperationResultUpdated despite there being no updates. I'm not concerned that the call to Update is happening, the same values get defaulted. But I think returning OperationResultUpdated is wrong since nothing was actually updated. I think that OperationResultUpdated should only be returned when the ResourceVersion is not equal to the original.
https://github.com/kubernetes-sigs/controller-runtime/compare/master...mcristina422:cmpResourseVersion?expand=1#diff-4c146d38c03cf55e321de9ad53af4452R220-R238

I originally came across this issue setting ports on Service similar to #464 (comment)

I recreated the issue in a Deployment with my proposed fix in https://github.com/kubernetes-sigs/controller-runtime/compare/master...mcristina422:cmpResourseVersion?expand=1

@DirectXMan12 do you think my proposal makes sense

[DirectXMan12]

I think that makes sense, off the top of my head. I'm assuming the previous semantics were "did I submit an update", not "did an update actually occur" though. Anyone else have thoughts?

cc @alexeldeib who touched that code last.

/kind feature
/priority awaiting-more-evidence

[mcristina422]

The reason I prefer it not to be just "did I submit an update" is that I use the result to determine if the Owned resource I am updating has been modified. If so, end the reconciliation early and let the update of the Owned object trigger a rereconcile and progress further into the reconcile logic. If this is not intended behavior thats understandable

[alexeldeib]

Seems reasonable. Currently if an object was found remotely (i.e., it's an update) but there is no difference, we no-op locally and return OperationResultNone, nil:

controller-runtime/pkg/controller/controllerutil/controllerutil.go

Lines 216 to 218 in edf7177

	if equality.Semantic.DeepEqual(existing, obj) {
	return OperationResultNone, nil
	}

I think this change makes our behavior consistent so that OperationResultNone + no error means "the object you submitted and the object in the API server were the same". Right now we have an edge case where we tell the user that the blackbox create or update succeeded, but they have to go diff the objects themselves to figure out what happened. I personally think the important semantic is that something changed -- depending on using CreateOrUpdate to submit no-ops to the API server seems like something I'd discourage anyway.

[alexeldeib]

@mcristina422 can you clarify your use case slightly? You have a controller on e.g. the parent objects and when the parent tries to CreateOrUpdate a child object, you want to use the OperationResultUpdated to let the child re-trigger the parent to reconcile?

Without depending on OperationResult here, you should be able to do this by having the parent CreateOrUpdate the child, and then check for a status field on the child object.

[fejta-bot]

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

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

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 rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

[k8s-ci-robot]

@fejta-bot: Closing this issue.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

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

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.