Merge lp:~james-page/charm-helpers/worker-multipler-wsgi into lp:charm-helpers

[James Page]

Refactor worker multiplier code.

Ensure WSGI processes are capped by default when running in containers.

Skew public processes in favour of admin processes.

[lp:~james-page/charm-helpers/worker-multipler-wsgi]

739. By James Page on 2017-04-26

Tidy docstrings

[Alex Kavanagh (ajkavanagh)]

Looks good. Just a thought on Pythonic idiomatic code.

[lp:~james-page/charm-helpers/worker-multipler-wsgi]

740. By James Page on 2017-04-26

Make things more pythonic

[Alex Kavanagh (ajkavanagh)]

LGTM.

review: Approve

=== modified file 'charmhelpers/contrib/openstack/context.py'
--- charmhelpers/contrib/openstack/context.py	2017-04-25 17:13:07 +0000
+++ charmhelpers/contrib/openstack/context.py	2017-04-26 09:46:29 +0000
@@ -1232,31 +1232,50 @@
 DEFAULT_MULTIPLIER = 2
 
 
+def _calculate_workers():
+    '''
+    Determine the number of worker processes based on the CPU
+    count of the unit containing the application.
+
+    Workers will be limited to MAX_DEFAULT_WORKERS in
+    container environments where no worker-multipler configuration
+    option been set.
+
+    @returns int: number of worker processes to use
+    '''
+    multiplier = config('worker-multiplier') or DEFAULT_MULTIPLIER
+    count = int(_num_cpus() * multiplier)
+    if multiplier > 0 and count == 0:
+        count = 1
+
+    if config('worker-multiplier') is None and is_container():
+        # NOTE(jamespage): Limit unconfigured worker-multiplier
+        #                  to MAX_DEFAULT_WORKERS to avoid insane
+        #                  worker configuration in LXD containers
+        #                  on large servers
+        # Reference: https://pad.lv/1665270
+        count = min(count, MAX_DEFAULT_WORKERS)
+
+    return count
+
+
+def _num_cpus():
+    '''
+    Compatibility wrapper for calculating the number of CPU's
+    a unit has.
+
+    @returns: int: number of CPU cores detected
+    '''
+    try:
+        return psutil.cpu_count()
+    except AttributeError:
+        return psutil.NUM_CPUS
+
+
 class WorkerConfigContext(OSContextGenerator):
 
-    @property
-    def num_cpus(self):
-        # NOTE: use cpu_count if present (16.04 support)
-        if hasattr(psutil, 'cpu_count'):
-            return psutil.cpu_count()
-        else:
-            return psutil.NUM_CPUS
-
     def __call__(self):
-        multiplier = config('worker-multiplier') or DEFAULT_MULTIPLIER
-        count = int(self.num_cpus * multiplier)
-        if multiplier > 0 and count == 0:
-            count = 1
-
-        if config('worker-multiplier') is None and is_container():
-            # NOTE(jamespage): Limit unconfigured worker-multiplier
-            #                  to MAX_DEFAULT_WORKERS to avoid insane
-            #                  worker configuration in LXD containers
-            #                  on large servers
-            # Reference: https://pad.lv/1665270
-            count = min(count, MAX_DEFAULT_WORKERS)
-
-        ctxt = {"workers": count}
+        ctxt = {"workers": _calculate_workers()}
         return ctxt
 
 
@@ -1264,7 +1283,7 @@
 
     def __init__(self, name=None, script=None, admin_script=None,
                  public_script=None, process_weight=1.00,
-                 admin_process_weight=0.75, public_process_weight=0.25):
+                 admin_process_weight=0.25, public_process_weight=0.75):
         self.service_name = name
         self.user = name
         self.group = name
@@ -1276,8 +1295,7 @@
         self.public_process_weight = public_process_weight
 
     def __call__(self):
-        multiplier = config('worker-multiplier') or 1
-        total_processes = self.num_cpus * multiplier
+        total_processes = _calculate_workers()
         ctxt = {
             "service_name": self.service_name,
             "user": self.user,

=== modified file 'tests/contrib/openstack/test_os_contexts.py'
--- tests/contrib/openstack/test_os_contexts.py	2017-04-19 12:42:07 +0000
+++ tests/contrib/openstack/test_os_contexts.py	2017-04-26 09:46:29 +0000
@@ -2604,15 +2604,16 @@
         }
         self.assertEquals(result, expected)
 
-    @patch.object(context, 'psutil')
+    @patch.object(context, '_calculate_workers')
     @patch.object(context, 'git_determine_python_path')
     @patch.object(context, 'git_determine_usr_bin')
-    def test_wsgi_worker_config_context(self, usr_bin, python_path, psutil):
+    def test_wsgi_worker_config_context(self, usr_bin, python_path,
+                                        _calculate_workers):
         self.config.return_value = 2  # worker-multiplier=2
         usr_bin_path = '/usr/bin'
         usr_bin.return_value = usr_bin_path
         python_path.return_value = None
-        psutil.cpu_count.return_value = 4
+        _calculate_workers.return_value = 8
         service_name = 'service-name'
         script = '/usr/bin/script'
         ctxt = context.WSGIWorkerConfigContext(name=service_name,
@@ -2625,8 +2626,8 @@
             "admin_script": None,
             "public_script": None,
             "processes": 8,
-            "admin_processes": 6,
-            "public_processes": 2,
+            "admin_processes": 2,
+            "public_processes": 6,
             "threads": 1,
             "usr_bin": usr_bin_path,
             "python_path": None,
@@ -2688,83 +2689,74 @@
                           {'notifications': 'True'})
 
     @patch.object(context, 'psutil')
-    def test_workerconfig_context_xenial(self, _psutil):
-        self.config.side_effect = fake_config({
-            'worker-multiplier': 1,
-        })
+    def test_num_cpus_xenial(self, _psutil):
         _psutil.cpu_count.return_value = 4
-        worker = context.WorkerConfigContext()
-        self.assertTrue(worker.num_cpus, 4)
+        self.assertTrue(context._num_cpus(), 4)
 
     @patch.object(context, 'psutil')
-    def test_workerconfig_context_trusty(self, _psutil):
-        self.config.side_effect = fake_config({
-            'worker-multiplier': 2,
-        })
+    def test_num_cpus_trusty(self, _psutil):
         _psutil.NUM_CPUS = 4
-        worker = context.WorkerConfigContext()
-        self.assertTrue(worker.num_cpus, 2)
+        self.assertTrue(context._num_cpus(), 4)
 
-    def test_workerconfig_context_float(self):
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_float(self, _num_cpus):
         self.config.side_effect = fake_config({
             'worker-multiplier': 0.3
         })
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 4):
-            worker = context.WorkerConfigContext()
-            self.assertTrue(worker.num_cpus, 1)
+        _num_cpus.return_value = 4
+        self.assertTrue(context._calculate_workers(), 4)
 
-    def test_workerconfig_context_not_quite_0(self):
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_not_quite_0(self, _num_cpus):
         # Make sure that the multiplier evaluating to somewhere between
         # 0 and 1 in the floating point range still has at least one
         # worker.
         self.config.side_effect = fake_config({
             'worker-multiplier': 0.001
         })
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 100):
-            worker = context.WorkerConfigContext()
-            self.assertTrue(worker.num_cpus, 1)
+        _num_cpus.return_value = 100
+        self.assertTrue(context._calculate_workers(), 1)
 
-    def test_workerconfig_context_0(self):
+    @patch.object(context, 'psutil')
+    def test_calculate_workers_0(self, _psutil):
         self.config.side_effect = fake_config({
             'worker-multiplier': 0
         })
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 2):
-            worker = context.WorkerConfigContext()
-            self.assertTrue(worker.num_cpus, 0)
-
-    def test_workerconfig_context_noconfig(self):
-        self.config.return_value = None
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 1):
-            worker = context.WorkerConfigContext()
-            self.assertEqual({'workers': 2}, worker())
-
-    def test_workerconfig_context_noconfig_container(self):
-        self.config.return_value = None
-        self.is_container.return_value = True
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 1):
-            worker = context.WorkerConfigContext()
-            self.assertEqual({'workers': 2}, worker())
-
-    def test_workerconfig_context_noconfig_lotsa_cpus_container(self):
-        self.config.return_value = None
-        self.is_container.return_value = True
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 32):
-            worker = context.WorkerConfigContext()
-            self.assertEqual({'workers': 4}, worker())
-
-    def test_workerconfig_context_noconfig_lotsa_cpus_not_container(self):
-        self.config.return_value = None
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 32):
-            worker = context.WorkerConfigContext()
-            self.assertEqual({'workers': 64}, worker())
-
-    def test_workerconfig_context_withconfig(self):
-        self.config.side_effect = fake_config({
-            'worker-multiplier': 4,
-        })
-        with patch.object(context.WorkerConfigContext, 'num_cpus', 2):
-            worker = context.WorkerConfigContext()
-            self.assertEqual({'workers': 8}, worker())
+        _psutil.cpu_count.return_value = 2
+        self.assertTrue(context._calculate_workers(), 0)
+
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_noconfig(self, _num_cpus):
+        self.config.return_value = None
+        _num_cpus.return_value = 1
+        self.assertTrue(context._calculate_workers(), 2)
+
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_noconfig_container(self, _num_cpus):
+        self.config.return_value = None
+        self.is_container.return_value = True
+        _num_cpus.return_value = 1
+        self.assertTrue(context._calculate_workers(), 2)
+
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_noconfig_lotsa_cpus_container(self,
+                                                             _num_cpus):
+        self.config.return_value = None
+        self.is_container.return_value = True
+        _num_cpus.return_value = 32
+        self.assertTrue(context._calculate_workers(), 4)
+
+    @patch.object(context, '_num_cpus')
+    def test_calculate_workers_noconfig_lotsa_cpus_not_container(self,
+                                                                 _num_cpus):
+        self.config.return_value = None
+        _num_cpus.return_value = 32
+        self.assertTrue(context._calculate_workers(), 64)
+
+    @patch.object(context, '_calculate_workers', return_value=256)
+    def test_worker_context(self, calculate_workers):
+        self.assertEqual(context.WorkerConfigContext()(),
+                         {'workers': 256})
 
     def test_apache_get_addresses_no_network_splits(self):
         self.https.return_value = True