Add percona support, add kitchen and tests with serverspec #14

aalmenar · 2016-11-02T14:41:31Z

Add percona support, add kitchen and tests with serverspec and integrate pull request /pull/13 with some changes to include percona and versions of databases.

chr4

Thanks for the pull-request! Aside from the small change requests, this looks good to me!

chr4 · 2016-11-07T13:10:41Z

providers/password.rb

@@ -21,7 +21,7 @@

 action :set do
  r = execute "Assign mysql password for #{new_resource.user} user" do
-    query = "UPDATE user SET authentication_string = PASSWORD('#{Shellwords.escape(new_resource.password)}') WHERE User = '#{Shellwords.escape(new_resource.user)}'"
+    query = "UPDATE user SET #{node['mysqld']['pwd_col']} = PASSWORD('#{Shellwords.escape(new_resource.password)}') WHERE User = '#{Shellwords.escape(new_resource.user)}'"


Please use Shellwords.escape()

chr4 · 2016-11-07T13:10:43Z

attributes/defaults.rb

+    default['mysqld']['pwd_col'] = 'authentication_string'
+  end
+else
+  default['mysqld']['pwd_col'] = 'authentication_string'


Is the else really required? Unless mariadb, percona or mysql was set, we're going to have some trouble later on anyway, won't we?

You're right, will cleanup an commit again.

chr4 · 2016-11-07T13:10:46Z

attributes/defaults.rb

+    default['mysqld']['pwd_col'] = 'authentication_string'
+  end
+when 'mysql'
+  if node['platform'] == 'debian' && node['platform_version'].to_f < 9.0


It seems like we should add an attribute node['mysqld']['mysql']['version'] analogue to the percona configuration and check for the version instead of node['platform_version']. The version attribute can then be defaulted for a specific platform.

Sounds better, this was only to use the right column when setting the password and using default distro packages, in this case, debian jessie comes with mysql 5.5, and stretch at the moment with 5.6 which i believe will end having 5.7 which support authentication_string.

Using chef_sugar maybe does it make it more readable, but adds a dependency.

With the changes proposed above, I was thinking about something like this:

case node['mysqld']['db_install'] when 'mariadb' default['mysqld']['pwd_col'] = 'Password' when 'percona' if node['mysqld']['percona']['version'] < 5.7 default['mysqld']['pwd_col'] = 'Password' else default['mysqld']['pwd_col'] = 'authentication_string' end when 'mysql' if node['mysqld']['mysql']['version'] < 5.7 default['mysqld']['pwd_col'] = 'Password' else default['mysqld']['pwd_col'] = 'authentication_string' end end

Then have something like this ?

if node['platform'] == 'debian' && node['platform_version'].to_i < 10 # At this time, stretch still uses mysql 5.6 default['mysqld']['mysql']['version'] = 5.6 elsif node['platform'] == 'ubuntu' && node['platform_version'].to_f < 16.04 default['mysqld']['mysql']['version'] = 5.6 else default['mysqld']['mysql']['version'] = 5.7 end

Running kitchen tests right now, if everything goes smoothly will commit this change.

chr4 · 2016-11-07T13:10:49Z

attributes/defaults.rb

+default['mysqld']['repository']['percona']['mirror'] = 'http://repo.percona.com/apt'
+
+if node['mysqld']['repository']['percona']['version'] == '5.7'
+  default['mysqld']['percona_packages'] = %w(percona-server-server-5.7)


I'd suggest moving the package attributes percona_packages mariadb_packages and mariadb_galera_packages into a sub-attribute, like: default['mysqld']['mariadb']['packages']. Then we'd have all the repository-related configuration in one hash.

Fine, this change its better, but also will move attributes of repositories to that hierarchy.

I think we should also migrate the repository attributes to be consistent with the new hierarchy:

default['mysqld']['mariadb']['version'] = 10.1 # We shouldn't use a string here, I suppose default['mysqld']['mariadb']['repository'] = 'http://ftp.hosteurope.de/mirror/mariadb.org/repo'

The version attribute can then be used later to determine the correct pwd_col.

aalmenar · 2016-11-07T14:22:46Z

All requested changes have been commited but the one about using platform_version.

aalmenar · 2016-11-07T15:43:49Z

Everything went ok. Any other suggestions ?

chr4 · 2016-11-07T16:07:08Z

I've just encountered a problem, and I don't have a simple/ clean solution for it.

When releasing 2.0.0, I've added a note to the README on compatibility, which says that users of < mysql-5.7.6 should version-lock the cookbook to 1.0.5. This was due to the fact that the authentication_string column was introduced with mysql-5.7.6, and not with a major relase change. Therefore, just checking for < 5.7 is not enough. In my opinion, adding a check for the minor-version used is kind of an overkill.

I'm sorry to find this out after you fixed some issues, but I think that best for maintenance is to only support recent mysql/ mariadb/ percona versions that actually use authentication_string (basically mariadb and percona/ mysql since 5.7.6).

Would that still work for you?

aalmenar · 2016-11-07T16:11:01Z

Mysql 5.7 but lower than 5.7.6 is not found unless installed by hand specifically on the repositories affected. The changes introduced bring support to avoid version locking the cookbook to 1.0.5 since the problem was basically the authentication_string column.

aalmenar · 2016-11-07T16:18:15Z

In percona apt repository, you can't find something lower than 5.7.11.

On Debian jessie you will find 5.5, and there is no way to install 5.7 automatically using this recipe. At this time, stretch which is debian 9.0 has mysql 5.6 at this time, unless it's updated with sid packages, then it will upgrade to 5.7.15, so no issues other than changing the mysql-server version supported on this distro.

I don't think this will be an issue in the foreseeable future.

chr4 · 2016-11-07T16:21:53Z

So you'd say even though the check for < 5.7 is not entirely correct, it should be a non-issue in practise?

I think there's more to it. When I was adding support for 5.7, I've adjusted the default settings in this commit, see also this comment for further reference.

While it is surely possible to adjust them manually, my advise to users of Debian therefore would be to use MariaDB from the provided repositories. Databases are often used for critical data, and I personally what to have safe defaults for the supported versions. I'm aware, that this leaves Debian users that don't want to use third-party repositories (or don't want MariaDB) out, but I think this is the best compromise currently.

aalmenar · 2016-11-07T16:25:14Z

Ok, but that it's another issue, not the thing addressed because mysql was <5.7.6 and the Password was stored on Password field and not authentication_string.

Those can be addapted to handle 5.6 changes. But afaik when you run the tests with kitchen the database starts and works. Therefore i dont see it causing any issues.

chr4 · 2016-11-07T16:28:35Z

Here's my suggestion: We split this pull-request up into two:

Percona support (which looks good to me)
Add support for versions < 5.7.6

Then we can discuss pros and cons for backwards compatibility seperately. Sounds good?

aalmenar · 2016-11-07T16:29:34Z

+1

aalmenar · 2016-11-07T16:56:55Z

How would you like to do this ? close this pull request and open 2 separate ones ?

chr4 · 2016-11-07T17:06:06Z

Whatever is easiest for you. You can also remove the 5.6 part and force-push if this is more convenient.

aalmenar · 2016-11-07T17:32:59Z

Done

chr4

Besides the README, this looks good to me!

chr4 · 2016-11-07T18:23:52Z

attributes/defaults.rb

-# Attribute that defines whether MariaDB or MySQL should be used
-default['mysqld']['use_mariadb'] = true
+# Attribute that defines whether MariaDB, MySQL or Percona should be used
+default['mysqld']['db_install'] = 'mariadb'


The only thing missing now is a note in the README explaining the use of the db_install attribute.

Im seeing right now, the repository configuration must be included inside install to avoid asking the user to include the repository configuration, with percona this is necessary but i have my doubts with mariadb.

Can you elaborate? I'm not sure what you mean.

Ok.

Percona 5.6 is included on ubuntu 16.04, but it will work since we require mysql 5.7 at least. So, include_recipe 'mysqld::percona_repository' needs to be included, but its a hassle since you already configured an attribute to use percona.

The same happens with mariadb, to ensure the version we want, we must include mariadb.org repositories to avoid this failing if you select version 10.2 and using default xenial repositories.

Ah i understand. Vor MariaDB, this is solved as the default recipe includes the apt_repository recipe, too. So, while you can use the cookbook without the apt repository, the default "just works". For percona, you're right. One option would be to just leave it as it is (and add a warning to the README), or automatically include the apt_repository in the install recipe when percona is selected in the attribute.

Updated docs and forced mysqld::percona_repository when installing percona.

chr4 · 2016-11-08T12:16:11Z

recipes/install.rb

@@ -26,6 +26,7 @@
 when 'mariadb'
  Array(node['mysqld']['mariadb']['packages']).each { |pkg| package pkg }
 when 'percona'
+  include_recipe 'mysqld::percona_repository'


A comment explaining why the repository is set up would be nice for the future. Especially since we're not doing it for mariadb.

chr4 · 2016-11-08T12:28:42Z

recipes/install.rb

@@ -26,6 +26,7 @@
 when 'mariadb'
  Array(node['mysqld']['mariadb']['packages']).each { |pkg| package pkg }
 when 'percona'
+  # This is included here, since percona 5.7 is not included in any of the distros supported.


Thanks! My suggestion to make it a little more clear:
Always use the official percona repository, as some distributions use percona-5.6, and the default attributes of this cookbook require 5.7

chr4 · 2016-11-08T12:31:23Z

recipes/install.rb

@@ -26,6 +26,7 @@
 when 'mariadb'
  Array(node['mysqld']['mariadb']['packages']).each { |pkg| package pkg }


Unlike with percona, the apt repository for mariadb is not automatically set up in this recipe. While the default recipe makes sure the repository is used, not including it here makes it possible to use distribution packages of mariadb. This is a little dangerous, as the cookbook officially only supports mysql-5.7 and mariadb-10.1

I'm not sure whether it might be a good idea to include the apt repository here, too.
But as there's no check whether mysql-5.7 is used, we might leave this up to the user. I'm a little concernded when it's possible to easily install a non-supported database version that might lead into problems. Thoughts on this?

…ate pull request chr4-cookbooks/pull/13 with some changes to include percona and versions of databases. Use cookstyle instead of rubocop. (https://github.com/chef/cookstyle)

chr4 · 2016-11-08T16:21:38Z

Thoughts on whether to automatically include the mariadb repository in the install recipe?
The default mariadb version in Ubuntu Xenial (16.04) is mariadb-10.0.

As mariadb-10.0 potentially has similar issues with incompatible my.cnf settings, it might be a safe idea to also include the apt repository recipe in the install recipe (similar to percona).

The issue is a little smaller, as the default recipe automatically includes the repository recipe.
Automatically including the recipe prevents the user from using the default package, but adds some more consistency.

Thoughts on this?

chr4 · 2016-11-08T16:37:27Z

recipes/install.rb

@@ -18,9 +18,18 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 #

+# This way we force to have updated indexes
+include_recipe 'apt'


Is this required? apt-get update should be run automatically when adding the repository below.

chr4 · 2016-11-08T16:40:08Z

attributes/defaults.rb

@@ -54,6 +60,14 @@
  default['mysqld']['my.cnf']['mysqld_safe']['skip_log_error'] = true
 end

+# Password columns for user passwords
+default['mysqld']['pwd_col'] = case node['mysqld']['db_install']


Would it make sense to document the pwd_col attribute in the README? It could be overridden using override['mysqld']['pwd_col'] if necessary.

It might be a good idea to keep this internal, though.

in order to hide this from the user completely, we could remove the attribute and move the decision for the proper pwd_col to the the password provider.
I think the user doesn't need to be able to change which column is actually used.

edit: Would it be possible to use the SET PASSWORD statement instead?
mariadb
mysql
edit2: I just noticed the deprecation notice for the statement.

An attribute won't hurt, and opens up the option for advanced users to use this cookbook with older mysql versions.
The only concern I have is, that the cookbook advises and deploys defaults that are unsafe, not sane, etc. I'm not sure howto "enforce" this (or whether it's a good idea to do so), besides a warning in the README.

This was referenced Nov 2, 2016

Percona support #11

Closed

Changed providers/password to use attribute pwd_col #13

Closed

chr4 requested changes Nov 7, 2016

View reviewed changes

chr4 mentioned this pull request Nov 7, 2016

Test Kitchen issues found on password setting #12

Closed

aalmenar force-pushed the master branch from e515485 to 8a1c508 Compare November 7, 2016 15:01

aalmenar force-pushed the master branch from 74c78cd to 328d44d Compare November 7, 2016 17:32

chr4 approved these changes Nov 7, 2016

View reviewed changes

chr4 requested changes Nov 7, 2016

View reviewed changes

chr4 reviewed Nov 8, 2016

View reviewed changes

Add percona support, add kitchen and tests with serverspec and integr…

4398fff

…ate pull request chr4-cookbooks/pull/13 with some changes to include percona and versions of databases. Use cookstyle instead of rubocop. (https://github.com/chef/cookstyle)

aalmenar force-pushed the master branch from 21705ec to 4398fff Compare November 8, 2016 12:36

chr4 approved these changes Nov 8, 2016

View reviewed changes

chr4 reviewed Nov 8, 2016

View reviewed changes

chr4 requested changes Nov 8, 2016

View reviewed changes

		@@ -26,6 +26,7 @@
		when 'mariadb'
		Array(node['mysqld']['mariadb']['packages']).each { \|pkg\| package pkg }

Add percona support, add kitchen and tests with serverspec #14

Are you sure you want to change the base?

Add percona support, add kitchen and tests with serverspec #14

Conversation

aalmenar commented Nov 2, 2016

chr4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chr4 Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

aalmenar Nov 7, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aalmenar commented Nov 7, 2016

aalmenar commented Nov 7, 2016

chr4 commented Nov 7, 2016

aalmenar commented Nov 7, 2016 • edited Loading

aalmenar commented Nov 7, 2016

chr4 commented Nov 7, 2016

aalmenar commented Nov 7, 2016

chr4 commented Nov 7, 2016

aalmenar commented Nov 7, 2016

aalmenar commented Nov 7, 2016

chr4 commented Nov 7, 2016

aalmenar commented Nov 7, 2016

chr4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chr4 commented Nov 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dens49 Nov 9, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chr4 Nov 7, 2016 •

edited

Loading

aalmenar Nov 7, 2016 •

edited

Loading

aalmenar commented Nov 7, 2016 •

edited

Loading

Dens49 Nov 9, 2016 •

edited

Loading