-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
healthcheck: stop showing wrong status when --no-healthcheck
is set
#13585
healthcheck: stop showing wrong status when --no-healthcheck
is set
#13585
Conversation
Containers started with `--no-healthcheck` are configured to contain no healthcheck and test configured as `NONE`. Podman shows wrong status as such use cases. Following commit fixes the faulty behavior of stauts field for containers started with `--no-healthcheck` Signed-off-by: Aditya R <[email protected]>
[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 |
// Check if healthcheck is not nil and --no-healthcheck option is not set. | ||
// If --no-healthcheck is set Test will be always set to `[NONE]` so no need | ||
// to update status in such case. | ||
if c.config.HealthCheckConfig != nil && !(len(c.config.HealthCheckConfig.Test) == 1 && c.config.HealthCheckConfig.Test[0] == "NONE") { |
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.
Could len(c.config.HealthCheckConfig.Test) > 1 and still have a value somewhere of "none"? If so what should we do there?
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.
@rhatdan Then most likely its manually done by user and its not set via --no-healthcheck
so that case should not be treated as equivalent to --no-healthcheck
IMO.
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.
Should --no-healthcheck conflict with --health-cmd then?
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.
yes, but that check should probably be much much further up the stack imho
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.
@baude is correct I think we already have a conflict check for that.
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.
Here is the line which conflicts: https://github.com/containers/podman/blob/main/pkg/specgenutil/specgen.go#L245
LGTM |
/lgtm |
Containers started with
--no-healthcheck
are configured to contain nohealth-check and test configured as
NONE
. Podman shows wrong status assuch use cases.
Following commit fixes the faulty behavior of stauts field for
containers started with
--no-healthcheck
Closes: #13578