CP-49720 De-duplicate GC kick optimisation

Both LVHDSR and FileSR in their _kickGC() method would check to see if a
GC was already in progress and avoid starting one if so. This code was
duplicated and can be moved into the cleanup.start_gc() method.

Signed-off-by: Tim Smith <tim.smith@cloud.com>

Author: Tim Smith

drivers/FileSR.py

@@ -31,7 +31,7 @@
 import time
 import glob
 from uuid import uuid4
-from lock import Lock, LOCK_TYPE_GC_RUNNING
+from lock import Lock
 import xmlrpc.client
 import XenAPI # pylint: disable=import-error
 from constants import CBTLOG_TAG
@@ -378,31 +378,8 @@ def _process_replay(self, data):
             index += 1
 
     def _kickGC(self):
-        # don't bother if an instance already running (this is just an
-        # optimization to reduce the overhead of forking a new process if we
-        # don't have to, but the process will check the lock anyways)
-        lockRunning = Lock(LOCK_TYPE_GC_RUNNING, self.uuid)
-        if not lockRunning.acquireNoblock():
-            if cleanup.should_preempt(self.session, self.uuid):
-                util.SMlog("Aborting currently-running coalesce of garbage VDI")
-                try:
-                    if not cleanup.abort(self.uuid, soft=True):
-                        util.SMlog("The GC has already been scheduled to "
-                                "re-start")
-                except util.CommandException as e:
-                    if e.code != errno.ETIMEDOUT:
-                        raise
-                    util.SMlog('failed to abort the GC')
-                finally:
-                    return
-            else:
-                util.SMlog("A GC instance already running, not kicking")
-                return
-        else:
-            lockRunning.release()
-
         util.SMlog("Kicking GC")
-        cleanup.start_gc(self.uuid)
+        cleanup.start_gc(self.session, self.uuid)
 
     def _isbind(self):
         # os.path.ismount can't deal with bind mount

drivers/LVHDSR.py

@@ -36,7 +36,7 @@
 import cleanup
 import blktap2
 from journaler import Journaler
-from lock import Lock, LOCK_TYPE_GC_RUNNING
+from lock import Lock
 from refcounter import RefCounter
 from ipc import IPCFlag
 from lvmanager import LVActivator
@@ -1289,29 +1289,8 @@ def _prepareTestMode(self):
             util.SMlog("Setting env %s" % self.ENV_VAR_VHD_TEST[self.testMode])
 
     def _kickGC(self):
-        # don't bother if an instance already running (this is just an
-        # optimization to reduce the overhead of forking a new process if we
-        # don't have to, but the process will check the lock anyways)
-        lockRunning = Lock(LOCK_TYPE_GC_RUNNING, self.uuid)
-        if not lockRunning.acquireNoblock():
-            if cleanup.should_preempt(self.session, self.uuid):
-                util.SMlog("Aborting currently-running coalesce of garbage VDI")
-                try:
-                    if not cleanup.abort(self.uuid, soft=True):
-                        util.SMlog("The GC has already been scheduled to "
-                                "re-start")
-                except util.CommandException as e:
-                    if e.code != errno.ETIMEDOUT:
-                        raise
-                    util.SMlog('failed to abort the GC')
-            else:
-                util.SMlog("A GC instance already running, not kicking")
-                return
-        else:
-            lockRunning.release()
-
         util.SMlog("Kicking GC")
-        cleanup.start_gc(self.uuid)
+        cleanup.start_gc(self.session, self.uuid)
 
     def ensureCBTSpace(self):
         # Ensure we have space for at least one LV

drivers/cleanup.py

@@ -3205,7 +3205,27 @@ def gc(session, srUuid, inBackground, dryRun=False):
         _gc(session, srUuid, dryRun, immediate=True)
 
 
-def start_gc(sr_uuid):
+def start_gc(session, sr_uuid):
+    # don't bother if an instance already running (this is just an
+    # optimization to reduce the overhead of forking a new process if we
+    # don't have to, but the process will check the lock anyways)
+    lockRunning = lock.Lock(lock.LOCK_TYPE_GC_RUNNING, sr_uuid)
+    if not lockRunning.acquireNoblock():
+        if should_preempt(session, sr_uuid):
+            util.SMlog("Aborting currently-running coalesce of garbage VDI")
+            try:
+                if not abort(sr_uuid, soft=True):
+                    util.SMlog("The GC has already been scheduled to re-start")
+            except util.CommandException as e:
+                if e.code != errno.ETIMEDOUT:
+                    raise
+                util.SMlog('failed to abort the GC')
+        else:
+            util.SMlog("A GC instance already running, not kicking")
+            return
+    else:
+        lockRunning.release()
+
     util.SMlog(f"Starting GC file is {__file__}")
     subprocess.run([__file__, '-b', '-u', sr_uuid, '-g'],
                    stdout=subprocess.PIPE, stderr=subprocess.PIPE, close_fds=True)

tests/test_FileSR.py

@@ -450,6 +450,9 @@ def setUp(self):
         lock_patcher = mock.patch('FileSR.Lock')
         self.mock_lock = lock_patcher.start()
 
+        lock_patcher_cleanup = mock.patch('cleanup.lock.Lock')
+        self.mock_lock_cleanup = lock_patcher_cleanup.start()
+
         xapi_patcher = mock.patch('SR.XenAPI')
         self.mock_xapi = xapi_patcher.start()
         self.mock_session = mock.MagicMock()