Remove left-over logic from switch to finalizing everything #643
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bloodearnest
force-pushed
the
fix-error-erroring
branch
3 times, most recently
from
71060c8 to
ac2af20
Compare
bloodearnest
force-pushed
the
fix-error-erroring
branch
from
ac2af20 to
9ae9a97
Compare
madwort
approved these changes
Aug 30, 2023
ExecutorState.ERROR can be confusing. It is meant to indicate that the *executor* has errored - a job exiting with an exit code is expected behaviour, not an *executor* error. As such, `get_status()` should not be returning ExecutorState.ERROR when a job is OOM killed - that's business as usual, and should be handled by `finalise()` as normal. When we refactored job-runner recently for everything to go via `finalise()`, I think this behaviour was mistakenly preserved from the old way of doing things. It resulted in job-runner treating OOM kills as INTERNAL_ERRORS, and obscuring the message to the users and needlessly paging teck-support. Now we only handle the OOM case in one place, I de-factored the functions that were previously shared between the two locations Fixes #634
If a resuable action happened to be the last job to execute, then the repo in the manifest file was set to the action's repo, not the study's repo. However, we no longer need the repo in the manifest files, so just set it none to avoid this problem. Once we have a releases UI, we can get rid of the manifest file code alltogether.
bloodearnest
force-pushed
the
fix-error-erroring
branch
from
9ae9a97 to
6d7d502
Compare
|
deployed |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
ExecutorState.ERROR can be confusing. It is meant to indicate that the
executor has errored - a job exiting with an exit code is expected
behaviour, not an executor error.
As such,
get_status()should not be returning ExecutorState.ERROR whena job is OOM killed - that's business as usual, and should be handled by
finalise()as normal.When we refactored job-runner recently for everything to go via
finalise(), I think this behaviour was mistakenly preserved from theold way of doing things. It resulted in job-runner treating OOM kills
as INTERNAL_ERRORS, and obscuring the message to the users and
needlessly paging teck-support.
Now we only handle the OOM case in one place, I de-factored the
functions that were previously shared between the two locations
Fixes #634