Merge pull request #7250 from p12tic/buildstep-fix-locks-when-skipped

process: Fix lock handling when step is skipped

master/buildbot/process/buildstep.py

@@ -264,6 +264,7 @@ class BuildStep(results.ResultComputingConfigMixin,
     descriptionSuffix = None  # extra information to append to suffix
     updateBuildSummaryPolicy = None
     locks = []
+    _locks_to_acquire = []
     progressMetrics = ()  # 'time' is implicit
     useProgress = True  # set to False if step is really unpredictable
     build = None
@@ -600,19 +601,20 @@ def startStep(self, remote):
     @defer.inlineCallbacks
     def _setup_locks(self):
 
-        self.locks = yield self.build.render(self.locks)
+        locks = yield self.build.render(self.locks)
 
         # convert all locks into their real form
         botmaster = self.build.builder.botmaster
-        self.locks = yield botmaster.getLockFromLockAccesses(self.locks, self.build.config_version)
+        locks = yield botmaster.getLockFromLockAccesses(locks, self.build.config_version)
 
         # then narrow WorkerLocks down to the worker that this build is being
         # run on
-        self.locks = [(l.getLockForWorker(self.build.workerforbuilder.worker.workername),
-                       la)
-                      for l, la in self.locks]
+        self._locks_to_acquire = [
+            (l.getLockForWorker(self.build.workerforbuilder.worker.workername), la)
+            for l, la in locks
+        ]
 
-        for l, _ in self.locks:
+        for l, _ in self._locks_to_acquire:
             if l in self.build.locks:
                 log.msg(f"Hey, lock {l} is claimed by both a Step ({self}) and the"
                         f" parent Build ({self.build})")
@@ -677,12 +679,12 @@ def addTestResult(self, setid, value, test_name=None, test_code_path=None, line=
                                                             line=line, duration_ns=duration_ns)
 
     def acquireLocks(self, res=None):
-        if not self.locks:
+        if not self._locks_to_acquire:
             return defer.succeed(None)
         if self.stopped:
             return defer.succeed(None)
-        log.msg(f"acquireLocks(step {self}, locks {self.locks})")
-        for lock, access in self.locks:
+        log.msg(f"acquireLocks(step {self}, locks {self._locks_to_acquire})")
+        for lock, access in self._locks_to_acquire:
             for waited_lock, _, _ in self._acquiringLocks:
                 if lock is waited_lock:
                     continue
@@ -695,7 +697,7 @@ def acquireLocks(self, res=None):
                 d.addCallback(self.acquireLocks)
                 return d
         # all locks are available, claim them all
-        for lock, access in self.locks:
+        for lock, access in self._locks_to_acquire:
             lock.claim(self, access)
         self._acquiringLocks = []
         self._waitingForLocks = False
@@ -743,8 +745,8 @@ def _interrupt_impl(self, reason):
         yield self._maybe_interrupt_cmd(reason)
 
     def releaseLocks(self):
-        log.msg(f"releaseLocks({self}): {self.locks}")
-        for lock, access in self.locks:
+        log.msg(f"releaseLocks({self}): {self._locks_to_acquire}")
+        for lock, access in self._locks_to_acquire:
             if lock.isOwner(self, access):
                 lock.release(self, access)
             else:

master/buildbot/test/unit/process/test_buildstep.py

@@ -300,38 +300,67 @@ def test_regularLocks(self):
         self.assertIn('workername', real_worker_lock.locks)
         self.assertTrue(real_worker_lock.locks['workername'].isAvailable(self, lock_accesses[1]))
 
+    @defer.inlineCallbacks
+    def test_regular_locks_skip_step(self):
+        # BuildStep should not try to acquire locks when it's skipped
+        lock = locks.MasterLock("masterlock")
+        lock_access = locks.LockAccess(lock, "exclusive")
+
+        self.setup_step(buildstep.BuildStep(
+            locks=[locks.LockAccess(lock, "counting")],
+            doStepIf=False
+        ))
+
+        real_lock = yield self.build.builder.botmaster.getLockByID(lock, 0)
+        real_lock.claim(self, lock_access)
+
+        self.expect_outcome(result=SKIPPED)
+        yield self.run_step()
+
     @defer.inlineCallbacks
     def test_cancelWhileLocksAvailable(self):
 
         def _owns_lock(step, lock):
-            access = [step_access for step_lock, step_access in step.locks if step_lock == lock][0]
+            access = [
+                step_access for step_lock, step_access in step._locks_to_acquire
+                if step_lock == lock
+            ][0]
             return lock.isOwner(step, access)
 
         def _lock_available(step, lock):
-            access = [step_access for step_lock, step_access in step.locks if step_lock == lock][0]
+            access = [
+                step_access for step_lock, step_access in step._locks_to_acquire
+                if step_lock == lock
+            ][0]
             return lock.isAvailable(step, access)
 
         lock1 = locks.MasterLock("masterlock1")
-        real_lock1 = locks.RealMasterLock(lock1)
         lock2 = locks.MasterLock("masterlock2")
-        real_lock2 = locks.RealMasterLock(lock2)
 
         stepa = self.setup_step(self.FakeBuildStep(locks=[
-            (real_lock1, locks.LockAccess(lock1, 'exclusive'))
+            locks.LockAccess(lock1, 'exclusive')
         ]))
         stepb = self.setup_step(self.FakeBuildStep(locks=[
-            (real_lock2, locks.LockAccess(lock2, 'exclusive'))
+            locks.LockAccess(lock2, 'exclusive')
         ]))
 
         stepc = self.setup_step(self.FakeBuildStep(locks=[
-            (real_lock1, locks.LockAccess(lock1, 'exclusive')),
-            (real_lock2, locks.LockAccess(lock2, 'exclusive'))
+            locks.LockAccess(lock1, 'exclusive'),
+            locks.LockAccess(lock2, 'exclusive')
         ]))
         stepd = self.setup_step(self.FakeBuildStep(locks=[
-            (real_lock1, locks.LockAccess(lock1, 'exclusive')),
-            (real_lock2, locks.LockAccess(lock2, 'exclusive'))
+            locks.LockAccess(lock1, 'exclusive'),
+            locks.LockAccess(lock2, 'exclusive')
         ]))
 
+        real_lock1 = yield self.build.builder.botmaster.getLockByID(lock1, 0)
+        real_lock2 = yield self.build.builder.botmaster.getLockByID(lock2, 0)
+
+        yield stepa._setup_locks()
+        yield stepb._setup_locks()
+        yield stepc._setup_locks()
+        yield stepd._setup_locks()
+
         # Start all the steps
         yield stepa.acquireLocks()
         yield stepb.acquireLocks()