From ef3fe0233e21a4e23fe7ebd2619e936dcc06bd27 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Tue, 12 May 2020 10:47:07 +0100 Subject: [PATCH 1/3] CP-33617 Group non-function lines under __main__ Signed-off-by: Tim Smith --- drivers/mpathcount.py | 208 +++++++++++++++++++++--------------------- 1 file changed, 105 insertions(+), 103 deletions(-) diff --git a/drivers/mpathcount.py b/drivers/mpathcount.py index 091095497..84649ade0 100755 --- a/drivers/mpathcount.py +++ b/drivers/mpathcount.py @@ -24,24 +24,6 @@ import glob import json -supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp','lvmofcoe', 'gfs2'] - -LOCK_TYPE_HOST = "host" -LOCK_NS1 = "mpathcount1" -LOCK_NS2 = "mpathcount2" - -MAPPER_DIR = "/dev/mapper" -mpp_path_update = False -match_bySCSIid = False -mpath_enabled = True - -if len(sys.argv) == 3: - match_bySCSIid = True - SCSIid = sys.argv[1] - mpp_path_update = True - mpp_entry = sys.argv[2] - -cached_DM_maj = None def get_dm_major(): global cached_DM_maj if not cached_DM_maj: @@ -162,90 +144,110 @@ def get_SCSIidlist(devconfig, sm_config): SCSIidlist.append(re.sub("^scsi-","",key)) return SCSIidlist -try: - session = util.get_localAPI_session() -except: - print "Unable to open local XAPI session" - sys.exit(-1) - -localhost = session.xenapi.host.get_by_uuid(get_localhost_uuid()) -# Check whether multipathing is enabled (either for root dev or SRs) -try: - if get_root_dev_major() != get_dm_major(): - hconf = session.xenapi.host.get_other_config(localhost) - assert(hconf['multipathing'] == 'true') - mpath_enabled = True -except: - mpath_enabled = False - -# Check root disk if multipathed -try: - if get_root_dev_major() == get_dm_major(): - def _remove(key): - session.xenapi.host.remove_from_other_config(localhost,key) - def _add(key, val): - session.xenapi.host.add_to_other_config(localhost,key,val) - config = session.xenapi.host.get_other_config(localhost) - maps = mpath_cli.list_maps() - # Ensure output headers are not in the list - if 'name' in maps: - maps.remove('name') - # first map will always correspond to the root dev, dm-0 - assert(len(maps) > 0) - i = maps[0] - if (not match_bySCSIid) or i == SCSIid: - util.SMlog("Matched SCSIid %s, updating " \ - " Host.other-config:mpath-boot " % i) - key="mpath-boot" - if not config.has_key(key): - update_config(key, i, "", _remove, _add) - else: - update_config(key, i, config[key], _remove, _add) -except: - util.SMlog("MPATH: Failure updating Host.other-config:mpath-boot db") - mpc_exit(session, -1) - -try: - pbds = session.xenapi.PBD.get_all_records_where("field \"host\" = \"%s\"" % localhost) -except: - mpc_exit(session,-1) - -try: - for pbd in pbds: - def remove(key): - session.xenapi.PBD.remove_from_other_config(pbd,key) - def add(key, val): - session.xenapi.PBD.add_to_other_config(pbd,key,val) - record = pbds[pbd] - config = record['other_config'] - SR = record['SR'] - srtype = session.xenapi.SR.get_type(SR) - if srtype in supported: - devconfig = record["device_config"] - sm_config = session.xenapi.SR.get_sm_config(SR) - SCSIidlist = get_SCSIidlist(devconfig, sm_config) - if not len(SCSIidlist): - continue - for i in SCSIidlist: - if match_bySCSIid and i != SCSIid: - continue - util.SMlog("Matched SCSIid, updating %s" % i) - key = "mpath-" + i - if not mpath_enabled: - remove(key) - remove('multipathed') - elif mpp_path_update: - util.SMlog("Matched SCSIid, updating entry %s" % str(mpp_entry)) - update_config(key, i, mpp_entry, remove, add, mpp_path_update) +if __name__ == '__main__': + supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp','lvmofcoe', 'gfs2'] + + LOCK_TYPE_HOST = "host" + LOCK_NS1 = "mpathcount1" + LOCK_NS2 = "mpathcount2" + + MAPPER_DIR = "/dev/mapper" + mpp_path_update = False + match_bySCSIid = False + mpath_enabled = True + + if len(sys.argv) == 3: + match_bySCSIid = True + SCSIid = sys.argv[1] + mpp_path_update = True + mpp_entry = sys.argv[2] + + cached_DM_maj = None + + try: + session = util.get_localAPI_session() + except: + print "Unable to open local XAPI session" + sys.exit(-1) + + localhost = session.xenapi.host.get_by_uuid(get_localhost_uuid()) + # Check whether multipathing is enabled (either for root dev or SRs) + try: + if get_root_dev_major() != get_dm_major(): + hconf = session.xenapi.host.get_other_config(localhost) + assert(hconf['multipathing'] == 'true') + mpath_enabled = True + except: + mpath_enabled = False + + # Check root disk if multipathed + try: + if get_root_dev_major() == get_dm_major(): + def _remove(key): + session.xenapi.host.remove_from_other_config(localhost,key) + def _add(key, val): + session.xenapi.host.add_to_other_config(localhost,key,val) + config = session.xenapi.host.get_other_config(localhost) + maps = mpath_cli.list_maps() + # Ensure output headers are not in the list + if 'name' in maps: + maps.remove('name') + # first map will always correspond to the root dev, dm-0 + assert(len(maps) > 0) + i = maps[0] + if (not match_bySCSIid) or i == SCSIid: + util.SMlog("Matched SCSIid %s, updating " \ + " Host.other-config:mpath-boot " % i) + key="mpath-boot" + if not config.has_key(key): + update_config(key, i, "", _remove, _add) else: - if not config.has_key(key): - update_config(key, i, "", remove, add) + update_config(key, i, config[key], _remove, _add) + except: + util.SMlog("MPATH: Failure updating Host.other-config:mpath-boot db") + mpc_exit(session, -1) + + try: + pbds = session.xenapi.PBD.get_all_records_where("field \"host\" = \"%s\"" % localhost) + except: + mpc_exit(session,-1) + + try: + for pbd in pbds: + def remove(key): + session.xenapi.PBD.remove_from_other_config(pbd,key) + def add(key, val): + session.xenapi.PBD.add_to_other_config(pbd,key,val) + record = pbds[pbd] + config = record['other_config'] + SR = record['SR'] + srtype = session.xenapi.SR.get_type(SR) + if srtype in supported: + devconfig = record["device_config"] + sm_config = session.xenapi.SR.get_sm_config(SR) + SCSIidlist = get_SCSIidlist(devconfig, sm_config) + if not len(SCSIidlist): + continue + for i in SCSIidlist: + if match_bySCSIid and i != SCSIid: + continue + util.SMlog("Matched SCSIid, updating %s" % i) + key = "mpath-" + i + if not mpath_enabled: + remove(key) + remove('multipathed') + elif mpp_path_update: + util.SMlog("Matched SCSIid, updating entry %s" % str(mpp_entry)) + update_config(key, i, mpp_entry, remove, add, mpp_path_update) else: - update_config(key, i, config[key], remove, add) -except: - util.SMlog("MPATH: Failure updating db. %s" % sys.exc_info()) - mpc_exit(session, -1) - -util.SMlog("MPATH: Update done") - -mpc_exit(session,0) + if not config.has_key(key): + update_config(key, i, "", remove, add) + else: + update_config(key, i, config[key], remove, add) + except: + util.SMlog("MPATH: Failure updating db. %s" % sys.exc_info()) + mpc_exit(session, -1) + + util.SMlog("MPATH: Update done") + + mpc_exit(session,0) From 5cf74f06d3e6561eb62578acdfe58fbf0b39f58a Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Tue, 12 May 2020 15:21:47 +0100 Subject: [PATCH 2/3] CP-33617 Move some mpathcount.py code into functions Move some of the functionality of mpathcount.py into functions to make it a little easier to write unit tests for it Signed-off-by: Tim Smith --- drivers/mpathcount.py | 109 +++++++++++++++++++++++------------------- 1 file changed, 59 insertions(+), 50 deletions(-) diff --git a/drivers/mpathcount.py b/drivers/mpathcount.py index 84649ade0..6577c7b12 100755 --- a/drivers/mpathcount.py +++ b/drivers/mpathcount.py @@ -24,6 +24,19 @@ import glob import json +supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp', 'lvmofcoe', 'gfs2'] + +LOCK_TYPE_HOST = "host" +LOCK_NS1 = "mpathcount1" +LOCK_NS2 = "mpathcount2" + +MAPPER_DIR = "/dev/mapper" +mpp_path_update = False +match_bySCSIid = False +mpath_enabled = True +SCSIid = 'NOTSUPPLIED' +mpp_entry = 'NOTSUPPLIED' + def get_dm_major(): global cached_DM_maj if not cached_DM_maj: @@ -144,18 +157,45 @@ def get_SCSIidlist(devconfig, sm_config): SCSIidlist.append(re.sub("^scsi-","",key)) return SCSIidlist -if __name__ == '__main__': - supported = ['iscsi','lvmoiscsi','rawhba','lvmohba', 'ocfsohba', 'ocfsoiscsi', 'netapp','lvmofcoe', 'gfs2'] - - LOCK_TYPE_HOST = "host" - LOCK_NS1 = "mpathcount1" - LOCK_NS2 = "mpathcount2" +def check_root_disk(config, maps, remove, add): + if get_root_dev_major() == get_dm_major(): + # Ensure output headers are not in the list + if 'name' in maps: + maps.remove('name') + # first map will always correspond to the root dev, dm-0 + assert(len(maps) > 0) + i = maps[0] + if (not match_bySCSIid) or i == SCSIid: + util.SMlog("Matched SCSIid %s, updating " \ + " Host.other-config:mpath-boot " % i) + key="mpath-boot" + if not config.has_key(key): + update_config(key, i, "", remove, add) + else: + update_config(key, i, config[key], remove, add) - MAPPER_DIR = "/dev/mapper" - mpp_path_update = False - match_bySCSIid = False - mpath_enabled = True +def check_devconfig(devconfig, sm_config, config, remove, add): + SCSIidlist = get_SCSIidlist(devconfig, sm_config) + if not len(SCSIidlist): + return + for i in SCSIidlist: + if match_bySCSIid and i != SCSIid: + continue + util.SMlog("Matched SCSIid, updating %s" % i) + key = "mpath-" + i + if not mpath_enabled: + remove(key) + remove('multipathed') + elif mpp_path_update: + util.SMlog("Matched SCSIid, updating entry %s" % str(mpp_entry)) + update_config(key, i, mpp_entry, remove, add, mpp_path_update) + else: + if not config.has_key(key): + update_config(key, i, "", remove, add) + else: + update_config(key, i, config[key], remove, add) +if __name__ == '__main__': if len(sys.argv) == 3: match_bySCSIid = True SCSIid = sys.argv[1] @@ -182,27 +222,14 @@ def get_SCSIidlist(devconfig, sm_config): # Check root disk if multipathed try: - if get_root_dev_major() == get_dm_major(): - def _remove(key): - session.xenapi.host.remove_from_other_config(localhost,key) - def _add(key, val): - session.xenapi.host.add_to_other_config(localhost,key,val) - config = session.xenapi.host.get_other_config(localhost) - maps = mpath_cli.list_maps() - # Ensure output headers are not in the list - if 'name' in maps: - maps.remove('name') - # first map will always correspond to the root dev, dm-0 - assert(len(maps) > 0) - i = maps[0] - if (not match_bySCSIid) or i == SCSIid: - util.SMlog("Matched SCSIid %s, updating " \ - " Host.other-config:mpath-boot " % i) - key="mpath-boot" - if not config.has_key(key): - update_config(key, i, "", _remove, _add) - else: - update_config(key, i, config[key], _remove, _add) + def _remove(key): + session.xenapi.host.remove_from_other_config(localhost,key) + def _add(key, val): + session.xenapi.host.add_to_other_config(localhost,key,val) + config = session.xenapi.host.get_other_config(localhost) + maps = mpath_cli.list_maps() + check_root_disk(config, maps, _add, _remove) + except: util.SMlog("MPATH: Failure updating Host.other-config:mpath-boot db") mpc_exit(session, -1) @@ -225,25 +252,7 @@ def add(key, val): if srtype in supported: devconfig = record["device_config"] sm_config = session.xenapi.SR.get_sm_config(SR) - SCSIidlist = get_SCSIidlist(devconfig, sm_config) - if not len(SCSIidlist): - continue - for i in SCSIidlist: - if match_bySCSIid and i != SCSIid: - continue - util.SMlog("Matched SCSIid, updating %s" % i) - key = "mpath-" + i - if not mpath_enabled: - remove(key) - remove('multipathed') - elif mpp_path_update: - util.SMlog("Matched SCSIid, updating entry %s" % str(mpp_entry)) - update_config(key, i, mpp_entry, remove, add, mpp_path_update) - else: - if not config.has_key(key): - update_config(key, i, "", remove, add) - else: - update_config(key, i, config[key], remove, add) + check_devconfig(devconfig, sm_config, config, remove, add) except: util.SMlog("MPATH: Failure updating db. %s" % sys.exc_info()) mpc_exit(session, -1) From 0fd0bdeb06cc738fd524feb41efa69f1be3b94c9 Mon Sep 17 00:00:00 2001 From: Tim Smith Date: Tue, 19 May 2020 16:13:28 +0100 Subject: [PATCH 3/3] CP-33617 Initial set of unit tests for mpathcount.py Signed-off-by: Tim Smith --- .../3600a098038303973743f486833396d44 | 0 tests/test_mpathcount.py | 203 ++++++++++++++++++ 2 files changed, 203 insertions(+) create mode 100644 test_support/fake_mapper/3600a098038303973743f486833396d44 create mode 100644 tests/test_mpathcount.py diff --git a/test_support/fake_mapper/3600a098038303973743f486833396d44 b/test_support/fake_mapper/3600a098038303973743f486833396d44 new file mode 100644 index 000000000..e69de29bb diff --git a/tests/test_mpathcount.py b/tests/test_mpathcount.py new file mode 100644 index 000000000..1b5cdb7ec --- /dev/null +++ b/tests/test_mpathcount.py @@ -0,0 +1,203 @@ +""" +Unit tests for mpathcount +""" +import errno +import os +import unittest +import mock +import testlib +import mpathcount +import SR +import util + +#class FakeXapi(object): + #def __init__(self): + #self.srRecord = { + #'name_label': 'dummy' + #} + + #def isPluggedHere(self): + #return True + + #def isMaster(self): + #return True + +# pylint: disable=W0613; mocks don't need to be accessed +# pylint: disable=R0201; methods must be instance for nose to work +# pylint: disable=W0212; unit tests are permitted to snoop +class TestMpathCount(unittest.TestCase): + """ + Unit tests for mpathcount + """ + + @mock.patch('mpathcount.os', autospec=True) + def test_get_root_dev_major(self, mock_os): + mpathcount.get_root_dev_major() + assert(mock_os.major.called) + + @mock.patch('mpathcount.mpath_cli', autospec=True) + def test_get_path_count(self, mpath_cli): + mpath_cli.get_topology.return_value = [ + "multipathd> show map 3600a098038303973743f486833396d44 topology", + "3600a098038303973743f486833396d44 dm-1 NETAPP ,LUN C-Mode", + "size=200G features='4 queue_if_no_path pg_init_retries 50 retain_attached_hw_handle' hwhandler='1 alua' wp=rw", + "`-+- policy='service-time 0' prio=50 status=active", + " |- 0:0:0:4 sdr 65:16 active ready running", + " |- 0:0:1:4 sdg 8:96 active ready running", + " |- 7:0:0:4 sdab 65:176 failed faulty running", + " `- 7:0:1:4 sdam 66:96 failed faulty running" + ] + count, total = mpathcount.get_path_count('3600a098038303973743f486833396d44') + self.assertEqual(4, total, msg='total should be 4, was {}'.format(total)) + self.assertEqual(2, count, msg='count should be 2, was {}'.format(count)) + + @mock.patch('mpathcount.get_path_count', return_value=(2,4)) + def test_update_config(self, get_path_count): + store={'fred': ''} + def remove(key): + if key in store: + del store[key] + + def add(key, val): + store[key] = val + + ## Point this to a place where SCSIid files either do or + ## don't exist, to exercise different branches + mpathcount.MAPPER_DIR = "test_support/fake_mapper" + + mpathcount.update_config("fred", "3600a098038303973743f486833396d44", "[2, 4]", remove, add, True) + self.assertIn('MPPEnabled', store, msg="Key 'MPPEnabled' not present in store") + self.assertIn('multipathed', store, msg="Key 'multipathed' not present in store") + self.assertEqual('[2, 4]', store['fred'], msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['fred'])) + + store={'fred': ''} + mpathcount.update_config("fred", "3600a098038303973743f486833396d44", "[2, hamster]", remove, add, False) + self.assertIn('multipathed', store, msg="Key 'multipathed' not present in store") + self.assertEqual('[2, 4]', store['fred'], msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['fred'])) + + store={'fred': ''} + mpathcount.update_config("fred", "3600a098038303973743f486833396d44", "[2, 2]", remove, add, False) + self.assertIn('multipathed', store, msg="Key 'multipathed' not present in store") + self.assertEqual('[2, 4]', store['fred'], msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['fred'])) + + store={'fred': ''} + mpathcount.update_config("fred", "3600a098038303973743f486833396d44", "", remove, add, False) + self.assertIn('multipathed', store, msg="Key 'multipathed' not present in store") + self.assertEqual('[2, 4]', store['fred'], msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['fred'])) + + store={'fred': ''} + mpathcount.update_config("fred", "NotARealItem", "", remove, add, False) + self.assertNotIn('multipathed', store, msg="Key 'multipathed' present in store. Should not be.") + self.assertNotIn('fred', store, msg="Key 'fred' present in store. Should not be.") + + @mock.patch('mpathcount.get_dm_major') + @mock.patch('mpathcount.get_root_dev_major') + @mock.patch('mpathcount.update_config', autospec=True) + def test_check_root_disk(self, update_config, get_root_dev_major, get_dm_major): + store={} + def fake_update_config(k, s, v, a, t): + store[k] = v + + get_root_dev_major.return_value = 4 + get_dm_major.return_value = 4 + update_config.side_effect = fake_update_config + mpathcount.match_bySCSIid = False + maps = ["3600a098038303973743f486833396d44", 'name'] + mpathcount.check_root_disk({}, maps, None, None) + self.assertIn('mpath-boot', store, msg="Key 'mpath-boot' not present in store") + + mpathcount.match_bySCSIid = True + mpathcount.SCSIid = "3600a098038303973743f486833396d44" + maps = ["3600a098038303973743f486833396d44"] + store={} + mpathcount.check_root_disk({'mpath-boot': '[2, 4]'}, maps, None, None) + self.assertIn('mpath-boot', store, msg="Key 'mpath-boot' not present in store") + self.assertEqual('[2, 4]', store['mpath-boot'], + msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['mpath-boot'])) + + get_root_dev_major.return_value = 4 + get_dm_major.return_value = 2 + mpathcount.match_bySCSIid = False + maps = ["3600a098038303973743f486833396d44", 'name'] + store={} + mpathcount.check_root_disk({}, maps, None, None) + self.assertNotIn('mpath-boot', store, msg="Key 'mpath-boot' present in store. Should not be.") + + @mock.patch('mpathcount.update_config', autospec=True) + def test_check_devconfig(self, update_config): + store={} + def remove(key): + if key in store: + print("del {}".format(key)) + del store[key] + + def fake_update_config(k, s, v, a, t): + store[k] = v + + update_config.side_effect = fake_update_config + + store={} + mpathcount.match_bySCSIid = False + mpathcount.check_devconfig( + {}, + {}, + {'mpath-3600a098038303973743f486833396d40': '[2, 4]'}, + remove, None) + self.assertNotIn('mpath-3600a098038303973743f486833396d40', store, + msg="Key 'mpath-3600a098038303973743f486833396d40' present in store. Should not be.") + + store={} + mpathcount.match_bySCSIid = False + mpathcount.check_devconfig( + {}, + {'SCSIid': '3600a098038303973743f486833396d40,3600a098038303973743f486833396d41'}, + {'mpath-3600a098038303973743f486833396d40': '[2, 4]'}, + remove, None) + self.assertIn('mpath-3600a098038303973743f486833396d40', store, + msg="Key 'mpath-3600a098038303973743f486833396d40' not present in store") + self.assertIn('mpath-3600a098038303973743f486833396d41', store, + msg="Key 'mpath-3600a098038303973743f486833396d41' not present in store") + self.assertEqual('[2, 4]', store['mpath-3600a098038303973743f486833396d40'], + msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['mpath-3600a098038303973743f486833396d40'])) + self.assertEqual('', store['mpath-3600a098038303973743f486833396d41'], + msg="Key config not updated correctly (should be '', was '{}')".format(store['mpath-3600a098038303973743f486833396d41'])) + + store={} + mpathcount.match_bySCSIid = False + mpathcount.check_devconfig( + {'SCSIid': '3600a098038303973743f486833396d40'}, + {}, + {'mpath-3600a098038303973743f486833396d40': '[2, 4]'}, + remove, None) + self.assertIn('mpath-3600a098038303973743f486833396d40', store, + msg="Key 'mpath-3600a098038303973743f486833396d40' not present in store") + self.assertEqual('[2, 4]', store['mpath-3600a098038303973743f486833396d40'], + msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['mpath-3600a098038303973743f486833396d40'])) + + store={} + mpathcount.match_bySCSIid = False + mpathcount.check_devconfig( + {'provider': 'present', 'ScsiId': '3600a098038303973743f486833396d40'}, + {}, + {'mpath-3600a098038303973743f486833396d40': '[2, 4]'}, + remove, None) + self.assertIn('mpath-3600a098038303973743f486833396d40', store, + msg="Key 'mpath-3600a098038303973743f486833396d40' not present in store") + self.assertEqual('[2, 4]', store['mpath-3600a098038303973743f486833396d40'], + msg="Key config not updated correctly (should be '[2, 4]', was '{}')".format(store['mpath-3600a098038303973743f486833396d40'])) + + store={ + 'mpath-3600a098038303973743f486833396d40': '[2, 4]', + 'multipathed': True + } + mpathcount.match_bySCSIid = False + mpathcount.mpath_enabled = False + mpathcount.check_devconfig( + {}, + {'SCSIid': '3600a098038303973743f486833396d40,3600a098038303973743f486833396d41'}, + {'mpath-3600a098038303973743f486833396d40': '[2, 4]'}, + remove, None) + self.assertNotIn('multipathed', store, msg="Key 'multipathed' present in store. Should not be.") + self.assertNotIn('mpath-3600a098038303973743f486833396d40', store, + msg="Key 'mpath-3600a098038303973743f486833396d40' present in store. Should not be.") +