Improve efficiency and accuracy of mysqld.GetVersionString

[mattlord]

Description

The following vitess binaries call the tabletmanager FullStatus RPC:

• vtorc: It runs it via refreshAllTablets() every 15 seconds by default
• vtadmin: It runs it on the tablet view pages (I believe with a regular auto refresh)
• vtctldclient: Any time the GetFullStatus command is used

In the vtorc (for sure) and vtadmin (I think) cases, it's run automatically and periodically. So over time, this RPC can be called many thousands of times on a tablet. And this RPC is merely one thing that ends up calling the VtMySQLRoot() function. This function was blindly prepending /usr/sbin: to the PATH and updating the environment variable every time it was called when VT_MYSQL_ROOT was not set:

vitess/go/vt/env/env.go

Lines 69 to 79 in 3147240

	func VtMysqlRoot() (string, error) {
	// if the environment variable is set, use that
	if root := os.Getenv("VT_MYSQL_ROOT"); root != "" {
	return root, nil
	}
	// otherwise let's look for mysqld in the PATH.
	// ensure that /usr/sbin is included, as it might not be by default
	// This is the default location for mysqld from packages.
	newPath := fmt.Sprintf("/usr/sbin:%s", os.Getenv("PATH"))
	os.Setenv("PATH", newPath)

In the case of the FullStatus RPC, it ends up there via:

1. vitess/go/vt/vttablet/tabletmanager/rpc_replication.go

   Lines 95 to 96 in 3147240

   	// Version string "majorVersion.minorVersion.patchRelease"
   	version, err := tm.MysqlDaemon.GetVersionString(ctx)

2. vitess/go/vt/mysqlctl/mysqld.go

   Line 1188 in 3147240

   func (mysqld *Mysqld) GetVersionString(ctx context.Context) (string, error) {

3. vitess/go/vt/mysqlctl/mysqld.go

   Lines 192 to 195 in 3147240

   	// GetVersionString runs mysqld --version and returns its output as a string
   	func GetVersionString() (string, error) {
   	noSocketFile()
   	mysqlRoot, err := vtenv.VtMysqlRoot()

This ends up making a sys exec call to execute mysqld --version:

vitess/go/vt/mysqlctl/mysqld.go

Line 203 in 3147240

_, version, err := execCmd(mysqldPath, []string{"--version"}, nil, mysqlRoot, nil)

And every time we do this the PATH env var grows by 11 bytes as /usr/sbin: is prepended to it and saved in the env every time. When we exec the mysqld command it inherits a copy-on-write copy of the vttablet's env (with the ever growing PATH value) — because we set cmd.Env to nil. Eventually this will cause the execve call to fail with E2BIG (Argument list too long) as the the arguments and env size count against the ARG_MAX OS limit.

If you want to try and repeat it yourself, you can do so using this manual test: #15095 (comment)

I think that we should at least backport this to v18 since we expect everyone to be using vtorc, vtadmin, and vtctldclient.

Related Issue(s)

• Fixes: Bug Report: GetFullStatus results in E2BIG #15095

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

[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

Attention: 7 lines in your changes are missing coverage. Please review.

Comparison is base (ebf0106) 47.70% compared to head (4cd461a) 47.79%.
Report is 6 commits behind head on main.

Files	Patch %	Lines
go/vt/mysqlctl/mysqld.go	0.00%	6 Missing ⚠️
go/vt/env/env.go	93.33%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15096      +/-   ##
==========================================
+ Coverage   47.70%   47.79%   +0.09%     
==========================================
  Files        1155     1155              
  Lines      240222   240319      +97     
==========================================
+ Hits       114596   114865     +269     
+ Misses     117021   116882     -139     
+ Partials     8605     8572      -33     

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

[GuptaManan100]

I know this is a draft right now, but the changes look good to me! Thank-you for fixing this.

[deepthi]

I'm sure there's a good reason you changed this, but it doesn't feel necessary.

[mattlord]

Just so that we're using the same const in the regex and where we build the string to match it when querying mysqld.