Merge lp:~thedac/charms/precise/postgresql/package-holding into lp:charms/postgresql

[David Ames]

Allow the postgresql-$VERSION package to be held in production environments
Revert back to correct use of md5 for nagios check via pgpass

[Stuart Bishop (stub)]

An alternative approach is to dump the 'need_upgrade' logic in update_repos_and_packages(), and the charm should never attempt to upgrade the package (we already filter out already installed packages from the list of packages we punt to fetch.apt_install()). If this meets your concerns, it would be my preferred approach as discussions on the juju mailing list recently make me think that package upgrades is not the responsibility of the charm as it just can't be done well there (no hook is fired if a new security update is published in the archive). We have other solutions such as Landscape and automated security package installation for this.

If we need to keep this approach, it would be nice to validate the value of the new config item in validate_config(), since we don't have true enum support. If other charms would find it useful, consider shoving it in charm-helpers or perhaps a subordinate charm.

We should have a test for the nagios connection to ensure we don't break it again. https://code.launchpad.net/~stub/charms/precise/postgresql/fix-failover-tests/+merge/214316 has all the work to make this trivial in test.py (deploy relevant services and relations, introspect the relation settings for credentials, open a database connection via a SSH tunnel to confirm those credentials work from that unit). If you don't want to rebase from my branch and add the test, please open a bug so it doesn't get lost and I'll add one soon.

[Stuart Bishop (stub)]

Also, if we keep this approach , the call to ensure_package_status() should probably go in update_repos_and_packages() rather than config_changed(). This encapsulates all the apt stuff better, and will stop update_repos_and_packages() upgrading your packages before you get a chance to set the hold/install flag. At the moment, I think if you set the config item on an existing service then the packages will get upgraded before the hold takes effect.

review: Needs Fixing

[James Troup (elmo)]

Stuart Bishop <email address hidden> writes:

> An alternative approach is to dump the 'need_upgrade' logic in
> update_repos_and_packages(), and the charm should never attempt to
> upgrade the package (we already filter out already installed
> packages from the list of packages we punt to
> fetch.apt_install()). If this meets your concerns, it would be my
> preferred approach as discussions on the juju mailing list recently
> make me think that package upgrades is not the responsibility of the
> charm as it just can't be done well there (no hook is fired if a new
> security update is published in the archive). We have other
> solutions such as Landscape and automated security package
> installation for this.

So, to be clear this is not about trying to handle upgrades within the
charm. What we're trying to do is prevent a Landscape auto-upgrade
profile from upgrading postgres and taking down a production service
as a result). Landscape will respect package holds, so it seems like
(being able to) set package holds within the charm is the logical
place to do so.

--
James

[Stuart Bishop (stub)]

On 18 April 2014 22:51, James Troup <email address hidden> wrote:

> So, to be clear this is not about trying to handle upgrades within the
> charm. What we're trying to do is prevent a Landscape auto-upgrade
> profile from upgrading postgres and taking down a production service
> as a result). Landscape will respect package holds, so it seems like
> (being able to) set package holds within the charm is the logical
> place to do so.

This makes me wonder if it would be better to add a list of held
packages to the Landscape subordinate charm where all services can
make use of it. This would be a little awkward, as it requires the
operator to specify the primary PostgreSQL package name, whereas the
PostgreSQL charm knows the package name to hold.

(I'm not blocking on any of this - worst case is we document the
feature as experimental if we are not sure we want to support it in
its current form)

--
Stuart Bishop <email address hidden>

[David Ames (thedac)]

> An alternative approach is to dump the 'need_upgrade' logic in
> update_repos_and_packages(), and the charm should never attempt to upgrade the
> package (we already filter out already installed packages from the list of
> packages we punt to fetch.apt_install()). If this meets your concerns, it
> would be my preferred approach as discussions on the juju mailing list
> recently make me think that package upgrades is not the responsibility of the
> charm as it just can't be done well there (no hook is fired if a new security
> update is published in the archive). We have other solutions such as Landscape
> and automated security package installation for this.

I agree we should dump the need_upgrade logic as the charm should not be responsible for upgrading. However, the intent is to hold the packge such that landscape or other update services will also not upgrade postgresql (if held) in a production environment. This is standard for us in other charms.

> If we need to keep this approach, it would be nice to validate the value of
> the new config item in validate_config(), since we don't have true enum
> support. If other charms would find it useful, consider shoving it in charm-
> helpers or perhaps a subordinate charm.

Will do the validate_config().
We should look into adding this to charm-helpers.

> We should have a test for the nagios connection to ensure we don't break it
> again. https://code.launchpad.net/~stub/charms/precise/postgresql/fix-
> failover-tests/+merge/214316 has all the work to make this trivial in test.py
> (deploy relevant services and relations, introspect the relation settings for
> credentials, open a database connection via a SSH tunnel to confirm those
> credentials work from that unit). If you don't want to rebase from my branch
> and add the test, please open a bug so it doesn't get lost and I'll add one
> soon.

We should look at this as well.

> Also, if we keep this approach , the call to ensure_package_status() should
> probably go in update_repos_and_packages() rather than config_changed(). This
> encapsulates all the apt stuff better, and will stop
> update_repos_and_packages() upgrading your packages before you get a chance to
> set the hold/install flag. At the moment, I think if you set the config item
> on an existing service then the packages will get upgraded before the hold
> takes effect.

Initial testing having this in update_repos_and_packages() there is a bit of a chicken and egg problem. If the package is held before it is installed apt complains. We need to explore a robust solution for this.

I have created RT#69634 to track this process.

[lp:~thedac/charms/precise/postgresql/package-holding]

93. By David Ames on 2014-04-18

Use validate_config, move ensure_package_status to update_repos_and_packages(), handle when postgres not yet installed

[David Ames (thedac)]

> On 18 April 2014 22:51, James Troup <email address hidden> wrote:
>
> > So, to be clear this is not about trying to handle upgrades within the
> > charm. What we're trying to do is prevent a Landscape auto-upgrade
> > profile from upgrading postgres and taking down a production service
> > as a result). Landscape will respect package holds, so it seems like
> > (being able to) set package holds within the charm is the logical
> > place to do so.
>
> This makes me wonder if it would be better to add a list of held
> packages to the Landscape subordinate charm where all services can
> make use of it. This would be a little awkward, as it requires the
> operator to specify the primary PostgreSQL package name, whereas the
> PostgreSQL charm knows the package name to hold.
>
> (I'm not blocking on any of this - worst case is we document the
> feature as experimental if we are not sure we want to support it in
> its current form)

So again, this is pretty standard for IS charms (apache, haproxy, canonical-is-charms/postgresql) and we are just trying to bring postgres up to that standard.

I have

Check pagack_state with validate_config
Moved ensure_package_status to update_repos_and_packages()
Handled when postgres is not yet installed

Please take another look

[Stuart Bishop (stub)]

> I have
>
> Check pagack_state with validate_config
> Moved ensure_package_status to update_repos_and_packages()
> Handled when postgres is not yet installed
>
> Please take another look

Please change the bare except to 'except subprocess.CalledProcessError'. Bare excepts are always bugs, since they catch ugly exceptions you have no hope of handling correctly.

Please open a bug for adding the nagios connection test. Please open a bug for testing this new held package behavior. The charm is complex enough that features without tests will be broken. I'm happy to add the tests.

Otherwise, good to go.

review: Approve

[Stuart Bishop (stub)]

IF you would rather avoid the run() call to dpkg entirely, I think you will be able to tell if the postgresql package is installed by looking at the filtered list of packages to install.

packages = fetch.filter_installed_packages(packages)

At this point, I think charm helpers has removed any packages already installed, so if postgresql-x.x is not in the list you are good to hold/unhold it.

[lp:~thedac/charms/precise/postgresql/package-holding]

94. By David Ames on 2014-05-06

Use filtered package list to determine if postgres is installed

[UCI Engine Bot (ue-uci-engine)]

Used filtered package list. Please do final review and merge.

Bugs filed:
https://bugs.launchpad.net/charms/+bug/1316361
https://bugs.launchpad.net/charms/+bug/1316362

=== modified file 'config.yaml'
--- config.yaml	2014-04-04 12:05:49 +0000
+++ config.yaml	2014-05-06 00:01:34 +0000
@@ -432,3 +432,9 @@
     type: string
     default: ""
     description: EXPERIMENTAL. OpenStack Swift tenant name.
+  package_status:
+    default: "install"
+    type: string
+    description: |
+        The status of service-affecting packages will be set to this value in the dpkg database.
+        Useful valid values are "install" and "hold".

=== modified file 'hooks/hooks.py'
--- hooks/hooks.py	2014-04-28 13:49:07 +0000
+++ hooks/hooks.py	2014-05-06 00:01:34 +0000
@@ -866,10 +866,23 @@
         local_state[name] = config_data.get(name, None)
     local_state.save()
 
+    package_status = config_data['package_status']
+    if package_status not in ['install', 'hold']:
+        valid = False
+        log("package_status must be 'install' or 'hold' not '{}'"
+            "".format(package_status), CRITICAL)
+
     if not valid:
         sys.exit(99)
 
 
+def ensure_package_status(package, status):
+    selections = ''.join(['{} {}\n'.format(package, status)])
+    dpkg = subprocess.Popen(['dpkg', '--set-selections'],
+                                stdin=subprocess.PIPE)
+    dpkg.communicate(input=selections)
+
+
 #------------------------------------------------------------------------------
 # Core logic for permanent storage changes:
 # NOTE the only 2 "True" return points:
@@ -1584,6 +1597,10 @@
         packages.append('pgtune')
     packages.extend((hookenv.config('extra-packages') or '').split())
     packages = fetch.filter_installed_packages(packages)
+    # Set package state for main postgresql package if installed
+    if 'postgresql-{}'.format(version) not in packages:
+        ensure_package_status('postgresql-{}'.format(version), 
+                              hookenv.config('package_status'))
     fetch.apt_install(packages, fatal=True)
 
 

=== modified file 'templates/pg_hba.conf.tmpl'
--- templates/pg_hba.conf.tmpl	2014-03-04 21:50:06 +0000
+++ templates/pg_hba.conf.tmpl	2014-05-06 00:01:34 +0000
@@ -5,7 +5,7 @@
 # Database administrative login by UNIX sockets
 local   all         root,postgres                     ident map=superusers
 local   replication root,postgres                     ident map=superusers
-local   all         nagios                            ident
+local   all         nagios                            md5
 
 {% if access_list is defined -%}
 {% for unit in access_list -%}