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

healthcheck, libpod: Read healthcheck event output from os pipe #13129

Merged
merged 2 commits into from
Feb 7, 2022
Merged

healthcheck, libpod: Read healthcheck event output from os pipe #13129

merged 2 commits into from
Feb 7, 2022

Conversation

flouthoc
Copy link
Collaborator

@flouthoc flouthoc commented Feb 3, 2022

It seems we are ignoring output from health check session.
Open a valid pipe to health-check session in order read its output.

Use common pipe for both stdout/stderr since that was the previous
behavior as well.

Closes: #13083

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 3, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: flouthoc

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 3, 2022
@flouthoc flouthoc force-pushed the healthcheck-session-read-from-pipe branch from 9c9b5f0 to c31265f Compare February 3, 2022 13:27
@umohnani8
Copy link
Member

changes LGTM but tests are unhappy

@flouthoc flouthoc force-pushed the healthcheck-session-read-from-pipe branch from c31265f to b00cc23 Compare February 3, 2022 15:05
Luap99
Luap99 reviewed Feb 3, 2022
View reviewed changes
test/e2e/healthcheck_run_test.go Outdated Show resolved Hide resolved
libpod/healthcheck.go Show resolved Hide resolved
@flouthoc flouthoc force-pushed the healthcheck-session-read-from-pipe branch 6 times, most recently from 8073840 to ed8a1c3 Compare February 4, 2022 10:04
flouthoc
flouthoc commented Feb 4, 2022
View reviewed changes
test/system/220-healthcheck.bats
@@ -80,7 +80,6 @@ EOF
Status | healthy
FailingStreak | 0
Log[-1].ExitCode | 0
Log[-1].Output |
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Had to retrofit the bad tests which expects no output from health-check. While this PR fixes that.
Not checking log output here since we are already doing that in e2e just avoiding redundancy.

Copy link
Member

Choose a reason for hiding this comment

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

Please also check the output here

Copy link
Member

@edsantiago edsantiago Feb 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please no [EDIT: in reference to @flouthoc's anti-test comment, not @Luap99's favorable one]. This is not "redundancy". e2e tests run only in CI; system tests run on real live actual systems, before packages make it to RHEL or Fedora. They are our last-ditch effort to catch problems.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@edsantiago Thanks added a new test for this. But i had to modify jq output so it contains newline character rather than actual newline. Could you review bats part please.

@flouthoc
Copy link
Collaborator Author

flouthoc commented Feb 4, 2022

@containers/podman-maintainers PTAL

@flouthoc
healthcheck, libpod: Read healthcheck event output from os pipe
4f77331 
It seems we are ignoring output from healthcheck session.
Open a valid pipe to healthcheck session in order read its output.

Use common pipe for both `stdout/stderr` since that was the previous
behviour as well.

Signed-off-by: Aditya R <[email protected]>
@flouthoc
tests: retrofit healthcheck system tests
3cf64a8 
All the healthcheck return output now but systems tests is written to
expect empty output which seems wrong.

Modify jq output to contain newline character rather than actual newline

Signed-off-by: Aditya R <[email protected]>
@flouthoc flouthoc force-pushed the healthcheck-session-read-from-pipe branch from 4ebcb6c to 3cf64a8 Compare February 4, 2022 15:46
@flouthoc flouthoc requested review from edsantiago and Luap99 February 4, 2022 15:48
@TomSweeneyRedHat
Copy link
Member

LGTM if @edsantiago is happy with the test adjustments.
Happy green test buttons

@edsantiago
Copy link
Member

LGTM! Since this fixes the almost-two-year-old no-output bug, could you please also remove the unnecessary workaround?

index 6159a2807..c502ad669 100644
--- a/test/system/220-healthcheck.bats
+++ b/test/system/220-healthcheck.bats
@@ -15,9 +15,6 @@ function _check_health {
     run_podman inspect --format "{{json .State.Healthcheck}}" healthcheck_c

     parse_table "$tests" | while read field expect;do
-        # (kludge to deal with parse_table and empty strings)
-        if [ "$expect" = "''" ]; then expect=""; fi
-
         actual=$(jq ".$field" <<<"$output")
         is "$actual" "$expect" "$testname - .State.Healthcheck.$field"
     done

Thank you! Nice work!

@rhatdan
Copy link
Member

rhatdan commented Feb 7, 2022

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 7, 2022
@openshift-merge-robot openshift-merge-robot merged commit b1dcd02 into containers:main Feb 7, 2022
edsantiago added a commit to edsantiago/libpod that referenced this pull request Feb 7, 2022
@edsantiago
Cleanup: remove obsolete/misleading bug workaround
d0d1ffa 
Followup to containers#13129: remove a no-longer-necessary workaround
for a healthcheck bug.

Signed-off-by: Ed Santiago <[email protected]>
@edsantiago edsantiago mentioned this pull request Feb 7, 2022
Merged
mheon pushed a commit to mheon/libpod that referenced this pull request Feb 10, 2022
@edsantiago @mheon
Cleanup: remove obsolete/misleading bug workaround
9aee36b 
Followup to containers#13129: remove a no-longer-necessary workaround
for a healthcheck bug.

Signed-off-by: Ed Santiago <[email protected]>
patrycja-guzik pushed a commit to patrycja-guzik/podman that referenced this pull request Feb 15, 2022
@edsantiago @patrycja-guzik
Cleanup: remove obsolete/misleading bug workaround
ebdb3bd 
Followup to containers#13129: remove a no-longer-necessary workaround
for a healthcheck bug.

Signed-off-by: Ed Santiago <[email protected]>
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 21, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@mheon mheon mheon left review comments

@edsantiago edsantiago Awaiting requested review from edsantiago

@Luap99 Luap99 Awaiting requested review from Luap99

Assignees

@rhatdan rhatdan

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Healthcheck output is never shown
8 participants
@flouthoc @umohnani8 @TomSweeneyRedHat @edsantiago @rhatdan @mheon @Luap99 @openshift-merge-robot