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

systemctl installed default on Debian regardless of init system #1610

Closed
wants to merge 3 commits into from

Conversation

@deHakkelaar
Copy link

deHakkelaar commented Jul 14, 2017

Before change on a systemd Raspi:

pi@noads:~ $ stat /sbin/init
  File: ‘/sbin/init’ -> ‘/lib/systemd/systemd’
pi@noads:~ $ pihole restartdns
pi@noads:~ $

Before change on an upstart Raspi:

xbian@monmc ~ $ /sbin/init --version
init (upstart 1.13.2)
xbian@monmc ~ $ pihole restartdns
Failed to restart dnsmasq.service: No such method 'RestartUnit'
See system logs and 'systemctl status dnsmasq.service' for details.
xbian@monmc ~ $

After change on a systemd Raspi:

pi@noads:~ $ pihole restartdns
pi@noads:~ $

After change on an upstart Raspi:

xbian@monmc ~ $ pihole restartdns
[ ok ] Restarting DNS forwarder and DHCP server: dnsmasq.
xbian@monmc ~ $

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

  • I have read and understood the contributors guide.
  • I have checked that another pull request for this purpose does not exist.
  • I have considered, and confirmed that this submission will be valuable to others.
  • I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  • I give this submission freely, and claim no ownership to its content.

How familiar are you with the codebase?:

6


systemctl installed default on Debian regardless of init system

This template was created based on the work of udemy-dl.

PromoFaux and others added 2 commits Jun 20, 2017
* Fix handling of wildcard help text

* Rewrite help text for better handling of params

* Replace misleading letter variable

* stash changes on branch switch, else it fails if any changes have been made.

* Make changes according to comment in #1384

* Update queryFunc()

* Allow scanList() to search files using a wildcard by removing quotes wrapped around `${list}`
* scanList() will not provide a domain ouput on each string if exact is specified (`grep -l`)
* Remove unused processWildcards() function
* Return a message if no domain is specified
* IDN domains are converted to punycode when running a `pihole -q` search if the `python` package is available, otherwise will revert to current behaviour
* Scan Blacklist & Wildcards first, exiting from search if a match is found (Fixes #1330)
* Use one `grep` subshell to search for all "*.domains" lists at once (opposed to looping to get every matching file name, and then spawning a `grep` instance for every matching file)
* queryFunc() will not return "(0 results)" output from files where no match is found
* Sort results based off list number
* Return a message if no results are found

* Update basic-install.sh

* Update block page. Allow for setupVars setting of CUSTOMBLOCKPAGE (bool) to prevent it being overwritten

* simplify

* further simplify

* fix inteliJ IDEA complaints

* even further simplify

* tidy up output

* revert line, looks tidyer

* clarify

* Revert "Ensure any changes to blocking page are updated."

* We test for dpkg lock on line 830 directly, no need for the check also
in the template section.

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>

* Display FTL version & version.sh rewrite

While testing to make sure `pihole -v` would output `pihole-FTL version`, I noticed some options didn't work how I expected them to. For example, if I use `pihole -v -p`, I would expect to see the version output of Pi-hole Core. Instead, I'm informed that it's an invalid option.

I've had the following things in mind while rewriting this:
  * I'm operating under the assumption that FTL is only installed if the Admin Console is (Line 113 exit 0)
  * I have modified the help text to only output with `pihole -v --help`
  * I have modified all output to be more similar to the output style of `grep` and `curl` (Ditching ":::")

Testing output:
```
w3k@MCT:~$ pihole -v
  Pi-hole version is v3.0.1-14-ga928cd3 (Latest: v3.0.1)
  Admin Console version is v3.0-9-g3760482 (Latest: v3.0.1)
  FTL version is v2.6.2 (Latest: v2.6.2)
w3k@MCT:~$ pihole -v -c
  Current Pi-hole version is v3.0.1-14-ga928cd3
  Current Admin Console version is v3.0-9-g3760482
  Current FTL version is v2.6.2
w3k@MCT:~$ pihole -v -l
  Latest Pi-hole version is v3.0.1
  Latest Admin Console version is v3.0.1
  Latest FTL version is v2.6.2
w3k@MCT:~$ pihole -v -p --hash
  Current Pi-hole hash is a928cd3
w3k@MCT:~$ pihole -v -a --hash
  Current Admin Console hash is 3760482
w3k@MCT:~$ pihole -v --help
Usage: pihole -v [REPO | OPTION] [OPTION]
Show Pi-hole, Web Admin & FTL versions
  <Shows all Repositories and Options>
w3k@MCT:~$ pihole -v -foo
  Invalid Option!
```

* Update -h to work as --hash

Also provide error output as per #1447 (comment)

* Perform EXACT searches on HOSTS lists correctly

`\s` on the end may be overkill, but it is the existing scanList() behaviour.

* Fixed indentation

* Minimise string duplication & other minor changes

Instead of duplicating output strings, rewrite core/web/ftlOutput() into one neat versionOutput().

* Modified syntax to be valid for Shellcheck

* Log and echo gateway responses

* Update queryFunc() to search Whitelist

If there is a match in Whitelist/Blacklist/Wildcards, `[ ! -t 1 ]` will cause the search to end if the terminal is closed when the script is called. This has the intended effect of allowing a user to search for a W/B/W domain (as well as all the adlists it's found in) using `pihole -q` via Terminal, but the script will stop searching after a W/B/W match when called by the block page.

* Wrap in double brackets

* Provide remote hashes for version.sh

 * Provide remote hashes for comparison
 * Use double braces for all conditions (for consistency)
 * Suppress potential "cd" error output
 * Provide "not applicable" output upon any hash request for FTL

* whitelist on website blocked doesnt work (#1452)

Since Pi-hole redirects ad domains to itself, accessing the script via de.ign.com is the same as pi.hole in this case. The fix should be as simple as adding a / before admin on this line.

* Solve piholeLogFlush.sh having to be issued 2 x to clear logs (#1460)

Simplified the command -v syntax, and added a sleep 3 timer to the first execution of the log rotation. The second execution was being issued while the first was still running, thus it would fail and you would have to issue the "Flush Logs" command a second time.

* Use `echo "ABC" | pihole tricorder` to upload to Pi-hole's medical tricorder. Uses SSL if available.

* Update list.sh

I believe this has feature parity with `sed /foo/ Id` but also supports busybox, and my alpine docker ;)

* Document `sed` substitution for user readability

Comment the oneliner with explanations of what each step does.

* Update Help Output (#1467)



* File consistency

* Tabs to 2 spaces
* Corrected indenting
* Double braced conditionals
* Quoted variables within conditionals

* Standardise core help text

* Added help text for disable command
* Added help text for logging command

* Clean up

* Fixed certain new lines and spaces

* Sync with development branch

* Formatting consistency

* Tabs to 2 spaces
* Corrected indenting
* Double braced conditionals
* Quoted variables within conditionals
* Fixed certain newlines and spaces

* Admin help text

* Added help text for interface command

* Sync with development branch

* Formatting consistency

* Tabs to 2 spaces
* Fixed some wording
* Fixed certain spaces

* Formatting consistency

* Minor wording changes
* Tabs to 2 spaces
* Corrected indenting
* Double braced conditionals
* Quoted variables within conditionals
* Fixed certain newlines and spaces

* Blacklist help text

* Formatting consistency

* Tabs to 2 spaces
* Corrected indenting

* Cronometer help text

* Formatting consistency

* Fixed certain newlines and spaces
* Corrected indenting

* Checkout warning alteration

* Add checkout help text

* Corrected help output

* Show help for "pihole -a -i --help"

* Fix "pihole disable --help" and "pihole -l --help"

* Show help for "pihole -v -h" 

* Indent output text
* Minor help text change

* Show help for "pihole checkout --help"

* Tricorder: Insecure Opt-out

* Check to see if Tricorder is being called directly
* Provide opt-out for insecure transmission of debug log
* Remove mention of internal function from help menu

* 🌮 is the new :shipit: squirrel

* Wording changes and bug fix

* Fix wildcard help text

* -wild is not a valid option since we're already using -wild

* Fix logrotation: manual flushing should be done twice, but automated rotation at midnight should only be done *once*!

* Print echos only when manual flushing is requested

* Add "quiet" mode + update comments in the cron file

* Confirm Tricorder is online

* Scan port 9998 to confirm the availability of "tricorder.pi-hole.net"
* Exit codes for upload process

* Formatting consistency

* Add link to Windows DNS Swapper

See #1400

* Install loopback firewall rules for FTL (#1419)

* Install loopback firewall rules for FTL

* FirewallD FTL ports

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>

* Remove firewallD FTL local rules.

Local rules should not be blocked in firewallD, not requred for internal service FTD>

* Reinstate https rules, and delete FTL rules

Fixes earlier commit.

* Retrieve local repos on repair (#1481)

* Retrieve local repos on repair

* Change conditional to check for repair
* Change wording of Update/Reconfigure message
* Fixed indenting

* Perform "git reset --hard" on reconfigure

* Change directory before trying to reset repository. Fixes #1489

* No need to `cd $PWD` as it doesn't affect flow of caller script.

Signed-off-by: Dan Schaper <dan.schaper@pi-hole.net>

* Refine output of password status in basic-install.sh:displayFinalMessage(). Fixes #1488 (#1490)

* Rewrite Chronometer to output more stats

* Fix output IPv4 addr when removing CIDR notation (#1498)

* Move wildcards file if blocking is disabled (#1495)

* Move wildcards file if blocking is diabled

* Delete newline

* Roll back merge #1417 (#1494)

* Update ISSUE_TEMPLATE.md

* Remove Question option

* Prefer ULA over GUA addresses [IPv6] (#1508)

* On installs with GUA and ULA's we should prefer ULA's as it's been demonstrated that GUA's can and often are rotated by ISPs. Fixes #1473

* Add test for link-local address detection

* Add ULA-only and GUA-only tests

* Add test_IPv6_GUA_ULA_test and test_IPv6_ULA_GUA_test

* Add ""

* Add mock_command_2 command that can mock a command with more than one argument (as "ip -6 address") and result multiple lines of results

* Make mock_command_2 more similar to the original mock_command

* Correct comments

* Fixed remaining comments

* Fixed one last comment...

* Fixed a comment...

* Add weekly logrotation of FTL's log (#1509)

* Update LICENSE of the project to EUPL v1.2

* Make clear that NO is the default if the user just hits return (#1514)

* Add tricorderFunc back as usable function (#1515)

As per #1464

* Don't update FTL when there is a core update (as this will update FTL a second time). Fixes #1516

* Add FTL tests to the test suite (#1510)

* Add first version of FTL tests

* Wait one second to allow FTL to start up and analyze our mock log

* Add test_FTL_telnet_statistics

* Added test_FTL_telnet_top_clients

* Add test_FTL_telnet_top_domains

* Revert "Add FTL tests to the test suite (#1510)" (#1519)

This reverts commit cf6a1ac.

* Trim version output when update is successful (#1527)

* Change ownership of /etc/pihole to user/group pihole. Fixes #1529 (#1530)

* Delete temporary files after installing the FTL binary. Fixes #1525

* Change from admin to approvers teams

* Introduce new file black.list for blacklist content

* Add "pihole -g -b" to *only* update black.list (saves a bunch of time when adding/changing only blacklisted files - won'tdownload lal lists, but only processes the blacklist and restars dnsmasq)

* Remove useless cat

* Improve displayed messages and overall logic

* Disable black.list on "pihole disable"

* cp + rm === mv (well, almost)
Before change on a systemd Raspi:

pi@noads:~ $ stat /sbin/init
  File: ‘/sbin/init’ -> ‘/lib/systemd/systemd’

pi@noads:~ $ pihole restartdns
pi@noads:~ $

Before change on an upstart Raspi:

xbian@monmc ~ $ /sbin/init --version
init (upstart 1.13.2)

xbian@monmc ~ $ pihole restartdns
Failed to restart dnsmasq.service: No such method 'RestartUnit'
See system logs and 'systemctl status dnsmasq.service' for details.
xbian@monmc ~ $

After change on a systemd Raspi:

pi@noads:~ $ pihole restartdns
pi@noads:~ $

After change on an upstart Raspi:

xbian@monmc ~ $ pihole restartdns
[ ok ] Restarting DNS forwarder and DHCP server: dnsmasq.
xbian@monmc ~ $
@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 14, 2017

This should solve the XBian issue, correct?

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 14, 2017

Correct.
XBian uses upstart for init so systemctl does not apply.

@mkreisl

This comment has been minimized.

Copy link

mkreisl commented Jul 14, 2017

This should solve the XBian issue, correct?

Not only XBian issue, required if init system is not systemd

@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 14, 2017

We had a check in the code for non-systemd based installs, it just got modified to check for the systemd binaries as a check, instead of checking to see what the actual init system was that was in play. We run in to similar problems with Synology chroot/jailed installs where the jailed systemd isn't the true init system.

Had to suppress output on upstart as text got mangled:

xbian@monmc ~ $ pihole -g
::: Cleaning up un-needed files... done!
:::
[ ok efresh lists in dnsmasq...[....] Restarting DNS forwarder and DHCP server: dnsmasq.
 done!
::: DNS service is running
::: Pi-hole blocking is Enabled
@dschaper dschaper changed the base branch from master to development Jul 16, 2017
@dschaper

This comment has been minimized.

Copy link
Member

dschaper commented Jul 16, 2017

Can you rebase your pull off of the Development branch, we don't allow PR's against master.

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 19, 2017

The dev branch is altered already in such a way that I am not sure if I can apply the original patch on this dev branch.
The dev branch depends on systemctl (systemd) not showing any output if a dnsmasq restart is executed successfully.
With upstart, there is always an output returned, not only on error, as can be seen from below.

Systemd without and with an error introduced:

pi@noads:~ $ pihole restartdns
pi@noads:~ $
pi@noads:~ $ pihole restartdns
Job for dnsmasq.service failed. See 'systemctl status dnsmasq.service' and 'journalctl -xn' for details.

Upstart without and with an error introduced:

xbian@monmc ~ $ pihole restartdns
[ ok ] Restarting DNS forwarder and DHCP server: dnsmasq.
xbian@monmc ~ $
xbian@monmc ~ $ pihole restartdns
[FAIL] Restarting DNS forwarder and DHCP server: configuration syntax check failed!
xbian@monmc ~ $

Below bit, that was added to the dev branch, in the restartDNS function will always be FALSE with an upstart or systemv systems:

    if [[ -z "${output}" ]]; then
      echo -e "${OVER}  ${TICK} ${str}"
    else
      echo -e "${OVER}  ${CROSS} ${output}"
    fi

Not sure what to do except install the dev branch on two Pi's for testing.

@WaLLy3K

This comment has been minimized.

Copy link
Contributor

WaLLy3K commented Jul 19, 2017

So the issue here is that systemctl is useless, and we want to get rid of it? The following code will read the $? exit code of service, and only print the output when the exit code is not 0:

(I also took a moment to clean up the restart service code)

restartDNS() {
  local svcOption str output status

  # Get PID of dnsmasq to determine if it needs to start or restart
  if pidof dnsmasq &> /dev/null; then
    svcOption="restart"
  else
    svcOption="start"
  fi

  str="${svcOption^}ing DNS service"
  echo -ne "  ${INFO} ${str}..."

  output=$( { service dnsmasq "${svcOption}"; } 2>&1 )
  status="$?"

  if [[ "${status}" -eq 0 ]]; then
    echo -e "${OVER}  ${TICK} ${str}"
  else
    echo -e "${OVER}  ${CROSS} ${output}"
  fi
}

Would this achieve what you're looking for, or is there something else I'm missing? 😄

WaLLy3K referenced this pull request Jul 19, 2017
* Prevent Web Admin from printing unnecessary msgs

* Make DNS restart behaviour consistent
@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 19, 2017

Looking way better.
I dropped that code snippet on both of my stable Pi-hole systems (one is upstart and other is systemd).

Systemd:

pi@noads:~ $ pihole restartdns
   Restarting DNS service...   Restarting DNS service

Systemd with a forced error:

pi@noads:~ $ pihole restartdns
   Restarting DNS service...   Job for dnsmasq.service failed. See 'systemctl status dnsmasq.service' and 'journalctl -xn' for details.

Upstart:

xbian@monmc ~ $ pihole restartdns
   Restarting DNS service...   Restarting DNS service
xbian@monmc ~ $

Upstart whith a forced error:

xbian@monmc ~ $ pihole restartdns
   Restarting DNS service...   Restarting DNS forwarder and DHCP server: configuration syntax check failed!
xbian@monmc ~ $

But with updating the lists, I still have below bug for upstart when I force an error.

Systemd:

pi@noads:~ $ pihole -g
::: Refresh lists in dnsmasq...   Starting DNS service...   Starting DNS service
 done!
::: DNS service is running
::: Pi-hole blocking is Enabled
pi@noads:~ $

Systemd with a forced error:

pi@noads:~ $ pihole -g
::: Refresh lists in dnsmasq...   Restarting DNS service...   Job for dnsmasq.service failed. See 'systemctl status dnsmasq.service' and 'journalctl -xn' for details.
 done!
::: DNS service is NOT running
pi@noads:~ $

Upstart:

xbian@monmc ~ $ pihole -g
::: Refresh lists in dnsmasq...   Restarting DNS service...   Restarting DNS service
 done!
::: DNS service is running
::: Pi-hole blocking is Enabled

Upstart with a forced error:

xbian@monmc ~ $ pihole -g
::: Refresh lists in dnsmasq...   Restarting DNS service...   Restarting DNS forwarder and DHCP server: configuration syntax check failed!
 done!
::: DNS service is running
::: Pi-hole blocking is Enabled

I'll have a look if I can figure this out.

@WaLLy3K

This comment has been minimized.

Copy link
Contributor

WaLLy3K commented Jul 19, 2017

pi@noads:~ $ pihole restartdns
Restarting DNS service... Restarting DNS service

Does your system show a new line between ... Restarting that failed to paste across to GitHub?

Sorry; I assume it's "overwriting" the text as it's supposed to?

It makes the output really hard to follow when pasted 😄

Also, ::: would indicate you're running on Master. To test this, you're best off to drop the code above into development.

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 19, 2017

No newline is displayed.
The output on upstart is exactly the same as on systemd (if no errors).
Ps. do mind, I only copied the snippet to my Pi's that still run stable release.

@WaLLy3K

This comment has been minimized.

Copy link
Contributor

WaLLy3K commented Jul 19, 2017

If you could provide a screenshot of the issue you're facing, I should be able to better understand what's happening!

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 19, 2017

The issue with the sippet is that on upstart, pihole believes dnsmasq is still running when I force a dnsmasq error (syntax check failed):

xbian@monmc ~ $ pihole -g
::: Refresh lists in dnsmasq...   Restarting DNS service...   Restarting DNS forwarder and DHCP server: configuration syntax check failed!
 done!
::: DNS service is running
::: Pi-hole blocking is Enabled

Do you mean you want to see a screenshot of a successful dnsrestart on upstart as well as systemd ?

@WaLLy3K

This comment has been minimized.

Copy link
Contributor

WaLLy3K commented Jul 19, 2017

The issue with the sippet is that on upstart, pihole believes dnsmasq is still running

If there's a result for pidof dnsmasq, then it's going to believe it's up and running before it runs any command.

As far as screenshots go, I'd just like to see any errors that are cropping up because I'm still failing to comprehend what's actually happening here (Though 3AM will do that to you!)

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Jul 19, 2017

My mistake.
dnsmasq keeps on running if I force an error by mangling the dnsmasq.conf file and restart dnsmasq (pidof dnsmasq).
I was expecting dnsmasq refusing to start/restart if I force an error but it does not.
I'll investigate/test a bit more and let you know.

@WaLLy3K WaLLy3K mentioned this pull request Jul 20, 2017
5 of 5 tasks complete
@WaLLy3K WaLLy3K mentioned this pull request Jul 24, 2017
5 of 5 tasks complete
@jacobsalmela

This comment has been minimized.

Copy link
Contributor

jacobsalmela commented Nov 25, 2017

@deHakkelaar did @WaLLy3K's PR #1630 fix this for you? Please let me know since this PR has gone stale.

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Nov 26, 2017

Sorry, I fell ill and forgot about it.
I'll rig a sandbox to check and let you know.

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Nov 28, 2017

Preliminary findings not good.
Previously, I just had to do this to get Pi-hole running on upstart:

https://discourse.pi-hole.net/t/ftl-on-xbian-pi/2882/15?u=dehakkelaar

Now I have issues with dnsmasq and lighttpd just installing stable:

http://paste.ubuntu.com/26065895/

And luckily dhcpcd5 didnt activate or it would probably have broken the XBian configuration tools and Kodi XBian add-on that allows to set network settings.
Will investigate bit more.

Install log here:

http://paste.ubuntu.com/26065912/

@deHakkelaar

This comment has been minimized.

Copy link
Author

deHakkelaar commented Nov 28, 2017

Oops, lighttpd just needed a reboot.
Other issues remain.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.