-
Notifications
You must be signed in to change notification settings - Fork 209
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
worker: Improve retrieving extended reason from os-autoinst #3096
worker: Improve retrieving extended reason from os-autoinst #3096
Conversation
9bb5bdc
to
769db42
Compare
} | ||
} | ||
catch { | ||
$reason = "$reason: Corrupted state file could not be read"; | ||
if ($reason eq 'done') { | ||
$reason = "$reason: terminated with corrupted state file"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we find a phrasing that includes a user action? If I would read this as user I would ask "So, what now?"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The user might file an os-autoinst bug after checking that everything was setup correctly. Of course the same it true for Reason: backend died: Migrate to file failed
mentioned as example in the commit message.
Not sure whether it makes sense to include a hint like that. They usually file bugs when they see incompletes and can not make sense of them anyways.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. But under which circumstances could this fail in this way?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the point of view of openQA this JSON file is provided by an external source (os-autoinst and any user can mess with it) so it makes sense to validate the input here (as it was already the case before my changes) and report possible problems. In this case it would be especially bad to let an exception pass because the worker should in any case mark the job as done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hm, fine
* Take the relevant component into account because for https://progress.opensuse.org/issues/64884 we need to distinguish between test crashes and backend crashes * Ensure retrieving the extended reason does not interfere with stop reasons from the worker itself by handling worker-specific stop reasons first (and fix tests which were expecting that) With a change in os-autoinst to propagate the relevant component it will look like this: ``` Reason: tests died: unable to load, check the log for the cause (e.g. syntax error) Reason: backend died: Migrate to file failed ```
…carding it It is still a good idea to limit the reason but I suppose it make sense to pass the first line at least. Otherwise we needed to be very careful on the os-autoinst side not to produce multi-line error messages.
769db42
to
3f747c2
Compare
Codecov Report
@@ Coverage Diff @@
## master #3096 +/- ##
=======================================
Coverage 92.02% 92.02%
=======================================
Files 211 211
Lines 12845 12851 +6
=======================================
+ Hits 11820 11826 +6
Misses 1025 1025
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice. Cleaner than in before, fixed the "quit" mistake and added more functionality.
https://progress.opensuse.org/issues/64884 we need to distinguish
between test crashes and backend crashes
reasons from the worker itself by handling worker-specific stop reasons
first
With a change in os-autoinst to propagate the relevant component it will
look like this: