Puppet Expertise Needed: Chasing Installer PostgreSQL Role Already Exists Errors

Devan_Goodwin1 · April 25, 2014, 7:53pm

I've been chasing a solution to this bug:
http://projects.theforeman.org/issues/5012

Bearing in mind I have little to no puppet background, I wanted to solicit
some ideas before I try reporting this upstream against the puppet
postgresql module.

First discovery was that use of ~> resource chaining was involved, for
example to get rid of the candlepin role error, all I had to do was change
line 88 here from ~> to ->, implying that candlepin::config should not
tell candlepin::database to refresh itself if the config changes.

https://github.com/Katello/katello-installer/blob/master/modules/candlepin/manifests/init.pp#L88

However in PR I discovered that this is not a suitable fix as it may cause
services to stop getting restarted on config changes.

Since then I've been trying to track down why the problem only appears with
notification chaining. I have established that deleting the database/role
and re-running the installer does not cause the issue, but also deleting
a candlepin config file will cause the issue. I.e. my quickest reproducer:

service tomcat6 stop && su - postgres -c 'dropdb candlepin' && su -
postgres -c "psql -c 'drop user candlepin'" && rm -f /etc/tomcat6/server.xml

katello-devel-installer --user=dgoodwin --group=dgoodwin
–deployment-dir=/home/dgoodwin -v -d

With some extensive logging added into the puppet postgresql module I see
something like this:

http://fpaste.org/97055/98454560/

I can simplify the output but basically the problem (and difference between
this and a successful run) starts at line #14 in the second paste above
when we see refresh called, which is found here:

github.com

puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/type/postgresql_psql.rb#L87


newparam(:search_path) do
  desc "The schema search path to use when executing the SQL command"
end


newparam(:psql_path) do
  desc "The path to psql executable."
  defaultto("psql")
end


newparam(:psql_user) do
  desc "The system user account under which the psql command should be executed."
  defaultto("postgres")
end


newparam(:psql_group) do
  desc "The system user group account under which the psql command should be executed."
  defaultto("postgres")
end


newparam(:cwd, :parent => Puppet::Parameter::Path) do
  desc "The working directory under which the psql command should be executed."

Immediately I am confused as to why the resource is run once, then refresh
is run, I would assume that puppet is going to combine the two into one
pass.

So my first question, does that behaviour sound like sane puppet behaviour?
We appear to have a resource created once, run, then refreshed.

I'm assuming the answer to that is yes because perhaps refresh does some
special things, and puppet is doing the right thing. At that point I am
suspecting the bug exists here:

github.com

puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/provider/postgresql_psql/ruby.rb

Puppet::Type.type(:postgresql_psql).provide(:ruby) do

  def run_unless_sql_command(sql)
    # for the 'unless' queries, we wrap the user's query in a 'SELECT COUNT',
    # which makes it easier to parse and process the output.
    run_sql_command('SELECT COUNT(*) FROM (' <<  sql << ') count')
  end

  def run_sql_command(sql)
    if resource[:search_path]
      sql = "set search_path to #{Array(resource[:search_path]).join(',')}; #{sql}"
    end

    command = [resource[:psql_path]]
    command.push("-d", resource[:db]) if resource[:db]
    command.push("-p", resource[:port]) if resource[:port]
    command.push("-t", "-c", '"' + sql.gsub('"', '\"') + '"')

    environment = get_environment

This file has been truncated. show original

command() is called only once in this scenario, it correctly checks the
unless, sees the role does not exist, and proceeds to call command=(). The
command runs, then postgresql_psql.rb triggers refresh, which calls sync,
which ends up calling command=() again, but unless condition has already
been checked.

So I am wondering if command=() should be the point at which unless is
checked?

Any help greatly appreciated, I suspect this is very close to where the
issue is but expertise lacking to figure out how to fix. Opening the issue
upstream might be an option but would love to know if others agree this
appears to be the issue first.

Thanks,

Devan

Dominic_Cleal · April 28, 2014, 2:21pm

> I've been chasing a solution to this bug:
> Bug #5012: Candlepin role already exists - Katello - Foreman
>
> Bearing in mind I have little to no puppet background, I wanted to
> solicit some ideas before I try reporting this upstream against the
> puppet postgresql module.
>
> First discovery was that use of ~> resource chaining was involved, for
> example to get rid of the candlepin role error, all I had to do was
> change line 88 here from ~> to ->, implying that candlepin::config
> should not tell candlepin::database to refresh itself if the config
> changes.
>
> https://github.com/Katello/katello-installer/blob/master/modules/candlepin/manifests/init.pp#L88
>
> However in PR I discovered that this is not a suitable fix as it may
> cause services to stop getting restarted on config changes.
>
> Since then I've been trying to track down why the problem only appears
> with notification chaining. I have established that deleting the
> database/role and re-running the installer does not cause the issue,
> but also deleting a candlepin config file will cause the issue. I.e.
> my quickest reproducer:
>
> service tomcat6 stop && su - postgres -c 'dropdb candlepin' && su -
> postgres -c "psql -c 'drop user candlepin'" && rm -f /etc/tomcat6/server.xml
>
> katello-devel-installer --user=dgoodwin --group=dgoodwin
> --deployment-dir=/home/dgoodwin -v -d
>
> With some extensive logging added into the puppet postgresql module I
> see something like this:
>
> http://fpaste.org/97055/98454560/
>
> I can simplify the output but basically the problem (and difference
> between this and a successful run) starts at line #14 in the second
> paste above when we see refresh called, which is found here:
>
> https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/type/postgresql_psql.rb#L87
>
> Immediately I am confused as to why the resource is run once, then
> refresh is run, I would assume that puppet is going to combine the two
> into one pass.

Be careful with the word "run", it doesn't really mean anything in
Puppet's terminology, so is ambiguous.

The two events you see are different: the first is Puppet synchronising
the resource (checking its properties are in the expected state and
changing/syncing them if not, which only ever happens once), and the
second is a notification (refresh) arising from a change in another
resource.

These are almost always held to be separate events. (Services are the
only exception, which don't need refreshing if they've already
synchronised to the 'running' state.)

> So my first question, does that behaviour sound like sane puppet
> behaviour? We appear to have a resource created once, run, then refreshed.
>
> I'm assuming the answer to that is yes because perhaps refresh does some
> special things, and puppet is doing the right thing. At that point I am
> suspecting the bug exists here:
>
> https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/provider/postgresql_psql/ruby.rb

Correct, it's sane Puppet behaviour, but IMHO the type isn't handling it
properly. exec follows a similar pattern, but obeys the "unless" parameter.

> command() is called only once in this scenario, it correctly checks the
> unless, sees the role does not exist, and proceeds to call command=().
> The command runs, then postgresql_psql.rb triggers refresh, which calls
> sync, which ends up calling command=() again, but unless condition has
> already been checked.
>
> So I am wondering if command=() should be the point at which unless is
> checked?
>
> Any help greatly appreciated, I suspect this is very close to where the
> issue is but expertise lacking to figure out how to fix. Opening the
> issue upstream might be an option but would love to know if others agree
> this appears to be the issue first.

I think command=() is a bit too low-level to be the right place to
implement it, though it'd probably work.

I'd suggest that the refresh() method on the type or maybe the sync()
method on the command property check whether the property is in sync
before forcing the sync. The exec type uses checks (part of the types
system) and implements this in a completely different way - though in
reality, these two types/providers should mirror each other, or even
better, use shared code.

···

On 25/04/14 20:53, Devan Goodwin wrote:

–
Dominic Cleal
Red Hat Engineering

Devan_Goodwin1 · April 28, 2014, 4:29pm

Thanks for the feedback, I've got an upstream issue filed,
https://tickets.puppetlabs.com/browse/PUP-2376

Will see if that gets any traction and try to work out something that might
be a safe fix if not, and attempt to do so in the type rather than the
provider.

Cheers,

Devan

···

On Mon, Apr 28, 2014 at 11:21 AM, Dominic Cleal wrote:

On 25/04/14 20:53, Devan Goodwin wrote:

I’ve been chasing a solution to this bug:
Bug #5012: Candlepin role already exists - Katello - Foreman

Bearing in mind I have little to no puppet background, I wanted to
solicit some ideas before I try reporting this upstream against the
puppet postgresql module.

First discovery was that use of ~> resource chaining was involved, for
example to get rid of the candlepin role error, all I had to do was
change line 88 here from ~> to ->, implying that candlepin::config
should not tell candlepin::database to refresh itself if the config
changes.

https://github.com/Katello/katello-installer/blob/master/modules/candlepin/manifests/init.pp#L88

However in PR I discovered that this is not a suitable fix as it may
cause services to stop getting restarted on config changes.

Since then I’ve been trying to track down why the problem only appears
with notification chaining. I have established that deleting the
database/role and re-running the installer does not cause the issue,
but also deleting a candlepin config file will cause the issue. I.e.
my quickest reproducer:

service tomcat6 stop && su - postgres -c ‘dropdb candlepin’ && su -
postgres -c “psql -c ‘drop user candlepin’” && rm -f
/etc/tomcat6/server.xml

katello-devel-installer --user=dgoodwin --group=dgoodwin
–deployment-dir=/home/dgoodwin -v -d

With some extensive logging added into the puppet postgresql module I
see something like this:

http://fpaste.org/97055/98454560/

I can simplify the output but basically the problem (and difference
between this and a successful run) starts at line #14 in the second
paste above when we see refresh called, which is found here:

https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/type/postgresql_psql.rb#L87

Immediately I am confused as to why the resource is run once, then
refresh is run, I would assume that puppet is going to combine the two
into one pass.

Be careful with the word “run”, it doesn’t really mean anything in
Puppet’s terminology, so is ambiguous.

The two events you see are different: the first is Puppet synchronising
the resource (checking its properties are in the expected state and
changing/syncing them if not, which only ever happens once), and the
second is a notification (refresh) arising from a change in another
resource.

These are almost always held to be separate events. (Services are the
only exception, which don’t need refreshing if they’ve already
synchronised to the ‘running’ state.)

So my first question, does that behaviour sound like sane puppet
behaviour? We appear to have a resource created once, run, then
refreshed.

I’m assuming the answer to that is yes because perhaps refresh does some
special things, and puppet is doing the right thing. At that point I am
suspecting the bug exists here:

https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/provider/postgresql_psql/ruby.rb

Correct, it’s sane Puppet behaviour, but IMHO the type isn’t handling it
properly. exec follows a similar pattern, but obeys the "unless"
parameter.

command() is called only once in this scenario, it correctly checks the
unless, sees the role does not exist, and proceeds to call command=().
The command runs, then postgresql_psql.rb triggers refresh, which calls
sync, which ends up calling command=() again, but unless condition has
already been checked.

So I am wondering if command=() should be the point at which unless is
checked?

Any help greatly appreciated, I suspect this is very close to where the
issue is but expertise lacking to figure out how to fix. Opening the
issue upstream might be an option but would love to know if others agree
this appears to be the issue first.

I think command=() is a bit too low-level to be the right place to
implement it, though it’d probably work.

I’d suggest that the refresh() method on the type or maybe the sync()
method on the command property check whether the property is in sync
before forcing the sync. The exec type uses checks (part of the types
system) and implements this in a completely different way - though in
reality, these two types/providers should mirror each other, or even
better, use shared code.

–
Dominic Cleal
Red Hat Engineering

–
You received this message because you are subscribed to the Google Groups
"foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an
email to foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Dominic_Cleal · April 28, 2014, 4:35pm

It should probably be filed as a bug against the module, not against
Puppet core. (https://tickets.puppetlabs.com/browse/MODULES)

···

-- Dominic Cleal Red Hat Engineering

On 28/04/14 17:29, Devan Goodwin wrote:

Thanks for the feedback, I’ve got an upstream issue filed,
https://tickets.puppetlabs.com/browse/PUP-2376

Will see if that gets any traction and try to work out something that
might be a safe fix if not, and attempt to do so in the type rather than
the provider.

Cheers,

Devan

On Mon, Apr 28, 2014 at 11:21 AM, Dominic Cleal <dcleal+g@redhat.com > mailto:dcleal+g@redhat.com> wrote:

On 25/04/14 20:53, Devan Goodwin wrote:
> I've been chasing a solution to this bug:
> http://projects.theforeman.org/issues/5012
>
> Bearing in mind I have little to no puppet background, I wanted to
> solicit some ideas before I try reporting this upstream against the
> puppet postgresql module.
>
> First discovery was that use of ~> resource chaining was involved, for
> example to get rid of the candlepin role error, all I had to do was
> change line 88 here from ~> to ->, implying that candlepin::config
> should *not* tell candlepin::database to refresh itself if the config
> changes.
>
>
https://github.com/Katello/katello-installer/blob/master/modules/candlepin/manifests/init.pp#L88
>
> However in PR I discovered that this is not a suitable fix as it may
> cause services to stop getting restarted on config changes.
>
> Since then I've been trying to track down why the problem only appears
> with notification chaining. I have established that deleting the
> database/role and re-running the installer does *not* cause the issue,
> but also deleting a candlepin config file *will* cause the issue. I.e.
> my quickest reproducer:
>
> service tomcat6 stop && su - postgres -c 'dropdb candlepin' && su -
> postgres -c "psql -c 'drop user candlepin'" && rm -f
/etc/tomcat6/server.xml
>
> katello-devel-installer --user=dgoodwin --group=dgoodwin
> --deployment-dir=/home/dgoodwin -v -d
>
> With some extensive logging added into the puppet postgresql module I
> see something like this:
>
> http://fpaste.org/97055/98454560/
>
> I can simplify the output but basically the problem (and difference
> between this and a successful run) starts at line #14 in the second
> paste above when we see refresh called, which is found here:
>
>
https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/type/postgresql_psql.rb#L87
>
> Immediately I am confused as to why the resource is run once, then
> refresh is run, I would assume that puppet is going to combine the two
> into one pass.

Be careful with the word "run", it doesn't really mean anything in
Puppet's terminology, so is ambiguous.

The two events you see are different: the first is Puppet synchronising
the resource (checking its properties are in the expected state and
changing/syncing them if not, which only ever happens once), and the
second is a notification (refresh) arising from a change in another
resource.

These are almost always held to be separate events.  (Services are the
only exception, which don't need refreshing if they've already
synchronised to the 'running' state.)

> So my first question, does that behaviour sound like sane puppet
> behaviour? We appear to have a resource created once, run, then
refreshed.
>
> I'm assuming the answer to that is yes because perhaps refresh
does some
> special things, and puppet is doing the right thing. At that point
I am
> suspecting the bug exists here:
>
>
https://github.com/puppetlabs/puppetlabs-postgresql/blob/master/lib/puppet/provider/postgresql_psql/ruby.rb

Correct, it's sane Puppet behaviour, but IMHO the type isn't handling it
properly.  exec follows a similar pattern, but obeys the "unless"
parameter.

> command() is called only once in this scenario, it correctly
checks the
> unless, sees the role does not exist, and proceeds to call command=().
> The command runs, then postgresql_psql.rb triggers refresh, which
calls
> sync, which ends up calling command=() again, but unless condition has
> already been checked.
>
> So I am wondering if command=() should be the point at which unless is
> checked?
>
> Any help greatly appreciated, I suspect this is very close to
where the
> issue is but expertise lacking to figure out how to fix. Opening the
> issue upstream might be an option but would love to know if others
agree
> this appears to be the issue first.

I think command=() is a bit too low-level to be the right place to
implement it, though it'd probably work.

I'd suggest that the refresh() method on the type or maybe the sync()
method on the command property check whether the property is in sync
before forcing the sync.  The exec type uses checks (part of the types
system) and implements this in a completely different way - though in
reality, these two types/providers should mirror each other, or even
better, use shared code.

--
Dominic Cleal
Red Hat Engineering

--
You received this message because you are subscribed to the Google
Groups "foreman-dev" group.
To unsubscribe from this group and stop receiving emails from it,
send an email to foreman-dev+unsubscribe@googlegroups.com
<mailto:foreman-dev%2Bunsubscribe@googlegroups.com>.
For more options, visit https://groups.google.com/d/optout.

–
You received this message because you are subscribed to the Google
Groups “foreman-dev” group.
To unsubscribe from this group and stop receiving emails from it, send
an email to foreman-dev+unsubscribe@googlegroups.com
mailto:foreman-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Devan_Goodwin1 · April 28, 2014, 6:40pm

Re-filed: https://tickets.puppetlabs.com/browse/MODULES-775

···

On Mon, Apr 28, 2014 at 1:35 PM, Dominic Cleal wrote:

It should probably be filed as a bug against the module, not against
Puppet core. (https://tickets.puppetlabs.com/browse/MODULES)