Merge lp:~stub/charms/trusty/cassandra/blamejuju into lp:charms/trusty/cassandra

[Stuart Bishop]

A provisioning failure rate of 1% means a complex test suite provisioning 50 units will fail half the time. This makes charms that have comprehensive tests look bad when compared to charms with minimal tests that are less likely to hit provisioning issues.

This branch attempts to make things fairer on the charm, skipping tests when a timeout has occurred and we detect that provisioning has failed. False negatives, where tests fail for reasons beyond our control, are a cause of major delays in landing bug fixes and improvements. Instead, I'm chosing potential false positives where a test that should fail is skipped because the cloud happened to be misbehaving. Care will need to be taken when landing, to ensure a run has completed without skipped tests in the CI system. This should keep the tests green, except when the charm really is at fault, and provide a fairer guide to charm quality (although less useful for judging cloud quality).

This test fixture is shared with the PostgreSQL charm and hopefully will one day become a layer to help develop comprehensive test suites.

The output can still be rather noisy when provisioning issues hit during deployment, as amulet has installed a noisy signal handler I can't see how to override. I think tidying this up will first require amulet work.

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/2276/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/2255/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-lxc/2301/

review: Needs Fixing (automated testing)

[Review Queue (review-queue)]

This item has failed automated testing! Results available here http://juju-ci.vapour.ws:8080/job/charm-bundle-test-aws/2280/

review: Needs Fixing (automated testing)

[Adam Israel (aisrael)]

Hey stub,

I've had a chance to review these test changes today. The changes look fine to me, and the tests succeed against AWS.

Two pieces of feedback:
- The tests take a really long time. Like, 10 hours long. Is that consistent with your testing, and if so, do you expect this to always be the case?
- The tests as they are, specifically amuletfixture.py but likely other spots as well, aren't juju 2.0 compatible. We're still running 1.25 in CI, but that will be upgraded to 2.0 upon release.

This has my +1, and I'll merge it once I've finished reviewing your other pending Cassandra changes.

review: Approve

Unmerged revisions

371. By Stuart Bishop on 2016-01-20

Skip tests and blame others on provisioning failures, rather than victimize the charm

370. By Stuart Bishop on 2016-01-20

Sync updates to amuletfixture.py from the PostgreSQL charm

=== modified file 'testing/amuletfixture.py'
--- testing/amuletfixture.py	2015-09-25 07:33:01 +0000
+++ testing/amuletfixture.py	2016-01-20 15:19:10 +0000
@@ -14,43 +14,45 @@
 # You should have received a copy of the GNU General Public License
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
-from functools import wraps
 import json
 import os
 import shutil
 import subprocess
 import tempfile
 import time
+import unittest
 
 import amulet
+import amulet.helpers
+import amulet.sentry
 import yaml
 
 
 class AmuletFixture(amulet.Deployment):
-    def __init__(self, series, verbose=False):
-        if verbose:
-            super(AmuletFixture, self).__init__(series=series)
-        else:
-            # We use a wrapper around juju-deployer so we can fix how it is
-            # invoked. In particular, turn off all the noise so we can
-            # actually read our test output.
-            juju_deployer = os.path.abspath(os.path.join(
-                os.path.dirname(__file__), os.pardir, 'lib',
-                'juju-deployer-wrapper.py'))
-            super(AmuletFixture, self).__init__(series=series,
-                                                juju_deployer=juju_deployer)
-        assert self.series == series
+    def __init__(self, series, charm_dir=None):
+        self.charm_dir = charm_dir  # If None, reset by repackage_charm()
+        # We use a wrapper around juju-deployer so we can adjust how it is
+        # invoked. In particular, only make noise on failure.
+        juju_deployer = os.path.abspath(os.path.join(
+            os.path.dirname(__file__), os.pardir, 'lib',
+            'juju-deployer-wrapper.py'))
+        super(AmuletFixture, self).__init__(series=series,
+                                            juju_deployer=juju_deployer)
 
-    def setUp(self):
+    def setUp(self, keep=None):
         self._temp_dirs = []
 
-        self.reset_environment(force=True)
+        if keep:
+            self.reset_environment(keep=keep)
+        else:
+            self.reset_environment(force=True)
 
         # Repackage our charm to a temporary directory, allowing us
         # to strip our virtualenv symlinks that would otherwise cause
         # juju to abort. We also strip the .bzr directory, working
         # around Bug #1394078.
-        self.repackage_charm()
+        if self.charm_dir is None:
+            self.repackage_charm()
 
         # Fix amulet.Deployment so it doesn't depend on environment
         # variables or the current working directory, but rather the
@@ -69,31 +71,35 @@
         os.environ['JUJU_REPOSITORY'] = temp_repo
         os.makedirs(os.path.join(temp_repo, self.series), mode=0o700)
 
-    def tearDown(self, reset_environment=True):
+    def tearDown(self, reset_environment=True, keep=None):
         if reset_environment:
-            self.reset_environment()
+            self.reset_environment(keep=keep)
         if self.org_repo is None:
             del os.environ['JUJU_REPOSITORY']
         else:
             os.environ['JUJU_REPOSITORY'] = self.org_repo
 
-    def deploy(self, timeout=None):
+    def deploy(self, timeout=None, keep=None):
         '''Deploying or updating the configured system.
 
         Invokes amulet.Deployer.setup with a nicer name and standard
         timeout handling.
         '''
+        # First, ensure any existing environment is completely
+        # torn down. juju-deployer seems to forget to deploy
+        # services if there is an existing service in the environment
+        # in the process of being destroyed.
+        self.reset_environment(keep=keep)
         if timeout is None:
             timeout = int(os.environ.get('AMULET_TIMEOUT', 900))
 
-        # juju-deployer is buried under here, and has race conditions.
-        # Sleep a bit before invoking it, so its cached view of the
-        # environment matches reality.
-        time.sleep(15)
-
         # If setUp fails, tearDown is never called leaving the
         # environment setup. This is useful for debugging.
-        self.setup(timeout=timeout)
+        try:
+            self.setup(timeout=timeout)
+        except amulet.helpers.TimeoutError:
+            self._blame_timeout()
+            raise
         self.wait(timeout=timeout)
 
     def __del__(self):
@@ -101,6 +107,29 @@
             if os.path.exists(temp_dir):
                 shutil.rmtree(temp_dir, ignore_errors=True)
 
+    def add_unit(self, service, units=1, target=None, timeout=None):
+        # Work around Bug #1510000
+        if not isinstance(units, int) or units < 1:
+            raise ValueError('Only positive integers can be used for units')
+        if target is not None and units != 1:
+            raise ValueError(
+                "Can't deploy more than one unit when specifying a target.")
+        if service not in self.services:
+            raise ValueError('Service needs to be added before you can scale')
+
+        self.services[service]['num_units'] = \
+            self.services[service].get('num_units', 1) + units
+
+        if self.deployed:
+            args = ['add-unit', service, '-n', str(units)]
+            if target is not None:
+                args.extend(["--to", target])
+            amulet.helpers.juju(args)
+            if timeout is None:
+                timeout = int(os.environ.get('AMULET_TIMEOUT', 900))
+            self.sentry = amulet.sentry.Talisman(self.services,
+                                                 timeout=timeout)
+
     def get_status(self):
         try:
             raw = subprocess.check_output(['juju', 'status', '--format=json'],
@@ -114,16 +143,96 @@
 
     def wait(self, timeout=None):
         '''Wait until the environment has reached a stable state.'''
+        cmd = ['juju', 'wait', '-q']
         if timeout is None:
             timeout = int(os.environ.get('AMULET_TIMEOUT', 900))
-        cmd = ['timeout', str(timeout), 'juju', 'wait', '-q']
+        cmd = ['timeout', str(timeout)] + cmd
         try:
             subprocess.check_output(cmd, universal_newlines=True)
         except subprocess.CalledProcessError as x:
+            if x.returncode == 124:
+                self._blame_timeout()  # Maybe raises an exception.
             print(x.output)
             raise
 
-    def reset_environment(self, force=False):
+    # Override if you want a fatal exception.
+    blame_exception = unittest.SkipTest
+
+    def blame(self, headline, msg):
+        '''
+        If the failure is not our fault, skip the test. This will
+        lead to false positive test runs, but seems better than
+        bogging down reviews because a cloud 1% failure rate
+        translates to a 50% test suite failure rate. Once we stop
+        expecting juju and providers to fail, we can stop this
+        and go back to reasonable behaviour.
+
+        Override if you want a fatal exception.
+        '''
+        raise unittest.SkipTest('{} {}'.format(headline.upper(), msg))
+
+    def _blame_timeout(self):
+        '''A timeout happened. Investigate and allocate blame.
+
+        Raises an self.blame_exception if failure determined to be Juju
+        or provider fault.
+
+        Does nothing if blame cannot be made.
+        '''
+        status = self.get_status()
+
+        provisioning = "Provisioning Failure"
+        if len(status.get('machines', {})) <= 1:
+            self.blame(provisioning,
+                       'No machines provisioned')
+        for machine, info in status.get('machines', {}).items():
+            agent_state = info.get('agent-state')
+            if agent_state != 'started':
+                self.blame(provisioning,
+                           'Machine {} in {} state'.format(machine,
+                                                           agent_state))
+            if not info.get('dns-name'):
+                self.blame(provisioning,
+                           'Machine {} has no IP address'.format(machine))
+
+            if not info.get('instance-id'):
+                self.blame(provisioning,
+                           'Machine {} has no instance-id'.format(machine))
+
+            if not info.get('agent-version'):
+                self.blame(provisioning,
+                           'Machine {} has no juju agent'.format(machine))
+
+        # TODO: We don't check subordinates yet. Do these ever fail if
+        # the primary is provisioned fine?
+        for service, service_info in status.get('services', {}).items():
+            leader_found = False
+            for unit, unit_info in service_info.get('units', {}).items():
+                if not unit_info.get('machine'):
+                    self.blame(provisioning,
+                               'Unit {} has not been allocated a machine'
+                               ''.format(unit))
+                if not unit_info.get('public-address'):
+                    self.blame(provisioning,
+                               'Unit {} has no IP address'.format(unit))
+                if not unit_info.get('agent-version'):
+                    self.blame(provisioning,
+                               'Unit {} has no agent'.format(unit))
+                agent_state = unit_info.get('agent-state')
+                if agent_state != 'started':
+                    self.blame(provisioning,
+                               'Unit {} has agent state {}'
+                               ''.format(agent_state))
+                cmd = ['run', '--unit', unit, 'is-leader']
+                if amulet.helpers.juju(cmd).strip() == 'True':
+                    leader_found = True
+            if not leader_found:
+                self.blame("Juju Leadership Failure",
+                           "Service {} has no leader".format(service))
+
+    def reset_environment(self, force=False, keep=None):
+        if keep is None:
+            keep = frozenset()
         if force:
             status = self.get_status()
             machines = [m for m in status.get('machines', {}).keys()
@@ -133,50 +242,47 @@
                                  '--force'] + machines,
                                 stdout=subprocess.DEVNULL,
                                 stderr=subprocess.DEVNULL)
+
         fails = dict()
+        keep_machines = set(["0"])
         while True:
             status = self.get_status()
             service_items = status.get('services', {}).items()
-            if not service_items:
-                break
+
             for service_name, service in service_items:
+                if service_name in keep:
+                    # Don't mess with this service.
+                    keep_machines.update([unit['machine'] for unit
+                                          in service['units'].values()])
+                    continue
+
                 if service.get('life', '') not in ('dying', 'dead'):
                     subprocess.call(['juju', 'destroy-service', service_name],
                                     stdout=subprocess.PIPE,
                                     stderr=subprocess.STDOUT)
+
                 for unit_name, unit in service.get('units', {}).items():
                     if unit.get('agent-state', None) == 'error':
-                        if force:
-                            # If any units have failed hooks, unstick them.
-                            # This should no longer happen now we are
-                            # using the 'destroy-machine --force' command
-                            # earlier.
-                            try:
-                                subprocess.check_output(
-                                    ['juju', 'resolved', unit_name],
-                                    stderr=subprocess.STDOUT)
-                            except subprocess.CalledProcessError:
-                                # A previous 'resolved' call make cause a
-                                # subsequent one to fail if it is still
-                                # being processed. However, we need to keep
-                                # retrying because after a successful
-                                # resolution a subsequent hook may cause an
-                                # error state.
-                                pass
-                        else:
-                            fails[unit_name] = unit
+                        fails[unit_name] = unit
+
+            services = set(k for k, v in service_items
+                           if k not in keep)
+            if not services:
+                break
+
             time.sleep(1)
 
         harvest_machines = []
         for machine, state in status.get('machines', {}).items():
-            if machine != "0" and state.get('life') not in ('dying', 'dead'):
+            if machine not in keep_machines and (state.get('life')
+                                                 not in ('dying', 'dead')):
                 harvest_machines.append(machine)
 
         if harvest_machines:
             cmd = ['juju', 'remove-machine', '--force'] + harvest_machines
             subprocess.check_output(cmd, stderr=subprocess.STDOUT)
 
-        if fails:
+        if fails and not force:
             raise Exception("Teardown failed", fails)
 
     def repackage_charm(self):
@@ -206,10 +312,6 @@
 
         repack_root = tempfile.mkdtemp(suffix='.charm')
         self._temp_dirs.append(repack_root)
-        # juju-deployer now requires the series in the path when
-        # deploying from an absolute path.
-        repack_root = os.path.join(repack_root, self.series)
-        os.makedirs(repack_root, mode=0o700)
 
         self.charm_dir = os.path.join(repack_root, self.charm_name)
 
@@ -219,16 +321,3 @@
         # infinite recursion.
         shutil.copytree(src_charm_dir, self.charm_dir, symlinks=True,
                         ignore=shutil.ignore_patterns('.venv?', '.bzr'))
-
-
-# Bug #1417097 means we need to monkey patch Amulet for now.
-real_juju = amulet.helpers.juju
-
-
-@wraps(real_juju)
-def patched_juju(args, env=None):
-    args = [str(a) for a in args]
-    return real_juju(args, env)
-
-amulet.helpers.juju = patched_juju
-amulet.deployer.juju = patched_juju