When possible, use grouped critpath data for greenwave queries and show it in UI tooltips #4759
Conversation
| ocontext = contexts[0] | ||
| for group in self.critpath_groups.split(" "): | ||
| contexts.insert(0, f"{ocontext}_{group}_critpath") | ||
| elif self.critpath: | ||
| contexts.insert(0, contexts[0] + "_critpath") |
There was a problem hiding this comment.
Note: I don't actually remember why we use insert here and not just append, but I don't want to change it since we probably did it for some kind of reason. I'd usually have added a comment explaining it, don't know why I didn't.
AdamWill
force-pushed
the
critpath-json-extended
branch
3 times, most recently
from
312e40a to
0abf511
Compare
|
Oh, one more note on this: even when all else is resolved, we should not merge this (or at least not deploy it in production) until the necessary greenwave policies are created, as querying a greenwave context that doesn't exist is a fatal error. I'll link PR(s) related to this when they're ready. |
|
I'm guessing the integration test failures are due to the missing db migration. |
|
I had started to create the database migration file, but then I started to think if a UnicodeText column is the best choice here... |
|
Well it may not be the best choice, but it was the easy choice ;) What would be the consequences of that for the API? What exactly would I get shown as the critpath groups for the update in an API query response? and no, critpath group names (the value used as the "name" here, anyway) don't ever contain spaces, at least by convention. What we're actually getting here are the group "id"s so far as comps is concerned, e.g. https://pagure.io/fedora-comps/blob/main/f/comps-f38.xml.in#_633 - we're getting the |
It would return a list of CritpathGroup items, say: instead of |
|
OK, that would probably work fine. I'll take a look and see if I can do it that way, then. |
|
Oh, I guess that would make the 'display the names as part of the tooltip' bit of this a bit harder to implement. It was quite nice that that was so trivial in my version. |
|
Um. I'm no database expert, but isn't this a many-to-many relationship? An update can have many critical path groups, and any critical path group will be associated with many updates. I don't see how I could make this work with one-to-many semantics? |
|
So, I'm about half way through doing this with many-to-many semantics, but it does actually involve a lot more code than just using a space-separated list. I have to do all the same stuff we do with is this really better? do you want me to keep going with this? Or am I missing something and I could implement this in a less painful way? |
|
Yeah, indeed it's a many-to-many relationship. I guess it doesn't worth the work, sorry... I'll generate the database migration file for your current PR, let's see if that is enough for the integration tests to pass. |
|
oh, a side note is that postgres actually supports an array type for columns. But only postgresql. so if we only care about supporting postgres, we could actually use that here, I guess. But I decided to go with the space separated list just because it felt a bit strange relying on something that only postgres supports. |
mattiaverga
force-pushed
the
critpath-json-extended
branch
from
69b8dcf to
373ebb4
Compare
|
Hmm, looks like two issues: I used a feature of tempfile that's apparently too new, and a test I didn't run in my local checks is failing. I'll look at them later tonight or tomorrow. |
AdamWill
force-pushed
the
critpath-json-extended
branch
10 times, most recently
from
512200d to
c18017f
Compare
|
OK, so that was more work than expected, and I had to fix your CI system a bit, but now it's good, I think. ;) |
AdamWill
force-pushed
the
critpath-json-extended
branch
from
c18017f to
80182b5
Compare
|
OK, that fail isn't my fault. |
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>
|
release-engineering/greenwave#95 would allow us to query multiple decision contexts in a single greenwave query, which would basically eliminate performance overheads and extra resource usage from these multiple context queries. This would be useful already, but it would be super useful for the changes in this PR, since they mean we will wind up querying rather more than just two contexts on many critpath updates. |
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
force-pushed
the
critpath-json-extended
branch
from
80182b5 to
baded1c
Compare
|
Thinking about it, we can merge this before we have the greenwave policies and so on set up, because our stg/prod Bodhi deployments will not use it until we change their config files to specify So I think we can drop the WIP tag now. It'd actually be nice to merge this and release Bodhi, then I can send a big PR for the infra ansible repo that adds the policies, adds a cron job or whatever to run the script daily, and changes the Bodhi config to use the JSON critpath type all at once (for stg only at first, I guess). |
|
https://pagure.io/fedora-infra/ansible/pull-request/1230 is an infra-ansible PR to enhance the greenwave policy so it'd be correct for this change (while also still working the same as at present if bodhi is not in the new grouped critpath mode). |
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>
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>
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
force-pushed
the
critpath-json-extended
branch
from
baded1c to
765674e
Compare
|
Ping? @mattiaverga can you drop the WIP label and review this? It'd be great to have it merged, because I want to move forward with trying to pare down how many tests openQA runs on each update, and this is needed to do that. As I mentioned above, I think it is actually safe to merge this right away, because we won't start hitting these new paths in production till we change the config settings. Thanks a lot! Oh, it would make sense to review and merge #4784 at the same time, because without that, this PR makes the proliferation of gating statuses worse. |
|
I was waiting for anyone else to drop their review about the database migration, but... I'll review it ASAP. Meanwhile, why the coverage of bodhi-server has been dropped to 98%? |
|
I had to manually edit the database on some updates to test this and I'm not sure I'm doing it right, but... |
bodhi-server/bodhi/server/models.py
Outdated
| # this is correct if update is already in testing... | ||
| contexts = ["bodhi_update_push_stable"] | ||
| if self.request == UpdateRequest.testing and self.status == UpdateStatus.pending: | ||
| # ...but if it is pending, we want to know if it can go to testing | ||
| contexts = ["bodhi_update_push_testing"] | ||
| if self.critpath: | ||
| if self.critpath_groups is not None: |
There was a problem hiding this comment.
I think this should just be if self.critpath_groups: to exclude empty field
There was a problem hiding this comment.
ah, yeah. my intent there was that we should always hit the first part of the conditional in grouped critpath 'mode', and if the update was not in any critpath groups, it shouldn't do anything, because the string is empty. Then we wouldn't even have to evaluate the elif self.critpath, because we know an update with critpath_groups as "" is not critpath. But I forgot that even splitting an empty string gives you a single-item list.
The way to 'fix' it the way I intended would be this:
if self.critpath_groups is not None:
if self.critpath_groups:
ocontext = contexts[0]
for group in self.critpath_groups.split(" "):
contexts.insert(0, f"{ocontext}_{group}_critpath")
elif self.critpath:
contexts.insert(0, contexts[0] + "_critpath")
return contexts
but your way is less obtuse and should always achieve the same effect, it just means we end up evaluating one extra statement when we're in grouped 'mode' and the update is not in the critpath, which isn't worth bothering about. (And this way we would always evaluate two conditions in grouped mode...) So I'll go with your approach. I'll have to see if I can make the tests cover this case too.
Since the server/client/messages split, we run pytest out of those subdirectories, not the top level directory. But there is no .coveragerc in those directories, so we weren't actually applying our coverage config customizations, including omitted files. We also drop the required coverage for bodhi-server to 98, as since this check was lost, actual coverage has dropped below 99 (to 98.59%). Signed-off-by: Adam Williamson <awilliam@redhat.com>
This new column is a nullable space-separated string listing the critical path groups, if any, of which packages in the update are members. The empty string means "no package in the update is a member of any critpath group" and is equivalent to critpath=False. A value of null/None means the configured critpath type does not expose grouped information (i.e. it's not critpath.type=json). We add a method and a util function to support figuring this out. This is kind of the long and humdrum way around, but it keeps things clearer overall than making get_critpath_components return different things for different types, for e.g. Signed-off-by: Adam Williamson <awilliam@redhat.com>
I explained that in the commit "Move .coveragerc files to server/client/messages": Since the server/client/messages split, we run pytest out of Basically, that rule has not actually been applied to any PR since the split into the three separate modules, and in that time, the coverage has dropped. You can try applying only that commit (none of my other changes), bump the level to 99, and it should fail. edit: for example, compare the very bottom of the unit test run on this PR, which looks like this: to the same thing from your recent PR #4819 , which looks like this: note the total there is 93% , way under 99%, but the "Required test coverage" check does not happen at all so the test doesn't fail. Both things are because the I only caught this because the |
When possible - i.e. when the critpath.type implementation provides grouped information, i.e. when it's "json" - this makes Bodhi query greenwave for a different context for each critical path group an update contains package(s) from. The intent is to allow us to run and gate on different sets of tests for different critical path groups. For instance, if an update is only in the critical path for Server, it doesn't make much sense to run and gate on GNOME/Workstation-related tests. If an update is only in the critical path for LXDE, maybe we don't need to run or gate on any tests at all (as we don't have any for LXDE). This should only be deployed in production after the Fedora greenwave policy has been updated to contain a policy for each critical path group defined in `critpath.py`. A risk of this approach is that we will actually hit an exception if we wind up querying for a context that has no policies. It may be possible to use wildcards in the 'null' policy to defend against this, I will check. We update a lot of related tests to use this codepath rather than the old one as we may, in future, want to drop the old codepath and only support this one. But we keep one test on the old codepath to make sure it works. Signed-off-by: Adam Williamson <awilliam@redhat.com>
This follows on the previous critical path group work by having the tooltip for the critical path icon in the web UI tell you which critical path groups the update is in, if we are on the grouped path. Signed-off-by: Adam Williamson <awilliam@redhat.com>
AdamWill
force-pushed
the
critpath-json-extended
branch
from
765674e to
363640b
Compare
|
OK, I rebased, changed that, and added a test that hopefully captures that case. |
|
Oh, also, as well as #4784 it would make sense to group this with #4821 - that is, make sure we have all three before the next release. They all can stand alone, and each is useful without either of the others, but they kinda make sense together. Including this one alone would rather increase the problem of sending a lot of greenwave queries and so generating a lot of 'noise' on the update page. 4784 makes the appearance of the page cleaner when we have to send a lot of queries, and 4821 stops us actually having to send a lot of queries just for the critpath stuff (but 4784 is still useful even after 4821 for the case of updates with a lot of packages in them, which will still generate a lot of queries because of the subject limit). Unfortunately I think 4821 will need quite a lot of rebasing on top of this one, I should've thought to just write it on top of this in the first place... |
There was a problem hiding this comment.
Well, I can't test the group decision context because I get Found no applicable policies for koji_build subjects at gating point(s) bodhi_update_push_stable_core_critpath in fedora-38 from staging Greenwave, while the production instance doesn't allow the connection due to CORS blocking...
Everything else works fine now, so let's push forward this one.
|
I tested it by running a local greenwave too. I dealt with the CORS thing by using a Firefox extension - I think https://addons.mozilla.org/en-US/firefox/addon/cors-everywhere/?utm_source=addons.mozilla.org&utm_medium=referral&utm_content=search . I have a PR to add the policies to our real greenwave config - https://pagure.io/fedora-infra/ansible/pull-request/1230 . It should work without this change being in production, so I may just go ahead and merge that soon. Then you should be able to test against the real greenwaves. |
This builds on #4755 to actually use the grouped critpath data for useful things. Primarily, it splits up the greenwave critpath queries by group. As well as the non-critpath greenwave query we run for all updates, for critpath updates, we would now run as many additional queries as there are critpath groups represented in the update, one per group. The point of this is so we can have different gating policies for the different groups (which would also allow us to only run a subset of tests for updates which are e.g. only on the "server critpath"; currently we have to run all tests for all critpath updates, because all critpath updates are gated on the full set of tests).
The obvious drawback of this is we are running additional greenwave queries. It'd be nice if we could somehow group or batch them, but at least with current greenwave we can't. I might look at doing that on the greenwave end. I don't think this is a crucial problem, though. In production, Bodhi talking to Greenwave should be very fast as they're on the same network.
We also would show which critpath groups a critpath update includes packages from in the tooltip shown when you hover over the icon indicating an update is in in the critical path. It might be nice to make this information available more prominently too, but at least doing this was easy.
One thing I know is missing from this PR so far: it needs a database migration file, but I can't generate one as I haven't been able to successfully set up a Bodhi development environment with working postgres database. I could write one by hand easily enough, but not sure if it matters what it's called. If I just need to use a random string in the file name I can do that. Otherwise, if someone with a working dev env could generate the migration file for me, that'd be great.
One thing I think should be OK but I'm not 100% sure: I'd want the new critpath_groups property to be exposed in the API - i.e. when I ask the Bodhi API for an update, I want to know what critpath groups it's in (this will be necessary for the "only schedule the appropriate tests for the groups the update is in" part of this). I don't think I need to do anything extra to make this happen, but if I do, please let me know.