Sync up

libs/sm/cleanup.py

@@ -2316,7 +2316,6 @@ def _doCoalesceLeaf(self, vdi):
         # garbage
 
         # update the VDI record
-        vdi.parent.delConfig(VDI.DB_VHD_PARENT)
         if vdi.parent.raw:
             vdi.parent.setConfig(VDI.DB_VDI_TYPE, vhdutil.VDI_TYPE_RAW)
         vdi.parent.delConfig(VDI.DB_VHD_BLOCKS)
@@ -2331,6 +2330,9 @@ def _doCoalesceLeaf(self, vdi):
         vdi.parent.children = []
         vdi.parent = None
 
+        if parent.parent is None:
+            parent.delConfig(VDI.DB_VHD_PARENT)
+
         extraSpace = self._calcExtraSpaceNeeded(vdi, parent)
         freeSpace = self.getFreeSpace()
         if freeSpace < extraSpace:
@@ -2633,10 +2635,12 @@ def _finishInterruptedCoalesceLeaf(self, childUuid, parentUuid):
         Util.log("*** FINISH LEAF-COALESCE")
         vdi = self.getVDI(childUuid)
         if not vdi:
+            Util.log(f"_finishInterruptedCoalesceLeaf, vdi {childUuid} not found, aborting")
             raise util.SMException("VDI %s not found" % childUuid)
         try:
             self.forgetVDI(parentUuid)
         except XenAPI.Failure:
+            Util.logException('_finishInterruptedCoalesceLeaf')
             pass
         self._updateSlavesOnResize(vdi)
         util.fistpoint.activate("LVHDRT_coaleaf_finish_end", self.uuid)

libs/sm/core/XE_SR_ERRORCODES.xml

@@ -513,6 +513,12 @@
                 <value>117</value>
         </code>
 
+        <code>
+                <name>PVMultiIDs</name>
+                <description>PVs found with multiple SCSI IDs</description>
+                <value>119</value>
+        </code>
+
        <!-- Agent database query errors 150+ -->
         <code>
                 <name>APISession</name>

libs/sm/drivers/LVHDoISCSISR.py

@@ -471,11 +471,21 @@ def attach(self, sr_uuid):
                     scsiutil.rescan([self.iscsi.adapter[a]])
 
             self._pathrefresh(LVHDoISCSISR)
+
+            # Check that we only have PVs for the volume group with the expected SCSI ID
+            lvutil.checkPVScsiIds(self.vgname, self.SCSIid)
+
             LVHDSR.LVHDSR.attach(self, sr_uuid)
         except Exception as inst:
             for i in self.iscsiSRs:
                 i.detach(sr_uuid)
+
+            # If we already have a proper error just raise it
+            if isinstance(inst, xs_errors.SROSError):
+                raise
+
             raise xs_errors.XenError("SRUnavailable", opterr=inst)
+
         self._setMultipathableFlag(SCSIid=self.SCSIid)
 
     def detach(self, sr_uuid):

libs/sm/lvutil.py

@@ -23,6 +23,7 @@
 import time
 
 from fairlock import Fairlock
+from sm.core import scsiutil
 from sm.core import util
 from sm.core import xs_errors
 import xml.dom.minidom
@@ -613,6 +614,19 @@ def setActiveVG(path, active):
     text = cmd_lvm([CMD_VGCHANGE, "-a" + val, path])
 
 
+def checkPVScsiIds(vgname, SCSIid):
+    # Get all the PVs for the specified vgName even if not active
+    cmd = [CMD_PVS, '-a', '--select', f'vgname={vgname}', '--no-headings']
+    text = cmd_lvm(cmd)
+    pv_paths = [x.split()[0] for x in text.splitlines()]
+    for pv_path in pv_paths:
+        pv_scsi_id = scsiutil.getSCSIid(pv_path)
+        if pv_scsi_id != SCSIid:
+            raise xs_errors.XenError(
+                'PVMultiIDs',
+                opterr=f'Found PVs {",".join(pv_paths)} and unexpected '
+                       f'SCSI ID {pv_scsi_id}, expected {SCSIid}')
+
 @lvmretry
 def create(name, size, vgname, tag=None, size_in_percentage=None):
     if size_in_percentage:
@@ -728,13 +742,6 @@ def deactivateNoRefcount(path):
 
 @lvmretry
 def _deactivate(path):
-    # Records what is using the LVM path in case there is an issue.
-    # In most cases this should be nothing.
-    try:
-        (rc, stdout, stderr) = util.doexec(['/usr/sbin/fuser', "-v", path])
-        util.SMlog(f"fuser {path} => {rc} / '{stdout}' / '{stderr}'")
-    except:
-        pass
     text = cmd_lvm([CMD_LVCHANGE, "-an", path])
 
 

tests/test_LVHDoISCSISR.py

@@ -159,6 +159,9 @@ def deepcopy(to_copy):
         lvlock_patcher = mock.patch('sm.drivers.LVHDSR.lvutil.Fairlock')
         self.mock_lvlock = lvlock_patcher.start()
 
+        pv_check_patcher = mock.patch('sm.drivers.LVHDoISCSISR.lvutil.checkPVScsiIds', autospec=True)
+        self.mock_pv_check = pv_check_patcher.start()
+
         self.addCleanup(mock.patch.stopall)
 
         super().setUp()

tests/test_cleanup.py

@@ -7,14 +7,19 @@
 from uuid import uuid4
 
 from sm import cleanup
+from sm import fjournaler
 from sm.core import lock
+from sm.core.util import SMException
 
 from sm.core import util
 from sm import vhdutil
 
 from sm import ipc
 
 import XenAPI
+from XenAPI import Failure
+
+MEGA = 1024 * 1024
 
 
 class FakeFile(object):
@@ -1815,6 +1820,46 @@ def test_not_plugged_retry(self):
         # Assert
         self.assertIsNotNone(sr)
 
+    def test_doCoalesceLeaf_no_parent_after(self):
+        sr = create_cleanup_sr(self.xapi_mock)
+        sr.journaler = mock.create_autospec(fjournaler.Journaler)
+        parent_uuid = str(uuid.uuid4())
+        mock_parent = mock.MagicMock()
+        mock_parent.uuid = parent_uuid
+        mock_parent.parent = None
+        mock_parent.raw = False
+        mock_parent.getSizeVHD.return_value = 10 * MEGA
+
+        vdi_uuid = str(uuid.uuid4())
+        mock_vdi = mock.MagicMock()
+        mock_vdi.uuid = vdi_uuid
+        mock_vdi.getSizeVHD.return_value = 10 * MEGA
+        mock_vdi.parent = mock_parent
+
+        sr._doCoalesceLeaf(mock_vdi)
+
+        mock_parent.delConfig.assert_called_with("vhd-parent")
+
+    def test_doCoalesceLeaf_parent_remains_after(self):
+        sr = create_cleanup_sr(self.xapi_mock)
+        sr.journaler = mock.create_autospec(fjournaler.Journaler)
+        parent_uuid = str(uuid.uuid4())
+        mock_parent = mock.MagicMock()
+        mock_parent.uuid = parent_uuid
+        mock_parent.parent = mock.MagicMock()
+        mock_parent.raw = False
+        mock_parent.getSizeVHD.return_value = 10 * MEGA
+
+        vdi_uuid = str(uuid.uuid4())
+        mock_vdi = mock.MagicMock()
+        mock_vdi.uuid = vdi_uuid
+        mock_vdi.getSizeVHD.return_value = 10 * MEGA
+        mock_vdi.parent = mock_parent
+
+        sr._doCoalesceLeaf(mock_vdi)
+
+        self.assertNotIn('vhd-parent', mock_parent.delConfig.call_args)
+
 
 class TestLockGCActive(unittest.TestCase):
 
@@ -1919,3 +1964,46 @@ def test_cannot_acquire_if_other_process_holds_sr_lock(self):
         # When
         with self.assertRaises(BlockingIOError):
             gcLock.acquireNoblock()
+
+
+class TestFileSR(TestSR):
+    """
+    Exercise the specfic bits of the FileSR support
+    """
+
+    def setUp(self):
+        super().setUp()
+
+        self.sr_uuid = str(uuid.uuid4())
+
+    def _make_test_sr(self):
+        self.mock_sr =  cleanup.FileSR(self.sr_uuid, self.xapi_mock,
+                                       createLock=False, force=False)
+
+    def test_finishInterruptedCoalesceLeaf_no_vdi(self):
+        self._make_test_sr()
+        self.mock_sr.vdis = {}
+
+        child_vdi_uuid = str(uuid.uuid4())
+        parent_vdi_uuid = str(uuid.uuid4())
+
+        with self.assertRaises(SMException) as sme:
+            self.mock_sr._finishInterruptedCoalesceLeaf(child_vdi_uuid, parent_vdi_uuid)
+
+        self.assertRegex(str(sme.exception), r"VDI \S+ not found")
+
+    def test_finishInterruptedCoalesceLeaf_forget_fail_and_complete(self):
+        child_vdi_uuid = str(uuid.uuid4())
+        parent_vdi_uuid = str(uuid.uuid4())
+
+        self._make_test_sr()
+        self.mock_sr.vdis = {
+            child_vdi_uuid: mock.create_autospec(cleanup.FileVDI)
+        }
+
+        def forgetVdi(self, uuid):
+            raise Failure(f"ForgetVDI {uuid} failed")
+
+        self.xapi_mock.forgetVDI.side_effect = forgetVdi
+
+        self.mock_sr._finishInterruptedCoalesceLeaf(child_vdi_uuid, parent_vdi_uuid)

tests/test_lvutil.py

@@ -6,6 +6,7 @@
 import testlib
 import lvmlib
 from sm.core import util
+from sm.core import xs_errors
 
 from sm import lvutil
 
@@ -394,6 +395,27 @@ def test_command_error(self, mock_smlog, mock_cmd_lvm):
         ])
         mock_smlog.assert_called_with("PVs in VG vg1: []")
 
+    @mock.patch('sm.core.xs_errors.XML_DEFS', 'libs/sm/core/XE_SR_ERRORCODES.xml')
+    @mock.patch('sm.lvutil.scsiutil.getSCSIid', autospec=True)
+    def test_check_PV_SCSI_IDs_failure(self, mock_get_scsi_id, mock_smlog, mock_cmd_lvm):
+        mock_cmd_lvm.return_value = """  /dev/disk/by-id/scsi-360014057b7f26e2962843fa8a0645fbd VG_XenStorage-401d198b-60ab-1f21-1359-bd4f127b8f38 lvm2 d--       0      0
+  /dev/disk/by-id/scsi-36001405fdd436fa7f854cd685fc3b1fd VG_XenStorage-401d198b-60ab-1f21-1359-bd4f127b8f38 lvm2 a--  <49.99g 49.98g
+"""  # noqa: E501
+        mock_get_scsi_id.side_effect = ['36001405fdd436fa7f854cd685fc3b1fd',
+                                        '360014057b7f26e2962843fa8a0645fbd']
+        with self.assertRaises(xs_errors.SROSError) as srose:
+            lvutil.checkPVScsiIds('VG_XenStorage-401d198b-60ab-1f21-1359-bd4f127b8f38',
+                                  '36001405fdd436fa7f854cd685fc3b1fd')
+        self.assertEqual(119, srose.exception.errno)
+
+    @mock.patch('sm.lvutil.scsiutil.getSCSIid', autospec=True)
+    def test_check_PV_SCSI_IDs_success(self, mock_get_scsi_id, mock_smlog, mock_cmd_lvm):
+        mock_cmd_lvm.return_value = """  /dev/disk/by-id/scsi-360014057b7f26e2962843fa8a0645fbd VG_XenStorage-401d198b-60ab-1f21-1359-bd4f127b8f38 lvm2 d--       0      0"""  # noqa: E501
+        mock_get_scsi_id.side_effect = ['36001405fdd436fa7f854cd685fc3b1fd']
+        lvutil.checkPVScsiIds('VG_XenStorage-401d198b-60ab-1f21-1359-bd4f127b8f38',
+                              '36001405fdd436fa7f854cd685fc3b1fd')
+
+
 @mock.patch('sm.lvutil.cmd_lvm')
 @mock.patch('sm.core.util.SMlog', autospec=True)
 class TestGetPVsWithUUID(unittest.TestCase):
@@ -431,3 +453,4 @@ def test_command_error(self, mock_smlog, mock_cmd_lvm):
             mock.call("Warning: Invalid or empty line in pvs output: Invalid return value."),
             mock.call("PVs with uuid uuid1: []")
         ])
+