Bug 13170 - unbound-dhcp-leases-bridge : stale settings cache
Summary: unbound-dhcp-leases-bridge : stale settings cache
Status: CLOSED FIXED
Alias: None
Product: IPFire
Classification: Unclassified
Component: --- (show other bugs)
Version: 2
Hardware: x86_64 Unspecified
: - Unknown - Minor Usability
Assignee: Michael Tremer
QA Contact:
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2023-07-05 15:05 UTC by bitbanger
Modified: 2024-04-08 17:17 UTC (History)
2 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description bitbanger 2023-07-05 15:05:51 UTC
Affects: IPFire 2.27 core update 167 and later

In commit 091cb62 of 2022-04-04 an error was introduced into unbound-dhcp-leases-bridge, function read_settings:

@functools.cache property

Function read_settings returns a collection of variables. With the same argument to the function, the same collection of variables is returned. So far, so good. The error is that the VALUE of one or more of the variables may have changed after the collection of variables was cached, i.e., the cache may have become stale.

Example: The administrator changes the DHCP domain (“Domain name suffix”) in the IPFire web GUI.

Fix: Remove the “@functools.cache” property from the definition of function read_settings.
Comment 1 Michael Tremer 2023-07-05 16:12:21 UTC
Hmm, this does not seem to be very trivial.

Removing the cached attribute would result in us re-reading the configuration file once we process an entry (which can easily be hundreds) because usually we read the domain name.

I considered the process to read the configuration once and requiring a restart after anything has changed. That seems to be however not be happening.

I would prefer to go the restart route as it is quite unlikely for the configuration to change. In that case, we would also have to restart unbound as it would have all old entries cached and we won't be able to find the old ones.
Comment 2 bitbanger 2023-07-06 13:23:29 UTC
Here's something else to consider:

unbound-dhcp-leases-bridge reads a maximum of 4 settings values but has to read 3 settings files to get them. If those 3 or 4 settings were duplicated into a single file stored on a memory filesystem, there would be only 1/3 as many file read operations and each read operation would be faster.

The disadvantage to this is that it creates a maintenance problem if, in the future, any additional settings become required by unbound-dhcp-leases-bridge. The additional settings would have to be duplicated into the consolidated file as well and this could easily be overlooked, especially if a different programmer were making the changes. (I think it unlikely that additional settings would become required, but who knows?)

Before making a decision I suggest you estimate the number of file read operations per minute for whatever you would consider to be a large number of hosts on IPFire local networks, factoring in lease time and renewal at 50% of lease time.

I assume that when you talk about restarting you mean restarting dhcp and unbound, not restarting IPFire. Yes, a domain name change would require such a restart.
Comment 3 Michael Tremer 2023-07-10 18:10:45 UTC
(In reply to bitbanger from comment #2)
> Here's something else to consider:
> 
> unbound-dhcp-leases-bridge reads a maximum of 4 settings values but has to
> read 3 settings files to get them. If those 3 or 4 settings were duplicated
> into a single file stored on a memory filesystem, there would be only 1/3 as
> many file read operations and each read operation would be faster.

Yes, this is the status of IPFire 2. All of that is going to be rewritten, because those text files are not a very good thing these days.

> The disadvantage to this is that it creates a maintenance problem if, in the
> future, any additional settings become required by
> unbound-dhcp-leases-bridge. The additional settings would have to be
> duplicated into the consolidated file as well and this could easily be
> overlooked, especially if a different programmer were making the changes. (I
> think it unlikely that additional settings would become required, but who
> knows?)

I don't think this solution is getting much development any more. It is fundamentally flawed and just a "best effort" kind of thing.

The ISC DHCP server does not have any interface to share any leases that have been handed out and it is now EOL. The bridge will go with it and we will need to replace this soon with something else that has better integration.

> Before making a decision I suggest you estimate the number of file read
> operations per minute for whatever you would consider to be a large number
> of hosts on IPFire local networks, factoring in lease time and renewal at
> 50% of lease time.

I maintain installations where there are multiple leases being handed out per second. So the bridge can almost occupy one very powerful CPU core. Re-reading the configuration files is not an option.

> I assume that when you talk about restarting you mean restarting dhcp and
> unbound, not restarting IPFire. Yes, a domain name change would require such
> a restart.

Yes, I did check and when the DHCP configuration is being changed, the bridge is being restarted. So this should work just fine.

What doesn't, and what is presumably giving you this problem is, that we don't clear any data from Unbound when the bridge goes down. I do have a solution for that though as I have a couple of other problems to solve here, and this bug is a small by-product I would say.

Previously unbound could not reload its own configuration, but only restart and therefore would loose the cache. Now, it can reload its configuration without losing its cache which means that we no longer need to be funny with all those commands and just write the leases to a file and ask Unbound to re-read that:

> https://git.ipfire.org/?p=people/ms/ipfire-2.x.git;a=commitdiff;h=5409e647c16cb643261d3e711dd1f0f6b09671b5

That way, this will all be less code, more straight forward, and killing a few birds with one stone.
Comment 4 Michael Tremer 2024-04-08 17:17:23 UTC
This has been shipped a little while ago.