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

Trying to close #703

Closed
3 tasks done
arthurdarcet opened this issue Mar 1, 2020 · 3 comments
Closed
3 tasks done

Trying to close #703

arthurdarcet opened this issue Mar 1, 2020 · 3 comments

Comments

@arthurdarcet
Copy link

In raising this issue, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your issue:

  • I have read and understood the contributors guide.
  • The issue I am reporting can be replicated
  • The issue I am reporting isn't a duplicate

How familiar are you with the codebase?:

1


[BUG | ISSUE]

On startup, FTL tries to close all file descriptors from 0 to ulimit -n (the maximum number of open files) - here https://github.com/pi-hole/FTL/blob/master/dnsmasq/dnsmasq.c#L149

On setups where this limit is high, this takes a large amount of time - or even "hangs" indefinitely. Docker sets this limit to ~1M for instance - and the kubernetes setup I am using (k3s) is setting it to "Infinity", which translates to 1073741816.

I would suggest switching over to something based on the actually open files - like was done for the rpm package: cf https://bugzilla.redhat.com/show_bug.cgi?id=1537564 and https://github.com/rpm-software-management/rpm/pull/444/files for an actual patch

Work around is to run a ulimit -n 1024 before starting FTL in the container - but that's not really a clean solution. (ie add command: [ sh, -c, ulimit -n 1024 && /s6-init ] in the k8s container)

Might be related to pi-hole/docker-pi-hole#561 - I think I was hitting that issue as well

Device specifics

Hardware Type: k3s - kubernetes 1.17 / docker 19.03.6-ce

@DL6ER
Copy link
Member

DL6ER commented Mar 1, 2020

The code you're referring to is the original upstream code of dnsmasq, see: http://thekelleys.org.uk/gitweb/?p=dnsmasq.git;a=blob;f=src/dnsmasq.c;h=573aac069b75b801ad1ddf5305af5010ca1ab88a;hb=HEAD#l151

Any modification to the dnsmasq code needs to be imported from the upstream project to avoid deviations inevitably leading to merge conflicts down the road.

Please contact Simon Kelley over the dnsmasq mailing list for this issue, I'm sure he will have something to say about this: [email protected]

As soon as the dnsmasq projects adopts a code, we can discuss cherry-picking it into our code even before the change is released for the official dnsmasq release.

@arthurdarcet
Copy link
Author

Ok thanks - didn't realise that was dnsmasq code. I'll reach out to the dnsmasq ML

@DL6ER
Copy link
Member

DL6ER commented Mar 8, 2020

Just a quick heads up: The code has already landed in FTL (e26f786) and is currently waiting in update/dnsmasq for merging into release/v5.0 when dnsmasq v2.81 is released.

@DL6ER DL6ER reopened this Mar 8, 2020
@DL6ER DL6ER closed this as completed Mar 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants