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

Added Alpine Linux support #4827

Closed
wants to merge 6 commits into from
Closed

Added Alpine Linux support #4827

wants to merge 6 commits into from

Conversation

giulio-coa
Copy link

@giulio-coa giulio-coa commented Jul 18, 2022

Thank you for your contribution to the Pi-hole Community!

Please read the comments below to help us consider your Pull Request.

We are all volunteers and completing the process outlined will help us review your commits quicker.

Please make sure you

  1. Base your code and PRs against the repositories developmental branch.
  2. Sign Off all commits as we enforce the DCO for all contributions

  • What does this PR aim to accomplish?:

I would like to add Alpine Linux support so that I can create lighter Pi-hole Docker Images.

  • How does this PR accomplish the above?:

I have done the changes to adapt the code to Alpine Linux.
The pre-requisites to run Pi-hole on Alpine are:

  1. have bash installed (apk add bash)
  2. have busybox-initscripts installed (apk add busybox-initscripts)
  3. Obviously, run the scripts as root or have sudo installed (apk add sudo; echo '%wheel ALL=(ALL) ALL' > /etc/sudoers.d/wheel)

I tried to tests my changes trough tox, but I failed...

The only file I haven't changed completely is advance/Scripts/setupLCD.sh because it uses Adafruits.
I don't know about project Adafruits and the links used in the code are no longer valid.

To completely support Alpine Linux, you must add it to versions.pi-hole.net and dev-supportedos.pi-hole.net.

  • What documentation changes (if any) are needed to support this PR?:

Replace this with a detailed list of any necessary changes


By submitting this pull request, I confirm the following:

  1. I have read and understood the contributors guide, as well as this entire template. I understand which branch to base my commits and Pull Requests against.
  2. I have commented my proposed changes within the code and I have tested my changes.
  3. I am willing to help maintain this change if there are issues with it later.
  4. It is compatible with the EUPL 1.2 license
  5. I have squashed any insignificant commits. (git rebase)

  • I have read the above and my PR is ready for review. Check this box to confirm

Signed-off-by: Giulio Coa <giulioc008@gmail.com>
Signed-off-by: Giulio Coa <giulioc008@gmail.com>
Signed-off-by: Giulio Coa <giulioc008@gmail.com>
@giulio-coa giulio-coa changed the base branch from master to development July 18, 2022 16:30
…port

# Conflicts:
#	automated install/basic-install.sh
@PromoFaux
Copy link
Member

Thanks for the contribution!

so that I can create lighter Pi-hole Docker Images.

This might make it easier to have an official alpine image, too.

I tried to tests my changes trough tox, but I failed...

This reminds me, I keep thinking about setting up dev container for vscode to make running tests locally easier. It should just be a case of having Python 3.8 installed, install requirements from test/requirements.txt, and then tox -c test/tox.${DISTRO}.ini, where ${DISTRO} is the name of the distro you want to be testing

The only file I haven't changed completely is advance/Scripts/setupLCD.sh

You can ignore this, and probably we can remove it, but that's for another PR

To completely support Alpine Linux, you must add it to versions.pi-hole.net and dev-supportedos.pi-hole.net.

I can do the latter for now. Can you get me an output from /etc/os-release please?

@PromoFaux
Copy link
Member

Just a note, for tests to run here you'll need to add alpine to the distro matrix in .github/workflows/test.yml (distro-test job)

@giulio-coa
Copy link
Author

giulio-coa commented Jul 18, 2022

I can do the latter for now. Can you get me an output from /etc/os-release please?

Thanks for the help

# cat /etc/os-release 
NAME="Alpine Linux"
ID=alpine
VERSION_ID=3.16.0
PRETTY_NAME="Alpine Linux v3.16"
HOME_URL="https://alpinelinux.org/"
BUG_REPORT_URL="https://gitlab.alpinelinux.org/alpine/aports/-/issues"

@giulio-coa
Copy link
Author

Just a note, for tests to run here you'll need to add alpine to the distro matrix in .github/workflows/test.yml (distro-test job)

Done

@giulio-coa
Copy link
Author

giulio-coa commented Jul 18, 2022

This reminds me, I keep thinking about setting up dev container for vscode to make running tests locally easier. It should just be a case of having Python 3.8 installed, install requirements from test/requirements.txt, and then tox -c test/tox.${DISTRO}.ini, where ${DISTRO} is the name of the distro you want to be testing

I did what you said, but I get this output:

ERROR: invocation failed (exit code 1), logfile: #############/pi-hole/test/.tox/log/GLOB-0.log
============================================================================== log start ==============================================================================
/usr/lib/python3.10/site-packages/setuptools/installer.py:27: SetuptoolsDeprecationWarning: setuptools.installer is deprecated. Requirements should be satisfied by a PEP 517 installer.
  warnings.warn(
error: Multiple top-level modules discovered in a flat-layout: ['test_fedora_support', 'test_centos_7_support', 'test_centos_common_support', 'test_any_automated_install', 'test_centos_fedora_common_support', 'test_any_utils', 'test_centos_8_support'].

To avoid accidental inclusion of unwanted files or directories,
setuptools will not proceed with this build.

If you are trying to create a single distribution with multiple modules
on purpose, you should not rely on automatic discovery.
Instead, consider the following options:

1. set up custom discovery (`find` directive with `include` or `exclude`)
2. use a `src-layout`
3. explicitly set `py_modules` or `packages` with a list of names

To find more information, look for "package discovery" on setuptools docs.

=============================================================================== log end ===============================================================================
ERROR: FAIL could not package project - v = InvocationError('/usr/bin/python setup.py sdist --formats=zip --dist-dir #############/pi-hole/test/.tox/dist', 1)

@PromoFaux
Copy link
Member

Actually, thinking about it - I'm trying to remember if I even had to install python/the requirements locally... I vaguely recall just installing tox via apt and then running the tox command... but it was such a long time ago that I set this machine up, I don't remember!

https://github.com/pi-hole/pi-hole/tree/master/test#readme

For me, the test are running - but they are all failing, so that will need looking at once you have the tests running locally

@giulio-coa
Copy link
Author

Just a note, for tests to run here you'll need to add alpine to the distro matrix in .github/workflows/test.yml (distro-test job)

It is the only test that the code fails.
I probably wrote something wrong in the tests/ files

@PromoFaux
Copy link
Member

It's the _alpine.Dockerfile

From your OP:

The pre-requisites to run Pi-hole on Alpine are:

  1. have bash installed (apk add bash)
  2. have busybox-initscripts installed (apk add busybox-initscripts)
  3. obviously, run the scripts as root or have sudo installed (apk add sudo; echo '%wheel ALL=(ALL) ALL' > /etc/sudoers.d/wheel)

So obviously the prerequisites need to be installed in the image used to run the tests :)

Adding bash busybox-initscripts to the existing apk add command allows for some of the tests to pass, but there are still 13 failing

@giulio-coa
Copy link
Author

Adding bash busybox-initscripts to the existing apk add command allows for some of the tests to pass, but there are still 13 failing

Some tests do not pass because they need to be modified to add support to Alpine (use of OpenRC, etc.)
I have no experience with pytest and the utilities used, so I need a hand 😅

@giulio-coa
Copy link
Author

@PromoFaux I don't understand why these 3 tests fail
I know it's because /usr/bin/pihole-FTL is missing, but I don't know why that happens

@giulio-coa
Copy link
Author

giulio-coa commented Jul 19, 2022

I also need to know how to mock commands like systemctl to do its Alpine version (rc-service, rc-update, etc.)
https://github.com/giulio-coa/pi-hole/blob/new/alpine-support/test/test_any_automated_install.py#L161-L194

On Alpine, the paths for the scripts are:

  • Obviously, /bin/<COMMAND> (i.e. /bin/bash, etc.)
  • /sbin/<COMMAND> (i.e. /sbin/apk, etc.)
  • Obviously, /usr/bin/<COMMAND> (i.e. /usr/bin/getent, etc.)
  • /usr/sbin/<COMMAND> (i.e. /usr/sbin/openvpn, etc.)

You can get they from command -v <COMMAND>

@dschaper
Copy link
Member

You don't need to mock systemctl, you need to add support for OpenRC.

automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
automated install/basic-install.sh Outdated Show resolved Hide resolved
@giulio-coa
Copy link
Author

giulio-coa commented Jul 19, 2022

You don't need to mock systemctl, you need to add support for OpenRC.

It is what I said, but I don't know how to do it

@giulio-coa giulio-coa requested a review from dschaper July 19, 2022 17:36
Signed-off-by: Giulio Coa <giulioc008@gmail.com>
@giulio-coa
Copy link
Author

giulio-coa commented Jul 22, 2022

So, now the only two things to do are:

  • Add support to OpenRC in tests
  • Fix bug /usr/bin/pihole-FTL: No such file or directory

@PromoFaux @dschaper is there any idea?
I don't know the test tool you use 😅

@PromoFaux
Copy link
Member

I'll have a play about locally, but I don't really know anything about alpine - I also don't really know the test system all that well, so it's just a case of trial and error (or throwing mud at the wall to see what sticks)

@PromoFaux
Copy link
Member

So, taking one of the failing tests, e.g test_FTL_binary_installed_and_responsive_no_errors - I have spun up a container using the dockerfile you wrote for Alpine test image, and am trying to run what the test runs manually, and we see here that pihole-FTL cannot be run.

adam@adam-pc:~/pi-hole$ docker run -it --rm pytest_pihole:test_container bash
bash-5.1# source /opt/pihole/basic-install.sh
    create_pihole_user
    funcOutput=$(get_binary_name)
    binary="pihole-FTL${funcOutput##*pihole-FTL}"
    theRest="${funcOutput%pihole-FTL*}"
    FTLdetect "${binary}" "${theRest}"
  [✓] Creating user 'pihole'

  [i] FTL Checks...

  [✓] Detected x86_64 processor
  [i] Checking for existing FTL binary...
  [✓] Downloading and Installing FTL
bash-5.1# pihole-FTL version
bash: /usr/bin/pihole-FTL: No such file or directory

Do you actually have a working Pi-hole install on an Alpine system? I cannot even get the installer to run inside the container...

@PromoFaux
Copy link
Member

Ah. The installer is downloading the standard linux-x86_64 binary, it needs to be downloading the musl-linux-x86_64 one

Inside get_binary_name an additional check needs to be added, like so:

# 64bit
printf "%b  %b Detected x86_64 processor\\n" "${OVER}" "${TICK}"
# set the binary to be used
detected_os=$(grep '^ID=' /etc/os-release | cut -d '=' -f2 | tr -d '"')
if [[ "${detected_os}" == "alpine" ]]; then
  l_binary="pihole-FTL-musl-linux-x86_64"
else
  l_binary="pihole-FTL-linux-x86_64"
fi

That solves the /usr/bin/pihole-FTL: No such file or directory issue, and additionally test_FTL_binary_installed_and_responsive_no_errors. Still failing on 2 tests though

@giulio-coa
Copy link
Author

giulio-coa commented Jul 23, 2022

Ah. The installer is downloading the standard linux-x86_64 binary, it needs to be downloading the musl-linux-x86_64 one

And other processors ?

Signed-off-by: Giulio Coa <giulioc008@gmail.com>
@PromoFaux
Copy link
Member

And other processors ?

Good question, and to be honest - I think maybe we need to take a step back here and rethink things. Some unordered thoughts:

  • It would be nice to do this without having to rely on the system having bash - but in order to do that, all scripts need to be made POSIX compatible so that they can run in sh. That's a pretty large hurdle in itself
  • We would need to add FTL build images to include -musl images for all possible archs (See here)
  • The tests are proving difficult to "get right" - maybe we need to rethink the tests, too?
  • Most people are going to be installing Pi-hole on a Raspberry Pi/SBC of some sort very few of those users are likely to be running Alpine
  • A smaller docker image is definitely a nice to have, there is an issue tracking this docker image based on Alpine? docker-pi-hole#980 (which also has a link to a project on Gitlab that is a full conversion to run exclusively on alpine)
  • This is a lot of effort and added complication for something that affects a relatively small percentage of both existing and potential users.

Ultimately, what I'm saying is "If we're going to do this, let's do it properly". That's not to say the effort you've put into looking at this is not appreciated - but I just think that maybe it needs to be approached from another angle

@giulio-coa
Copy link
Author

* It would be nice to do this _without_ having to rely on the system having bash - but in order to do that, all scripts need to be made POSIX compatible so that they can run in `sh`. That's a pretty large hurdle in itself

Are you sure ?
Most operating systems have /bin/sh a symlink to /bin/bash

@PromoFaux
Copy link
Member

The thinking being that the most obvious reason to use alpine over other distros is having a smaller image size for containers (maybe on hardware, but I've never personally seen it) - so being POSIX compatible and removing the need for bash brings down that overall image size. Maybe not by much, but I've seen people lose their minds (hyperbole!) When the diff between one release of an image and another is 3-4MB

I've not built any images with/without bash to compare though, maybe it's not all that much different.

@dschaper
Copy link
Member

Are you sure ?
Most operating systems have /bin/sh a symlink to /bin/bash

Not Alpine. It uses dash and busybox. You have to manually add in bash.

@dschaper
Copy link
Member

Let's try this in a clean and fresh PR. I won't accept this PR:

  1. It touches way to many files.
  2. It had commits and changes that have nothing to do with the topic at hand.
  3. It's been force pushed and modified too many times.

Please try to do these changes on your own fork and get things to a place where the PR can be accepted with only a review of code and functionality. We are volunteering our time and trying to review a moving target is asking too much of us.

@dschaper dschaper closed this Jul 24, 2022
@giulio-coa giulio-coa deleted the new/alpine-support branch July 24, 2022 20:54
@r10513
Copy link

r10513 commented Aug 20, 2022

I set up my docker image based on alpine (with also unbound).. I know, there might be a lot of things that can be improved.. but it is a baseline. I welcome any help

https://github.com/r10513/pihole_alpine

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

4 participants