[Issue #125] Runaway process termination on startup #133

oleg-nenashev · 2016-11-26T21:39:57Z

Edited by @NextTurn: Resolves #125. Changes summary:

IWinSWExtension now offers hooks for the wrapped process startup/termination
Some generic process management logic has been moved from Main to ProcessHelper in WinSW Base
New extension point - Runaway Process Killer with some basic tests

This change introduces an extension, which performs automatic termination of runaway processes on startup. Sample configuration:

<?xml version="1.0" encoding="utf-8" ?>
<service>
  <id>SERVICE_NAME</id>
  <name>Jenkins Slave</name>
  <description>This service runs a slave for Jenkins continuous integration system.</description>
  <executable>%BASE%\sleep.bat</executable>
  <arguments></arguments>
  <logmode>rotate</logmode>

  <extensions>
	<!-- This is a sample configuration for the RunawayProcessKiller extension. -->
    <extension enabled="true" className="winsw.Plugins.RunawayProcessKiller.RunawayProcessKillerExtension" id="killOnStartup">
      <!-- Absolute path to the PID file, which stores ID of the previously launched process. -->
      <pidfile>%BASE%\pid.txt</pidfile>
      <!-- Defines the process termination timeout in milliseconds. This timeout will be applied multiple times for each child process. -->
      <stopTimeout>5000</stopTimeout>
      <!-- If true, the parent process will be terminated first if the runaway process gets terminated. -->
      <stopParentFirst>false</stopParentFirst>
    </extension>
  </extensions>
</service>

https://issues.jenkins-ci.org/browse/JENKINS-39231

…xtensions

…class

oleg-nenashev · 2016-11-26T21:44:44Z

Also created https://github.com/kohsuke/winsw/releases/tag/PR-133 with winsw.exe for evaluation

oleg-nenashev · 2016-11-26T21:44:53Z

@reviewbybees

ghost · 2016-11-26T21:48:09Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

oleg-nenashev · 2016-11-28T08:40:07Z

Plugin documentation will be delivered later once I get #129 integrated

jtnord · 2016-11-28T12:33:25Z

src/Core/WinSWCore/Util/ProcessHelper.cs

+        /// <returns>List of child process PIDs</returns>
+        public static List<int> GetChildPids(int pid)
+        {
+            var searcher = new ManagementObjectSearcher("Select * From Win32_Process Where ParentProcessID=" + pid);


Does this still work with a limited access user? (ie not running as SYSTEM but a locked down (non-admin) service account)

It used to work at least. The code has been just migrated to another place without change, so I do not expect differences

ok, missed that :o

jtnord · 2016-11-28T12:37:10Z

src/Core/WinSWCore/Util/XmlHelper.cs

@@ -13,7 +13,7 @@ public class XmlHelper
        /// </summary>
        /// <param name="node">Parent node</param>
        /// <param name="tagName">Element name</param>
-        /// <param name="optional">If otional, don't throw an exception if the elemen is missing</param>
+        /// <param name="optional">If optional, don't throw an exception if the elemen is missing</param>


you missed the second typo on the line :) (element)

jtnord · 2016-11-28T12:37:28Z

src/Plugins/RunawayProcessKiller/Properties/AssemblyInfo.cs

+[assembly: AssemblyConfiguration("")]
+[assembly: AssemblyCompany("")]
+[assembly: AssemblyProduct("RunawayProcessKiller")]
+[assembly: AssemblyCopyright("Copyright ©  2015")]


Will fix it

jtnord · 2016-11-28T12:38:55Z

src/Plugins/RunawayProcessKiller/RunawayProcessKiller.csproj

+  </PropertyGroup>
+  <ItemGroup>
+    <Reference Include="log4net">
+      <HintPath>..\..\packages\log4net.2.0.5\lib\net20-full\log4net.dll</HintPath>


NIT: does a user need to do more setup now to get log4net so they can compile this project? (where can they obtain it from - is the README updated?)

No need to do additional steps. log4net is being merged into the executable using ILMerge. A user still gets a single binary

jtnord · 2016-11-28T12:40:38Z

src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs

+            }
+            else
+            {
+                Logger.Warn("The requested PID file '" + Pidfile + "' does not exist. The runaway process won't be checked");


NIT: is this really a warning? - seems like INFO as this is expected (at least for the first time you startup the service)

And for the first time we have no idea if there is a runaway process. So I think it's a valid Warning

jtnord · 2016-11-28T12:42:40Z

src/Plugins/RunawayProcessKiller/RunawayProcessKillerExtension.cs

+            }
+
+            // Kill the runaway process
+            ProcessHelper.StopProcessAndChildren(pid, this.StopTimeout, this.StopParentProcessFirst);


This is very scary.... on windows PIDs can be reused... and if this is due to a reboot after say patching the PID could depend on the order of startup of services - so you could (attempt) kill a very important system service.

There should be some form of check that the parent process was the one started (perhaps by setting a magic environment cookie - or at least looking like it is a java process aka Jenkins)

jtnord · 2016-11-28T12:44:17Z

src/Test/winswTests/Extensions/RunawayProcessKillerTest.cs

+                + "  <id>SERVICE_NAME</id>                                                                                          "
+                + "  <name>Jenkins Slave</name>                                                                                     "
+                + "  <description>This service runs a slave for Jenkins continuous integration system.</description>                "
+                + "  <executable>C:\\Program Files\\Java\\jre7\\bin\\java.exe</executable>                                               "


this test is likely to fail on a CI system like Jenkins if this is needed - if its not - then should really be something like x:\ignored\not\used to avoid confusion.

This test does not actually start the executable. Hence should be fine

jtnord · 2016-11-28T12:47:06Z

@oleg-nenashev generally the code looks good - but killing process trees because a PID was left in a file looks very very scary to me - and I fear you could end up killing very important services on a restart of the OS (due to the old PID being used by a different service after restart) at which point you could crash windows (if its a system critical process) and which point you get boot loops....

oleg-nenashev · 2016-11-28T12:49:54Z

Yes, I agree the process validation needs to be extended

…rs service Id before terminating it

oleg-nenashev · 2016-11-30T11:38:46Z

@jtnord I have added the check of the Service Id environment of the process, which needs to be terminated

jtnord

Looks good to me

oleg-nenashev · 2016-12-01T07:03:22Z

@reviewbybees done
Here we go

oleg-nenashev added 3 commits November 26, 2016 20:43

Add extension point for tracking process startup and termination in e…

535e842

…xtensions

Decouple Some process management logic to a standalone ProcessHelper …

24a5e93

…class

[Issue winsw#125] - Introduce the RunawayProcessKiller extension

0a59da5

oleg-nenashev mentioned this pull request Nov 26, 2016

New plugin: Terminate runaway processes on startup [JENKINS-39231] #125

Closed

oleg-nenashev added needs-review new-feature labels Nov 26, 2016

oleg-nenashev changed the title ~~[Issue #125] Process termination on startup~~ [Issue #125] Runaway process termination on startup Nov 28, 2016

jtnord reviewed Nov 28, 2016

View reviewed changes

oleg-nenashev added the work-in-progress label Nov 28, 2016

oleg-nenashev added 2 commits November 30, 2016 12:36

Generalize usage of System environment variables for processes

5e83547

RunawayProcessKillerExtension: Check if the environment variable refe…

92367f4

…rs service Id before terminating it

jtnord approved these changes Nov 30, 2016

View reviewed changes

oleg-nenashev removed the work-in-progress label Nov 30, 2016

oleg-nenashev merged commit 62b495f into winsw:master Dec 1, 2016

oleg-nenashev deleted the processTerminationOnStartup branch December 1, 2016 07:03

oleg-nenashev restored the processTerminationOnStartup branch December 3, 2016 14:18

nxtn modified the milestones: 2.0.1, 2.0.0 Mar 25, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Issue #125] Runaway process termination on startup #133

[Issue #125] Runaway process termination on startup #133

oleg-nenashev commented Nov 26, 2016 •

edited by nxtn

Loading

oleg-nenashev commented Nov 26, 2016

oleg-nenashev commented Nov 26, 2016

ghost commented Nov 26, 2016

oleg-nenashev commented Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord Nov 28, 2016

jtnord Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord Nov 28, 2016

oleg-nenashev Nov 28, 2016

jtnord commented Nov 28, 2016

oleg-nenashev commented Nov 28, 2016

oleg-nenashev commented Nov 30, 2016

jtnord left a comment

oleg-nenashev commented Dec 1, 2016

[Issue #125] Runaway process termination on startup #133

[Issue #125] Runaway process termination on startup #133

Conversation

oleg-nenashev commented Nov 26, 2016 • edited by nxtn Loading

oleg-nenashev commented Nov 26, 2016

oleg-nenashev commented Nov 26, 2016

ghost commented Nov 26, 2016

oleg-nenashev commented Nov 28, 2016

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

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

jtnord commented Nov 28, 2016

oleg-nenashev commented Nov 28, 2016

oleg-nenashev commented Nov 30, 2016

jtnord left a comment

Choose a reason for hiding this comment

oleg-nenashev commented Dec 1, 2016

oleg-nenashev commented Nov 26, 2016 •

edited by nxtn

Loading