🐛 Add race option to detect raced codes #10899

sivchari · 2024-07-18T09:28:35Z

What this PR does / why we need it:

I added -race option to go test command. This option can find the raced code.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sivchari · 2024-07-19T03:19:00Z

/assign

fabriziopandini · 2024-08-02T13:52:37Z

/area testing
/test pull-cluster-api-e2e-full-main

k8s-ci-robot · 2024-08-02T13:52:40Z

@fabriziopandini: The specified target(s) for /test were not found.
The following commands are available to trigger required jobs:

/test pull-cluster-api-build-main
/test pull-cluster-api-e2e-blocking-main
/test pull-cluster-api-e2e-conformance-ci-latest-main
/test pull-cluster-api-e2e-conformance-main
/test pull-cluster-api-e2e-main
/test pull-cluster-api-e2e-mink8s-main
/test pull-cluster-api-e2e-upgrade-1-30-1-31-main
/test pull-cluster-api-test-main
/test pull-cluster-api-test-mink8s-main
/test pull-cluster-api-verify-main

The following commands are available to trigger optional jobs:

/test pull-cluster-api-apidiff-main

Use /test all to run the following jobs that were automatically triggered:

pull-cluster-api-apidiff-main
pull-cluster-api-build-main
pull-cluster-api-e2e-blocking-main
pull-cluster-api-test-main
pull-cluster-api-verify-main

In response to this:

/area testing
/test pull-cluster-api-e2e-full-main

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

fabriziopandini · 2024-08-02T13:52:53Z

/test pull-cluster-api-e2e-main

fabriziopandini · 2024-08-02T13:53:39Z

Overall lgtm, running E2E to validate changes in the in memory provider

Makefile

chrischdi · 2024-08-05T07:26:24Z

/lgtm

k8s-ci-robot · 2024-08-05T07:26:30Z

LGTM label has been added.

Git tree hash: 5931a1618319937c284bd75a36f1709a484a6c7e

sbueringer · 2024-08-19T17:38:59Z

test/infrastructure/inmemory/pkg/runtime/cache/sync.go

@@ -50,10 +50,10 @@ func (c *cache) startSyncer(ctx context.Context) error {
 		c.syncQueue.ShutDown()
 	}()

-	syncLoopStarted := false
+	syncLoopStarted := make(chan struct{})


I think I would drop this entirely.

This now just checks that we got until l.56. I'm not sure I understand why we are waiting for that. At this point the only guarantee is that the log was written, which doesn't make sense

Sounds good to me. I fixed it.

sbueringer · 2024-08-19T17:40:40Z

@sivchari 2 smaller findings. Sorry for the misunderstanding here: #10899 (comment)

sivchari · 2024-08-20T01:03:46Z

/retest

sivchari · 2024-08-20T01:14:38Z

When I remove the syncLoopStarted, the other race errors are spawned.
I'd investigate later.

sbueringer · 2024-08-20T17:48:20Z

When I remove the syncLoopStarted, the other race errors are spawned. I'd investigate later.

Hm not sure why that is, but these errors look entirely unrelated (they are even in a different go module)

sivchari · 2024-09-24T12:01:01Z

@sbueringer
Sorry for too late reply and thank you so much for your efforts.
I updated this branch.
Perhaps a little speed reduction, but I think it's more important to detect race codes than its speed.
Again, thanks for your brilliant jobs.

sbueringer · 2024-09-24T12:25:15Z

@sivchari Can you re-add the -race flags to the test targets that don't have it already? So we can compare how muich longer the Job tests if we enable the race detector everywhere

sivchari · 2024-10-02T02:10:29Z

@sbueringer
Sorry, I was on vacation.

Can you re-add the -race flags to the test targets that don't have it already? So we can compare how muich longer the Job tests if we enable the race detector everywhere

I'm not sure what you mean. -race is already added on each task (e.g. test, test-junit). It's enough, isn't it ? If not, please teach me what you want to do.

sbueringer · 2024-10-02T05:31:25Z

No worries. -race is only set on some test targets. A previous version of this PR was setting it on all. I would like to set it on all test targets again that don't have it at the moment

sivchari · 2024-10-04T04:26:04Z

You mean, you want to remove !race tag in all test targets, right ?

sbueringer · 2024-10-04T06:47:47Z

No, I want to add -race to all test Makefile targets that don't have it yet. For example test-docker-infrastructure

A previous version of your PR already had it correctly on all targets. My PR only added it to the most important test targets

sivchari · 2024-10-07T09:26:07Z

Okay, I got it. Sorry for taking your time.
I'd take care of it by the end of this week.

sbueringer · 2024-10-08T05:14:03Z

Thx! No problem at all and no rush! 😀

Signed-off-by: sivchari <[email protected]>

sivchari · 2024-10-13T15:54:41Z

I re-added the -race to each target

sbueringer · 2024-10-14T08:51:14Z

/test pull-cluster-api-test-main

sbueringer · 2024-10-19T12:19:58Z

test/infrastructure/inmemory/pkg/runtime/cache/sync.go

@@ -50,10 +50,8 @@ func (c *cache) startSyncer(ctx context.Context) error {
 		c.syncQueue.ShutDown()
 	}()



@sivchari I took another look. Let's keep this but make it concurrency safe
l.53

var syncLoopStarted atomic.Bool

l.56

syncLoopStarted.Store(true)

l.85

if !syncLoopStarted.Load() {

@SubhasmitaSw
Thank you for commenting about it. Surely, it might be right, but I think it's not necessary. In l.83, we check if all workers starts by atomic.Load(&workers) < int64(c.syncConcurrency) and I believe it's enough to achieve to block data race. Thanks.

The check in l.83++ does not include the syncloop (l.53-l.63)

Thanks you for your comment. Sorry, I mentioned codes after changing.

syncLoopStarted is only used in codes below

go func() { log.Info("Starting sync loop") syncLoopStarted = true for { select { case <-time.After(c.syncPeriod / 4): c.syncGroup(ctx) case <-ctx.Done(): return } } }()

if err := wait.PollUntilContextTimeout(ctx, 50*time.Millisecond, 5*time.Second, false, func(context.Context) (done bool, err error) { if !syncLoopStarted { return false, nil } return true, nil }); err != nil { return fmt.Errorf("failed to start sync loop: %v", err) }

c.syncGroup is originally concurrency program since using goroutine. So it depends on the timing of Go runtime to change syncLoopStarted from false to true. In other words, we don't originally consider the timing when this variable is true. Thus, I think the syncLoopStarted is unnecessary. We can keep concurrency safe here.
What do you think ?

I'm not sure I understand what you are saying. Yes the old code was not correctly waiting for syncLoopStarted to be true. So it might not be that important. But I would prefer cleanly waiting for the "syncLoop" go routine to be started compared to not waiting for it.

What I wanted to say is that syncLoopStarted might we can be worth removing. But when I confirmed your previous comment, I'd decide to keep this variable while changing the type to atomic.Bool. Thanks.

Signed-off-by: sivchari <[email protected]>

sbueringer · 2024-10-23T08:51:09Z

Thx!

/lgtm
/approve

k8s-ci-robot · 2024-10-23T08:51:16Z

LGTM label has been added.

Git tree hash: 17be7766e93a71470cfcbe0052cc592aaf013aa4

k8s-ci-robot · 2024-10-23T08:51:18Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sbueringer

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [sbueringer]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jul 18, 2024

k8s-ci-robot requested review from fabriziopandini and richardcase July 18, 2024 09:28

k8s-ci-robot assigned sivchari Jul 19, 2024

k8s-ci-robot added the area/testing Issues or PRs related to testing label Aug 2, 2024

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Aug 2, 2024

sbueringer reviewed Aug 2, 2024

View reviewed changes

Makefile Outdated Show resolved Hide resolved

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 3, 2024

k8s-ci-robot assigned chrischdi Aug 5, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 5, 2024

sbueringer reviewed Aug 19, 2024

View reviewed changes

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 20, 2024

k8s-ci-robot requested a review from chrischdi August 20, 2024 00:07

sivchari force-pushed the add-race-option branch from 3d82fb9 to 567e04f Compare August 27, 2024 06:57

sivchari force-pushed the add-race-option branch from ddd99a1 to a832abf Compare September 24, 2024 11:59

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Sep 24, 2024

sivchari added 5 commits October 14, 2024 00:28

fix: race code

57995c9

Signed-off-by: sivchari <[email protected]>

remove sync variable

1b505e3

Signed-off-by: sivchari <[email protected]>

use sync chan

7cc8b28

Signed-off-by: sivchari <[email protected]>

drop chan

02c8ae0

Signed-off-by: sivchari <[email protected]>

add -race option again

4d91c48

Signed-off-by: sivchari <[email protected]>

sivchari force-pushed the add-race-option branch from caa47dc to 4d91c48 Compare October 13, 2024 15:31

sbueringer reviewed Oct 19, 2024

View reviewed changes

use atomic package

6ce1b66

Signed-off-by: sivchari <[email protected]>

k8s-ci-robot assigned sbueringer Oct 23, 2024

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 23, 2024

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 23, 2024

k8s-ci-robot merged commit 214ab6d into kubernetes-sigs:main Oct 23, 2024
18 checks passed

k8s-ci-robot added this to the v1.9 milestone Oct 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🐛 Add race option to detect raced codes #10899

🐛 Add race option to detect raced codes #10899

sivchari commented Jul 18, 2024

sivchari commented Jul 19, 2024

fabriziopandini commented Aug 2, 2024

k8s-ci-robot commented Aug 2, 2024

fabriziopandini commented Aug 2, 2024

fabriziopandini commented Aug 2, 2024

chrischdi commented Aug 5, 2024

k8s-ci-robot commented Aug 5, 2024

sbueringer Aug 19, 2024

sivchari Aug 20, 2024

sbueringer commented Aug 19, 2024

sivchari commented Aug 20, 2024

sivchari commented Aug 20, 2024

sbueringer commented Aug 20, 2024 •

edited

Loading

sivchari commented Sep 24, 2024 •

edited

Loading

sbueringer commented Sep 24, 2024

sivchari commented Oct 2, 2024

sbueringer commented Oct 2, 2024

sivchari commented Oct 4, 2024

sbueringer commented Oct 4, 2024 •

edited

Loading

sivchari commented Oct 7, 2024

sbueringer commented Oct 8, 2024

sivchari commented Oct 13, 2024

sbueringer commented Oct 14, 2024

sbueringer Oct 19, 2024

sivchari Oct 22, 2024

sbueringer Oct 22, 2024 •

edited

Loading

sivchari Oct 22, 2024

sbueringer Oct 22, 2024 •

edited

Loading

sivchari Oct 23, 2024

sbueringer commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

		@@ -50,10 +50,8 @@ func (c *cache) startSyncer(ctx context.Context) error {
		c.syncQueue.ShutDown()
		}()

🐛 Add race option to detect raced codes #10899

🐛 Add race option to detect raced codes #10899

Conversation

sivchari commented Jul 18, 2024

sivchari commented Jul 19, 2024

fabriziopandini commented Aug 2, 2024

k8s-ci-robot commented Aug 2, 2024

fabriziopandini commented Aug 2, 2024

fabriziopandini commented Aug 2, 2024

chrischdi commented Aug 5, 2024

k8s-ci-robot commented Aug 5, 2024

sbueringer Aug 19, 2024

Choose a reason for hiding this comment

sivchari Aug 20, 2024

Choose a reason for hiding this comment

sbueringer commented Aug 19, 2024

sivchari commented Aug 20, 2024

sivchari commented Aug 20, 2024

sbueringer commented Aug 20, 2024 • edited Loading

sivchari commented Sep 24, 2024 • edited Loading

sbueringer commented Sep 24, 2024

sivchari commented Oct 2, 2024

sbueringer commented Oct 2, 2024

sivchari commented Oct 4, 2024

sbueringer commented Oct 4, 2024 • edited Loading

sivchari commented Oct 7, 2024

sbueringer commented Oct 8, 2024

sivchari commented Oct 13, 2024

sbueringer commented Oct 14, 2024

sbueringer Oct 19, 2024

Choose a reason for hiding this comment

sivchari Oct 22, 2024

Choose a reason for hiding this comment

sbueringer Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

sivchari Oct 22, 2024

Choose a reason for hiding this comment

sbueringer Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

sivchari Oct 23, 2024

Choose a reason for hiding this comment

sbueringer commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

k8s-ci-robot commented Oct 23, 2024

sbueringer commented Aug 20, 2024 •

edited

Loading

sivchari commented Sep 24, 2024 •

edited

Loading

sbueringer commented Oct 4, 2024 •

edited

Loading

sbueringer Oct 22, 2024 •

edited

Loading

sbueringer Oct 22, 2024 •

edited

Loading