This repository has been archived by the owner on Aug 29, 2018. It is now read-only.
cartridge control scripts: Fix exit value handling #6274
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
|
@Miciah this all makes sense. Can't find any issues with your changes. LGTM. Thanks for fixing this. |
|
Thanks, Tim! @VojtechVitek, would you be able to double-check this PR? I saw your name on some related changes in the Git log. |
|
LGTM // Disclaimer: I'm ex Red Hatter :) |
| echo "Restarting PHP ${OPENSHIFT_PHP_VERSION} cartridge (Apache+mod_php)" | ||
| ensure_httpd_restart_succeed "$HTTPD_PID_FILE" "$HTTPD_CFG_FILE" | ||
| if [ -f "$HTTPD_PID_FILE" ]; then | ||
| ensure_httpd_restart_succeed "$HTTPD_PID_FILE" "$HTTPD_CFG_FILE" || ret=$? |
There was a problem hiding this comment.
On second thought, maybe we expect this always to succeed and would rather the script fail loudly if it does not.
Fix some problems with how we handle exit values in control scripts. Most control scripts have `#!/bin/bash -e` or `set -e` so that they exit immediately if a command exits with a non-zero return value if that command is not part of a compound command or conditional statement. Thus if we anticipate that a command may fail, we must put it in a compound command or a conditional statement. This commit makes the following changes: • Replace `foo; [ $? -eq 0 ] && bar` with `local rc=0; foo || rc=$?; [[ "$rc" = 0 ]] && bar` where foo is an especially long command. • Replace `foo; [ $? -eq 0 ] || bar` with `if ! foo; then bar; fi`. • Replace `foo; ret=$?; if [ $ret -eq 0 ]; then` with `if foo; then`. • Replace `foo; ret=$?; if [ $ret -ne 0 ]; then` with `if ! foo; then`. • Replace `foo; bar $?` with `local rc=0; foo || rc=$?; bar "$rc"`. • Replace `foo; return $?` with `local ret=0; foo || ret=$?; return $ret`. • If a script runs with `/bin/bash -e` and ends with `exit $?`, delete the `exit $?` because it is redundant. Moreover, several control scripts had inverted logic around the /bin/kill command: These scripts would run /bin/kill, check its exit value, and retry /bin/kill if the first attempt returned 0. However, an exit value of 0 from /bin/kill means that it succeeded, so this code was retrying exactly when it did not need to do so. This commit fixes this logic.
Miciah
force-pushed
the
cartridge-control-scripts-fix-exit-value-handling
branch
from
a20fb19
to
b6765ea
Compare
|
Sorry for pinging you, Vojtech, and thanks for graciously providing a review! I updated the PR not to modify [test] [extended:cartridge,gear] |
|
Blocked by failures in the gear extended tests, which will be fixed by #6291. |
|
[test] again, please! |
|
Failed because of EC2 timeouts. [test] again, please! |
|
Lets try to [test] again, its been awhile. |
|
Cartridge group 4 tests appear to be failing: |
|
[test] again, please! |
|
Please [test] again! |
|
[test] again, please! |
|
Evaluated for online test up to b6765ea |
|
Online Test Results: FAILURE (https://ci.dev.openshift.redhat.com/jenkins/job/test_pull_requests/9344/) (Extended Tests: cartridge, gear) |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Fix some problems with how we handle exit values in control scripts.
Most control scripts have
#!/bin/bash -eorset -eso that they exit immediately if a command exits with a non-zero return value if that command is not part of a compound command or conditional statement. Thus if we anticipate that a command may fail, we must put it in a compoundcommand or a conditional statement. This commit makes the following changes:
• Replace
foo; [ $? -eq 0 ] && barwithlocal rc=0; foo || rc=$?; [[ "$rc" = 0 ]] && barwhere foo is an especially long command.• Replace
foo; [ $? -eq 0 ] || barwithif ! foo; then bar; fi.• Replace
foo; ret=$?; if [ $ret -eq 0 ]; thenwithif foo; then.• Replace
foo; ret=$?; if [ $ret -ne 0 ]; thenwithif ! foo; then.• Replace
foo; bar $?withlocal rc=0; foo || rc=$?; bar "$rc".• Replace
foo; return $?withlocal ret=0; foo || ret=$?; return $ret.• If a script runs with
/bin/bash -eand ends withexit $?, delete theexit $?because it is redundant.Moreover, several control scripts had inverted logic around the
/bin/killcommand: These scripts would run/bin/kill, check its exit value, and retry/bin/killif the first attempt returned 0. However, an exit value of 0 from /bin/kill means that it succeeded, so this code was retrying exactly when it did not need to do so. This commit fixes this logic.@tiwillia, @dobbymoodge, does this make sense, or am I silly?