Merge lp:~niedbalski/juju-deployer/fix-lp1454720 into lp:juju-deployer

[Jorge Niedbalski]

Not forcefully create a machine if already exists on the environment.

[lp:~niedbalski/juju-deployer/fix-lp1454720]

147. By Jorge Niedbalski on 2015-05-13

Added docstrings

[Tim Van Steenburgh (tvansteenburgh)]

LGTM, thanks Jorge! One inline comment for cleanup.

review: Approve

[lp:~niedbalski/juju-deployer/fix-lp1454720]

148. By Jorge Niedbalski on 2015-05-13

Addressed @tvan review

[Jorge Niedbalski (niedbalski)]

@tvansteenburgh

Thanks for reviewing. I just Re-submitted using your recommendations

=== modified file 'deployer/action/importer.py'
--- deployer/action/importer.py	2015-03-24 17:38:56 +0000
+++ deployer/action/importer.py	2015-05-13 19:46:10 +0000
@@ -56,6 +56,11 @@
             else:
                 self.env.add_units(svc.name, abs(delta))
 
+    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())
+
     def create_machines(self):
         """Create machines as specified in the machine spec in the bundle.
 
@@ -70,26 +75,42 @@
             foo: bar
         1:
           constraints: "mem=4G"
-        
+
         This method first attempts to create any machines in the 'machines'
         section of the bundle specification with the given constraints and
         annotations.  Then, if there are any machines requested for containers
         in the style of <container type>:new, it creates those machines and
-        adds them to the machines map."""
+        adds them to the machines map.
+
+        If the machine already exists on the environment, then the new machine
+        creation is skipped.
+        """
         machines = self.deployment.get_machines()
+
         if machines:
             self.log.info("Creating machines...")
             for machine_name, spec in machines.items():
-                self.machines[machine_name] = self.env.add_machine(
-                        series=spec.get('series', 
+                if self.machine_exists(machine_name):
+                    self.log.info(
+                        """Machine %s already exists on environment, """
+                        """skipping creation""" % machine_name)
+                    self.machines[machine_name] = self.deployment.get_machine(
+                        machine_name)
+                else:
+                    self.log.info("Machine %s will be created" % machine_name)
+                    self.machines[machine_name] = self.env.add_machine(
+                        series=spec.get('series',
                                         self.deployment.data['series']),
                         constraints=spec.get('constraints'))
-                annotations = spec.get('annotations')
-                if annotations:
-                    self.env.set_annotation(
-                        self.machines[machine_name],
-                        annotations,
-                        entity_type='machine')
+
+                if isinstance(spec, dict):
+                    annotations = spec.get('annotations')
+                    if annotations:
+                        self.env.set_annotation(
+                            self.machines[machine_name],
+                            annotations,
+                            entity_type='machine')
+
             # In the case of <container type>:new, we need to create a machine
             # before creating the container on which the service will be
             # deployed.  This is stored in the machines map which will be used
@@ -97,7 +118,15 @@
             for service in self.deployment.get_services():
                 placement = self.deployment.get_unit_placement(service, None)
                 for container_host in placement.get_new_machines_for_containers():
-                    self.machines[container_host] = self.env.add_machine()
+                    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)
+                    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)
 
     def get_charms(self):

=== modified file 'deployer/deployment.py'
--- deployer/deployment.py	2015-03-18 18:19:43 +0000
+++ deployer/deployment.py	2015-05-13 19:46:10 +0000
@@ -59,6 +59,12 @@
         """
         self.machines = machines
 
+    def get_machine(self, id):
+        """Return a dict containing machine options
+        None is returned if the machine doesn't exists in
+        the bundle YAML."""
+        return self.get_machines().get(id, None)
+
     def get_machines(self):
         """Return a dict mapping machine names to machine options.
 
@@ -77,7 +83,7 @@
     @staticmethod
     def _machines_placement_sort(svc_a, svc_b):
         """Sort machines with machine placement in mind.
-        
+
         If svc_a is colocated alongside svc_b, svc_b needs to be deployed
         first, so that it exists by the time svc_a is deployed, and vice
         versa; this sorts first based on this fact, then secondly based on

=== added file 'deployer/tests/test_data/v4/container-existing-machine.yaml'
--- deployer/tests/test_data/v4/container-existing-machine.yaml	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_data/v4/container-existing-machine.yaml	2015-05-13 19:46:10 +0000
@@ -0,0 +1,42 @@
+services:
+  mediawiki:
+    charm: cs:precise/mediawiki-10
+    num_units: 1
+    options:
+      debug: false
+      name: Please set name of wiki
+      skin: vector
+    annotations:
+      gui-x: "609"
+      gui-y: "-15"
+    to:
+      - "1"
+  mysql:
+    charm: cs:precise/mysql-28
+    num_units: 1
+    options:
+      binlog-format: MIXED
+      block-size: 5
+      dataset-size: 80%
+      flavor: distro
+      ha-bindiface: eth0
+      ha-mcastport: 5411
+      max-connections: -1
+      preferred-storage-engine: InnoDB
+      query-cache-size: -1
+      query-cache-type: "OFF"
+      rbd-name: mysql1
+      tuning-level: safest
+      vip_cidr: 24
+      vip_iface: eth0
+    annotations:
+      gui-x: "610"
+      gui-y: "255"
+    to:
+      - "lxc:1"
+series: precise
+relations:
+- - mediawiki:db
+  - mysql:db
+machines:
+  1: 1

=== modified file 'deployer/tests/test_importer.py'
--- deployer/tests/test_importer.py	2015-03-24 17:38:56 +0000
+++ deployer/tests/test_importer.py	2015-05-13 19:46:10 +0000
@@ -94,3 +94,24 @@
         self.assertEqual(
             env.add_machine.call_args_list[1][1],
             {'series': 'trusty', 'constraints': 'mem=512M'})
+
+    @skip_if_offline
+    @mock.patch('deployer.action.importer.time')
+    def test_importer_existing_machine(self, mock_time):
+        self.options.configs = [
+            os.path.join(self.test_data_dir, 'v4',
+                         'container-existing-machine.yaml')]
+        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)
+
+        importer = Importer(env, deploy, self.options)
+        importer.run()
+
+        env.add_machine.assert_not_called()