🐛 Reset resource version if fake client Create call failed

[muvaf]

Fixes #918

This breaks the code that relies on first checking if an object already exists via the error returned from Create and then run Patch. Since Patch call of fake client does an Update operation, which checks resource version match, it returns object was changed error in these cases because the local object now has resource version 1 but what storage has is resource version 0 since the client was initialized with that object.

[k8s-ci-robot]

Welcome @muvaf!

It looks like this is your first PR to kubernetes-sigs/controller-runtime 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/controller-runtime has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

[k8s-ci-robot]

Hi @muvaf. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

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.

[djzager]

As I understand the scenario being described:

• user calls NewFakeClient([]runtime.Object{myObject}), it's unlikely that the user would set the resource version here so the stored version is ""
• user calls Create(gvr, myObject, myObject.Namespace), this returns an error because myObject already exists
• user calls Update(gvr, myObject, myObject.Namespace), this fails because https://github.com/kubernetes-sigs/controller-runtime/blob/master/pkg/client/fake/client.go#L121-L123

Assuming that this change addresses this scenario, what happens if you started the client with an object, made a series of Update(...) calls, called Create(...) which would fail because the object already exists? This change would not fix a subsequent call to Update(...) in this case as the stored objectMeta.ResourceVersion != "".

My personal feeling is that there is not a guarantee to be made in the case that you call Create(...) that a subsequent call to Update(...) with the same object should work. Namely, I believe if you wish to call Update(...) you should first Get(...), modify that object, and then call Update(...). WDYT?

[muvaf]

Assuming that this change addresses this scenario, what happens if you started the client with an object, made a series of Update(...) calls, called Create(...) which would fail because the object already exists? This change would not fix a subsequent call to Update(...) in this case as the stored objectMeta.ResourceVersion != "".

Each Update increases the resource version and updates the local object you submitted, so Create will fail saying resource version cannot be set in create and Updates should work since the local object is same as what's stored.

My personal feeling is that there is not a guarantee to be made in the case that you call Create(...) that a subsequent call to Update(...) with the same object should work. Namely, I believe if you wish to call Update(...) you should first Get(...), modify that object, and then call Update(...). WDYT?

Actually, the problem is not about Update. In my case, Create is called and if the resource exists I call Patch, which normally doesn't require you to know resource version at all. However, fake client uses Update under the hood when Patch is called. What it does is bascially applying the patch locally and then call Update and in that process resourceVersion suddenly becomes important. I think this process is problematic as well but this PR aims to fix only Create to match the existing real client behavior, i.e. object should not be modified if the call fails.

[djzager]

Thank you for the explanation.

Actually, the problem is not about Update. In my case, Create is called and if the resource exists I call Patch, which normally doesn't require you to know resource version at all. However, fake client uses Update under the hood when Patch is called.

Looking at the current implementation of Patch I see nothing to indicate that Update is being used "under the hood". Is it possible that you are using an older version of controller runtime?

I think this process is problematic as well but this PR aims to fix only Create to match the existing real client behavior, i.e. object should not be modified if the call fails.

I am a little lost here, my apologies, but this PR is modifying the object when Create fails (ie. accessor.SetResourceVersion("")).

[muvaf]

No problem!

Looking at the current implementation of Patch I see nothing to indicate that Update is being used "under the hood". Is it possible that you are using an older version of controller runtime?

Here testing.ObjectReaction is called and testing.NewPatchAction is supplied in the next call. When you look at the implementation of testing.ObjectReaction (client-go v0.18.0 is used by controller-runtime master.) you'll see that case PatchActionImpl does the patch operation via json data and call Update.

I am a little lost here, my apologies, but this PR is modifying the object when Create fails (ie. accessor.SetResourceVersion("")).

If you look at a few lines before the actual Create call, it requires resourceVersion to be empty even to make it there. So, we know that it was "", then Create changes it to "1". If call fails, it stays "1". What this PR does is to revert it back to "" since the call failed.

[djzager]

Now I got it. Thanks.

/lgtm

[shawn-hurley]

/ok-to-test

[muvaf]

It seems to be a flake.

/retest

[muvaf]

@shawn-hurley thanks for the test! Seems like this is ready to merge.

[djzager]

@muvaf One more thing. I think it would be appropriate for you to add a test to pkg/client/fake/client_test.go that verifies the resourceVersion is unchanged after failed Create.

[muvaf]

I think it would be appropriate for you to add a test to pkg/client/fake/client_test.go that verifies the resourceVersion is unchanged after failed Create.

@djzager @shawn-hurley Added a test that verifies the submitted object is not changed after a failed Create call. I've validated that the test is valid by commenting out the added line accessor.SetResourceVersion("") and seeing the test fail.

[alvaroaleman]

/lgtm
/assign @vincepri

[djzager]

Thanks for the fix @muvaf

[shawn-hurley]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: muvaf, shawn-hurley

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 [shawn-hurley]

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

[alvaroaleman]

/retest