From 54e17e9d636bb1f53c03c93537f03c9ff959ee2b Mon Sep 17 00:00:00 2001 From: Mark Syms Date: Mon, 13 May 2024 13:22:54 +0100 Subject: [PATCH 1/2] CA-392823: ensure no device mapper conflicts in LVHDSR detach Within the detach operation checks are made that none of the device mapper devices for the SR volume group have opern handles. The falls false if operations on other LVM srs happen to call a device mapper operation such as `lvs` or `vgs` at the point the check is made as these operations can result in all of the devices managed by the device mapper subsystem being accessed as part of the scan. this then gives a false response for the open file handles check. Signed-off-by: Mark Syms --- drivers/LVHDSR.py | 17 ++++++++++------- tests/test_LVHDSR.py | 30 +++++++++++++++++++++++++++++- 2 files changed, 39 insertions(+), 8 deletions(-) diff --git a/drivers/LVHDSR.py b/drivers/LVHDSR.py index 21441e365..f15c92d8f 100755 --- a/drivers/LVHDSR.py +++ b/drivers/LVHDSR.py @@ -51,6 +51,7 @@ from xmlrpc.client import DateTime import glob from constants import CBTLOG_TAG +from fairlock import Fairlock DEV_MAPPER_ROOT = os.path.join('/dev/mapper', lvhdutil.VG_PREFIX) geneology = {} @@ -601,13 +602,15 @@ def detach(self, uuid): if util.extractSRFromDevMapper(fileName) != self.uuid: continue - # check if any file has open handles - if util.doesFileHaveOpenHandles(fileName): - # if yes, log this and signal failure - util.SMlog("LVHDSR.detach: The dev mapper entry %s has open " \ - "handles" % fileName) - success = False - continue + with Fairlock('devicemapper'): + # check if any file has open handles + if util.doesFileHaveOpenHandles(fileName): + # if yes, log this and signal failure + util.SMlog( + f"LVHDSR.detach: The dev mapper entry {fileName} has " + "open handles") + success = False + continue # Now attempt to remove the dev mapper entry if not lvutil.removeDevMapperEntry(fileName, False): diff --git a/tests/test_LVHDSR.py b/tests/test_LVHDSR.py index da786b2d1..94a390d3f 100644 --- a/tests/test_LVHDSR.py +++ b/tests/test_LVHDSR.py @@ -104,7 +104,7 @@ def test_undoAllInflateJournals( @mock.patch('LVHDSR.Lock', autospec=True) @mock.patch('SR.XenAPI') @testlib.with_context - def test_attach_success(self, + def test_srlifecycle_success(self, context, mock_xenapi, mock_lock, @@ -196,7 +196,35 @@ def get_vdi_by_uuid(vdi_uuid): # Arrange (2) sr = self.create_LVHDSR(master=True, command='sr_detach', sr_uuid=sr_uuid) + + # Arrange for detach + self.stubout('LVHDSR.Fairlock') + mock_remove_device = self.stubout( + 'LVHDSR.lvutil.removeDevMapperEntry') + mock_glob = self.stubout('glob.glob') + mock_vdi_uuid = "72101dbd-bd62-4a14-a03c-afca8cceec86" + mock_filepath = os.path.join( + '/dev/mapper/', 'VG_XenStorage' + f'--{sr_uuid.replace("-", "--")}-' + f'{mock_vdi_uuid.replace("-", "--")}') + mock_glob.return_value = [mock_filepath] + mock_open_handles = self.stubout( + 'LVHDSR.util.doesFileHaveOpenHandles') + + # Act (Detach) + with self.assertRaises(Exception): + # Fail the first one with busy handles + mock_open_handles.return_value = True + sr.detach(sr.uuid) + + # Now succeed + mock_open_handles.return_value = False sr.detach(sr.uuid) + + # Assert for detach + mock_remove_device.assert_called_once_with(mock_filepath, False) + + # Create new SR mock_lvm_cache.return_value.checkLV.return_value = True sr = self.create_LVHDSR(master=True, command='sr_attach', sr_uuid=sr_uuid) From 293fb74515c34ad2c2c34c9a3efdf1749cc9708d Mon Sep 17 00:00:00 2001 From: Mark Syms Date: Tue, 14 May 2024 09:07:35 +0100 Subject: [PATCH 2/2] CA-392823: pid_is_alive expects pid to be an int. Signed-off-by: Mark Syms --- drivers/util.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/util.py b/drivers/util.py index f6b9ba8d8..8fa80da4a 100755 --- a/drivers/util.py +++ b/drivers/util.py @@ -1496,7 +1496,7 @@ def findRunningProcessOrOpenFile(name, process=True): # There is no specific guarantee of when a PID's /proc directory will disappear # when it exits, particularly relative to filedescriptor cleanup, so we want to # make sure we're not reporting a false positive. - processandpids = [ x for x in processandpids if pid_is_alive(x[1]) ] + processandpids = [x for x in processandpids if pid_is_alive(int(x[1]))] for pp in processandpids: SMlog(f"File {name} has an open handle with process {pp[0]} with pid {pp[1]}")