dnsproxy: Remove usage of regex lru #22584

odinuge · 2022-12-06T21:11:36Z

Since we now have a separate reference counted cache storing compiled regexes. This will both make more space for other values in the cache, as well as making the huge regexes used in the dnsproxy avaible to gc when they are no longer used.

Signed-off-by: Odin Ugedal ougedal@palantir.com

Fixes: #22241

dnsproxy: stop using the regex lru in the dns proxy to avoid keeping large unused regex in memory when no longer needed

odinuge · 2022-12-06T21:13:27Z

Ref. #22241 this will make all users able to utilize the benefits from #21288 when it comes to memory usage of unused regexes.

Not sure if we should remove all of it at once tho. Any thoughts @christarazi ?

pkg/fqdn/dnsproxy/proxy.go

christarazi · 2022-12-06T21:23:29Z

pkg/fqdn/dnsproxy/proxy.go

@@ -347,7 +347,7 @@ func (c regexCache) lookupOrCompileRegex(pattern string) (*regexp.Regexp, error)
 		entry.referenceCount += 1
 		return entry.regex, nil
 	}
-	regex, err := re.CompileRegex(pattern)
+	regex, err := regexp.Compile(pattern)


I think we can update the godoc on re.CompileRegex to say it's deprecated and not to use it.

Additionally, searching for all instances of re.CompileRegex and replacing them could work. Or alternatively, you could just make re.CompileRegex simply be a wrapper for regexp.Compile, that way you don't risk missing any paths.

We can then remove this code in the v1.14 release cycle after we've deprecated it properly here.

Finally back from 🌴 now, so ready to keep pushing these!

Do you have a preference here? I think a cache for the other usage of re.CompileRegex might be valuable to avoid compiling them all the time, but I don't have all the context on that.

On second thought, maybe we don't want to entirely remove the LRU.

The problem with caching regexes as we've learned is if we don't clean them up. In your fixes, you've attached a refcount to the regex in the DNS proxy. But there are other areas too, where we can attach cleanup logic.

The other areas for regex compilation is policy YAML validation, aka pkg/fqdn/matchpattern.Validate() and during each DNS request in the DNS proxy. I see value in caching the regex in these cases.

In the former case, we could mitigate the issue of keeping the regexes around by hooking into the policy-delete event and removing the names (which map to regexes) in the to-be-deleted policy from the cache. In the latter case, we can use an LRU. I feel an LRU is suitable in this case because the access pattern will be based on the DNS requests that the workloads are performing.

Should these cases share the same LRU? Well, that was kinda the idea behind the initial LRU implementation to begin with. I think as long as we manage the lifecycle and ensure we delete "stale" entries, then we will probably be fine regarding all the memory issues we've had in the past (and that you've helped fix 🙂 ).

jrajahalme · 2023-01-04T08:12:07Z

checkpatch fails due to a typo in the commit message:

  Warning: WARNING:TYPO_SPELLING: 'avaible' may be misspelled - perhaps 'available'?
  #8: 
  as well as making the huge regexes used in the dnsproxy avaible to gc

jrajahalme · 2023-01-04T08:13:59Z

/test

christarazi · 2023-01-04T18:26:03Z

Seems that the Jenkins tests failed due to what seems to be a rebasing issue. Will rebase and re-run.

christarazi · 2023-01-04T18:53:20Z

/test

github-actions · 2023-02-27T02:03:36Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

github-actions · 2023-04-06T01:52:49Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

github-actions · 2023-05-20T01:51:42Z

This pull request has been automatically marked as stale because it
has not had recent activity. It will be closed if no further activity
occurs. Thank you for your contributions.

odinuge · 2023-05-23T08:28:06Z

Forgot about this since I thought #23429 mitigated it, but seems like that only conflicted with one of the changes.

Pushed an updated version now

christarazi · 2023-05-23T23:06:56Z

CI pointing out more places to remove

 Error: pkg/fqdn/dnsproxy/proxy_test.go:197:9: undefined: re

christarazi

One nit on the commit title / msg, is that we are not entirely removing the usage of the LRU, but simply avoiding it in certain cases, see https://github.com/cilium/cilium/pull/22584/files#r1063772535. I think we should clarify that because it can certainly cause confusion for future readers. Maybe an additional code comment on the LRU and non-LRU code paths to explicitly state why a code path uses one or the other.

odinuge · 2023-05-24T19:02:42Z

Hmm. The commit message body is in the context of the title with the dnsproxy: prefix, so I think it makes sense. But yeah, I can rewrite it tomorrow to make it even more explicit if its a bit ambiguous.

Since we now have a separate reference counted cache storing compiled regexes, we no longer need the lru here. This will both make more space for other values in the cache, as well as making the huge regexes used in the dnsproxy available to gc when they are no longer used. The regex LRU is still being used elsewhere in the cilium codebase, where reusing compiled regexes makes sense, while reference counting is non-trivial. Signed-off-by: Odin Ugedal <ougedal@palantir.com> Signed-off-by: Odin Ugedal <odin@uged.al>

christarazi · 2023-06-06T22:16:07Z

/test

odinuge · 2023-06-09T14:32:59Z

Mind retesting this @christarazi ? Thanks

christarazi · 2023-06-09T22:02:36Z

/ci-e2e

christarazi · 2023-06-09T22:02:44Z

/ci-multicluster

maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Dec 6, 2022

github-actions bot added the kind/community-contribution This was a contribution made by a community member. label Dec 6, 2022

odinuge commented Dec 6, 2022

View reviewed changes

pkg/fqdn/dnsproxy/proxy.go Outdated Show resolved Hide resolved

odinuge marked this pull request as ready for review December 6, 2022 21:15

odinuge requested review from a team as code owners December 6, 2022 21:15

odinuge requested review from jibi and jrajahalme December 6, 2022 21:15

christarazi reviewed Dec 6, 2022

View reviewed changes

jrajahalme added the release-note/misc This PR makes changes that have no direct user impact. label Jan 4, 2023

maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jan 4, 2023

christarazi force-pushed the ou-disable-lru-dnsproxy branch from 7fdcb03 to 3ac9b0b Compare January 4, 2023 18:26

christarazi added kind/enhancement This would improve or streamline existing functionality. area/proxy Impacts proxy components, including DNS, Kafka, Envoy and/or XDS servers. labels Jan 4, 2023

jibi removed their request for review January 24, 2023 13:23

christarazi marked this pull request as draft January 25, 2023 19:37

github-actions bot added stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. and removed stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. labels Feb 27, 2023

christarazi added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Mar 6, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 6, 2023

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label Apr 19, 2023

github-actions bot added the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 20, 2023

odinuge force-pushed the ou-disable-lru-dnsproxy branch 2 times, most recently from f9950f6 to edf9613 Compare May 23, 2023 08:27

odinuge marked this pull request as ready for review May 23, 2023 08:28

odinuge force-pushed the ou-disable-lru-dnsproxy branch from edf9613 to 854769f Compare May 23, 2023 08:55

github-actions bot removed the stale The stale bot thinks this issue is old. Add "pinned" label to prevent this from becoming stale. label May 24, 2023

odinuge force-pushed the ou-disable-lru-dnsproxy branch 2 times, most recently from cf2e56f to 7138f62 Compare May 24, 2023 08:13

christarazi reviewed May 24, 2023

View reviewed changes

odinuge force-pushed the ou-disable-lru-dnsproxy branch from 7138f62 to f1f6ffe Compare June 2, 2023 17:47

christarazi approved these changes Jun 6, 2023

View reviewed changes

christarazi force-pushed the ou-disable-lru-dnsproxy branch from f1f6ffe to 11a9206 Compare June 6, 2023 22:15

christarazi removed the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Jun 6, 2023

jrajahalme approved these changes Jun 12, 2023

View reviewed changes

maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Jun 12, 2023

christarazi merged commit 3b258a3 into cilium:main Jun 12, 2023
61 of 62 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dnsproxy: Remove usage of regex lru #22584

dnsproxy: Remove usage of regex lru #22584

odinuge commented Dec 6, 2022 •

edited by christarazi

odinuge commented Dec 6, 2022

christarazi Dec 6, 2022

christarazi Dec 6, 2022

christarazi Dec 6, 2022

odinuge Jan 6, 2023

christarazi Jan 6, 2023 •

edited

jrajahalme commented Jan 4, 2023

jrajahalme commented Jan 4, 2023

christarazi commented Jan 4, 2023

christarazi commented Jan 4, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Apr 6, 2023

github-actions bot commented May 20, 2023

odinuge commented May 23, 2023

christarazi commented May 23, 2023

christarazi left a comment

odinuge commented May 24, 2023

christarazi commented Jun 6, 2023

odinuge commented Jun 9, 2023

christarazi commented Jun 9, 2023

christarazi commented Jun 9, 2023

dnsproxy: Remove usage of regex lru #22584

dnsproxy: Remove usage of regex lru #22584

Conversation

odinuge commented Dec 6, 2022 • edited by christarazi

odinuge commented Dec 6, 2022

christarazi Dec 6, 2022

Choose a reason for hiding this comment

christarazi Dec 6, 2022

Choose a reason for hiding this comment

christarazi Dec 6, 2022

Choose a reason for hiding this comment

odinuge Jan 6, 2023

Choose a reason for hiding this comment

christarazi Jan 6, 2023 • edited

Choose a reason for hiding this comment

jrajahalme commented Jan 4, 2023

jrajahalme commented Jan 4, 2023

christarazi commented Jan 4, 2023

christarazi commented Jan 4, 2023

github-actions bot commented Feb 27, 2023

github-actions bot commented Apr 6, 2023

github-actions bot commented May 20, 2023

odinuge commented May 23, 2023

christarazi commented May 23, 2023

christarazi left a comment

Choose a reason for hiding this comment

odinuge commented May 24, 2023

christarazi commented Jun 6, 2023

odinuge commented Jun 9, 2023

christarazi commented Jun 9, 2023

christarazi commented Jun 9, 2023

odinuge commented Dec 6, 2022 •

edited by christarazi

christarazi Jan 6, 2023 •

edited