<div dir="ltr"><div dir="ltr">Hi Bartos,</div><div><br></div><div>I checked the current evaluator code and you're correct that it exceeds the recursion depth for complex expressions.</div><div>Given your script, I was able to confirm that I'm hitting the same recursion depths as well.</div><div><br></div><div>Packrat[1] is an optimization (pyparsing.ParserElement.enablePackrat()) that uses memoization to reduce recursion depths but</div><div>that is already enabled in the evaluator (if not for it, simpler evaluations like "max(1, 2, 3)" wouldn't even return in 1000 recursion</div><div>depth and would fail). It has a cache size of 128 by default which I tried tweaking to make it unbounded but that didn't help.</div><div><br></div><div>Second approach which I tried was to use pyparsing_common instead of defining expressions in a custom way (as pyparsing already</div><div>has them optimized) which reduced the recursion depth by a measure of "10" with just redefining two variables.</div><div>I've proposed a patch for the same here[2] which is a minor change and thanks to your script, I was able to verify this improvement.</div><div><br></div><div>The main problem I see is with the infixNotation we use[3] as we're passing a lot of expressions to it which is known to reduce performance.</div><div><br></div><div>Thanks for raising the concern and feel free to open a bug at launchpad[4] and we can discuss this issue at our bug squad meeting.</div><div><br></div><div>[1] <a href="https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L231">https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L231</a></div><div>[2] <a href="https://review.opendev.org/c/openstack/cinder/+/835553">https://review.opendev.org/c/openstack/cinder/+/835553</a></div><div>[3] <a href="https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L260-L271">https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L260-L271</a></div><div>[4] <a href="https://launchpad.net/cinder">https://launchpad.net/cinder</a></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Mar 28, 2022 at 8:15 PM <a href="mailto:bartosz.rabiega@ovhcloud.com">bartosz.rabiega@ovhcloud.com</a> <<a href="mailto:bartosz.rabiega@ovhcloud.com">bartosz.rabiega@ovhcloud.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello,<br>
<br>
I wanted to share something I've recently discovered - there is a <br>
strange issue with cinder evaluating complex configuration formulas.<br>
<br>
For example when using complex goodness_function it's quite easy to hit:<br>
ERROR cinder.scheduler.weights.goodness maximum recursion depth <br>
exceeded: RecursionError: maximum recursion depth exceeded<br>
<br>
I did some tests to find the root cause. It seems that pyparsing module <br>
does deep recursive parsing quite a lot (default recursion limit is 1000).<br>
<br>
Not sure how to solve it but it would be nice to at least have a warning <br>
in the docs.<br>
Results and methodology below.<br>
<br>
Tests were done using following code snippet<br>
-------------------------------------------<br>
from cinder.scheduler.evaluator.evaluator import evaluate<br>
import sys<br>
<br>
formulas = [<br>
"1",<br>
"1 + 1",<br>
"max(1, 2, 3)",<br>
"max(1 + (10 / 20), 2, 3)",<br>
"(1 + max(1 + (10 / 20), 2, 3)) / 100",<br>
"((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb",<br>
"(((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) <br>
/ 100",<br>
]<br>
<br>
stats = {<br>
'allocated_capacity_gb': 50000,<br>
'total_capacity_gb': 60000,<br>
'free_capacity_gb': 10000,<br>
}<br>
<br>
for f in formulas:<br>
for i in range(10, 2000, 10):<br>
sys.setrecursionlimit(i)<br>
try:<br>
res = evaluate(f, stats=stats)<br>
print(f"{i}: {f}")<br>
break<br>
except RecursionError:<br>
pass<br>
<br>
<br>
Test results<br>
--------------------------------<br>
Cinder 14.3.1, pyparsing<3.0<br>
210: 1<br>
210: 1 + 1<br>
450: max(1, 2, 3)<br>
600: max(1 + (10 / 20), 2, 3)<br>
790: (1 + max(1 + (10 / 20), 2, 3)) / 100<br>
980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb<br>
1160: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) <br>
/ 100<br>
<br>
Cinder 16.4.2, pyparsing<3.0<br>
220: 1<br>
220: 1 + 1<br>
450: max(1, 2, 3)<br>
610: max(1 + (10 / 20), 2, 3)<br>
790: (1 + max(1 + (10 / 20), 2, 3)) / 100<br>
980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb<br>
1170: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) <br>
/ 100<br>
<br>
Cinder 19.1.0, pyparsing<3.0<br>
220: 1<br>
220: 1 + 1<br>
450: max(1, 2, 3)<br>
610: max(1 + (10 / 20), 2, 3)<br>
790: (1 + max(1 + (10 / 20), 2, 3)) / 100<br>
980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb<br>
1170: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) <br>
/ 100<br>
<br>
<br>
BR<br>
<br>
</blockquote></div></div>