Bug 1464120 - excluding 'KVM' substring from hypervisor blocks almost all machines
excluding 'KVM' substring from hypervisor blocks almost all machines
Status: CLOSED CURRENTRELEASE
Product: Beaker
Classification: Community
Component: scheduler (Show other bugs)
24
Unspecified Unspecified
unspecified Severity unspecified (vote)
: 24.4
: ---
Assigned To: Dan Callaghan
Michael Petlan
: Patch
Depends On:
Blocks:
  Show dependency treegraph
 
Reported: 2017-06-22 09:19 EDT by Michael Petlan
Modified: 2017-10-02 23:57 EDT (History)
4 users (show)

See Also:
Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2017-10-02 23:57:45 EDT
Type: Bug
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)

  None (edit)
Description Michael Petlan 2017-06-22 09:19:27 EDT
Description of problem:

The following XML hostRequire snippet which should exclude the machines that have 'KVM' substring in the hypervisor field blocks out almost all non-KVM boxes too. It does not matter whether the <hypervisor/> tag is closed in <system></system> or not.

<not><hypervisor op="like" value="%KVM%" /></not>


Version-Release number of selected component (if applicable):
current production Beaker --> 24.3

How reproducible:
100%

Steps to Reproduce:
1. bkr list-systems --available --xml='<hostRequire><not><hypervisor op="like" value="%KVM%" /></not></hostRequire>'
2.
3.

Actual results:

The mentioned command gives us almost only powerpc boxes.

Expected results:

All the non-KVM x86_64 boxes, s390x boxes, aarch64 boxes, etc. are included in the list.

Additional info:
Comment 1 Michael Petlan 2017-07-10 05:03:41 EDT
Please note that '--bare' is not an ideal solution, since it adds

<and>
   <hypervisor value=""/>
</and>

to the hostFilter tag, which is nice, but it removes other virt-types too (ibm lpar in this case).
Comment 2 Dan Callaghan 2017-07-10 21:02:00 EDT
So the <not><hypervisor op="like" value="%KVM%" /></not> filter produces:

SELECT ...
FROM system LEFT OUTER JOIN hypervisor ON hypervisor.id = system.hypervisor_id 
WHERE hypervisor.hypervisor NOT LIKE %s

which runs afoul of NULL comparison. That is, hypervisor NOT LIKE NULL is NULL, it's not true. Basically the same bug as 886816 but we only fixed that one for =/!= whereas this is LIKE/NOT LIKE.
Comment 3 Dan Callaghan 2017-07-10 21:12:45 EDT
That means you have a workaround which is to use:

<hypervisor op="!=" value="KVM" />
<hypervisor op="!=" value="PowerKVM" />

which produces a clause with the necessary NULL handling, thanks to the fix in bug 886816:

SELECT ...
FROM system LEFT OUTER JOIN hypervisor ON hypervisor.id = system.hypervisor_id 
WHERE (hypervisor.hypervisor IS NULL OR hypervisor.hypervisor != %s) AND (hypervisor.hypervisor IS NULL OR hypervisor.hypervisor != %s)
Comment 4 Michael Petlan 2017-07-11 10:27:34 EDT
Hi Dan, thanks for the workaround, it seems to work. So is it that <hypervisor op="!=" value="KVM"/> does produce the clause the NULL handling while <not><hypervisor op="=" value="KVM"/></not> (which should be equal) does *not* produce the clause with the NULL handling, thus the second variant is not equal to the first?

It seems that if I use the snippets from comment #3, it works, but if I change it into <not><hypervisor op="=" value="KVM"/></not>, it gives the same wrong result as with "like"...
Comment 5 Dan Callaghan 2017-07-11 19:36:42 EDT
Yes, both op="like" and wrapping with <not/> will give incorrect results. In bug 886816 the only case I considered (and fixed) was with op="!=". Actually I think <not/> did not exist yet back then :-)
Comment 6 Dan Callaghan 2017-07-11 21:06:50 EDT
I am having trouble coming up with an implementation for <hypervisor/> in needpropertyxml that gives the correct answer for any arbitrary nesting inside of <not/> <and/> <or/> etc...

I think ultimately this problems stems from the fact that we are using NULL in this field to indicate "no hypervisor" rather than using NULL to mean "unknown" which is the traditional SQL sense.

Actually it's more like *either* unknown *or* no hypervisor, we start out assuming that a machine is not virtualised until an inventory scan is run on it where we might discover that it is.

But we could introduce a specific value to mean "not virtualised" which would eliminate a lot of the special casing for NULLs in the <hypervisor/> filter.

In most other places we don't have this problem because we use proper database enum types. For hypervisor however it is still an FK to an enum table because we want to allow admins to define new hypervisor types dynamically.
Comment 7 Dan Callaghan 2017-07-12 00:37:26 EDT
Another option is to alter <hypervisor/> to use the "NULL-safe" equality comparison operator. In MySQL that's <=>, in standard SQL it's IS NOT DISTINCT FROM. SQLAlchemy even has support for producing that *but* only from version 1.1 onwards (we have 0.9) and it also doesn't know about MySQL's non-standard version of the operator.

That wouldn't help with the op="like" case though. There is no "NULL-safe" LIKE operator.
Comment 8 Dan Callaghan 2017-07-12 00:42:04 EDT
A third option is to just replace the left-hand side of the comparison with a COALESCE() to implicitly map NULLs to some value such as empty string, and compare against that. That actually makes the code *simpler* while still handling all the operators as expected, and works correctly with <not/> and other arbitrarily nested conditions.

I was a little wary that changing the LHS of the comparison from a plain column to COALESCE(hypervisor, '') would ruin the query performance and cause MySQL to skip the index for that column. But it turns out the EXPLAIN looks the same when compared with what we have now. That is, we are *already* getting a less optimal query plan because of the hypervisor IS NULL OR hypervisor != 'KVM' clause that we are doing currently. Which makes sense because that's essentially equivalent to using COALESCE() on the LHS.

So it seems like we can go with this approach.
Comment 9 Dan Callaghan 2017-07-12 00:50:47 EDT
https://gerrit.beaker-project.org/5736
Comment 11 Michael Petlan 2017-08-28 08:48:06 EDT
Thanks for fixing! Tested against the HUB mentioned in comment #10 and it looks OK.

VERIFIED.
Comment 12 Dan Callaghan 2017-10-02 23:57:45 EDT
Beaker 24.4 has been released.

Note You need to log in before you can comment on or make changes to this bug.