Skip to content

[v5.4-rhel] Fix: Ensure HealthCheck exec session terminates on timeout #26456

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

Conversation

Honny1
Copy link
Member

@Honny1 Honny1 commented Jun 18, 2025

Previously, the HealthCheck exec session would not terminate on timeout, allowing the healthcheck to run indefinitely.
Because Docker does that, Podman should kill the HealthCheck when it reaches the timeout.

In order to backport this patch, you need to backport this patch as well (It is included in this PR):

This Backport contains:

Comparing 499ea11#diff-bf52e0a2dfcea051750fa86d05c8a2ce247c66a92a8562570f4d3555d76664a6R872
this linesession.PIDData = getPidData(pid) is removed because we would have to backport this PR #25906.

Fixes: https://issues.redhat.com/browse/RHEL-96916
Fixes: https://issues.redhat.com/browse/RHEL-96917

Does this PR introduce a user-facing change?

Fixed a bug where healthchecks exceeding their timeout were not properly terminated; they now receive SIGTERM then SIGKILL, aligning with Docker and preventing indefinite execution.
The HelathCheck is executed without writing to the database. 

Honny1 added 3 commits June 18, 2025 08:42
Previously, the HealthCheck exec session would not terminate on timeout, allowing the healthcheck to run indefinitely.

Fixes: https://issues.redhat.com/browse/RHEL-86096
Fixes: https://issues.redhat.com/browse/RHEL-96916
Fixes: https://issues.redhat.com/browse/RHEL-96917

Signed-off-by: Jan Rodák <[email protected]>
(cherry picked from commit 499ea11)
Signed-off-by: Jan Rodák <[email protected]>
…r timeout is 0

Aligns behavior with documentation stating SIGKILL should be sent immediately if the timeout is zero.

Fixes: https://issues.redhat.com/browse/RHEL-96916
Fixes: https://issues.redhat.com/browse/RHEL-96917

Signed-off-by: Jan Rodák <[email protected]>
(cherry picked from commit 077649f)
@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 Jun 18, 2025
@Honny1 Honny1 marked this pull request as ready for review June 18, 2025 07:08
@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 Jun 18, 2025
Copy link
Member

@lsm5 lsm5 left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

openshift-ci bot commented Jun 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Honny1, lsm5

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 Jun 18, 2025
@TomSweeneyRedHat TomSweeneyRedHat added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2025
@TomSweeneyRedHat
Copy link
Member

TomSweeneyRedHat commented Jun 18, 2025

For auditing reasons, this is based on:
Run HealthCheck without saving the ExecSession to the database #25003
Fix: Ensure HealthCheck exec session terminates on timeout #26086 commit
Fix: Ensure HealthCheck exec session terminates on timeout #26086 commit

if err != nil {
return -1, err
}
session.PID = pid
Copy link
Member

Choose a reason for hiding this comment

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

In 499ea11#diff-bf52e0a2dfcea051750fa86d05c8a2ce247c66a92a8562570f4d3555d76664a6R872
I'm seeing this line after this one:
session.PIDData = getPidData(pid)

Is that not needed?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is not needed because we would have to backport this PR #25906.

@TomSweeneyRedHat TomSweeneyRedHat removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 18, 2025
@TomSweeneyRedHat
Copy link
Member

One Question, otherwise LGTM

@TomSweeneyRedHat
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jun 23, 2025
@openshift-merge-bot openshift-merge-bot bot merged commit f944b21 into containers:v5.4-rhel Jun 23, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants