Open Stack

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/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L231
[2] https://review.opendev.org/c/openstack/cinder/+/835553
[3]
https://github.com/openstack/cinder/blob/04da59177d97e5f57cb5000f2cfe21d86221139a/cinder/scheduler/evaluator/evaluator.py#L260-L271
[4] https://launchpad.net/cinder

On Mon, Mar 28, 2022 at 8:15 PM bartosz.rabiega at ovhcloud.com <
bartosz.rabiega at 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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstack.org/pipermail/openstack-discuss/attachments/20220329/d5164f62/attachment-0001.htm>