Hi Bartos,

I checked the current evaluator code and you're correct that it exceeds the recursion depth for complex expressions. Given your script, I was able to confirm that I'm hitting the same recursion depths as well.

Packrat[1] is an optimization (pyparsing.ParserElement.enablePackrat()) that uses memoization to reduce recursion depths but 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 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.

Second approach which I tried was to use pyparsing_common instead of defining expressions in a custom way (as pyparsing already has them optimized) which reduced the recursion depth by a measure of "10" with just redefining two variables. 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.

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.

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.

[1] https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d8622... [2] https://review.opendev.org/c/openstack/cinder/+/835553 [3] https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d8622... [4] https://launchpad.net/cinder

On Mon, Mar 28, 2022 at 8:15 PM bartosz.rabiega@ovhcloud.com < bartosz.rabiega@ovhcloud.com> wrote:

Hello,

I wanted to share something I've recently discovered - there is a strange issue with cinder evaluating complex configuration formulas.

For example when using complex goodness_function it's quite easy to hit: ERROR cinder.scheduler.weights.goodness maximum recursion depth exceeded: RecursionError: maximum recursion depth exceeded

I did some tests to find the root cause. It seems that pyparsing module does deep recursive parsing quite a lot (default recursion limit is 1000).

Not sure how to solve it but it would be nice to at least have a warning in the docs. Results and methodology below.

Tests were done using following code snippet

from cinder.scheduler.evaluator.evaluator import evaluate import sys

formulas = [ "1", "1 + 1", "max(1, 2, 3)", "max(1 + (10 / 20), 2, 3)", "(1 + max(1 + (10 / 20), 2, 3)) / 100", "((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb", "(((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) / 100", ]

stats = { 'allocated_capacity_gb': 50000, 'total_capacity_gb': 60000, 'free_capacity_gb': 10000, }

for f in formulas: for i in range(10, 2000, 10): sys.setrecursionlimit(i) try: res = evaluate(f, stats=stats) print(f"{i}: {f}") break except RecursionError: pass

Test results

Cinder 14.3.1, pyparsing<3.0 210: 1 210: 1 + 1 450: max(1, 2, 3) 600: max(1 + (10 / 20), 2, 3) 790: (1 + max(1 + (10 / 20), 2, 3)) / 100 980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb 1160: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) / 100

Cinder 16.4.2, pyparsing<3.0 220: 1 220: 1 + 1 450: max(1, 2, 3) 610: max(1 + (10 / 20), 2, 3) 790: (1 + max(1 + (10 / 20), 2, 3)) / 100 980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb 1170: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) / 100

Cinder 19.1.0, pyparsing<3.0 220: 1 220: 1 + 1 450: max(1, 2, 3) 610: max(1 + (10 / 20), 2, 3) 790: (1 + max(1 + (10 / 20), 2, 3)) / 100 980: ((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb 1170: (((1 + max(1 + (10 / 20), 2, 3)) / 100) + stats.total_capacity_gb) / 100

BR