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: Populate destination name when destination server is specified #21063

Merged
merged 3 commits into from
Dec 14, 2024

Conversation

adriananeci
Copy link
Contributor

@adriananeci adriananeci commented Dec 4, 2024

Currently, if the destination is specified via server and not name, sync windows cannot target a cluster by its name even if support for it was added via #7817. That is because the destination name is not populated when using the server setting.

Checklist:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • The title of the PR conforms to the Toolchain Guide
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).
  • My new feature complies with the feature status guidelines.
  • I have added a brief description of why this PR is necessary and/or what this PR solves.
  • Optional. My organization is added to USERS.md.
  • Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

@adriananeci adriananeci requested a review from a team as a code owner December 4, 2024 21:11
Copy link

bunnyshell bot commented Dec 4, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

@adriananeci adriananeci changed the title Populate destination name when destination server is specified [fix] Populate destination name when destination server is specified Dec 4, 2024
@adriananeci adriananeci changed the title [fix] Populate destination name when destination server is specified fix: Populate destination name when destination server is specified Dec 4, 2024
@adriananeci adriananeci requested review from a team as code owners December 4, 2024 21:18
@adriananeci adriananeci force-pushed the get_server_when_name branch 2 times, most recently from 9ef0222 to 666316f Compare December 4, 2024 21:30
util/argo/argo.go Outdated Show resolved Hide resolved
util/argo/argo.go Outdated Show resolved Hide resolved
@adriananeci adriananeci force-pushed the get_server_when_name branch 6 times, most recently from f469315 to 61a3db6 Compare December 9, 2024 22:52
Copy link

codecov bot commented Dec 9, 2024

Codecov Report

Attention: Patch coverage is 60.86957% with 27 lines in your changes missing coverage. Please review.

Project coverage is 55.22%. Comparing base (2f51067) to head (94b9247).
Report is 15 commits behind head on master.

Files with missing lines Patch % Lines
applicationset/utils/clusterUtils.go 57.14% 6 Missing and 3 partials ⚠️
util/argo/argo.go 65.38% 6 Missing and 3 partials ⚠️
server/application/application.go 0.00% 5 Missing ⚠️
pkg/apis/application/v1alpha1/types.go 76.47% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #21063      +/-   ##
==========================================
+ Coverage   55.20%   55.22%   +0.02%     
==========================================
  Files         324      324              
  Lines       55596    55658      +62     
==========================================
+ Hits        30689    30738      +49     
- Misses      22285    22288       +3     
- Partials     2622     2632      +10     

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

applicationset/utils/clusterUtils.go Outdated Show resolved Hide resolved
server/application/application.go Show resolved Hide resolved
Signed-off-by: Adrian Aneci <[email protected]>
Signed-off-by: Adrian Aneci <[email protected]>
@gdsoumya gdsoumya merged commit 030a7be into argoproj:master Dec 14, 2024
27 checks passed
@gdsoumya
Copy link
Member

/cherry-pick release-2.13

Copy link

Cherry-pick failed with Merge error 030a7be7e2fd0ae976c9b73196d777ea72bada09 into temp-cherry-pick-cc74e0-release-2.13

@gdsoumya
Copy link
Member

/cherry-pick release-2.12

Copy link

Cherry-pick failed with Merge error 030a7be7e2fd0ae976c9b73196d777ea72bada09 into temp-cherry-pick-cc74e0-release-2.12

@gdsoumya
Copy link
Member

/cherry-pick release-2.11

Copy link

Cherry-pick failed with Merge error 030a7be7e2fd0ae976c9b73196d777ea72bada09 into temp-cherry-pick-cc74e0-release-2.11

@gdsoumya
Copy link
Member

@adriananeci the cherry pick bot seems to fail cherry picking this pr into the release branches. Can you raise separate PRs to cherry pick this change into the relevant branches?

adriananeci added a commit to adriananeci/argo-cd that referenced this pull request Dec 14, 2024
…rgoproj#21063)

* Populate destination name when destination server is specified

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[email protected]>
@adriananeci
Copy link
Contributor Author

adriananeci commented Dec 14, 2024

Raised #21176 for cherry picking it into 2.13 release branch

adriananeci added a commit to adriananeci/argo-cd that referenced this pull request Dec 14, 2024
…rgoproj#21063)

* Populate destination name when destination server is specified

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[email protected]>
gdsoumya pushed a commit that referenced this pull request Dec 14, 2024
…21063) (cherry-pick 2.13) (#21176)

* fix: Populate destination name when destination server is specified (#21063)

* Populate destination name when destination server is specified

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[email protected]>

* Go lint fixes

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[email protected]>
@adriananeci adriananeci deleted the get_server_when_name branch December 14, 2024 12:09
@crenshaw-dev
Copy link
Member

Looks like this might have introduced a race condition: https://github.com/argoproj/argo-cd/actions/runs/12327129773/job/34408870839

@adriananeci
Copy link
Contributor Author

Hmm, if there is a race condition I think it should have been there for SetInferredServer method too. Maybe there weren't enough tests to validate it until now?

@crenshaw-dev
Copy link
Member

The failure is on an old test, TestMaxPodLogsRender:

=== Failed
=== FAIL: server/application TestMaxPodLogsRender/PodLogs#02 (0.25s)
time="2024-12-14T05:21:17Z" level=info msg="Starting configmap/secret informers"
time="2024-12-14T05:21:17Z" level=info msg="Configmap/secret informer synced"
==================
WARNING: DATA RACE
Write at 0x00c007c8a138 by goroutine 3293:
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationDestination).SetInferredName()
      /home/runner/work/argo-cd/argo-cd/pkg/apis/application/v1alpha1/types.go:3335 +0x584
  github.com/argoproj/argo-cd/v2/util/argo.ValidateDestination()
      /home/runner/work/argo-cd/argo-cd/util/argo/argo.go:522 +0x55f
  github.com/argoproj/argo-cd/v2/server/application.(*Server).getApplicationClusterConfig()
      /home/runner/work/argo-cd/argo-cd/server/application/application.go:1303 +0xfc
  github.com/argoproj/argo-cd/v2/server/application.(*Server).PodLogs()
      /home/runner/work/argo-cd/argo-cd/server/application/application.go:1740 +0x1170
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender.func3()
      /home/runner/work/argo-cd/argo-cd/server/application/application_test.go:2217 +0x1bb
  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

Previous read at 0x00c007c8a138 by goroutine 3291:
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationDestination).String()
      /home/runner/work/argo-cd/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18720 +0x184
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*ApplicationSpec).String()
      /home/runner/work/argo-cd/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:19268 +0x572
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*Application).String()
      /home/runner/work/argo-cd/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18694 +0x11a
  github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1.(*Application).String()
      /home/runner/work/argo-cd/argo-cd/pkg/apis/application/v1alpha1/generated.pb.go:18693 +0xb9
  fmt.(*pp).handleMethods()
      /opt/hostedtoolcache/go/1.23.3/x64/src/fmt/print.go:673 +0x6ea
  fmt.(*pp).printArg()
      /opt/hostedtoolcache/go/1.23.3/x64/src/fmt/print.go:756 +0xb4b
  fmt.(*pp).doPrintf()
      /opt/hostedtoolcache/go/1.23.3/x64/src/fmt/print.go:1173 +0x10ce
  fmt.Sprintf()
      /opt/hostedtoolcache/go/1.23.3/x64/src/fmt/print.go:239 +0x5c
  github.com/stretchr/testify/mock.Arguments.Diff()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:965 +0x1ac
  github.com/stretchr/testify/mock.(*Mock).findExpectedCall()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:383 +0x147
  github.com/stretchr/testify/mock.(*Mock).MethodCalled()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:491 +0xac
  github.com/stretchr/testify/mock.(*Mock).Called()
      /home/runner/go/pkg/mod/github.com/stretchr/[email protected]/mock/mock.go:481 +0x195
  github.com/argoproj/argo-cd/v2/server/application/mocks.(*Broadcaster).OnAdd()
      /home/runner/work/argo-cd/argo-cd/server/application/mocks/Broadcaster.go:17 +0x136
  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()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:72 +0x86

Goroutine 3293 (running) created at:
  testing.(*T).Run()
      /opt/hostedtoolcache/go/1.23.3/x64/src/testing/testing.go:1743 +0x825
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender()
      /home/runner/work/argo-cd/argo-cd/server/application/application_test.go:2216 +0x4d5
  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

Goroutine 3291 (running) created at:
  k8s.io/apimachinery/pkg/util/wait.(*Group).Start()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/pkg/util/wait/wait.go:70 +0xe4
  k8s.io/client-go/tools/cache.(*sharedProcessor).addListener()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/shared_informer.go:748 +0x224
  k8s.io/client-go/tools/cache.(*sharedIndexInformer).AddEventHandlerWithResyncPeriod()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/shared_informer.go:623 +0x770
  k8s.io/client-go/tools/cache.(*sharedIndexInformer).AddEventHandler()
      /home/runner/go/pkg/mod/k8s.io/[email protected]/tools/cache/shared_informer.go:561 +0x51
  github.com/argoproj/argo-cd/v2/server/application.NewServer()
      /home/runner/work/argo-cd/argo-cd/server/application/application.go:120 +0x1a5
  github.com/argoproj/argo-cd/v2/server/application.newTestAppServerWithEnforcerConfigure()
      /home/runner/work/argo-cd/argo-cd/server/application/application_test.go:297 +0x2b71
  github.com/argoproj/argo-cd/v2/server/application.createAppServerWithMaxLodLogs()
      /home/runner/work/argo-cd/argo-cd/server/application/application_test.go:2280 +0x9fd
  github.com/argoproj/argo-cd/v2/server/application.TestMaxPodLogsRender()
      /home/runner/work/argo-cd/argo-cd/server/application/application_test.go:2214 +0x38d
  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
==================
    testing.go:1399: race detected during execution of test

=== FAIL: server/application TestMaxPodLogsRender (3.41s)

@adriananeci
Copy link
Contributor Author

adriananeci commented Dec 15, 2024

The failure is on an old test, TestMaxPodLogsRender

@crenshaw-dev That's true, but the race condition was always there. It just didn't have a test associated with to validate this scenario.
To confirm it was there even before this PR, I just did a simple test like:

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

@adriananeci
Copy link
Contributor Author

Raised #21182 for tracking this race condition.

dudo pushed a commit to dudo/argo-cd that referenced this pull request Jan 18, 2025
…rgoproj#21063)

* Populate destination name when destination server is specified

Signed-off-by: Adrian Aneci <[email protected]>

* Lint nit

Signed-off-by: Adrian Aneci <[email protected]>

* Trigger CI

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[email protected]>
Signed-off-by: Brett C. Dudo <[email protected]>
revitalbarletz pushed a commit to revitalbarletz/argo-cd that referenced this pull request Jan 20, 2025
…rgoproj#21063)

* Populate destination name when destination server is specified

Signed-off-by: Adrian Aneci <[email protected]>

* Lint nit

Signed-off-by: Adrian Aneci <[email protected]>

* Trigger CI

Signed-off-by: Adrian Aneci <[email protected]>

---------

Signed-off-by: Adrian Aneci <[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