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 Query Log details across FTL restart #1285

Merged
merged 4 commits into from
Feb 2, 2022

Conversation

DL6ER
Copy link
Member

@DL6ER DL6ER commented Jan 19, 2022

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


When restarting FTL, it imports history from the database to repopulate the history. So far, this was only possible with a reduced set of variables to keep the database from growing forever. Recent database optimizations #1255 reduced the database size significantly allowing us to store more data in the database.

This PR adds storing all the details necessary for the query log so a restart goes (almost) invisible and without any data loss.

*) almost because some queries may not have been completed due to the forced restart and according service interruption.

@DL6ER DL6ER changed the title Loss-free FTL restart Keep Query Log details across FTL restart Jan 19, 2022
@DL6ER DL6ER marked this pull request as ready for review January 30, 2022 10:36
@DL6ER DL6ER added the PR: Approval Required Open Pull Request, needs approval label Jan 30, 2022
src/database/query-table.c Show resolved Hide resolved
test/test_suite.bats Outdated Show resolved Hide resolved
@yubiuser
Copy link
Member

yubiuser commented Jan 31, 2022

I tried it but it did not really work for me. I got a lot of

[2022-01-31 20:07:14.640 24751M] DB warn: REPLY value 52 is invalid, 1643656012
[2022-01-31 20:07:14.640 24751M] DB warn: REPLY value 116 is invalid, 1643656013
[2022-01-31 20:07:14.640 24751M] DB warn: REPLY value 116 is invalid, 1643656013
[2022-01-31 20:07:14.640 24751M] DB warn: REPLY value 52 is invalid, 1643656017

But there is no reply_type with 52 or 116 in the database.

Bildschirmfoto zu 2022-01-31 20-34-28

I also queried a test domain which was successfully stored in the database with all information, including reply_typ=4. After a restart, the query log displays Reply N/A

src/database/query-table.c Show resolved Hide resolved
src/database/query-table.c Show resolved Hide resolved
@DL6ER
Copy link
Member Author

DL6ER commented Feb 1, 2022

Thanks for your review, not sure how I missed this. We also have to set the response_time_calculated property to true or the reply will not be shown on the dashboard. The API still presented the data but the Javascript skipped it as it thought: When we don't have a response time, we cannot have a reply type either (as there is not response so far). Should be fixed.

@yubiuser
Copy link
Member

yubiuser commented Feb 1, 2022

Thanks for the update.

There are no REPLY value 52 is invalid warnings any more. Also the reply time and reply type dashboard issue is fixed.

@DL6ER DL6ER merged commit ed73b76 into development Feb 2, 2022
@DL6ER DL6ER deleted the new/database_reply_replytime_dnssec branch February 2, 2022 05:10
@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/a-graph-visualizing-response-times-of-forwarded-queries/53320/1

@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-ftl-v5-14-web-v5-11-and-core-v5-9-released/53529/1

@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/resolve-client-ip-adresses-for-the-long-term-data-section/55655/2

@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/add-hostname-of-clients-to-long-term-query-database/50103/11

@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/can-we-have-an-option-to-keep-old-queries-when-we-do-restartdns/37304/8

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

Successfully merging this pull request may close these issues.

3 participants