manageiq icon indicating copy to clipboard operation
manageiq copied to clipboard

[WIP] Miq expression interp

Open kbrock opened this issue 2 years ago • 8 comments

parent:

  • https://github.com/ManageIQ/manageiq/issues/22397

Made some progress

I wanted to see what our code looks like for preprocessing the whole tree up front. The code was parsing the same fields over and over for to_ruby. The thought is if to_ruby has all the data it needs, then an interpreter will have all the data it needs.

  • don't lookup fields multiple times in to_ruby but rather once in the preprocessor
  • don't lookup fields and validate in valid? but rather one time in the preprocessor
  • embed field objets in the expr tree itself.
  • drop col_details and just use the cached field value from the preprocessor

There are still a few places to remove more Field.parse calls, but seems good enough.


From here we want to transition focus on Condition.subst. This is where we spend the most of our time and create the most objects.

If we can verify that we don't use mode="tag_expr", then we can avoid the _to_ruby.gsub() { _subst}.gsub() { _subst } code and have the to_ruby produce the correct code in the first place.

@Fryguy do you know anything about mode="tag_expr" and if we would have customers still using that?

kbrock avatar Apr 12 '24 21:04 kbrock

@Fryguy do you know anything about mode="tag_expr" and if we would have customers still using that?

I did a little spelunking, and in 2008 @gtanzillo (Hi Greg!) introduced mode=tag_expr_v2 in 37762294131e0cc24e05adf13fa5bbc85fa19200 (pre-open-source), and replaced its usage in a bunch of seeded policies to use that. Below is an example diff of one of those changes where you can see the old format and the new format. Note that tag_expr_v2 is the MiqExpression format, as you can see in https://github.com/ManageIQ/manageiq/blob/master/app/models/condition.rb#L68 where it's used directly in MiqExpression.

Maybe @gtanzillo can remember anything here on this old format?

Even so, I'd be super-surprised if tag_expr still existed in any form for existing users/customers as 2008 was the big switch. git grep and org wide searches tell me this isn't used anymore and we can drop it.

@ vmdb/product/policies/env_prod_vm_host.yml:7 @ modifier: "allow"
 who: "system"
 what: "start"
 rule:
-    mode: "tag_expr"
-    expr: |-
-        <exist ref=host>/managed/environment/prod</exist> &&
-        <exist>/managed/environment/prod</exist>
+    mode: "tag_expr_v2"
+    expr:
+        and:
+            - exist:
+                field: Host.managed-environment
+                value: prod
+            - exist:
+                 field: Vm.managed-environment
+                 value: prod

Fryguy avatar Apr 12 '24 22:04 Fryguy

update:

  • rebase

kbrock avatar May 24 '24 19:05 kbrock

Checked commits https://github.com/kbrock/manageiq/compare/4150ce6482730999b8909fa2af9ca03905e34047~...ddf44703044041f55d6859e6552ebdcef19063ae with ruby 3.1.5, rubocop 1.56.3, haml-lint 0.51.0, and yamllint 3 files checked, 2 offenses detected

lib/miq_expression.rb

  • [ ] :exclamation: - Line 335, Col 11 - Style/HashTransformValues - Prefer transform_values over each_with_object.
  • [ ] :exclamation: - Line 356, Col 14 - Performance/CollectionLiteralInLoop - Avoid immutable Array literals in loops. It is better to extract it into a local variable or a constant.

miq-bot avatar Jul 16 '24 22:07 miq-bot