Make DB interval adjustable #172

DL6ER · 2017-12-12T18:04:05Z

By submitting this pull request, I confirm the following (please check boxes, eg [X]) Failure to fill the template will close your PR:

Please submit all pull requests against the development branch. Failure to do so will delay or deny your request

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

Make interval in which FTL stores queries in the long term database easily adjustable via the config file.

It defaults to 1.0 (once per minute).
It cannot be lower than 0.1 (every six seconds).
It cannot be larger than 1440.0 (once a day).

Related to pi-hole/pi-hole#1839

This template was created based on the work of udemy-dl.

…asily adjustable via the config file. Signed-off-by: DL6ER <dl6er@dl6er.de>

Signed-off-by: DL6ER <dl6er@dl6er.de> Conflicts: README.md config.c

AzureMarker · 2017-12-12T18:21:20Z

Wouldn't writing to the database in intervals longer than a day miss some queries?

Signed-off-by: DL6ER <dl6er@dl6er.de>

AzureMarker · 2017-12-12T18:30:56Z

config.c

+ // How often do we store queries in FTL's database [minutes]?
+ // this value can be a floating point number, e.g. "DBINTERVAL=0.5"
+ // defaults to: 1 (once per minute)
+ config.DBinterval = 1;


The default should be 60 (once every 60 seconds) based on how it is used below.

Signed-off-by: DL6ER <dl6er@dl6er.de>

… is done (but before). This ensures that the queries will be written to the database even if the user specifies DBINTERVAL=n*60 (where n=1,2,3,...) which is a special that was not explicitly considered so far. Signed-off-by: DL6ER <dl6er@dl6er.de>

graysky2 · 2017-12-13T18:21:10Z

@DL6ER - I'm thinking that you probably also want to force a write-out before /usr/bin/pihole-FTL exits?

This could be handled by the init system (systemd for example) if there is a command one can pass to pihole-FTL that causes the db to be written.

DL6ER · 2017-12-13T18:38:08Z

force a write-out when /usr/bin/pihole-FTL exits

No, I don't really want that. It is unforeseeable how long the write will take and I consider it as bad practice to react only delayed to requests to terminate. I think a sane write out interval (i.e. several times an hour) should be sufficient to achieve that everything is properly saved.

There is only one scenario where you could loose some queries and that is having a very long interval, shutting down your Pi-hole and then powering it back on at least 48 hours later. I'm not sure if that is a very likely scenario as you wouldn't have DNS functionality in your network as long as Pi-hole is shut down.

graysky2 · 2017-12-13T18:45:20Z

@DL6ER - Sounds good, just wanted to check.

graysky2 · 2017-12-13T22:26:14Z

@DL6ER - The Arch package for pi-hole-ftl has this PR#172 and PR#167 applied to the standard v2.12. I have /etc/pihole/pihole-FTL.conf setup to sync every 121 minutes, but I haven't seen a sync occur (4 hours of uptime).

Local time is 5:25 PM:

% ls -lrt /etc/pihole
...
-rw-r--r-- 1 pihole pihole  147456 Dec 13 12:30 pihole-FTL.db

% cat /etc/pihole/pihole-FTL.conf 
SOCKET_LISTENING=localonly
TIMEFRAME=rolling24h
QUERY_DISPLAY=yes
AAAA_QUERY_ANALYSIS=yes
MAXDBDAYS=365
RESOLVE_IPV6=no
RESOLVE_IPV4=yes
DBINTERVAL=121.0
DBFILE=/etc/pihole/pihole-FTL.db

DL6ER · 2017-12-13T22:29:17Z

Strange... I will set the timer to 121.0 for me as well and report tomorrow what happened over night.

edit: So far, so good:

[2017-12-13 23:30:00.570]    DBINTERVAL: saving to DB file every 7260 seconds

graysky2 · 2017-12-13T23:48:20Z

I got the same...

% grep -i dbint /run/log/pihole-ftl/pihole-FTL.log 
[2017-12-13 13:26:21.132]    DBINTERVAL: saving to DB file every 7260 seconds

Yet still no writes (local time is 18:49)...

% stat /etc/pihole/pihole-FTL.db 
  File: /etc/pihole/pihole-FTL.db
  Size: 147456    	Blocks: 288        IO Block: 4096   regular file
Device: 30h/48d	Inode: 260690      Links: 1
Access: (0644/-rw-r--r--)  Uid: (  996/  pihole)   Gid: (  996/  pihole)
Access: 2017-12-13 14:49:00.053508686 -0500
Modify: 2017-12-13 12:30:00.196199782 -0500
Change: 2017-12-13 13:26:21.298180015 -0500
 Birth: -

DL6ER · 2017-12-14T06:42:45Z

@graysky2 It seems to work fine for me, writes are done every 02:01 (HH:MM):

[2017-12-14 00:51:00.185] Notice: Queries stored in DB: 826
[2017-12-14 00:51:00.186] Notice: Database size is 138.87 MB, deleted 0 rows
[...]
[2017-12-14 02:52:00.192] Notice: Queries stored in DB: 382
[2017-12-14 02:52:00.194] Notice: Database size is 138.90 MB, deleted 0 rows
[...]
[2017-12-14 04:53:00.184] Notice: Queries stored in DB: 412
[2017-12-14 04:53:00.185] Notice: Database size is 138.92 MB, deleted 0 rows
[...]
[2017-12-14 06:54:00.143] Notice: Queries stored in DB: 391
[2017-12-14 06:54:00.145] Notice: Database size is 138.95 MB, deleted 0 rows

and

$ stat /etc/pihole/pihole-FTL.db 
  File: ‘/etc/pihole/pihole-FTL.db’
  Size: 138950656       Blocks: 271400     IO Block: 4096   regular file
Device: b302h/45826d    Inode: 149592      Links: 1
Access: (0644/-rw-r--r--)  Uid: (  999/  pihole)   Gid: (  996/  pihole)
Access: 2017-08-02 17:20:12.882001730 +0200
Modify: 2017-12-14 06:54:00.115726043 +0100
Change: 2017-12-14 06:54:00.115726043 +0100
 Birth: -

graysky2 · 2017-12-14T09:47:19Z

Are you using 2.12 plus those 2 patches or the dev branch?

DL6ER · 2017-12-14T15:43:04Z

It continued as expected for me so far (08:55, 10:56, 12:57, 14:58).

I'm on the branch of this PR. I quickly reviewed what that means:

v2.12 + #166 + #168 + #171 + all comits in this PR

However, nothing outside of this PR should be changing anything here. How about other values? I think you said already that 60 is working, how about 61?

graysky2 · 2017-12-14T20:11:01Z

Very odd... a value of 2 works just fine. I am testing 61 now and will report back.

graysky2 · 2017-12-14T21:55:15Z

Nope... a value of 61 has failed to write.

% grep -i dbint /etc/pihole/pihole-FTL.conf       
DBINTERVAL=61

% grep -i dbint /run/log/pihole-ftl/pihole-FTL.log
[2017-12-14 14:51:19.538]    DBINTERVAL: saving to DB file every 3660 seconds

Local time is 16:55:

% stat /etc/pihole/pihole-FTL.db
  File: /etc/pihole/pihole-FTL.db
  Size: 208896    	Blocks: 408        IO Block: 4096   regular file
Device: 30h/48d	Inode: 260690      Links: 1
Access: (0644/-rw-r--r--)  Uid: (  996/  pihole)   Gid: (  996/  pihole)
Access: 2017-12-14 14:51:19.558958997 -0500
Modify: 2017-12-14 14:50:00.107371181 -0500
Change: 2017-12-14 14:50:00.107371181 -0500
 Birth: -

DL6ER · 2017-12-14T22:21:14Z

@graysky2 I'm confused. Are you sure that your Arch version hast the whole PR merged into it? Including 5951e5b ?

graysky2 · 2017-12-14T23:16:02Z

@DL6ER - Looks like that is omitted... see here: https://aur.archlinux.org/cgit/aur.git/commit/?h=pi-hole-ftl&id=6efb21f2818cc18b68e86cbf03e79fe742c6f510

graysky2 · 2017-12-14T23:21:21Z

The X and checkmarks in the git log are confusing to me...
...So our package should include 2.12 + which commits?

https://github.com/pi-hole/FTL/compare/95bdd62777347f2117070ce8060b2ca1cc3072c0...8ab56e58f29fc3e5263ab51b80534154832ffbd6.patch

???

AzureMarker · 2017-12-15T02:24:27Z

Are you trying to make the current branch a package? Why do you need to distinguish between commits?

graysky2 · 2017-12-15T09:02:42Z

@Mcat12 - No, the intent is to incorporate the adjustable db into 2.12. The patch I referenced above is working with a DBINTERVAL=121.0 by the way.

DL6ER · 2017-12-15T10:22:54Z

Okay, so there is no issue with this PR or our code base but only in the way you applied which modifications to the last tagged release. Good to know, thanks for doing extensive testing BTW.

graysky2 · 2017-12-15T17:46:04Z

@DL6ER - You're welcome and thank you for the quick implementation of this request. Is the patch I referenced above the correct set of commits to apply to 2.12 for the Arch package?

DL6ER · 2017-12-16T09:04:52Z

@graysky2

Is the patch I referenced above the correct set of commits to apply to 2.12 for the Arch package?

Not sure what/which link you are referring to... If you are asking what the diff is that leads to this branch, then it is https://github.com/pi-hole/FTL/compare/master...5951e5b.diff
However, if it is working now for you, then I'd assume you already have all required commits added to your build system. We targeted the next release rather sooner than later (ideally before Christmas), so all changes will become live pretty soon.

graysky2 · 2017-12-16T11:31:03Z

Thanks again. Will wait for 2.12.1 or whatever you bump to.

DL6ER added 2 commits December 12, 2017 18:40

Make interval in which FTL stores queries in the long term database e…

06b9e02

…asily adjustable via the config file. Signed-off-by: DL6ER <dl6er@dl6er.de>

Merge branch 'development' into new/DBinterval

59edb2a

Signed-off-by: DL6ER <dl6er@dl6er.de> Conflicts: README.md config.c

DL6ER added the Enhancement label Dec 12, 2017

DL6ER mentioned this pull request Dec 12, 2017

Bug/design flaw: /etc/pihole/pihole-FTL.db is on an SD card pi-hole/pi-hole#1839

Closed

3 tasks

Maximum should be one day

05afda1

Signed-off-by: DL6ER <dl6er@dl6er.de>

AzureMarker reviewed Dec 12, 2017

View reviewed changes

Default should be once per minute

effca04

Signed-off-by: DL6ER <dl6er@dl6er.de>

AzureMarker previously approved these changes Dec 13, 2017

View reviewed changes

DL6ER dismissed AzureMarker’s stale review via e72fbcb December 13, 2017 16:58

DL6ER force-pushed the new/DBinterval branch from e72fbcb to 5951e5b Compare December 13, 2017 17:25

AzureMarker approved these changes Dec 13, 2017

View reviewed changes

DL6ER merged commit 8ab56e5 into development Dec 13, 2017

DL6ER deleted the new/DBinterval branch December 13, 2017 21:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make DB interval adjustable #172

Make DB interval adjustable #172

DL6ER commented Dec 12, 2017 •

edited

Loading

AzureMarker commented Dec 12, 2017

AzureMarker Dec 12, 2017

graysky2 commented Dec 13, 2017 •

edited

Loading

DL6ER commented Dec 13, 2017

graysky2 commented Dec 13, 2017

graysky2 commented Dec 13, 2017

DL6ER commented Dec 13, 2017 •

edited

Loading

graysky2 commented Dec 13, 2017 •

edited

Loading

DL6ER commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 14, 2017 via email •

edited

Loading

DL6ER commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 14, 2017

graysky2 commented Dec 14, 2017 •

edited

Loading

DL6ER commented Dec 14, 2017

graysky2 commented Dec 14, 2017

graysky2 commented Dec 14, 2017 •

edited

Loading

AzureMarker commented Dec 15, 2017

graysky2 commented Dec 15, 2017 •

edited

Loading

DL6ER commented Dec 15, 2017

graysky2 commented Dec 15, 2017

DL6ER commented Dec 16, 2017 •

edited

Loading

graysky2 commented Dec 16, 2017

Make DB interval adjustable #172

Make DB interval adjustable #172

Conversation

DL6ER commented Dec 12, 2017 • edited Loading

10

AzureMarker commented Dec 12, 2017

AzureMarker Dec 12, 2017

Choose a reason for hiding this comment

graysky2 commented Dec 13, 2017 • edited Loading

DL6ER commented Dec 13, 2017

graysky2 commented Dec 13, 2017

graysky2 commented Dec 13, 2017

DL6ER commented Dec 13, 2017 • edited Loading

graysky2 commented Dec 13, 2017 • edited Loading

DL6ER commented Dec 14, 2017 • edited Loading

graysky2 commented Dec 14, 2017 via email • edited Loading

DL6ER commented Dec 14, 2017 • edited Loading

graysky2 commented Dec 14, 2017

graysky2 commented Dec 14, 2017 • edited Loading

DL6ER commented Dec 14, 2017

graysky2 commented Dec 14, 2017

graysky2 commented Dec 14, 2017 • edited Loading

AzureMarker commented Dec 15, 2017

graysky2 commented Dec 15, 2017 • edited Loading

DL6ER commented Dec 15, 2017

graysky2 commented Dec 15, 2017

DL6ER commented Dec 16, 2017 • edited Loading

graysky2 commented Dec 16, 2017

DL6ER commented Dec 12, 2017 •

edited

Loading

graysky2 commented Dec 13, 2017 •

edited

Loading

DL6ER commented Dec 13, 2017 •

edited

Loading

graysky2 commented Dec 13, 2017 •

edited

Loading

DL6ER commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 14, 2017 via email •

edited

Loading

DL6ER commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 14, 2017 •

edited

Loading

graysky2 commented Dec 15, 2017 •

edited

Loading

DL6ER commented Dec 16, 2017 •

edited

Loading