anderseknert
commented
1 month ago Thanks for that detailed report! A contribution would be most welcome 😃
Open amirsalarsafaei opened 1 month ago
anderseknert
commented
1 month ago Thanks for that detailed report! A contribution would be most welcome 😃
ashutosh-narkar
commented
1 month ago I'd be open to work on a PR and a solution I actually have done one on opa-envoy-plugin and would like to contribute to the community:)
Awesome!
Making the cache size configurable may not be ideal. You could do it via the context but does not seem correct.
@johanfylling may know the reason for setting the default to 100 but we could just increase that.
Currently we remove only one value from the cache, may be that can be improved.
amirsalarsafaei
commented
1 month ago @ashutosh-narkar setting the default to any number may not be ideal, because the default rule or bundle static data maybe beyond that, and that can cause the same issue
ashutosh-narkar
commented
1 month ago setting the default to any number may not be ideal, because the default rule or bundle static data maybe beyond that, and that can cause the same issue
Sure. The thing we probably need to understand is what is the norm here. Whatever value is selected there are always going to be some cases where we see performance issues. The question is then if that's an edge case, which is not easy to figure out. Since we cannot make the value configurable, looking into improving caching behavior would be a possible solution. WDYT @johanfylling?
johanfylling
commented
1 month ago Setting the cache size to 100 was made with the assumption that a large set of regex patterns is very uncommon, and that slow performance is a better tradeoff than OOM.
In the original OOM fix, we opted to not use a more sophisticated eviction strategy than to simply remove a random entry from the cache. An option could be to revisit this, and instead evict maybe the least used entry in the cache (just as an example). Anything complicated here that require us to scan or reorder the cache would of course come with it's own performance penalty. But from your issue description, it sounds like the number of patterns might still be too large for this to be a viable solution.
An argument for making the cache size configurable through config-file/env-var would be that whomever is launching OPA is likely also the person/team to know the memory limitations of the host machine.
amirsalarsafaei
commented
1 month ago What about re-using the caching package? It has configurable cache size limit in bytes which is better than a counter.
amirsalarsafaei
commented
1 month ago Hi again, I read a little about glob library, maybe we should change our view of the matter at hand. Library The readme states that “This library is created for compile-once patterns”, if someone needs to match dynamic patterns (based on input or changing policy) they should use regex. Maybe we should warn users of the OOM and set a large number for the cap. To give you some perspective of why 100 is not enough for a glob, imagine having more than 1000 rest endpoints which may or may not contain some * as part of their path due to restful principles. Glob is the obvious choice over regex in this case. The endpoints rarely change without a bundle/policy change.
The other solution that I was thinking about was taking another argument from the user in the function glob.match called persist, it’s disabled by default but if the user enables it the compiled glob will always get persisted in another map no matter what, without a cap! This brings back the OOM problem ,but with good naming and documentation, we can mitigate it.
@johanfylling
johanfylling
commented
1 month ago Raising the glob cache size to somewhere in the thousands should be benign. What would be a suitable limit for your use case @amirsalarsafaei?
We still might want this to be configurable, though 🤔.
amirsalarsafaei
commented
1 month ago @johanfylling I actually took a shortcut by creating a template file and replacing glob match with some thing like this
input.parsed_path == ["v1", "users", input.parsed_path[2]]input.parsed_path is path split by /, however being involved with glob.match makes me think that this change isn’t right.
There should be a difference between regex and glob. Glob is slower at compilation so setting a 100 cap would hold it back. But I think setting the cap to 1000 or 5000 would be a good start but wouldn’t address the issue completely.
johanfylling
commented
3 weeks ago @amirsalarsafaei, for paths that don't require mid-point ** globs, I think your solution looks very idiomatic for Regal :).
johanfylling
commented
3 weeks ago Raising the cache limit too far would bring us back to the memory issue previously solved. I think we should aim for a solution that accommodates most scenarios without being destructive. OOM being a halting issue, and cache overflow "just" being a performance issue, I'm inclined to favor the OOM issue.
I suggest we move the glob cache to the BuiltinContext, and add a configuration option to allow admins to set the cache size. This way, we can keep the size as-is, but allow users to modify it (or even make it unbound) if cache performance becomes a concern.
As extra credits, we could even add something like a glob_cache_usage/glob_cache_full metric and/or log messages for when the cache is full to inform users that they've reached a performance-degrading situation that can be affected through conf.
Short description
upgrading opa from version v0.66.0 to v0.67.0 can cause performance hits due to the solution in #6846. imagine this policy:
I have a fixed large number of path so memory leak isn't a concern here but the cap causes re calculation of the regex pattern and in result a significant performance loss. I was able to achieve 25-50% better latency in 99.99% percentile increasing the cap size to my data size (benchmarks are attached at the end).
I can think of 2 possible solutions:
I'd be open to work on a PR and a solution I actually have done one on opa-envoy-plugin and would like to contribute to the community:)
Benchmark Results:
command:
bench --bundle bundle-test.tar.gz --input input.json 'data.smth.authz.allow' --e2e --count 10*the bundle is created with optimization level of 0 because using optimization caused more than 100x latency but that's another issue that I'm investigating:D
Opa v0.67.0:
Modified Version: