test(datarace): Fix datarace issue in spanstat.go

[sayboras]

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

Fix datarace issue in spanstat.go

[maintainer-s-little-helper]

Please set the appropriate release note label.

[joestringer]

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.

[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.

@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)

[aanm]

one small non-blocking nit

[pchaigno]

test-me-please

[coveralls]

Coverage increased (+0.02%) to 36.922% when pulling db6872a on sayboras:bugfix/span-unsafe into 3da5bcf on cilium:master.

[christarazi]

Nice!

[joestringer]

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.

[joestringer]

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.

[sayboras]

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]

@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.

[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. :)

[joestringer]

test-me-please

[sayboras]

@joestringer @pchaigno seems like all required jobs finished succesfully now, thanks for your inputs, and do let me know if anything else is required.