Merge lp:~wgrant/juju-deployer/slaughter-suboordinate-slumber into lp:juju-deployer

[William Grant]

Refactor subordinate 60-second sleep away.

Refactor subordinate 60-second sleep away.

r146 caused add_units to wait 60 seconds for every subordinate service.
This was to avoid a crash when it attempted to retrieve the number of
units of the service before Juju had had a chance to include the service
in status output yet. But add_units just skips any subordinate service
anyway, so a much less slow fix is to just move the subordinate check to
the top of the loop.

I also cleaned up some tests which patched the mock environment to skip
the formerly dreadfully slow bits of add_units.

[Tim Van Steenburgh (tvansteenburgh)]

This is great, thanks William. I left one inline comment for your review.

review: Needs Fixing

[lp:~wgrant/juju-deployer/slaughter-suboordinate-slumber]

201. By William Grant on 2016-11-03

Fix potential crash if units is None.

[Tim Van Steenburgh (tvansteenburgh)]

LGTM. Bonus points for the alliterate branch name.

review: Approve

=== modified file 'deployer/action/importer.py'
--- deployer/action/importer.py	2016-09-22 14:10:50 +0000
+++ deployer/action/importer.py	2016-11-03 12:52:41 +0000
@@ -23,22 +23,15 @@
         # Add units to existing services that don't match count.
         env_status = self.env.status()
 
-        # Workaround an issue where watch output doesn't include subordinate
-        # services right away, and add_unit would fail while attempting to add
-        # units to a non-existant service. See lp:1421315 for details.
-        cur_units = 0
         for svc in self.deployment.get_services():
-            delay = time.time() + 60
-            while delay > time.time():
-                if svc.name in env_status['services']:
-                    cur_units = len(
-                        (env_status['services'][svc.name].get('units') or ())
-                    )
-                    if cur_units > 0:
-                        break
-                time.sleep(5)
-                env_status = self.env.status()
+            charm = self.deployment.get_charm_for(svc.name)
+            if charm.is_subordinate():
+                self.log.warning(
+                    "Config specifies num units for subordinate: %s",
+                    svc.name)
+                continue
 
+            cur_units = len(env_status['services'][svc.name].get('units') or ())
             delta = (svc.num_units - cur_units)
 
             if delta <= 0:
@@ -47,13 +40,6 @@
                     svc.name)
                 continue
 
-            charm = self.deployment.get_charm_for(svc.name)
-            if charm.is_subordinate():
-                self.log.warning(
-                    "Config specifies num units for subordinate: %s",
-                    svc.name)
-                continue
-
             self.log.info(
                 "Adding %d more units to %s" % (abs(delta), svc.name))
             if svc.unit_placement:

=== modified file 'deployer/tests/base.py'
--- deployer/tests/base.py	2016-05-07 23:48:30 +0000
+++ deployer/tests/base.py	2016-11-03 12:52:41 +0000
@@ -1,13 +1,12 @@
 from __future__ import absolute_import
 import inspect
+import itertools
 import logging
 import os
 import unittest
 import shutil
 import tempfile
 
-import mock
-
 import deployer
 from deployer.config import ConfigStack
 
@@ -82,37 +81,3 @@
             os.environ.update(original_environ)
 
         os.environ.update(kw)
-
-
-def patch_env_status(env, services, machines=()):
-    """Simulate that the given mock env has the status described in services.
-
-    This function is used so that tests do not have to wait minutes for
-    service units presence when the importer is used with the given env.
-
-    The services argument is a dict mapping service names with the number of
-    their units. This will be reflected by the status returned when the
-    importer adds the units (see "deployer.action.importer.Importer.add_unit").
-
-    The machines argument can be used to simulate that the given machines are
-    present in the Juju environment.
-    """
-    services_status = dict(
-        (k, {'units': dict((i, {}) for i in range(v))})
-        for k, v in services.items()
-    )
-    machines_status = dict((i, {}) for i in machines)
-    env.status.side_effect = [
-        # There are no services initially.
-        {'services': {}, 'machines': machines_status},
-        {'services': {}, 'machines': machines_status},
-        # This is the workaround check for subordinate charms presence:
-        # see lp:1421315 for details.
-        {'services': services_status, 'machines': machines_status},
-        {'services': services_status, 'machines': machines_status},
-        # After we exited the workaround loop, we can just mock further
-        # status results.
-        mock.MagicMock(),
-        mock.MagicMock(),
-        mock.MagicMock(),
-    ]

=== modified file 'deployer/tests/test_guiserver.py'
--- deployer/tests/test_guiserver.py	2016-07-12 02:29:52 +0000
+++ deployer/tests/test_guiserver.py	2016-11-03 12:52:41 +0000
@@ -12,10 +12,7 @@
 
 from deployer import guiserver
 from deployer.feedback import Feedback
-from deployer.tests.base import (
-    patch_env_status,
-    skip_if_offline,
-)
+from deployer.tests.base import skip_if_offline
 
 
 class TestGetDefaultGuiserverOptions(unittest.TestCase):
@@ -274,8 +271,6 @@
     def test_importer_behavior(self, mock_sleep, mock_environment):
         # The importer executes the expected environment calls.
         self.addCleanup(self.cleanup_series_path)
-        patch_env_status(mock_environment(), {'mysql': 1, 'wordpress': 1})
-        mock_environment.reset_mock()
         with self.patch_juju_home():
             self.import_bundle(version=3)
         mock_sleep.assert_has_calls([mock.call(5.1), mock.call(60)])
@@ -298,7 +293,8 @@
                 None, 1, None, 'precise'),
             mock.call.set_annotation(
                 'wordpress', {'gui-y': '414.547', 'gui-x': '425.347'}),
-            mock.call.add_units('mysql', 1),
+            mock.call.add_units('mysql', 2),
+            mock.call.add_units('wordpress', 1),
             mock.call.add_relation('mysql:db', 'wordpress:db'),
             mock.call.close(),
         ], any_order=True)

=== modified file 'deployer/tests/test_importer.py'
--- deployer/tests/test_importer.py	2016-05-07 23:48:30 +0000
+++ deployer/tests/test_importer.py	2016-11-03 12:52:41 +0000
@@ -8,7 +8,6 @@
 
 from .base import (
     Base,
-    patch_env_status,
     skip_if_offline,
 )
 
@@ -56,7 +55,6 @@
         stack = ConfigStack(self.options.configs)
         deploy = stack.get('wiki')
         env = mock.MagicMock()
-        patch_env_status(env, {'wiki': 1, 'db': 1})
         importer = Importer(env, deploy, self.options)
         importer.run()
 
@@ -76,7 +74,6 @@
         stack = ConfigStack(self.options.configs)
         deploy = stack.get('wiki')
         env = mock.MagicMock()
-        patch_env_status(env, {'wiki': 1, 'db': 1})
         importer = Importer(env, deploy, self.options)
         importer.run()
         self.assertFalse(env.add_relation.called)
@@ -90,7 +87,6 @@
         stack = ConfigStack(self.options.configs)
         deploy = stack.get(self.options.configs[0])
         env = mock.MagicMock()
-        patch_env_status(env, {'mediawiki': 1, 'mysql': 1})
         importer = Importer(env, deploy, self.options)
         importer.run()
 
@@ -110,7 +106,12 @@
         stack = ConfigStack(self.options.configs)
         deploy = stack.get(self.options.configs[0])
         env = mock.MagicMock()
-        patch_env_status(env, {'mediawiki': 1, 'mysql': 1}, machines=[1])
+
+        config = {'status.return_value.get.return_value': {
+            1: {}
+        }}
+
+        env.configure_mock(**config)
 
         importer = Importer(env, deploy, self.options)
         importer.run()