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

Greenwave: query both contexts for critpath updates (#4259) #4266

Merged
merged 1 commit into from Oct 18, 2021

Conversation

AdamWill
Copy link
Contributor

As discussed in #4259 and also reported in
https://pagure.io/fedora-ci/general/issue/263 , my previous
change to have Bodhi query on a different Greenwave 'decision
context' for critical path updates - allowing us to gate
critpath updates on openQA test results - caused a problem with
package-specific gating policies. The instructions and examples
for writing these only include the original decision contexts
(bodhi_update_push_testing and bodhi_update_push_stable), they
do not also recommend including the _critpath contexts, and
this is indeed how all real-world package-specific policies have
been written. This has resulted in package-specific policies
being effectively ignored when the package in question is a
part of a critical path update, because Bodhi is querying on
the _critpath context but the package-specific policy doesn't
include it.

To avoid this problem, this change makes Bodhi query greenwave
on both relevant contexts for critical path updates. The logic
for parsing the results doesn't need to change because we were
already coping with multiple queries for the case where an
update has too many subjects for a single query.

This does change the behaviour of one method -
Update.get_test_gating_info() - and one API endpoint -
/updates/{id}/get-test-results . These now both return lists
of Greenwave decision dicts, rather than a single Greenwave
decision dict. There's no way I can see to maintain behaviour
here, and the existing behaviour was already wrong for updates
with lots of packages (this method and endpoint should have
been adjusted when we initially implemented batched requests
for such updates, but they were missed). I don't think the method
or the endpoint were commonly used.

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

@AdamWill AdamWill requested a review from a team as a code owner October 16, 2021 00:05
As discussed in fedora-infra#4259 and also reported in
https://pagure.io/fedora-ci/general/issue/263 , my previous
change to have Bodhi query on a different Greenwave 'decision
context' for critical path updates - allowing us to gate
critpath updates on openQA test results - caused a problem with
package-specific gating policies. The instructions and examples
for writing these only include the original decision contexts
(bodhi_update_push_testing and bodhi_update_push_stable), they
do not also recommend including the _critpath contexts, and
this is indeed how all real-world package-specific policies have
been written. This has resulted in package-specific policies
being effectively ignored when the package in question is a
part of a critical path update, because Bodhi is querying on
the _critpath context but the package-specific policy doesn't
include it.

To avoid this problem, this change makes Bodhi query greenwave
on *both* relevant contexts for critical path updates. The logic
for parsing the results doesn't need to change because we were
already coping with multiple queries for the case where an
update has too many subjects for a single query.

This does change the behaviour of one method -
Update.get_test_gating_info() - and one API endpoint -
/updates/{id}/get-test-results . These now both return lists
of Greenwave decision dicts, rather than a single Greenwave
decision dict. There's no way I can see to maintain behaviour
here, and the existing behaviour was already wrong for updates
with lots of packages (this method and endpoint should have
been adjusted when we initially implemented batched requests
for such updates, but they were missed). I don't think the method
or the endpoint were commonly used.

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

I don't think the failed tests relate to the PR. Something to do with fedora-messaging in the integration test environment.

Copy link

@thrix thrix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, did not find anything worth commenting

Copy link
Contributor

@mattiaverga mattiaverga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think anything else other than Bodhi uses that API endpoint, so I think we can safely change the output.

Since we're still in freeze and Bodhi 5.7.1 is still in staging, I think it could be nice to have this pushed into the next release.

@mergify mergify bot merged commit b933a07 into fedora-infra:develop Oct 18, 2021
@AdamWill
Copy link
Contributor Author

I don't think anything else other than Bodhi uses that API endpoint, so I think we can safely change the output.

Since we're still in freeze and Bodhi 5.7.1 is still in staging, I think it could be nice to have this pushed into the next release.

yes, we absolutely want this in the next release and we want the next release out ASAP, as the problem is causing real pain for people trying to use the package-specific policies. sorry about that!

@psss
Copy link

psss commented Oct 20, 2021

Thanks for the fix, @AdamWill.

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

Successfully merging this pull request may close these issues.

Package-specific gating policies may not work correctly with critpath packages
4 participants