Fix service status detection on Debian-based OSes #1349
Conversation
postgresql::params is a classBreaking changes to this file WILL impact these 2 modules (exact match):Breaking changes to this file MAY impact these 3 modules (near match):This module is declared in 70 of 579 indexed public
|
There was a problem hiding this comment.
Would a similar fix work for Debian? Perhaps then all the logic to determine $service_status can be made trivial. In fact, isn't this the default implementation? So just set it to undef?
You could also copy this for non-systemd support (which Debian still officially has):
puppetlabs-postgresql/manifests/params.pp
Lines 85 to 91 in 821b5c5
Now I also see that RH-based distros don't respect the service_status global parameter, which is a bug in itself.
|
I would think it would also work on Debian. After reading all this it wasn't quite clear to me as to why all this logic is there (instead of, as you mentioned, rely on the default, which is working fine) so I didn't dare wiping it, just fixed its behavior for 18 and up. I'd be all for removing all this :) |
I opened #1351 for this.
I'd be in favor of this. @david22swan any thoughts? |
This is to test out if puppetlabs/puppetlabs-postgresql#1349 could fix our broken Ubuntu 20.04 tests.
|
@ekohl @david22swan I rewrote it to match the bits @ekohl added for RHEL, tested (successfully) it on Debian 10/11, Ubuntu {18,20,22}.04. I think people who rely on other service tooling than |
| } elsif $facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '18.04') >= 0 { | ||
| $service_status = pick($service_status, "/usr/sbin/service ${service_name}@*-main status") | ||
| } elsif $facts['os']['name'] == 'Ubuntu' and versioncmp($facts['os']['release']['major'], '15.04') >= 0 { | ||
| # Ubuntu releases since vivid use systemd | ||
| $service_status = pick($service_status, "/usr/sbin/service ${service_name} status") | ||
| if pick($service_provider, $facts['service_provider']) == 'systemd' { | ||
| $service_reload = "systemctl reload ${service_name}" | ||
| $service_status = pick($service_status, "systemctl status ${service_name}") | ||
| } else { | ||
| $service_status = pick($service_status, "/etc/init.d/${service_name} status | /bin/egrep -q 'Running clusters: .+|online'") | ||
| $service_reload = "service ${service_name} reload" | ||
| $service_status = pick($service_status, "service ${service_name} status") |
There was a problem hiding this comment.
Oh, I think these paths need to be absolute.
There was a problem hiding this comment.
Oh yeah, I cheated a bit, as it moved from /bin/ to /usr/bin/ or the other way around, but I think both work anyway (as does omitting the path, but I agree, having full path is better :) ).
There was a problem hiding this comment.
Oh disregard that, I copy/pasted this bit, throughout that file the commands are used without absolute path. I think we could/should change that, but I think now local style is "without full path"
There was a problem hiding this comment.
I'd be OK with that (I've seen that Puppet does apply $PATH to it, unlike exec), but then the unit tests should match.
There was a problem hiding this comment.
Thanks! I think SLES is unrelated but I can't tell because https://github.com/puppetlabs/puppetlabs-postgresql/actions/runs/2653307902 excluded SLES.
ekohl
changed the title
/usr/sbin/service postgresql@*-main statuson at least Ubuntu 18.04, 20.04 and 22.04, always exits 0 (even when service is down), so puppet always believes the service is up (which both causes all kinds of failures and prevents puppet from starting the stopped service).Not sure why all the convoluted logic in this bit is needed, but this fixes it for me on 18 and 20 (and also 22 but that's not yet supported by this module).