Codecov Report

Merging #139 (3492d42) into master (17ea625) will increase coverage by 1.14%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master     #139      +/-   ##
==========================================
+ Coverage   41.85%   43.00%   +1.14%     
==========================================
  Files          14       14              
  Lines         743      765      +22     
==========================================
+ Hits          311      329      +18     
- Misses        387      389       +2     
- Partials       45       47       +2     

Flags with carried forward coverage won't be shown. Click here to find out more.

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 17ea625...3492d42. Read the comment docs.

Alternatively, instead of using fsnotify and an asynchronous update of the database, it would also be possible to:

• check the leases file for a matching lease on every request instead of once on initialisation. Pro: simplest solution, Cons: more IO and if something breaks in the time between startup and a client requesting a lease, the client if left without a lease. If this same problem would occur during startup, CoreDHCP would just not start and you'd notice. OTOH, this could be solved by using the old database until it gets refreshed.
• always refresh the database on a request if this wasn't done for the last X minutes: Pro: less frequent I/O than the previous solution. Cons: you'd have to either hardcode a decent cache interval which might not meet user's requirements, or you'd have to make this configurable.

What do you think @Natolumin ?

I don't have a strong opinion on that point, doing it async is ok I think, and doing it during requests may impact latency a bit but not noticeably (unless the file is on NFS or something)

What I'm less sure about here is the design idea that it gets updated without any user intervention when the file updates. 2 (somewhat related) concerns:

• This will probably work ok when a file is atomically replaced (make a tempfile, rename(2) onto the previous file), but when writing into the file directly, we could be parsing incomplete files. In the best case scenario that will fail to parse (and throw some errors in the log), but we could also parse valid-but-unintended lines, eg:

aa:bb:cc:dd:ee:ff 192.0.2.1 # we parse this incomplete line
aa:bb:cc:dd:ee:ff 192.0.2.128 # when this was desired

• There is no feedback mechanism, or any way for the person/program/tool who wrote the file, to know :
  • When it has been seen and is being processed: inotify on linux for example doesn't catch all events, mmap(2) is explicitly not covered, so you could modify the file in such a way that it would never be refreshed by coredhcp - this point would be covered by your proposals the later comment
  • When it is finished processing (so the writer program can start a new update / not write the file while it is being read)

I'm not saying we need to address all of these right now! it's probably fine to select a few points we cannot/don't want to handle and explicitly call them out in the documentation.

Traditionally, unix programs have been using SIGHUP to reload this kind of configuration and/or files, and we could probably consider something like that, although that would be a larger project and probably not in the scope of this PR if we want to do it properly

I think I'll need more time to make up my own mind about what I think we should be doing here, but please give it a thought and share your own conclusions as well

Any chance of getting this merge? dhcplb by facebook also uses fsnotify to get reload files on changes. Code is here: https://github.com/facebookincubator/dhcplb/blob/2e66b270a1a257e37b07fdbc72b719914fe5ab5c/lib/filesourcer.go

@Natolumin I understand the issues when a file is appended to instead of replaced, but replacing the entire file would be my exact use case and we could specify this in the docs.

I've been thinking about extending the Plugin interface to include a Reload/Refresh function that is triggered on SIGHUP. This would specifically not reload the configuration/interfaces bound/loaded plugins of the server itself, but it could be very helpful in this case. If you would like to see a proposal for that as well/instead, I'm happy to send in another PR. This would fit my use case fine as well, so I'm fine with either solution.

In #143 I showcase how I would like to implement refreshing plugins on SIGHUP. Note that it is far from done, first wanted to get your opinions. If we would go for such an approach, I think it is far more flexible and this PR would be obsolete.

I understand the issues when a file is appended to instead of replaced, but replacing the entire file would be my exact use case and we could specify this in the docs.

I got to the same conclusion, that this is mostly fine and will work for most usecases.
Having this kind of autorefresh is worthwhile in any case (even if we have a lightweight sighup we may not want to reload the entire server everytime), but maybe gated behind a config option, it would be too surprising if it happens automatically by default
I'm ready to merge this PR if we add an option to the setup string (I was thinking "autorefresh" but if you have better ideas for wording that also works) to enable this behavior

I'm fine with that @Natolumin, thanks for considering this PR!

sorry for the long wait, this looks all good :)

Thanks @Natolumin !