Automated Test Results view doesn't catch incorrect waivers

[AdamWill]

Someone manually filed incorrect waivers for https://bodhi.fedoraproject.org/updates/FEDORA-2023-9a0b560d10 . They filed waivers for Fedora CI tests (whose subject is the package NVR), but filed the waivers with the subject as the update alias.

This meant that greenwave didn't consider the waivers valid and the update was still gated. However, the Automated Test Results view showed a 'thumbs-up' in the row for each failed test, with a mouseover indicating the failure had been waived.

This is because the code in make_row for adding this "update has been waived" icon is unsophisticated. It just checks whether there is any waiver for the testcase, and considers the failure waived if there is. It doesn't check whether the waiver is for the correct subject.

I'm filing an issue not a PR because I looked at writing a PR and it's kind of a pain (it's not just an easy one-liner, you'd have to rejig things a bit), so I'm prioritizing other stuff right now. But I wanted to flag the issue in case anyone else wants to fix it.

@vkadlcik for reference (if we make this better, Bodhi wouldn't seem to suggest you'd successfully waived the failures when you hadn't, and might even be able to tell you you filed a bad waiver...)

[AdamWill]

Just saw a similar issue with https://bodhi.fedoraproject.org/updates/FEDORA-2023-de5276064a where the Automated Tests tab was showing all the failures as waived, even though several of them actually had not been waived yet. Not sure why that is yet, will look at it.

[AdamWill]

Haaaar. I think I see why it happens now.

In update.html we construct the list of waivers at the update level, every time we get a response from greenwave, in a hash that's just keyed on test case names.

At no point do we take the subject into account at all when constructing the waiver list.

So if a given test case has been waived for any subject in the update, when we're in make_row constructing the table row for that test case for any subject, if the test is failed, we'll give it a thumbs up. That is, if there's a waiver for test case "sometest" on subject "foo-1.0-1.fc39", and the same test case also failed for subject "bar-2.0-1.fc39" in the same update, we'll put the this-has-been-waived thumbs-up in the row for bar as well as the row for foo. Even though the failure for bar has not actually been waived.

So turns out I was right to comment on this bug, because ultimately it's the same issue: we're not taking subjects into account properly when parsing waivers in the frontend.

I think the way to fix this is more or less to construct the waiver hash by subject then testcase, not just by testcase, and rejig make_row a bit so it's aware what subject it's operating on.

[AdamWill]

Actually, there should be a much more reliable way to do this.

Instead of primarily parsing the waivers - which means we have to reconstruct the work greenwave does to decide which waivers actually apply to which subjects - we can just parse the satisfied requirements. Maybe this wasn't the case when this code was written, but these days, the entry in greenwave's satisfied_requirements for a failure that was waived has value "test-result-failed-waived" for the key type, and a key waiver_id whose value is the id of the relevant waiver.

So instead of parsing the waivers we can parse the satisfied requirements, and when we see a waived one, just use the waiver ID to get the text description from the waivers...

edit: thinking about it, that would also partly address #5414 , which is nice.

[mattiaverga]

I wonder if it would be better to migrate this complex javascript logic into a python based endpoint and use javascript only to load the output of the endpoint... this way we could write down tests. I also think the code could be easier to maintain.

[AdamWill]

For a while I kinda wanted to burn down all this "have the frontend query greenwave itself and do lots of stuff with the results" code and just have the frontend get all the necessary info from the backend, but that turned out to be rather difficult because the backend would wind up kinda becoming a kinda shadow combined resultsdb and waiverdb, with a huge store of results and waivers that would need updating constantly...currently the backend code that talks to greenwave doesn't get or parse the test results, it does a non-verbose query and only considers the gating status...

Are you suggesting we should have an API endpoint that the frontend can hit to get the backend to do a greenwave query, parse the results, and send nicely-parsed info to the frontend to render? or is it more that the frontend talks to greenwave then sends the data it gets to the backend for the backend to parse? or...do we make it so any time update.html gets rendered, the backend talks to greenwave first, does the parsing, and passes the nicely-parsed data in for the javascript to render?

I guess I'm just having trouble visualizing how exactly the flows would work.

[mattiaverga]

I have to first understand exactly what the js code does (I never looked too deep into it).
But I certainly don't want to store anything into bodhi db. I was thinking about a backend endpoint wrapper to be queried live by update.html, do the work by quering greenwave, then return the nice results to the frontend.
I think we should do that (if we decide to do so) by a backend queried by an ajax call in update.html, having a function called while rendering the update page would slow down the entire page generation, so I don't think it's recommended.

[AdamWill]

So basically we'd do things more or less the way we currently do - render most of the page immediately, then do the greenwave query live and populate the Automated Results tab (and improve the gating status text) as the results come in - but move the actual work of doing the query and processing the results into the backend, via an API endpoint that the frontend code would hit while rendering the page?

I guess that adds an extra frontend<->backend round trip over what we have right now, right? But it might ultimately wind up being faster because all the greenwave queries would happen in the backend, which is very network-close to greenwave, rather than happening between the user's system and greenwave, which is a much longer roundtrip...so long as the backend could cope with the additional load of doing all that greenwave querying and parsing...

[AdamWill]

I guess it might also let us keep a closer sync between the frontend and backend's idea of the gating status, because as a byproduct we could have the update's gating status in the backend updated each time anyone hits the page for the update...since we're hitting greenwave anyway we should be able to do that for free.

[AdamWill]

as to what the JS code does: it queries greenwave (using information passed in from the backend to construct the actual query - it calls update.greenwave_request_batches_json to get it). The request has verbose set to True, which causes greenwave to return not just the gating status and lists of passed and failed requirements, but also a list of all test results greenwave considers to be relevant (so, really, we're kinda using greenwave as a proxy for resultsdb there). It uses the data it gets back from greenwave to calculate a gating status summary like "All required tests passed", which is shown in the right-hand column of the Details tab and at the top of the Automated Tests tab. (We used to just take summaries from the greenwave response and show them, but the problem with this is it is weird and confusing when we have to run more than one greenwave query, so now we work out a single combined summary of all the greenwave queries).

It also uses it to construct the entire contents of the Automated Tests tab, which is the most complex thing it does. It needs to show a row for every test result it gets, but also a row for any 'missing' results, which it finds from the 'unsatisfied requirements' from greenwave. It also shows little emblems indicating whether each test result is "required" in a gating policy (ones that are 'required' get an asterisk) and whether failed results have been waived, which is the bit we're concerned with here (waived failures get a thumbs-up).

[AdamWill]

While we think about that, I went ahead and wrote a PR which I hope should fix this (though as you note, writing this much javascript without a test suite for a safety net is no fun).