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

Use systemd in test containers #5044

Closed
wants to merge 17 commits into from
Closed

Use systemd in test containers #5044

wants to merge 17 commits into from

Conversation

yubiuser
Copy link
Member

@yubiuser yubiuser commented Dec 1, 2022

What does this PR aim to accomplish?:

PR #4924 added a native systemd service file. All our currently supported OS run systemd. However, during the automatic test suite we don't run systemd so far and mock systemctl call.

mock_command_2(
"systemctl",
{
"enable lighttpd": ("", "0"),
"restart lighttpd": ("", "0"),
"start lighttpd": ("", "0"),
"enable pihole-FTL": ("", "0"),
"restart pihole-FTL": ("", "0"),
"start pihole-FTL": ("", "0"),
"*": ('echo "systemctl call with $@"', "0"),

Some images already require installation of an initsystem (e.g. centoOS and Fedora). This PR adds systemdas initsystem to the images so that the containers start with systemd as PID 1. This mimics the real-world OS situation better.

How does this PR accomplish the above?:

  1. Adds the required systemd packages to the images.
  2. Starts the test containers with --privileged instead of --cap-add=ALL (required to start systemd within the container) Uses podman instead of docker as container manager
  3. Removes the mocked systemctl from the test suite.
  4. Allows pihole restartdns to use systemctl instead of service if available. See this PR Allow pihole restart via systemctl #5045 for more details

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)
  6. I have checked that another pull request for this purpose does not exist.
  7. I have considered, and confirmed that this submission will be valuable to others.
  8. I accept that this submission may not be used, and the pull request closed at the will of the maintainer.
  9. I give this submission freely, and claim no ownership to its content.

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

@yubiuser
Copy link
Member Author

yubiuser commented Dec 1, 2022

I removed the mocked systemctl command in 8e6bb15. However, we could leave it in there and bypass it by checking if systemd is PID 1. This would keep the code in case we want to add a OS which is not using systemd as init system in the future.

@yubiuser
Copy link
Member Author

yubiuser commented Dec 1, 2022

I'll make a separate PR from 097e758. Just leaving it in here at the moment as it is a requirement for some OS to pass the test. I'll rebase once the other PR is merged.


Add:
PR #5045 failed the test and will always fail them until we remove/bypass the mock systemctl. I'll leave it in this PR then and amend the PR description.

@yubiuser yubiuser changed the title Tests systemd Use systemd in test containers Dec 1, 2022
@yubiuser yubiuser mentioned this pull request Dec 1, 2022
1 task
@yubiuser
Copy link
Member Author

yubiuser commented Dec 2, 2022

Converting to draft until I've figured out why CentOS 8 needs 2:45h to pass the test on GH. Runs fine locally within 5 mins.

@yubiuser yubiuser marked this pull request as draft December 2, 2022 06:12
@yubiuser yubiuser marked this pull request as ready for review December 3, 2022 21:05
@PromoFaux
Copy link
Member

According to the logs this is still taking a long time for Centos8 - have you got to the bottom of why?

@DL6ER
Copy link
Member

DL6ER commented Dec 11, 2022

Allows pihole restartdns to use systemctl instead of service if available

I don't think this is necessary as service is a wrapper. Do we really need this (complexity)?

@yubiuser
Copy link
Member Author

I think yes, that's why I created the PR.

All distros use systemd by default, we have a native systemd unit now. Some container images already need an additional initsystem to pass the test (fedora, centos) - there is no service installed. Instead of installing sysVinit we should test with what is actually used by the distros.
Background of all of this: We use the service file (and now prestart.sh) to set file permissions and create files/folders. We check for them. So we need to start the scripts/service to set the permissions.

@yubiuser
Copy link
Member Author

I just pushed the identical commits to a new branch and created PR #5057 and it finished in 4 minutes for CentOS8
https://github.com/pi-hole/pi-hole/actions/runs/3670895068/jobs/6205732893

@yubiuser yubiuser added PR: Approval Required Open Pull Request, needs approval and removed ON HOLD labels Dec 11, 2022
@PromoFaux
Copy link
Member

Maybe there was just something wrong a week ago and it just needed a kick today. I think we can close the other PR.

Do we really need this (complexity)?

One thing I do like here is that it removes some of the clusterfuck code in the tests that mocks systemctl, and anything we can do to make the tests easier to follow and maintain is a good thing in my book.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Dec 19, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Dec 20, 2022
@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

test/_centos_8.Dockerfile Outdated Show resolved Hide resolved
@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Dec 29, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Dec 29, 2022
@github-actions
Copy link

Conflicts have been resolved.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions
Copy link

Conflicts have been resolved.

@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Feb 17, 2023
@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Mar 4, 2023
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Mar 4, 2023
@github-actions
Copy link

github-actions bot commented Mar 4, 2023

Conflicts have been resolved.

@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Apr 15, 2023
Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Apr 15, 2023
@github-actions
Copy link

Conflicts have been resolved.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Jun 17, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Jun 18, 2023
@github-actions
Copy link

Conflicts have been resolved.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Jun 25, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Signed-off-by: Christian König <ckoenig@posteo.de>
@github-actions github-actions bot removed the PR: Merge Conflict Issue blocking check and merge of code label Jun 25, 2023
@github-actions
Copy link

Conflicts have been resolved.

@github-actions github-actions bot added the PR: Merge Conflict Issue blocking check and merge of code label Jul 18, 2023
@github-actions github-actions bot added the stale label Aug 17, 2023
@github-actions
Copy link

Existing merge conflicts have not been addressed. This PR is considered abandoned.

@github-actions github-actions bot closed this Aug 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Approval Required Open Pull Request, needs approval PR: Merge Conflict Issue blocking check and merge of code stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants