[JUJU-4012] Allow runners to stop based on an error #28
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
The following allows a runner to be stopped based on an error message. This makes it much more flexible in the phase of workers stopping based on an error, yet we don't want them to restart for some reason.
SimonRichardson
changed the title
manadart
reviewed
Jun 26, 2023
| starter.die <- fmt.Errorf("non-fatal error") | ||
| starter.assertStarted(c, false) | ||
| starter.assertNeverStarted(c, delay) | ||
| c.Assert(worker.Stop(runner), gc.IsNil) |
There was a problem hiding this comment.
Should we test that worker.Runner returns a NotFound after it gets the error?
manadart
approved these changes
Jun 26, 2023
There was a problem hiding this comment.
What do you think of inverting the bool and calling the function ShouldRestart?
I think I like this better, because automatically restarting non-fatal exits is a core consideration of the runner, and that's what this is addressing.
Happy to add the functionality in any case.
It's clearer than the previous name. By indicating what will happen if you return false it prevents a restart happening.
|
/merge |
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.
The following allows a runner to be stopped based on an error message. This makes it much more flexible in the phase of workers stopping based on an error, yet we don't want them to restart for some reason.
In addition, returning nil from some workers in the expectation that will ensure a clean kill is also mistaken. If using a runner to ensure workers stay running, returning nil will just cause that worker to restart after the delay. This probably isn't what we want in all scenarios. Adding this check allows us to add logic based on returned error cases to control the lifecycle of the worker.
This shouldn't require a new version either. Although we're adding a new field to the RunnerParams. We're backward compatible by ensuring that IsStoppable returns false by default.