<div dir="ltr"><div><div><div>Hi Gauthier,<br><br></div>We've discussed these on irc today and I've now removed ExecStartPre and committed your diff for the sysv init script.<br><br></div>Thanks for the diff.<br><br></div>Cheers.<br></div><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 27, 2015 at 9:38 PM, Federico Schwindt <span dir="ltr"><<a href="mailto:fgsch@lodoss.net" target="_blank">fgsch@lodoss.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div><span class=""><div>>
Seems the prestart is required for reloads.<br><br></div></span>I haven't tested RH but this doesn't seem to be the case in Debian - ExecStartPre is only called before (re)start.<br><br></div>The sysv init script change might be needed though. Setting vmod_dir will also affects the test btw.<br></div><div class="HOEnZb"><div class="h5"><div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jul 27, 2015 at 11:04 AM, Delacroix, Gauthier <span dir="ltr"><<a href="mailto:Gauthier.Delacroix@coreye.fr" target="_blank">Gauthier.Delacroix@coreye.fr</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Federico,<br>
<br>
Seems the prestart is required for reloads.<br>
<br>
Here is a new patch, just including DAEMON_OPTS in both tests.<br>
<br>
Gauthier<br>
<br>
>From 4c11e58d6270591f93be58551496412e3118aeda Mon Sep 17 00:00:00 2001<br>
From: Gauthier Delacroix <<a href="mailto:gauthier.delacroix@coreye.fr" target="_blank">gauthier.delacroix@coreye.fr</a>><br>
Date: Mon, 27 Jul 2015 11:55:13 +0200<br>
Subject: [PATCH] Include DAEMON_OPTS in service prestart tests<br>
<br>
---<br>
redhat/varnish.initrc | 2 +-<br>
redhat/varnish.service | 2 +-<br>
2 files changed, 2 insertions(+), 2 deletions(-)<br>
<br>
diff --git a/redhat/varnish.initrc b/redhat/varnish.initrc<br>
index 117e334..03f47fb 100755<br>
--- a/redhat/varnish.initrc<br>
+++ b/redhat/varnish.initrc<br>
@@ -126,7 +126,7 @@ rh_status_q() {<br>
<span><br>
configtest() {<br>
if [ -f "$VARNISH_VCL_CONF" ]; then<br>
- $exec -f "$VARNISH_VCL_CONF" -C -n /tmp > /dev/null && echo "Syntax ok"<br>
</span>+ $exec -f "$VARNISH_VCL_CONF" -C -n /tmp $DAEMON_OPTS > /dev/null && echo "Syntax ok"<br>
else<br>
<span> echo "VARNISH_VCL_CONF is unset or does not point to a file"<br>
</span> fi<br>
diff --git a/redhat/varnish.service b/redhat/varnish.service<br>
index a4f3355..ca93b3d 100644<br>
<span>--- a/redhat/varnish.service<br>
+++ b/redhat/varnish.service<br>
@@ -27,7 +27,7 @@ EnvironmentFile=/etc/varnish/varnish.params<br>
Type=forking<br>
PIDFile=/var/run/varnish.pid<br>
PrivateTmp=true<br>
-ExecStartPre=/usr/sbin/varnishd -C -f $VARNISH_VCL_CONF<br>
</span>+ExecStartPre=/usr/sbin/varnishd -C -f $VARNISH_VCL_CONF $DAEMON_OPTS<br>
<span> ExecStart=/usr/sbin/varnishd \<br>
-P /var/run/varnish.pid \<br>
-f $VARNISH_VCL_CONF \<br>
</span>--<br>
1.8.3.msysgit.0<br>
<span><br>
De : Federico Schwindt [mailto:<a href="mailto:fgsch@lodoss.net" target="_blank">fgsch@lodoss.net</a>]<br>
</span>Envoyé : mercredi 15 juillet 2015 20:07<br>
À : Delacroix, Gauthier <<a href="mailto:Gauthier.Delacroix@coreye.fr" target="_blank">Gauthier.Delacroix@coreye.fr</a>><br>
Cc : <a href="mailto:varnish-dev@varnish-cache.org" target="_blank">varnish-dev@varnish-cache.org</a>; <a href="mailto:ssm@redpill-linpro.com" target="_blank">ssm@redpill-linpro.com</a><br>
<span>Objet : Re: [PATCH] Variable for vcl_dir in startup scripts<br>
<br>
</span><span>Hi Gauthier,<br>
<br>
Apologies for the delay.<br>
<br>
In all fairness I copied this from the Debian systemd files [1] in an attempt to unify the service files.<br>
I don't see any particular reason for having this, specially if it's the source of issues.<br>
<br>
Stig, is there any reason for the ExecStartPre? The commit message says "debian/varnish.service: Test configuration before starting, add reload".<br>
I don't see any difference in the output for an invalid config with ExecStartPre when I run either `systemctl start' or `systemctl status'.<br>
<br>
Thanks.<br>
<br>
1. <a href="http://anonscm.debian.org/cgit/pkg-varnish/pkg-varnish.git/tree/debian" rel="noreferrer" target="_blank">http://anonscm.debian.org/cgit/pkg-varnish/pkg-varnish.git/tree/debian</a><br>
<br>
</span><span>On Thu, Jun 25, 2015 at 5:12 PM, Delacroix, Gauthier <<a href="mailto:Gauthier.Delacroix@coreye.fr" target="_blank">Gauthier.Delacroix@coreye.fr</a>> wrote:<br>
Federico,<br>
<br>
I've initially set vcl_dir in DAEMON_OPTS in varnish.params with 4.0.2, but it's not included in the syntax check added in 4.0.3, making my config unable to start without changing varnish.service.<br>
<br>
I'm not aware of the reason behind this syntax check (won't a syntax error make startup fail anyway ?) but I assumed you had a good reason not to include DAEMON_OPTS.<br>
<br>
As it's better to change varnish.params rather than varnish.service, my proposal was to add a way to include parameters needed to make the config work in the syntax check, without including the full DAEMON_OPTS.<br>
<br>
If you finally think DAEMON_OPTS can be included in the syntax check, then I can send a really smaller patch.<br>
<br>
Gauthier<br>
<br>
</span>De : Federico Schwindt [mailto:<a href="mailto:fgsch@lodoss.net" target="_blank">fgsch@lodoss.net</a>]<br>
<span>Envoyé : jeudi 25 juin 2015 17:53<br>
À : Delacroix, Gauthier<br>
</span>Cc : <a href="mailto:varnish-dev@varnish-cache.org" target="_blank">varnish-dev@varnish-cache.org</a><br>
<span>Objet : Re: [PATCH] Variable for vcl_dir in startup scripts<br>
<br>
Hi,<br>
<br>
My first inclination is to ensure that the ExecStartPre line uses the same parameters as ExecStart.<br>
<br>
So my questions is how do you set vcl_dir? Do you edit varnish.service or varnish.params?<br>
<br>
Wouldn't adding DAEMON_OPTS to ExecStartPre (and configtest) do it?<br>
<br>
</span><span>On Thu, Jun 25, 2015 at 3:26 PM, Delacroix, Gauthier <<a href="mailto:Gauthier.Delacroix@coreye.fr" target="_blank">Gauthier.Delacroix@coreye.fr</a>> wrote:<br>
Here is another patch proposal to make syntax check handle parameters required to compile the VCL (vcl_dir, etc.) without creating a startup variable for each parameter.<br>
<br>
It just adds a COMPILE_OPTS that is merged in DAEMON_OPTS to start Varnish but is used alone in the syntax check.<br>
<br>
Gauthier<br>
<br>
>From d1567a956d53a489aa4ace66ce0b1c1ef745570b Mon Sep 17 00:00:00 2001<br>
</span>From: Gauthier Delacroix <<a href="mailto:gauthier.delacroix@coreye.fr" target="_blank">gauthier.delacroix@coreye.fr</a>><br>
<div><div>Date: Thu, 25 Jun 2015 15:06:08 +0200<br>
Subject: [PATCH] Add COMPILE_OPTS in startup scripts to make syntax check<br>
check handle compilation parameters<br>
<br>
---<br>
redhat/varnish.initrc | 6 ++++--<br>
redhat/varnish.params | 8 ++++++++<br>
redhat/varnish.service | 3 ++-<br>
redhat/varnish.sysconfig | 11 ++++++++++-<br>
4 files changed, 24 insertions(+), 4 deletions(-)<br>
<br>
diff --git a/redhat/varnish.initrc b/redhat/varnish.initrc<br>
index 117e334..0bde074 100755<br>
--- a/redhat/varnish.initrc<br>
+++ b/redhat/varnish.initrc<br>
@@ -126,9 +126,11 @@ rh_status_q() {<br>
<br>
configtest() {<br>
if [ -f "$VARNISH_VCL_CONF" ]; then<br>
- $exec -f "$VARNISH_VCL_CONF" -C -n /tmp > /dev/null && echo "Syntax ok"<br>
+ $exec -f "$VARNISH_VCL_CONF" -C -n /tmp $COMPILE_OPTS > /dev/null \<br>
+ && echo "Syntax ok"<br>
else<br>
- echo "VARNISH_VCL_CONF is unset or does not point to a file"<br>
+ echo "VARNISH_VCL_CONF is unset or does not point to a file"<br>
+ echo "Also check that COMPILE_OPTS is set depending on the VCL config"<br>
fi<br>
}<br>
<br>
diff --git a/redhat/varnish.params b/redhat/varnish.params<br>
index 27a14dd..970d088 100644<br>
--- a/redhat/varnish.params<br>
+++ b/redhat/varnish.params<br>
@@ -31,5 +31,13 @@ VARNISH_TTL=120<br>
VARNISH_USER=varnish<br>
VARNISH_GROUP=varnish<br>
<br>
+# Startup options required to compile the configuration.<br>
+# The following run-time parameters must be defined here, if needed:<br>
+# cc_command, group_cc, vcc_allow_inline_c, vcc_err_unref, vcc_unsafe_path,<br>
+# vcl_dir, vmod_dir<br>
+# Defining them in DAEMON_OPTS may result in a syntax check failure.<br>
+# See the man page varnishd(1).<br>
+#COMPILE_OPTS="-p vcl_dir=/etc/varnish -p vcc_err_unref=on"<br>
+<br>
# Other options, see the man page varnishd(1)<br>
#DAEMON_OPTS="-p thread_pool_min=5 -p thread_pool_max=500 -p thread_pool_timeout=300"<br>
diff --git a/redhat/varnish.service b/redhat/varnish.service<br>
index a4f3355..a08db58 100644<br>
--- a/redhat/varnish.service<br>
+++ b/redhat/varnish.service<br>
@@ -27,7 +27,7 @@ EnvironmentFile=/etc/varnish/varnish.params<br>
Type=forking<br>
PIDFile=/var/run/varnish.pid<br>
PrivateTmp=true<br>
-ExecStartPre=/usr/sbin/varnishd -C -f $VARNISH_VCL_CONF<br>
+ExecStartPre=/usr/sbin/varnishd -C -f $VARNISH_VCL_CONF $COMPILE_OPTS<br>
ExecStart=/usr/sbin/varnishd \<br>
-P /var/run/varnish.pid \<br>
-f $VARNISH_VCL_CONF \<br>
@@ -38,6 +38,7 @@ ExecStart=/usr/sbin/varnishd \<br>
-g $VARNISH_GROUP \<br>
-S $VARNISH_SECRET_FILE \<br>
-s $VARNISH_STORAGE \<br>
+ $COMPILE_OPTS \<br>
$DAEMON_OPTS<br>
<br>
ExecReload=/usr/sbin/varnish_reload_vcl<br>
diff --git a/redhat/varnish.sysconfig b/redhat/varnish.sysconfig<br>
index 6aa2354..0e376ff 100644<br>
--- a/redhat/varnish.sysconfig<br>
+++ b/redhat/varnish.sysconfig<br>
@@ -91,6 +91,14 @@ VARNISH_STORAGE="malloc,${VARNISH_STORAGE_SIZE}"<br>
# # Default TTL used when the backend does not specify one<br>
VARNISH_TTL=120<br>
#<br>
+# Startup options required to compile the configuration.<br>
+# The following run-time parameters must be defined here, if needed:<br>
+# cc_command, group_cc, vcc_allow_inline_c, vcc_err_unref, vcc_unsafe_path,<br>
+# vcl_dir, vmod_dir<br>
+# Defining them in DAEMON_OPTS may result in a syntax check failure.<br>
+# See the man page varnishd(1).<br>
+#COMPILE_OPTS="-p vcl_dir=/etc/varnish -p vcc_err_unref=on"<br>
+#<br>
# # DAEMON_OPTS is used by the init script. If you add or remove options, make<br>
# # sure you update this section, too.<br>
DAEMON_OPTS="-a ${VARNISH_LISTEN_ADDRESS}:${VARNISH_LISTEN_PORT} \<br>
@@ -102,7 +110,8 @@ DAEMON_OPTS="-a ${VARNISH_LISTEN_ADDRESS}:${VARNISH_LISTEN_PORT} \<br>
-p thread_pool_timeout=${VARNISH_THREAD_TIMEOUT} \<br>
-u varnish -g varnish \<br>
-S ${VARNISH_SECRET_FILE} \<br>
- -s ${VARNISH_STORAGE}"<br>
+ -s ${VARNISH_STORAGE}" \<br>
+ ${COMPILE_OPTS}<br>
#<br>
<br>
<br>
--<br>
1.8.3.msysgit.0<br>
<br>
<br>
<br>
_______________________________________________<br>
varnish-dev mailing list<br>
</div></div><a href="mailto:varnish-dev@varnish-cache.org" target="_blank">varnish-dev@varnish-cache.org</a><br>
<a href="https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev" rel="noreferrer" target="_blank">https://www.varnish-cache.org/lists/mailman/listinfo/varnish-dev</a><br>
<br>
</blockquote></div><br></div>
</div></div></blockquote></div><br></div>