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

Fix race test failure in core/cache #16732

Merged
merged 1 commit into from
Jan 4, 2024
Merged

Fix race test failure in core/cache #16732

merged 1 commit into from
Jan 4, 2024

Conversation

wallyworld
Copy link
Member

@wallyworld wallyworld commented Jan 4, 2024 •
edited
Loading

A ci test run showed a race in the core/cache tests. This fixes that.

WARNING: DATA RACE
Write at 0x00c00117a600 by goroutine 12135:
  runtime.mapassign_fast64()
      /usr/local/go/src/runtime/map_fast64.go:93 +0x0
  github.com/juju/juju/core/cache.(*Resident).registerWorker()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/resident.go:232 +0x95
  github.com/juju/juju/core/cache.newConfigWatcher()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/watcher.go:136 +0x26c
  github.com/juju/juju/core/cache.(*Model).WatchConfig()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/model.go:160 +0x16b
  github.com/juju/juju/apiserver/facades/agent/logger.(*LoggerAPI).WatchLoggingConfig()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/apiserver/facades/agent/logger/logger.go:51 +0x21a
  runtime.call32()
      /usr/local/go/src/runtime/asm_amd64.s:748 +0x42
  reflect.Value.Call()
      /usr/local/go/src/reflect/value.go:380 +0xb5
  github.com/juju/rpcreflect.newMethod.func7()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/vendor/github.com/juju/rpcreflect/type.go:358 +0xf0
  github.com/juju/juju/apiserver.(*srvCaller).Call()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/apiserver/root.go:191 +0x14d
  github.com/juju/juju/rpc.(*Conn).runRequest()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/rpc/server.go:571 +0x2ab
  github.com/juju/juju/rpc.(*Conn).handleRequest.func1()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/rpc/server.go:475 +0x15b

Previous read at 0x00c00117a600 by goroutine 9998:
  runtime.mapiterinit()
      /usr/local/go/src/runtime/map.go:816 +0x0
  github.com/juju/juju/core/cache.(*Resident).cleanupWorkers()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/resident.go:259 +0x86
  github.com/juju/juju/core/cache.(*Resident).cleanup()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/resident.go:251 +0x2e
  github.com/juju/juju/core/cache.(*Resident).evict()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/resident.go:240 +0x26
  github.com/juju/juju/core/cache.(*Controller).removeModel()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/controller.go:340 +0x15c
  github.com/juju/juju/core/cache.(*Controller).loop()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/core/cache/controller.go:164 +0xbf8
  github.com/juju/juju/core/cache.(*Controller).loop-fm()
      <autogenerated>:1 +0x33
  gopkg.in/tomb%2ev2.(*Tomb).run()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/vendor/gopkg.in/tomb.v2/tomb.go:163 +0x43
  gopkg.in/tomb%2ev2.(*Tomb).Go.func2()
      /home/jenkins/workspace/unit-tests-race-amd64/go-path/src/github.com/juju/juju/vendor/gopkg.in/tomb.v2/tomb.go:159 +0x44

Also, to help diagnose intermittent migration ci test failures, turn on debug logging for the models.
And fix some issues with the ci test infrastructure to ensure the add_model helper is used rather than the raw add-model. This ensures that we record the debug log.

QA steps

unit tests

Jira card: JUJU-5217

@wallyworld
Fix race test failure in core/cache
a6658be 
Add debugging to migration ci tests
hpidcock
hpidcock approved these changes Jan 4, 2024
View reviewed changes
Copy link
Member

@hpidcock hpidcock left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for the fix.

@wallyworld
Copy link
Member Author

/merge

@jujubot jujubot merged commit 4acafa2 into juju:3.3 Jan 4, 2024
16 of 20 checks passed
@wallyworld wallyworld mentioned this pull request Jan 4, 2024
Merged
@wallyworld wallyworld added the 3.3 label Jan 4, 2024
@wallyworld wallyworld mentioned this pull request Jan 4, 2024
Merged
jujubot added a commit that referenced this pull request Jan 4, 2024
@jujubot
Merge pull request #16734 from wallyworld/racefix-fix
92a5cac 
#16734

#16732 fixed a race failure but there was a small mistake in calling unlock.

## QA steps

run unit tests
jujubot added a commit that referenced this pull request Jan 4, 2024
@jujubot
Merge pull request #16733 from wallyworld/merge-3.3-20240104a
d03ebad 
#16733

Merge 3.3

#16732 [from wallyworld/fix-cache-race](4acafa2)
wallyworld pushed a commit to wallyworld/juju that referenced this pull request Jan 4, 2024
@jujubot @wallyworld
Merge pull request juju#16733 from wallyworld/merge-3.3-20240104a
a6cef9b 
juju#16733

Merge 3.3

juju#16732 [from wallyworld/fix-cache-race](juju@4acafa2)
@jack-w-shaw jack-w-shaw mentioned this pull request Jan 8, 2024
Merged
jujubot added a commit that referenced this pull request Jan 8, 2024
@jujubot
Merge pull request #16745 from jack-w-shaw/3.4-into-main
cff7efa 
#16745

Merges:
- #16692
- #16721
- #16723
- #16728
- #16730
- #16731
- #16732
- #16734
- #16733
- #16718
- #16727
- #16735

Conflicts:
- api/apiclient.go
- api/client/charms/downloader_s3.go
- api/client/charms/localcharmclient_test.go
- api/client/charms/mocks/objectstore_mock.go
- api/client/charms/package_test.go
- apiserver/admin.go
- apiserver/apiserver.go
- apiserver/facades/client/charms/client_integration_test.go
- apiserver/objects_test.go
- apiserver/root.go
- cmd/containeragent/unit/manifolds.go
- cmd/juju/application/deploy.go
- cmd/juju/application/deploy_test.go
- cmd/juju/application/refresh_test.go
- cmd/jujud/agent/caasoperator/manifolds.go
- core/cache/resident.go
- core/logger/labels.go
- internal/migration/migration.go
- internal/s3client/client.go
- internal/worker/deployer/unit_manifolds.go
- internal/worker/s3caller/client.go
- internal/worker/s3caller/manifold.go
- internal/worker/s3caller/worker.go
- internal/worker/uniter/manifold.go
- worker/s3caller/client.go

_Mostly_ trivial merge conflicts. The only difficulty was surrounding `objectstore.Session`, `s3cleint.Session` and `s3Client.Charm`. I believe I have found the best solution, however. One for @SimonRichardson to look at probably
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hpidcock hpidcock hpidcock approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@wallyworld @hpidcock @jujubot