From ec14b87a814127d8dcb65bca6a35a60edb9736c2 Mon Sep 17 00:00:00 2001 From: "Steven C. Saliman" Date: Wed, 29 Apr 2015 07:52:37 -0600 Subject: [PATCH 1/3] Added options for including/excluding triggers and routines, and fixed a permission problem that was preventing triggers from being backed up --- README.md | 8 ++ manifests/backup/mysqlbackup.pp | 2 + manifests/backup/mysqldump.pp | 4 +- manifests/backup/xtrabackup.pp | 2 + manifests/server/backup.pp | 4 + spec/acceptance/mysql_backup_spec.rb | 42 ++++++ spec/classes/mysql_server_backup_spec.rb | 169 ++++++++++++++++++++++- templates/mysqlbackup.sh.erb | 25 +++- 8 files changed, 244 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 4c6a459..1bb3017 100644 --- a/README.md +++ b/README.md @@ -377,6 +377,14 @@ Specify an array of databases to back up. Whether a separate file be used per database. Valid values are 'true', 'false'. Defaults to 'false'. +#####`include_routines` + +Whether or not to include routines for each database when doing a `file_per_database` backup. Defaults to `false`. + +#####`include_triggers` + +Whether or not to include triggers for a each database when doing a `file_per_database` backup. Defaults to `true`. + #####`ensure` Allows you to remove the backup scripts. Valid values are 'present', 'absent'. Defaults to 'present'. diff --git a/manifests/backup/mysqlbackup.pp b/manifests/backup/mysqlbackup.pp index 9d92b6c..e7bb4d9 100644 --- a/manifests/backup/mysqlbackup.pp +++ b/manifests/backup/mysqlbackup.pp @@ -12,6 +12,8 @@ class mysql::backup::mysqlbackup ( $delete_before_dump = false, $backupdatabases = [], $file_per_database = false, + $include_triggers = true, + $include_routines = false, $ensure = 'present', $time = ['23', '5'], $postscript = false, diff --git a/manifests/backup/mysqldump.pp b/manifests/backup/mysqldump.pp index 51f912c..649dcaf 100644 --- a/manifests/backup/mysqldump.pp +++ b/manifests/backup/mysqldump.pp @@ -12,6 +12,8 @@ class mysql::backup::mysqldump ( $delete_before_dump = false, $backupdatabases = [], $file_per_database = false, + $include_triggers = true, + $include_routines = false, $ensure = 'present', $time = ['23', '5'], $postscript = false, @@ -28,7 +30,7 @@ class mysql::backup::mysqldump ( ensure => $ensure, user => "${backupuser}@localhost", table => '*.*', - privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ], + privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER' ], require => Mysql_user["${backupuser}@localhost"], } diff --git a/manifests/backup/xtrabackup.pp b/manifests/backup/xtrabackup.pp index 073048d..a9c76ba 100644 --- a/manifests/backup/xtrabackup.pp +++ b/manifests/backup/xtrabackup.pp @@ -13,6 +13,8 @@ class mysql::backup::xtrabackup ( $delete_before_dump = false, $backupdatabases = [], $file_per_database = false, + $include_triggers = true, + $include_routines = false, $ensure = 'present', $time = ['23', '5'], $postscript = false, diff --git a/manifests/server/backup.pp b/manifests/server/backup.pp index e2cac2f..554c9a4 100644 --- a/manifests/server/backup.pp +++ b/manifests/server/backup.pp @@ -12,6 +12,8 @@ class mysql::server::backup ( $delete_before_dump = false, $backupdatabases = [], $file_per_database = false, + $include_routines = false, + $include_triggers = true, $ensure = 'present', $time = ['23', '5'], $postscript = false, @@ -33,6 +35,8 @@ class mysql::server::backup ( 'delete_before_dump' => $delete_before_dump, 'backupdatabases' => $backupdatabases, 'file_per_database' => $file_per_database, + 'include_routines' => $include_routines, + 'include_triggers' => $include_triggers, 'ensure' => $ensure, 'time' => $time, 'postscript' => $postscript, diff --git a/spec/acceptance/mysql_backup_spec.rb b/spec/acceptance/mysql_backup_spec.rb index 7f6e322..4bc52d2 100644 --- a/spec/acceptance/mysql_backup_spec.rb +++ b/spec/acceptance/mysql_backup_spec.rb @@ -129,4 +129,46 @@ describe 'mysql::server::backup class' do end end end + + context 'with triggers and routines' do + it 'when configuring mysql backups with triggers and routines' do + pp = <<-EOS + class { 'mysql::server': root_password => 'password' } + mysql::db { [ + 'backup1', + 'backup2' + ]: + user => 'backup', + password => 'secret', + } + package { 'bzip2': + ensure => present, + } + class { 'mysql::server::backup': + backupuser => 'myuser', + backuppassword => 'mypassword', + backupdir => '/tmp/backups', + backupcompress => true, + file_per_database => true, + include_triggers => true, + include_routines => true, + postscript => [ + 'rm -rf /var/tmp/mysqlbackups', + 'rm -f /var/tmp/mysqlbackups.done', + 'cp -r /tmp/backups /var/tmp/mysqlbackups', + 'touch /var/tmp/mysqlbackups.done', + ], + execpath => '/usr/bin:/usr/sbin:/bin:/sbin:/opt/zimbra/bin', + require => Package['bzip2'], + } + EOS + apply_manifest(pp, :catch_failures => true) + end + + it 'should run mysqlbackup.sh with no errors' do + shell("/usr/local/sbin/mysqlbackup.sh") do |r| + expect(r.stderr).to eq("") + end + end + end end diff --git a/spec/classes/mysql_server_backup_spec.rb b/spec/classes/mysql_server_backup_spec.rb index c7e2583..0cd93e0 100644 --- a/spec/classes/mysql_server_backup_spec.rb +++ b/spec/classes/mysql_server_backup_spec.rb @@ -24,7 +24,7 @@ describe 'mysql::server::backup' do :require => 'Class[Mysql::Server::Root_password]') } it { is_expected.to contain_mysql_grant('testuser@localhost/*.*').with( - :privileges => ['SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS'] + :privileges => ['SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER'] ).that_requires('Mysql_user[testuser@localhost]') } it { is_expected.to contain_cron('mysql-backup').with( @@ -43,13 +43,26 @@ describe 'mysql::server::backup' do )} it 'should have compression by default' do - is_expected.to contain_file('mysqlbackup.sh').with( - :content => /bzcat -zc/ + is_expected.to contain_file('mysqlbackup.sh').with_content( + /bzcat -zc/ ) end + it 'should skip backing up events table by default' do - is_expected.to contain_file('mysqlbackup.sh').with( - :content => /EVENTS="--ignore-table=mysql.event"/ + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="--ignore-table=mysql.event"/ + ) + end + + it 'should not mention triggers by default because file_per_database is false' do + is_expected.to contain_file('mysqlbackup.sh').without_content( + /.*triggers.*/ + ) + end + + it 'should not mention routines by default because file_per_database is false' do + is_expected.to contain_file('mysqlbackup.sh').without_content( + /.*routines.*/ ) end @@ -109,7 +122,7 @@ describe 'mysql::server::backup' do it 'should be able to backup events table' do is_expected.to contain_file('mysqlbackup.sh').with_content( - /EVENTS="--events"/ + /ADDITIONAL_OPTIONS="--events"/ ) end end @@ -130,6 +143,78 @@ describe 'mysql::server::backup' do /mysql | bzcat -zc \${DIR}\\\${PREFIX}mysql_`date'/ ) end + + it 'should backup triggers by default' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --triggers"/ + ) + end + + it 'should skip backing up routines by default' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-routines"/ + ) + end + + context 'with include_triggers set to true' do + let(:params) do + default_params.merge({ + :backupdatabases => ['mysql'], + :include_triggers => true + }) + end + + it 'should backup triggers when asked' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --triggers"/ + ) + end + end + + context 'with include_triggers set to false' do + let(:params) do + default_params.merge({ + :backupdatabases => ['mysql'], + :include_triggers => false + }) + end + + it 'should skip backing up triggers when asked to skip' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-triggers"/ + ) + end + end + + context 'with include_routines set to true' do + let(:params) do + default_params.merge({ + :backupdatabases => ['mysql'], + :include_routines => true + }) + end + + it 'should backup routines when asked' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --routines"/ + ) + end + end + + context 'with include_routines set to false' do + let(:params) do + default_params.merge({ + :backupdatabases => ['mysql'], + :include_triggers => true + }) + end + + it 'should skip backing up routines when asked to skip' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-routines"/ + ) + end + end end context 'with file per database' do @@ -155,6 +240,78 @@ describe 'mysql::server::backup' do ) end end + + it 'should backup triggers by default' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --triggers"/ + ) + end + + it 'should skip backing up routines by default' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-routines"/ + ) + end + + context 'with include_triggers set to true' do + let(:params) do + default_params.merge({ + :file_per_database => true, + :include_triggers => true + }) + end + + it 'should backup triggers when asked' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --triggers"/ + ) + end + end + + context 'with include_triggers set to false' do + let(:params) do + default_params.merge({ + :file_per_database => true, + :include_triggers => false + }) + end + + it 'should skip backing up triggers when asked to skip' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-triggers"/ + ) + end + end + + context 'with include_routines set to true' do + let(:params) do + default_params.merge({ + :file_per_database => true, + :include_routines => true + }) + end + + it 'should backup routines when asked' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --routines"/ + ) + end + end + + context 'with include_routines set to false' do + let(:params) do + default_params.merge({ + :file_per_database => true, + :include_triggers => true + }) + end + + it 'should skip backing up routines when asked to skip' do + is_expected.to contain_file('mysqlbackup.sh').with_content( + /ADDITIONAL_OPTIONS="\$ADDITIONAL_OPTIONS --skip-routines"/ + ) + end + end end context 'with postscript' do diff --git a/templates/mysqlbackup.sh.erb b/templates/mysqlbackup.sh.erb index 2148208..a2fb4e7 100755 --- a/templates/mysqlbackup.sh.erb +++ b/templates/mysqlbackup.sh.erb @@ -21,10 +21,25 @@ ROTATE=<%= [ Integer(@backuprotate) - 1, 0 ].max %> PREFIX=mysql_backup_ <% if @ignore_events %> -EVENTS="--ignore-table=mysql.event" +ADDITIONAL_OPTIONS="--ignore-table=mysql.event" <% else %> -EVENTS="--events" +ADDITIONAL_OPTIONS="--events" <% end %> +<%# Only include routines or triggers if we're doing a file per database -%> +<%# backup. This happens if we named databases, or if we explicitly set -%> +<%# file per database mode -%> +<% if !@backupdatabases.empty? || @file_per_database -%> +<% if @include_triggers -%> +ADDITIONAL_OPTIONS="$ADDITIONAL_OPTIONS --triggers" +<% else -%> +ADDITIONAL_OPTIONS="$ADDITIONAL_OPTIONS --skip-triggers" +<% end -%> +<% if @include_routines -%> +ADDITIONAL_OPTIONS="$ADDITIONAL_OPTIONS --routines" +<% else -%> +ADDITIONAL_OPTIONS="$ADDITIONAL_OPTIONS --skip-routines" +<% end -%> +<% end -%> ##### STOP CONFIG #################################################### PATH=<%= @execpath %> @@ -49,18 +64,18 @@ cleanup mysql -u${USER} -p${PASS} -s -r -N -e 'SHOW DATABASES' | while read dbname do mysqldump -u${USER} -p${PASS} --opt --flush-logs --single-transaction \ - ${EVENTS} \ + ${ADDITIONAL_OPTIONS} \ ${dbname} <% if @backupcompress %>| bzcat -zc <% end %>> ${DIR}/${PREFIX}${dbname}_`date +%Y%m%d-%H%M%S`.sql<% if @backupcompress %>.bz2<% end %> done <% else -%> mysqldump -u${USER} -p${PASS} --opt --flush-logs --single-transaction \ - ${EVENTS} \ + ${ADDITIONAL_OPTIONS} \ --all-databases <% if @backupcompress %>| bzcat -zc <% end %>> ${DIR}/${PREFIX}`date +%Y%m%d-%H%M%S`.sql<% if @backupcompress %>.bz2<% end %> <% end -%> <% else -%> <% @backupdatabases.each do |db| -%> mysqldump -u${USER} -p${PASS} --opt --flush-logs --single-transaction \ - ${EVENTS} \ + ${ADDITIONAL_OPTIONS} \ <%= db %><% if @backupcompress %>| bzcat -zc <% end %>> ${DIR}/${PREFIX}<%= db %>_`date +%Y%m%d-%H%M%S`.sql<% if @backupcompress %>.bz2<% end %> <% end -%> <% end -%> From 49f273a42cf532288f7d8383d6a7fb952f1cb121 Mon Sep 17 00:00:00 2001 From: "Steven C. Saliman" Date: Thu, 30 Apr 2015 12:37:38 -0600 Subject: [PATCH 2/3] Made the 'TRIGGER'privilege of mysqldump backups depend on whether or not we are actually backing up triggers --- manifests/backup/mysqldump.pp | 22 ++++++++++++++++------ spec/classes/mysql_server_backup_spec.rb | 9 +++++++++ 2 files changed, 25 insertions(+), 6 deletions(-) diff --git a/manifests/backup/mysqldump.pp b/manifests/backup/mysqldump.pp index 649dcaf..f1c924f 100644 --- a/manifests/backup/mysqldump.pp +++ b/manifests/backup/mysqldump.pp @@ -26,12 +26,22 @@ class mysql::backup::mysqldump ( require => Class['mysql::server::root_password'], } - mysql_grant { "${backupuser}@localhost/*.*": - ensure => $ensure, - user => "${backupuser}@localhost", - table => '*.*', - privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER' ], - require => Mysql_user["${backupuser}@localhost"], + if $include_triggers { + mysql_grant { "${backupuser}@localhost/*.*": + ensure => $ensure, + user => "${backupuser}@localhost", + table => '*.*', + privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER' ], + require => Mysql_user["${backupuser}@localhost"], + } + } else { + mysql_grant { "${backupuser}@localhost/*.*": + ensure => $ensure, + user => "${backupuser}@localhost", + table => '*.*', + privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ], + require => Mysql_user["${backupuser}@localhost"], + } } cron { 'mysql-backup': diff --git a/spec/classes/mysql_server_backup_spec.rb b/spec/classes/mysql_server_backup_spec.rb index 0cd93e0..81abb30 100644 --- a/spec/classes/mysql_server_backup_spec.rb +++ b/spec/classes/mysql_server_backup_spec.rb @@ -27,6 +27,15 @@ describe 'mysql::server::backup' do :privileges => ['SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER'] ).that_requires('Mysql_user[testuser@localhost]') } + context 'with triggers excluded' do + let(:params) do + { :include_triggers => false }.merge(default_params) + end + it { is_expected.to contain_mysql_grant('testuser@localhost/*.*').with( + :privileges => ['SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS'] + ).that_requires('Mysql_user[testuser@localhost]') } + end + it { is_expected.to contain_cron('mysql-backup').with( :command => '/usr/local/sbin/mysqlbackup.sh', :ensure => 'present' From 1018a0370a5e8532ae737f649a8e7b4601e1f533 Mon Sep 17 00:00:00 2001 From: "Steven C. Saliman" Date: Thu, 30 Apr 2015 14:21:08 -0600 Subject: [PATCH 3/3] Cleaned up the privilege assignment in the mysqldump backup script --- manifests/backup/mysqldump.pp | 25 +++++++++++-------------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/manifests/backup/mysqldump.pp b/manifests/backup/mysqldump.pp index f1c924f..8aea324 100644 --- a/manifests/backup/mysqldump.pp +++ b/manifests/backup/mysqldump.pp @@ -26,22 +26,19 @@ class mysql::backup::mysqldump ( require => Class['mysql::server::root_password'], } + if $include_triggers { - mysql_grant { "${backupuser}@localhost/*.*": - ensure => $ensure, - user => "${backupuser}@localhost", - table => '*.*', - privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER' ], - require => Mysql_user["${backupuser}@localhost"], - } + $privs = [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS', 'TRIGGER' ] } else { - mysql_grant { "${backupuser}@localhost/*.*": - ensure => $ensure, - user => "${backupuser}@localhost", - table => '*.*', - privileges => [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ], - require => Mysql_user["${backupuser}@localhost"], - } + $privs = [ 'SELECT', 'RELOAD', 'LOCK TABLES', 'SHOW VIEW', 'PROCESS' ] + } + + mysql_grant { "${backupuser}@localhost/*.*": + ensure => $ensure, + user => "${backupuser}@localhost", + table => '*.*', + privileges => $privs, + require => Mysql_user["${backupuser}@localhost"], } cron { 'mysql-backup':