(puppetlabsGH-198) Fix race condition on postgresql startup

This patch is a fix for the race condition that keeps occuring during
postgresql setup. Its very rare on its own, but when you are using this
module in a CI environment it happens quite frequently.

Basically what happens is that sometimes the service will announce the
database has started, but really it is still working in the background.
Sometimes the unix socket may not be listening, and sometimes the
system is still loading and you get a weird client error.

The fix itself is a modification to postgresql::validate_db_connection
so that it is able to connect on the local unix socket, plus retry
until the database is available.

This new and improved validate_db_connection can then be put into the
build pipeline (in the service class in particular) to ensure the
database is started before continuing on with the remaining steps.

This in effect blocks the puppet module from continuing until the
postgresql database is fully started and able to receive connections
which is perfect.

Tests and documentation provided.

Signed-off-by: Ken Barber <[email protected]>

Author: kbarber

README.md

@@ -749,20 +749,32 @@ Example usage:
 Uniquely identify this resource, but functionally does nothing.
 
 ####`database_host`
-The hostname of the database you wish to test.
+The hostname of the database you wish to test. Defaults to 'undef' which generally uses the designated local unix socket.
 
 ####`database_port`
-Port to use when connecting.
+Port to use when connecting. Default to 'undef' which generally defaults to 5432 depending on your PostgreSQL packaging.
 
 ####`database_name`
-The name of the database you wish to test.
+The name of the database you wish to test. Defaults to 'postgres'.
 
 ####`database_username`
-Username to connect with.
+Username to connect with. Defaults to 'undef', which when using a unix socket and ident auth will be the user you are running as. If the host is remote you must provide a username.
 
 ####`database_password`
 Password to connect with. Can be left blank, but that is not recommended.
 
+####`run_as`
+The user to run the `psql` command with for authenticiation. This is important when trying to connect to a database locally using Unix sockets and `ident` authentication. It is not needed for remote testing.
+
+####`sleep`
+Upon failure, sets the number of seconds to sleep for before trying again.
+
+####`tries`
+Upon failure, sets the number of attempts before giving up and failing the resource.
+
+####`create_db_first`
+This will ensure the database is created before running the test. This only really works if your test is local. Defaults to `true`.
+
 
 ###Function: postgresql\_password
 If you need to generate a postgres encrypted password, use `postgresql_password`. You can call it from your production manifests if you don't mind them containing the clear text versions of your passwords, or you can call it from the command line and then copy and paste the encrypted password into your manifest:

files/validate_postgresql_connection.sh

@@ -0,0 +1,31 @@
+#!/usr/bin/env bash
+
+# usage is: validate_db_connection 2 50 psql
+
+SLEEP=$1
+TRIES=$2
+PSQL=$3
+
+STATE=1
+
+for (( c=1; c<=$TRIES; c++ ))
+do
+  echo $c
+  if [ $c -gt 1 ]
+  then
+    echo 'sleeping'
+    sleep $SLEEP
+  fi
+
+  /bin/echo "SELECT 1" | $PSQL
+  STATE=$?
+
+  if [ $STATE -eq 0 ]
+  then
+    exit 0
+  fi
+done
+
+echo 'Unable to connect to puppetdb'
+
+exit 1

manifests/client.pp

@@ -11,4 +11,19 @@
     tag     => 'postgresql',
   }
 
+  $file_ensure = $package_ensure ? {
+    'present' => 'file',
+    true      => 'file',
+    'absent'  => 'absent',
+    false     => 'absent',
+    default   => 'file',
+  }
+  file { "/usr/local/bin/validate_postgresql_connection.sh":
+    ensure => $file_ensure,
+    source => "puppet:///modules/postgresql/validate_postgresql_connection.sh",
+    owner  => 0,
+    group  => 0,
+    mode   => 0755,
+  }
+
 }

manifests/server/database.pp

@@ -48,7 +48,7 @@
   postgresql_psql { "Check for existence of db '${dbname}'":
     command => 'SELECT 1',
     unless  => "SELECT datname FROM pg_database WHERE datname='${dbname}'",
-    require => Class['postgresql::server']
+    require => Class['postgresql::server::service']
   }~>
   exec { $createdb_command :
     refreshonly => true,

manifests/server/service.pp

@@ -4,6 +4,7 @@
   $service_name     = $postgresql::server::service_name
   $service_provider = $postgresql::server::service_provider
   $service_status   = $postgresql::server::service_status
+  $user             = $postgresql::server::user
 
   $service_ensure = $ensure ? {
     present => true,
@@ -19,4 +20,19 @@
     hasstatus => true,
     status    => $service_status,
   }
+
+  if($service_ensure) {
+    # This blocks the class before continuing if chained correctly, making
+    # sure the service really is 'up' before continuing.
+    #
+    # Without it, we may continue doing more work before the database is
+    # prepared leading to a nasty race condition.
+    postgresql::validate_db_connection { 'validate_service_is_running':
+      run_as          => $user,
+      sleep           => 1,
+      tries           => 30,
+      create_db_first => false,
+      require         => Service['postgresqld'],
+    }
+  }
 }

manifests/validate_db_connection.pp

@@ -4,26 +4,58 @@
 #
 # See README.md for more details.
 define postgresql::validate_db_connection(
-  $database_host,
-  $database_name,
-  $database_password,
-  $database_username,
-  $database_port = 5432
+  $database_host     = undef,
+  $database_name     = 'postgres',
+  $database_password = undef,
+  $database_username = undef,
+  $database_port     = undef,
+  $run_as            = undef,
+  $sleep             = 2,
+  $tries             = 10,
+  $create_db_first   = true
 ) {
   require postgresql::client
 
   $psql_path = $postgresql::params::psql_path
 
-  # TODO: port to ruby
-  $psql = "${psql_path} --tuples-only --quiet -h ${database_host} -U ${database_username} -p ${database_port} --dbname ${database_name}"
+  $cmd_init = "${psql_path} --tuples-only --quiet "
+  $cmd_host = $database_host ? {
+    default => "-h ${database_host} ",
+    undef   => ""
+  }
+  $cmd_user = $database_username ? {
+    default => "-U ${database_username} ",
+    undef   => ""
+  }
+  $cmd_port = $database_port ? {
+    default => "-p ${database_port} ",
+    undef   => ""
+  }
+  $cmd_dbname = $database_name ? {
+    default => "--dbname ${database_name} ",
+    undef   => ""
+  }
+  $env = $database_password ? {
+    default => "PGPASSWORD=${database_password}",
+    undef   => undef,
+  }
+  $cmd = join([$cmd_init, $cmd_host, $cmd_user, $cmd_port, $cmd_dbname])
+  $validate_cmd = "/usr/local/bin/validate_postgresql_connection.sh ${sleep} ${tries} '${cmd}'"
+
+  # This is more of a safety valve, we add a little extra to compensate for the
+  # time it takes to run each psql command.
+  $timeout = (($sleep + 2) * $tries)
 
   $exec_name = "validate postgres connection for ${database_host}/${database_name}"
   exec { $exec_name:
-    command     => '/bin/false',
-    unless      => "/bin/echo \"SELECT 1\" | ${psql}",
+    command     => "echo 'Unable to connect to defined database using: ${cmd}' && false",
+    unless      => $validate_cmd,
     cwd         => '/tmp',
-    environment => "PGPASSWORD=${database_password}",
+    environment => $env,
     logoutput   => 'on_failure',
+    user        => $run_as,
+    path        => '/bin',
+    timeout     => $timeout,
     require     => Package['postgresql-client'],
   }
 
@@ -36,5 +68,7 @@
   # We accomplish this by using Puppet's resource collection syntax to search
   # for the Database resource in our current catalog; if it exists, the
   # appropriate relationship is created here.
-  Postgresql::Server::Database<|title == $database_name|> -> Exec[$exec_name]
+  if($create_db_first) {
+    Postgresql::Server::Database<|title == $database_name|> -> Exec[$exec_name]
+  }
 }

spec/system/validate_db_connection_spec.rb

@@ -25,6 +25,40 @@ class { 'postgresql::server': }
     end
   end
 
+  it 'should run puppet with no changes declared if socket connectivity works' do
+    pp = <<-EOS.unindent
+      postgresql::validate_db_connection { 'foo':
+        database_name => 'foo',
+        run_as        => 'postgres',
+      }
+    EOS
+
+    puppet_apply(pp) do |r|
+      r.exit_code.should == 0
+    end
+  end
+
+  it 'should keep retrying if database is down' do
+    # So first we shut the db down, then background a startup routine with a
+    # sleep 10 in front of it. That way rspec-system should continue while
+    # the pause and db startup happens in the background.
+    shell("/etc/init.d/postgresql* stop")
+    shell('nohup bash -c "sleep 10; /etc/init.d/postgresql* start" > /dev/null 2>&1 &')
+
+    pp = <<-EOS.unindent
+      postgresql::validate_db_connection { 'foo':
+        database_name => 'foo',
+        tries         => 30,
+        sleep         => 1,
+        run_as        => 'postgres',
+      }
+    EOS
+
+    puppet_apply(pp) do |r|
+      r.exit_code.should == 0
+    end
+  end
+
   it 'should run puppet with no changes declared if db ip connectivity works' do
     pp = <<-EOS.unindent
       postgresql::validate_db_connection { 'foo':

spec/unit/classes/server_spec.rb

@@ -14,6 +14,9 @@
   describe 'with no parameters' do
     it { should include_class("postgresql::params") }
     it { should include_class("postgresql::server") }
+    it 'should validate connection' do
+      should contain_postgresql__validate_db_connection('validate_service_is_running')
+    end
   end
 
   describe 'manage_firewall => true' do

spec/unit/defines/validate_db_connection_spec.rb

@@ -13,6 +13,10 @@
     'test'
   end
 
+  describe 'should work with only default parameters' do
+    it { should contain_postgresql__validate_db_connection('test') }
+  end
+
   describe 'should work with all parameters' do
     let :params do
       {
@@ -21,6 +25,9 @@
         :database_password => 'test',
         :database_username => 'test',
         :database_port => 5432,
+        :run_as => 'postgresq',
+        :sleep => 4,
+        :tries => 30,
       }
     end
     it { should contain_postgresql__validate_db_connection('test') }