-
Notifications
You must be signed in to change notification settings - Fork 297
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
OCPBUGS-33181: Fixed audit-logs sigterm failing to terminate gracefully #3972
OCPBUGS-33181: Fixed audit-logs sigterm failing to terminate gracefully #3972
Conversation
@Joseph-Goergen: This pull request references Jira Issue OCPBUGS-33181, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
@@ -967,3 +966,12 @@ func buildKonnectivityVolumeClusterCerts(v *corev1.Volume) { | |||
DefaultMode: pointer.Int32(0640), | |||
} | |||
} | |||
|
|||
func renderAuditLogScript(auditLogFilePath string) string { | |||
var script = `#!/bin/bash |
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.
Why are you starting a new script when the command calling this is /bin/bash
? Do you really need #!/bin/bash
?
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.
Probably not, I was mainly just following what others do when calling /bin/bash -c
like
hypershift/contrib/ci/hypershift-ci-1.yaml
Lines 111 to 119 in 85148d5
- args: | |
- -c | |
- | | |
#!/bin/bash | |
set -euo pipefail | |
/usr/bin/oc get istag -n hypershift hypershift-operator:latest -ojsonpath='{.image.dockerImageReference}' > /installer-data/image-ref | |
command: | |
- /bin/bash |
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.
But in this case, the previous code doesn't do that. So unless there's a good reason, please continue the pattern of the previous audit container setup.
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.
Previously, it was calling tail
directly, so it couldn't have - if we're going to use bash
it seems prudent to continue with euo pipefail
- it will reduce the chance we introduce any of the common bugs those guard against.
/jira refresh |
@jeffnowicki: This pull request references Jira Issue OCPBUGS-33181, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
||
func renderAuditLogScript(auditLogFilePath string) string { | ||
var script = `#!/bin/bash | ||
trap exit SIGTERM |
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.
Are you relying on implicit behavior of bash
when you call exit
? Could we instead explicitly forward SIGTERM
to children on SIGINT
?
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.
@stevekuznetsov is this what you'd like Joe to do: https://unix.stackexchange.com/questions/146756/forward-sigterm-to-child-in-bash?
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.
#!/usr/bin/env bash
set -o errexit
set -o nounset
set -o pipefail
function cleanup() {
for child in $( jobs -p ); do
kill "${child}"
done
wait
}
trap cleanup EXIT
for (( i = 0; i < 999; i++ )); do
child.sh $i
wait $!
done
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.
Something like this would work
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.
@Joseph-Goergen ptal
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.
Updated and tested(using a 4.15 release branch)
97c40ae
to
3ce718a
Compare
@@ -967,3 +966,23 @@ func buildKonnectivityVolumeClusterCerts(v *corev1.Volume) { | |||
DefaultMode: pointer.Int32(0640), | |||
} | |||
} | |||
|
|||
func renderAuditLogScript(auditLogFilePath string) string { |
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 we please export the function and re-use it in the OAPI deployment?
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.
Done!
cafc72c
to
5052adc
Compare
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
@stevekuznetsov |
/retest-required |
2 similar comments
/retest-required |
/retest-required |
/test e2e-kubevirt-aws-ovn |
done | ||
wait | ||
} | ||
trap cleanup EXIT |
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 tested this and it doesn't work unless EXIT
is SIGTERM
.
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.
@rtheis I tested this using the openshift-apiserver
# time kubectl delete pod -n master-coskjif10c6fhqphi0hg openshift-apiserver-55476ddfc-h4ldv
pod "openshift-apiserver-55476ddfc-h4ldv" deleted
real 1m47.992s
user 0m0.273s
sys 0m0.109s
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.
@Joseph-Goergen my point exactly, it shouldn't take 1 minute 47 seconds to fully terminate the pod.
/retest-required |
/test e2e-kubevirt-aws-ovn |
1 similar comment
/test e2e-kubevirt-aws-ovn |
c918f83
to
82df7b3
Compare
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
/retest-required |
1 similar comment
/retest-required |
/test e2e-kubevirt-aws-ovn |
@stevekuznetsov May I get a re-review? I had to rebase |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Joseph-Goergen, rtheis, sjenning, stevekuznetsov 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 |
@Joseph-Goergen: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
/test e2e-kubevirt-aws-ovn |
239a333
into
openshift:main
@Joseph-Goergen: Jira Issue OCPBUGS-33181: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-33181 has been moved to the MODIFIED state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
[ART PR BUILD NOTIFIER] This PR has been included in build ose-hypershift-container-v4.17.0-202405240410.p0.g239a333.assembly.stream.el9 for distgit hypershift. |
/cherry-pick release-4.15 |
@Joseph-Goergen: new pull request created: #4089 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/cherry-pick release-4.16 |
@rtheis: new pull request created: #4090 In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
What this PR does / why we need it:
Which issue(s) this PR fixes (optional, use
fixes #<issue_number>(, fixes #<issue_number>, ...)
format, where issue_number might be a GitHub issue, or a Jira story:Fixes #
Checklist