From c9c492725ed581db1ba49a1a5985bfcf4952e82d Mon Sep 17 00:00:00 2001
From: Dmitry Tantsur <dtantsur@protonmail.com>
Date: Tue, 3 Nov 2020 16:33:14 +0100
Subject: [PATCH] Limit the default value of [api]api_workers to 4

Each ironic-api process consumes non-negligible amount of RAM,
defaulting to CPU core count may result in many hundres of megabytes
occupied by ironic-api processes. Limit the default value to 4
and let people who actually need more than that pick their value.

Change-Id: I5aefa8c6c7aadc56aea151647e1c0a5af54ada4c
---
 ironic/common/wsgi_service.py                      | 11 +++++++++--
 ironic/conf/api.py                                 |  6 +++---
 ironic/tests/unit/common/test_wsgi_service.py      | 14 +++++++++++---
 .../notes/api-workers-c06ea95a0c55b8cf.yaml        |  9 +++++++++
 4 files changed, 32 insertions(+), 8 deletions(-)
 create mode 100644 releasenotes/notes/api-workers-c06ea95a0c55b8cf.yaml

diff --git a/ironic/common/wsgi_service.py b/ironic/common/wsgi_service.py
index 5c30d708ea..e7bbe9dcd2 100644
--- a/ironic/common/wsgi_service.py
+++ b/ironic/common/wsgi_service.py
@@ -20,6 +20,9 @@ from ironic.common.i18n import _
 from ironic.conf import CONF
 
 
+_MAX_DEFAULT_WORKERS = 4
+
+
 class WSGIService(service.ServiceBase):
     """Provides ability to launch ironic API from wsgi app."""
 
@@ -32,8 +35,12 @@ class WSGIService(service.ServiceBase):
         """
         self.name = name
         self.app = app.VersionSelectorApplication()
-        self.workers = (CONF.api.api_workers
-                        or processutils.get_worker_count())
+        self.workers = (
+            CONF.api.api_workers
+            # NOTE(dtantsur): each worker takes a substantial amount of memory,
+            # so we don't want to end up with dozens of them.
+            or min(processutils.get_worker_count(), _MAX_DEFAULT_WORKERS)
+        )
         if self.workers and self.workers < 1:
             raise exception.ConfigInvalid(
                 _("api_workers value of %d is invalid, "
diff --git a/ironic/conf/api.py b/ironic/conf/api.py
index 80a30acfc0..ba23627b08 100644
--- a/ironic/conf/api.py
+++ b/ironic/conf/api.py
@@ -44,9 +44,9 @@ opts = [
                ),
     cfg.IntOpt('api_workers',
                help=_('Number of workers for OpenStack Ironic API service. '
-                      'The default is equal to the number of CPUs available '
-                      'if that can be determined, else a default worker '
-                      'count of 1 is returned.')),
+                      'The default is equal to the number of CPUs available, '
+                      'but not more than 4. One worker is used if the CPU '
+                      'number cannot be detected.')),
     cfg.BoolOpt('enable_ssl_api',
                 default=False,
                 help=_("Enable the integrated stand-alone API to service "
diff --git a/ironic/tests/unit/common/test_wsgi_service.py b/ironic/tests/unit/common/test_wsgi_service.py
index 5af26bf37f..bc63c0dd26 100644
--- a/ironic/tests/unit/common/test_wsgi_service.py
+++ b/ironic/tests/unit/common/test_wsgi_service.py
@@ -23,12 +23,12 @@ CONF = cfg.CONF
 
 
 class TestWSGIService(base.TestCase):
+    @mock.patch.object(processutils, 'get_worker_count', lambda: 2)
     @mock.patch.object(wsgi_service.wsgi, 'Server', autospec=True)
     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)
+        self.assertEqual(2, test_service.workers)
         mock_server.assert_called_once_with(CONF, service_name,
                                             test_service.app,
                                             host='0.0.0.0',
@@ -41,11 +41,19 @@ class TestWSGIService(base.TestCase):
         test_service = wsgi_service.WSGIService("ironic_api")
         self.assertEqual(8, test_service.workers)
 
+    @mock.patch.object(processutils, 'get_worker_count', lambda: 3)
     @mock.patch.object(wsgi_service.wsgi, 'Server', autospec=True)
     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)
+        self.assertEqual(3, test_service.workers)
+
+    @mock.patch.object(processutils, 'get_worker_count', lambda: 42)
+    @mock.patch.object(wsgi_service.wsgi, 'Server', autospec=True)
+    def test_workers_set_default_limit(self, mock_server):
+        self.config(api_workers=0, group='api')
+        test_service = wsgi_service.WSGIService("ironic_api")
+        self.assertEqual(4, test_service.workers)
 
     @mock.patch.object(wsgi_service.wsgi, 'Server', autospec=True)
     def test_workers_set_negative_setting(self, mock_server):
diff --git a/releasenotes/notes/api-workers-c06ea95a0c55b8cf.yaml b/releasenotes/notes/api-workers-c06ea95a0c55b8cf.yaml
new file mode 100644
index 0000000000..075b8e2a46
--- /dev/null
+++ b/releasenotes/notes/api-workers-c06ea95a0c55b8cf.yaml
@@ -0,0 +1,9 @@
+---
+upgrade:
+  - |
+    The default value of ``[api]api_workers`` is now limited to 4. Set it
+    explicitly if you need a higher value.
+fixes:
+  - |
+    No longer launches too many API workers on systems with a lot of CPU
+    cores by default.