-
Notifications
You must be signed in to change notification settings - Fork 2.9k
Fixes #27651 - Fix health inspect/ps for rootfs containers with empty… #27744
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
/packit test --identifier cockpit-revdeps |
|
Account Yarboa has no write access nor is author of PR! |
@jasonoh11 please ask someone to rerun failed test as above |
Honny1
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! I have just one small nit.
1968b16 to
260fa31
Compare
|
Hi @Honny1 thanks for reviewing! I pushed an update according to your suggestion. It says there's still a workflow waiting for approval from a maintainer, is there any more action needed on my end or is this ready for approval? Also, @Yarboa mentioned a failed test, does this need to be resolved? Thanks! |
pkg/ps/ps.go
Outdated
| return err | ||
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CI is not happy.
+ ./bin/golangci-lint run --build-tags=apparmor,seccomp,selinux
pkg/ps/ps.go:230:1: File is not properly formatted (gofumpt)
^
1 issues:
* gofumpt: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cleaned up the spacing, thanks!
|
Some tests are flaky. I can re-run them for you. |
260fa31 to
6631a09
Compare
…with empty healthcheck Signed-off-by: Jason Oh <jasonoh@utexas.edu>
pkg/ps/ps.go
Outdated
| return err | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+ ./bin/golangci-lint run --build-tags=apparmor,seccomp,selinux
pkg/ps/ps.go:229:1: File is not properly formatted (gofumpt)
^
1 issues:
* gofumpt: 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed, thanks
6631a09 to
caaf292
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
… healthcheck
Checklist
Ensure you have completed the following checklist for your pull request to be reviewed:
commits. (
git commit -s). (If needed, usegit commit -s --amend). The author email must matchthe sign-off email address. See CONTRIBUTING.md
for more information.
Fixes: #00000in commit message (if applicable)make validatepr(format/lint checks)Noneif no user-facing changes)Does this PR introduce a user-facing change?
None
This fixes #27651, which raises the issue in which containers hold a stale "starting" status even though no healthchecks are configured.
As @mheon pointed out, the bug lies in the following condition:
if c.config.HealthCheckConfig != nil && (len(c.config.HealthCheckConfig.Test) != 1 || c.config.HealthCheckConfig.Test[0] != "NONE")In the case of a non-nil HealthCheckConfig and a nil or empty HealthCheckConfig.Test, the condition incorrectly enters the code path that handles when a health check is actually configured.
These conditions seem to arise when building a container with
--rootfsas opposed to building from an image.I extended HasHealthCheck() to correctly evaluate the various combinations of HealthCheckConfig and HealthCheckConfig.Test, and used this function to replace repeated, incorrect code causing both
podman inspect rootfs-test --format '{{json .State.Health}}'andpodman psto incorrectly return stale statuses.I also included a unit test for HasHealthCheck