Prefetch syncs to redis #1446

kwitsch · 2024-04-12T19:31:46Z

Changes:

reloadCacheEntry removes EDNS0 records from response
reloadCacheEntry publishes its result to Redis

Closes #1422

Related #1420

codecov · 2024-04-12T19:35:58Z

Codecov Report

Attention: Patch coverage is 62.98077% with 77 lines in your changes are missing coverage. Please review.

Project coverage is 92.92%. Comparing base (fe84ab8) to head (7b2fcc2).
Report is 65 commits behind head on main.

Files	Patch %	Lines
util/dns.go	17.85%	44 Missing and 2 partials ⚠️
redis/redis.go	80.64%	9 Missing and 9 partials ⚠️
resolver/caching_resolver.go	76.78%	8 Missing and 5 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1446      +/-   ##
==========================================
- Coverage   93.88%   92.92%   -0.97%     
==========================================
  Files          78       79       +1     
  Lines        6361     5030    -1331     
==========================================
- Hits         5972     4674    -1298     
+ Misses        300      251      -49     
- Partials       89      105      +16

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ThinkChaos

I have a couple comments but it's mostly stuff I noticed about the existing code when comparing with the new one!

resolver/caching_resolver.go

ThinkChaos · 2024-04-12T20:57:04Z

resolver/caching_resolver.go

-	} else {
-		util.LogOnError(ctx, fmt.Sprintf("can't prefetch '%s' ", domainName), err)
+
+		if r.redisClient != nil {


The behavior here is slightly different from putInCache: if .Pack() failed, we're not sending the response to Redis. I think not sending it is better, so maybe update putInCache to match?

Ur correct sending it when pack fails is an error in putInCache

ThinkChaos · 2024-04-12T21:03:00Z

resolver/caching_resolver.go

+			r.redisClient.PublishCache(cacheKey, &res)
+		}
+
+		return &packed, r.adjustTTLs(response.Res.Answer)


This is also a slight difference compared to the putInCache flow: here Redis sees the unadjusted TTLs whereas when we add values using putInCache Redis gets the adjusted TTLs.
My initial reaction is this is correct, and the other code should change since other blocky instances might have different TTL configuration.

Sending the unadjusted TTL to Redis isn't a problem since received entries are TTL adjusted upon receiving before they are put in the local cache.

This way instances with different configs can share the same sync channel without breaking each other's settings.

That was the base idea when I implemented it. I get that this is counterintuitive and should be changed.

ThinkChaos · 2024-04-12T21:16:48Z

resolver/caching_resolver.go

+		// don't cache any EDNS OPT records
+		util.RemoveEdns0Record(respCopy)


Nice catch!

Based on my other comments and this being missing before, I think we should try and factorize the code to have a single func we can call from here and putInCache. Maybe something that adds the response to Redis, and returns the local cache TTL + the entry with adjusted TTL.

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

kwitsch · 2024-04-13T22:28:58Z

@ThinkChaos I started refactoring the whole caching_resolver.
May take a bit more time but I think there's some enhancement potential regarding code duplication and inconsistent behavior.

Thanks for the input. 👍

I'll let you know when I'm done.

kwitsch · 2024-04-19T22:15:33Z

@ThinkChaos still needs some tests but may I get your opinion on the current changes?
I'm a bit unsure if the direction I took was an advancement? 🤔

ThinkChaos · 2024-04-19T23:48:54Z

resolver/caching_resolver.go

+		r.redisClient.PublishCache(cacheKey, cacheCopy)
+	}
+
+	return &packed, util.ToTTLDuration(ttl)


I think the biggest cause of dupe code is that here we need to return the new cache entry,
but in r.Resolver we need to call r.resultCache.Put.
So we could change the prefetch API to be more like the normal caching flow:
PrefetchingOptions.ReloadFn func(ctx context.Context, key string) (*T, time.Duration)
could be something like OnAboutToExpire func(ctx context.Context, key string),
where the callback is expected to call Put if they want to update the value.
That way reloadCacheEntry could become a tiny wrapper around r.Resolve (or a new resolve method they both call).

From a quick check, ReloadFn is only used by this code (and the cache tests),
and Put allows replacing a value, so I think that should be doable!

From there I think most things will fall into place :)

Thanks for the input I think that was the part which caused me the most headaches. 😅

I also try to refactor the caches to streamline the resolver behavior. 👍

ThinkChaos · 2024-04-19T23:53:10Z

resolver/caching_resolver.go

+			ttl := r.modifyResponseTTL(response.Res)
+			if ttl > noCacheTTL {
+				cacheCopy := r.putInCache(logger, cacheKey, response)
+				if cacheCopy != nil && r.redisClient != nil {
+					r.redisClient.PublishCache(cacheKey, cacheCopy)
+				}
+			}


This still adjusts the TTL twice, but if we can get everything down to a single code path, it means putInCache can also publish to Redis, which will make this whole part simpler, and avoid needing to return a copy out etc. and just use it in a couple lines in a row.

Maybe wrap it in another function? 🤔

I tried removing the publish Boolean parameter since it seemed misplaced.
On the other hand also a method to publish without put is needed (prefetching)... 🥴
I'm a little troubled fitting these 3 requirements in one logical block.

On the other hand also a method to publish without put is needed (prefetching)... 🥴

Not sure if you saw my other comment, but I think we can make this go away without too much trouble.

Already commented on it. 👍

Sorry, read and replayed in reverse order. 🫣

ThinkChaos · 2024-04-19T23:55:49Z

I had to checkout the branch locally to browse with my editor cause it is indeed a lot of functions calling combined in a couple different ways so definitely hard to follow!

kwitsch · 2024-04-20T00:00:27Z

I had to checkout the branch locally to browse with my editor cause it is indeed a lot of functions calling combined in a couple different ways so definitely hard to follow!

Yeah I know, that's one of the reasons why I was a bit unsure about the direction. 🫣

kwitsch · 2024-04-20T21:29:52Z

Good thing I looked deeper into the caches since I found a remaining context.Background() : https://github.com/0xERR0R/blocky/blob/main/cache%2Fexpirationcache%2Fexpiration_cache.go#L129

kwitsch added the 🐞 bug Something isn't working label Apr 12, 2024

kwitsch added this to the v0.24 milestone Apr 12, 2024

kwitsch enabled auto-merge (squash) April 12, 2024 19:36

ThinkChaos reviewed Apr 12, 2024

View reviewed changes

kwitsch disabled auto-merge April 12, 2024 22:09

kwitsch and others added 3 commits April 13, 2024 00:16

reloadCacheEntry removes edns and publishes to redis

a411c5f

Update resolver/caching_resolver.go

6a89907

Co-authored-by: ThinkChaos <ThinkChaos@users.noreply.github.com>

added transformAndPublish

8607144

kwitsch force-pushed the issues/1422 branch from 5abeeee to 8607144 Compare April 13, 2024 00:10

kwitsch marked this pull request as draft April 13, 2024 00:52

kwitsch added 10 commits April 14, 2024 13:31

wip

43e59a4

dns util rework

5eb351f

fixed generics

0f6c847

linebreaks in doctag

27c60e7

refactoring

7f39983

added ToTTLDuration

66a7155

added fast returns to reduce loops

6de43c8

fast return and comments

46441ba

added dns unit tests

f28e58c

no fast return since adjustment is needed to set minttl

5dc3a16

kwitsch marked this pull request as ready for review April 19, 2024 21:53

ThinkChaos reviewed Apr 19, 2024

View reviewed changes

kwitsch marked this pull request as draft April 20, 2024 20:25

Merge branch 'main' into issues/1422

afbb305

kwitsch added 11 commits April 21, 2024 15:38

use correct context

37fbb42

redis refactoring 1

350b2fe

caching_resolver refactoring

423ed03

redis refactoring 2

4928399

fast return on zero or below

17aca2c

redis refactoring

05446fa

fix caching resolver test

c798689

nil check

f8093f3

new pointers

61c83af

removed const

73cbd9c

redis refactoring 2

7b2fcc2

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Prefetch syncs to redis #1446

Prefetch syncs to redis #1446

kwitsch commented Apr 12, 2024 •

edited

codecov bot commented Apr 12, 2024 •

edited

ThinkChaos left a comment

ThinkChaos Apr 12, 2024

kwitsch Apr 12, 2024

ThinkChaos Apr 12, 2024

kwitsch Apr 12, 2024

ThinkChaos Apr 12, 2024

kwitsch commented Apr 13, 2024 •

edited

kwitsch commented Apr 19, 2024

ThinkChaos Apr 19, 2024

kwitsch Apr 20, 2024

ThinkChaos Apr 19, 2024 •

edited

kwitsch Apr 20, 2024

ThinkChaos Apr 20, 2024

kwitsch Apr 20, 2024

ThinkChaos commented Apr 19, 2024

kwitsch commented Apr 20, 2024

kwitsch commented Apr 20, 2024

		// don't cache any EDNS OPT records
		util.RemoveEdns0Record(respCopy)

Prefetch syncs to redis #1446

Are you sure you want to change the base?

Prefetch syncs to redis #1446

Conversation

kwitsch commented Apr 12, 2024 • edited

codecov bot commented Apr 12, 2024 • edited

Codecov Report

ThinkChaos 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwitsch commented Apr 13, 2024 • edited

kwitsch commented Apr 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ThinkChaos Apr 19, 2024 • edited

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

ThinkChaos commented Apr 19, 2024

kwitsch commented Apr 20, 2024

kwitsch commented Apr 20, 2024

kwitsch commented Apr 12, 2024 •

edited

codecov bot commented Apr 12, 2024 •

edited

kwitsch commented Apr 13, 2024 •

edited

ThinkChaos Apr 19, 2024 •

edited