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

Return nil health when inspecting containers without healthchecks #21388

Merged
merged 1 commit into from
Jan 31, 2024
Merged

Return nil health when inspecting containers without healthchecks #21388

merged 1 commit into from
Jan 31, 2024

Conversation

ashley-cui
Copy link
Member

When inspecting a container that does not define any health check, the health field should return nil. This matches docker behavior.

Does this PR introduce a user-facing change?

The `podman inspect` command now returns nil health when inspecting containers without healthchecks.

@openshift-ci openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. release-note labels Jan 26, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Jan 26, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ashley-cui

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 Jan 26, 2024
@ashley-cui ashley-cui marked this pull request as ready for review January 26, 2024 19:21
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 26, 2024
@ashley-cui
Copy link
Member Author

Fixes: #18792

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 14e979d86e24f6db7e41a8100694fea51b2b6932. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 2a936bc73f635776c48118339887f920b2eacd18. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 29e46941c6de791138e1a2aaee1b54b806633e01. @martinpitt, @jelly, @mvollmer please check.

@TomSweeneyRedHat
Copy link
Member

@ashley-cui looks like you might have upset other tests.

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Jan 27, 2024
@ashley-cui ashley-cui force-pushed the healthcheck branch 2 times, most recently from 9390d56 to 8e8f3fc Compare January 27, 2024 02:27
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 5ab50cee094e23f23e2bf89761ca27bc2534e7f7. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 9390d566dff9154e6ace0a9ccc2bdf2e8dc8c9bc. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 5ab50cee094e23f23e2bf89761ca27bc2534e7f7. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 8e8f3fcd1fdb976ea7716bd4445db3fe3164a03b. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 9390d566dff9154e6ace0a9ccc2bdf2e8dc8c9bc. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 8e8f3fcd1fdb976ea7716bd4445db3fe3164a03b. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 5ab50cee094e23f23e2bf89761ca27bc2534e7f7. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 9390d566dff9154e6ace0a9ccc2bdf2e8dc8c9bc. @martinpitt, @jelly, @mvollmer please check.

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 8e8f3fcd1fdb976ea7716bd4445db3fe3164a03b. @martinpitt, @jelly, @mvollmer please check.

rhatdan
rhatdan reviewed Jan 27, 2024
View reviewed changes
libpod/container_inspect.go Outdated
if reflect.DeepEqual(healthCheckState, define.HealthCheckResults{}) {
data.State.Health = &healthCheckState
}
data.State.Health = nil
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong, you set it and then unset it. Should this be in a else statement?

@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit 1546f27104e2a9ea47cffd2638ba63145f51adae. @martinpitt, @jelly, @mvollmer please check.

martinpitt added a commit to martinpitt/cockpit-podman that referenced this pull request Jan 28, 2024
@martinpitt
Don't assume State.Health always exists
20c044a 
containers/podman#21388 drops the `State.Health`
field for containers without health checks. This causes an `undefined`
crash in the container sort function.
jelly pushed a commit to cockpit-project/cockpit-podman that referenced this pull request Jan 28, 2024
@martinpitt @jelly
Don't assume State.Health always exists
9c861da 
containers/podman#21388 drops the `State.Health`
field for containers without health checks. This causes an `undefined`
crash in the container sort function.
@rhatdan
Copy link
Member

rhatdan commented Jan 28, 2024

LGTM

@rhatdan
Copy link
Member

rhatdan commented Jan 28, 2024

@containers/podman-maintainers PTAL

baude
baude reviewed Jan 28, 2024
View reviewed changes
pkg/api/handlers/compat/containers.go Outdated

for _, item := range log {
res := &types.HealthcheckResult{}
s, _ := time.Parse(time.RFC3339Nano, item.Start)
Copy link
Member

Choose a reason for hiding this comment

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

if you are comfy with ignoring this error, i would appreciate a comment here ... also ignored on the next line

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed, the error is now handled. Thanks!

@baude
Copy link
Member

baude commented Jan 28, 2024

one nit, lets wait for @martinpitt before merging ... else, LGTM

@martinpitt
Copy link
Contributor

martinpitt commented Jan 28, 2024 via email

Luap99
Luap99 reviewed Jan 29, 2024
View reviewed changes
libpod/container_inspect.go Outdated
@@ -196,7 +197,11 @@ func (c *Container) getContainerInspectData(size bool, driverData *define.Driver
// An error here is not considered fatal; no health state will be displayed
logrus.Error(err)
} else {
data.State.Health = healthCheckState
if reflect.DeepEqual(healthCheckState, define.HealthCheckResults{}) {
Copy link
Member

Choose a reason for hiding this comment

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

why are we using DeepEqual here? first it slow but second we already are checking if there is healthcheck with
c.config.HealthCheckConfig != nil so we should just assign the struct here.
The health key should only be missing if there is no healthcheck but not when the log is just empty (like it is doing here)

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, thanks!

@ashley-cui ashley-cui force-pushed the healthcheck branch 2 times, most recently from 14d5626 to 4ce2a10 Compare January 29, 2024 15:02
Luap99
Luap99 reviewed Jan 29, 2024
View reviewed changes
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

LGTM

@mheon
Copy link
Member

mheon commented Jan 29, 2024

LGTM

@mheon
Copy link
Member

mheon commented Jan 29, 2024

/lgtm
/hold

@mheon
Copy link
Member

mheon commented Jan 29, 2024

Tests are angry

@TomSweeneyRedHat
Copy link
Member

On the surface, the changes LGTM, but tests are angry and this looks like it might be a real test issue:

[+1967s] Summarizing 1 Failure:
[+1967s]   [FAIL] Podman healthcheck run [It] podman disable healthcheck with --no-healthcheck must not show starting on status
[+1967s]   /var/tmp/go/src/github.com/containers/podman/test/e2e/healthcheck_run_test.go:40
[+1967s]

@ashley-cui
Return nil health when inspecting containers without healthchecks
a1c47f0 
When inspecting a container that does not define any health check, the health field should return nil. This matches docker behavior.

Signed-off-by: Ashley Cui <[email protected]>
@ashley-cui ashley-cui added the 5.0 label Jan 31, 2024
@packit-as-a-service Packit-as-a-Service
Copy link

Cockpit tests failed for commit a1c47f0. @martinpitt, @jelly, @mvollmer please check.

@martinpitt
Copy link
Contributor

martinpitt commented Jan 31, 2024 •
edited
Loading

Wrt. the rawhide failure, the test instaled the wrong cockpit-podman version again (from rawhide, instead of from our main-builds COPR). I'll investigate today, please disregard this ATM.

I filed cockpit-project/cockpit-podman#1543 to fix this.

@martinpitt martinpitt mentioned this pull request Jan 31, 2024
Merged
@martinpitt
Copy link
Contributor

@ashley-cui the rawhide cockpit-revdeps should go green now, I landed a fix/hack. I can't retry it myself, but you should be able to on https://github.com/containers/podman/pull/21388/checks?check_run_id=21048234276

@mheon
Copy link
Member

mheon commented Jan 31, 2024

/lgtm
/hold cancel

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 31, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 1698fa0 into containers:main Jan 31, 2024
90 of 92 checks passed
@cevich cevich mentioned this pull request Jan 31, 2024
Merged
@ashley-cui ashley-cui mentioned this pull request Feb 6, 2024
Closed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Apr 30, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Apr 30, 2024
@ashley-cui ashley-cui deleted the healthcheck branch June 17, 2024 14:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@Luap99 Luap99 Luap99 left review comments

@baude baude baude left review comments

@rhatdan rhatdan rhatdan left review comments

Assignees

@mheon mheon

Labels
5.0 approved Indicates a PR has been approved by an approver from all required OWNERS files. kind/api-change Change to remote API; merits scrutiny 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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ashley-cui @TomSweeneyRedHat @martinpitt @rhatdan @baude @mheon @Luap99