Allow querying multiple decision contexts in a single decision

[AdamWill]

When Greenwave was designed we never really envisaged a use for querying multiple decision contexts at once. We assumed it'd only ever make sense to care about exactly one context for whatever decision you're trying to make.

However, we wound up using multiple decision contexts to solve the problem of wanting different policies for the same gating point depending on the contents of the thing being gated. For a specific example, when gating Fedora updates, we have a bodhi_update_push_testing context that we always query for any update being pushed to testing, but we also have a bodhi_update_push_testing_critpath context that we only query for critical path updates (because it requires some tests that are only run on critical path updates). A pending Bodhi pull request - fedora-infra/bodhi#4759 - would make Bodhi query even more decision contexts for critical path updates.

Currently when Bodhi wants to query multiple decision contexts, it has to ask for one decision per context. This is pretty inefficient. It actually turns out to be quite easy to just allow the decision_context value to be a list of strings as well as a single string; all that really needs to be done to handle this is a fairly small tweak to Policy.matches(). Since we already iterate over every policy, I don't think this even has any significant performance impact, it should be about as fast to check every policy against a list of decision contexts as it is to check it against a single context.

Signed-off-by: Adam Williamson awilliam@redhat.com

[AdamWill]

FWIW, I've actually tested using this change in Bodhi, by running a dev instance of Bodhi against a dev instance of Greenwave with this patch applied. The changes to Bodhi are small and it definitely improves efficiency (mainly because Greenwave doesn't query the results twice for each package in the update).

[hluk]

That looks great, thanks!

Added one minor suggestion about stray print().

[AdamWill]

Oh, I guess one thing I forgot to note: the decisions about when to require a specific type or change a string to a list of strings are actually a bit arbitrary. You may wonder why I didn't have to change any of the Decision() instantiations in greenwave/tests/test_policies.py, which all just give the decision context as a bare string: that's because doing that actually works fine, because I made Policy.matches() handle that case. But it still felt 'right' to have make_decision() also convert from a single string to a list, even though it technically doesn't need to (it could just pass the value to Decision() whether it's a string or a list and things would magically work out OK).

I guess the way I thought about it is, the actual canonical path where this stuff gets used in production - which, AFAIK, is only through make_decision() - should be consistent and always use a list, so we know what it's doing. But we may as well have Policy.matches() and the Decision class still work with it being a string, because it's easy to do so what the hell, and it avoids having to change all those test cases.

I also considered changing applicable_decision_context_product_version_pairs to actually return lists of decision contexts, which would make it "more efficient", but decided to leave it the way it is, because if we change it, the decision change messages would be a bit different and consumers might not be expecting the change. People will still be able to use the decisions it publishes (with the decision contexts as strings) just fine.

[AdamWill]

thanks for merging this! what would be the schedule for pushing it to production? I kinda have a whole chain of things in bodhi then openQA update scheduling queued up behind it.

[hluk]

thanks for merging this! what would be the schedule for pushing it to production? I kinda have a whole chain of things in bodhi then openQA update scheduling queued up behind it.

Any chance you can verify it in stage? The container image with latest changes should be deployed there (I don't see any errors/warnings in logs).

To deploy to prod, I could just bump the container image to prod without doing the full release.

[AdamWill]

I can 'verify' it by running manual queries, but it's hard to check everything works "for real" because staging Bodhi doesn't really simulate production Bodhi very well.

I just threw a manual query at it and I think it was fine, except I also noticed openQA staging's resultsdb submitter has been turned off so the results aren't there to confirm we get a 'passed' decision...I've turned it back on and it's catching up, but it may take a while. Still, AFAICS things are sane for the old updates that are listed in staging Bodhi - it's getting decisions back from greenwave that match the results in resultsdb, i.e. none so everything fails gating, but the decisions are correct and contain the right list of expected test results. So I don't think this broke anything for how we currently have Bodhi deployed at least.

[AdamWill]

Oh, as for how this gets pushed to prod exactly: we'd need the 'prod' tag on quay to be updated, because bodhi has integration tests that deploy the 'prod' greenwave container. My PR will fail the bodhi tests as long as they're running against a greenwave that doesn't have this change.

[hluk]

The change is now in production. I do not see any problems in logs. 🙌

[AdamWill]

whew! thanks. i'll work on the Bodhi PR.