Exclude Service::AGGREGATE_ALL_VM_ATTRS from MiqExp.to_sql

[NickLaMuro]

These calls, while they are Arel and could be converted, would most likely be used in "filter" type scenarios, and would not be good candidates for that since they would perform horribly when used on more than one record at a time (which filtering would do that).

Instead of preventing these records from showing up as a filter option, just make sure they can't be used with SQL when they are. That said, they will cause N+1 hell, so they really shouldn't be filtered on... but...

¯\(°_o)/¯

Alternative solution explanation

This is one possible solution for this BZ. The other would be wrapping the arel_attribute values in the MiqExpression#to_arel method with a Arel::Nodes::Grouping call, which would allow eq calls to be done on these, and the other arel-based MiqExpression operators. This solution was proposed first because of the concerns raised in #11502 which add the Service::AGGREGATE_ALL_VM_ATTRS virtual_attributes:

These are DEFINITELY not queries that you want to run in a index type fashion, as they will not scale. This is because the nested selects will individually run for each service record returned. In this case, it will end up in a tying up a process, so these methods need to be used with care.

As mentioned above, this means if a user does decide to filter based on these columns, it will most likely be slow, but it will be a bunch of short queries to the DB instead of a single massive and crippling one. Ideally these filters just shouldn't be used for filtering Services.

Links

• https://bugzilla.redhat.com/show_bug.cgi?id=1539710
• Update Service Aggregation Methods to use SQL #11502
• 251de4e

Steps for Testing/QA

I will add some tests in another commit/rebased commit, but for now, you can test with that the following doesn't raise an error in a bin/rails c:

irb> MiqExpression.new("=" => {"field" => "Service-aggregate_all_vm_cpus", "value" => "454"}).to_sql
#=> [nil, nil, {:supported_by_sql=>false}]

[NickLaMuro]

cc/ @imtayadeway @kbrock

[miq-bot]

Checked commits NickLaMuro/manageiq@e7955bf~...0d32b87 with ruby 2.3.3, rubocop 0.52.0, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. ⭐

[NickLaMuro]

@gtanzillo Seeing as this was originally targeted for a 5.9 BZ, should we be backporting this to Gaprindashvili? Also, I have confirmed that this causes issues as far back as Darga as well (though, I think we don't support that version any more), should we backport to other branches as well?

[simaishi]

Gaprindashvili backport details:

$ git log -1
commit 01a01787c8c75d76ebf2d6f84d0b6ffdb9678021
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Jan 31 10:24:37 2018 -0500

    Merge pull request #16915 from NickLaMuro/bz-1539710-quick-fix-avoid-service-agg-col-with-miq-exp
    
    Exclude Service::AGGREGATE_ALL_VM_ATTRS from MiqExp.to_sql
    (cherry picked from commit 57095386dfb0fe1d799a1ad7bfc6fef6278dac98)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1540698

[simaishi]

Fine backport details:

$ git log -1
commit a6d06865680a9741aa22ee35d21227d86c01077e
Author: Gregg Tanzillo <[email protected]>
Date:   Wed Jan 31 10:24:37 2018 -0500

    Merge pull request #16915 from NickLaMuro/bz-1539710-quick-fix-avoid-service-agg-col-with-miq-exp
    
    Exclude Service::AGGREGATE_ALL_VM_ATTRS from MiqExp.to_sql
    (cherry picked from commit 57095386dfb0fe1d799a1ad7bfc6fef6278dac98)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1540699