Catch pipeline failures and return proper error status. #243
Conversation
This file contains hidden or 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
ndokos
force-pushed
the
wip-ssh-status
branch
2 times, most recently
from
6bae2b7 to
2a52de9
Compare
Member
Author
|
@atheurer: can you please review this? |
Member
|
Looks good to me. Could we add a unit test that catches hidden pipeline failures? |
ndokos
force-pushed
the
wip-ssh-status
branch
2 times, most recently
from
e2a9dd7 to
bb68731
Compare
Member
Author
|
@portante: I'll add a couple more tests but the underlying structure is sound, I think. |
| @@ -1,4 +1,5 @@ | |||
| #!/bin/bash | |||
| # -*- mode: shell-script; indent-tabs-mode: t; sh-basic-offset: 8; sh-indentation: 8; sh-indent-for-case-alt: + -*- | |||
Member
There was a problem hiding this comment.
Can we specify real tabs instead of spaces for emacs?
Member
Author
There was a problem hiding this comment.
I thought this is what indent-tabs-mode: t does. The doc says
Indentation can insert tabs if this is non-nil.
Member
Author
|
I'm going to squash the three non-unit-test commits into one and combine the commit messages. |
ndokos
force-pushed
the
wip-ssh-status
branch
2 times, most recently
from
ff20a01 to
a55f87e
Compare
Scripts where pipelines like this: ssh remote command | process output are used do not deal with ssh errors properly. The pipeline returns the status of the last command. Set the pipefail option in base: that way pipelines will return the status of the last failed command in the pipeline, not that of the last command. o pbench-register-tool now exits with the correct status of the pipeline. pbench-register-tool-set counts pbench-register-tool failures and exits with status equal to the number of failed pbench-register-tool calls. o Extend ssh status checking to pbench-clear-tools. There are two cases to consider: pbench-clear-tools is called with or without a --name=foo option. In the first case, the intent is to clear a single tool. pbench-clear-tools will try to ssh to all the remotes and clear the tool; it will also try to count how many tools are left (with another ssh) and if none, it will delete the local @Remote entry. If the first ssh fails, we do not continue to the second: we just count it as a failure, but since we cannot find out the state of the remote, we assume that it still has tools left and we do not remove the local entry for the remote. In the second case, the intent is to clear all tools, so even if the ssh fails, we remove the local entry for the remote. IOW, pbench-clear-tools with no --name argument might leave junk lying around on the remote if we can't get to it, but it clears everything locally. In either case, we return the number of ssh failures as the exit status of the command. o pbench-start-tools now checks its ssh pipeline for errors and returns status properly. In addition, pbench-start-tools is modified to call pbench-kill-tools before starting a new run. It has often been the case that tools from earlier runs are not cleaned up properly, particularly when the run is interrupted. Although that is a problem that should be resolved in the calling script, using "trap 'pbench-kill-tools' INT QUIT EXIT", this is an attempt to make tool handling more robust and avoid the most common failure scenarios.
Replace wait with a loop of "wait $pid" for each background process created, so we can get the exit status of the process. Each non-zero status counts as an error. The total number of errors is then returned as the status of pbench-postprocess-tools.
Modify mock ssh to return failures on a given host name ("fubar").
Modify unittests to pass when the test is expected to fail
and does so with an expected non-zero status code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Neither pbench-register-tool, nor pbench-register-tool-set
was checking for errors. In addition, the ssh | sed pipeline
in pbench-register-tool was returning the sed status which was
0.
Set the pipefail option in base: that way pipelines will return
the status of the last failed command in the pipeline, not that of the
last command. In particular, ssh failures will be caught.
pbench-register-tool now exits with the status of the pipeline.
pbench-register-tool-set counts pbench-register-tool failures
and exits with status equal to the number of failed pbench-register-tool
calls.