bug: Race condition when setting application destination inferred server or name

[adriananeci]

Checklist:

• I've searched in the docs and FAQ for my answer: https://bit.ly/argocd-faq.
• I've included steps to reproduce the bug.
• I've pasted the output of argocd version.

Describe the bug

Once #21063 was merged, a race condition was surfaced when trying to set the application destination inferred server or name.

To Reproduce

• adjust the testApp (https://github.com/argoproj/argo-cd/blob/master/server/application/application_test.go#L2264 from testApp := newTestApp to testApp := newTestAppWithDestName
• run the test using go test -race -run TestMaxPodLogsRender ./server/application -count=1

This race condition is not always popping up. Since it is intermittently triggered, you might need to run the test few times in order to hit the race condition

Expected behaviour

No race conditions should occur.

Screenshots

None

Version

master branch

Logs

WARNING: DATA RACE
Write at 0x00c000b09518 by goroutine 400:
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationDestination).SetInferredServer()
      /Users/aneci/gits/argo-cd/pkg/apis/application/v1alpha1/types.go:3316 +0x134
  github.com/argoproj/argo-cd/v2/util/argo.ValidateDestination()
      /Users/aneci/gits/argo-cd/util/argo/argo.go:507 +0x120
  github.com/argoproj/argo-cd/v2/server/application.(*Server).getApplicationClusterConfig()
      /Users/aneci/gits/argo-cd/server/application/application.go:1297 +0x70
  github.com/argoproj/argo-cd/v2/server/application.(*Server).PodLogs()
      /Users/aneci/gits/argo-cd/server/application/application.go:1734 +0xb10
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender.func2()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:2204 +0x190
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x40

Previous read at 0x00c000b09518 by goroutine 398:
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationDestination).String()
      /Users/aneci/gits/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18718 +0x38
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationSpec).String()
      /Users/aneci/gits/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:19268 +0x450
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*Application).String()
      /Users/aneci/gits/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18694 +0xd4
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*Application).String()
      /Users/aneci/gits/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18693 +0x80
  fmt.(*pp).handleMethods()
      /opt/homebrew/opt/go/libexec/src/fmt/print.go:673 +0x54c
  fmt.(*pp).printArg()
      /opt/homebrew/opt/go/libexec/src/fmt/print.go:756 +0x874
  fmt.(*pp).doPrintf()
      /opt/homebrew/opt/go/libexec/src/fmt/print.go:1173 +0xc58
  fmt.Sprintf()
      /opt/homebrew/opt/go/libexec/src/fmt/print.go:239 +0x50
  github.com/stretchr/testify/mock.Arguments.Diff()
      /Users/aneci/gits/argo-cd/vendor/github.com/stretchr/testify/mock/mock.go:965 +0x140
  github.com/stretchr/testify/mock.(*Mock).findExpectedCall()
      /Users/aneci/gits/argo-cd/vendor/github.com/stretchr/testify/mock/mock.go:383 +0x118
  github.com/stretchr/testify/mock.(*Mock).MethodCalled()
      /Users/aneci/gits/argo-cd/vendor/github.com/stretchr/testify/mock/mock.go:491 +0x6c
  github.com/stretchr/testify/mock.(*Mock).Called()
      /Users/aneci/gits/argo-cd/vendor/github.com/stretchr/testify/mock/mock.go:481 +0x150
  github.com/argoproj/argo-cd/v2/server/application/mocks.(*Broadcaster).OnAdd()
      /Users/aneci/gits/argo-cd/server/application/mocks/Broadcaster.go:17 +0xe0
  k8s.io/client-go/tools/cache.(*processorListener).run.func1()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/client-go/tools/cache/shared_informer.go:978 +0x244
  k8s.io/apimachinery/pkg/util/wait.BackoffUntil.func1()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:226 +0x48
  k8s.io/apimachinery/pkg/util/wait.BackoffUntil()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:227 +0x94
  k8s.io/apimachinery/pkg/util/wait.JitterUntil()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:204 +0xe4
  k8s.io/apimachinery/pkg/util/wait.Until()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/backoff.go:161 +0x88
  k8s.io/client-go/tools/cache.(*processorListener).run()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/client-go/tools/cache/shared_informer.go:972 +0x38
  k8s.io/client-go/tools/cache.(*processorListener).run-fm()
      <autogenerated>:1 +0x34
  k8s.io/apimachinery/pkg/util/wait.(*Group).Start.func1()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:72 +0x80

Goroutine 400 (running) created at:
  testing.(*T).Run()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x5e0
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:2203 +0x324
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x40

Goroutine 398 (running) created at:
  k8s.io/apimachinery/pkg/util/wait.(*Group).Start()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/apimachinery/pkg/util/wait/wait.go:70 +0xd4
  k8s.io/client-go/tools/cache.(*sharedProcessor).addListener()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/client-go/tools/cache/shared_informer.go:748 +0x1a0
  k8s.io/client-go/tools/cache.(*sharedIndexInformer).AddEventHandlerWithResyncPeriod()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/client-go/tools/cache/shared_informer.go:623 +0x550
  k8s.io/client-go/tools/cache.(*sharedIndexInformer).AddEventHandler()
      /Users/aneci/gits/argo-cd/vendor/k8s.io/client-go/tools/cache/shared_informer.go:561 +0x4c
  github.com/argoproj/argo-cd/v2/server/application.NewServer()
      /Users/aneci/gits/argo-cd/server/application/application.go:120 +0xf8
  github.com/argoproj/argo-cd/v2/server/application.newTestAppServerWithEnforcerConfigure()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:297 +0x2160
  github.com/argoproj/argo-cd/v2/server/application.newTestAppServer()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:139 +0x5c
  github.com/argoproj/argo-cd/v2/server/application.createAppServerWithMaxLodLogs()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:2283 +0x754
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender()
      /Users/aneci/gits/argo-cd/server/application/application_test.go:2201 +0x1ec
  testing.tRunner()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1690 +0x184
  testing.(*T).Run.gowrap1()
      /opt/homebrew/opt/go/libexec/src/testing/testing.go:1743 +0x40

[crenshaw-dev]

I'd recommend that we revert the change until the race condition is fixed. I'm seeing ~2/3 test runs fail, and that's going to be a drag on PR velocity until the bug is fixed.

[adriananeci]

I would like to propose to temporary disable that particular test instead since the race condition will still be there even after reverting the PR (It will be just masked as there will be no tests to validate it).

[crenshaw-dev]

How can we be confident that the changes haven't make the race condition worse?

[adriananeci]

How can we be confident that the changes haven't make the race condition worse?

Indeed, the race condition might be worse now since it might trigger on both SetInferredServer and SetInferredName even if only one of the methods will get called at any point in time(they are mutually exclusive).

I've just raised #21201 btw. I hope it addresses the race condition.

[adriananeci]

Unfortunately that PR didn't solve it 😞 . The race being so nondeterministic I thought it will be addressing it after running it many times locally without any failures 😄 .

[crenshaw-dev]

I'm curious if a radical refactor could eliminate the problem entirely: #21189

I'm not sure I like the approach yet, but I think it's worth looking at.

[adriananeci]

I like that approach, it would simplify a lot the logic in many areas and also will most probably reduce if not eliminates existing related bugs and race conditions.

Meanwhile I've raised an alternative PR to this one to temporary avoid running the test when -race flag is enabled, #21204