Merge vlovich/buildbot:switch-git-to-sigterm (PR #1702)

master/buildbot/steps/source/git.py

@@ -337,11 +337,35 @@ def _dovccmd(self, command, abandonOnFailure=True, collectStdout=False, initialS
                 full_command.append('-c')
                 full_command.append('%s=%s' % (name, value))
         full_command.extend(command)
+
+        # check for the interruptSignal flag
+        sigtermTime = None
+        interruptSignal = None
+
+        # If possible prefer to send a SIGTERM to git before we send a SIGKILL.
+        # If we send a SIGKILL, git is prone to leaving around stale lockfiles.
+        # By priming it with a SIGTERM first we can ensure that it has a chance to shut-down gracefully
+        # before getting terminated
+        if not self.slaveVersionIsOlderThan("shell", "2.16"):
+            # git should shut-down quickly on SIGTERM.  If it doesn't don't let it
+            # stick around for too long because this is on top of any timeout
+            # we have hit.
+            sigtermTime = 1
+        else:
+            # Since sigtermTime is unavailable try to just use SIGTERM by itself instead of
+            # killing.  This should be safe.
+            if self.slaveVersionIsOlderThan("shell", "2.15"):
+                log.msg("NOTE: slave does not allow master to specify interruptSignal. This may leave a stale lockfile around if the command is interrupted/times out\n")
+            else:
+                interruptSignal = 'TERM'
+
         cmd = remotecommand.RemoteShellCommand(self.workdir,
                                                full_command,
                                                env=self.env,
                                                logEnviron=self.logEnviron,
                                                timeout=self.timeout,
+                                               sigtermTime=sigtermTime,
+                                               interruptSignal=interruptSignal,
                                                collectStdout=collectStdout,
                                                initialStdin=initialStdin)
         cmd.useLog(self.stdio_log, False)

master/buildbot/test/fake/remotecommand.py

@@ -234,6 +234,58 @@ def runBehaviors(self, command):
         for behavior in self.behaviors:
             yield self.runBehavior(behavior[0], behavior[1:], command)
 
+    def expectationPassed(self, exp):
+        """
+        Some expectations need to be able to distinguish pass/fail of
+        nested expectations.
+
+        This will get invoked once for every nested exception and once
+        for self unless anything fails.  Failures are passed to raiseExpectationFailure for
+        handling.
+
+        @param exp: The nested exception that passed or self.
+        """
+        pass
+
+    def raiseExpectationFailure(self, exp, failure):
+        """
+        Some expectations may wish to supress failure.
+        The default expectation does not.
+
+        This will get invoked if the expectations fails on a command.
+
+        @param exp: the expectation that failed.  this could be self or a nested exception
+        """
+        raise failure
+
+    def shouldAssertCommandEqualExpectation(self):
+        """
+        Whether or not we should validate that the current command matches the expecation.
+        Some expectations may not have a way to match a command.
+        """
+        return True
+
+    def shouldRunBehaviors(self):
+        """
+        Whether or not, once the command matches the expectation,
+        the behaviors should be run for this step.
+        """
+        return True
+
+    def shouldKeepMatchingAfter(self, command):
+        """
+        Expectations are by default not kept matching multiple commands.
+
+        Return True if you want to re-use a command for multiple commands.
+        """
+        return False
+
+    def nestedExpectations(self):
+        """
+        Any sub-expectations that should be validated.
+        """
+        return []
+
     def __repr__(self):
         return "Expect(" + repr(self.remote_command) + ")"
 

master/buildbot/test/util/steps.py

@@ -33,6 +33,51 @@
 from twisted.internet import task
 
 
+def _dict_diff(d1, d2):
+    """
+    Given two dictionaries describe their difference
+    For nested dictionaries, key-paths are concatenated with the '.' operator
+
+    @return The list of keys missing in d1, the list of keys missing in d2, and the differences
+    in any nested keys
+    """
+    d1_keys = set(d1.keys())
+    d2_keys = set(d2.keys())
+    both = d1_keys & d2_keys
+
+    missing_in_d1 = []
+    missing_in_d2 = []
+    different = []
+
+    for k in both:
+        if isinstance(d1[k], dict) and isinstance(d2[k], dict):
+            missing_in_v1, missing_in_v2, different_in_v = _dict_diff(d1[k], d2[k])
+            missing_in_d1.extend(['{0}.{1}'.format(k, m) for m in missing_in_v1])
+            missing_in_d2.extend(['{0}.{1}'.format(k, m) for m in missing_in_v2])
+            for child_k, left, right in different_in_v:
+                different.append(('{0}.{1}'.format(k, child_k), left, right))
+            continue
+        if d1[k] != d2[k]:
+            different.append((k, d1[k], d2[k]))
+    missing_in_d1.extend(d2_keys - both)
+    missing_in_d2.extend(d1_keys - both)
+    return missing_in_d1, missing_in_d2, different
+
+
+def _describe_cmd_difference(exp, command):
+    if exp.args == command.args:
+        return
+
+    missing_in_exp, missing_in_cmd, diff = _dict_diff(exp.args, command.args)
+    if missing_in_exp:
+        print('Keys in cmd missing from expectation: {0}'.format(missing_in_exp))
+    if missing_in_cmd:
+        print('Keys in expectation missing from command: {0}'.format(missing_in_cmd))
+    if diff:
+        formatted_diff = ['"{0}": expected {1}, vs actual {2}'.format(*d) for d in diff]
+        print('Key differences between expectation and command: {0}'.format('\n'.join(formatted_diff)))
+
+
 class BuildStepMixin(object):
 
     """
@@ -68,6 +113,16 @@ def tearDownBuildStep(self):
         del remotecommand.FakeRemoteCommand.testcase
 
     # utilities
+    def _getSlaveCommandVersionWrapper(self):
+        originalGetSlaveCommandVersion = self.step.build.getSlaveCommandVersion
+
+        def getSlaveCommandVersion(cmd, oldversion):
+            if cmd == 'shell':
+                if hasattr(self, 'slaveShellCommandVersion'):
+                    return self.slaveShellCommandVersion
+            return originalGetSlaveCommandVersion(cmd, oldversion)
+
+        return getSlaveCommandVersion
 
     def setupStep(self, step, slave_version={'*': "99.99"}, slave_env={},
                   buildFiles=[], wantDefaultWorkdir=True, wantData=True,
@@ -231,6 +286,8 @@ def runStep(self):
 
         @returns: Deferred
         """
+        self.step.build.getSlaveCommandVersion = self._getSlaveCommandVersionWrapper()
+
         self.conn = mock.Mock(name="SlaveBuilder(connection)")
         self.step.setupProgress()
         d = self.step.startStep(self.conn)
@@ -276,6 +333,41 @@ def check(result):
 
     # callbacks from the running step
 
+    @defer.inlineCallbacks
+    def _validate_expectation(self, exp, command):
+        got = (command.remote_command, command.args)
+
+        for child_exp in exp.nestedExpectations():
+            try:
+                yield self._validate_expectation(child_exp, command)
+                exp.expectationPassed(exp)
+            except AssertionError as e:
+                # log this error, as the step may swallow the AssertionError or
+                # otherwise obscure the failure.  Trial will see the exception in
+                # the log and print an [ERROR].  This may result in
+                # double-reporting, but that's better than non-reporting!
+                log.err()
+                exp.raiseExpectationFailure(child_exp, e)
+
+        if exp.shouldAssertCommandEqualExpectation():
+            # handle any incomparable args
+            for arg in exp.incomparable_args:
+                self.failUnless(arg in got[1],
+                                "incomparable arg '%s' not received" % (arg,))
+                del got[1][arg]
+
+            # first check any ExpectedRemoteReference instances
+            try:
+                self.assertEqual((exp.remote_command, exp.args), got)
+            except AssertionError:
+                _describe_cmd_difference(exp, command)
+                raise
+
+        if exp.shouldRunBehaviors():
+            # let the Expect object show any behaviors that are required
+            yield exp.runBehaviors(command)
+
+    @defer.inlineCallbacks
     def _remotecommand_run(self, command, step, conn, builder_name):
         self.assertEqual(step, self.step)
         self.assertEqual(conn, self.conn)
@@ -285,26 +377,20 @@ def _remotecommand_run(self, command, step, conn, builder_name):
             self.fail("got command %r when no further commands were expected"
                       % (got,))
 
-        exp = self.expected_remote_commands.pop(0)
-
-        # handle any incomparable args
-        for arg in exp.incomparable_args:
-            self.failUnless(arg in got[1],
-                            "incomparable arg '%s' not received" % (arg,))
-            del got[1][arg]
+        exp = self.expected_remote_commands[0]
 
-        # first check any ExpectedRemoteReference instances
         try:
-            self.assertEqual((exp.remote_command, exp.args), got)
-        except AssertionError:
+            yield self._validate_expectation(exp, command)
+            exp.expectationPassed(exp)
+        except AssertionError as e:
             # log this error, as the step may swallow the AssertionError or
             # otherwise obscure the failure.  Trial will see the exception in
             # the log and print an [ERROR].  This may result in
             # double-reporting, but that's better than non-reporting!
             log.err()
-            raise
+            exp.raiseExpectationFailure(exp, e)
+        finally:
+            if not exp.shouldKeepMatchingAfter(command):
+                self.expected_remote_commands.pop(0)
 
-        # let the Expect object show any behaviors that are required
-        d = exp.runBehaviors(command)
-        d.addCallback(lambda _: command)
-        return d
+        defer.returnValue(command)