Better gating status #4221

AdamWill · 2021-05-18T22:12:25Z

This is an attempt to improve some issues around gating status for critical path updates, as reported in #4219 . Combined, these patches should mean we don't pingpong the state, and for the first two hours after an update is created or edited, the state will be 'waiting' if there are missing results but no failures.

AdamWill · 2021-05-19T06:11:05Z

edit: ok, cleaned up now. For some reason setting update.date_modified in the test just doesn't work, but fortunately setting update.date_submitted does work and does the same job, so hey. Let's go with that. This now passes all tests locally; the failures are due to a version mismatch in the CI environment and nothing to do with this PR, I don't think.

AdamWill · 2021-05-20T16:13:44Z

Ping? Anybody? I would like to help maintainers avoid unnecessary distress here :D

bodhi/server/models.py

hluk · 2021-05-21T08:10:03Z

+1 I think it's a good idea.

AdamWill · 2021-05-21T21:05:17Z

So how about this version? I think it's even simpler, and it manages to avoid some duplication with the old _unsatisfied_requirements property (now renamed _greenwave_requirements). I also like that the docstring exactly matches the code.

I did go and look at the greenwave side code and I think the basic idea here is safe. I don't think it should be possible for this logic to be wrong. You can see the main relevant part of the greenwave code here - it seems pretty clear-cut to me. I also checked that it's not possible for Greenwave to produce a decision with policies_satisfied but without the requirement lists - they are all produced together in a single statement. The only time those things are not in the response is on an error path, and we handle error responses differently.

hluk · 2021-05-23T08:12:42Z

bodhi/server/models.py


-        return unsatisfied_requirements
+        return TestGatingStatus.failed


Not sure if it is worth optimizing for this case, but it could stop fetching any next "greenwave_request_batches" when there is an unsatisfied requirement (that is not test-result-missing when last modification is under 2hrs).

I'm assuming that it won't happen too often so it is fine as is.

Hmm, I see what you mean. The trade-off there is that we'd probably have to put back the near-duplication I took out in this version, unless I can think of a clever way to do it. I can think about it a bit.

hluk · 2021-05-23T08:13:03Z

+1 Looks good, much easier to read.

hluk · 2021-05-23T08:17:21Z

bodhi/server/models.py

@@ -3051,7 +3075,8 @@ def waive_test_results(self, username, comment=None, tests=None):
        # Ensure we can always iterate over tests
        tests = tests or []

-        for requirement in self._unsatisfied_requirements:
+        # these are the unsatisfied requirements
+        for requirement in self._greenwave_requirements[1]:


A simple class for the requirement would avoid some mistakes and need for such explanation comments.

class GreenwaveRequirements: def __init__(self): self.satisfied = [] self.unsatisfied = []

well, if we want to do that we could just use collections.namedtuple. I thought about it but it seemed a bit excessive for just this one case. Can change it if you like?

Yup, sounds good, better than having to use an index. 👍

AdamWill · 2021-05-28T23:28:37Z

So I updated to a version which does short-circuit later batches if a definite 'failure' case is encountered in an earlier query batch. It does make things rather more complicated, though, so for the record, if we prefer the simpler but slightly less efficient code, the commit with that version is 7dfd24f282f17774b68f75c5b5b77d91fa1673c6. I can restore the PR to that state (then change it to use a namedtuple) if we prefer it.

AdamWill · 2021-06-01T17:03:54Z

ping? I don't think the test failures here are to do with the PR, btw.

hluk

LGTM

mattiaverga

Most of the test failures are unrelated, but the flake8 test fails:
./bodhi/tests/server/tasks/test_check_policies.py:130:101: E501 line too long (101 > 100 characters)

Can you also add a news fragment in /news ?

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

mattiaverga · 2021-06-07T08:29:27Z

Looks good, however I'd like to see some test results before approving.

AdamWill · 2021-06-12T01:21:51Z

Are you waiting for something from me here?

mattiaverga · 2021-06-12T07:01:06Z

No, I'm waiting for some admin to manually merge #4226 (or for some admin to give me admin access to Bodhi) so that required tests can pass and mergify can auto merge this PR.

Sorry, but I can't override failing tests at the moment.

waiting for tests fix

lgriffin · 2021-09-02T20:07:38Z

@Zlopez this is the other ticket!

Zlopez · 2021-09-07T08:23:25Z

@AdamSaleh The tests are failing on Fedora Messaging consumer container not ready. I see the same issue happened in #4244, but I'm not sure if this is just some temporary issue that will go away next time I try to run the tests.

Could this be just that the rabbitmq container takes too long to start and this is why we get timeout?

Zlopez · 2021-09-07T09:32:16Z

Because the pip-pydocstyle test is failing with container not ready issue. I ran the tests locally for it and found two issues:

bodhi/server/models.py:2218 in private method `_greenwave_requirements_generator`:
        D205: 1 blank line required between summary line and description (found 0)
bodhi/server/models.py:2218 in private method `_greenwave_requirements_generator`:
        D400: First line should end with a period (not 'd')

@AdamWill Could you address these issues in the PR?

AdamWill · 2021-09-07T15:35:43Z

Thanks, will do.

On the normal path of creating an update - where it is created and immediately submitted to updates-testing - we're setting the gating status twice. We call `set_request`, which calls `update_test_gating_status()` if the request is updates-testing and gating is required, and then almost immediately afterwards, we straight up set the status to 'waiting'. That doesn't make much sense. This changes things so that the `new()` method doesn't hard set the status to 'waiting' if it's calling `set_request` and the request is to updates-testing, as is usually the case. Signed-off-by: Adam Williamson <awilliam@redhat.com>

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

…ra#4219) This is part of addressing fedora-infra#4219. Currently we always count the gating status as failed if requirements are unsatisfied. However, it's usual for test results to not be present yet for a little while after the update is created or edited. Ideally we would have a mechanism for knowing for sure whether the tests in question are pending, but we don't, so let's use a dumb rule instead: if there are no failures (only missing results), and it's less than two hours since the update was last modified, we'll return 'waiting' as the status. After two hours, maybe the tests actually didn't get scheduled or the policy is wrong, so we'll return 'failed'. This also simplifies the logic used to decide the status, and reduces code duplication. We can determine the status using only the number of satisfied and unsatisfied requirements: if there are none of either it is ignored, if there are satisfied requirements and no unsatisfied requirements it is passed, otherwise it is failed or waiting. As part of this we change the mock greenwave responses used in a lot of other tests to use test-result-failed not -missing, because they currently use -missing but expect the gating status to be 'failed', which it no longer always will be. We also have to make the mock greenwave responses used in tests that hit this code a bit more realistic - they have to always include the satisfied and unsatisfied requirement lists. Note I've checked greenwave code and it does always include these in a successful query response. Signed-off-by: Adam Williamson <awilliam@redhat.com>

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

Zlopez

I ran the tests locally and everything is now passing. The code also looks good to me.

mattiaverga

Oh, it was stuck waiting for me... sorry

AdamWill · 2021-09-08T15:34:19Z

Oof, is the merge commit really necessary? Would you like me to rebase it?

mattiaverga · 2021-09-08T15:49:21Z

Oof, is the merge commit really necessary? Would you like me to rebase it?

mergify should handle it automatically. The PR got stuck again because the f33-diff-cover test has failed, but I think it's unrelated to the changes.
I'll give it another try.

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

AdamWill requested a review from a team as a code owner May 18, 2021 22:12

AdamWill mentioned this pull request May 18, 2021

Less scary UI and email rules for gating status changes #4219

Closed

AdamWill force-pushed the better-gating-status branch 4 times, most recently from 56072c3 to 0b1a3c3 Compare May 19, 2021 02:41

AdamWill force-pushed the better-gating-status branch from 0b1a3c3 to e90270c Compare May 19, 2021 06:51

hluk reviewed May 21, 2021

View reviewed changes

bodhi/server/models.py Outdated Show resolved Hide resolved

AdamWill force-pushed the better-gating-status branch from e90270c to 7dfd24f Compare May 21, 2021 20:57

hluk reviewed May 23, 2021

View reviewed changes

mattiaverga added this to the 5.7.1 milestone May 25, 2021

AdamWill force-pushed the better-gating-status branch from 7dfd24f to 9f07dc9 Compare May 27, 2021 21:12

hluk approved these changes Jun 2, 2021

View reviewed changes

mattiaverga linked an issue Jun 3, 2021 that may be closed by this pull request

Less scary UI and email rules for gating status changes #4219

Closed

mattiaverga previously requested changes Jun 3, 2021

View reviewed changes

AdamWill added a commit to AdamWill/bodhi that referenced this pull request Jun 3, 2021

Add news snippet for gating status changes (PR fedora-infra#4221)

a856f0c

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

AdamWill force-pushed the better-gating-status branch from 9f07dc9 to a856f0c Compare June 3, 2021 22:13

mattiaverga self-requested a review June 7, 2021 08:29

lgriffin mentioned this pull request Sep 2, 2021

Trying to fix pip CI tests #4226

Closed

AdamWill added a commit to AdamWill/bodhi that referenced this pull request Sep 7, 2021

Add news snippet for gating status changes (PR fedora-infra#4221)

469cba0

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

AdamWill force-pushed the better-gating-status branch from 556e152 to 469cba0 Compare September 7, 2021 15:39

AdamWill added 2 commits September 7, 2021 08:42

Add news snippet for gating status changes (PR fedora-infra#4221)

34c55c3

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

AdamWill force-pushed the better-gating-status branch from 469cba0 to 34c55c3 Compare September 7, 2021 15:43

Zlopez approved these changes Sep 8, 2021

View reviewed changes

mattiaverga approved these changes Sep 8, 2021

View reviewed changes

Merge branch 'develop' into better-gating-status

35a3b65

Merge branch 'develop' into better-gating-status

1fdfa5c

mergify bot merged commit 1bce05d into fedora-infra:develop Sep 8, 2021

AdamSaleh pushed a commit that referenced this pull request Sep 9, 2021

Add news snippet for gating status changes (PR #4221)

b4452db

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Better gating status #4221

Better gating status #4221

AdamWill commented May 18, 2021

AdamWill commented May 19, 2021 •

edited

Loading

AdamWill commented May 20, 2021

hluk commented May 21, 2021

AdamWill commented May 21, 2021

hluk May 23, 2021

AdamWill May 23, 2021

hluk commented May 23, 2021

hluk May 23, 2021

AdamWill May 23, 2021

hluk May 24, 2021

AdamWill commented May 28, 2021 •

edited

Loading

AdamWill commented Jun 1, 2021

hluk left a comment

mattiaverga left a comment

mattiaverga commented Jun 7, 2021

AdamWill commented Jun 12, 2021

mattiaverga commented Jun 12, 2021

lgriffin commented Sep 2, 2021

Zlopez commented Sep 7, 2021

Zlopez commented Sep 7, 2021

AdamWill commented Sep 7, 2021

Zlopez left a comment

mattiaverga left a comment

AdamWill commented Sep 8, 2021

mattiaverga commented Sep 8, 2021


		return unsatisfied_requirements
		return TestGatingStatus.failed

Better gating status #4221

Better gating status #4221

Conversation

AdamWill commented May 18, 2021

AdamWill commented May 19, 2021 • edited Loading

AdamWill commented May 20, 2021

hluk commented May 21, 2021

AdamWill commented May 21, 2021

hluk May 23, 2021

Choose a reason for hiding this comment

AdamWill May 23, 2021

Choose a reason for hiding this comment

hluk commented May 23, 2021

hluk May 23, 2021

Choose a reason for hiding this comment

AdamWill May 23, 2021

Choose a reason for hiding this comment

hluk May 24, 2021

Choose a reason for hiding this comment

AdamWill commented May 28, 2021 • edited Loading

AdamWill commented Jun 1, 2021

hluk left a comment

Choose a reason for hiding this comment

mattiaverga left a comment

Choose a reason for hiding this comment

mattiaverga commented Jun 7, 2021

AdamWill commented Jun 12, 2021

mattiaverga commented Jun 12, 2021

lgriffin commented Sep 2, 2021

Zlopez commented Sep 7, 2021

Zlopez commented Sep 7, 2021

AdamWill commented Sep 7, 2021

Zlopez left a comment

Choose a reason for hiding this comment

mattiaverga left a comment

Choose a reason for hiding this comment

AdamWill commented Sep 8, 2021

mattiaverga commented Sep 8, 2021

AdamWill commented May 19, 2021 •

edited

Loading

AdamWill commented May 28, 2021 •

edited

Loading