[PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

- The change of the apache initscript in CU181 made the restart comand a stop followed by a
   start. From bug13656 it looks like when the start is run the pid file may not yet be
   fully removed and so apache is not started, although the pid is being removed.
- This patch checks if the pid file is still present every second up to 10 seconds. Once the
   pid file is gone then the stop command is completed and the initscript moves to the start
   command.
- Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
   second if the file is still present. If at the end of 10 seconds it is still present
   then something went wrong with the pid removal.
- I have tested this patch on my vm system and it worked but it would be good to be
   reviewed to make sure that it is a reasonable approach that has been used and if required
   changed in whatever way makes the best sense.

Fixes: Bug13656
Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
---
 src/initscripts/system/apache | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
index 18eb86e2f..087e4084e 100644
--- a/src/initscripts/system/apache
+++ b/src/initscripts/system/apache
@@ -2,7 +2,7 @@
 ###############################################################################
 #                                                                             #
 # IPFire.org - A linux based firewall                                         #
-# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
+# Copyright (C) 2007-2024  IPFire Team  <info(a)ipfire.org>                     #
 #                                                                             #
 # This program is free software: you can redistribute it and/or modify        #
 # it under the terms of the GNU General Public License as published by        #
@@ -87,6 +87,16 @@ case "$1" in
 	stop)
 		boot_mesg "Stopping Apache daemon..."
 		/usr/sbin/apachectl -k stop
+		COUNTER=0
+		while [ -e /var/run/httpd.pid ]
+		do
+			sleep 1
+			(( COUNTER++ ))
+			if [ $COUNTER -eq 10 ]; then
+				boot_mesg "pid not removed after 10 seconds"
+				break
+			fi
+		done
 		evaluate_retval
 		;;
 
-- 
2.44.0

Re: [PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 2747 bytes --]

Hello Adolf,

I don’t think that we should need this loop. I believe that /usr/sbin/apachectl -k stop should only return once the service has properly been terminated. Is that an incorrect assumption?

-Michael

> On 24 Apr 2024, at 17:56, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> - The change of the apache initscript in CU181 made the restart comand a stop followed by a
>   start. From bug13656 it looks like when the start is run the pid file may not yet be
>   fully removed and so apache is not started, although the pid is being removed.
> - This patch checks if the pid file is still present every second up to 10 seconds. Once the
>   pid file is gone then the stop command is completed and the initscript moves to the start
>   command.
> - Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
>   second if the file is still present. If at the end of 10 seconds it is still present
>   then something went wrong with the pid removal.
> - I have tested this patch on my vm system and it worked but it would be good to be
>   reviewed to make sure that it is a reasonable approach that has been used and if required
>   changed in whatever way makes the best sense.
> 
> Fixes: Bug13656
> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
> ---
> src/initscripts/system/apache | 12 +++++++++++-
> 1 file changed, 11 insertions(+), 1 deletion(-)
> 
> diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
> index 18eb86e2f..087e4084e 100644
> --- a/src/initscripts/system/apache
> +++ b/src/initscripts/system/apache
> @@ -2,7 +2,7 @@
> ###############################################################################
> #                                                                             #
> # IPFire.org - A linux based firewall                                         #
> -# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
> +# Copyright (C) 2007-2024  IPFire Team  <info(a)ipfire.org>                     #
> #                                                                             #
> # This program is free software: you can redistribute it and/or modify        #
> # it under the terms of the GNU General Public License as published by        #
> @@ -87,6 +87,16 @@ case "$1" in
> stop)
> boot_mesg "Stopping Apache daemon..."
> /usr/sbin/apachectl -k stop
> + COUNTER=0
> + while [ -e /var/run/httpd.pid ]
> + do
> + sleep 1
> + (( COUNTER++ ))
> + if [ $COUNTER -eq 10 ]; then
> + boot_mesg "pid not removed after 10 seconds"
> + break
> + fi
> + done
> evaluate_retval
> ;;
> 
> -- 
> 2.44.0
> 

Re: [PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 4071 bytes --]

Hi Michael,

On 26/04/2024 16:45, Michael Tremer wrote:
> Hello Adolf,
> 
> I don’t think that we should need this loop. I believe that /usr/sbin/apachectl -k stop should only return once the service has properly been terminated. Is that an incorrect assumption?

Yes, I am afraid it is.

See my feedback in bug13657 from yesterday.

I ran some tests with the new Apache script and if you have a lot of IPFire capabilities enabled then I found that in one test after running the stop command and getting an OK the start command found a pid was already present so it didn't start Apache. However that pid was the old one (I recorded the pid before I ran the CU185 update), which was still hanging around when the start command was initiated. After not starting Apache because the old pid was still present that pid then disappeared as its removal finally occurred but then Apache was not running so the WUI froze.

In the second test Apache gave the message "Address already in use" when the start command was initiated and so again Apache was not started after the stop.

The only time I had the stop followed by start work was if I had an IPFire install with no other capabilities enabled such as Web Proxy, OpenVPN, IPS. Then the update from CU184 to CU185 went smoothly without any freezing of the WUI and the Stop command was completed when the Start command started but not when other IPFire capabilities were enabled.

Regards,

Adolf.
> 
> -Michael
> 
>> On 24 Apr 2024, at 17:56, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>
>> - The change of the apache initscript in CU181 made the restart comand a stop followed by a
>>    start. From bug13656 it looks like when the start is run the pid file may not yet be
>>    fully removed and so apache is not started, although the pid is being removed.
>> - This patch checks if the pid file is still present every second up to 10 seconds. Once the
>>    pid file is gone then the stop command is completed and the initscript moves to the start
>>    command.
>> - Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
>>    second if the file is still present. If at the end of 10 seconds it is still present
>>    then something went wrong with the pid removal.
>> - I have tested this patch on my vm system and it worked but it would be good to be
>>    reviewed to make sure that it is a reasonable approach that has been used and if required
>>    changed in whatever way makes the best sense.
>>
>> Fixes: Bug13656
>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>> ---
>> src/initscripts/system/apache | 12 +++++++++++-
>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>
>> diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
>> index 18eb86e2f..087e4084e 100644
>> --- a/src/initscripts/system/apache
>> +++ b/src/initscripts/system/apache
>> @@ -2,7 +2,7 @@
>> ###############################################################################
>> #                                                                             #
>> # IPFire.org - A linux based firewall                                         #
>> -# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
>> +# Copyright (C) 2007-2024  IPFire Team  <info(a)ipfire.org>                     #
>> #                                                                             #
>> # This program is free software: you can redistribute it and/or modify        #
>> # it under the terms of the GNU General Public License as published by        #
>> @@ -87,6 +87,16 @@ case "$1" in
>> stop)
>> boot_mesg "Stopping Apache daemon..."
>> /usr/sbin/apachectl -k stop
>> + COUNTER=0
>> + while [ -e /var/run/httpd.pid ]
>> + do
>> + sleep 1
>> + (( COUNTER++ ))
>> + if [ $COUNTER -eq 10 ]; then
>> + boot_mesg "pid not removed after 10 seconds"
>> + break
>> + fi
>> + done
>> evaluate_retval
>> ;;
>>
>> -- 
>> 2.44.0
>>
> 

Re: [PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Michael Tremer]

[-- Attachment #1: Type: text/plain, Size: 4691 bytes --]

Hello,

> On 26 Apr 2024, at 17:08, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
> 
> Hi Michael,
> 
> On 26/04/2024 16:45, Michael Tremer wrote:
>> Hello Adolf,
>> I don’t think that we should need this loop. I believe that /usr/sbin/apachectl -k stop should only return once the service has properly been terminated. Is that an incorrect assumption?
> 
> Yes, I am afraid it is.

Okay, looking at the source, you are correct. Apache only sends itself a SIGTERM and that is it.

> See my feedback in bug13657 from yesterday.
> 
> I ran some tests with the new Apache script and if you have a lot of IPFire capabilities enabled then I found that in one test after running the stop command and getting an OK the start command found a pid was already present so it didn't start Apache. However that pid was the old one (I recorded the pid before I ran the CU185 update), which was still hanging around when the start command was initiated. After not starting Apache because the old pid was still present that pid then disappeared as its removal finally occurred but then Apache was not running so the WUI froze.
> 
> In the second test Apache gave the message "Address already in use" when the start command was initiated and so again Apache was not started after the stop.
> 
> The only time I had the stop followed by start work was if I had an IPFire install with no other capabilities enabled such as Web Proxy, OpenVPN, IPS. Then the update from CU184 to CU185 went smoothly without any freezing of the WUI and the Stop command was completed when the Start command started but not when other IPFire capabilities were enabled.

I just sent this patch to the list: https://patchwork.ipfire.org/project/ipfire/patch/20240426152838.3768448-1-michael.tremer(a)ipfire.org/

This sets the PIDFILE variable so that killproc will wait for the main process to exit. This should do what you want to do without adding any custom code.

Can you please test and confirm?

Best,
-Michael

> 
> Regards,
> 
> Adolf.
>> -Michael
>>> On 24 Apr 2024, at 17:56, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>> 
>>> - The change of the apache initscript in CU181 made the restart comand a stop followed by a
>>>   start. From bug13656 it looks like when the start is run the pid file may not yet be
>>>   fully removed and so apache is not started, although the pid is being removed.
>>> - This patch checks if the pid file is still present every second up to 10 seconds. Once the
>>>   pid file is gone then the stop command is completed and the initscript moves to the start
>>>   command.
>>> - Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
>>>   second if the file is still present. If at the end of 10 seconds it is still present
>>>   then something went wrong with the pid removal.
>>> - I have tested this patch on my vm system and it worked but it would be good to be
>>>   reviewed to make sure that it is a reasonable approach that has been used and if required
>>>   changed in whatever way makes the best sense.
>>> 
>>> Fixes: Bug13656
>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>> ---
>>> src/initscripts/system/apache | 12 +++++++++++-
>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
>>> index 18eb86e2f..087e4084e 100644
>>> --- a/src/initscripts/system/apache
>>> +++ b/src/initscripts/system/apache
>>> @@ -2,7 +2,7 @@
>>> ###############################################################################
>>> #                                                                             #
>>> # IPFire.org - A linux based firewall                                         #
>>> -# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
>>> +# Copyright (C) 2007-2024  IPFire Team  <info(a)ipfire.org>                     #
>>> #                                                                             #
>>> # This program is free software: you can redistribute it and/or modify        #
>>> # it under the terms of the GNU General Public License as published by        #
>>> @@ -87,6 +87,16 @@ case "$1" in
>>> stop)
>>> boot_mesg "Stopping Apache daemon..."
>>> /usr/sbin/apachectl -k stop
>>> + COUNTER=0
>>> + while [ -e /var/run/httpd.pid ]
>>> + do
>>> + sleep 1
>>> + (( COUNTER++ ))
>>> + if [ $COUNTER -eq 10 ]; then
>>> + boot_mesg "pid not removed after 10 seconds"
>>> + break
>>> + fi
>>> + done
>>> evaluate_retval
>>> ;;
>>> 
>>> -- 
>>> 2.44.0
>>> 

Re: [PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 5450 bytes --]

Hi Michael,

On 26/04/2024 17:30, Michael Tremer wrote:
> Hello,
> 
>> On 26 Apr 2024, at 17:08, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>
>> Hi Michael,
>>
>> On 26/04/2024 16:45, Michael Tremer wrote:
>>> Hello Adolf,
>>> I don’t think that we should need this loop. I believe that /usr/sbin/apachectl -k stop should only return once the service has properly been terminated. Is that an incorrect assumption?
>>
>> Yes, I am afraid it is.
> 
> Okay, looking at the source, you are correct. Apache only sends itself a SIGTERM and that is it.
> 
>> See my feedback in bug13657 from yesterday.
>>
>> I ran some tests with the new Apache script and if you have a lot of IPFire capabilities enabled then I found that in one test after running the stop command and getting an OK the start command found a pid was already present so it didn't start Apache. However that pid was the old one (I recorded the pid before I ran the CU185 update), which was still hanging around when the start command was initiated. After not starting Apache because the old pid was still present that pid then disappeared as its removal finally occurred but then Apache was not running so the WUI froze.
>>
>> In the second test Apache gave the message "Address already in use" when the start command was initiated and so again Apache was not started after the stop.
>>
>> The only time I had the stop followed by start work was if I had an IPFire install with no other capabilities enabled such as Web Proxy, OpenVPN, IPS. Then the update from CU184 to CU185 went smoothly without any freezing of the WUI and the Stop command was completed when the Start command started but not when other IPFire capabilities were enabled.
> 
> I just sent this patch to the list: https://patchwork.ipfire.org/project/ipfire/patch/20240426152838.3768448-1-michael.tremer(a)ipfire.org/
> 
> This sets the PIDFILE variable so that killproc will wait for the main process to exit. This should do what you want to do without adding any custom code.
> 
> Can you please test and confirm?

I can't test that it fixes the problem with the update from CU184 to CU185. If I put the changes into the Apache initscript in CU184 it will get overwritten by the shipping of the Apache initscript as it currently stands.

The only test I could do is make sure it works by running the initscript with the patch changes added, but I expect it will work.

The only way I can test that effects I found yesterday no longer happen is either by merging the change into CU185 or by removing the shipping of the Apache initscript in CU185 until I have carried out the test.

Regards,

Adolf.

> 
> Best,
> -Michael
> 
>>
>> Regards,
>>
>> Adolf.
>>> -Michael
>>>> On 24 Apr 2024, at 17:56, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>>
>>>> - The change of the apache initscript in CU181 made the restart comand a stop followed by a
>>>>    start. From bug13656 it looks like when the start is run the pid file may not yet be
>>>>    fully removed and so apache is not started, although the pid is being removed.
>>>> - This patch checks if the pid file is still present every second up to 10 seconds. Once the
>>>>    pid file is gone then the stop command is completed and the initscript moves to the start
>>>>    command.
>>>> - Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
>>>>    second if the file is still present. If at the end of 10 seconds it is still present
>>>>    then something went wrong with the pid removal.
>>>> - I have tested this patch on my vm system and it worked but it would be good to be
>>>>    reviewed to make sure that it is a reasonable approach that has been used and if required
>>>>    changed in whatever way makes the best sense.
>>>>
>>>> Fixes: Bug13656
>>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>> ---
>>>> src/initscripts/system/apache | 12 +++++++++++-
>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
>>>> index 18eb86e2f..087e4084e 100644
>>>> --- a/src/initscripts/system/apache
>>>> +++ b/src/initscripts/system/apache
>>>> @@ -2,7 +2,7 @@
>>>> ###############################################################################
>>>> #                                                                             #
>>>> # IPFire.org - A linux based firewall                                         #
>>>> -# Copyright (C) 2007-2022  IPFire Team  <info(a)ipfire.org>                     #
>>>> +# Copyright (C) 2007-2024  IPFire Team  <info(a)ipfire.org>                     #
>>>> #                                                                             #
>>>> # This program is free software: you can redistribute it and/or modify        #
>>>> # it under the terms of the GNU General Public License as published by        #
>>>> @@ -87,6 +87,16 @@ case "$1" in
>>>> stop)
>>>> boot_mesg "Stopping Apache daemon..."
>>>> /usr/sbin/apachectl -k stop
>>>> + COUNTER=0
>>>> + while [ -e /var/run/httpd.pid ]
>>>> + do
>>>> + sleep 1
>>>> + (( COUNTER++ ))
>>>> + if [ $COUNTER -eq 10 ]; then
>>>> + boot_mesg "pid not removed after 10 seconds"
>>>> + break
>>>> + fi
>>>> + done
>>>> evaluate_retval
>>>> ;;
>>>>
>>>> -- 
>>>> 2.44.0
>>>>
> 

Re: [PATCH] apache: Fixes bug13656 - Add delay between stop & start of restart command.

[Adolf Belka]

[-- Attachment #1: Type: text/plain, Size: 5782 bytes --]

Hi Michael,

On 26/04/2024 19:08, Adolf Belka wrote:
> Hi Michael,
>
> On 26/04/2024 17:30, Michael Tremer wrote:
>> Hello,
>>
>>> On 26 Apr 2024, at 17:08, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>
>>> Hi Michael,
>>>
>>> On 26/04/2024 16:45, Michael Tremer wrote:
>>>> Hello Adolf,
>>>> I don’t think that we should need this loop. I believe that /usr/sbin/apachectl -k stop should only return once the service has properly been terminated. Is that an incorrect assumption?
>>>
>>> Yes, I am afraid it is.
>>
>> Okay, looking at the source, you are correct. Apache only sends itself a SIGTERM and that is it.
>>
>>> See my feedback in bug13657 from yesterday.
>>>
>>> I ran some tests with the new Apache script and if you have a lot of IPFire capabilities enabled then I found that in one test after running the stop command and getting an OK the start command found a pid was already present so it didn't start Apache. However that pid was the old one (I recorded the pid before I ran the CU185 update), which was still hanging around when the start command was initiated. After not starting Apache because the old pid was still present that pid then disappeared as its removal finally occurred but then Apache was not running so the WUI froze.
>>>
>>> In the second test Apache gave the message "Address already in use" when the start command was initiated and so again Apache was not started after the stop.
>>>
>>> The only time I had the stop followed by start work was if I had an IPFire install with no other capabilities enabled such as Web Proxy, OpenVPN, IPS. Then the update from CU184 to CU185 went smoothly without any freezing of the WUI and the Stop command was completed when the Start command started but not when other IPFire capabilities were enabled.
>>
>> I just sent this patch to the list: https://patchwork.ipfire.org/project/ipfire/patch/20240426152838.3768448-1-michael.tremer(a)ipfire.org/
>>
>> This sets the PIDFILE variable so that killproc will wait for the main process to exit. This should do what you want to do without adding any custom code.
>>
>> Can you please test and confirm?
>
I saw that the change has been put into CU186 (next).

I did an update from CU185 to CU186 in unstable and it went without any problems so it looks like the patch change is working fine.

Regards,

Adolf.

> I can't test that it fixes the problem with the update from CU184 to CU185. If I put the changes into the Apache initscript in CU184 it will get overwritten by the shipping of the Apache initscript as it currently stands.
>
> The only test I could do is make sure it works by running the initscript with the patch changes added, but I expect it will work.
>
> The only way I can test that effects I found yesterday no longer happen is either by merging the change into CU185 or by removing the shipping of the Apache initscript in CU185 until I have carried out the test.
>
> Regards,
>
> Adolf.
>
>>
>> Best,
>> -Michael
>>
>>>
>>> Regards,
>>>
>>> Adolf.
>>>> -Michael
>>>>> On 24 Apr 2024, at 17:56, Adolf Belka <adolf.belka(a)ipfire.org> wrote:
>>>>>
>>>>> - The change of the apache initscript in CU181 made the restart comand a stop followed by a
>>>>>    start. From bug13656 it looks like when the start is run the pid file may not yet be
>>>>>    fully removed and so apache is not started, although the pid is being removed.
>>>>> - This patch checks if the pid file is still present every second up to 10 seconds. Once the
>>>>>    pid file is gone then the stop command is completed and the initscript moves to the start
>>>>>    command.
>>>>> - Rather than apply a fixed delay of 2 or 3 or 4 seconds I used a while loop to check every
>>>>>    second if the file is still present. If at the end of 10 seconds it is still present
>>>>>    then something went wrong with the pid removal.
>>>>> - I have tested this patch on my vm system and it worked but it would be good to be
>>>>>    reviewed to make sure that it is a reasonable approach that has been used and if required
>>>>>    changed in whatever way makes the best sense.
>>>>>
>>>>> Fixes: Bug13656
>>>>> Tested-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>>> Signed-off-by: Adolf Belka <adolf.belka(a)ipfire.org>
>>>>> ---
>>>>> src/initscripts/system/apache | 12 +++++++++++-
>>>>> 1 file changed, 11 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/src/initscripts/system/apache b/src/initscripts/system/apache
>>>>> index 18eb86e2f..087e4084e 100644
>>>>> --- a/src/initscripts/system/apache
>>>>> +++ b/src/initscripts/system/apache
>>>>> @@ -2,7 +2,7 @@
>>>>> ###############################################################################
>>>>> # #
>>>>> # IPFire.org - A linux based firewall                                         #
>>>>> -# Copyright (C) 2007-2022  IPFire Team <info(a)ipfire.org>                     #
>>>>> +# Copyright (C) 2007-2024  IPFire Team <info(a)ipfire.org>                     #
>>>>> # #
>>>>> # This program is free software: you can redistribute it and/or modify        #
>>>>> # it under the terms of the GNU General Public License as published by        #
>>>>> @@ -87,6 +87,16 @@ case "$1" in
>>>>> stop)
>>>>> boot_mesg "Stopping Apache daemon..."
>>>>> /usr/sbin/apachectl -k stop
>>>>> + COUNTER=0
>>>>> + while [ -e /var/run/httpd.pid ]
>>>>> + do
>>>>> + sleep 1
>>>>> + (( COUNTER++ ))
>>>>> + if [ $COUNTER -eq 10 ]; then
>>>>> + boot_mesg "pid not removed after 10 seconds"
>>>>> + break
>>>>> + fi
>>>>> + done
>>>>> evaluate_retval
>>>>> ;;
>>>>>
>>>>> -- 
>>>>> 2.44.0
>>>>>
>>