Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: mockapiserver data race in create memorystorage #340

Merged
merged 1 commit into from
Jun 23, 2023

Conversation

yuwenma
Copy link
Contributor

@yuwenma yuwenma commented Jun 23, 2023

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:

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 23, 2023
@yuwenma
Copy link
Contributor Author

yuwenma commented Jun 23, 2023

/assign @justinsb

@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jun 23, 2023
@justinsb
Copy link
Contributor

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 k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 23, 2023
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023
@justinsb
Copy link
Contributor

Thanks for rebasing

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 23, 2023
@k8s-ci-robot k8s-ci-robot merged commit f988556 into kubernetes-sigs:master Jun 23, 2023
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants