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