[JENKINS-59152] - Reduce the default process soft-kill timeout from 2 minutes to 5 seconds #4225
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
slonopotamus
approved these changes
Sep 25, 2019
MRamonLeon
added
the
needs-more-reviews
Member
|
The original issue that introduced it (JENKINS-17116) suggested 5 seconds, so we need to analyze/understand why it was changed to 2 minutes. |
Member
#3414 (comment) implemented it like that (and has drawn quite a lot of attention for this decision since). |
slonopotamus
added a commit
to slonopotamus/workflow-durable-task-step-plugin
that referenced
this pull request
Sep 27, 2019
Test currently passes. This is caused by the fact that Jenkins test framework ignores exceptions happening during build process. However, if you look at test output, you'll notice all bad effects described in JENKINS-59152: 1. Pinging continues for a long time after "Sending interrupt signal to process" 2. FileMonitoringTask$FileMonitoringController.cleanup fails to remove a directory because 20s timer in DurableTaskStep.Execution#stop works independently of SoftKillWaitSeconds and doesn't properly wait until processes are killed 3. There's InterruptedException because WindowsOSProcess#killSoftly was interrupted by timeout in CpsThread#stop 4. There's IllegalStateException from CpsStepContext#completed because CpsThread#stop attempted to complete step after it was already completed by stopTask in DurableTaskStep.Execution References: JENKINS-59152, JENKINS-17116, jenkinsci/jenkins#4216, jenkinsci/jenkins#4225
slonopotamus
added a commit
to slonopotamus/workflow-durable-task-step-plugin
that referenced
this pull request
Sep 27, 2019
Test currently passes. This is caused by the fact that Jenkins test framework ignores exceptions happening during build process. However, if you look at test output, you'll notice all bad effects described in JENKINS-59152: 1. Pinging continues for a long time after "Sending interrupt signal to process" 2. FileMonitoringTask$FileMonitoringController.cleanup fails to remove a directory because 20s timer in DurableTaskStep.Execution#stop works independently of SoftKillWaitSeconds and doesn't properly wait until processes are killed 3. There's InterruptedException because WindowsOSProcess#killSoftly was interrupted by timeout in CpsThread#stop 4. There's IllegalStateException from CpsStepContext#completed because CpsThread#stop attempted to complete step after it was already completed by stopTask in DurableTaskStep.Execution References: JENKINS-59152, JENKINS-17116, jenkinsci/jenkins#4216, jenkinsci/jenkins#4225
oleg-nenashev
approved these changes
Sep 28, 2019
Member
oleg-nenashev
left a comment
oleg-nenashev
left a comment
There was a problem hiding this comment.
I think it will improve the situation.
We will likely need to mention it in upgrade guidelines for LTS (CC @daniel-beck ) so that users of long timeout get a warning about the change, but I am fine with the change per se. Our process management is still big area for improvement
oleg-nenashev
added
ready-for-merge
oleg-nenashev
changed the title
oleg-nenashev
added
the
on-hold
Member
|
I will keep it for the next weekly so that reviewers have more time to react if they disagree with this change |
res0nance
approved these changes
Sep 30, 2019
oleg-nenashev
removed
the
on-hold
Member
|
I plan to merge it tomorrow if no negative feedback |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See JENKINS-59152 and analysis done by @slonopotamus.
Let's reduce the default value for soft-kill to 5 seconds to avoid interference by other mechanisms that want to avoid stalling and stuck processes.
Proposed changelog entries
Desired reviewers
@oleg-nenashev @slonopotamus