Approve jobs if at least older jobs passed #168
Conversation
michaelgrifalconi
changed the title
I can assure it works, I ran a lot of dry-runs on my machine with dashboard and openQA live data! |
|
If the code it's looking good enough, I will move on adapting the tests! |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
https://gist.github.com/michaelgrifalconi/4d07eee0197c929db7c2a11b85759edb Mind that stuff like Will be removed as I mention on the other conversations |
|
New log with less frequent/redundant and more descriptive logs: https://gist.github.com/michaelgrifalconi/41b6451b36cdfb87814b9ce9635a9459 Would not reduce too much logging, since it as something you would look only in case of problems, and do not disturb anyone (of course as long as it's not so huge to be expensive on resources or just clutter when debugging) |
There was a problem hiding this comment.
My feedback after the QE Tools workshop about this topic:
- What are alternatives that we can consider, how about again enabling automatic retry?
- How about shifting more aggregate tests to incidents, especially the tricky ones more prone to failing?
- There is no feedback to openQA so it will make it even harder for reviewers to find which jobs are actually blocking -> Hence my suggestion is instead to cover this logic in a separate application which simply writes "approvable_for" comments on openQA directly. This way there is direct feedback to reviewers same as to qem-bot. Thinking even further here such external applications can be triggered from openQA job hooks like scripts in https://github.com/os-autoinst/scripts/ do. I suggest you take a look into https://github.com/os-autoinst/scripts/blob/master/openqa-label-known-issues-and-investigate-hook for this
This comment was marked as resolved.
This comment was marked as resolved.
michaelgrifalconi
force-pushed
the
try-harder
branch
from
5c87c45
to
b0587b6
Compare
|
Before too much developent+review effort is invested please keep #168 (review) in mind which I think we agreed upon to follow up with. |
I agree on making sure automatic retries should be set at least at RETRY=1 globally. Where would be a good place to set that? Medium types?
Sure, there are tickets open for that. Still this is a different topic and I believe both things do help.
No, introducing a new thing that changes the behavior of something, from a different place makes it even more frustrating for an engineer to understand "what is happening and why". Right now this is the smallest tweak possible to increase the quality of life for maintenance test developers. About the visibility issue, I think that all current test failure should be fixed/softfailed at some point, since they surely block updates that were not tested the day before(like newly released one). I see that would still be nice to have visibility of what is really blocking an update and what could be ignored for a specific update request (by "approvable for comment"). Too bad that the dashboard does not show that either AFAIK. For this topic I would either:
I have no strong opinions for either option, as long as we don't spend too much time discussing on cosmetics and also new cosmetics changes do not require me to do more rebase of this PR (which i believe to be more important than suddenly changing code styles) |
|
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Considering that:
I would like to proceed as it is, add the necessary tests and then as soon as it is merged we can start a discussion on how to improve visibility and consider all various options like commenting and or moving to different bot job, etc. |
|
CI failures in https://github.com/openSUSE/qem-bot/actions/runs/8276775359/job/22649198458?pr=168#step:5:221
Yes, I can accept that although be aware that concerns were raised by others than just me about the overall approach. So besides the CI failures I see only two points missing before I can approve:
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #168 +/- ##
==========================================
+ Coverage 67.00% 67.74% +0.73%
==========================================
Files 25 25
Lines 1664 1730 +66
==========================================
+ Hits 1115 1172 +57
- Misses 549 558 +9 ☔ View full report in Codecov by Sentry. |
|
Right when I was feeling ready with this, I found a confirmation of the issue caused by using SMELT ids instead of RR ids. |
Having lunch brought me some wisdom. We can make use of the same workaround described in the open issue to avoid falling in the rabbit hole of fixing everything in the system at once. |
|
I think I am ready for the last review. If it's all fine I will do one last consolidation of commits and we can merge! |
michaelgrifalconi
force-pushed
the
try-harder
branch
from
17ea796
to
806cbc0
Compare
michaelgrifalconi
changed the title
michaelgrifalconi
force-pushed
the
try-harder
branch
from
806cbc0
to
5391f07
Compare
openqabot/approver.py
Outdated
| job = older_jobs["data"][i] | ||
| job_build = job["build"][:-2] | ||
| job_build_date = datetime.strptime(job_build, "%Y%m%d") | ||
|
|
||
| # Check the job is not too old | ||
| if job_build_date < oldest_build_usable: | ||
| log.info( | ||
| "Cannot ignore aggregate failure %s for update %s because: Older jobs are too old to be considered" | ||
| % (failed_job_id, inc) | ||
| ) | ||
| return False | ||
|
|
||
| if job["result"] == "passed" or job["result"] == "softfailed": | ||
| # Check the job contains the update under test | ||
| job_settings = self.client.get_single_job(job["id"]) | ||
| if not regex.match(str(job_settings)): | ||
| # Likely older jobs don't have it either. Giving up | ||
| log.info( | ||
| "Cannot ignore aggregate failure %s for update %s because: Older passing jobs do not have update under test" | ||
| % (failed_job_id, inc) | ||
| ) | ||
| return False | ||
|
|
||
| if not self.validate_job_qam(job["id"]): | ||
| log.info( | ||
| "Cannot ignore failed aggregate %s using %s for update %s because is not present in qem-dashboard. It's likley about an older release request" | ||
| % (failed_job_id, job["id"], inc) | ||
| ) | ||
| return False | ||
|
|
||
| log.info( | ||
| "Ignoring failed aggregate %s and using instead %s for update %s" | ||
| % (failed_job_id, job["id"], inc) | ||
| ) | ||
| return True |
There was a problem hiding this comment.
This for-body is now indented a bit too much. Can you extract a method here?
There was a problem hiding this comment.
Not a huge fan of creating methods for things that get called only in one place in the code, making me jump around in the file to follow the flow, but I guess it's a personal preference. Will do as requested
There was a problem hiding this comment.
I refactored a bit the code, without the need of a new method I removed a nested if to get down of one level of indentation and make it more readable
okurz
changed the title
michaelgrifalconi
force-pushed
the
try-harder
branch
from
96aae51
to
05e0a06
Compare
|
Yeah I usually try to show what I change instead of force push on every change since it makes more difficult to check and might hide some stuff! Rebased now |
- if aggregate update failed, do not give up immediately - look at openQA previous jobs, if present, green, not too old, still present in the qem-dashboard (to avoid using tests about different Release Requests) and it includes the update under test: ignore that failure This is to reduce the impact of one test being broken one day, a different test another day and the update not being approved even if combined result give all green, just not at the same time.
michaelgrifalconi
force-pushed
the
try-harder
branch
from
05e0a06
to
813f225
Compare
|
Thank for the approval! I have no rights to merge so someone else will have to do it :) |
https://progress.opensuse.org/issues/97118
This is to reduce the impact of one test being broken one day, a different test another day and the update not being approved even if combined result give all green, just not at the same time.
I did not touch the tests yet. Before investing more I would like to discuss about the new logic and it's implementation.