Merge lp:~frankban/juju-deployer/unit-placement-fix into lp:juju-deployer

[Francesco Banconi]

Fix deployments of bundles v4.

Fix some incorrect behavior of new v4 bundles, mostly
concerning validation and ordering of services.

Improve status retrieval while adding unit.

Fix checking for existing machines: it did work
properly only with top level machines.

Fix a problem with machines mapping while
co-locating units to other services, units or
to declared machines.

Improve the logic used for detecting v4 vs v3
bundle syntax: the machines section in v4 can
be safely omitted accotding to the spec.

Fix sleep loops and memory leaks while running
tests exercising the importer.

Some lint/code style clean up.

Bump version up a little.

[Tim Van Steenburgh (tvansteenburgh)]

One inline comment, plus I wonder if you could point out which change fixes the test memory leak problem? Otherwise looks good.

[Brad Crittenden (bac)]

Thanks for this fix Francesco. It LGTM.

review: Approve (code)

[Francesco Banconi (frankban)]

As mentioned on IRC, the sleep loops are solved by setting up the wnv mock with patch_env_status.
I also removed the reloading logic, as requested.
Thank you both for the reviews!

[lp:~frankban/juju-deployer/unit-placement-fix]

159. By Francesco Banconi on 2015-08-06

Remove the reloading logic in importer.add_unit.

[Tim Van Steenburgh (tvansteenburgh)]

LGTM!

review: Approve

[Madison Scott-Clary (makyo)]

Thanks Francesco, much appreciated, this LGTM. QA okay on local.

review: Approve (code and qa)

=== modified file 'deployer/action/importer.py'
--- deployer/action/importer.py	2015-08-04 12:50:12 +0000
+++ deployer/action/importer.py	2015-08-06 14:43:20 +0000
@@ -14,13 +14,11 @@
         self.options = options
         self.env = env
         self.deployment = deployment
-        self.machines = {}
 
     def add_units(self):
         self.log.debug("Adding units...")
         # Add units to existing services that don't match count.
         env_status = self.env.status()
-        reloaded = False
 
         # Workaround an issue where watch output doesn't include subordinate
         # services right away, and add_unit would fail while attempting to add
@@ -56,18 +54,9 @@
             self.log.info(
                 "Adding %d more units to %s" % (abs(delta), svc.name))
             if svc.unit_placement:
-                # Reload status once after non placed services units are done.
-                if reloaded is False:
-                    # Improved crappy workaround juju-core api inconsistency
-                    delay = time.time() + 60
-                    while delay > time.time():
-                        time.sleep(5.1)
-                        env_status = self.env.status()
-                        if svc.name in env_status['services']:
-                            break
-
-                    reloaded = True
-
+                # Reload status before each placed unit is deployed, so that
+                # co-location to other units can take place properly.
+                env_status = self.env.status()
                 placement = self.deployment.get_unit_placement(svc, env_status)
                 for mid in range(cur_units, svc.num_units):
                     self.env.add_unit(svc.name, placement.get(mid))
@@ -76,8 +65,7 @@
 
     def machine_exists(self, id):
         """Checks if the given id exists on the current environment."""
-        return int(id) in map(int,
-                              self.env.status().get('machines', {}).keys())
+        return str(id) in map(str, self.env.status().get('machines', {}))
 
     def create_machines(self):
         """Create machines as specified in the machine spec in the bundle.
@@ -104,19 +92,23 @@
         creation is skipped.
         """
         machines = self.deployment.get_machines()
+        machines_map = {}
 
         if machines:
             self.log.info("Creating machines...")
             for machine_name, spec in machines.items():
                 if self.machine_exists(machine_name):
+                    # XXX frankban: do we really want this? The machine
+                    # identifiers as included in v4 bundles are not intended
+                    # to refer to real machines. A mapping could be provided
+                    # but this kind of implicit mapping seems weak.
                     self.log.info(
                         """Machine %s already exists on environment, """
                         """skipping creation""" % machine_name)
-                    self.machines[machine_name] = self.deployment.get_machine(
-                        machine_name)
+                    machines_map[machine_name] = str(machine_name)
                 else:
                     self.log.info("Machine %s will be created" % machine_name)
-                    self.machines[machine_name] = self.env.add_machine(
+                    machines_map[machine_name] = self.env.add_machine(
                         series=spec.get('series',
                                         self.deployment.data['series']),
                         constraints=spec.get('constraints'))
@@ -125,7 +117,7 @@
                     annotations = spec.get('annotations')
                     if annotations:
                         self.env.set_annotation(
-                            self.machines[machine_name],
+                            machines_map[machine_name],
                             annotations,
                             entity_type='machine')
 
@@ -139,13 +131,12 @@
                     if self.machine_exists(machine_name):
                         self.log.info("Machine %s already exists,"
                                       "skipping creation" % machine_name)
-                        self.machines[container_host] = self.deployment.get_machine(
-                            container_host)
+                        machines_map[container_host] = str(container_host)
                     else:
                         self.log.info("A new container will be created"
                                       "on machine: %s" % container_host)
-                        self.machines[container_host] = self.env.add_machine()
-            self.deployment.set_machines(machines)
+                        machines_map[container_host] = self.env.add_machine()
+            self.deployment.set_machines(machines_map)
 
     def get_charms(self):
         # Get Charms
 
=== modified file 'deployer/config.py'
--- deployer/config.py	2015-03-16 20:00:02 +0000
+++ deployer/config.py	2015-08-06 14:43:20 +0000
@@ -51,9 +51,9 @@
                 raise
 
         # Check if this is a v4 bundle.
-        if 'services' in yaml_result:
-            if 'machines' in yaml_result:
-                self.version = 4
+        services = yaml_result.get('services')
+        if isinstance(services, dict) and 'services' not in services:
+            self.version = 4
             yaml_result = {config_file: yaml_result}
 
         self.yaml[config_file] = yaml_result
@@ -64,7 +64,7 @@
         return sorted(self.data)
 
     def get(self, key):
-        if not key in self.data:
+        if key not in self.data:
             self.log.warning("Deployment %r not found. Available %s",
                              key, ", ".join(self.keys()))
             raise ErrorExit()
@@ -99,7 +99,7 @@
         return parents
 
     def _resolve_inherited(self, deploy_data):
-        if not 'inherits' in deploy_data:
+        if 'inherits' not in deploy_data:
             return deploy_data
         inherits = parents = self._inherits(deploy_data)
         for parent_name in parents:
 
=== modified file 'deployer/deployment.py'
--- deployer/deployment.py	2015-05-13 14:16:46 +0000
+++ deployer/deployment.py	2015-08-06 14:43:20 +0000
@@ -38,7 +38,7 @@
         pprint.pprint(self.data)
 
     def get_service(self, name):
-        if not name in self.data['services']:
+        if name not in self.data['services']:
             return
         return Service(name, self.data['services'][name])
 
@@ -95,9 +95,9 @@
                 # Check for colocation.  This naively assumes that there is no
                 # circularity in placements.
                 if x_in_y(svc_b, svc_a):
+                    return 1
+                if x_in_y(svc_a, svc_b):
                     return -1
-                if x_in_y(svc_a, svc_b):
-                    return 1
                 # If no colocation exists, simply compare names.
                 return cmp(svc_a.name, svc_b.name)
             return 1
 
=== modified file 'deployer/service.py'
--- deployer/service.py	2015-03-18 18:19:43 +0000
+++ deployer/service.py	2015-08-06 14:43:20 +0000
@@ -220,7 +220,7 @@
 
     def _fill_placement(self):
         """Fill the placement spec with necessary data.
-        
+
         From the spec:
         A unit placement may be specified with a service name only, in which
         case its unit number is assumed to be one more than the unit number of
@@ -238,7 +238,7 @@
             return
 
         self.service.svc_data['to'] = (
-            unit_mapping + 
+            unit_mapping +
             list(itertools.repeat(unit_mapping[-1], unit_count - len(unit_mapping)))
         )
         unit_mapping = self.service.unit_placement
@@ -256,9 +256,9 @@
 
     def _parse_placement(self, placement):
         """Parse a unit placement statement.
-        
+
         In version 4 bundles, unit placement statements take the form of
-        
+
           (<containertype>:)?(<unit>|<machine>|new)
 
         This splits the placement into a container, a placement, and a unit
@@ -278,7 +278,7 @@
         containers, and/or services must be internally consistent, consistent
         with other services in the deployment, and consistent with any machines
         specified in the 'machines' block of the deployment.
-        
+
         A feedback object is returned, potentially with errors and warnings
         inside it.
         """
@@ -294,55 +294,51 @@
 
         services = dict([(s.name, s) for s in self.deployment.get_services()])
         machines = self.deployment.get_machines()
-        container = None
-        unit_number = None
+        service_name = self.service.name
 
         for i, placement in enumerate(unit_placement):
-            container, placement, unit_number = self._parse_placement(placement)
+            container, target, unit_number = self._parse_placement(placement)
 
+            # Validate the container type.
             if container and container not in ('lxc', 'kvm'):
                 feedback.error(
-                    "Invalid container type: %s service: %s placement: %s" \
-                        % (container, self.service.name, unit_placement[i]))
-            # XXX Nesting containers not supported yet.
-            # Makyo - 2015-03-01
-            if container is not None and not (placement.isdigit()
-                    or placement == 'new'):
-                feedback.error(
-                    "Invalid target for container: %s" % (
-                        unit_placement[i]))
+                    'Invalid container type: {} service: {} placement: {}'
+                    ''.format(container, service_name, placement))
             # Specify an existing machine (or, if the number is in the
             # list of machine specs, one of those).
-            if placement.isdigit():
-                if placement in machines:
-                    continue
-                else:
-                    feedback.error(
-                        ("Service placement to machine "
-                         "not supported %s to %s") % (
-                             self.service.name, unit_placement[i]))
-            # Specify a machine from the machine spec.
-            elif placement in self.deployment.get_machines():
+            if str(target) in machines:
                 continue
-            # Specify a service for colocation.
-            elif placement in services:
-                # Specify a particular unit for colocation.
-                if unit_number is not None and \
-                        unit_number > services[placement].num_units:
-                    feedback.error(
-                        "Service unit does not exist, %s to %s/%s" % (
-                            self.service.name, placement, unit_number))
-                elif self.deployment.get_charm_for(placement).is_subordinate():
-                    feedback.error(
-                        "Cannot place to a subordinate service: %s -> %s" % (
-                            self.service.name, placement))
+            if target.isdigit():
+                feedback.error(
+                    'Service placement to machine not supported: {} to {}'
+                    ''.format(service_name, placement))
+            # Specify a service for co-location.
+            elif target in services:
+                # Specify a particular unit for co-location.
+                if unit_number is not None:
+                    try:
+                        unit_number = int(unit_number)
+                    except (TypeError, ValueError):
+                        feedback.error(
+                            'Invalid unit number for placement: {} to {}'
+                            ''.format(service_name, unit_number))
+                        continue
+                    if unit_number > services[target].num_units:
+                        feedback.error(
+                            'Service unit does not exist: {} to {}/{}'
+                            ''.format(service_name, target, unit_number))
+                        continue
+                if self.deployment.get_charm_for(target).is_subordinate():
+                    feedback.error(
+                        'Cannot place to a subordinate service: {} -> {}'
+                        ''.format(service_name, target))
             # Create a new machine or container.
-            elif placement == 'new':
+            elif target == 'new':
                 continue
             else:
                 feedback.error(
-                        "Invalid service placement %s to %s" % (
-                        self.service.name, unit_placement[i]))
+                    'Invalid service placement: {} to {}'
+                    ''.format(service_name, placement))
         return feedback
 
     def get_new_machines_for_containers(self):
@@ -367,6 +363,8 @@
         svc = self.service
 
         unit_mapping = svc.unit_placement
+        if not unit_mapping:
+            return None
         unit_placement = placement = str(unit_mapping[unit_number])
         container = None
         u_idx = unit_number
@@ -375,14 +373,17 @@
         if placement == 'new':
             return None
 
-        container, placement, unit_number = self._parse_placement(unit_placement)
+        container, placement, unit_number = self._parse_placement(
+            unit_placement)
 
         if placement in self.machines_map:
-            return self._format_placement(self.machines_map[placement], container)
+            return self._format_placement(
+                self.machines_map[placement], container)
 
         # Handle <container_type>:new
         if placement == 'new':
             return self._format_placement(
-                self.machines_map['%s/%d' % (self.service.name, u_idx)], container)
+                self.machines_map['%s/%d' % (self.service.name, u_idx)],
+                container)
 
         return self.colocate(status, placement, u_idx, container, svc)
 
=== modified file 'deployer/tests/base.py'
--- deployer/tests/base.py	2015-03-18 18:19:43 +0000
+++ deployer/tests/base.py	2015-08-06 14:43:20 +0000
@@ -6,6 +6,8 @@
 import StringIO
 import tempfile
 
+import mock
+
 import deployer
 from deployer.config import ConfigStack
 
@@ -77,3 +79,37 @@
             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_data/v4/container-existing-machine.yaml'
--- deployer/tests/test_data/v4/container-existing-machine.yaml	2015-05-13 14:06:34 +0000
+++ deployer/tests/test_data/v4/container-existing-machine.yaml	2015-08-06 14:43:20 +0000
@@ -39,4 +39,4 @@
 - - mediawiki:db
   - mysql:db
 machines:
-  1: 1
+  1: {}
 
=== added file 'deployer/tests/test_data/v4/placement-invalid-number.yaml'
--- deployer/tests/test_data/v4/placement-invalid-number.yaml	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_data/v4/placement-invalid-number.yaml	2015-08-06 14:43:20 +0000
@@ -0,0 +1,8 @@
+series: trusty
+services:
+  django:
+    charm: cs:trusty/django
+    to:
+      - lxc:haproxy/bad-wolf
+  haproxy:
+    charm: cs:trusty/haproxy
 
=== modified file 'deployer/tests/test_deployment.py'
--- deployer/tests/test_deployment.py	2015-03-18 18:19:43 +0000
+++ deployer/tests/test_deployment.py	2015-08-06 14:43:20 +0000
@@ -106,12 +106,17 @@
             d._machines_placement_sort(
                 FauxService(name="x", unit_placement=['asdf']),
                 FauxService(name="y", unit_placement=['lxc:x/1'])
-            ), 1)
+            ), -1)
         self.assertEqual(
             d._machines_placement_sort(
                 FauxService(name="y", unit_placement=['lxc:x/1']),
                 FauxService(name="x", unit_placement=['asdf'])
-            ), -1)
+            ), 1)
+        self.assertEqual(
+            d._machines_placement_sort(
+                FauxService(name="x", unit_placement=['y']),
+                FauxService(name="y")
+            ), 1)
         self.assertEqual(
             d._machines_placement_sort(
                 FauxService(name="x", unit_placement=['asdf']),
@@ -195,12 +200,12 @@
         self.assertIn(
             'Cannot place to a subordinate service: nova-compute -> nrpe\n',
             output)
-        
+
     @skip_if_offline
     def test_validate_invalid_placement_subordinate_v4(self):
         # Placement validation fails if a subordinate charm is provided.
         deployment = self.get_deployment_and_fetch_v4(
-                'placement-invalid-subordinate.yaml')
+            'placement-invalid-subordinate.yaml')
         with self.assertRaises(ErrorExit):
             deployment.validate_placement()
         output = self.output.getvalue()
@@ -208,6 +213,15 @@
             'Cannot place to a subordinate service: nova-compute -> nrpe\n',
             output)
 
+    def test_validate_invalid_unit_number(self):
+        # Placement validation fails if an invalid unit number is provided.
+        deployment = self.get_deployment_v4('placement-invalid-number.yaml')
+        with self.assertRaises(ErrorExit):
+            deployment.validate_placement()
+        output = self.output.getvalue()
+        self.assertIn(
+            'Invalid unit number for placement: django to bad-wolf\n', output)
+
     def test_get_unit_placement_v3(self):
         d = self.get_named_deployment_v3("stack-placement.yaml", "stack")
         status = {
@@ -281,12 +295,11 @@
         feedback = placement.validate()
         self.assertEqual(feedback.get_errors(), [
             'Invalid container type: asdf service: mysql placement: asdf:0',
-            'Service placement to machine not supported mysql to asdf:0',
-            'Invalid target for container: lxc:asdf',
-            'Invalid service placement mysql to lxc:asdf',
-            'Service placement to machine not supported mysql to 1',
-            'Service unit does not exist, mysql to wordpress/3',
-            'Invalid service placement mysql to asdf'])
+            'Service placement to machine not supported: mysql to asdf:0',
+            'Invalid service placement: mysql to lxc:asdf',
+            'Service placement to machine not supported: mysql to 1',
+            'Service unit does not exist: mysql to wordpress/3',
+            'Invalid service placement: mysql to asdf'])
 
     def test_get_unit_placement_v4_simple(self):
         d = self.get_deployment_v4('simple.yaml')
 
=== modified file 'deployer/tests/test_guiserver.py'
--- deployer/tests/test_guiserver.py	2015-03-17 10:40:34 +0000
+++ deployer/tests/test_guiserver.py	2015-08-06 14:43:20 +0000
@@ -11,7 +11,10 @@
 
 from deployer import guiserver
 from deployer.feedback import Feedback
-from deployer.tests.base import skip_if_offline
+from deployer.tests.base import (
+    patch_env_status,
+    skip_if_offline,
+)
 
 
 class TestGetDefaultGuiserverOptions(unittest.TestCase):
@@ -268,6 +271,8 @@
     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()
         mock_sleep.assert_has_calls([mock.call(5.1), mock.call(60)])
@@ -289,8 +294,7 @@
                 None, 1, None),
             mock.call.set_annotation(
                 'wordpress', {'gui-y': '414.547', 'gui-x': '425.347'}),
-            mock.call.add_units('mysql', 2),
-            mock.call.add_units('wordpress', 1),
+            mock.call.add_units('mysql', 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	2015-05-13 14:06:34 +0000
+++ deployer/tests/test_importer.py	2015-08-06 14:43:20 +0000
@@ -6,7 +6,11 @@
 from deployer.action.importer import Importer
 from deployer.utils import yaml_dump, yaml_load
 
-from base import Base, skip_if_offline
+from base import (
+    Base,
+    patch_env_status,
+    skip_if_offline,
+)
 
 
 class Options(dict):
@@ -51,6 +55,7 @@
         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()
 
@@ -60,10 +65,6 @@
                 'wiki', 'cs:precise/mediawiki', '', config, None, 1, None))
         env.add_relation.assert_called_once_with('wiki', 'db')
 
-        self.assertEqual(
-            yaml_load(yaml_dump(yaml_load(yaml_dump(config)))),
-            config)
-
     @skip_if_offline
     @mock.patch('deployer.action.importer.time')
     def test_importer_no_relations(self, mock_time):
@@ -71,10 +72,10 @@
         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, False)
+        self.assertFalse(env.add_relation.called)
 
     @skip_if_offline
     @mock.patch('deployer.action.importer.time')
@@ -84,6 +85,7 @@
         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()
 
@@ -104,14 +106,8 @@
         stack = ConfigStack(self.options.configs)
         deploy = stack.get(self.options.configs[0])
         env = mock.MagicMock()
-
-        config = {'status.return_value.get.return_value': {
-            1: {}
-        }}
-
-        env.configure_mock(**config)
+        patch_env_status(env, {'mediawiki': 1, 'mysql': 1}, machines=[1])
 
         importer = Importer(env, deploy, self.options)
         importer.run()
-
-        env.add_machine.assert_not_called()
+        self.assertFalse(env.add_machine.called)
 
=== modified file 'setup.py'
--- setup.py	2015-08-04 17:57:46 +0000
+++ setup.py	2015-08-06 14:43:20 +0000
@@ -3,7 +3,7 @@
 
 setup(
     name="juju-deployer",
-    version="0.5.0",
+    version="0.5.1",
     description="A tool for deploying complex stacks with juju.",
     long_description=open("README").read(),
     author="Kapil Thangavelu",