Merge lp:~mariosplivalo/charms/trusty/percona-cluster/enable-binlogs into lp:~openstack-charmers-archive/charms/trusty/percona-cluster/next

[Mario Splivalo]

This adds two charm configuration options - enable_binlogs and binlogs_path. Binlog_path defaults to /var/log/mysql/mysql.bin (it's because my.cnf which is packaged with mysql-common package suggests that location, and also because mysql charm defaults with binlogs at that location), while enable_binlogs defaults to 'False'.

[Felipe Reyes (freyes)]

Hi Mario,

This feature makes a lot of sense, and it worked for me, but please add unit tests for your patch, I also have minor comments in the diff.

Thanks,

review: Needs Fixing

[James Page (james-page)]

bzr merge lp:~mariosplivalo/charms/trusty/percona-cluster/enable-binlogs
...
Text conflict in hooks/charmhelpers/contrib/database/mysql.py
1 conflicts encountered.

review: Needs Fixing

[James Page (james-page)]

review: Needs Fixing

[Mario Splivalo (mariosplivalo)]

> bzr merge lp:~mariosplivalo/charms/trusty/percona-cluster/enable-binlogs
> ...
> Text conflict in hooks/charmhelpers/contrib/database/mysql.py
> 1 conflicts encountered.

I belive this is because of the prerequisite branch that needs merging first.

[Mario Splivalo (mariosplivalo)]

Based on discussion with Ed, this will be changed in a manner so that a new function is introduced, 'configure_binlogs'. That one will be called from install hook, after mysql has been installed, and will restart mysql for the change to take place.
It will also be called from config_changed hook, but it won't restart mysql - it will log a WARNING inside logs to tell administrator/operator to manually restart mysql when appropriate.

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #11893 percona-cluster-next for mariosplivalo mp274394
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/11893/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #11062 percona-cluster-next for mariosplivalo mp274394
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/11062/

[Billy Olsen (billy-olsen)]

I'm a little confused. Based on the feedback from James and Ed, I wasn't expecting this creating of the directories in the render_config function - or am I missing something?

review: Needs Information

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #11231 percona-cluster-next for mariosplivalo mp274394
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/11231/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #12091 percona-cluster-next for mariosplivalo mp274394
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/12091/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #7421 percona-cluster-next for mariosplivalo mp274394
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/7421/

[Mario Splivalo (mariosplivalo)]

> I'm a little confused. Based on the feedback from James and Ed, I wasn't
> expecting this creating of the directories in the render_config function - or
> am I missing something?

You are right. I have isolated this into separate function, configure_binlogs_directory().

[Billy Olsen (billy-olsen)]

I think this is close, but the two calls to the configure_binlogs_directory() feels a bit kludgy - I think that needs a little rework or thought there.

review: Needs Fixing

[lp:~mariosplivalo/charms/trusty/percona-cluster/enable-binlogs]

61. By Mario Splivalo on 2015-12-03

Conflicts resolved (work from another workstation)

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #13811 percona-cluster-next for mariosplivalo mp274394
    UNIT FAIL: unit-test failed

UNIT Results (max last 2 lines):
make: *** [test] Error 1
ERROR:root:Make target returned non-zero.

Full unit test output: http://paste.ubuntu.com/13644697/
Build: http://10.245.162.77:8080/job/charm_unit_test/13811/

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #14811 percona-cluster-next for mariosplivalo mp274394
    LINT FAIL: lint-test failed

LINT Results (max last 2 lines):
make: *** [lint] Error 1
ERROR:root:Make target returned non-zero.

Full lint test output: http://paste.ubuntu.com/13644698/
Build: http://10.245.162.77:8080/job/charm_lint_check/14811/

[Ryan Beisner (1chb1n)]

FYI, the percona cluster install hook is failing:

2015-12-03 14:28:13 INFO install Processing triggers for libc-bin (2.19-0ubuntu6.6) ...
2015-12-03 14:28:13 INFO install Traceback (most recent call last):
2015-12-03 14:28:13 INFO install File "/var/lib/juju/agents/unit-mysql-0/charm/hooks/install.real", line 46, in <module>
2015-12-03 14:28:13 INFO install from percona_utils import (
2015-12-03 14:28:13 INFO install ImportError: cannot import name configure_binlogs

review: Needs Fixing

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #8132 percona-cluster-next for mariosplivalo mp274394
    AMULET FAIL: amulet-test failed

AMULET Results (max last 2 lines):
make: *** [functional_test] Error 1
ERROR:root:Make target returned non-zero.

Full amulet test output: http://paste.ubuntu.com/13657282/
Build: http://10.245.162.77:8080/job/charm_amulet_test/8132/

[Mario Splivalo (mariosplivalo)]

This is WIP, no idea why this was submitted...

[Mario Splivalo (mariosplivalo)]

> This is WIP, no idea why this was submitted...

Because it was set to 'Needs review' :( My fault.

Unmerged revisions

61. By Mario Splivalo on 2015-12-03

Conflicts resolved (work from another workstation)

60. By Mario Splivalo on 2015-10-18

Merged /next

59. By Mario Splivalo on 2015-10-16

Isolate creation of binlog directory into a separate function

58. By Mario Splivalo on 2015-10-09

Merged from /next

57. By Mario Splivalo on 2015-03-12

Fixed typos, as suggested by freyes

56. By Mario Splivalo on 2015-03-12

Fix lint errors

55. By Mario Splivalo on 2015-03-06

Remove call to lchownr as it won't change owner of the base directory.
Instead, use os.chown.

54. By Mario Splivalo on 2015-03-06

Fix directory creation

53. By Mario Splivalo on 2015-03-05

Make sure mysql can start even when binlog_path is pointing to
nonexistant path

52. By Mario Splivalo on 2015-03-04

Create binlog_directory, if missing

=== modified file 'config.yaml'
--- config.yaml	2015-10-29 12:30:18 +0000
+++ config.yaml	2015-12-03 13:33:30 +0000
@@ -152,3 +152,14 @@
     description: |
       A comma-separated list of nagios servicegroups. If left empty, the
       nagios_context will be used as the servicegroup.
+  enable-binlogs:
+    default: False
+    type: boolean
+    description: Turns on MySQL binary logs. The placement of the logs is controlled with the binlogs_path config option.
+  binlogs-path:
+    default: /var/log/mysql/mysql-bin.log
+    type: string
+    description: |
+      Location on the filesystem where binlogs are going to be placed.
+      Default mimics what mysql-common would do for mysql (although not the
+      best place for binlogs)

=== modified file 'hooks/percona_hooks.py'
--- hooks/percona_hooks.py	2015-11-16 10:14:55 +0000
+++ hooks/percona_hooks.py	2015-12-03 13:33:30 +0000
@@ -51,6 +51,7 @@
     get_cluster_hosts,
     configure_sstuser,
     configure_mysql_root_password,
+    configure_binlogs,
     relation_clear,
     assert_charm_supports_ipv6,
     unit_sorted,
@@ -61,6 +62,7 @@
     notify_bootstrapped,
     is_bootstrapped,
     get_wsrep_value,
+    configure_binlogs_directory,
     assess_status,
 )
 from charmhelpers.contrib.database.mysql import (
@@ -94,6 +96,8 @@
                       'op monitor interval="1s" timeout="30s" '
                       'OCF_CHECK_LEVEL="1"')
 
+MYSQL_DATA_DIR = '/var/lib/mysql'
+
 
 @hooks.hook('install.real')
 def install():
@@ -107,8 +111,10 @@
     configure_mysql_root_password(config('root-password'))
     # Render base configuration (no cluster)
     render_config()
+    configure_binlogs_directory()       # this creates binlogs dir if needed
     apt_update(fatal=True)
     apt_install(determine_packages(), fatal=True)
+    configure_binlogs_directory()       # this fixed the owner/permissions
     configure_sstuser(config('sst-password'))
 
 
@@ -125,7 +131,9 @@
         'sst_password': config('sst-password'),
         'innodb_file_per_table': config('innodb-file-per-table'),
         'table_open_cache': config('table-open-cache'),
-        'lp1366997_workaround': config('lp1366997-workaround')
+        'lp1366997_workaround': config('lp1366997-workaround'),
+        'binlogs_path': config('binlogs-path'),
+        'enable_binlogs': config('enable-binlogs')
     }
 
     if config('prefer-ipv6'):
@@ -202,8 +210,7 @@
     for r_id in relation_ids('shared-db'):
         for unit in related_units(r_id):
             shared_db_changed(r_id, unit)
-
-
+            
 @hooks.hook('upgrade-charm')
 def upgrade():
     check_bootstrap = False
@@ -240,6 +247,9 @@
     # applies if min-cluster-size is provided and is used to avoid extraneous
     # configuration changes and premature bootstrapping as the cluster is
     # deployed.
+
+    configure_binlogs_directory()
+
     if is_sufficient_peers():
         try:
             # NOTE(jamespage): try with leadership election
@@ -262,6 +272,7 @@
                 log("Leader unit - bootstrap required=%s" % (not bootstrapped),
                     DEBUG)
                 render_config_restart_on_changed(clustered, hosts,
+                                                
                                                  bootstrap=not bootstrapped)
             elif bootstrapped:
                 log("Cluster is bootstrapped - configuring mysql on this node",

=== modified file 'hooks/percona_utils.py'
--- hooks/percona_utils.py	2015-10-10 16:07:05 +0000
+++ hooks/percona_utils.py	2015-12-03 13:33:30 +0000
@@ -6,9 +6,12 @@
 import os
 import shutil
 import uuid
+import pwd
+import grp
 
 from charmhelpers.core.host import (
-    lsb_release
+    lsb_release,
+    mkdir
 )
 from charmhelpers.core.hookenv import (
     charm_dir,
@@ -363,6 +366,25 @@
         relation_set(relation_id=rid, **{'bootstrap-uuid': cluster_uuid})
 
 
+def configure_binlogs_directory():
+    # When this function is called before mysql is installed, we need to
+    # create binlogs_path directory world-writable.
+    # We need that directory as otherwise mysql won't start.
+    binlogs_directory = os.path.dirname(config('binlogs-path'))
+    if not os.path.isdir(binlogs_directory):
+        mkdir(binlogs_directory, 'root', 'root', 0o777)
+
+    # This is called again after mysql has been installed (and from
+    # config-changed hook, we need to set proper directory permissions
+    try:
+        uid = pwd.getpwnam('mysql').pw_uid
+        gid = grp.getgrnam('mysql').gr_gid
+        os.chown(binlogs_directory, uid, gid)
+        os.chmod(binlogs_directory, 0o750)
+    except KeyError:
+        pass
+
+
 def cluster_in_sync():
     '''
     Determines whether the current unit is in sync

=== modified file 'templates/my.cnf'
--- templates/my.cnf	2015-06-04 14:40:54 +0000
+++ templates/my.cnf	2015-12-03 13:33:30 +0000
@@ -25,6 +25,10 @@
 # In order for Galera to work correctly binlog format should be ROW
 binlog_format=ROW
 
+{% if enable_binlogs -%}
+log_bin={{ binlogs_path }}
+{% endif -%}
+
 # MyISAM storage engine has only experimental support
 default_storage_engine=InnoDB