Allow disabling process killing on interruption #3293

apetres · 2018-02-14T15:33:26Z

Plugins may want to do custom action when the execution of a remote process is interrupted. Currently it is not possible, because
the process started on a slave is killed on interruption.
My actual use case: https://groups.google.com/forum/#!msg/jenkinsci-dev/s6IkynOzdqE/2eFNKN9lAAAJ

My solution is to have a flag called killWhenInterrupted, set by default to true, which prevents process killing on the slave when unset.

Plugins may want to do custom action when the execution of a remote process is interrupted. Currently it is not possible, because the process started on a slave is killed on interruption. My actual use case: https://groups.google.com/forum/#!msg/jenkinsci-dev/s6IkynOzdqE/2eFNKN9lAAAJ My solution is to have a flag called killWhenInterrupted, set by default to true, which prevents process killing on the slave when unset.

oleg-nenashev

Launcher should retain the default behavior when killWhenInterrupted is not invoked. My suggestion would be to invert flags and to use dontKillWhenInterrupted or so

oleg-nenashev · 2018-03-03T11:49:57Z

core/src/main/java/hudson/Launcher.java

+         * Indicates whether the process should be killed on interruption.
+         * 
+         * @return {@code this}
+         * @since 2.107


Needs to be removed before merge

oleg-nenashev · 2018-03-03T11:51:12Z

core/src/main/java/hudson/Launcher.java

-                boolean quiet, @CheckForNull String workDir, @Nonnull TaskListener listener) {
+        private final boolean killWhenInterrupted;
+
+        RemoteLaunchCallable(@Nonnull List<String> cmd, @CheckForNull boolean[] masks, @CheckForNull String[] env,


LGTM, no external usages in detached components and plugins.

oleg-nenashev · 2018-03-03T11:52:08Z

core/src/main/java/hudson/Proc.java

+    /**
+     * Indicates whether the process should be killed on interruption.
+     */
+    protected boolean killWhenInterrupted = true;


I would suggest using final private fields and getters

It can't be final since the value is set in the implementations.
Also it can't be private in order to be accessible from implementations. Maybe I should add a setter and make it private?

oleg-nenashev · 2018-03-03T11:54:54Z

core/src/main/java/hudson/Proc.java

@@ -217,7 +222,13 @@ public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out, File
        public LocalProc(String[] cmd,String[] env,InputStream in,OutputStream out,OutputStream err,File workDir) throws IOException {
            this( calcName(cmd),
                  stderr(environment(new ProcessBuilder(cmd),env).directory(workDir), err==null || err== SELFPUMP_OUTPUT),
-                  in, out, err );
+                  in, out, err, true );


Would be preferable to just call the new constructor below to avoid the logic duplication

oleg-nenashev · 2018-03-03T11:57:16Z

core/src/main/java/hudson/Launcher.java

@@ -181,6 +181,8 @@ public Computer getComputer() {
         */
        protected boolean reverseStdin, reverseStdout, reverseStderr;

+        protected boolean killWhenInterrupted;


🐛 it causes a regression in external API users. The default value will be false though the logic was always killing processes before the change

You are right. Flag was inverted.

apetres

killWhenInterrupted flag was inverted.

oleg-nenashev

The code looks good to me. There are more formatting cleanup than real code though.

@apetres BTW, have you already prototyped a patch for your plugin with this change? If yes, it would be nice to have a link

oleg-nenashev · 2018-03-08T10:07:58Z

core/src/main/java/hudson/Launcher.java

@@ -80,7 +80,7 @@
 *
 *
 * @author Kohsuke Kawaguchi
- * @see FilePath#createLauncher(TaskListener) 
+ * @see FilePath#createLauncher(TaskListener)


It would be preferable to keep refactorings in a separate pull request. But this PR is a non-backportable feature, so I can live with that

Sorry for that, my IDE is on steroids. I reverted the refactorings.

oleg-nenashev · 2018-03-08T10:10:53Z

core/src/main/java/hudson/Launcher.java

@@ -441,6 +443,10 @@ public ProcStarter writeStdin() {
            return this;
        }

+        public ProcStarter dontKillWhenInterrupted() {


Add some Javadoc with @since TODO

oleg-nenashev · 2018-03-08T10:12:03Z

core/src/main/java/hudson/Proc.java

@@ -64,6 +64,11 @@
 public abstract class Proc {
    protected Proc() {}

+    /**
+     * Indicates that the process should not be killed on interruption.
+     */


@since TODO. Protected fields are a part of API

oleg-nenashev · 2018-03-08T10:14:48Z

core/src/main/java/hudson/Launcher.java

@@ -181,6 +181,8 @@ public Computer getComputer() {
         */
        protected boolean reverseStdin, reverseStdout, reverseStderr;

+        protected boolean dontKillWhenInterrupted;


Some documentation would be useful + @since TODO

oleg-nenashev · 2018-03-08T10:16:38Z

Happy to approve once Javadoc is added

daniel-beck · 2018-03-08T17:45:27Z

How does this compare to ProcessKillingVeto?

apetres

@oleg-nenashev here is the link to the prototype plugin: Itiviti/testcomplete-plugin@566ce25

apetres · 2018-03-09T10:20:45Z

@daniel-beck AFAICT ProcessKillingVeto always prevents the killing of a process with given pid/arguments,etc. This is an undesired behavior for me because I do want to kill the process, but only after I executed custom action on the caller (plugin) side, see the link for the prototype implementation above.

oleg-nenashev · 2018-03-10T19:55:48Z

Thanks @apetres !

daniel-beck · 2018-03-10T22:14:00Z

No proposed changelog entry 👎

daniel-beck · 2018-03-10T22:34:46Z

always prevents the killing of a process with given pid/arguments,etc. This is an undesired behavior for me because I do want to kill the process, but only after I executed custom action on the caller (plugin) side, see the link for the prototype implementation above.

I don't see the problem with that BUILD_ID is among the env vars of the IOSProcess you'd veto, so you can trivially detect "your" process -- and, while vetoing the regular killing, implement your own as you did.

IMO this PR is not well justified was merged prematurely.

…)" This reverts commit e7fbce3.

oleg-nenashev · 2018-03-10T22:45:29Z

@daniel-beck Actually checking environment of a standalone process is a big problem for Windows. In Jenkins this resolution most probably does not work properly for 32-bit systems now: jenkinsci/winp#48

So I would not rely on this logic in Jenkins now.

But I agree that I merged it too prematurely, sorry about that

daniel-beck · 2018-03-11T00:07:10Z

@oleg-nenashev No worries, easy to revert and seems straightforward to submit again for further review.

daniel-beck · 2018-03-11T00:07:52Z

I'd also really like to see a test for this feature in core, if reasonably possible.

Revert "Allow disabling process killing on interruption (#3293)"

daniel-beck · 2018-03-11T12:14:01Z

I merged the revert PR #3339

@apetres This proposed change is not rejected, it was just prematurely merged, and given our release frequency and commitment to maintain API compatibility, not reverting would equal permanent acceptance starting later today with the release of 2.111. I'm sorry about this mess.

We're of course open to a resubmission of this PR against the current master, ideally with the concerns expressed above addressed.

apetres · 2018-03-12T08:40:09Z

Ok, so I should:

propose a changelog entry
add a test in core

@daniel-beck about your concerns above:

you can trivially detect "your" process

Yes, I can. Since only one instance of TestComplete/TestExecute process can be run at a time, I can even detect it using the arguments, something like here. So I don't have to rely on env vars.

while vetoing the regular killing, implement your own as you did

It's not clear for me how to kill a vetoed process from the plugin's side, since it only has access to the Proc object. Also IMO killing a vetoed process contradicts the concept of vetoing. Maybe I could run native killing instructions in a separate, joined process, after I ran my custom code (but IMO that's not really nice).

daniel-beck · 2018-03-12T10:43:51Z

@apetres Tests would be great! In general, what we expect from a PR is noted in the PR template at https://github.com/jenkinsci/jenkins/blob/master/.github/PULL_REQUEST_TEMPLATE.md

I think a reasonable start to continue this review would be the existing code + tests. Note that you'll probably have to re-do the code changes since it was merged and reverted (unless you're a Git wizard and know things I don't).

Additionally, an explanation why ProcessKillingVeto would not work for your use case would be good. I don't see how using that to not just kill the process and implement your own logic is inferior to this approach (although @oleg-nenashev makes a good argument there).

apetres · 2018-03-12T13:03:50Z

@daniel-beck ProcessKillingVeto is used to prevent killing processes. That means that they keep running even after the build is finished. From my plugin's side there is no way to kill the vetoed process because Proc.kill() won't do anything because of the veto.

For me this is an undesired side effect because I do want to kill the process after I run some logic on the plugin's side. Keeping it running after the build would cause an issue with future builds running on the same executor.

This is how the logic my plugin implementation looks like, for more details check the link to the prototype implementation.

Proc process;
try {
    Launcher.ProcStarter processStarter = launcher.launch()
        .cmds(args)
    //  .dontKillWhenInterrupted(true);
    process = processStarter.start();
    process.join();
    processTestResults()
} catch (InterruptedException e) {
   // InterruptedException usually is caught earlier on the executor's side, resulting in
   // TestComplete being killed and not saving the rest results correctly. That way I end
   // up without any test results, having ran hours of tests for nothing. That is the reason
   // I don't want to kill it
   stopTestCompleteNormally();
   processTestResults();
   // if I implement ProcessKillingVeto for the TestComplete process the line below won't
   // have any effect due to the veto. The veto prevent the killing of the process
   // on the executor side too which may result in a rogue TestComplete if stopTestCompleteNormally()
   // fails killing the process.
   process.kill();
   //Let Jenkins mark it as ABORTED
   throw e;
}

Now that I take a second look maybe implementing ProcessKillingVeto plus some custom native killing instructions (instead of process.kill()) would do the job. But IMO it would be much more nice with a flag on Proc's side.

daniel-beck · 2018-03-12T13:38:42Z

Or just add a flag that a given process should be killable after being processed by your custom logic.

Still, this approach does seem nicer, so I don't consider that a blocker.

apetres · 2018-03-14T13:21:38Z

Created a new PR with the desired changes.

oleg-nenashev requested changes Mar 3, 2018

View reviewed changes

oleg-nenashev added the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 3, 2018

Invert killWhenInterrupted flag

c6e8a7c

apetres commented Mar 6, 2018

View reviewed changes

oleg-nenashev reviewed Mar 8, 2018

View reviewed changes

oleg-nenashev removed the needs-comments-resolved Comments in the PR need to be addressed before it can be considered for merging label Mar 8, 2018

Add javadoc

e94b721

apetres commented Mar 9, 2018

View reviewed changes

oleg-nenashev approved these changes Mar 10, 2018

View reviewed changes

oleg-nenashev added the ready-for-merge The PR is ready to go, and it will be merged soon if there is no negative feedback label Mar 10, 2018

oleg-nenashev merged commit e7fbce3 into jenkinsci:master Mar 10, 2018

daniel-beck added a commit to daniel-beck/jenkins that referenced this pull request Mar 10, 2018

Revert "Allow disabling process killing on interruption (jenkinsci#3293…

fe38050

…)" This reverts commit e7fbce3.

daniel-beck mentioned this pull request Mar 10, 2018

Revert "Allow disabling process killing on interruption (#3293)" #3339

Merged

daniel-beck added a commit that referenced this pull request Mar 11, 2018

Merge pull request #3339 from daniel-beck/revert-3293

81f93cd

Revert "Allow disabling process killing on interruption (#3293)"

apetres mentioned this pull request Mar 14, 2018

[JENKINS-50166] Option to not to kill process on interruption #3351

Closed

Allow disabling process killing on interruption #3293

Allow disabling process killing on interruption #3293

Conversation

apetres commented Feb 14, 2018

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apetres left a comment

Choose a reason for hiding this comment

oleg-nenashev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oleg-nenashev commented Mar 8, 2018

daniel-beck commented Mar 8, 2018

apetres left a comment

Choose a reason for hiding this comment

apetres commented Mar 9, 2018

oleg-nenashev commented Mar 10, 2018

daniel-beck commented Mar 10, 2018

daniel-beck commented Mar 10, 2018

oleg-nenashev commented Mar 10, 2018

daniel-beck commented Mar 11, 2018

daniel-beck commented Mar 11, 2018

daniel-beck commented Mar 11, 2018

apetres commented Mar 12, 2018

daniel-beck commented Mar 12, 2018

apetres commented Mar 12, 2018

daniel-beck commented Mar 12, 2018

apetres commented Mar 14, 2018