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(settings): race condition on settings configMap #21225

Merged
merged 7 commits into from
Dec 18, 2024

Conversation

agaudreault
Copy link
Member

@agaudreault agaudreault commented Dec 17, 2024

Sometimes, the test are failing with a race condition on settings usage. We return a pointer to the value of the ArgoCM in multiple thread and assume all methods will be nice and never update the object. updateConfigMap exist to write back the configMap to Kubernetes. Updating the object in the informer should never be done. The same is true for the secrets

This PR returns a deepCopy of what is in the informer, causing a slight performance impact. However, now threads can modify that object without side effects on other threads. The usage is simpler than using a RW Mutex or a concurrent map.

This is not perfect, because the secret informer/lister are exposed externally, but it improves the current situation.

Ran on https://github.com/argoproj/argo-cd/commits/3057a285bc2dde547db0947f62115f32c4804838

time="2024-12-16T22:49:44Z" level=info msg="Comparing app state (cluster: https://localhost:6443, namespace: fake-dest-ns)" application=fake-argocd-ns/my-app
==================
WARNING: DATA RACE
Read at 0x00c0063468f0 by goroutine 5181:
  github.com/argoproj/argo-cd/v2/util/settings.updateSettingsFromConfigMap()
      /home/runner/work/argo-cd/argo-cd/util/settings/settings.go:1488 +0x5a
  github.com/argoproj/argo-cd/v2/util/settings.(*SettingsManager).GetSettings()
      /home/runner/work/argo-cd/argo-cd/util/settings/settings.go:1349 +0x4c4
  github.com/argoproj/argo-cd/v2/util/db.(*db).GetCluster()
      /home/runner/work/argo-cd/argo-cd/util/db/cluster.go:228 +0x6a
  github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).canProcessApp()
      /home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:2283 +0x4b3
  github.com/argoproj/argo-cd/v2/controller.(*ApplicationController).newApplicationInformerAndLister.func5()
      /home/runner/work/argo-cd/argo-cd/controller/appcontroller.go:2377 +0x58
  k8s.io/client-go/tools/cache.ResourceEventHandlerFuncs.OnAdd()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/controller.go:246 +0x63
  k8s.io/client-go/tools/cache.(*ResourceEventHandlerFuncs).OnAdd()
      <autogenerated>:1 +0x1b
  k8s.io/client-go/tools/cache.(*processorListener).run.func1()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/shared_informer.go:978 +0x287
  k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/backoff.go:226 +0x41
  k8s.io/apimachinery/pkg/util/wait.BackoffUntil()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/backoff.go:227 +0xc4
  k8s.io/apimachinery/pkg/util/wait.JitterUntil()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/backoff.go:204 +0x10a
  k8s.io/apimachinery/pkg/util/wait.Until()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/backoff.go:161 +0x9b
  k8s.io/client-go/tools/cache.(*processorListener).run()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/shared_informer.go:972 +0x38
  k8s.io/client-go/tools/cache.(*processorListener).run-fm()
      <autogenerated>:1 +0x33
  k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
make: *** [Makefile:421: test-race-local] Error 1
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:72 +0x86

Previous write at 0x00c0063468f0 by goroutine 5140:
  github.com/argoproj/argo-cd/v2/util/settings.(*SettingsManager).getConfigMap()
      /home/runner/work/argo-cd/argo-cd/util/settings/settings.go:737 +0x107
  github.com/argoproj/argo-cd/v2/util/settings.(*SettingsManager).GetResourceOverrides()
      /home/runner/work/argo-cd/argo-cd/util/settings/settings.go:933 +0x3e
  github.com/argoproj/argo-cd/v2/controller.(*appStateManager).getComparisonSettings()
      /home/runner/work/argo-cd/argo-cd/controller/state.go:408 +0xa4
  github.com/argoproj/argo-cd/v2/controller.(*appStateManager).CompareAppState()
      /home/runner/work/argo-cd/argo-cd/controller/state.go:475 +0x1b0
  github.com/argoproj/argo-cd/v2/controller.TestCompareAppStateSkipHook()
      /home/runner/work/argo-cd/argo-cd/controller/state_test.go:405 +0x8f7
  testing.tRunner()
      /opt/hostedtoolcache/go/1.23.3/x64/src/testing/testing.go:1690 +0x226
  testing.(*T).Run.gowrap1()
      /opt/hostedtoolcache/go/1.23.3/x64/src/testing/testing.go:1743 +0x44

@agaudreault agaudreault requested a review from a team as a code owner December 17, 2024 18:11
Copy link

bunnyshell bot commented Dec 17, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@agaudreault agaudreault marked this pull request as draft December 17, 2024 19:39
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Alexandre Gaudreault <[email protected]>
Copy link

codecov bot commented Dec 17, 2024

Codecov Report

Attention: Patch coverage is 81.63265% with 9 lines in your changes missing coverage. Please review.

Project coverage is 55.07%. Comparing base (46bfc10) to head (4fdb122).
Report is 13 commits behind head on master.

Files with missing lines Patch % Lines
util/settings/settings.go 79.54% 6 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21225      +/-   ##
==========================================
+ Coverage   55.03%   55.07%   +0.03%     
==========================================
  Files         338      338              
  Lines       57075    57048      -27     
==========================================
+ Hits        31410    31417       +7     
+ Misses      22968    22933      -35     
- Partials     2697     2698       +1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@agaudreault agaudreault marked this pull request as ready for review December 17, 2024 22:26
@agaudreault
Copy link
Member Author

agaudreault commented Dec 17, 2024

Will test in a real instance with #21208 and 2.13.

Edit: tested and seems to work without issues

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this!

Copy link
Member

@rumstead rumstead left a comment

Choose a reason for hiding this comment

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

LGTM

@crenshaw-dev crenshaw-dev merged commit 13b7b09 into argoproj:master Dec 18, 2024
28 checks passed
@agaudreault agaudreault deleted the setting-race-cond branch December 18, 2024 16:22
dudo pushed a commit to dudo/argo-cd that referenced this pull request Jan 18, 2025
* fix(settings): race condition on settings configMap

Signed-off-by: Alexandre Gaudreault <[email protected]>

* fix secret update

Signed-off-by: Alexandre Gaudreault <[email protected]>

* simplify sync method usage

Signed-off-by: Alexandre Gaudreault <[email protected]>

* remove unnecessary lister usage

Signed-off-by: Alexandre Gaudreault <[email protected]>

* tests

Signed-off-by: Alexandre Gaudreault <[email protected]>

* fix cache nil ref

Signed-off-by: Alexandre Gaudreault <[email protected]>

---------

Signed-off-by: Alexandre Gaudreault <[email protected]>
Signed-off-by: Brett C. Dudo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants