kubernetes-sigs / kubebuilder-declarative-pattern

A toolkit for building declarative operators with kubebuilder
Apache License 2.0
252 stars 84 forks source link

fix: mockapiserver data race in create memorystorage #340

Closed yuwenma closed 1 year ago

yuwenma commented 1 year ago

What this PR does / why we need it: This PR fixes a data race issue in test where two threads in MockKubeAPIserver memorystorage read/write to the same resourceInformation

Verify With this PR, the following command should no longer give the data race warning (see data race log)

 go test -race sigs.k8s.io/kubebuilder-declarative-pattern/pkg/patterns/declarative

data race log

WARNING: DATA RACE
Write at 0x00c000792d50 by goroutine 112:
  runtime.mapaccessK()
      /usr/local/go/src/runtime/map.go:518 +0x1ec
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MemoryStorage).CreateObject()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/memorystorage.go:194 +0x204
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*postResource).Run()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/postresource.go:72 +0x3d4
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MockKubeAPIServer).ServeHTTP()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/apiserver.go:334 +0x4680
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2936 +0x548
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1995 +0x8d8
  net/http.(*Server).Serve.func3()
      /usr/local/go/src/net/http/server.go:3089 +0x4c

Previous read at 0x00c000792d50 by goroutine 93:
  runtime.mapdelete()
      /usr/local/go/src/runtime/map.go:695 +0x47c
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*resourceStorage).watch()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/memorystorage_watch.go:68 +0x3a8
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MemoryStorage).Watch()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/memorystorage_watch.go:23 +0x42c
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*listResource).doWatch()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/listresource.go:111 +0x378
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*listResource).Run()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/listresource.go:61 +0x188
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MockKubeAPIServer).ServeHTTP()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/apiserver.go:334 +0x4680
  net/http.serverHandler.ServeHTTP()
      /usr/local/go/src/net/http/server.go:2936 +0x548
  net/http.(*conn).serve()
      /usr/local/go/src/net/http/server.go:1995 +0x8d8
  net/http.(*Server).Serve.func3()
      /usr/local/go/src/net/http/server.go:3089 +0x4c

Goroutine 112 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3089 +0x620
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MockKubeAPIServer).StartServing.func1()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/apiserver.go:66 +0x68

Goroutine 93 (running) created at:
  net/http.(*Server).Serve()
      /usr/local/go/src/net/http/server.go:3089 +0x620
  sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver.(*MockKubeAPIServer).StartServing.func1()
      /Users/yuwenma/go/src/sigs.k8s.io/kubebuilder-declarative-pattern/mockkubeapiserver/apiserver.go:66 +0x68

Which issue(s) this PR fixes:

yuwenma commented 1 year ago

/assign @justinsb

justinsb commented 1 year ago

Thanks for fixing. If we ever do a higher-performance version of the storage, we should snapshot objects to avoid locking everything during the initial stream of objects. But this isn't intended as a high performance mock, so safe & correct is good here!

Not sure if we need a rebase to pick up you go 1.20 fix, I'll try to trigger a retest.

/approve /lgtm

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, yuwenma

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](https://github.com/kubernetes-sigs/kubebuilder-declarative-pattern/blob/master/OWNERS)~~ [justinsb] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment
justinsb commented 1 year ago

Thanks for rebasing

/lgtm