-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes #9875: Better docker service restart #56
Conversation
Can you file an issue here: http://projects.theforeman.org/projects/foreman/issues/new and update the commit message to indicate that it fixes that issue, for example: |
Improve the check for the docker service in order to restart it if needed. Also check for systemd based system first and then fall back to the service based utilities.
14c81fc
to
af4751a
Compare
@mccun934 update, thanks for pointing the guidelines. |
#restart if docker service is installed | ||
service docker status >/dev/null && \ | ||
# restart docker if it is installed and running | ||
if [ -f /usr/lib/systemd/system/docker.service ]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit files installed by packages are placed in /usr/lib/systemd/system/
, but system administrators may also install unit files in /etc/systemd/system/
. An administrator might even do something crazy like nuke the default service file and install a custom one to /etc/systemd/system/
. As a result, this sort of test may be better:
if which systemctl >& /dev/null; then
echo 'systemctl is available'
elif which service >& /dev/null; then
echo 'service is available'
fi
>&
sends both stdout and stderr to the specified file handle. See here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ichimonji10 even though I use the check you proposed I will have to check for those unit files to know if docker is installed or not. If the sysadmin changes places so the message showing that the services was restarted will not be printed.
Also other stuff I left according to what I found in other places in this codebase. But I can definitely update as per your suggestions, let's see what others tell about them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elyezer, I totally forgot about checking for docker.service in particular, so my example code is incomplete. Whoops! Still, I think there's a case to be made for using utilities to check for the presence of unit files or service files instead of directly querying the filesystem for the presence of files. I'd love to hear what others have to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example deals with the case where the systemctl
executable and docker.service
unit file may or may not exist in standard locations on the system:
if systemctl list-unit-files docker.service | grep '^docker.service' >&/dev/null; then
echo 'systemctl docker.service may be used'
elif [ -f /etc/init.d/docker ]; then
echo 'service docker may be used'
fi
I am not terribly knowledgeable about service
and where service files may exist on a system.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ichimonji10 I tried your suggestion and get the following. Also I have to remove the docker.service option from list-unit-files
. I will need to deal with the output of systemd not being available and that will make the code bigger. I will stick with 'Simple is better than complex.' here.
[root@rhel66 ~]# if systemctl list-unit-files | grep '^docker.service' >&/dev/null; then echo 'have docker'; else echo 'does not have docker'; fi
-bash: systemctl: command not found
does not have docker
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@elyezer OK. That sounds good.
@parthaa can you take a look at this? |
Thanks @elyezer @Ichimonji10 ! |
Fixes #9875: Better docker service restart
❤️ |
Thank you! |
Improve the check for the docker service in order to restart it if
needed. Also check for systemd based system first and then fall back to
the service based utilities.