In handlers/restart_services.yml there is:
handlers/restart_services.yml
- name: restart fedmsg-gateway command: /usr/local/bin/conditional-restart.sh fedmsg-gateway fedmsg-gateway - name: restart fedmsg-hub command: /usr/local/bin/conditional-restart.sh fedmsg-hub fedmsg-hub # Note that, we're cool with arbitrary restarts on bodhi-backend02, just # not bodhi-backend01 or bodhi-backend03. 01 and 03 is where the releng/mash· # stuff happens and we # don't want to interrupt that. when: inventory_hostname not in ['bodhi-backend01.phx2.fedoraproject.org', 'bodhi-backend03.phx2.fedoraproject.org'] - name: restart fedmsg-irc command: /usr/local/bin/conditional-restart.sh fedmsg-irc fedmsg-irc - name: restart fedmsg-relay command: /usr/local/bin/conditional-restart.sh fedmsg-relay fedmsg-relay
All of that is broken (at least on recent Fedoras). Because there are no packages named fedmsg-gateway, fedmsg-hub, fedmsg-irc and fedmsg-relay. Therefore the service is never restarted.
fedmsg-gateway
fedmsg-hub
fedmsg-irc
fedmsg-relay
I wanted to fix it, but I don't really understand the purpose of conditional-restart.sh. Why checking a package name in order to restart a service? Why not use the standard approach that is used for all other services in that file? E.g.:
conditional-restart.sh
- name: restart fedmsg-hub service: name=fedmsg-hub state=restarted
Thanks for resolving this.
CC @jskladan @frantisekz
Why checking a package name in order to restart a service? Why not use the standard approach that is used for all other services in that file?
I think the idea is to avoid restarting the service where it's not running or not installed.
I'm not sure how service action behaves when it's not installed. But even that was the case, why is it specific to just fedmsg?
service
Right, this has broken with @jclines move of everything into on fedmsg package.
The idea of the script was when they were split out, you never would know which subpackages were installed. So, when you changed fedmsg-config and wanted to restart fedmsg services that depended on that, you couldn't know which of them was installed or running. In some cases packages would be installed, but the service wasn't running and you didn't want to start it in that case.
So, I guess the easiest fix is to change all those handlers to call the script with 'current-name python2-fedmsg' so if python2-fedmsg is installed restart current-name, if not just say it worked and go on.
Does that make sense. I can make a patch sometime soon if you like...
The idea of the script was when they were split out, you never would know which subpackages were installed. So, when you changed fedmsg-config and wanted to restart fedmsg services that depended on that, you couldn't know which of them was installed
Wouldn't it be safer to check for the presence of the service file, rather than depend on a package name? The first approach is less likely to break and universal (if I want to restart fedmsg-irc, I check that fedmsg-irc.service exists) than the latter one (combination of a service name and a package name).
fedmsg-irc.service
or running. In some cases packages would be installed, but the service wasn't running and you didn't want to start it in that case.
That's not checked by conditional-restart.sh currently, though.
Sure, that should fix the issue for now. It still seems fragile to me, though. But definitely better than having it non-functional :)
But you also need to know if the fedmsg-irc service is enabled and running. If it's not and you restart it, it will be running after this when it shouldn't be.
or running. In some cases packages would be installed, but the service wasn't running and you didn't want to start it in that case. That's not checked by conditional-restart.sh currently, though.
But with the old subpackages you would only have something installed you wanted to run. Now with all of them in one package you don't know.
So, I guess the easiest fix is to change all those handlers to call the script with 'current-name python2-fedmsg' so if python2-fedmsg is installed restart current-name, if not just say it worked and go on. Sure, that should fix the issue for now. It still seems fragile to me, though. But definitely better than having it non-functional :)
Or, we could just redo this script. Basically have it check if any of the services are enabled, if so, if they are running, if so, restart them. If not exit 0 and go on?
I think the assumption that a package installed means a service enabled is... too presuming.
Yes, that is the best way I believe. You can use systemctl show for that.
systemctl show
Also, it's a bit confusing that different services in handlers/restart_services.yml use a different approach. People might get caught on that.
I'm quite surprised (and disappointed) the standard ansible service module can't do this. Might be worth a pull request. Or writing your own ansible module wrapping that one (which is a bit cleaner than a shell script). But this is a just a side remark.
ok. I solved this by changing the wrapper script to just call 'systemctl try-restart'.
I wish ansible systemd module supported this. ;(
Metadata Update from @kevin: - Issue close_status updated to: Fixed - Issue status updated to: Closed (was: Open)
Login to comment on this ticket.