Add initial support for capabilities

[morgo]

This eliminates the need to set MYSQL_FLAVOR. I added it to go/vt/ rather than go/mysql/ based on that it is very mysqld (and mysqlctl) centric.

There is an existing flavor-specific system for connections in go/mysql, but it branches between mysqld and mariadb. I looked at opportunities to merge the two, but this system is more about capabilities and differences between versions for bootstrapping binaries in mysqlctl. It also has a chicken and egg problem: the connection system requires a database installed, and this system uses capabilities to figure out how it should be installed (there is no database yet).

Fixes #4998

Signed-off-by: Morgan Tocker tocker@gmail.com

[morgo]

Looking forward to your feedback!

cc @dkhenry @jvaidya @enisoc

[morgo]

I have changed the design slightly since the previous behavior broke backward compatibility: now MYSQL_FLAVOR is ignored completely.

The problem was that MYSQL56 was used to represent any MySQL version (5.6, 5.7, 8.0). The flavor system was using this too literally, and thus it tried to run mysql_install_db on MySQL 5.7 and 8.0.

I have changed Percona Server to include "mysql" config files, and tested on Percona Server/MySQL from 5.6-8.0. For MariaDB I tested 10.0-10.4. 10.4 breaks, but I believe this issue is pre-existing.

The idea to allow MYSQL_FLAVOR to take precedence was based on feedback @jvaidya provided, where he was concerned that the version-detection might be excessively magic. But given that allowing it makes backward compatibility hard, my suggestion is that it just be eliminated.

[enisoc]

This is the first time I've seen a capabilities implementation, so forgive me if this is standard practice for reasons I don't understand, but at a high level I have doubts about the use of constants here. I'll describe this high level concern in this top-level thread. The rest of the line comments are assuming we keep the constants.

Currently, the only way we're using the Capability constants is in hard-coded calls to HasCapability(foo) which simply selects a particular case from this switch. If this is the way we expect capabilities to be used, this switch statement seems like an unnecessarily roundabout way to allow callers to select a specific code path. We could instead just define a function for each capability, perhaps as a method on a Capabilities object.

That is, instead of mysqld.HasCapability(MySQLUpgradeInServer), the call site would be mysqld.Capabilities.HasMySQLUpgradeInServer(). That gets rid of all the constants and avoids the need to worry about invalid values being passed in (default case below), as well as worrying about whether someone will persist capability constants and reuse them against different versions of the code (see below).

If you had planned to use these capability constants in other, more sophisticated ways later, I could see why they would be needed. For example, do you want to allow serialization and transmission of capabilities over the wire? Or even just dealing with lists of capabilities in-memory? If so, we should talk about what those future use cases are, and also considerations like ensuring stability of the constants across code changes. If not, it feels to me like the complexity of the enumeration+switch is unnecessary.

[enisoc]

I have changed the design slightly since the previous behavior broke backward compatibility: now MYSQL_FLAVOR is ignored completely.

The problem was that MYSQL56 was used to represent any MySQL version (5.6, 5.7, 8.0).

Oops I just submitted a review based on the code before this change. Before we drop MYSQL_FLAVOR, we need to be careful we don't break support for the use case in which mysqld is not available locally at all, or at least if it's there, it's not necessarily the same version we're connecting to. This happens when vttablet is run against a remote mysqld, which is very common, especially as part of bootstrapping into Vitess in the first place.

Ideally, we can detect the version of a remote mysqld by connecting to it and asking it, in that case. The go/mysql package can already do that, I think. The only reason we are using mysqld --version here is because in some cases we need to know the version before we start mysqld (e.g. to know which .cnf files to include).

My hope is that we can prove that the code paths that require mysqld --version detection are never used in the case of remote mysqld. At first glance, I would expect this to be true, but we should verify it.

[deepthi]

@morgo I think it will be good to create an issue that outlines the problem and the proposed solution. Once any comments on the design are addressed, it will be much easier to review the PR.

[morgo]

@morgo I think it will be good to create an issue that outlines the problem and the proposed solution. Once any comments on the design are addressed, it will be much easier to review the PR.

That is a good idea. I am going to push some of the config changes prior (described in #4990 ), and then after this PR. It will reduce the size and scope.

[morgo]

I didn't realize I had not finished the detectVersion refactoring when I pushed. Sorry, don't review for now.. I will update soon.

[morgo]

Oops I just submitted a review based on the code before this change. Before we drop MYSQL_FLAVOR, we need to be careful we don't break support for the use case in which mysqld is not available locally at all, or at least if it's there, it's not necessarily the same version we're connecting to. This happens when vttablet is run against a remote mysqld, which is very common, especially as part of bootstrapping into Vitess in the first place.

I chatted with @sougou about this, and it shouldn't be a problem. mysqlctl is used for local mysqld management, so in the case there is no local mysqld, there is no mysqlctl.

[morgo]

@enisoc followup ping. Many changes, looking for your review :-)

[sougou]

Responding to @enisoc's comment about hiding the capabilities using separate implementations:

I looked at how these capabilities were used. I think moving to separate implementations will be too big of a leap from where we are today. The capabilities approach allows us to replace existing conditional logic to use these abstracted out functions.

In other words, it feels like there will be a lot of shared code between different implementations, and will require us to refactor the common parts out.

So, I think this is a good starting point for this.

[enisoc]

@morgo wrote:

I chatted with @sougou about this, and it shouldn't be a problem. mysqlctl is used for local mysqld management, so in the case there is no local mysqld, there is no mysqlctl.

Even though no one uses the mysqlctl or mysqlctld binaries in that case, we still call into the mysqlctl package (via mysqlctl.Mysqld) in various places. Those places that don't depend on a local mysqld ought to all use the flavor detection in the go/mysql package, which works for remote mysqld because it uses the live connection metadata. However, we don't have proof that this is true since the separation is not completely clean.

With that said, I'm willing to trust the tests on this. And if remote mysqld isn't tested, then that feature doesn't actually exist IMO.

@sougou wrote:

Responding to @enisoc's comment about hiding the capabilities using separate implementations

I wasn't suggesting separate implementations. There's only one, concrete implementation; no interfaces. I just suggested making separate functions instead of one big function with a switch, since we don't foresee the need to treat capabilities as data. It seems like @morgo was ok with that suggestion, so we're good.

[enisoc]

Sorry for the delay. I went on vacation and then got swamped by PS stuff when I got back.

Looking good overall! Mostly just style comments and some error-handling changes.

[enisoc]

I'm still worried about the error handling in NewMysqld. Let me know if my reasoning isn't clear. We can set up a call to discuss it.

[enisoc]

Looks good overall. Just some final style nits. Thanks for being patient with the long review process.

[enisoc]

LGTM

[deepthi]

Nice work!