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.

Sign up for GitHub

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

Jump to bottom

Make FakeAPIClient threadsafe #4790

Merged
merged 3 commits into from
Sep 17, 2020
Merged

Make FakeAPIClient threadsafe #4790

merged 3 commits into from
Sep 17, 2020

Conversation

briandealwis
Copy link
Member

Description

In fixing #4757 I discovered that we hit data race issues in the Docker FakeAPIClient when running make test locally. We don't seem to be running tests with -race. I could have papered over the issue with a sync.Mutex, but it felt like it would turn into a game of whackamole.

--- FAIL: TestCacheFindMissing (0.01s)
/pkg/skaffold/build/cache/TestCacheFindMissing/TestCacheFindMissing
==================
WARNING: DATA RACE
Read at 0x00c000180990 by goroutine 80:
  github.com/GoogleContainerTools/skaffold/testutil.(*FakeAPIClient).ImagePull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/testutil/fake_image_api.go:186 +0x4d
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker.(*localDaemon).Pull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/docker/image.go:336 +0x378
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).tryImport()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:129 +0x6a3
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookup()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:60 +0x5ac
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:41 +0x147

Previous write at 0x00c000180990 by goroutine 81:
  github.com/GoogleContainerTools/skaffold/testutil.(*FakeAPIClient).ImagePull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/testutil/fake_image_api.go:186 +0xd9
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker.(*localDaemon).Pull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/docker/image.go:336 +0x378
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).tryImport()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:129 +0x6a3
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookup()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:60 +0x5ac
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:41 +0x147

Goroutine 80 (running) created at:
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:40 +0x16d
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).Build.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/retrieve.go:44 +0x8d

Goroutine 81 (finished) created at:
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:40 +0x16d
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).Build.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/retrieve.go:44 +0x8d
==================
==================
WARNING: DATA RACE
Read at 0x00c000304340 by goroutine 80:
  runtime.growslice()
      /usr/local/go/src/runtime/slice.go:76 +0x0
  github.com/GoogleContainerTools/skaffold/testutil.(*FakeAPIClient).ImagePull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/testutil/fake_image_api.go:186 +0x235
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker.(*localDaemon).Pull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/docker/image.go:336 +0x378
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).tryImport()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:129 +0x6a3
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookup()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:60 +0x5ac
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:41 +0x147

Previous write at 0x00c000304340 by goroutine 81:
  github.com/GoogleContainerTools/skaffold/testutil.(*FakeAPIClient).ImagePull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/testutil/fake_image_api.go:186 +0x9b
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/docker.(*localDaemon).Pull()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/docker/image.go:336 +0x378
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).tryImport()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:129 +0x6a3
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookup()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:60 +0x5ac
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:41 +0x147

Goroutine 80 (running) created at:
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:40 +0x16d
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).Build.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/retrieve.go:44 +0x8d

Goroutine 81 (finished) created at:
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).lookupArtifacts()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/lookup.go:40 +0x16d
  github.com/GoogleContainerTools/skaffold/pkg/skaffold/build/cache.(*cache).Build.func1()
      /Users/bdealwis/Projects/Skaffold/repo-skaffold/pkg/skaffold/build/cache/retrieve.go:44 +0x8d
==================
time="2020-09-15T17:07:38-04:00" level=info msg="Cache check complete in 2.422486ms"
    TestCacheFindMissing/TestCacheFindMissing: testing.go:906: race detected during execution of test
    TestCacheFindMissing: testing.go:906: race detected during execution of test
    --- FAIL: TestCacheFindMissing/TestCacheFindMissing (0.01s)
    : testing.go:906: race detected during execution of test

@briandealwis briandealwis requested a review from a team as a code owner September 15, 2020 21:11
@codecov
Copy link

codecov bot commented Sep 16, 2020 •
edited
Loading

Codecov Report

Merging #4790 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #4790   +/-   ##
=======================================
  Coverage   73.85%   73.85%           
=======================================
  Files         347      347           
  Lines       13796    13796           
=======================================
  Hits        10189    10189           
  Misses       2972     2972           
  Partials      635      635

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 10275c6...bc477f7. Read the comment docs.

nkubala
nkubala approved these changes Sep 16, 2020
View reviewed changes
Copy link
Contributor

@nkubala nkubala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was more involved than i thought it would be :) CI looks like it flaked, merge when green

@briandealwis briandealwis merged commit 2b50012 into GoogleContainerTools:master Sep 17, 2020
@briandealwis briandealwis deleted the racy-docker-api branch September 17, 2020 17:02
@fmotrifork fmotrifork mentioned this pull request Oct 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nkubala nkubala nkubala approved these changes

@ilya-zuyev ilya-zuyev Awaiting requested review from ilya-zuyev

Assignees
No one assigned
Labels
cla: yes size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@briandealwis @nkubala @googlebot