Flakes: Address Common Unit Test Races #12546

mattlord · 2023-03-03T03:43:26Z

Description

The unit race workflow was failing pretty regularly in the CI (~20% of the time), and the two failures that I would regularly see were:

testVDiffEnv related races — most often in the wrangler unit tests, as they have the widest use
- These were simply NOT concurrency-safe
glog.MaxSize related races — most often in the wrangler unit tests as they create a LOT of loggers, primarily ConsoleLoggers and MemoryLoggers
- The way we modified the glog.MaxSize global exported variable in our --log_rotate_max_size flag handling was not concurrency-safe

I was able to quickly repeat these failures locally as well using the method below. After the changes in this PR, I was able to run it successfully 100 times in a row:

cnt=1
while true; do
    echo -n "Attempt# ${cnt} :: "
    go test -timeout 30s -parallel 6 -count 1 -race vitess.io/vitess/go/vt/wrangler 2>>/tmp/results
    if [[ $? -ne 0 ]]; then
         echo "Failed on attempt ${cnt}"
         break
    fi
    let cnt+=1
    sleep 1
done

Attempt# 1 :: ok  	vitess.io/vitess/go/vt/wrangler	9.320s
...
Attempt# 100 :: ok  	vitess.io/vitess/go/vt/wrangler	9.503s

It was failing in the CI about 1 in 4-5 attempts. In this PR it passed 19 out of 20 times, 18 times in a row — which is at least a big improvement.

Related Issue(s)

Checklist

"Backport to:" labels have been added if this change should be back-ported
Tests were added or are not required
Did the new or modified tests pass consistently locally and on the CI
Documentation was added or is not required

Signed-off-by: Matt Lord <[email protected]>

vitess-bot · 2023-03-03T03:43:29Z

Signed-off-by: Matt Lord <[email protected]>

go/vt/log/log.go

dbussink · 2023-03-05T07:21:54Z

go/vt/log/log.go

@@ -78,5 +81,32 @@ var (
 // calls this function, or call this function directly before parsing
 // command-line arguments.
 func RegisterFlags(fs *pflag.FlagSet) {
-	fs.Uint64Var(&glog.MaxSize, "log_rotate_max_size", glog.MaxSize, "size in bytes at which logs are rotated (glog.MaxSize)")
+	flagVal := logRotateMaxSize{
+		val: "1887436800", // glog.MaxSize value (which is not concurrency safe)


Instead of copying the value here, can't we use glob.MaxSize itself here? Maybe with an atomic load then I guess?

oh, lol, come on GitHub PR UI #12546 (comment)

so, uhh, yeah I agree with @dbussink here 😂

That was the first thing I tried and it helped but not as much (failed every 20-30 times locally IIRC). I could go back to that though.

Giving it a go in the CI now (will do 20 test runs again): 0c5be06

IIRC, the race moved to pflag.Parse/TryParse around flag.TrickGlog but let's see.

Good news is that we don't seem to lose much if any reliability in the CI with that change: https://github.com/vitessio/vitess/actions/runs/4336855297/attempts/19

So I think we all feel better about it now. 🙂 Thanks!

Signed-off-by: Matt Lord <[email protected]>

ajm188

LGTM, one small (potential) improvement, take it or leave it! and thanks!

ajm188 · 2023-03-06T11:48:48Z

go/vt/vtctl/vdiff_env_test.go

@@ -94,6 +80,17 @@ func newTestVDiffEnv(sourceShards, targetShards []string, query string, position
 	}
 	env.wr = wrangler.NewTestWrangler(env.cmdlog, env.topoServ, env.tmc)

+	dialerName := fmt.Sprintf("VDiffTest-%d", rand.Intn(1000000000))


if we update this to take a *testing.T (or better, testing.TB), you can parameterize this to fmt.Sprintf("VDiffTest-%s", t.Name()) and avoid any chance of collision entirely

Good idea! I did that here (kept the rand int part for too): 2d7e509

Thanks!

go/vt/wrangler/wrangler_env_test.go

Signed-off-by: Matt Lord <[email protected]>

* Deflake unit race tests Signed-off-by: Matt Lord <[email protected]> * Try to address glog.MaxSize race Signed-off-by: Matt Lord <[email protected]> * Test w/o using literal copy of glog.MaxSize value in CI Signed-off-by: Matt Lord <[email protected]> * Make the dialer names truly unique Signed-off-by: Matt Lord <[email protected]> --------- Signed-off-by: Matt Lord <[email protected]>

* Flakes: Address Common Unit Test Races (#12546) * Deflake unit race tests Signed-off-by: Matt Lord <[email protected]> * Try to address glog.MaxSize race Signed-off-by: Matt Lord <[email protected]> * Test w/o using literal copy of glog.MaxSize value in CI Signed-off-by: Matt Lord <[email protected]> * Make the dialer names truly unique Signed-off-by: Matt Lord <[email protected]> --------- Signed-off-by: Matt Lord <[email protected]> * Use atomic.Bool for fakesqldb behavior flags (#12603) Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Max Englander <[email protected]> Co-authored-by: Matt Lord <[email protected]> --------- Signed-off-by: Matt Lord <[email protected]> Signed-off-by: Max Englander <[email protected]> Co-authored-by: Matt Lord <[email protected]> Co-authored-by: Max Englander <[email protected]>

Deflake unit race tests

7ccadf1

Signed-off-by: Matt Lord <[email protected]>

vitess-bot bot added NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 3, 2023

mattlord added Component: Build/CI Type: Testing Flakes and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says labels Mar 3, 2023

mattlord force-pushed the flakes_unit_test_race branch from a662305 to 89e142b Compare March 3, 2023 15:18

Try to address glog.MaxSize race

8a63f38

Signed-off-by: Matt Lord <[email protected]>

mattlord force-pushed the flakes_unit_test_race branch from 89e142b to 8a63f38 Compare March 3, 2023 15:39

mattlord marked this pull request as ready for review March 5, 2023 01:05

mattlord requested review from deepthi, rohit-nayak-ps and ajm188 as code owners March 5, 2023 01:06

mattlord requested review from dbussink and GuptaManan100 March 5, 2023 01:06

mattlord commented Mar 5, 2023

View reviewed changes

go/vt/log/log.go Outdated Show resolved Hide resolved

dbussink approved these changes Mar 5, 2023

View reviewed changes

Test w/o using literal copy of glog.MaxSize value in CI

0c5be06

Signed-off-by: Matt Lord <[email protected]>

ajm188 approved these changes Mar 6, 2023

View reviewed changes

Make the dialer names truly unique

2d7e509

Signed-off-by: Matt Lord <[email protected]>

mattlord merged commit ef28c13 into vitessio:main Mar 6, 2023

mattlord deleted the flakes_unit_test_race branch March 6, 2023 18:14

rohit-nayak-ps mentioned this pull request Mar 28, 2023

[release-16.0] Port two flaky test fixes #12603 and #12546 #12745

Merged

4 tasks

mattlord mentioned this pull request May 16, 2023

Flakes: vreplication unit test flakiness #13093

Closed

hmaurer mentioned this pull request Mar 21, 2024

oops #15542

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Flakes: Address Common Unit Test Races #12546

Flakes: Address Common Unit Test Races #12546

mattlord commented Mar 3, 2023 •

edited

Loading

vitess-bot bot commented Mar 3, 2023 •

edited by dbussink

Loading

dbussink Mar 5, 2023

ajm188 Mar 5, 2023

mattlord Mar 5, 2023 •

edited

Loading

mattlord Mar 5, 2023

mattlord Mar 6, 2023

ajm188 left a comment

ajm188 Mar 6, 2023

mattlord Mar 6, 2023

Flakes: Address Common Unit Test Races #12546

Flakes: Address Common Unit Test Races #12546

Conversation

mattlord commented Mar 3, 2023 • edited Loading

Description

Related Issue(s)

Checklist

vitess-bot bot commented Mar 3, 2023 • edited by dbussink Loading

Review Checklist

General

If a new flag is being introduced:

If a workflow is added or modified:

Bug fixes

Non-trivial changes

New/Existing features

Backward compatibility

dbussink Mar 5, 2023

Choose a reason for hiding this comment

ajm188 Mar 5, 2023

Choose a reason for hiding this comment

mattlord Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

mattlord Mar 5, 2023

Choose a reason for hiding this comment

mattlord Mar 6, 2023

Choose a reason for hiding this comment

ajm188 left a comment

Choose a reason for hiding this comment

ajm188 Mar 6, 2023

Choose a reason for hiding this comment

mattlord Mar 6, 2023

Choose a reason for hiding this comment

mattlord commented Mar 3, 2023 •

edited

Loading

vitess-bot bot commented Mar 3, 2023 •

edited by dbussink

Loading

mattlord Mar 5, 2023 •

edited

Loading