Add process-level buildroot validation to NailgunExecutor.
Review Request #3393 - Created Jan. 29, 2016 and submitted
|jsirois, nhoward_tw, patricklaw, stuhood|
In the case of stale pids for nailgun processes, when a given pid is checked for liveness and the new process is also a java process it can pass the process executable name check (
java) but not actually be a nailgun process owned by the buildroot. Because the
NailgunExecutorfingerprint also does not match a given new fingerprint (
$NEW_SHA), the current logic treats this as a "I am running but have a new fingerprint and thus need to restart" case and attempts to kill the stale pid before starting a new nailgun. This can cause unintended behavior such as permissions related exceptions as seen in #2659 or worse, the killing of unrelated java processes.
This change addresses this case by adding an inline check for a matching nailgun buildroot argument in the
CI is green @ https://travis-ci.org/pantsbuild/pants/builds/105578526
Additional manual testing:
While monitoring with
ps:1) `./pants clean-all && ./pants compile testprojects/src/java::` in `~/dev/pants` which starts nailgun owned by `~/dev/pants`. 2) `cp -avf ~/dev/pants ~/dev/pants2 && rm -rf ~/dev/pants2/.pants.d` to wipe cache without removing `.pids` (creating a stale pid situation) 3) `cd ~/dev/pants2 && ./pants compile testprojects/src/java::`
Verified that new nailgun processes are launched for the second buildroot and the first buildroot's processes are untouched.
return not ( process or # Check for walkers. process.status() == psutil.STATUS_ZOMBIE or # Check for stale pids. self.process_name and self.process_name != process.name() or # Extended checking. extended_check and not extended_check(process) )
I believe that's equivalent and easier to read, given the cases in this method. If it's
True, we return
False. If it's
False, we return
True, but that's what we would have done anyway in the fall-through below (now you can remove that too).
And if there's an exception the behavior is unchanged.
Address Nick and Patrick's feedback.
Revision 2 (+101 -13)