From ebae3c20818860dcab7d62197087562bb970fbbf Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Fri, 3 Jun 2022 19:33:49 +0200 Subject: [PATCH] Poison /sys access via various calls in test Unit test should never depend on the file system of the machine it runs on. So this patch injects a poison to various std lib calls to catch these cases. We only selectively poison access to /sys fs as there are legitimate cases to access temp files. When I isolated the poisoned test cases I tried to add a meaningful mocking around the sysfs reads based on the meaning of the test. Change-Id: I66636ebfe627cba8c4470cd84faa78f69249aa05 --- nova/test.py | 1 + nova/tests/fixtures/libvirt.py | 10 ++++ nova/tests/fixtures/nova.py | 59 +++++++++++++++++++ .../libvirt/test_report_cpu_traits.py | 4 +- .../regressions/test_bug_1928063.py | 4 +- nova/tests/unit/network/test_neutron.py | 4 ++ nova/tests/unit/pci/test_devspec.py | 8 +++ 7 files changed, 86 insertions(+), 4 deletions(-) diff --git a/nova/test.py b/nova/test.py index e48245c09362..d645d06cfa5c 100644 --- a/nova/test.py +++ b/nova/test.py @@ -285,6 +285,7 @@ class TestCase(base.BaseTestCase): quota.UID_QFD_POPULATED_CACHE_ALL = False self.useFixture(nova_fixtures.GenericPoisonFixture()) + self.useFixture(nova_fixtures.SysFsPoisonFixture()) # make sure that the wsgi app is fully initialized for all testcase # instead of only once initialized for test worker diff --git a/nova/tests/fixtures/libvirt.py b/nova/tests/fixtures/libvirt.py index 7c450c437996..a4c0fac2b49f 100644 --- a/nova/tests/fixtures/libvirt.py +++ b/nova/tests/fixtures/libvirt.py @@ -31,6 +31,7 @@ from nova.objects import fields as obj_fields from nova.tests.fixtures import libvirt_data as fake_libvirt_data from nova.virt.libvirt import config as vconfig from nova.virt.libvirt import driver as libvirt_driver +from nova.virt.libvirt import host # Allow passing None to the various connect methods @@ -2267,6 +2268,15 @@ class LibvirtFixture(fixtures.Fixture): self.mock_uname = self.useFixture( fixtures.MockPatch('os.uname', return_value=fake_uname)).mock + real_exists = os.path.exists + + def fake_exists(path): + if path == host.SEV_KERNEL_PARAM_FILE: + return False + return real_exists(path) + + self.useFixture(fixtures.MonkeyPatch('os.path.exists', fake_exists)) + # ...and on all machine types fake_loaders = [ { diff --git a/nova/tests/fixtures/nova.py b/nova/tests/fixtures/nova.py index 6c0c59a130b8..129b2f9abb07 100644 --- a/nova/tests/fixtures/nova.py +++ b/nova/tests/fixtures/nova.py @@ -1739,3 +1739,62 @@ class ReaderWriterLock(lockutils.ReaderWriterLock): 'threading.current_thread', eventlet.getcurrent) with mpatch if eventlet_patched else contextlib.ExitStack(): super().__init__(*a, **kw) + + +class SysFsPoisonFixture(fixtures.Fixture): + + def inject_poison(self, module_name, function_name): + import importlib + mod = importlib.import_module(module_name) + orig_f = getattr(mod, function_name) + if ( + isinstance(orig_f, mock.Mock) or + # FIXME(gibi): Is this a bug in unittest.mock? If I remove this + # then LibvirtReportSevTraitsTests fails as builtins.open is mocked + # there at import time via @test.patch_open. That injects a + # MagicMock instance to builtins.open which we check here against + # Mock (or even MagicMock) via isinstance and that check says it is + # not a mock. More interestingly I cannot reproduce the same + # issue with @test.patch_open and isinstance in a simple python + # interpreter. So to make progress I'm checking the class name + # here instead as that works. + orig_f.__class__.__name__ == "MagicMock" + ): + # the target is already mocked, probably via a decorator run at + # import time, so we don't need to inject our poison + return + + full_name = module_name + "." + function_name + + def toxic_wrapper(*args, **kwargs): + path = args[0] + if isinstance(path, bytes): + pattern = b'/sys' + elif isinstance(path, str): + pattern = '/sys' + else: + # we ignore the rest of the potential pathlike types for now + pattern = None + + if pattern and path.startswith(pattern): + raise Exception( + 'This test invokes %s on %s. It is bad, you ' + 'should mock it.' + % (full_name, path) + ) + else: + return orig_f(*args, **kwargs) + + self.useFixture(fixtures.MonkeyPatch(full_name, toxic_wrapper)) + + def setUp(self): + super().setUp() + self.inject_poison("os.path", "isdir") + self.inject_poison("builtins", "open") + self.inject_poison("glob", "iglob") + self.inject_poison("os", "listdir") + self.inject_poison("glob", "glob") + # TODO(gibi): Would be good to poison these too but that makes + # a bunch of test to fail + # self.inject_poison("os.path", "exists") + # self.inject_poison("os", "stat") diff --git a/nova/tests/functional/libvirt/test_report_cpu_traits.py b/nova/tests/functional/libvirt/test_report_cpu_traits.py index eb984c5145ec..99e68b7b5c9e 100644 --- a/nova/tests/functional/libvirt/test_report_cpu_traits.py +++ b/nova/tests/functional/libvirt/test_report_cpu_traits.py @@ -190,7 +190,6 @@ class LibvirtReportNoSevTraitsTests(LibvirtReportTraitsTestBase): class LibvirtReportSevTraitsTests(LibvirtReportTraitsTestBase): STUB_INIT_HOST = False - @test.patch_exists(SEV_KERNEL_PARAM_FILE, True) @test.patch_open(SEV_KERNEL_PARAM_FILE, "1\n") @mock.patch.object( fakelibvirt.virConnect, '_domain_capability_features', @@ -198,7 +197,8 @@ class LibvirtReportSevTraitsTests(LibvirtReportTraitsTestBase): def setUp(self): super(LibvirtReportSevTraitsTests, self).setUp() self.flags(num_memory_encrypted_guests=16, group='libvirt') - self.start_compute() + with test.patch_exists(SEV_KERNEL_PARAM_FILE, True): + self.start_compute() def test_sev_trait_on_off(self): """Test that the compute service reports the SEV trait in the list of diff --git a/nova/tests/functional/regressions/test_bug_1928063.py b/nova/tests/functional/regressions/test_bug_1928063.py index 2c773981c5a9..94d7b8122c28 100644 --- a/nova/tests/functional/regressions/test_bug_1928063.py +++ b/nova/tests/functional/regressions/test_bug_1928063.py @@ -30,7 +30,6 @@ class TestSEVInstanceReboot(base.ServersTestBase): """ microversion = 'latest' - @test.patch_exists(SEV_KERNEL_PARAM_FILE, True) @test.patch_open(SEV_KERNEL_PARAM_FILE, "1\n") @mock.patch.object( fakelibvirt.virConnect, '_domain_capability_features', @@ -40,7 +39,8 @@ class TestSEVInstanceReboot(base.ServersTestBase): # Configure the compute to allow SEV based instances and then start self.flags(num_memory_encrypted_guests=16, group='libvirt') - self.start_compute() + with test.patch_exists(SEV_KERNEL_PARAM_FILE, True): + self.start_compute() # Create a SEV enabled image for the test sev_image = copy.deepcopy(self.glance.image1) diff --git a/nova/tests/unit/network/test_neutron.py b/nova/tests/unit/network/test_neutron.py index b82bf5349f23..f1dd51598d09 100644 --- a/nova/tests/unit/network/test_neutron.py +++ b/nova/tests/unit/network/test_neutron.py @@ -8075,6 +8075,10 @@ class TestAPIPortbinding(TestAPIBase): mock_get_instance_pci_devs.assert_called_once_with( instance, pci_req_id) + @mock.patch.object( + pci_utils, 'is_physical_function', + new=mock.Mock(return_value=False) + ) @mock.patch.object( pci_utils, 'get_vf_num_by_pci_address', new=mock.MagicMock( diff --git a/nova/tests/unit/pci/test_devspec.py b/nova/tests/unit/pci/test_devspec.py index f51ee54ac1ba..41e48573d6e9 100644 --- a/nova/tests/unit/pci/test_devspec.py +++ b/nova/tests/unit/pci/test_devspec.py @@ -644,6 +644,10 @@ class PciDevSpecRemoteManagedTestCase(test.NoDBTestCase): pci = devspec.PciDeviceSpec(pci_info) self.assertTrue(pci.match(self.test_dev)) + @mock.patch( + 'nova.pci.utils.is_physical_function', + new=mock.Mock(return_value=False) + ) def test_remote_managed_vf_match_by_pci_obj(self): pci_info = {"vendor_id": "8086", "address": "0000:0a:00.2", "product_id": "5057", "physical_network": "hr_net", @@ -663,6 +667,10 @@ class PciDevSpecRemoteManagedTestCase(test.NoDBTestCase): pci_obj = objects.PciDevice.create(None, pci_dev) self.assertTrue(pci.match_pci_obj(pci_obj)) + @mock.patch( + 'nova.pci.utils.is_physical_function', + new=mock.Mock(return_value=False) + ) def test_remote_managed_vf_no_match_by_pci_obj(self): pci_info = {"vendor_id": "8086", "address": "0000:0a:00.0", "product_id": "5057", "physical_network": "hr_net",