Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Use anonyous FIFO instead of pipe in os::cmd output test #10040

Merged
merged 5 commits into from
Jul 29, 2016
Merged

Use anonyous FIFO instead of pipe in os::cmd output test #10040

merged 5 commits into from
Jul 29, 2016

Conversation

stevekuznetsov
Copy link
Contributor

Previously, the output of the last command was searched by
grep -q. When grep -q finds the text it is looking for,
it exits immediately with exit code 0. If the command feed-
ing grep -q with input is still running, it will write
to a pipe that has no readers, and Bash will terminate the
producer process with a SIGPIPE as this is invalid behavior.

Instead of using a cat | grep approach for searching in
os::cmd, we can therefore use grep <(file) approach, which
creates an anonymous FIFO to pass the data and therefore
guards against this sort of error.

Signed-off-by: Steve Kuznetsov [email protected]

fixes #9371

@stevekuznetsov
Copy link
Contributor Author

@liggitt PTAL

@stevekuznetsov
Copy link
Contributor Author

[test]

liggitt
liggitt reviewed Jul 26, 2016
View reviewed changes
hack/lib/cmd.sh
@@ -193,23 +193,14 @@ function os::cmd::internal::expect_exit_code_run_grep() {

local test_result=0
if [[ -n "${grep_args}" ]]; then
test_result=$( os::cmd::internal::run_collecting_output 'os::cmd::internal::get_results | grep -Eq "${grep_args}"'; echo $? )

test_result=$( os::cmd::internal::run_collecting_output "grep -Eq '${grep_args}' <(os::cmd::internal::get_results)"; echo $? )
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there a reason the quotes got inverted?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They were technically correct both times since the os::cmd::internal::run_collecting_output does an eval ... but it's better practice to have the args (e.g. ${grep_args}) expand in the shell before the eval so things like the stack trace can better signal to us what exactly was going on.

@liggitt
Copy link
Contributor

liggitt commented Jul 26, 2016

LGTM

@stevekuznetsov stevekuznetsov force-pushed the skuznets/os-cmd-output branch 2 times, most recently from fc484e0 to cbb9d32 Compare July 26, 2016 19:01
@stevekuznetsov
Copy link
Contributor Author

After going to hell and back trying to understand ten layers of Bash quoting rules I am happy with this now

[test]

@stevekuznetsov stevekuznetsov force-pushed the skuznets/os-cmd-output branch from cbb9d32 to 5ac5735 Compare July 26, 2016 19:10
@stevekuznetsov
Copy link
Contributor Author

Flake on e2e, this is ready @liggitt

@liggitt
Copy link
Contributor

liggitt commented Jul 27, 2016

The "no output" error surprised me, could it be related?
https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/4017/consoleFull#14085300745762928fe4b031e1b25ced2a

FAILURE after 59.220s: test/end-to-end/core.sh:21: executing 'oc get -n test pods -l name=database' expecting any result and text 'Running'; re-trying every 0.2s until completion or 60.000s: the command timed out
There was no output from the command.
There was no error output from the command.
[ERROR] PID 4343: hack/lib/cmd.sh:609: `return "${return_code}"` exited with status 1.
[INFO]      Stack Trace: 
[INFO]        1: hack/lib/cmd.sh:609: `return "${return_code}"`
[INFO]        2: test/end-to-end/core.sh:21: os::cmd::try_until_text
[INFO]        3: test/end-to-end/core.sh:303: wait_for_app
[INFO]   Exiting with code 1.

liggitt
liggitt reviewed Jul 27, 2016
View reviewed changes
test/cmd/basicresources.sh
@@ -349,9 +349,8 @@ echo 'projects command ok'

os::test::junit::declare_suite_start "cmd/basicresources/patch"
# Validate patching works correctly
oc login -u system:admin
os::cmd::expect_success 'oc login -u system:admin'
# Clean up group if needed to be re-entrant
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

move or remove comment

@stevekuznetsov stevekuznetsov force-pushed the skuznets/os-cmd-output branch from 5ac5735 to 703b5be Compare July 27, 2016 13:12
@stevekuznetsov
Copy link
Contributor Author

stevekuznetsov commented Jul 27, 2016 •
edited
Loading

The "no output" error surprised me, could it be related?

Still can't run e2e locally ... re[test]

although I would not expect that to be related, nothing changed with how we gather the output, just how we search through it

@stevekuznetsov
Copy link
Contributor Author

@liggitt didn't see the issue in e2e again, do not think it is related to this patch

@stevekuznetsov
Copy link
Contributor Author

@liggitt my comment triggered the currently running tests, but the push triggered the last series, which succeeded

@stevekuznetsov
Copy link
Contributor Author

conformance flake on #10002 #9195
@liggitt this is ready

@liggitt
Copy link
Contributor

liggitt commented Jul 27, 2016

[merge]

@stevekuznetsov
Copy link
Contributor Author

stevekuznetsov commented Jul 27, 2016 •
edited
Loading

test flaked on #9938:

FAILURE after 60.262s: test/cmd/quota.sh:25: executing 'oc describe appliedclusterresourcequota/for-deads-by-annotation -n bar --as deads' expecting any result and text 'secrets.*18'; re-trying every 0.2s until completion or 60.000s: the command timed out

/cc @deads2k : details

merge flaked on
#9959: https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin_integration/4076/consoleFull#-600577353578fb16ee4b059ed54a5eec0

@liggitt please re-tag

@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2016
@stevekuznetsov
Copy link
Contributor Author

rebased & ready again @liggitt

@liggitt
Copy link
Contributor

liggitt commented Jul 28, 2016

[merge]

@stevekuznetsov
Copy link
Contributor Author

@openshift-bot you're drunk, go home

@stevekuznetsov stevekuznetsov removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2016
@openshift-bot openshift-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2016
@stevekuznetsov stevekuznetsov removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 28, 2016
@stevekuznetsov
Use anonyous FIFO instead of pipe in os::cmd output test
e2c9dc6 
Previously, the output of the last command was searched by
`grep -q`. When `grep -q` finds the text it is looking for,
it exits immediately with exit code 0. If the command feed-
ing `grep -q` with input is still running, it will write
to a pipe that has no readers, and Bash will terminate the
producer process with a SIGPIPE as this is invalid behavior.

Instead of using a `cat | grep` approach for searching in
os::cmd, we can therefore use `grep <(file)` approach, which
creates an anonymous FIFO to pass the data and therefore
guards against this sort of error.

Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov
Ensured that all indentation in hack/lib/cmd.sh used tabs
a219186 
Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov
Improved Bash idioms in test/cmd/basicresources.sh
24f7a87 
Signed-off-by: Steve Kuznetsov <[email protected]>
@stevekuznetsov stevekuznetsov force-pushed the skuznets/os-cmd-output branch from 703b5be to 6badc53 Compare July 28, 2016 15:09
@stevekuznetsov
Copy link
Contributor Author

@liggitt robot is crazy, or GitHub API is crazy, since there were no merge conflicts, but I had to rebase anyway... will probably need another tag later
/grumble

@liggitt
Copy link
Contributor

liggitt commented Jul 28, 2016

[test][merge]

@openshift-bot
Copy link
Contributor

Evaluated for origin merge up to 6badc53

@openshift-bot
Copy link
Contributor

Evaluated for origin test up to 6badc53

@openshift-bot
Copy link
Contributor

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7067/)

@openshift-bot
Copy link
Contributor

openshift-bot commented Jul 29, 2016 •
edited
Loading

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/7067/) (Image: devenv-rhel7_4706)

@openshift-bot openshift-bot merged commit 10133fa into openshift:master Jul 29, 2016
@stevekuznetsov stevekuznetsov deleted the skuznets/os-cmd-output branch January 31, 2017 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

os::cmd sometimes doesn't find text in output
3 participants
@stevekuznetsov @liggitt @openshift-bot