test(datarace): Fix datarace issue in spanstat.go#11615
test(datarace): Fix datarace issue in spanstat.go#11615borkmann merged 1 commit intocilium:masterfrom
Conversation
|
Please set the appropriate release note label. |
dd36ee4 to
7f791c7
Compare
|
Hi @sayboras , do you need help or feedback on this issue/PR? It is currently marked as a release blocker for v1.8 so we will want to determine whether it's vital to merge for v1.8 and if so, help to get it merged. |
267eb27 to
8aa4d50
Compare
@joestringer hi, if you can help on this. I added lock for SpanStat struct, however, was having below copylocks issue. To resolve this, I need to use pointer (e.g. lock *lock.RWMutex), but then the drawback is to have constructor to initialize values. This must be done for all instances of SpanStat struct. Looking forward to hearing your input. Thanks copylocks issue$ golangci-lint run ./... --disable-all -E govet
daemon/cmd/fqdn.go:408:164: copylocks: notifyOnDNSMsg passes lock by value: github.com/cilium/cilium/pkg/fqdn/dnsproxy.ProxyRequestContext contains github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
func (d *Daemon) notifyOnDNSMsg(lookupTime time.Time, ep *endpoint.Endpoint, epIPPort string, serverAddr string, msg *dns.Msg, protocol string, allowed bool, stat dnsproxy.ProxyRequestContext) error {
^
pkg/endpoint/policy.go:215:33: copylocks: literal copies lock value from stats.waitingForIdentityCache: github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
"waitingForIdentityCache": stats.waitingForIdentityCache,
^
pkg/endpoint/policy.go:216:33: copylocks: literal copies lock value from stats.waitingForPolicyRepository: github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
"waitingForPolicyRepository": stats.waitingForPolicyRepository,
^
pkg/endpoint/policy.go:217:33: copylocks: literal copies lock value from stats.policyCalculation: github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
"policyCalculation": stats.policyCalculation,
^
pkg/fqdn/dnsproxy/proxy.go:373:77: copylocks: call of p.NotifyOnDNSMsg copies lock value: github.com/cilium/cilium/pkg/fqdn/dnsproxy.ProxyRequestContext contains github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
p.NotifyOnDNSMsg(time.Now(), nil, epIPPort, "", request, protocol, false, stat)
^
pkg/fqdn/dnsproxy/proxy.go:382:77: copylocks: call of p.NotifyOnDNSMsg copies lock value: github.com/cilium/cilium/pkg/fqdn/dnsproxy.ProxyRequestContext contains github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
p.NotifyOnDNSMsg(time.Now(), nil, epIPPort, "", request, protocol, false, stat)
^
pkg/fqdn/dnsproxy/proxy.go:394:90: copylocks: call of p.NotifyOnDNSMsg copies lock value: github.com/cilium/cilium/pkg/fqdn/dnsproxy.ProxyRequestContext contains github.com/cilium/cilium/pkg/spanstat.SpanStat contains github.com/cilium/cilium/pkg/lock.RWMutex (govet)
p.NotifyOnDNSMsg(time.Now(), ep, epIPPort, targetServerAddr, request, protocol, false, stat) |
8aa4d50 to
95c3b39
Compare
643502b to
58b8fbd
Compare
|
test-me-please |
joestringer
left a comment
There was a problem hiding this comment.
Looks good, passing stats by reference everywhere makes sense and is a reasonable way to resolve the govet issue.
It'd be good to squash the two commits together just to ensure that we won't hit any issues in the build process in future in case we need to bisect the tree across this commit.
There was a problem hiding this comment.
FYI, If you compose the SpanStat from lock.RWMutex directly (ie no field name), then these can be s.Lock() / defer s.Unlock() and you don't have to worry about whether to call it mutex or lock or so on :-)
FWIW we aren't consistently composing objects from locks this way across the codebase so I'm not fussed about which approach we take here.
There was a problem hiding this comment.
thanks, it's good to know. Composing really looks code look nicer. To make it easier for other reviewers, I will just leave as it is now. Will do it in another PR if required.
pchaigno
left a comment
There was a problem hiding this comment.
@sayboras I think the Runtime-4.9 failure is relevant here:
# github.com/cilium/cilium/pkg/fqdn/dnsproxy [github.com/cilium/cilium/pkg/fqdn/dnsproxy.test]
pkg/fqdn/dnsproxy/proxy_test.go:202:3: cannot use func literal (type func(time.Time, *endpoint.Endpoint, string, string, *dns.Msg, string, bool, ProxyRequestContext) error) as type NotifyOnDNSMsgFunc in argument to StartDNSProxy
You can reproduce by running privileged unit tests locally.
Thanks for pointing out, it's my silly mistake. I didn't enable custom build tag in my Goland settings. :) |
58b8fbd to
c998fff
Compare
As mentioned in cilium#10991, successDuration was not safely access from eventqueue Add RWLock to make this struct thread-safe Using pointer for spanstat to avoid copylocks issue highlighted by govet Closes cilium#10991 Signed-off-by: Tam Mach <sayboras@yahoo.com>
c998fff to
db6872a
Compare
|
test-me-please |
|
@joestringer @pchaigno seems like all required jobs finished succesfully now, thanks for your inputs, and do let me know if anything else is required. |
As mentioned in #10991, successDuration was not safely access from
eventqueue
Add RWLock to make this struct thread-safe
Closes #10991
Signed-off-by: Tam Mach sayboras@yahoo.com