The fake client does not support concurrent patching of ConfigMap resources

[akutz]

I was working on the PR https://github.com/vmware-tanzu/vm-operator/pull/673 when a test (there are concurrent attempts to update the ConfigMap in pkg/util/kube/storage_test.go) that I thought should have worked, unexpectedly failed with a 409 (conflict):

$ ginkgo --json-report ./ginkgo.report -focus "EncryptedStorageClass when there are concurrent attempts to update the ConfigMap"   -r
[1727280290] Kube Util Test Suite - 1/21 specs SSSS
------------------------------
• [FAILED] [0.003 seconds]
EncryptedStorageClass when there are concurrent attempts to update the ConfigMap [It] more than one patch should succeed
/Users/akutz/Projects/vmop/vmop/pkg/util/kube/storage_test.go:130

  [FAILED] StorageClass.name=0
  Expected success, but got an error:
      <*errors.StatusError | 0x140003c0320>: 
      Operation cannot be fulfilled on configmaps "encrypted-storage-class-names": object was modified
      {
          ErrStatus: {
              TypeMeta: {Kind: "", APIVersion: ""},
              ListMeta: {
                  SelfLink: "",
                  ResourceVersion: "",
                  Continue: "",
                  RemainingItemCount: nil,
              },
              Status: "Failure",
              Message: "Operation cannot be fulfilled on configmaps \"encrypted-storage-class-names\": object was modified",
              Reason: "Conflict",
              Details: {
                  Name: "encrypted-storage-class-names",
                  Group: "",
                  Kind: "configmaps",
                  UID: "",
                  Causes: nil,
                  RetryAfterSeconds: 0,
              },
              Code: 409,
          },
      }
  In [It] at: /Users/akutz/Projects/vmop/vmop/pkg/util/kube/storage_test.go:151 @ 09/25/24 11:04:52.802
------------------------------
SSSSSSSSSSSSSSSS

Summarizing 1 Failure:
  [FAIL] EncryptedStorageClass when there are concurrent attempts to update the ConfigMap [It] more than one patch should succeed
  /Users/akutz/Projects/vmop/vmop/pkg/util/kube/storage_test.go:151

Ran 1 of 21 Specs in 0.004 seconds
FAIL! -- 0 Passed | 1 Failed | 0 Pending | 20 Skipped
--- FAIL: TestKube (0.01s)
FAIL

The logic (the MarkEncryptedStorageClass function in pkg/util/kube/storage.go) seemed sound, and @sbueringer thought so too. He and I both also thought it should be possible to patch a ConfigMap concurrently at different keys.

After he tried it locally, @sbueringer suggested it may be the fake client. Sure enough, once I switched to envtest, it worked a treat.

It would be great if this did work in the fake client, or else if the fake client returned an error indicating specifically that it does not support non-optimistic patches.

[alvaroaleman]

He and I both also thought it should be possible to patch a ConfigMap concurrently at different keys

So the issue seems to be that in the fake client, patch operations by default require optimistic concurrency?

[sbueringer]

The scenario is that multiple clients are writing different keys concurrently without optimistic locking (without sending the resourceVersion).

The fake client returns a conflict error even though it shouldn't (with envtest it just works).

Another way to look at this:

• Concurrent writes without optimistic locking return conflict errors while they shouldn't
• Concurrent writes with optimistic locking return conflict errors as they should (I assume, I didn't test this case)

So yes, if you want to use something that behaves the same with the fake client and in reality you have to use optimistic locking (I would move to envtest instead of changing my prod code through 😃)

[alvaroaleman]

Makes sense, definitely something we should fix. Interesting that no one noticed before. I'd prefer to do it in the next minor and not in patch releases, because historically, pretty much any change to the fake client broke someones unit tests

[sbueringer]

Sounds good. We're definitely not in a rush, and nobody else noticed (or at least reported) it :)

[akutz]

So yes, if you want to use something that behaves the same with the fake client and in reality you have to use optimistic locking

I would argue this isn't actually a solution. Optimistic locking changes patching to the update semantic and removes the principal value obtained from patching vs. updating.

[sbueringer]

I wasn't seriously considering this as a solution :)

fakeclient should be just fixed and we're good

[akutz]

I wasn't seriously considering this as a solution :)

fakeclient should be just fixed and we're good

Ack. I just did not want anyone else reading this later to think switching to optimistic locking was a work-around since it changes a primary reason behind using patch. As you said, the only work-around right now is to use envtest.

[sbueringer]

Makes sense!

[alvaroaleman]

/assign

[sbueringer]

@akutz Would be great if you can give this eventually another try :)