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

Allow parallel FTL processes #2183

Closed
wants to merge 2 commits into from
Closed

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Feb 8, 2025

What does this implement/fix?

So far, FTL tried to prevent multiple processes from being started on the same machine. However, as the pihole-FTL binary became kind of a Swiss-army-knife offering integrated features such as sqlite3, sqlite3_rsync, sha256sum, gzip, lua, dhcp-discover, and others (like a small TLS certificate managing toolset), it becomes more and more likely that there are long-lived interactive pihole-FTL instances on the machine. They may coexist without any issues, however, once such an interactive process is running, restarting the original pihole-FTL may be prevented under certain circumstances.

Hence, I propose going a different, much easier path: Allow parallel processes to coexist peacefully much like most other daemons do. Duplicate processes will in the end not be able to fully start up as they cannot acquire the DNS and webserver ports. However, the currently possible situation of not being able to restart the main DNS-providing process is resolved hereby.

We allow the new parallelism by ensuring uniqueness of the shard memory objects by changing their name to include the main PID of each FTL process, e.g., FTL-34532-clients instead of (right now) the shorter FTL-clients.

No change to the post-stop command is needed as the deletion glob still matches the tweaked names:

Screenshot from 2025-02-08 18-39-40


Related issue or feature (if applicable): N/A

Pull request in docs with documentation (if applicable): N/A


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.
  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)

Checklist:

  • The code change is tested and works locally.
  • I based my code and PRs against the repositories developmental branch.
  • I signed off all commits. Pi-hole enforces the DCO for all contributions
  • I signed all my commits. Pi-hole requires signatures to verify authorship
  • I have read the above and my PR is ready for review.

…running duplicates don't interfere with each other

Signed-off-by: DL6ER <[email protected]>
@DL6ER DL6ER requested a review from a team February 8, 2025 18:24
@DL6ER DL6ER force-pushed the tweak/process_parallelism branch from 9ae653c to c1fa324 Compare February 8, 2025 18:33
@pralor-bot
Copy link

This pull request has been mentioned on Pi-hole Userspace. There might be relevant details there:

https://discourse.pi-hole.net/t/i-cant-reach-the-pi-hole-v6-web-interface/75639/27

Copy link
Member

@yubiuser yubiuser left a comment

Choose a reason for hiding this comment

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

This PR nicely fixes the issue with long-running interactive FTL processes parallel with a 'main' FTL doing the DNS work. However, it does not prevent users from manually starting a second 'full' FTL wich will only start some components and runs as kind of zombie in the background

2025-02-09_10-19

Maybe we could only allow one main FTL process and all other allowed FTL processes need to be called with a specific sub-process command (e.g. sqlite3, sha256sum,....)?

@DL6ER DL6ER mentioned this pull request Feb 9, 2025
5 tasks
@DL6ER
Copy link
Member Author

DL6ER commented Feb 9, 2025

Closing in favor of #2184

@DL6ER DL6ER closed this Feb 9, 2025
@DL6ER DL6ER deleted the tweak/process_parallelism branch February 9, 2025 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants