]> review.fuel-infra Code Review - openstack-build/neutron-build.git/commitdiff
Mock oslo policy HTTPCheck instead of urllib
authorKevin Benton <blak111@gmail.com>
Wed, 7 Oct 2015 02:28:47 +0000 (19:28 -0700)
committerIhar Hrachyshka <ihrachys@redhat.com>
Tue, 13 Oct 2015 15:26:23 +0000 (17:26 +0200)
We were mocking internal behavior of oslo policy by
patching urllib. This will break with the upcoming oslo
release that switches to requests.

This patch changes the mock to the HTTPCheck level and we
can leave implementation details testing up to oslo_policy.

Change-Id: I07957f01307e25f1547197c720eea6e3e7f0ef5a
Closes-Bug: #1503890
(cherry picked from commit a0f1d9d6de1560be91d3001c8ac9f880a7a5a7e0)

Add testresources used by oslo.db fixture

If we use oslo.db fixtures, we'll need the package or
the next version of oslo.db release will break us.

Closes-Bug: #1503501
Change-Id: I7dfbf240333095d91a414ba15a439bdc4804eb25
(cherry picked from commit 86ad967e40c2c6752ec0fb46cfd3098ede0c7178)

Fix functional test_server tests

Now oslo.service 0.10.0 no longer sends SIGHUP to parent and
children services.

This was a chance introduced by 286a6ea, and since it invalidated
the very logic under test, this must be revised.

(cherry picked from commit 090fe713592c2b6398d999bfa03b80cbb2054609)

Change-Id: I18a11283925369bc918002477774f196010a1bc3
Closes-bug: #1505438
(cherry picked from commit 090fe713592c2b6398d999bfa03b80cbb2054609)

Make test_server work with older versions of oslo.service

Change I18a11283925369bc918002477774f196010a1bc3 fixed the test for
oslo.service >= 0.10.0, but it also broke it for older versions of
oslo.service. Since the library has minimal version of >= 0.7.0 in
requirements.txt, test should pass for those versions too.

Now, instead of validating that either reset() or restart() of workers
are triggered on SIGHUP, just validate that .start() is triggered the
expected number of times (either way, no matter how oslo.service decide
to clean up the children, they exit and then are respawned).

Change-Id: I41f9d3af780b3178b075bc1e7084f417a2bd1378
Closes-Bug: #1505645
(cherry picked from commit 7bb40921660cf29beb68e338e205499efd6ffa36)

Fixed multiple py34 gate issues

1. Scope mock of 'open' to module

By mocking 'open' at the module level, we can avoid affecting
'open' calls from other modules.

2. Stop using LOG.exception in contexts with no sys.exc_info set

Python 3.4 logger fills in record.exc_info with sys.exc_info() result
[1], and then it uses it to determine the current exception [2] to
append to the log message. Since there is no exception, exc_info[1] is
None, and we get AttributeError inside traceback module.

It's actually a bug in Python interpreter that it attempt to access the
attribute when there is no exception. It turns out that it's fixed in
latest master of cPython [3] (the intent of the patch does not seem
relevant, but it removes the offending code while reshuffling the code).
Note that now cPython correctly checks the exception value before
accessing its attributes [4].

The patch in cPython that resulted in the failure is [5] and is present
since initial Python 3k releases.

The patch in fixtures that broke us is [6].

[1]: https://hg.python.org/cpython/file/tip/Lib/logging/__init__.py#l1412
[2]: https://hg.python.org/cpython/file/tip/Lib/logging/__init__.py#l575
[3]: https://hg.python.org/cpython/rev/73afda5a4e4c
[4]: https://hg.python.org/cpython/rev/73afda5a4e4c#l6.484
[5]: https://hg.python.org/cpython/rev/2ee09afee126
[6]: https://github.com/testing-cabal/fixtures/commit/67dd2956943261e845a866dab155208c51da937e

Closes-Bug: #1503847
Closes-Bug: #1504053
Co-Authored-By: Ihar Hrachyshka <ihrachys@redhat.com>
Change-Id: I456b7846b8a53e4d3f8c91583685e0e1eaa84719
(cherry picked from commit 8f58bbf38f5984e70d6e0be7427deb99d7782d1d)

neutron/manager.py
neutron/policy.py
neutron/services/provider_configuration.py
neutron/tests/functional/test_server.py
neutron/tests/unit/db/test_migration.py
neutron/tests/unit/test_policy.py
test-requirements.txt

index a2f22b16fa5471646cf2421d7d0a2787a0e950f3..51c9bf5cf48b0ad10f2cb3681e9c3212671f1914 100644 (file)
@@ -13,6 +13,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import sys
 import weakref
 
 from oslo_config import cfg
@@ -136,19 +137,22 @@ class NeutronManager(object):
     @staticmethod
     def load_class_for_provider(namespace, plugin_provider):
         if not plugin_provider:
-            LOG.exception(_LE("Error, plugin is not set"))
+            LOG.error(_LE("Error, plugin is not set"))
             raise ImportError(_("Plugin not found."))
         try:
             # Try to resolve plugin by name
             mgr = driver.DriverManager(namespace, plugin_provider)
             plugin_class = mgr.driver
-        except RuntimeError as e1:
+        except RuntimeError:
+            e1_info = sys.exc_info()
             # fallback to class name
             try:
                 plugin_class = importutils.import_class(plugin_provider)
-            except ImportError as e2:
-                LOG.exception(_LE("Error loading plugin by name, %s"), e1)
-                LOG.exception(_LE("Error loading plugin by class, %s"), e2)
+            except ImportError:
+                LOG.error(_LE("Error loading plugin by name"),
+                          exc_info=e1_info)
+                LOG.error(_LE("Error loading plugin by class"),
+                          exc_info=True)
                 raise ImportError(_("Plugin not found."))
         return plugin_class
 
index 99286a02661976eaafd618d36a6dae13794591c2..351b96b84dff3fae818526dc34943e90112d8984 100644 (file)
@@ -230,7 +230,7 @@ class OwnerCheck(policy.Check):
                 # If we are here split failed with both separators
                 err_reason = (_("Unable to find resource name in %s") %
                               self.target_field)
-                LOG.exception(err_reason)
+                LOG.error(err_reason)
                 raise exceptions.PolicyCheckError(
                     policy="%s:%s" % (self.kind, self.match),
                     reason=err_reason)
@@ -240,7 +240,7 @@ class OwnerCheck(policy.Check):
                 err_reason = (_("Unable to verify match:%(match)s as the "
                                 "parent resource: %(res)s was not found") %
                               {'match': self.match, 'res': parent_res})
-                LOG.exception(err_reason)
+                LOG.error(err_reason)
                 raise exceptions.PolicyCheckError(
                     policy="%s:%s" % (self.kind, self.match),
                     reason=err_reason)
index 9f8981be8236dbea9d2ef537732f6dc0f68f6871..c2837855a30da8802a11f5ae4cc1525f89cfcb32 100644 (file)
@@ -197,7 +197,7 @@ class ProviderConfiguration(object):
             if v['driver'] == driver:
                 msg = (_("Driver %s is not unique across providers") %
                        driver)
-                LOG.exception(msg)
+                LOG.error(msg)
                 raise n_exc.Invalid(msg)
 
     def _ensure_default_unique(self, type, default):
@@ -207,7 +207,7 @@ class ProviderConfiguration(object):
             if k[0] == type and v['default']:
                 msg = _("Multiple default providers "
                         "for service %s") % type
-                LOG.exception(msg)
+                LOG.error(msg)
                 raise n_exc.Invalid(msg)
 
     def add_provider(self, provider):
index e85dcec9b1789e9bb1034961dc8ba7402b8962c4..e04b31b1fadb3fd6cbccb6855d644adcf5227871 100644 (file)
@@ -34,8 +34,8 @@ from neutron import wsgi
 CONF = cfg.CONF
 
 # This message will be written to temporary file each time
-# reset method is called.
-FAKE_RESET_MSG = "reset".encode("utf-8")
+# start method is called.
+FAKE_START_MSG = "start".encode("utf-8")
 
 TARGET_PLUGIN = 'neutron.plugins.ml2.plugin.Ml2Plugin'
 
@@ -118,28 +118,26 @@ class TestNeutronServer(base.BaseTestCase):
             return [proc.pid for proc in psutil.process_iter()
                     if proc.pid == self.service_pid]
 
-    def _fake_reset(self):
-        """Writes FAKE_RESET_MSG to temporary file on each call."""
-
+    def _fake_start(self):
         with open(self.temp_file, 'a') as f:
-            f.write(FAKE_RESET_MSG)
+            f.write(FAKE_START_MSG)
 
     def _test_restart_service_on_sighup(self, service, workers=0):
-        """Test that a service correctly restarts on receiving SIGHUP.
+        """Test that a service correctly (re)starts on receiving SIGHUP.
 
         1. Start a service with a given number of workers.
         2. Send SIGHUP to the service.
-        3. Wait for workers (if any) to restart.
-        4. Assert that the pids of the workers didn't change after restart.
+        3. Wait for workers (if any) to (re)start.
         """
 
-        start_workers = self._start_server(callback=service, workers=workers)
-
+        self._start_server(callback=service, workers=workers)
         os.kill(self.service_pid, signal.SIGHUP)
 
-        # Wait for temp file to be created and its size become equal
-        # to size of FAKE_RESET_MSG repeated (workers + 1) times.
-        expected_size = len(FAKE_RESET_MSG) * (workers + 1)
+        expected_msg = FAKE_START_MSG * workers * 2
+
+        # Wait for temp file to be created and its size reaching the expected
+        # value
+        expected_size = len(expected_msg)
         condition = lambda: (os.path.isfile(self.temp_file)
                              and os.stat(self.temp_file).st_size ==
                              expected_size)
@@ -152,16 +150,12 @@ class TestNeutronServer(base.BaseTestCase):
                 {'filename': self.temp_file,
                  'size': expected_size}))
 
-        # Verify that reset has been called for parent process in which
-        # a service was started and for each worker by checking that
-        # FAKE_RESET_MSG has been written to temp file workers + 1 times.
+        # Verify that start has been called twice for each worker (one for
+        # initial start, and the second one on SIGHUP after children were
+        # terminated).
         with open(self.temp_file, 'r') as f:
             res = f.readline()
-            self.assertEqual(FAKE_RESET_MSG * (workers + 1), res)
-
-        # Make sure worker pids don't change
-        end_workers = self._get_workers()
-        self.assertEqual(start_workers, end_workers)
+            self.assertEqual(expected_msg, res)
 
 
 class TestWsgiServer(TestNeutronServer):
@@ -197,10 +191,10 @@ class TestWsgiServer(TestNeutronServer):
     def _run_wsgi(self, workers=0):
         """Start WSGI server with a test application."""
 
-        # Mock reset method to check that it is being called
-        # on receiving SIGHUP.
-        with mock.patch("neutron.wsgi.WorkerService.reset") as reset_method:
-            reset_method.side_effect = self._fake_reset
+        # Mock start method to check that children are started again on
+        # receiving SIGHUP.
+        with mock.patch("neutron.wsgi.WorkerService.start") as start_method:
+            start_method.side_effect = self._fake_start
 
             server = wsgi.Server("Test")
             server.start(self.application, 0, "0.0.0.0",
@@ -235,13 +229,13 @@ class TestRPCServer(TestNeutronServer):
     def _serve_rpc(self, workers=0):
         """Start RPC server with a given number of workers."""
 
-        # Mock reset method to check that it is being called
-        # on receiving SIGHUP.
-        with mock.patch("neutron.service.RpcWorker.reset") as reset_method:
+        # Mock start method to check that children are started again on
+        # receiving SIGHUP.
+        with mock.patch("neutron.service.RpcWorker.start") as start_method:
             with mock.patch(
                     "neutron.manager.NeutronManager.get_plugin"
             ) as get_plugin:
-                reset_method.side_effect = self._fake_reset
+                start_method.side_effect = self._fake_start
                 get_plugin.return_value = self.plugin
 
                 CONF.set_override("rpc_workers", workers)
@@ -285,7 +279,7 @@ class TestPluginWorker(TestNeutronServer):
                 pass
 
         # Make both ABC happy and ensure 'self' is correct
-        FakeWorker.reset = self._fake_reset
+        FakeWorker.start = self._fake_start
         workers = [FakeWorker()]
         self.plugin.return_value.get_workers.return_value = workers
         self._test_restart_service_on_sighup(service=self._start_plugin,
index a88a6e93ca4de417d6ef43eb0ac3a03e86e002cc..0ae98f2d0f9140981a568ee43adf0f175e2eb1d7 100644 (file)
@@ -308,7 +308,7 @@ class TestCli(base.BaseTestCase):
                 mock.patch.object(cli, '_use_separate_migration_branches',
                                   return_value=not branchless):
             fc.return_value.get_heads.return_value = heads
-            with mock.patch('six.moves.builtins.open') as mock_open:
+            with mock.patch.object(cli, 'open') as mock_open:
                 mock_open.return_value.__enter__ = lambda s: s
                 mock_open.return_value.__exit__ = mock.Mock()
                 mock_open.return_value.read.return_value = (
index ed230179fdf4c32e83b869efb40d07a8cab97720..bed9740c9252281c460fa628f230ff9f9f2157c2 100644 (file)
@@ -19,8 +19,6 @@ import mock
 from oslo_policy import policy as oslo_policy
 from oslo_serialization import jsonutils
 from oslo_utils import importutils
-import six
-import six.moves.urllib.request as urlrequest
 
 import neutron
 from neutron.api.v2 import attributes
@@ -105,25 +103,24 @@ class PolicyTestCase(base.BaseTestCase):
         result = policy.enforce(self.context, action, self.target)
         self.assertEqual(result, True)
 
-    @mock.patch.object(urlrequest, 'urlopen',
-                       return_value=six.StringIO("True"))
-    def test_enforce_http_true(self, mock_urlrequest):
+    #TODO(kevinbenton): replace these private method mocks with a fixture
+    @mock.patch.object(oslo_policy._checks.HttpCheck, '__call__',
+                       return_value=True)
+    def test_enforce_http_true(self, mock_httpcheck):
         action = "example:get_http"
         target = {}
         result = policy.enforce(self.context, action, target)
         self.assertEqual(result, True)
 
-    def test_enforce_http_false(self):
-
-        def fakeurlopen(url, post_data):
-            return six.StringIO("False")
-
-        with mock.patch.object(urlrequest, 'urlopen', new=fakeurlopen):
-            action = "example:get_http"
-            target = {}
-            self.assertRaises(oslo_policy.PolicyNotAuthorized,
-                              policy.enforce, self.context,
-                              action, target)
+    #TODO(kevinbenton): replace these private method mocks with a fixture
+    @mock.patch.object(oslo_policy._checks.HttpCheck, '__call__',
+                       return_value=False)
+    def test_enforce_http_false(self, mock_httpcheck):
+        action = "example:get_http"
+        target = {}
+        self.assertRaises(oslo_policy.PolicyNotAuthorized,
+                          policy.enforce, self.context,
+                          action, target)
 
     def test_templatized_enforcement(self):
         target_mine = {'tenant_id': 'fake'}
index ac9738eabe701cbc47ee11911832df272d314f90..cc612953b216e2e42ff16fba929d2300fa81a2fc 100644 (file)
@@ -13,6 +13,7 @@ sphinx!=1.2.0,!=1.3b1,<1.3,>=1.1.2
 oslosphinx>=2.5.0 # Apache-2.0
 testrepository>=0.0.18
 testtools>=1.4.0
+testresources>=0.2.4
 testscenarios>=0.4
 WebTest>=2.0
 oslotest>=1.10.0 # Apache-2.0