geerlingguy / ansible-role-varnish

Ansible Role - Varnish HTTP accelerator
https://galaxy.ansible.com/geerlingguy/varnish/
MIT License
81 stars 89 forks source link

reload fails for 4.1 #47

Closed wesnick closed 4 years ago

wesnick commented 7 years ago

OS: Ubuntu 16.04

systemd Reload configuration calls a script that looks eerily like old init.d script.

ExecReload=/usr/share/varnish/reload-vcl

This script in turn gets params from here: /etc/default/varnish which is not touched by this role.

When calling an ansible systemd reload handler (use case: update default.vcl but don't want to lose memory contents)

- name: reload varnish
  systemd: name=varnish state=reloaded

This fails because admin host is defined as "localhost" in untouched params file and "127.0.0.1" in ExecStart

I am happy to roll a patch for this.

geerlingguy commented 7 years ago

@wesnick - Don't have time to gather context right now, but is the systemd config also managed by this role? If so, can we just change the 127.0.0.1 to localhost?

Otherwise, it would be okay to have some parts of /etc/default/varnish managed by this role (I thought I remember managing bits of it in the past)—I'd prefer to be surgical about it, e.g. using lineinfile.

geerlingguy commented 7 years ago

Also, it looks like we should be using the systemd config file (and not /etc/default/varnish) for Xenial: https://github.com/geerlingguy/ansible-role-varnish/blob/master/tasks/main.yml#L36-L45

wesnick commented 7 years ago

I checked upstream. It looks like the reload script in the systemd config file is a holdover from init.d days https://bugs.launchpad.net/ubuntu/+source/varnish/+bug/1573561

In this script the old /etc/default/varnish file is sourced

$ cat /usr/share/varnish/reload-vcl |grep defaults
defaults=/etc/default/varnish
# msg_defaults_not_readable: defaults
msg_defaults_not_readable="Error: %s is not readable\n"
# msg_defaults_not_there: defaults
msg_defaults_not_there="Error: %s does not exist\n"
# Load defaults file
if [ -f "$defaults" ]
    if [ -r "$defaults" ]
    . "$defaults"
    printf >&2 "$msg_defaults_not_readable" $defaults
    printf >&2 "$msg_defaults_not_there" $defaults

I propose a patch that will only affect xenial, do a lineinfile on the line

defaults=/etc/default/varnish

and set it to

defaults=/etc/varnish/varnish.params

In reality this varnish.params file is not ever used by systemd, it is not loaded in the config file and even if it were, systemd does not do param expansion the way init.d scripts did. http://unix.stackexchange.com/questions/216780/why-does-bash-parameter-expansion-not-work-inside-systemd-service-files

wesnick commented 7 years ago

another interesting link https://github.com/varnishcache/pkg-varnish-cache/issues/24

wesnick commented 7 years ago

I have implemented the above fix in a forked version of this repo and reload works great for me now. I am willing to roll a patch only targeting xenial and add a test. Just say the word.

stale[bot] commented 4 years ago

This issue has been marked 'stale' due to lack of recent activity. If there is no further activity, the issue will be closed in another 30 days. Thank you for your contribution!

Please read this blog post to see the reasons why I mark issues as stale.

stale[bot] commented 4 years ago

This issue has been closed due to inactivity. If you feel this is in error, please reopen the issue or file a new issue with the relevant details.