Skip to content
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

Skip full distro check on start #771

Merged
merged 1 commit into from
Jan 27, 2021
Merged

Skip full distro check on start #771

merged 1 commit into from
Jan 27, 2021

Conversation

MichaIng
Copy link
Contributor

@MichaIng MichaIng commented Jan 24, 2021

Description

The distro_check function includes updating the APT cache, checking for dependencies, which is both not required on Docker start where all required packages are installed already. The only required steps from this function is the webserver user and config file names, which can be applied directly instead since we know that the Pi-hole Docker container is based on Debian.

Furthermore this solves the issue that updating the APT cache fails, when Pi-hole itself is used for DNS resolution, since pihole-FTL has not yet been started at this stage. That failure was not visible since "apt-get update" does not exist with error code (currently) when facing DNS resolving issues, even if not a single list could have been updated, and no other step is done that would require DNS resolving, until pihole-FTL is started. For a regular (non-Docker) install or update it is however reasonable to error out directly when the APT cache could not have been updated, to not defer the exit unnecessarily to a harder-to-debug stage.

Motivation and Context

Related issue: pi-hole/pi-hole#3537
This PR included a change of the dependency package checks, so that this check required a successful "apt-get update" to check if an APT package is actually available in the installed repository, ready to be installed. The previously used method, which is now used again since the PR has been reverted, doesn't always require the APT cache but succeeds as well if the checked package is already installed, and even if another installed package has it listed as (optional) dependency or conflict or in any other way, which means that the old-new method fails the actual aim of the check in two ways. Since with the current list of supported distribution versions, these package checks are not required anymore, it is however not a major motivation for this pull request, but more the fact that most of what distro_check does either fails (silently) or takes a longer time (apt-get update) while it is not required.

How Has This Been Tested?

2021-01-24 18:04:53 root@VM-Buster:~/pihole# docker start -i pihole
[s6-init] making user provided files available at /var/run/s6/etc...exited 0.
[s6-init] ensuring user provided files have correct perms...exited 0.
[fix-attrs.d] applying ownership & permissions fixes...
[fix-attrs.d] 01-resolver-resolv: applying...
[fix-attrs.d] 01-resolver-resolv: exited 0.
[fix-attrs.d] done.
[cont-init.d] executing container initialization scripts...
[cont-init.d] 20-start.sh: executing...
 ::: Starting docker specific checks & setup for docker pihole/pihole
Assigning random password: fwALqd-Q

  [i] Installing configs from /etc/.pihole...
  [i] Existing dnsmasq.conf found... it is not a Pi-hole file, leaving alone!
  [✓] Copying 01-pihole.conf to /etc/dnsmasq.d/01-pihole.conf
chown: cannot access '': No such file or directory
chmod: cannot access '': No such file or directory
chown: cannot access '/etc/pihole/dhcp.leases': No such file or directory
Existing DNS servers detected in setupVars.conf. Leaving them alone
::: Pre existing WEBPASSWORD found
DNSMasq binding to default interface: eth0
Added ENV to php:
                        "PHP_ERROR_LOG" => "/var/log/lighttpd/error.log",
                        "ServerIP" => "127.0.0.1",
                        "VIRTUAL_HOST" => "pi.hole",
Using IPv4 and IPv6
::: Preexisting ad list /etc/pihole/adlists.list detected ((exiting setup_blocklists early))
https://raw.githubusercontent.com/StevenBlack/hosts/master/hosts
::: Testing pihole-FTL DNS: FTL started!
::: Testing lighttpd config: Syntax OK
::: All config checks passed, cleared for startup ...
::: Enabling Query Logging
  [i] Enabling logging...
  [✓] Logging has been enabled!
 ::: Docker start setup complete
  [i] Neutrino emissions detected...
  [✓] Pulling blocklist source list into range

  [✓] Preparing new gravity database
  [i] Using libz compression

  [i] Target: https://raw.githubusercontent.com/StevenBlack/hosts/master/hosts
  [✓] Status: Retrieval successful
  [i] Received 59537 domains

  [✓] Storing downloaded domains in new gravity database
  [✓] Building tree
  [✓] Swapping databases
  [i] Number of gravity domains: 59537 (59537 unique domains)
  [i] Number of exact blacklisted domains: 0
  [i] Number of regex blacklist filters: 0
  [i] Number of exact whitelisted domains: 0
  [i] Number of regex whitelist filters: 0
  [✓] Cleaning up stray matter

  [✓] DNS service is listening
     [✓] UDP (IPv4)
     [✓] TCP (IPv4)
     [✓] UDP (IPv6)
     [✓] TCP (IPv6)

  [✓] Pi-hole blocking is enabled
  Pi-hole version is v5.2.4 (Latest: v5.2.4)
  AdminLTE version is v5.3.1 (Latest: v5.3.1)
  FTL version is v5.5.1 (Latest: v5.5.1)
[cont-init.d] 20-start.sh: exited 0.
[cont-init.d] done.
[services.d] starting services
Starting lighttpd
Starting pihole-FTL (no-daemon) as root
Starting crond
[services.d] done.
Types of changes
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

The distro_check function includes updating the APT cache, checking for dependencies, which is both not required on Docker start where all required packages are installed already. The only required steps from this function is the webserver user and config file names, which can be applied directly instead since we know that the Pi-hole Docker container is based on Debian.

Furthermore this solves the issue that updating the APT cache fails, when Pi-hole itself is used for DNS resolution, since pihole-FTL has not yet been started at this stage. That failure was not visible since "apt-get update" does not exist with error code (currently) when facing DNS resolving issues, even if not a single list could have been updated, and no other step is done that would require DNS resolving, until pihole-FTL is started. For a regular (non-Docker) install or update it is however reasonable to error out directly when the APT cache could not have been updated, to not defer the exit unnecessarily to a harder-to-debug stage.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng marked this pull request as ready for review January 24, 2021 17:13
@PromoFaux
Copy link
Member

:) You beat me to it. Thanks, I'll take a look over this in the week

@MichaIng
Copy link
Contributor Author

For completeness, if someone stumbles over the three chmod/chown errors in the log, those are not related to this PR and dealt with here instead: #676

Copy link
Member

@PromoFaux PromoFaux left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hard to argue with this change. Certainly speeds up the container start up.

Tested - and everything appears to be working as normal.

@PromoFaux PromoFaux merged commit 5735495 into pi-hole:dev Jan 27, 2021
@MichaIng MichaIng deleted the patch-2 branch January 27, 2021 21:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants