Fix flaky tests that use vtcombo

[GuptaManan100]

Description

Some tests like TestCanGetKeyspaces have been flaky. They occasionally end up panicking -

2024-06-13T08:34:17.3101964Z --- FAIL: TestCanGetKeyspaces (12.89s)
2024-06-13T08:34:17.3102209Z panic: runtime error: invalid memory address or nil pointer dereference [recovered]
2024-06-13T08:34:17.3102594Z 	panic: runtime error: invalid memory address or nil pointer dereference
2024-06-13T08:34:17.3102810Z [signal SIGSEGV: segmentation violation code=0x1 addr=0x18 pc=0x16d5060]
2024-06-13T08:34:17.3102825Z 
2024-06-13T08:34:17.3102924Z goroutine 957 [running]:
2024-06-13T08:34:17.3103056Z testing.tRunner.func1.2({0x1859800, 0x2d13550})
2024-06-13T08:34:17.3103371Z 	/opt/actions-runner/_work/_tool/go/1.22.4/x64/src/testing/testing.go:1631 +0x24a
2024-06-13T08:34:17.3103465Z testing.tRunner.func1()
2024-06-13T08:34:17.3103758Z 	/opt/actions-runner/_work/_tool/go/1.22.4/x64/src/testing/testing.go:1634 +0x377
2024-06-13T08:34:17.3103850Z panic({0x1859800?, 0x2d13550?})
2024-06-13T08:34:17.3104134Z 	/opt/actions-runner/_work/_tool/go/1.22.4/x64/src/runtime/panic.go:770 +0x132
2024-06-13T08:34:17.3104363Z vitess.io/vitess/go/cmd/vttestserver/cli.consumeEventStream({0x0, 0x0})
2024-06-13T08:34:17.3104756Z 	/opt/actions-runner/_work/vitess-private/vitess-private/go/cmd/vttestserver/cli/main_test.go:483 +0x80
2024-06-13T08:34:17.3105193Z vitess.io/vitess/go/cmd/vttestserver/cli.assertGetKeyspaces({_, _}, _, {{0xc000b87260, 0x0, {0x1af1cb9, 0xb}, {0x0, 0x0}, {0x0, ...}, ...}, ...})
2024-06-13T08:34:17.3105625Z 	/opt/actions-runner/_work/vitess-private/vitess-private/go/cmd/vttestserver/cli/main_test.go:469 +0x277
2024-06-13T08:34:17.3105861Z vitess.io/vitess/go/cmd/vttestserver/cli.TestCanGetKeyspaces(0xc00038a4e0)
2024-06-13T08:34:17.3106244Z 	/opt/actions-runner/_work/vitess-private/vitess-private/go/cmd/vttestserver/cli/main_test.go:254 +0x2b2
2024-06-13T08:34:17.3106386Z testing.tRunner(0xc00038a4e0, 0x1e42150)
2024-06-13T08:34:17.3106667Z 	/opt/actions-runner/_work/_tool/go/1.22.4/x64/src/testing/testing.go:1689 +0xfb
2024-06-13T08:34:17.3106791Z created by testing.(*T).Run in goroutine 1
2024-06-13T08:34:17.3107074Z 	/opt/actions-runner/_work/_tool/go/1.22.4/x64/src/testing/testing.go:1742 +0x390
2024-06-13T08:34:17.3107225Z FAIL	vitess.io/vitess/go/cmd/vttestserver/cli	61.087s

The problem was 2 fold. The first being, that when we setup a new cluster we only assert that we don't have an error, we don't require it. This causes the test to continue to run even when the cluster setup fails. We eventually end up using the cluster, which is nil (cause of the failure), and this panics the test.

So, the first thing we do is change all assert.Error calls to require.Error calls that setup a cluster using vtcombo.

Once we do this, we see the actual underlying error that fails these tests -

2024-06-14T06:09:31.9570978Z F0614 06:09:10.732810   38226 grpc_server.go:300] Cannot listen on port 29482 for gRPC: listen tcp :29482: bind: address already in use
2024-06-14T06:09:31.9571157Z     main_test.go:245: 
2024-06-14T06:09:31.9572114Z         	Error Trace:	/opt/actions-runner/_work//vitess/go/cmd/vttestserver/cli/main_test.go:245
2024-06-14T06:09:31.9572444Z         	Error:      	Received unexpected error:
2024-06-14T06:09:31.9573045Z         	            	process 'vtcombo' exited prematurely (err: exit status 1)
2024-06-14T06:09:31.9573324Z         	Test:       	TestCanGetKeyspaces
2024-06-14T06:09:31.9573554Z --- FAIL: TestCanGetKeyspaces (10.64s)

The problem is that we are choosing a random port for starting a cluster, and that port might already be in use by some other process. This PR fixes this issue by changing the logic of finding a random port to also verify that the port in question is also available for a tcp connection. If not, we retry and try a different random port.

I have verified that this fix indeed works, by 1 running the test multiple times, and 2 by ensuring that we correctly move away from ports that are already in use -

E0614 15:48:12.382166   84785 environment.go:243] Can't listen on port 100: listen tcp 127.0.0.1:100: bind: permission denied, trying next port
I0614 15:48:12.383906   84785 environment.go:248] Port 29622 is available, reserving..

Related Issue(s)

Checklist

• "Backport to:" labels have been added if this change should be back-ported to release branches
• If this change is to be back-ported to previous releases, a justification is included in the PR description
• Tests were added or are not required
• Did the new or modified tests pass consistently locally and on CI?
• Documentation was added or is not required

Deployment Notes

[vitess-bot]

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

• Ensure that the Pull Request has a descriptive title.
• Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

• Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

• Apply the release notes (needs details) label if users need to know about this change.
• New features should be documented.
• There should be some code comments as to why things are implemented the way they are.
• There should be a comment at the top of each new or modified test to explain what the test does.

New flags

• Is this flag really necessary?
• Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

• Each item in Jobs should be named in order to mark it as required.
• If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

• Protobuf changes should be wire-compatible.
• Changes to _vt tables and RPCs need to be backward compatible.
• RPC changes should be compatible with vitess-operator
• If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
• vtctl command output order should be stable and awk-able.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 68.55%. Comparing base (72149e2) to head (ee1f726).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #16178      +/-   ##
==========================================
- Coverage   68.57%   68.55%   -0.02%     
==========================================
  Files        1544     1544              
  Lines      197863   197873      +10     
==========================================
- Hits       135677   135651      -26     
- Misses      62186    62222      +36     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[arthurschreiber]

I'm not super familiar with this code, but couldn't we use port 0 for all these additional services (thus having the port be assigned on demand), and then store the port for further consumption once we know what the actual port is?

[dbussink]

I'm not super familiar with this code, but couldn't we use port 0 for all these additional services (thus having the port be assigned on demand), and then store the port for further consumption once we know what the actual port is?

Yeah, actually binding and then retrying or using 0 is the only real way to make it less flaky. Otherwise it will always still be racy since there's no way to guarantee it's still free when you try to bind.

But maybe something for a follow up PR? This change likely makes it significantly less flaky so it's at least an improvement but still there's a race possible.

[arthurschreiber]

I just checked, and apparently specifying port 0 for MySQL means it will fallback to 3306 according to the docs. 🙈 I know toxiproxy supports binding to a random port via 0, not sure how vtcombo behaves.

[arthurschreiber]

I agree this is a good first step though! 👍