This commit fixes a bug where a LogicalVolume would try to attach to a non-existing device and then raise an exception, rather than gracefully returning a negative result.
As a consequence, unit tests have also been introduced for Attach methods for block devices. Signed-off-by: Federico Morg Pareschi <[email protected]> --- lib/storage/bdev.py | 4 +- test/py/ganeti.storage.bdev_unittest.py | 85 +++++++++++++++++++++++++++ test/py/ganeti.storage.drbd_unittest.py | 18 ++++++ test/py/ganeti.storage.extstorage_unittest.py | 74 +++++++++++++++++++++++ 4 files changed, 180 insertions(+), 1 deletion(-) diff --git a/lib/storage/bdev.py b/lib/storage/bdev.py index ad6a796..1f95004 100644 --- a/lib/storage/bdev.py +++ b/lib/storage/bdev.py @@ -523,7 +523,9 @@ class LogicalVolume(base.BlockDev): """ self.attached = False if not lv_info: - lv_info = LogicalVolume._GetLvGlobalInfo()[self.dev_path] + lv_info = LogicalVolume._GetLvGlobalInfo().get(self.dev_path) + if not lv_info: + return False (status, major, minor, pe_size, stripes, pv_names) = lv_info self.major = major diff --git a/test/py/ganeti.storage.bdev_unittest.py b/test/py/ganeti.storage.bdev_unittest.py index deb7a85..a894e8f 100755 --- a/test/py/ganeti.storage.bdev_unittest.py +++ b/test/py/ganeti.storage.bdev_unittest.py @@ -52,6 +52,13 @@ def _FakeRunCmd(success, stdout, cmd): return utils.RunResult(exit_code, None, stdout, "", cmd, utils.process._TIMEOUT_NONE, 5) + +class FakeStatResult(object): + def __init__(self, st_mode): + self.st_mode = st_mode + self.st_rdev = 0 + + class TestRADOSBlockDevice(testutils.GanetiTestCase): """Tests for bdev.RADOSBlockDevice volumes @@ -191,6 +198,39 @@ class TestRADOSBlockDevice(testutils.GanetiTestCase): self.test_unique_id, [], 1024, None, self.test_params, True, {}) + @testutils.patch_object(bdev.RADOSBlockDevice, "_MapVolumeToBlockdev") + @testutils.patch_object(os, "stat") + def testAttach(self, stat_mock, map_mock): + """Test for bdev.RADOSBlockDevice.Attach()""" + stat_mock.return_value = FakeStatResult(0x6000) # bitmask for S_ISBLK + map_mock.return_value = "/fake/path" + dev = bdev.RADOSBlockDevice.__new__(bdev.RADOSBlockDevice) + dev.unique_id = self.test_unique_id + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(bdev.RADOSBlockDevice, "_MapVolumeToBlockdev") + @testutils.patch_object(os, "stat") + def testAttachFailureNotBlockdev(self, stat_mock, map_mock): + """Test for bdev.RADOSBlockDevice.Attach() failure, not a blockdev""" + stat_mock.return_value = FakeStatResult(0x0) + map_mock.return_value = "/fake/path" + dev = bdev.RADOSBlockDevice.__new__(bdev.RADOSBlockDevice) + dev.unique_id = self.test_unique_id + + self.assertEqual(dev.Attach(), False) + + @testutils.patch_object(bdev.RADOSBlockDevice, "_MapVolumeToBlockdev") + @testutils.patch_object(os, "stat") + def testAttachFailureNoDevice(self, stat_mock, map_mock): + """Test for bdev.RADOSBlockDevice.Attach() failure, no device found""" + stat_mock.side_effect = OSError("No device found") + map_mock.return_value = "/fake/path" + dev = bdev.RADOSBlockDevice.__new__(bdev.RADOSBlockDevice) + dev.unique_id = self.test_unique_id + + self.assertEqual(dev.Attach(), False) + class TestExclusiveStoragePvs(unittest.TestCase): """Test cases for functions dealing with LVM PV and exclusive storage""" @@ -524,6 +564,24 @@ class TestLogicalVolume(testutils.GanetiTestCase): self.test_unique_id, [], 1024, None, self.test_params, False, {}) + @testutils.patch_object(bdev.LogicalVolume, "_GetLvGlobalInfo") + def testAttach(self, info_mock): + """Test for bdev.LogicalVolume.Attach()""" + info_mock.return_value = {"/dev/fake/path": ("v", 1, 0, 1024, 0, ["test"])} + dev = bdev.LogicalVolume.__new__(bdev.LogicalVolume) + dev.dev_path = "/dev/fake/path" + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(bdev.LogicalVolume, "_GetLvGlobalInfo") + def testAttachFalse(self, info_mock): + """Test for bdev.LogicalVolume.Attach() with missing lv_info""" + info_mock.return_value = {} + dev = bdev.LogicalVolume.__new__(bdev.LogicalVolume) + dev.dev_path = "/dev/fake/path" + + self.assertEqual(dev.Attach(), False) + class TestPersistentBlockDevice(testutils.GanetiTestCase): """Tests for bdev.PersistentBlockDevice volumes @@ -560,6 +618,33 @@ class TestPersistentBlockDevice(testutils.GanetiTestCase): self.assertRaises(errors.ProgrammerError, bdev.PersistentBlockDevice.Create, self.test_unique_id, [], 1024, None, {}, True, {}) + @testutils.patch_object(os, "stat") + def testAttach(self, stat_mock): + """Test for bdev.PersistentBlockDevice.Attach()""" + stat_mock.return_value = FakeStatResult(0x6000) # bitmask for S_ISBLK + dev = bdev.PersistentBlockDevice.__new__(bdev.PersistentBlockDevice) + dev.dev_path = "/dev/fake/path" + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(os, "stat") + def testAttachFailureNotBlockdev(self, stat_mock): + """Test for bdev.PersistentBlockDevice.Attach() failure, not a blockdev""" + stat_mock.return_value = FakeStatResult(0x0) + dev = bdev.PersistentBlockDevice.__new__(bdev.PersistentBlockDevice) + dev.dev_path = "/dev/fake/path" + + self.assertEqual(dev.Attach(), False) + + @testutils.patch_object(os, "stat") + def testAttachFailureNoDevice(self, stat_mock): + """Test for bdev.PersistentBlockDevice.Attach() failure, no device found""" + stat_mock.side_effect = OSError("No device found") + dev = bdev.PersistentBlockDevice.__new__(bdev.PersistentBlockDevice) + dev.dev_path = "/dev/fake/path" + + self.assertEqual(dev.Attach(), False) + if __name__ == "__main__": testutils.GanetiTestProgram() diff --git a/test/py/ganeti.storage.drbd_unittest.py b/test/py/ganeti.storage.drbd_unittest.py index 6ac2876..902cc1b 100755 --- a/test/py/ganeti.storage.drbd_unittest.py +++ b/test/py/ganeti.storage.drbd_unittest.py @@ -556,6 +556,24 @@ class TestDRBD8Create(testutils.GanetiTestCase): self.test_unique_id, self.children, 123, None, {}, False, self.test_dyn_params) + @testutils.patch_object(drbd.DRBD8, "GetUsedDevs") + def testAttach(self, drbd_mock): + """Test for drbd.DRBD8Dev.Attach()""" + drbd_mock.return_value = [1] + dev = drbd.DRBD8Dev.__new__(drbd.DRBD8Dev) + dev._aminor = 1 + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(drbd.DRBD8, "GetUsedDevs") + def testAttach(self, drbd_mock): + """Test for drbd.DRBD8Dev.Attach() not finding a minor""" + drbd_mock.return_value = [] + dev = drbd.DRBD8Dev.__new__(drbd.DRBD8Dev) + dev._aminor = 1 + + self.assertEqual(dev.Attach(), False) + if __name__ == "__main__": testutils.GanetiTestProgram() diff --git a/test/py/ganeti.storage.extstorage_unittest.py b/test/py/ganeti.storage.extstorage_unittest.py index b8d2fd4..a888d10 100755 --- a/test/py/ganeti.storage.extstorage_unittest.py +++ b/test/py/ganeti.storage.extstorage_unittest.py @@ -31,12 +31,20 @@ """Script for unittesting the extstorage module""" +import os + from ganeti import errors from ganeti.storage import extstorage import testutils +class FakeStatResult(object): + def __init__(self, st_mode): + self.st_mode = st_mode + self.st_rdev = 0 + + class TestExtStorageDevice(testutils.GanetiTestCase): """Testing case for extstorage.ExtStorageDevice""" @@ -67,6 +75,72 @@ class TestExtStorageDevice(testutils.GanetiTestCase): self.test_unique_id, [], 123, None, {}, True, {}, name=self.name, uuid=self.uuid) + @testutils.patch_object(extstorage.ExtStorageDevice, "_ExtStorageAction") + @testutils.patch_object(os, "stat") + def testAttach(self, stat_mock, action_mock): + """Test for extstorage.ExtStorageDevice.Attach()""" + stat_mock.return_value = FakeStatResult(0x6000) # bitmask for S_ISBLK + action_mock.return_value = "/dev/path\nURI" + dev = extstorage.ExtStorageDevice.__new__(extstorage.ExtStorageDevice) + dev.unique_id = self.test_unique_id + dev.ext_params = {} + dev.name = self.name + dev.uuid = self.uuid + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(extstorage.ExtStorageDevice, "_ExtStorageAction") + def testAttachNoUserspaceURI(self, action_mock): + """Test for extstorage.ExtStorageDevice.Attach() with no userspace URI""" + action_mock.return_value = "" + dev = extstorage.ExtStorageDevice.__new__(extstorage.ExtStorageDevice) + dev.unique_id = self.test_unique_id + dev.ext_params = {} + dev.name = self.name + dev.uuid = self.uuid + + self.assertEqual(dev.Attach(), False) + + @testutils.patch_object(extstorage.ExtStorageDevice, "_ExtStorageAction") + def testAttachWithUserspaceURI(self, action_mock): + """Test for extstorage.ExtStorageDevice.Attach() with userspace URI""" + action_mock.return_value = "\nURI" + dev = extstorage.ExtStorageDevice.__new__(extstorage.ExtStorageDevice) + dev.unique_id = self.test_unique_id + dev.ext_params = {} + dev.name = self.name + dev.uuid = self.uuid + + self.assertEqual(dev.Attach(), True) + + @testutils.patch_object(extstorage.ExtStorageDevice, "_ExtStorageAction") + @testutils.patch_object(os, "stat") + def testAttachFailureNotBlockdev(self, stat_mock, action_mock): + """Test for extstorage.ExtStorageDevice.Attach() failure, not a blockdev""" + stat_mock.return_value = FakeStatResult(0x0) + action_mock.return_value = "/dev/path\nURI" + dev = extstorage.ExtStorageDevice.__new__(extstorage.ExtStorageDevice) + dev.unique_id = self.test_unique_id + dev.ext_params = {} + dev.name = self.name + dev.uuid = self.uuid + + self.assertEqual(dev.Attach(), False) + + @testutils.patch_object(extstorage.ExtStorageDevice, "_ExtStorageAction") + @testutils.patch_object(os, "stat") + def testAttachFailureNoDevice(self, stat_mock, action_mock): + """Test for extstorage.ExtStorageDevice.Attach() failure, no device found""" + stat_mock.side_effect = OSError("No device found") + action_mock.return_value = "/dev/path\nURI" + dev = extstorage.ExtStorageDevice.__new__(extstorage.ExtStorageDevice) + dev.unique_id = self.test_unique_id + dev.ext_params = {} + dev.name = self.name + dev.uuid = self.uuid + + self.assertEqual(dev.Attach(), False) + if __name__ == "__main__": testutils.GanetiTestProgram() -- 2.8.0.rc3.226.g39d4020
