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

Keep FTL database open #896

Merged
merged 4 commits into from
Oct 22, 2020
Merged

Keep FTL database open #896

merged 4 commits into from
Oct 22, 2020

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Sep 28, 2020

By submitting this pull request, I confirm the following:

  • 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?:

10


Reopening the FTL database may lead to rare race-collisions in SQLite3. We avoid them by keeping the database connection open all the time.

…3. We avoid them by keeping the database connection open all the time.

Signed-off-by: DL6ER <dl6er@dl6er.de>
Signed-off-by: DL6ER <dl6er@dl6er.de>
@DL6ER
Copy link
Member Author

DL6ER commented Oct 9, 2020

Confirmed working in #900

@DL6ER DL6ER marked this pull request as ready for review October 9, 2020 04:38
@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/long-term-data-not-stored-it-seems/39204/6

@DL6ER DL6ER added the PR: Approval Required Open Pull Request, needs approval label Oct 18, 2020
@DL6ER DL6ER mentioned this pull request Oct 18, 2020
This was linked to issues Oct 18, 2020
@DL6ER
Copy link
Member Author

DL6ER commented Oct 19, 2020

CodeFactor can be ignored on this one as well. The mentioned security issue Possible OS Command Injection (CWE-78) isn't a problem here as the argument to popen() is a constant hard-coded string like ip neigh show. Hard to sneak something malicious into this...

@ampfinger
Copy link

I dont know if you can but could you rebuild with latest changes?
Currently for raspberry it tells me branch not found. I think it is because latest build failed.
Thanks

@DL6ER
Copy link
Member Author

DL6ER commented Oct 20, 2020

The failed build is without consequence and due to changes in the development branch building environment. All binaries are still up and running. However, some of them have been renamed (for the sake of clarity) so they might not be found.

Can you please post the exact output?
And also the output of pihole -v

@yubiuser
Copy link
Member

sha1sum: WARNING: 1 computed checksum did NOT match
  [✗] Downloading and Installing FTL
  Error: Download of https://ftl.pi-hole.net/fix/persistent_FTL_database/pihole-FTL-aarch64-linux-gnu failed (checksum error)
  [✗] FTL Engine not installed

@ampfinger
Copy link

ampfinger commented Oct 20, 2020

[i] FTL: Branch is not available. Use pihole checkout ftl [branchname] to switch to a valid branch. [i] Warning: You are using FTL from a custom branch (fix/persistent_FTL_database) and might be missing future releases.

@DL6ER
Copy link
Member Author

DL6ER commented Oct 20, 2020

@ampfinger Can you please also run

pihole -v

? If you're already on latest development (or a branch based on it), this is expected because I need to update this branch.

@yubiuser The upload might have failed before, I'll have to invent a check for this. I let the CI rebuild the aarch64 binary, could you try once more please?

@ampfinger
Copy link

Pi-hole version is development v5.0-232-gde02bcc (Latest: v5.1.2)
AdminLTE version is devel v5.1.1-93-ge84bfa9b (Latest: v5.1.1)
FTL version is development vDev-f69dab5 (Latest: v5.2)

Now I wanted back to this branch but cant go back

@yubiuser
Copy link
Member

Updated worked fine now.

@churchofnoise
Copy link

churchofnoise commented Oct 20, 2020

[i] FTL: Branch is not available. Use pihole checkout ftl [branchname] to switch to a valid branch. [i] Warning: You are using FTL from a custom branch (fix/persistent_FTL_database) and might be missing future releases.

I face the same thing. Just tried pihole -up again and th result remains the same.

(Output of pihole -v:
Pi-hole version is development v5.0-603-gde02bcc (Latest: v5.1.2)
AdminLTE version is devel v5.1.1-127-ge84bfa9 (Latest: v5.1.1)
FTL version is fix/persistent_FTL_database vDev-036031e (Latest: v5.2))

@yubiuser
Copy link
Member

Try checking it out the branch again

pihole checkout ftl fix/persistent_FTL_database

@ampfinger
Copy link

Try checking it out the branch again

pihole checkout ftl fix/persistent_FTL_database

Nope not working - keep telling "branch not available"

@DL6ER
Copy link
Member Author

DL6ER commented Oct 20, 2020

I'll update the branch when I'm back home (I'm about 6 hrs from now). I cannot do it (reliably) from my phone.

@churchofnoise
Copy link

Try checking it out the branch again
pihole checkout ftl fix/persistent_FTL_database

Nope not working - keep telling "branch not available"

Dito

@churchofnoise
Copy link

I'll update the branch when I'm back home (I'm about 6 hrs from now). I cannot do it (reliably) from my phone.

Thanks!

@DL6ER
Copy link
Member Author

DL6ER commented Oct 20, 2020

Got later than expected. First thing I did was updating this branch, now I will cook dinner :-)

@ampfinger
Copy link

Got later than expected. First thing I did was updating this branch, now I will cook dinner :-)

Thanks, checkout is working again! Enjoy your meal :)

@churchofnoise
Copy link

Got later than expected. First thing I did was updating this branch, now I will cook dinner :-)

Thanks!
Do you think this is ready to be merged into development?

@DL6ER
Copy link
Member Author

DL6ER commented Oct 22, 2020

Do you think this is ready to be merged into development?

Yes. Hence it is not a draft PR any more and tagged with PR: Approval required, hoping to pick up the attention of another developer, soon. There are other PRs waiting to be merged into development as well. They will come whenever they are approved.

@DL6ER DL6ER merged commit 13bf568 into development Oct 22, 2020
@DL6ER DL6ER deleted the fix/persistent_FTL_database branch October 22, 2020 19:21
@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/pi-hole-core-web-v5-2-and-ftl-v5-3-released/40909/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code maintanance PR: Approval Required Open Pull Request, needs approval
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FTL crashed Long-term database not written after restart
6 participants