Merge "Separate WSGIService from RPCService"

9 files changed, 305 insertions, 231 deletions

diff --git a/ironic/cmd/api.py b/ironic/cmd/api.py
index fe2a058eb..e95bd8c51 100644
--- a/ironic/cmd/api.py
+++ b/ironic/cmd/api.py
@@ -22,6 +22,7 @@ import sys
 from oslo_config import cfg
 
 from ironic.common import service as ironic_service
+from ironic.common import wsgi_service
 from ironic.objects import base
 from ironic.objects import indirection
 
@@ -38,7 +39,7 @@ def main():
 
     # Build and start the WSGI app
     launcher = ironic_service.process_launcher()
-    server = ironic_service.WSGIService('ironic_api', CONF.api.enable_ssl_api)
+    server = wsgi_service.WSGIService('ironic_api', CONF.api.enable_ssl_api)
     launcher.launch_service(server, workers=server.workers)
     launcher.wait()
 
diff --git a/ironic/cmd/conductor.py b/ironic/cmd/conductor.py
index 14728a5a2..57dc90567 100644
--- a/ironic/cmd/conductor.py
+++ b/ironic/cmd/conductor.py
@@ -26,6 +26,7 @@ from oslo_log import log
 from oslo_service import service
 
 from ironic.common.i18n import _LW
+from ironic.common import rpc_service
 from ironic.common import service as ironic_service
 from ironic.conf import auth
 
@@ -58,12 +59,20 @@ def _check_auth_options(conf):
 
 
 def main():
+    # NOTE(lucasagomes): Safeguard to prevent 'ironic.conductor.manager'
+    # from being imported prior to the configuration options being loaded.
+    # If this happened, the periodic decorators would always use the
+    # default values of the options instead of the configured ones. For
+    # more information see: https://bugs.launchpad.net/ironic/+bug/1562258
+    # and https://bugs.launchpad.net/ironic/+bug/1279774.
+    assert 'ironic.conductor.manager' not in sys.modules
+
     # Parse config file and command line options, then start logging
     ironic_service.prepare_service(sys.argv)
 
-    mgr = ironic_service.RPCService(CONF.host,
-                                    'ironic.conductor.manager',
-                                    'ConductorManager')
+    mgr = rpc_service.RPCService(CONF.host,
+                                 'ironic.conductor.manager',
+                                 'ConductorManager')
 
     _check_auth_options(CONF)
 
diff --git a/ironic/common/rpc_service.py b/ironic/common/rpc_service.py
new file mode 100644
index 000000000..aece81adf
--- /dev/null
+++ b/ironic/common/rpc_service.py
@@ -0,0 +1,91 @@
+# -*- encoding: utf-8 -*-
+#
+# Copyright © 2012 eNovance <licensing@enovance.com>
+#
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+import signal
+
+from oslo_log import log
+import oslo_messaging as messaging
+from oslo_service import service
+from oslo_utils import importutils
+
+from ironic.common import context
+from ironic.common.i18n import _LE, _LI
+from ironic.common import rpc
+from ironic.objects import base as objects_base
+
+LOG = log.getLogger(__name__)
+
+
+class RPCService(service.Service):
+
+    def __init__(self, host, manager_module, manager_class):
+        super(RPCService, self).__init__()
+        self.host = host
+        manager_module = importutils.try_import(manager_module)
+        manager_class = getattr(manager_module, manager_class)
+        self.manager = manager_class(host, manager_module.MANAGER_TOPIC)
+        self.topic = self.manager.topic
+        self.rpcserver = None
+        self.deregister = True
+
+    def start(self):
+        super(RPCService, self).start()
+        admin_context = context.get_admin_context()
+
+        target = messaging.Target(topic=self.topic, server=self.host)
+        endpoints = [self.manager]
+        serializer = objects_base.IronicObjectSerializer()
+        self.rpcserver = rpc.get_server(target, endpoints, serializer)
+        self.rpcserver.start()
+
+        self.handle_signal()
+        self.manager.init_host(admin_context)
+
+        LOG.info(_LI('Created RPC server for service %(service)s on host '
+                     '%(host)s.'),
+                 {'service': self.topic, 'host': self.host})
+
+    def stop(self):
+        try:
+            self.rpcserver.stop()
+            self.rpcserver.wait()
+        except Exception as e:
+            LOG.exception(_LE('Service error occurred when stopping the '
+                              'RPC server. Error: %s'), e)
+        try:
+            self.manager.del_host(deregister=self.deregister)
+        except Exception as e:
+            LOG.exception(_LE('Service error occurred when cleaning up '
+                              'the RPC manager. Error: %s'), e)
+
+        super(RPCService, self).stop(graceful=True)
+        LOG.info(_LI('Stopped RPC server for service %(service)s on host '
+                     '%(host)s.'),
+                 {'service': self.topic, 'host': self.host})
+
+    def _handle_signal(self, signo, frame):
+        LOG.info(_LI('Got signal SIGUSR1. Not deregistering on next shutdown '
+                     'of service %(service)s on host %(host)s.'),
+                 {'service': self.topic, 'host': self.host})
+        self.deregister = False
+
+    def handle_signal(self):
+        """Add a signal handler for SIGUSR1.
+
+        The handler ensures that the manager is not deregistered when it is
+        shutdown.
+        """
+        signal.signal(signal.SIGUSR1, self._handle_signal)
diff --git a/ironic/common/service.py b/ironic/common/service.py
index be4ba0fa2..a9d9cc7e5 100644
--- a/ironic/common/service.py
+++ b/ironic/common/service.py
@@ -14,90 +14,16 @@
 # License for the specific language governing permissions and limitations
 # under the License.
 
-import signal
-
-from oslo_concurrency import processutils
 from oslo_log import log
-import oslo_messaging as messaging
 from oslo_service import service
-from oslo_service import wsgi
-from oslo_utils import importutils
 
-from ironic.api import app
 from ironic.common import config
-from ironic.common import context
-from ironic.common import exception
-from ironic.common.i18n import _, _LE, _LI
-from ironic.common import rpc
 from ironic.conf import CONF
 from ironic import objects
-from ironic.objects import base as objects_base
 
 LOG = log.getLogger(__name__)
 
 
-class RPCService(service.Service):
-
-    def __init__(self, host, manager_module, manager_class):
-        super(RPCService, self).__init__()
-        self.host = host
-        manager_module = importutils.try_import(manager_module)
-        manager_class = getattr(manager_module, manager_class)
-        self.manager = manager_class(host, manager_module.MANAGER_TOPIC)
-        self.topic = self.manager.topic
-        self.rpcserver = None
-        self.deregister = True
-
-    def start(self):
-        super(RPCService, self).start()
-        admin_context = context.get_admin_context()
-
-        target = messaging.Target(topic=self.topic, server=self.host)
-        endpoints = [self.manager]
-        serializer = objects_base.IronicObjectSerializer()
-        self.rpcserver = rpc.get_server(target, endpoints, serializer)
-        self.rpcserver.start()
-
-        self.handle_signal()
-        self.manager.init_host(admin_context)
-
-        LOG.info(_LI('Created RPC server for service %(service)s on host '
-                     '%(host)s.'),
-                 {'service': self.topic, 'host': self.host})
-
-    def stop(self):
-        try:
-            self.rpcserver.stop()
-            self.rpcserver.wait()
-        except Exception as e:
-            LOG.exception(_LE('Service error occurred when stopping the '
-                              'RPC server. Error: %s'), e)
-        try:
-            self.manager.del_host(deregister=self.deregister)
-        except Exception as e:
-            LOG.exception(_LE('Service error occurred when cleaning up '
-                              'the RPC manager. Error: %s'), e)
-
-        super(RPCService, self).stop(graceful=True)
-        LOG.info(_LI('Stopped RPC server for service %(service)s on host '
-                     '%(host)s.'),
-                 {'service': self.topic, 'host': self.host})
-
-    def _handle_signal(self, signo, frame):
-        LOG.info(_LI('Got signal SIGUSR1. Not deregistering on next shutdown '
-                     'of service %(service)s on host %(host)s.'),
-                 {'service': self.topic, 'host': self.host})
-        self.deregister = False
-
-    def handle_signal(self):
-        """Add a signal handler for SIGUSR1.
-
-        The handler ensures that the manager is not deregistered when it is
-        shutdown.
-        """
-        signal.signal(signal.SIGUSR1, self._handle_signal)
-
-
 def prepare_service(argv=None):
     argv = [] if argv is None else argv
     log.register_options(CONF)
@@ -124,56 +50,3 @@ def prepare_service(argv=None):
 
 def process_launcher():
     return service.ProcessLauncher(CONF)
-
-
-class WSGIService(service.ServiceBase):
-    """Provides ability to launch ironic API from wsgi app."""
-
-    def __init__(self, name, use_ssl=False):
-        """Initialize, but do not start the WSGI server.
-
-        :param name: The name of the WSGI server given to the loader.
-        :param use_ssl: Wraps the socket in an SSL context if True.
-        :returns: None
-        """
-        self.name = name
-        self.app = app.VersionSelectorApplication()
-        self.workers = (CONF.api.api_workers or
-                        processutils.get_worker_count())
-        if self.workers and self.workers < 1:
-            raise exception.ConfigInvalid(
-                _("api_workers value of %d is invalid, "
-                  "must be greater than 0.") % self.workers)
-
-        self.server = wsgi.Server(CONF, name, self.app,
-                                  host=CONF.api.host_ip,
-                                  port=CONF.api.port,
-                                  use_ssl=use_ssl)
-
-    def start(self):
-        """Start serving this service using loaded configuration.
-
-        :returns: None
-        """
-        self.server.start()
-
-    def stop(self):
-        """Stop serving this API.
-
-        :returns: None
-        """
-        self.server.stop()
-
-    def wait(self):
-        """Wait for the service to stop serving this API.
-
-        :returns: None
-        """
-        self.server.wait()
-
-    def reset(self):
-        """Reset server greenpool size to default.
-
-        :returns: None
-        """
-        self.server.reset()
diff --git a/ironic/common/wsgi_service.py b/ironic/common/wsgi_service.py
new file mode 100644
index 000000000..c2a9f235b
--- /dev/null
+++ b/ironic/common/wsgi_service.py
@@ -0,0 +1,76 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+from oslo_concurrency import processutils
+from oslo_log import log
+from oslo_service import service
+from oslo_service import wsgi
+
+from ironic.api import app
+from ironic.common import exception
+from ironic.common.i18n import _
+from ironic.conf import CONF
+
+LOG = log.getLogger(__name__)
+
+
+class WSGIService(service.ServiceBase):
+    """Provides ability to launch ironic API from wsgi app."""
+
+    def __init__(self, name, use_ssl=False):
+        """Initialize, but do not start the WSGI server.
+
+        :param name: The name of the WSGI server given to the loader.
+        :param use_ssl: Wraps the socket in an SSL context if True.
+        :returns: None
+        """
+        self.name = name
+        self.app = app.VersionSelectorApplication()
+        self.workers = (CONF.api.api_workers or
+                        processutils.get_worker_count())
+        if self.workers and self.workers < 1:
+            raise exception.ConfigInvalid(
+                _("api_workers value of %d is invalid, "
+                  "must be greater than 0.") % self.workers)
+
+        self.server = wsgi.Server(CONF, name, self.app,
+                                  host=CONF.api.host_ip,
+                                  port=CONF.api.port,
+                                  use_ssl=use_ssl)
+
+    def start(self):
+        """Start serving this service using loaded configuration.
+
+        :returns: None
+        """
+        self.server.start()
+
+    def stop(self):
+        """Stop serving this API.
+
+        :returns: None
+        """
+        self.server.stop()
+
+    def wait(self):
+        """Wait for the service to stop serving this API.
+
+        :returns: None
+        """
+        self.server.wait()
+
+    def reset(self):
+        """Reset server greenpool size to default.
+
+        :returns: None
+        """
+        self.server.reset()
diff --git a/ironic/tests/unit/common/test_rpc_service.py b/ironic/tests/unit/common/test_rpc_service.py
new file mode 100644
index 000000000..b1e064730
--- /dev/null
+++ b/ironic/tests/unit/common/test_rpc_service.py
@@ -0,0 +1,53 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import mock
+from oslo_config import cfg
+import oslo_messaging
+from oslo_service import service as base_service
+
+from ironic.common import context
+from ironic.common import rpc
+from ironic.common import rpc_service
+from ironic.conductor import manager
+from ironic.objects import base as objects_base
+from ironic.tests import base
+
+CONF = cfg.CONF
+
+
+@mock.patch.object(base_service.Service, '__init__', lambda *_, **__: None)
+class TestRPCService(base.TestCase):
+
+    def setUp(self):
+        super(TestRPCService, self).setUp()
+        host = "fake_host"
+        mgr_module = "ironic.conductor.manager"
+        mgr_class = "ConductorManager"
+        self.rpc_svc = rpc_service.RPCService(host, mgr_module, mgr_class)
+
+    @mock.patch.object(oslo_messaging, 'Target', autospec=True)
+    @mock.patch.object(objects_base, 'IronicObjectSerializer', autospec=True)
+    @mock.patch.object(rpc, 'get_server', autospec=True)
+    @mock.patch.object(manager.ConductorManager, 'init_host', autospec=True)
+    @mock.patch.object(context, 'get_admin_context', autospec=True)
+    def test_start(self, mock_ctx, mock_init_method,
+                   mock_rpc, mock_ios, mock_target):
+        mock_rpc.return_value.start = mock.MagicMock()
+        self.rpc_svc.handle_signal = mock.MagicMock()
+        self.rpc_svc.start()
+        mock_ctx.assert_called_once_with()
+        mock_target.assert_called_once_with(topic=self.rpc_svc.topic,
+                                            server="fake_host")
+        mock_ios.assert_called_once_with()
+        mock_init_method.assert_called_once_with(self.rpc_svc.manager,
+                                                 mock_ctx.return_value)
diff --git a/ironic/tests/unit/common/test_service.py b/ironic/tests/unit/common/test_service.py
deleted file mode 100644
index 92bcf8e1f..000000000
--- a/ironic/tests/unit/common/test_service.py
+++ /dev/null
@@ -1,100 +0,0 @@
-#    Licensed under the Apache License, Version 2.0 (the "License"); you may
-#    not use this file except in compliance with the License. You may obtain
-#    a copy of the License at
-#
-#         http://www.apache.org/licenses/LICENSE-2.0
-#
-#    Unless required by applicable law or agreed to in writing, software
-#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
-#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
-#    License for the specific language governing permissions and limitations
-#    under the License.
-
-import mock
-from oslo_concurrency import processutils
-from oslo_config import cfg
-import oslo_messaging
-from oslo_service import service as base_service
-
-from ironic.common import context
-from ironic.common import exception
-from ironic.common import rpc
-from ironic.common import service
-from ironic.conductor import manager
-from ironic.objects import base as objects_base
-from ironic.tests import base
-
-CONF = cfg.CONF
-
-
-@mock.patch.object(base_service.Service, '__init__', lambda *_, **__: None)
-class TestRPCService(base.TestCase):
-
-    def setUp(self):
-        super(TestRPCService, self).setUp()
-        host = "fake_host"
-        mgr_module = "ironic.conductor.manager"
-        mgr_class = "ConductorManager"
-        self.rpc_svc = service.RPCService(host, mgr_module, mgr_class)
-
-    @mock.patch.object(oslo_messaging, 'Target', autospec=True)
-    @mock.patch.object(objects_base, 'IronicObjectSerializer', autospec=True)
-    @mock.patch.object(rpc, 'get_server', autospec=True)
-    @mock.patch.object(manager.ConductorManager, 'init_host', autospec=True)
-    @mock.patch.object(context, 'get_admin_context', autospec=True)
-    def test_start(self, mock_ctx, mock_init_method,
-                   mock_rpc, mock_ios, mock_target):
-        mock_rpc.return_value.start = mock.MagicMock()
-        self.rpc_svc.handle_signal = mock.MagicMock()
-        self.rpc_svc.start()
-        mock_ctx.assert_called_once_with()
-        mock_target.assert_called_once_with(topic=self.rpc_svc.topic,
-                                            server="fake_host")
-        mock_ios.assert_called_once_with()
-        mock_init_method.assert_called_once_with(self.rpc_svc.manager,
-                                                 mock_ctx.return_value)
-
-
-class TestWSGIService(base.TestCase):
-    @mock.patch.object(service.wsgi, 'Server')
-    def test_workers_set_default(self, wsgi_server):
-        service_name = "ironic_api"
-        test_service = service.WSGIService(service_name)
-        self.assertEqual(processutils.get_worker_count(),
-                         test_service.workers)
-        wsgi_server.assert_called_once_with(CONF, service_name,
-                                            test_service.app,
-                                            host='0.0.0.0',
-                                            port=6385,
-                                            use_ssl=False)
-
-    @mock.patch.object(service.wsgi, 'Server')
-    def test_workers_set_correct_setting(self, wsgi_server):
-        self.config(api_workers=8, group='api')
-        test_service = service.WSGIService("ironic_api")
-        self.assertEqual(8, test_service.workers)
-
-    @mock.patch.object(service.wsgi, 'Server')
-    def test_workers_set_zero_setting(self, wsgi_server):
-        self.config(api_workers=0, group='api')
-        test_service = service.WSGIService("ironic_api")
-        self.assertEqual(processutils.get_worker_count(), test_service.workers)
-
-    @mock.patch.object(service.wsgi, 'Server')
-    def test_workers_set_negative_setting(self, wsgi_server):
-        self.config(api_workers=-2, group='api')
-        self.assertRaises(exception.ConfigInvalid,
-                          service.WSGIService,
-                          'ironic_api')
-        self.assertFalse(wsgi_server.called)
-
-    @mock.patch.object(service.wsgi, 'Server')
-    def test_wsgi_service_with_ssl_enabled(self, wsgi_server):
-        self.config(enable_ssl_api=True, group='api')
-        service_name = 'ironic_api'
-        srv = service.WSGIService('ironic_api', CONF.api.enable_ssl_api)
-        wsgi_server.assert_called_once_with(CONF, service_name,
-                                            srv.app,
-                                            host='0.0.0.0',
-                                            port=6385,
-                                            use_ssl=True)
diff --git a/ironic/tests/unit/common/test_wsgi_service.py b/ironic/tests/unit/common/test_wsgi_service.py
new file mode 100644
index 000000000..cfaadef95
--- /dev/null
+++ b/ironic/tests/unit/common/test_wsgi_service.py
@@ -0,0 +1,66 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import mock
+from oslo_concurrency import processutils
+from oslo_config import cfg
+
+from ironic.common import exception
+from ironic.common import wsgi_service
+from ironic.tests import base
+
+CONF = cfg.CONF
+
+
+class TestWSGIService(base.TestCase):
+    @mock.patch.object(wsgi_service.wsgi, 'Server')
+    def test_workers_set_default(self, mock_server):
+        service_name = "ironic_api"
+        test_service = wsgi_service.WSGIService(service_name)
+        self.assertEqual(processutils.get_worker_count(),
+                         test_service.workers)
+        mock_server.assert_called_once_with(CONF, service_name,
+                                            test_service.app,
+                                            host='0.0.0.0',
+                                            port=6385,
+                                            use_ssl=False)
+
+    @mock.patch.object(wsgi_service.wsgi, 'Server')
+    def test_workers_set_correct_setting(self, mock_server):
+        self.config(api_workers=8, group='api')
+        test_service = wsgi_service.WSGIService("ironic_api")
+        self.assertEqual(8, test_service.workers)
+
+    @mock.patch.object(wsgi_service.wsgi, 'Server')
+    def test_workers_set_zero_setting(self, mock_server):
+        self.config(api_workers=0, group='api')
+        test_service = wsgi_service.WSGIService("ironic_api")
+        self.assertEqual(processutils.get_worker_count(), test_service.workers)
+
+    @mock.patch.object(wsgi_service.wsgi, 'Server')
+    def test_workers_set_negative_setting(self, mock_server):
+        self.config(api_workers=-2, group='api')
+        self.assertRaises(exception.ConfigInvalid,
+                          wsgi_service.WSGIService,
+                          'ironic_api')
+        self.assertFalse(mock_server.called)
+
+    @mock.patch.object(wsgi_service.wsgi, 'Server')
+    def test_wsgi_service_with_ssl_enabled(self, mock_server):
+        self.config(enable_ssl_api=True, group='api')
+        service_name = 'ironic_api'
+        srv = wsgi_service.WSGIService('ironic_api', CONF.api.enable_ssl_api)
+        mock_server.assert_called_once_with(CONF, service_name,
+                                            srv.app,
+                                            host='0.0.0.0',
+                                            port=6385,
+                                            use_ssl=True)
diff --git a/releasenotes/notes/conductor_early_import-fd29fa8b89089977.yaml b/releasenotes/notes/conductor_early_import-fd29fa8b89089977.yaml
new file mode 100644
index 000000000..a16117051
--- /dev/null
+++ b/releasenotes/notes/conductor_early_import-fd29fa8b89089977.yaml
@@ -0,0 +1,5 @@
+---
+fixes:
+  - Fixes a bug which prevented the ironic-conductor service from
+    using the interval values from the configuration options, for
+    the periodic tasks. Instead, the default values had been used.