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:
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).
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.
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)
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"...
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 :-)
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.
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.
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.
https://gerrit.beaker-project.org/5736
Thanks for fixing! Tested against the HUB mentioned in comment #10 and it looks OK. VERIFIED.
Beaker 24.4 has been released.