-
-
Notifications
You must be signed in to change notification settings - Fork 197
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
Fix for pihole-FTL test
#1067
Fix for pihole-FTL test
#1067
Conversation
… up at process termination anyway. Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…hared memory Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
…g to shared memory locks. Other forks may want to log as well. Signed-off-by: DL6ER <dl6er@dl6er.de>
Confirmed working by original reporter in #1063 |
Any changes needed or suggested for the Docker image? |
Not needed but we should get rid of the @PromoFaux already added
but I am not in the position to make a PR as I have never used the Pi-hole docker container and wouldn't know if getting rid of them without replacement is the right thing to do. I'm however sure that |
Yes, I think we can at least step down to Instead, in
# Perhaps wrap it to be quiet unless it fails?
pihole-FTL -f test &> /var/logs/pihole-FTL.test.log
RES=$?
if [[ $RET != 0 ]]; then
cat /var/logs/pihole-FTL.test.log
fi One thing I would suggest looking into though is this: This makes sense, because the service will to endlessly try to restart in case it fails to start initially. It's a great feature to have to make it low maintenance and self-healing, even if it isn't the most elegant in implementation details. Because in a sense, we're doing a very similar cleanup as we'd want FTL to do on a clean shutdown, perhaps this is redundant code and we could use a |
This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there: https://discourse.pi-hole.net/t/pi-hole-ftl-v5-8-web-v5-5-and-core-v5-3-released/46237/1 |
Since piholeFTL test properly spins down it's no longer necessary to kill it. He's dead Jim Merge pi-hole#300, added `piholeFTL test` to the startup sequence to replace dnsmasq as a dependency for validate_env and gravity.sh. kill -9 was kept as a work around to a standing issue that `piholeFTL test` didn't spin down on it's own. This was fixed in pi-hole/FTL#1067, landed on Apr 14 2021 and confirmed working, as evinced by pi-hole#834 which was filed the same day it that fix landed. Signed-off-by: D.Rect <48034372+DistractionRectangle@users.noreply.github.com>
By submitting this pull request, I confirm the following:
How familiar are you with the codebase?:
10
Fix a few issues revealed when checking why
pihole-FTL test
doesn't work as expected:test
option is currently broken: It immediately terminates all FTL components (threads surrounding the resolver, etc.), however, it did not terminate the resolver itself leading to an attempt tokill
the resolver in the docker container.dnsmasq
config errors, etc.)This fixes #1063 and #1064