Merge lp:~frankban/juju-deployer/missing-units into lp:juju-deployer

[Francesco Banconi]

Disallow deploying a bundle when a service unit placement points to a subordinate service.

Disallow deploying a bundle when a service unit placement points to a subordinate service.

Without this kind of validation, the bundle would still fail later with an unhelpful KeyError: 'units'.

The big assumption here is that we want the deployer to fail in such cases. Do we? If not, what's the correct behavior?

[Madison Scott-Clary (makyo)]

LGTM, thanks for the fix. Per IRC, I believe that the placement directive should specify the unit on which the subordinate is deployed, rather than the subordinate unit itself, since we can't guarantee the order of subordinate deployments, thus can't guarantee which machine it would wind up on.

review: Approve

[Madison Scott-Clary (makyo)]

NB: this should land before https://code.launchpad.net/~makyo/juju-deployer/machines-and-placement/+merge/251857 and these changes made in that MP.

[Brad Crittenden (bac)]

LGTM. No QA

review: Approve (code)

[David Britton (dpb)]

Thanks -- I've marked as Approved, but put the MP back to WIP, please change back to 'needs review' when you have fixed up the one test issue, and I'll land it.

Code otherwise looks good.

Thanks for doing this, much friendlier error.

review: Approve

[Francesco Banconi (frankban)]

Thank you all for the reviews!

The fetching tests should now be properly skipped as suggested by David.
I realized the get_named_deployment is already defined in the Base test case, so I removed it from DeploymentTest. I also defined a helper skip decorator in Base to avoid repetition.
I also pushed some lint drive by fixes on the modified files.
Please take another look.

=== modified file 'deployer/service.py'
--- deployer/service.py	2015-01-22 15:22:18 +0000
+++ deployer/service.py	2015-03-17 10:41:55 +0000
@@ -106,6 +106,10 @@
                     feedback.error(
                         "Nested placement not supported %s -> %s -> %s" % (
                             self.service.name, p, services[p].unit_placement))
+                elif self.deployment.get_charm_for(p).is_subordinate():
+                    feedback.error(
+                        "Cannot place to a subordinate service: %s -> %s" % (
+                            self.service.name, p))
             else:
                 feedback.error(
                     "Invalid service placement %s to %s" % (

=== modified file 'deployer/tests/base.py'
--- deployer/tests/base.py	2014-01-23 12:55:11 +0000
+++ deployer/tests/base.py	2015-03-17 10:41:55 +0000
@@ -11,6 +11,8 @@
 
 
 TEST_OFFLINE = ("DEB_BUILD_ARCH" in os.environ or "TEST_OFFLINE" in os.environ)
+skip_if_offline = unittest.skipIf(
+    TEST_OFFLINE, 'Requires configured bzr launchpad id and network access')
 
 
 class Base(unittest.TestCase):

=== added file 'deployer/tests/test_data/stack-placement-invalid-subordinate.yaml'
--- deployer/tests/test_data/stack-placement-invalid-subordinate.yaml	1970-01-01 00:00:00 +0000
+++ deployer/tests/test_data/stack-placement-invalid-subordinate.yaml	2015-03-17 10:41:55 +0000
@@ -0,0 +1,11 @@
+stack:
+  series: precise
+  services:
+    nova-compute:
+      charm: cs:precise/nova-compute
+      to: nrpe
+    ceph:
+      to: lxc:nrpe=0
+    nrpe:
+      charm: cs:precise/nrpe
+      units: 2

=== modified file 'deployer/tests/test_deployment.py'
--- deployer/tests/test_deployment.py	2015-01-22 15:22:18 +0000
+++ deployer/tests/test_deployment.py	2015-03-17 10:41:55 +0000
@@ -1,14 +1,11 @@
 import base64
 import StringIO
 import os
-import unittest
-
-
-from deployer.config import ConfigStack
+
 from deployer.deployment import Deployment
 from deployer.utils import setup_logging, ErrorExit
 
-from .base import Base, TEST_OFFLINE
+from .base import Base, skip_if_offline
 
 
 class DeploymentTest(Base):
@@ -17,17 +14,19 @@
         self.output = setup_logging(
             debug=True, verbose=True, stream=StringIO.StringIO())
 
-    def get_named_deployment(self, file_name, stack_name):
-        return ConfigStack(
-            [os.path.join(
-                self.test_data_dir, file_name)]).get(stack_name)
+    def get_named_deployment_and_fetch(self, file_name, stack_name):
+        deployment = self.get_named_deployment(file_name, stack_name)
+        # Fetch charms in order to allow proper late binding config and
+        # placement validation.
+        repo_path = self.mkdir()
+        os.mkdir(os.path.join(repo_path, "precise"))
+        deployment.repo_path = repo_path
+        deployment.fetch_charms()
+        return deployment
 
-    @unittest.skipIf(TEST_OFFLINE,
-                     "Requires configured bzr launchpad id and network access")
+    @skip_if_offline
     def test_deployer(self):
-        d = ConfigStack(
-            [os.path.join(
-                self.test_data_dir, "blog.yaml")]).get('wordpress-prod')
+        d = self.get_named_deployment_and_fetch('blog.yaml', 'wordpress-prod')
         services = d.get_services()
         self.assertTrue([s for s in services if s.name == "newrelic"])
 
@@ -36,12 +35,6 @@
             d.data['inherits'],
             ['wordpress-stage', 'wordpress-base', 'metrics-base'])
 
-        # Fetch charms to verify late binding config values & validation.
-        t = self.mkdir()
-        os.mkdir(os.path.join(t, "precise"))
-        d.repo_path = t
-        d.fetch_charms()
-
         # Load up overrides and resolves
         d.load_overrides(["key=abc"])
         d.resolve()
@@ -99,6 +92,20 @@
         else:
             self.fail("Should fail")
 
+    @skip_if_offline
+    def test_validate_invalid_placement_subordinate(self):
+        # Placement validation fails if a subordinate charm is provided.
+        deployment = self.get_named_deployment_and_fetch(
+            'stack-placement-invalid-subordinate.yaml', 'stack')
+        with self.assertRaises(ErrorExit):
+            deployment.validate_placement()
+        output = self.output.getvalue()
+        self.assertIn(
+            'Cannot place to a subordinate service: ceph -> nrpe\n', output)
+        self.assertIn(
+            'Cannot place to a subordinate service: nova-compute -> nrpe\n',
+            output)
+
     def test_get_unit_placement(self):
         d = self.get_named_deployment("stack-placement.yaml", "stack")
         status = {

=== modified file 'deployer/tests/test_diff.py'
--- deployer/tests/test_diff.py	2014-01-23 12:40:57 +0000
+++ deployer/tests/test_diff.py	2015-03-17 10:41:55 +0000
@@ -2,20 +2,18 @@
 # pylint: disable=C0103
 import StringIO
 import os
-import unittest
 import shutil
 import tempfile
+
+from deployer.config import ConfigStack
 from deployer.env.mem import MemoryEnvironment
-from deployer.config import ConfigStack
 from deployer.utils import setup_logging
 
-from .base import Base, TEST_OFFLINE
+from .base import Base, skip_if_offline
 from ..action.diff import Diff
 
 
-# pylint: disable=C0111, R0904
[e-mail](TEST_OFFLINE,
-                 "Requires configured bzr launchpad id and network access")
+@skip_if_offline
 class DiffTest(Base):
 
     def setUp(self):

=== modified file 'deployer/tests/test_guiserver.py'
--- deployer/tests/test_guiserver.py	2015-02-03 12:06:59 +0000
+++ deployer/tests/test_guiserver.py	2015-03-17 10:41:55 +0000
@@ -11,7 +11,7 @@
 
 from deployer import guiserver
 from deployer.feedback import Feedback
-from deployer.tests.base import TEST_OFFLINE
+from deployer.tests.base import skip_if_offline
 
 
 class TestGetDefaultGuiserverOptions(unittest.TestCase):
@@ -26,9 +26,9 @@
             'bootstrap', 'branch_only', 'configs', 'debug', 'deploy_delay',
             'deployment', 'description', 'destroy_services', 'diff',
             'find_service', 'ignore_errors', 'juju_env', 'list_deploys',
-            'no_local_mods', 'no_relations', 'overrides', 'rel_wait', 'retry_count', 'series',
-            'skip_unit_wait', 'terminate_machines', 'timeout', 'update_charms',
-            'verbose', 'watch'
+            'no_local_mods', 'no_relations', 'overrides', 'rel_wait',
+            'retry_count', 'series', 'skip_unit_wait', 'terminate_machines',
+            'timeout', 'update_charms', 'verbose', 'watch'
         ])
         self.assertEqual(expected_keys, set(self.options.__dict__.keys()))
 
@@ -172,8 +172,7 @@
         mock_env_instance.close.assert_called_once_with()
 
 
[e-mail](TEST_OFFLINE,
-                 "Requires configured bzr launchpad id and network access")
+@skip_if_offline
 @mock.patch('deployer.guiserver.GUIEnvironment')
 class TestValidate(DeployerFunctionsTestMixin, unittest.TestCase):
 
@@ -192,8 +191,7 @@
                 self.apiurl, self.username, self.password, self.bundle)
 
 
[e-mail](TEST_OFFLINE,
-                 "Requires configured bzr launchpad id and network access")
+@skip_if_offline
 @mock.patch('deployer.guiserver.GUIEnvironment')
 class TestImportBundle(DeployerFunctionsTestMixin, unittest.TestCase):
 

=== modified file 'deployer/tests/test_importer.py'
--- deployer/tests/test_importer.py	2014-10-01 10:18:36 +0000
+++ deployer/tests/test_importer.py	2015-03-17 10:41:55 +0000
@@ -1,14 +1,12 @@
-import logging
+import os
+
 import mock
-import os
-import sys
-import unittest
 
 from deployer.config import ConfigStack
 from deployer.action.importer import Importer
 from deployer.utils import yaml_dump, yaml_load
 
-from base import Base, TEST_OFFLINE
+from base import Base, skip_if_offline
 
 
 class Options(dict):
@@ -46,8 +44,7 @@
             'verbose': True,
             'watch': False})
 
-    @unittest.skipIf(TEST_OFFLINE,
-                     "Requires configured bzr launchpad id and network access")
+    @skip_if_offline
     @mock.patch('deployer.action.importer.time')
     def test_importer(self, mock_time):
         # Trying to track down where this comes from http://pad.lv/1243827
@@ -67,8 +64,7 @@
             yaml_load(yaml_dump(yaml_load(yaml_dump(config)))),
             config)
 
-    @unittest.skipIf(TEST_OFFLINE,
-                     "Requires configured bzr launchpad id and network access")
+    @skip_if_offline
     @mock.patch('deployer.action.importer.time')
     def test_importer_no_relations(self, mock_time):
         self.options.no_relations = True