Caching: add global cache based on Redis (experimental) #1201

ghost · 2023-02-24T14:54:13Z

Purpose

Improve our cache implementation to make room to integrate other caching solutions. Provide an optional and experimental global cache layer based on Redis. This can be handy when multiple Zonemaster instances run concurrently (when dealing with batches for instance).

Context

https://gitlab.rd.nic.fr/zonemaster/zonemaster-engine/-/commit/b96181e460b5824dcd4ac913a3210c458970126a
and
https://gitlab.rd.nic.fr/zonemaster/zonemaster-engine/-/commit/0073398f8a4c47bb6f7f7d8dae8f8a2ddd2694a6

Credits to @blacksponge for the initial proposal

Changes

make Cache.pm an interface
move old Cache.pm logic into a LocalCache module
new RedisCache module (loaded only if configured in profile.json).

How to test this PR

install Redis and the following perl modules: Redis and Data::MessagePack. For Debian, this can be done with:
```
apt install redis libredis-perl libdata-messagepack-perl
```
start Redis service
modify the profile.json file to specify the Redis server (see share/profile_additional_properties.json) :
```
...
"cache": {
  "redis": {
     "server": "127.0.0.1:6379",
     "expire": 60
   }
},
...
```
run the tests or run Zonemaster on a zone to test
check that there is data stored in Redis
```
redis-cli KEYS 'ns*'
```

matsduf · 2023-02-26T22:23:22Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

+my $redis;
+our $object_cache = \%Zonemaster::Engine::Nameserver::Cache::object_cache;
+
+my $REDIS_EXPIRE = 3600; # seconds


Does that mean that TTL in the DNS record is ignored?

With this implementation yes. Rethinking about it, it might indeed be a good idea to set an expiry time to min(TTL, REDIS_EXPIRE). I've improved the logic to use the resource record TTL if available.

I do not understand with "if available". TTL is always set in a DNS record.

It is legal to use a shorter TLL than set in DNS, but what is the reason for for having such a limit?

If there is such a limit it should be configurable (but such configuration could be added later).

matsduf · 2023-03-07T09:13:13Z

@pnax, what are your plans for this?

matsduf · 2023-04-05T06:49:23Z

How to test this PR

Don't you have to start the redis service too?

matsduf

Documentation of how to run Redis is needed too.

matsduf · 2023-04-05T06:38:22Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

+sub set_key {
+    my ( $self, $hash, $packet ) = @_;
+    my $key = "ns:" . $self->address . ":" . $hash;
+
+    my $ttl = $REDIS_EXPIRE;
+
+    $self->data->{$hash} = $packet;
+    if ( defined $packet ) {
+        my $msg = $mp->pack({
+            data       => $packet->data,
+            answerfrom => $packet->answerfrom,
+            timestamp  => $packet->timestamp,
+            querytime  => $packet->querytime,
+        });
+        if ( $packet->answer ) {
+            my @rr = $packet->answer;
+            $ttl = min( map { $_->ttl } @rr );
+            $ttl = $ttl < $REDIS_EXPIRE ? $ttl : $REDIS_EXPIRE;
+        }
+        $self->redis->set( $key, $msg, 'EX', $ttl );
+    } else {
+        $self->redis->set( $key, '', 'EX', $ttl );
+    }
+}


If I understand the code correctly, the TTL will always be REDIS_EXPIRE if there is no response. If REDIS_EXPIRE is configuratble that seems reasonable.

If there is a response then the lowest TTL of the DNS records in the answer section, if any, will be used for TTL of that response. That seems reasonable.

If the answer section is empty it could be an NXDOMAIN or NODATA response, and then it would be reasonable to respect the TTL of the response by using the TTL of the SOA record in the authority section, which is the way of signaling the TTL for such responses.

If the answer section is empty and the response is a referral then the TTL of the NS RRset in the authority section would be reasonable to use.

If the RCODE is neither NOERROR nor NXDOMAIN then using REDIS_EXPIRE could be reasonable.

I've updated the logic to take your suggestion in consideration.

matsduf · 2023-04-05T06:57:03Z

share/profile_additional_properties.json

+    "redis": {
+        "server": "127.0.0.1:6379"
+    },


It seems like such configuration should rather be in the application (Backend, CLI) rather than in the library (Engine). I guess the model requires this to be in Engine. A positive side is that the global cache can be used for multiple executions of CLI.

ghost · 2023-04-05T13:15:54Z

Todo:

mark this caching layer as experimental
improve TTL usage

matsduf · 2023-04-14T14:53:37Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

+            if ( $rcode eq 'NXDOMAIN' ) {
+                $ttl = min( map { $_->minimum } @rr );
+            }
+            elsif ( $rcode eq 'NOERROR' ) {
+                $ttl = min( map { $_->ttl } @rr );
+            }


Negative caching is set to be the TTL value of SOA record (not the minimum field in the SOA RDATA) in the authority section.

If there are any NSEC or RRSIG records in the authority section, strictly they should be disregarded. Only the SOA record should be considered.

ghost · 2023-09-25T13:20:55Z

I've rebased this work on top of develop
I've addressed the remark on negative caching and using the TTL value of the SOA record instead of the minimum field

Please re-review.

lib/Zonemaster/Engine/Nameserver/Cache.pm

marc-vanderwal · 2023-11-03T13:00:00Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

+eval( <<EOS
+    use Data::MessagePack;
+    use Redis;
+EOS
+);


Have you tried eval with a block instead of a string?

Suggested change

eval( <<EOS

use Data::MessagePack;

use Redis;

EOS

);

eval {

use Data::MessagePack;

use Redis;

};

Suggestion applied.

marc-vanderwal · 2023-11-03T13:00:52Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

+
+
+if ( $@ ) {
+    die "Cant' use the Redis cache. Make sure the Data::MessagePack and Redis module are installed.\n";


Suggested change

die "Cant' use the Redis cache. Make sure the Data::MessagePack and Redis module are installed.\n";

die "Can't use the Redis cache. Make sure the Data::MessagePack and Redis modules are installed.\n";

marc-vanderwal · 2023-11-03T15:16:19Z

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

 our $object_cache = \%Zonemaster::Engine::Nameserver::Cache::object_cache;

-my $REDIS_EXPIRE = 3600; # seconds
+my $REDIS_EXPIRE = 5; # seconds


Why no longer 3600 seconds? Besides, it isn’t sufficiently clear from the variable’s name that it’s merely a default value.

Why no longer 3600 seconds?

I recall it being discussed during the F2F. @matsduf wanted a lower value. We agreed on 5 seconds. This is also the value suggested in the additional profile.

tgreenx

Nice work.

I think it would be good to have documentation of the available cache engines, e.g. in lib/Zonemaster/Engine/Profile.pm, even if its in experimental state.
The unitary tests of t/profiles.t should be updated as well for the new profile key. By the way, for future-proof reasons, what do you think of making that part generic? E.g:

    "cache" : {
        "name" : "redis",
        "server": "127.0.0.1:6379",
        "expire": 5
        }

lib/Zonemaster/Engine/Nameserver/Cache/LocalCache.pm

lib/Zonemaster/Engine/Nameserver/Cache/RedisCache.pm

lib/Zonemaster/Engine/Nameserver/Cache.pm

ghost · 2023-11-08T12:35:38Z

@tgreenx I should have addressed all your comments/suggestions. Good idea to have a generic entry in the profile for the cache layer.

tgreenx

@tgreenx I should have addressed all your comments/suggestions. Good idea to have a generic entry in the profile for the cache layer.

Thanks. I have expanded it a bit further, see commit efb6118. Let me know what you think, otherwise LGTM.

Make room for other caching solution. Move default cache into new LocalCache module.

Require Perl modules: Redis and Data::MessagePack

* use an eval block * fix a typo * make it clear variable is a default value

hannaeko

I have tested it and have no comment, it seems to be working correctly.

hannaeko · 2023-11-09T15:49:03Z

Can you edit the description so that the profile section uses the new cache section ?

ghost · 2023-11-09T16:29:07Z

Can you edit the description so that the profile section uses the new cache section ?

Done.

tgreenx

@pnax @blacksponge please re-review. I just added a commit (08f88cd) which adds testing for the provided subkeys of profile entry cache.redis.

matsduf · 2023-11-27T12:55:35Z

We need the documentation on how to use this new feature to be available. We also want the users to see that it exists. I suggest that a short section is included in the installation instructions for Zonemaster-Engine, at the end of that document as an experimental feature. As in "How to test this PR" in this PR it can be limited to one OS and very brief. The same text can probably be used.

matsduf · 2023-11-27T14:01:00Z

I installed redis and the other packages on FreeBSD. Created an updated profile. Ran zonemaster-cli with the updated profile. But the redis cache is empty and rerunning the test does not speed it up. How could I check?

# redis-cli KEYS 'ns*'
(empty array)

# redis-cli KEYS '*'
(empty array)

After discussion it appears 5 seconds is to low when testing this feature.

matsduf · 2023-12-01T12:50:24Z

I installed redis and the other packages on FreeBSD. Created an updated profile. Ran zonemaster-cli with the updated profile. But the redis cache is empty and rerunning the test does not speed it up. How could I check?
# redis-cli KEYS 'ns*'
(empty array)

# redis-cli KEYS '*'
(empty array)

With the new default, 300 instead of 5 seconds, I get the expected result. Checking "ripe.net" took 50 seconds first time and less than 10 seconds the second time. The redis-cli command above also gave a non-empty result.

matsduf · 2024-03-17T14:22:27Z

Release testing: A bug was found. Described in #1326 and a solution is provided in #1327. Else it looks fine.

ghost added this to the v2023.1 milestone Feb 24, 2023

ghost requested a review from hannaeko February 24, 2023 14:54

matsduf reviewed Feb 26, 2023

View reviewed changes

ghost marked this pull request as ready for review March 31, 2023 13:44

ghost added the V-Minor Versioning: The change gives an update of minor in version. label Mar 31, 2023

matsduf reviewed Apr 5, 2023

View reviewed changes

ghost changed the title ~~Caching: add global cache based on Redis~~ Caching: add global cache based on Redis (experimental) Apr 5, 2023

matsduf reviewed Apr 14, 2023

View reviewed changes

matsduf modified the milestones: v2023.1, v2023.2 May 2, 2023

ghost requested a review from matsduf September 25, 2023 13:20

matsduf reviewed Sep 25, 2023

View reviewed changes

lib/Zonemaster/Engine/Nameserver/Cache.pm Outdated Show resolved Hide resolved

matsduf requested review from mattias-p, tgreenx and marc-vanderwal November 2, 2023 14:55

marc-vanderwal reviewed Nov 3, 2023

View reviewed changes

tgreenx reviewed Nov 7, 2023

View reviewed changes

tgreenx previously approved these changes Nov 8, 2023

View reviewed changes

Alexandre Pion added 7 commits November 8, 2023 17:33

Use Class::Accessor instead of Moose

65ea249

Hide cache layer internals

3ab1742

Provide a caching interface

0bbe214

Make room for other caching solution. Move default cache into new LocalCache module.

New Redis global cache engine

e24f6e8

Require Perl modules: Redis and Data::MessagePack

Cache data based on TTL value

d731495

Add POD and experimental note

0c92c4f

Respect TTL with NXDOMAIN and NODATA

752cfd2

Alexandre Pion added 6 commits November 8, 2023 17:33

Clean up: remove unused routine

7746b7e

Update after review

3e7dd79

* use an eval block * fix a typo * make it clear variable is a default value

Apply suggestions from review

37c9572

Log cache hit or miss

3ce77b2

Use a generic "cache" entry in the profile

5b92142

Add profile unit test for "cache.redis" entry

1ac4951

tgreenx dismissed their stale review via 597941a November 8, 2023 16:38

tgreenx previously approved these changes Nov 8, 2023

View reviewed changes

Expand generic 'cache' entry in the profile, and its unit tests

efb6118

tgreenx dismissed their stale review via efb6118 November 8, 2023 17:25

tgreenx previously approved these changes Nov 8, 2023

View reviewed changes

hannaeko previously approved these changes Nov 9, 2023

View reviewed changes

Further expand generic 'cache' entry

08f88cd

tgreenx dismissed stale reviews from hannaeko and themself via 08f88cd November 13, 2023 10:45

tgreenx previously approved these changes Nov 13, 2023

View reviewed changes

Increase the minimum default expiration time

ae6934b

After discussion it appears 5 seconds is to low when testing this feature.

ghost dismissed tgreenx’s stale review via ae6934b November 28, 2023 13:59

tgreenx approved these changes Nov 28, 2023

View reviewed changes

ghost merged commit c69fc49 into zonemaster:develop Nov 30, 2023
3 checks passed

ghost deleted the caching branch November 30, 2023 11:11

matsduf mentioned this pull request Dec 1, 2023

Documentation of global cache zonemaster/zonemaster#1227

Merged

matsduf mentioned this pull request Dec 11, 2023

PRs for v2023.2 zonemaster/zonemaster#1232

Closed

matsduf added the S-ReleaseTested Status: The PR has been successfully tested in release testing label Mar 17, 2024

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Caching: add global cache based on Redis (experimental) #1201

Caching: add global cache based on Redis (experimental) #1201

ghost commented Feb 24, 2023 •

edited by ghost

Loading

matsduf Feb 26, 2023

ghost Mar 31, 2023

matsduf Mar 31, 2023

matsduf commented Mar 7, 2023

matsduf commented Apr 5, 2023

How to test this PR

matsduf left a comment

matsduf Apr 5, 2023

ghost Apr 5, 2023

matsduf Apr 5, 2023

ghost commented Apr 5, 2023 •

edited by ghost

Loading

matsduf Apr 14, 2023

ghost commented Sep 25, 2023

marc-vanderwal Nov 3, 2023

ghost Nov 6, 2023

marc-vanderwal Nov 3, 2023

ghost Nov 6, 2023

marc-vanderwal Nov 3, 2023

ghost Nov 6, 2023

tgreenx left a comment

ghost commented Nov 8, 2023

tgreenx left a comment •

edited

Loading

hannaeko left a comment •

edited

Loading

hannaeko commented Nov 9, 2023 •

edited

Loading

ghost commented Nov 9, 2023

tgreenx left a comment

matsduf commented Nov 27, 2023

matsduf commented Nov 27, 2023

matsduf commented Dec 1, 2023

matsduf commented Mar 17, 2024



		if ( $@ ) {
		die "Cant' use the Redis cache. Make sure the Data::MessagePack and Redis module are installed.\n";

	die "Cant' use the Redis cache. Make sure the Data::MessagePack and Redis module are installed.\n";
	die "Can't use the Redis cache. Make sure the Data::MessagePack and Redis modules are installed.\n";

Caching: add global cache based on Redis (experimental) #1201

Caching: add global cache based on Redis (experimental) #1201

Conversation

ghost commented Feb 24, 2023 • edited by ghost Loading

Purpose

Context

Changes

How to test this PR

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matsduf commented Mar 7, 2023

matsduf commented Apr 5, 2023

How to test this PR

matsduf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 5, 2023 • edited by ghost Loading

Choose a reason for hiding this comment

ghost commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgreenx left a comment

Choose a reason for hiding this comment

ghost commented Nov 8, 2023

tgreenx left a comment • edited Loading

Choose a reason for hiding this comment

hannaeko left a comment • edited Loading

Choose a reason for hiding this comment

hannaeko commented Nov 9, 2023 • edited Loading

ghost commented Nov 9, 2023

tgreenx left a comment

Choose a reason for hiding this comment

matsduf commented Nov 27, 2023

matsduf commented Nov 27, 2023

matsduf commented Dec 1, 2023

matsduf commented Mar 17, 2024

ghost commented Feb 24, 2023 •

edited by ghost

Loading

ghost commented Apr 5, 2023 •

edited by ghost

Loading

tgreenx left a comment •

edited

Loading

hannaeko left a comment •

edited

Loading

hannaeko commented Nov 9, 2023 •

edited

Loading