Race condition in fake client with Unstructured get

[sbueringer]

Currently testing Cluster API with latest main of CR.

Found the following race condition:

Write at 0x00c0003fa240 by goroutine 1648:
  runtime.mapassign()
      /usr/local/go/src/internal/runtime/maps/runtime_swiss.go:191 +0x0
  k8s.io/apimachinery/pkg/runtime.(*Scheme).AddKnownTypeWithName()
      /home/prow/go/pkg/mod/k8s.io/apimachinery@v0.34.0-rc.1/pkg/runtime/scheme.go:191 +0x4fa
  sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).addToSchemeIfUnknownAndUnstructuredOrPartial()
      /home/prow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.1-0.20250818110513-3554729cfb31/pkg/client/fake/client.go:1882 +0x2ca
  sigs.k8s.io/controller-runtime/pkg/client/fake.(*fakeClient).Get()
      /home/prow/go/pkg/mod/sigs.k8s.io/controller-runtime@v0.21.1-0.20250818110513-3554729cfb31/pkg/client/fake/client.go:609 +0xb5
  sigs.k8s.io/cluster-api/controllers/external.GetObjectFromContractVersionedRef()
      /home/prow/go/src/sigs.k8s.io/cluster-api/controllers/external/util.go:78 +0x549
  sigs.k8s.io/cluster-api/internal/controllers/machineset.(*Reconciler).reconcileExternalTemplateReference()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller.go:1561 +0x184
  sigs.k8s.io/cluster-api/internal/controllers/machineset.(*Reconciler).reconcileInfrastructure()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller.go:384 +0x149
  sigs.k8s.io/cluster-api/internal/controllers/machineset.(*Reconciler).reconcileInfrastructure-fm()
      <autogenerated>:1 +0x5d
  sigs.k8s.io/cluster-api/internal/controllers/machineset.(*Reconciler).Reconcile.wrapErrMachineSetReconcileFunc.func3()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller.go:286 +0x61
  sigs.k8s.io/cluster-api/internal/controllers/machineset.doReconcile()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller.go:296 +0xf8
  sigs.k8s.io/cluster-api/internal/controllers/machineset.(*Reconciler).Reconcile()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller.go:267 +0x16d7
  sigs.k8s.io/cluster-api/internal/controllers/machineset.TestMachineSetReconcile_MachinesCreatedConditionFalseOnBadInfraRef()
      /home/prow/go/src/sigs.k8s.io/cluster-api/internal/controllers/machineset/machineset_controller_test.go:1011 +0xfb9
  testing.tRunner()
      /usr/local/go/src/testing/testing.go:1792 +0x225
  testing.(*T).Run.gowrap1()
      /usr/local/go/src/testing/testing.go:1851 +0x44

(This is a Get with Unstructured with the fake client)

Previous read at 0x00c0003fa240 by goroutine 850:
  runtime.mapaccess1()
      /usr/local/go/src/internal/runtime/maps/runtime_swiss.go:43 +0x0
  k8s.io/apimachinery/pkg/runtime.(*Scheme).ObjectKinds()
      /home/prow/go/pkg/mod/k8s.io/apimachinery@v0.34.0-rc.1/pkg/runtime/scheme.go:273 +0x149
  k8s.io/apimachinery/pkg/runtime/serializer/json.(*Serializer).Decode()
      /home/prow/go/pkg/mod/k8s.io/apimachinery@v0.34.0-rc.1/pkg/runtime/serializer/json/json.go:166 +0xb72
  k8s.io/apimachinery/pkg/runtime.WithoutVersionDecoder.Decode()
      /home/prow/go/pkg/mod/k8s.io/apimachinery@v0.34.0-rc.1/pkg/runtime/helper.go:256 +0xb3
  k8s.io/apimachinery/pkg/runtime.(*WithoutVersionDecoder).Decode()
      <autogenerated>:1 +0xb1
  k8s.io/client-go/rest.Result.Into()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/rest/request.go:1462 +0x9a
  k8s.io/client-go/kubernetes/typed/core/v1.(*events).CreateWithEventNamespaceWithContext()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/kubernetes/typed/core/v1/event_expansion.go:86 +0x346
  k8s.io/client-go/kubernetes/typed/core/v1.(*events).CreateWithEventNamespace()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/kubernetes/typed/core/v1/event_expansion.go:69 +0x44
  k8s.io/client-go/kubernetes/typed/core/v1.(*EventSinkImpl).Create()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/kubernetes/typed/core/v1/event_expansion.go:217 +0x47
  k8s.io/client-go/tools/record.recordEvent()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/tools/record/event.go:340 +0x1de
  k8s.io/client-go/tools/record.(*eventBroadcasterImpl).recordToSink()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/tools/record/event.go:302 +0x26d
  k8s.io/client-go/tools/record.(*eventBroadcasterImpl).StartRecordingToSink.func1()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/tools/record/event.go:279 +0x64
  k8s.io/client-go/tools/record.(*eventBroadcasterImpl).StartEventWatcher.func1()
      /home/prow/go/pkg/mod/k8s.io/client-go@v0.34.0-rc.1/tools/record/event.go:418 +0x1a2

(Seems like a simple create event (CreateWithEventNamespaceWithContext))

Test code: https://github.com/kubernetes-sigs/cluster-api/blob/3adb25101d845af093cc44975b67cc1b94838e2e/internal/controllers/machineset/machineset_controller_test.go#L1005-L1010
PR: kubernetes-sigs/cluster-api#12634
Job: https://prow.k8s.io/view/gs/kubernetes-ci-logs/pr-logs/pull/kubernetes-sigs_cluster-api/12634/pull-cluster-api-test-main/1957403235211808768

Not sure if anything in controller-runtime changed or if there was a change in client-go.

[sbueringer]

Looks like the issue in my test is the following:

• Unstructured Get with fake client updates the scheme via addToSchemeIfUnknownAndUnstructuredOrPartial
• Another controller running in the background creates an event (which reads the scheme at some point)

I guess the way to avoid this is to make sure the scheme used by the fake client is not used anywhere else in parallel.

Seems like the event code is using the global scheme.Scheme

Which then probably means fake client should not be used with scheme.Scheme if something else that might reads the scheme is running in parallel.

I think we already had a few cases like this in the past in Cluster API.

Not sure if there is anything we want to do. Some ideas how we could improve fake client:

• copy scheme.Scheme in fake client Build() instead of using it directly (if that's possible)
• get rid of addToSchemeIfUnknownAndUnstructuredOrPartial somehow

[sbueringer]

cc @alvaroaleman (just fyi)

[alvaroaleman]

I tried to reproduce this on your PR @8a806364d00d946b4c65186998aaa9a6c1e81a5d but it succeeds:

$ go test ./internal/controllers/machineset/... -race -count=1
ok      sigs.k8s.io/cluster-api/internal/controllers/machineset 20.667s

[sbueringer]

Sorry, before or after this commit kubernetes-sigs/cluster-api@8a80636#diff-6c89b34d019e827dbd05797f8cb47bb08a6cbbf8ce3879697252be8944567867?

(Only the change in internal/controllers/machineset/machineset_controller_test.go)

[alvaroaleman]

Ah it was with that change, reverting it reproduces the issue.

So changing our code to copy the client-go scheme rather than using it fixes this issue but doesn't fix your test, it just makes it fail differently as it seems to rely on concurrent tests adding clusterv1 to the scheme:

FAIL: TestMachineSetOwnerReference (0.01s)
    --- FAIL: TestMachineSetOwnerReference/should_add_cluster_owner_reference_to_machine_set (0.01s)
panic: failed to get gvk for object sets.Set[k8s.io/apimachinery/pkg/runtime/schema.GroupVersionKind]: no kind is registered for the type v1beta2.MachineSet in scheme "pkg/runtime/scheme.go:110" [recovered, repanicked]

and I think there is a high probability there are lots of tests out there that assume adding something to the global scheme after instantiating a fake client without a custom scheme will make that something available in the previously-instantiated fakeclient, which will break. So I am not in favor of doing that.

The whole addToSchemeIfUnknownAndUnstructuredOrPartial isn't realy new so I am assuming this is rooted in changes to the recorder.

[sbueringer]

The whole addToSchemeIfUnknownAndUnstructuredOrPartial isn't realy new so I am assuming this is rooted in changes to the recorder.

I'm not sure if it even has anything to do with the new CR release. We had these issues popping up here and there for a while. Not sure which recorder change you mean (the event migration PR Is not merged yet)

So I am not in favor of doing that.

Yup. That's fine from my side