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

data race - GetKialiTokenForHomeCluster #6641

Open
jmazzitelli opened this issue Sep 19, 2023 · 5 comments · May be fixed by #6932
Open

data race - GetKialiTokenForHomeCluster #6641

jmazzitelli opened this issue Sep 19, 2023 · 5 comments · May be fixed by #6932
Assignees
@nrfox
Labels
bug Something isn't working

Comments

@jmazzitelli
Copy link
Collaborator

Saw this in a CI run - just documenting it now so I don't lose it. If this really isn't an issue, just close this. But we should at least quickly review the affected code.

==================
WARNING: DATA RACE
Read at 0x000004[49](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:50)91b0 by goroutine 396:
  github.com/kiali/kiali/kubernetes.GetKialiTokenForHomeCluster()
      /home/runner/work/kiali/kiali/kubernetes/token.go:24 +0x3e
  github.com/kiali/kiali/kubernetes.(*clientFactory).refreshClientIfTokenChanged()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:411 +0x2c9
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:395 +0x70
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAHomeClusterClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:456 +0x49
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1.1.1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:43 +0x97
  k8s.io/apimachinery/pkg/util/wait.loopConditionUntilContext.func1()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/loop.go:62 +0x7b
  k8s.io/apimachinery/pkg/util/wait.loopConditionUntilContext()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/loop.go:63 +0x2a7
  k8s.io/apimachinery/pkg/util/wait.PollUntilContextCancel()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/poll.go:33 +0x70
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1.1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:41 +0x164
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:57 +0x64

Previous write at 0x0000044991b0 by goroutine 397:
  github.com/kiali/kiali/kubernetes.GetKialiTokenForHomeCluster()
      /home/runner/work/kiali/kiali/kubernetes/token.go:37 +0x12a
  github.com/kiali/kiali/kubernetes.(*clientFactory).refreshClientIfTokenChanged()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:411 +0x2c9
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:395 +0x70
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAHomeClusterClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:456 +0x49
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).watchForClientChanges.func1()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:182 +0x13b

Goroutine 396 (running) created at:
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:22 +0x10e
  github.com/kiali/kiali/kubernetes/cache.NewKialiCache()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:129 +0x611
  github.com/kiali/kiali/business.initKialiCache()
      /home/runner/work/kiali/kiali/business/layer.go:66 +0x2c4
  github.com/kiali/kiali/business.Start()
      /home/runner/work/kiali/kiali/business/layer.go:91 +0x44
  github.com/kiali/kiali/server.(*Server).Start()
      /home/runner/work/kiali/kiali/server/server.go:92 +0x45
  main.main()
      /home/runner/work/kiali/kiali/kiali.go:112 +0x57b

Goroutine 397 (running) created at:
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).watchForClientChanges()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:178 +0x1d4
  github.com/kiali/kiali/kubernetes/cache.NewKialiCache()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:135 +0x656
  github.com/kiali/kiali/business.initKialiCache()
      /home/runner/work/kiali/kiali/business/layer.go:66 +0x2c4
  github.com/kiali/kiali/business.Start()
      /home/runner/work/kiali/kiali/business/layer.go:91 +0x44
  github.com/kiali/kiali/server.(*Server).Start()
      /home/runner/work/kiali/kiali/server/server.go:92 +0x45
  main.main()
      /home/runner/work/kiali/kiali/kiali.go:112 +0x57b
==================
==================
WARNING: DATA RACE
Read at 0x00000449c320 by goroutine 396:
  github.com/kiali/kiali/kubernetes.shouldRefreshToken()
      /home/runner/work/kiali/kiali/kubernetes/token.go:[50](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:51) +0x64
  github.com/kiali/kiali/kubernetes.GetKialiTokenForHomeCluster()
      /home/runner/work/kiali/kiali/kubernetes/token.go:24 +0x33
  github.com/kiali/kiali/kubernetes.(*clientFactory).refreshClientIfTokenChanged()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:411 +0x2c9
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:395 +0x70
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAHomeClusterClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:4[56](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:57) +0x49
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1.1.1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:43 +0x97
  k8s.io/apimachinery/pkg/util/wait.loopConditionUntilContext.func1()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/loop.go:62 +0x7b
  k8s.io/apimachinery/pkg/util/wait.loopConditionUntilContext()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/loop.go:63 +0x2a7
  k8s.io/apimachinery/pkg/util/wait.PollUntilContextCancel()
      /home/runner/go/pkg/mod/k8s.io/apimachinery@v0.27.2/pkg/util/wait/poll.go:33 +0x70
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1.1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:41 +0x164
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus.func1()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:57 +0x64

Previous write at 0x00000449c320 by goroutine 397:
  github.com/kiali/kiali/kubernetes.GetKialiTokenForHomeCluster()
      /home/runner/work/kiali/kiali/kubernetes/token.go:39 +0x337
  github.com/kiali/kiali/kubernetes.(*clientFactory).refreshClientIfTokenChanged()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:411 +0x2c9
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:395 +0x70
  github.com/kiali/kiali/kubernetes.(*clientFactory).GetSAHomeClusterClient()
      /home/runner/work/kiali/kiali/kubernetes/client_factory.go:456 +0x49
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).watchForClientChanges.func1()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:182 +0x13b

Goroutine 396 (running) created at:
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).pollIstiodForProxyStatus()
      /home/runner/work/kiali/kiali/kubernetes/cache/proxy_status.go:22 +0x10e
  github.com/kiali/kiali/kubernetes/cache.NewKialiCache()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:129 +0x611
  github.com/kiali/kiali/business.initKialiCache()
      /home/runner/work/kiali/kiali/business/layer.go:66 +0x2c4
  github.com/kiali/kiali/business.Start()
      /home/runner/work/kiali/kiali/business/layer.go:91 +0x44
  github.com/kiali/kiali/server.(*Server).Start()
      /home/runner/work/kiali/kiali/server/server.go:92 +0x45
  main.main()
      /home/runner/work/kiali/kiali/kiali.go:112 +0x[57](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:58)b

Goroutine 397 (running) created at:
  github.com/kiali/kiali/kubernetes/cache.(*kialiCacheImpl).watchForClientChanges()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:178 +0x1d4
  github.com/kiali/kiali/kubernetes/cache.NewKialiCache()
      /home/runner/work/kiali/kiali/kubernetes/cache/cache.go:135 +0x[65](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:66)6
  github.com/kiali/kiali/business.initKialiCache()
      /home/runner/work/kiali/kiali/business/layer.go:[66](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:67) +0x2c4
  github.com/kiali/kiali/business.Start()
      /home/runner/work/kiali/kiali/business/layer.go:[91](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:92) +0x44
  github.com/kiali/kiali/server.(*Server).Start()
      /home/runner/work/kiali/kiali/server/server.go:[92](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:93) +0x45
  main.main()
      /home/runner/work/kiali/kiali/kiali.go:[112](https://github.com/kiali/kiali/actions/runs/6236835841/job/16929649550?pr=6629#step:9:113) +0x57b
@jmazzitelli jmazzitelli added the bug Something isn't working label Sep 19, 2023
@jshaughn jshaughn added the backlog Triaged Issue added to backlog label Oct 12, 2023
@jshaughn
Copy link
Collaborator

jshaughn commented Dec 6, 2023

Closing as this has not gotten any follow-up.

@jshaughn jshaughn closed this as completed Dec 6, 2023
@nrfox
Copy link
Contributor

nrfox commented Dec 6, 2023

FWIW this is still an issue. You can still see these data race logs in CI: https://github.com/kiali/kiali/actions/runs/6973247695/job/18976996749?pr=6854#step:9:71. You'd be able to see these in production as well but the prod builds do not have the -race flag for perf reasons.

@jshaughn
Copy link
Collaborator

jshaughn commented Dec 6, 2023

@jmazzitelli Do you want to investigate this?

@nrfox
Copy link
Contributor

nrfox commented Dec 6, 2023

I took a cursory look at this and here's where I think the problem is:

kiali/kubernetes/cache/kube_cache.go

Lines 981 to 993 in 2ed984f

if err := checkIstioAPIsExist(c.client); err != nil {
return nil, err
}
selector, err := labels.Parse(labelSelector)
if err != nil {
return nil, err
}
// Read lock will prevent the cache from being refreshed while we are reading from the lister
// but it won't prevent other routines from reading from the lister.
defer c.cacheLock.RUnlock()
c.cacheLock.RLock()

Inside the kube cache methods, the checkIstioAPIsExist(c.client) is not thread safe and we need to lock before we call that method not after. Not sure if this will fix all the data race warnings but it should fix some.

@jshaughn jshaughn reopened this Dec 6, 2023
@jshaughn
Copy link
Collaborator

jshaughn commented Dec 6, 2023

@nrfox Make it so.

@nrfox nrfox linked a pull request Dec 6, 2023 that will close this issue
Draft
@jshaughn jshaughn removed the backlog Triaged Issue added to backlog label Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nrfox nrfox

Labels
bug Something isn't working
Projects
Kiali Sprint 24-7 | Kiali v1.86
Status: 🏗 In progress
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Fix race condition in kube cache by locking sooner nrfox/kiali
3 participants
@jmazzitelli @jshaughn @nrfox