Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Gating tests results are duplicated in Bodhi #4320

Closed
mruprich opened this issue Dec 17, 2021 · 9 comments
Closed

Gating tests results are duplicated in Bodhi #4320

mruprich opened this issue Dec 17, 2021 · 9 comments

Comments

@mruprich
Copy link

When gating is setup for a package, test results are seen in the update in Bodhi. For some reason they are duplicated:
https://bodhi.fedoraproject.org/updates/FEDORA-2021-81a0556d2a
Even when looking at the 'TEST GATING' at the right side of the Details tab in the update, it says 'All required tests passed' 'All required tests passed'.

Any idea why are the results shown twice?

@AdamWill
Copy link
Contributor

In this particular case it's due to #4266 . The update is for a critical path package, so two greenwave queries are run. The UI is showing the status and results from both queries.

I'm intending to work on this and some related issues when I can get enough roundtuits. It should be possible to make several improvements to how this stuff is displayed.

@AdamWill
Copy link
Contributor

I have a fix for this now, it's quite trivial, but I want to see if I can do anything about the gating results too before sending a PR.

AdamWill added a commit to AdamWill/bodhi that referenced this issue Mar 7, 2022
)

Since b933a07 , we query on two Greenwave 'decision contexts'
for critical path updates. A query with a different decision
context but the same subjects will always return the same set
of test results. So we wind up showing every test result twice,
on the Automated Test Results tab, because we just send the
output from every Greenwave query that gets run through
`handle_results`.

To avoid this, let's skip `handle_results` when the decision
context ended in "_critpath". We know we would always also run
the non-critpath query, so we will get the results from that
query. We do *not* skip `handle_unsatisfied_requirements`,
because whether any requirements are unsatisfied can and will
differ between queries with the same subject(s) but different
decision contexts (which is why we run two queries in the first
place).

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor

AdamWill commented Mar 7, 2022

I haven't been able to find time to work on the other problem, so I just sent what I have for skipping the dupe results for now, as #4411 .

AdamWill added a commit to AdamWill/bodhi that referenced this issue Mar 8, 2022
)

Since b933a07 , we query on two Greenwave 'decision contexts'
for critical path updates. A query with a different decision
context but the same subjects will always return the same set
of test results. So we wind up showing every test result twice,
on the Automated Test Results tab, because we just send the
output from every Greenwave query that gets run through
`handle_results`.

To avoid this, let's skip `handle_results` when the decision
context ended in "_critpath". We know we would always also run
the non-critpath query, so we will get the results from that
query. We do *not* skip `handle_unsatisfied_requirements`,
because whether any requirements are unsatisfied can and will
differ between queries with the same subject(s) but different
decision contexts (which is why we run two queries in the first
place).

Signed-off-by: Adam Williamson <awilliam@redhat.com>
mergify bot pushed a commit that referenced this issue Mar 9, 2022
Since b933a07 , we query on two Greenwave 'decision contexts'
for critical path updates. A query with a different decision
context but the same subjects will always return the same set
of test results. So we wind up showing every test result twice,
on the Automated Test Results tab, because we just send the
output from every Greenwave query that gets run through
`handle_results`.

To avoid this, let's skip `handle_results` when the decision
context ended in "_critpath". We know we would always also run
the non-critpath query, so we will get the results from that
query. We do *not* skip `handle_unsatisfied_requirements`,
because whether any requirements are unsatisfied can and will
differ between queries with the same subject(s) but different
decision contexts (which is why we run two queries in the first
place).

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor

AdamWill commented Oct 12, 2022

So I started digging into this again to outline the problem for a colleague who volunteered to help with the front end code, but in looking through and writing out a description of how it works, I started wondering...

Can we look at getting rid of the frontend Greenwave requests? I've always kinda disliked those. We have had multiple cases where the server-side and frontend stuff doesn't match up and people get confused: why does the server side (and so the comments on the left of the page) say gating is failed, but the frontend (and so the status shown on the right) say it's passed? or vice versa.

With current code, I think - I hope - the server side status should pretty much always be up to date and accurate. I think the only time that's potentially not the case is when the greenwave policies themselves change; we don't have anything hooked up to spot the policies changing (I'm not sure if a message is published for that or not) and re-calculate gating status. But that does not happen often. Aside from that, whenever a relevant result or waiver appears, we re-calculate the status, so it should always be accurate.

So, can we have the server side store the necessary info for what we want to show in the UI whenever it updates the gating status - overall gating status of the update, test results, and whether each failed test is gating - and just have the frontend code read that info and use it? To me, there are several advantages of this:

  • It gets rid of the potential for inconsistencies between the server side and frontend side
  • It makes it easier to solve this problem and avoids the need to have code on both sides that does the same thing: to solve this at present, we'd need the frontend to learn how to combine the results of multiple greenwave queries and decide what the overall status is, but the server side already has code to do that
  • It would save a lot of network traffic when people load update pages (good for everyone), and users would see the gating status and test results pretty much instantly, not "whenever all the greenwave queries are done"

Are we worried that would be keeping too much info in the db models? I guess storing all relevant test results for every update would work out to quite a lot of data...I could start working on this change, but I don't want to go too far on it if there are concerns with that approach. @mattiaverga , what do you think?

@AdamWill
Copy link
Contributor

AdamWill commented Oct 12, 2022

So, hmm, I guess we could already switch to using the server-side greenwave query data for generating just the "Gating status" text. We couldn't get rid of the frontend queries yet because the frontend query results are used to generate the actual list of test results shown on the Automated Results tab - but we could at least fix the badge and text under TEST GATING to be generated from the status we've already calculated on the server side, which would keep it always in sync with the status shown on the left, and solve the problem of us awkwardly showing the result of each separate query in a messy concatenation. I'll see if I can gin up a patch for that.

edit: thinking about it further, if we did that, we could cut the number of frontend queries down too - we'd only need to do enough to get all the test results. i.e. we would not need to do the extra critpath query (or, with #4759 , multiple extra critpath queries).

@mattiaverga
Copy link
Contributor

I'm not familiar with the code used in UI to fetch data from Greenwave, I've never looked too much into it. I don't remember who wrote that.
I wouldn't like to duplicate data already available from Greenwave into Bodhi database, especially because that data isn't strictly needed by Bodhi, but only for user interaction through the UI (display detailed results, waive tests, etc.). But that's just my personal opinion, it's best to coordinate with @abompard for development direction (or whoever is in charge).

@AdamWill
Copy link
Contributor

AdamWill commented Oct 13, 2022

so the tl;dr summary is that, to generate the list of test results shown on the Automated Tests tab of the update page and the gating status shown at the top of that page and under "TEST GATING" on the main update page, the frontend code runs the same set of greenwave queries as the backend - it uses update.greenwave_request_batches_json to get the list of queries to run. It runs the queries with verbose set to true so it gets test results in the responses. It then takes the summary text from each greenwave query and writes it into the HTML (so for an update which does a lot of queries - e.g. https://bodhi.fedoraproject.org/updates/FEDORA-2022-a895955321 - you get a really ugly long concatenated string of summaries), and generates the test result table from the results greenwave forwards.

I've always thought this was kind of awkward because it's duplicating all that work with the backend, which has already done it. And we do this every time anyone loads an update page, which is surely generating a whole lot of network traffic and load on greenwave that really isn't necessary.

But yes, to fully get rid of it we need to store the test results somewhere. We can't get away from that. If we want to display them in the UI we either have to have them stored for the frontend code to load, or we have to load them on the fly.

Meanwhile I've come up with a change that keeps the approach of the frontend code getting the data from greenwave, but has it produce a combined summary of the gating status instead of just concatenating all the query summary strings. I'll send a PR for that. In doing that, I did notice that the status summary the frontend shows is a bit more detailed than our backend TestGatingStatus representations; it shows how many tests failed or are missing, if any are. So to replace that with data read from the backend we'd have to tweak the backend to keep a bit more information.

AdamWill added a commit to AdamWill/bodhi that referenced this issue Oct 13, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor

#4784 is the PR to generate a combined summary after all greenwave queries are complete instead of just concatenating the summaries of each query as they are run. That would solve the second half of this bug report, i.e. it turns this:

Screenshot from 2022-10-13 17-41-48

into this:

Screenshot from 2022-10-13 17-42-49

AdamWill added a commit to AdamWill/bodhi that referenced this issue Oct 13, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/bodhi that referenced this issue Oct 14, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/bodhi that referenced this issue Oct 28, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
mattiaverga pushed a commit to AdamWill/bodhi that referenced this issue Oct 30, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill added a commit to AdamWill/bodhi that referenced this issue Oct 30, 2022
…ra#4320)

Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
mergify bot pushed a commit that referenced this issue Oct 31, 2022
Instead of showing one gating status per greenwave request,
which users find very confusing, keep track of requirement counts
as we run through the queries, then generate a single combined
gating status at the end.

Signed-off-by: Adam Williamson <awilliam@redhat.com>
@AdamWill
Copy link
Contributor

I believe everything here is resolved in 7.0 and this can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants