Merge lp:~abentley/juju-ci-tools/industrial-test into lp:juju-ci-tools

[Aaron Bentley]

Initial version of industrial testing.

This branch provides a minimal implementation of IndustrialTest.

The industrial_test.py script runs a bootstrap and destroy-environment using both the system juju and a specified juju. It accepts an existing environment name and appends -old and -new to it.

IndustrialTest provides run_attempt which returns an iterator of (boolean, boolean) to indicate whether a stage was run successfully by each client. Iteration stops when either client fails, and the environment is cleaned up. The last stage is assumed to be DestroyEnvironment, so if all stages succeed, no futher cleanup is performed.

Success and failure are implementation defined, but StageAttempt provides helpers. If a StageAttempt subclass provides _operation and _result, StageAttemp will treat exceptions from _operation as a failure of that client, and exceptions from _result will be translated into failure as well.

BootstrapAttempt uses the relatively-new temp_bootstrap_env contextmanager to bootstrap into its environment.

DestroyEnvironment is only considered to fail if the destroy-environment command has a non-zero exit status.

For local testing, uniquify_local ensures that the ports do not conflict between the two environments.

Future updates will flesh out the result statistic handling, support multiple runs, and add new kinds of stages.

[Curtis Hovey (sinzui)]

Thank you Aaron. I have questions about a try-finally block.

review: Needs Information (code)

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 14-10-17 09:47 AM, Curtis Hovey wrote:
>> + def run_stages(self): + for attempt in
>> self.stage_attempts: + result =
>> attempt.do_stage(self.old_client, self.new_client) + yield result
>> + if False in result: + try: +
>> self.old_client.destroy_environment() + finally:
>
> Sorry. I don't understand this. If one stage fails, we destroy
> both?, not the one we know that failed?

Yes. We want a side-by-side comparison of old_client vs new_client,
to minimize the chance that substrate-side errors are counted as
problems with one client, but not the other.

If we are doing a side-by-side comparison, a failure in either (or
both) clients means we must stop both clients, because it ceases to be
a side-by-side comparison if one client cannot continue.

I haven't implemented the part that re-runs an industrial test until
every stage has been attempted sufficient times, but when I do, a
failure in either client will cause us to re-run the industrial test,
in order to test the stages following the stage that failed. So even
if we weren't doing side-by-side comparisons, there is no advantage in
continuing to run one client after the other fails.

The exception to the rule that failures require a re-run is, of
course, if the failure occurs in the final stage.

> Doesn't this code swallow an error if the old_client errors?

It's a finally block, so it doesn't swallow exceptions by default.
But if new_client.destroy_environment also raises, it will swallow the
old_client.destroy_environment exception.

While the exception for old_client can be suppressed, the juju error
messages for each "juju destroy-environment" invocation will not be
suppressed, and the exception from destroy_environment is not very
interesting (it just tells you which non-zero exit status was used).

>> + def test_destroy_both_even_with_exception(self): +
>> old_client = FakeEnvJujuClient('old') + new_client =
>> FakeEnvJujuClient('new') + industrial =
>> IndustrialTest(old_client, new_client, + [FakeAttempt(False,
>> False), FakeAttempt(True, True)]) + attempt =
>> industrial.run_stages() + with patch.object(old_client,
>> 'destroy_environment', + side_effect=Exception) as oc_mock: +
>> with patch.object(new_client, 'destroy_environment', +
>> side_effect=Exception) as nc_mock: + with
>> self.assertRaises(Exception): + list(attempt)
>
> Sorry, I don't know which exception is raised? Do we expect both?

We expect the new_client exception. I don't know of a way of raising
two exceptions simultaneously, so I don't know how we could get both.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJUQSdHAAoJEK84cMOcf+9hyKAH/2ESEERPpZQeR42SzJUXVWUY
sZw7nDB4Eoe94QG2UgC8YyoZKLpCwy/Ga2IOFKXptlwQdZXaKjztL2Jc36JsTXk1
Bk4PW4QWr/BHxRx1pNroQw/+YBXasTijQPhJO5eE5MgGqtkt5Rg+mIYDhM40d98t
wu4qrNjg7mR42Nno9j8SC7JakXTJzytQCKHG98349Sq/M1Cer0ixuAZM8LV2qY2d
7ZpjnnF4IumJ5EO3ygjwZB3sAO4SV2WU7YslBDPX7qZr3aNuqdYoN4uhvaXmwuyB
/uKjNN+u/lXyHq2Ousow9SEXVe7A2qzHHVl9KRvFHpRwI8U8Np0rhjkqDlbjPb0=
=+BZl
-----END PGP SIGNATURE-----

[Curtis Hovey (sinzui)]

Is this summarise the try-finally issue correct? As we know know there is a chance that the old-client error can be swallowed by a new client error. When investigate an exception destroying the new-client we need to be mindful of an exception from old-client is missing. Maybe we can address this issue in the future by adding an additional logging step after each destroy to indicate that the old-client did destroy itself.

review: Needs Information (code)

[Aaron Bentley (abentley)]

Your summary is correct, but I misled you about the nature of exceptions destroy_environment is likely to raise.

I had forgotten that destroy_environment does not raise an exception when juju exits with failure. (It uses "call" rather "check_call", because juju returns a non-zero exit status if you try to destroy an environment that does not exist.)

So the sorts of exceptions you're likely to suppress are:
- string formatting issues when the 'destroy-environment' command cannot be constructed due to bogus values
- out-of-memory running fork()
- no-such-file running exec()

These are pretty rare and weird errors, and you'll only suppress them if new_client.destroy_environment() *also* raises a rare and weird error.

However, it's not hard to turn the try/finally into a try/except/finally where the except block logs the exception. Actually, it can be a try/except then, because after you've logged the exception, you shouldn't re-raise it.

[Curtis Hovey (sinzui)]

Thank you. This was very informative. Let's merge this code. If we have issues about destroy-env in the future, we can solve this issue.

review: Approve (code)

=== added file 'industrial_test.py'
--- industrial_test.py	1970-01-01 00:00:00 +0000
+++ industrial_test.py	2014-10-16 20:18:49 +0000
@@ -0,0 +1,128 @@
+#!/usr/bin/env python
+__metaclass__ = type
+
+from argparse import ArgumentParser
+import logging
+
+from jujuconfig import get_juju_home
+from jujupy import (
+    EnvJujuClient,
+    SimpleEnvironment,
+    temp_bootstrap_env,
+    )
+
+
+def uniquify_local(env):
+    if not env.local:
+        return
+    port_defaults = {
+        'api-port': 17070,
+        'state-port': 37017,
+        'storage-port': 8040,
+        'syslog-port': 6514,
+    }
+    for key, default in port_defaults.items():
+        env.config[key] = env.config.get(key, default) + 1
+
+
+class IndustrialTest:
+
+    @classmethod
+    def from_args(cls, env, new_juju_path, stage_attempts):
+        old_env = SimpleEnvironment.from_config(env)
+        old_env.environment = env + '-old'
+        old_client = EnvJujuClient.by_version(old_env)
+        new_env = SimpleEnvironment.from_config(env)
+        new_env.environment = env + '-new'
+        uniquify_local(new_env)
+        new_client = EnvJujuClient.by_version(new_env, new_juju_path)
+        return cls(old_client, new_client, stage_attempts)
+
+    def __init__(self, old_client, new_client, stage_attempts):
+        self.old_client = old_client
+        self.new_client = new_client
+        self.stage_attempts = stage_attempts
+
+    def run_attempt(self):
+        self.old_client.destroy_environment()
+        self.new_client.destroy_environment()
+        return self.run_stages()
+
+    def run_stages(self):
+        for attempt in self.stage_attempts:
+            result = attempt.do_stage(self.old_client, self.new_client)
+            yield result
+            if False in result:
+                try:
+                    self.old_client.destroy_environment()
+                finally:
+                    self.new_client.destroy_environment()
+                break
+
+
+class StageAttempt:
+
+    def __init__(self):
+        self.failure_clients = set()
+
+    def do_stage(self, old, new):
+        self.do_operation(old)
+        self.do_operation(new)
+        old_result = self.get_result(old)
+        new_result = self.get_result(new)
+        return old_result, new_result
+
+    def do_operation(self, client, output=None):
+        try:
+            self._operation(client)
+        except Exception as e:
+            logging.exception(e)
+            self.failure_clients.add(client)
+
+    def get_result(self, client):
+        if client in self.failure_clients:
+            return False
+        try:
+            return self._result(client)
+        except Exception:
+            return False
+
+
+class BootstrapAttempt(StageAttempt):
+
+    def _operation(self, client):
+        with temp_bootstrap_env(get_juju_home(), client):
+            client.bootstrap()
+
+    def _result(self, client):
+        client.wait_for_started()
+        return True
+
+
+class DestroyEnvironmentAttempt(StageAttempt):
+
+    def _operation(self, client):
+        client.juju('destroy-environment', ('-y', client.env.environment),
+                    include_e=False)
+
+    def _result(self, client):
+        return True
+
+
+def parse_args(args=None):
+    parser = ArgumentParser()
+    parser.add_argument('env')
+    parser.add_argument('new_juju_path')
+    return parser.parse_args(args)
+
+
+def main():
+    args = parse_args()
+    stages = [BootstrapAttempt(), DestroyEnvironmentAttempt()]
+    industrial = IndustrialTest.from_args(args.env, args.new_juju_path, stages)
+    results = list(industrial.run_attempt())
+    print results
+
+
+if __name__ == '__main__':
+    main()
 
=== modified file 'jujupy.py'
--- jujupy.py	2014-09-29 18:00:46 +0000
+++ jujupy.py	2014-10-16 20:18:49 +0000
@@ -424,6 +424,24 @@
             self.hpcloud = False
             self.maas = False
 
+    def __eq__(self, other):
+        if type(self) != type(other):
+            return False
+        if self.environment != other.environment:
+            return False
+        if self.config != other.config:
+            return False
+        if self.local != other.local:
+            return False
+        if self.hpcloud != other.hpcloud:
+            return False
+        if self.maas != other.maas:
+            return False
+        return True
+
+    def __ne__(self, other):
+        return not self == other
+
     @classmethod
     def from_config(cls, name):
         return cls(name, get_selected_environment(name)[0])
 
=== added file 'test_industrial_test.py'
--- test_industrial_test.py	1970-01-01 00:00:00 +0000
+++ test_industrial_test.py	2014-10-16 20:18:49 +0000
@@ -0,0 +1,265 @@
+__metaclass__ = type
+
+from contextlib import contextmanager
+import os.path
+from StringIO import StringIO
+from unittest import TestCase
+
+from mock import patch
+import yaml
+
+from industrial_test import (
+    BootstrapAttempt,
+    DestroyEnvironmentAttempt,
+    IndustrialTest,
+    parse_args,
+    StageAttempt,
+    )
+from jujupy import (
+    EnvJujuClient,
+    SimpleEnvironment,
+    )
+
+
+@contextmanager
+def parse_error(test_case):
+    stderr = StringIO()
+    with test_case.assertRaises(SystemExit):
+        with patch('sys.stderr', stderr):
+            yield stderr
+
+
+class TestParseArgs(TestCase):
+
+    def test_parse_args(self):
+        with parse_error(self) as stderr:
+            args = parse_args([])
+        self.assertRegexpMatches(
+            stderr.getvalue(), '.*error: too few arguments.*')
+        with parse_error(self) as stderr:
+            args = parse_args(['env'])
+        self.assertRegexpMatches(
+            stderr.getvalue(), '.*error: too few arguments.*')
+        args = parse_args(['rai', 'new-juju'])
+        self.assertEqual(args.env, 'rai')
+        self.assertEqual(args.new_juju_path, 'new-juju')
+
+
+class FakeAttempt:
+
+    def __init__(self, *result):
+        self.result = result
+
+    def do_stage(self, old_client, new_client):
+        return self.result
+
+
+class TestIndustrialTest(TestCase):
+
+    def test_init(self):
+        old_client = object()
+        new_client = object()
+        attempt_list = []
+        industrial = IndustrialTest(old_client, new_client, attempt_list)
+        self.assertIs(old_client, industrial.old_client)
+        self.assertIs(new_client, industrial.new_client)
+        self.assertIs(attempt_list, industrial.stage_attempts)
+
+    def test_from_args(self):
+        side_effect = lambda x, y=None: (x, y)
+        with patch('jujupy.EnvJujuClient.by_version', side_effect=side_effect):
+            with patch('jujupy.SimpleEnvironment.from_config',
+                       side_effect=lambda x: SimpleEnvironment(x, {})):
+                industrial = IndustrialTest.from_args(
+                    'foo', 'new-juju-path', [])
+        self.assertIsInstance(industrial, IndustrialTest)
+        self.assertEqual(industrial.old_client,
+                         (SimpleEnvironment('foo-old', {}), None))
+        self.assertEqual(industrial.new_client,
+                         (SimpleEnvironment('foo-new', {}), 'new-juju-path'))
+        self.assertNotEqual(industrial.old_client[0].environment,
+                            industrial.new_client[0].environment)
+
+    def test_run_stages(self):
+        old_client = FakeEnvJujuClient('old')
+        new_client = FakeEnvJujuClient('new')
+        industrial = IndustrialTest(old_client, new_client,
+                       [FakeAttempt(True, True), FakeAttempt(True, True)])
+        with patch('subprocess.call') as cc_mock:
+            result = industrial.run_stages()
+            self.assertItemsEqual(result, [(True, True), (True, True)])
+        self.assertEqual(len(cc_mock.mock_calls), 0)
+
+    def test_run_stages_old_fail(self):
+        old_client = FakeEnvJujuClient('old')
+        new_client = FakeEnvJujuClient('new')
+        industrial = IndustrialTest(old_client, new_client,
+                       [FakeAttempt(False, True), FakeAttempt(True, True)])
+        with patch('subprocess.call') as cc_mock:
+            result = industrial.run_stages()
+            self.assertItemsEqual(result, [(False, True)])
+        assert_juju_call(self, cc_mock, old_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'old', '--force', '-y'), 0)
+        assert_juju_call(self, cc_mock, new_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'new', '--force', '-y'), 1)
+
+    def test_run_stages_new_fail(self):
+        old_client = FakeEnvJujuClient('old')
+        new_client = FakeEnvJujuClient('new')
+        industrial = IndustrialTest(old_client, new_client,
+                       [FakeAttempt(True, False), FakeAttempt(True, True)])
+        with patch('subprocess.call') as cc_mock:
+            result = industrial.run_stages()
+            self.assertItemsEqual(result, [(True, False)])
+        assert_juju_call(self, cc_mock, old_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'old', '--force', '-y'), 0)
+        assert_juju_call(self, cc_mock, new_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'new', '--force', '-y'), 1)
+
+    def test_run_stages_both_fail(self):
+        old_client = FakeEnvJujuClient('old')
+        new_client = FakeEnvJujuClient('new')
+        industrial = IndustrialTest(old_client, new_client,
+                       [FakeAttempt(False, False), FakeAttempt(True, True)])
+        with patch('subprocess.call') as cc_mock:
+            result = industrial.run_stages()
+            self.assertItemsEqual(result, [(False, False)])
+        assert_juju_call(self, cc_mock, old_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'old', '--force', '-y'), 0)
+        assert_juju_call(self, cc_mock, new_client,
+                         ('juju', '--show-log', 'destroy-environment',
+                          'new', '--force', '-y'), 1)
+
+    def test_destroy_both_even_with_exception(self):
+        old_client = FakeEnvJujuClient('old')
+        new_client = FakeEnvJujuClient('new')
+        industrial = IndustrialTest(old_client, new_client,
+                       [FakeAttempt(False, False), FakeAttempt(True, True)])
+        attempt = industrial.run_stages()
+        with patch.object(old_client, 'destroy_environment',
+                          side_effect=Exception) as oc_mock:
+            with patch.object(new_client, 'destroy_environment',
+                              side_effect=Exception) as nc_mock:
+                with self.assertRaises(Exception):
+                    list(attempt)
+        oc_mock.assert_called_once_with()
+        nc_mock.assert_called_once_with()
+
+
+class TestStageAttempt(TestCase):
+
+    def test_do_stage(self):
+
+        class StubSA(StageAttempt):
+
+            def __init__(self):
+                super(StageAttempt, self).__init__()
+                self.did_op = []
+
+            def do_operation(self, client):
+                self.did_op.append(client)
+
+            def get_result(self, client):
+                return self.did_op.index(client)
+
+
+        attempt = StubSA()
+        old = object()
+        new = object()
+        result = attempt.do_stage(old, new)
+        self.assertEqual([old, new], attempt.did_op)
+        self.assertEqual(result, (0, 1))
+
+
+class FakeEnvJujuClient(EnvJujuClient):
+
+    def __init__(self, name='steve'):
+        super(FakeEnvJujuClient, self).__init__(
+            SimpleEnvironment(name, {'type': 'fake'}), '1.2', '/jbin/juju')
+
+    def wait_for_started(self):
+        with patch('sys.stdout'):
+            super(FakeEnvJujuClient, self).wait_for_started(0.01)
+
+    def juju(self, *args, **kwargs):
+        # Suppress stdout for juju commands.
+        with patch('sys.stdout'):
+            return super(FakeEnvJujuClient, self).juju(*args, **kwargs)
+
+
+
+def assert_juju_call(test_case, mock_method, client, expected_args,
+                     call_index=None):
+    if call_index is None:
+        test_case.assertEqual(len(mock_method.mock_calls), 1)
+        call_index = 0
+    empty, args, kwargs = mock_method.mock_calls[call_index]
+    test_case.assertEqual(args, (expected_args,))
+    test_case.assertEqual(kwargs.keys(), ['env'])
+    bin_dir = os.path.dirname(client.full_path)
+    test_case.assertRegexpMatches(kwargs['env']['PATH'],
+    r'^{}\:'.format(bin_dir))
+
+
+class TestBootstrapAttempt(TestCase):
+
+    def test_do_operation(self):
+        client = FakeEnvJujuClient()
+        bootstrap = BootstrapAttempt()
+        with patch('subprocess.check_call') as mock_cc:
+            bootstrap.do_operation(client)
+        assert_juju_call(self, mock_cc, client, (
+            'juju', '--show-log', 'bootstrap', '-e', 'steve',
+            '--constraints', 'mem=2G'))
+
+    def test_do_operation_exception(self):
+        client = FakeEnvJujuClient()
+        bootstrap = BootstrapAttempt()
+        with patch('subprocess.check_call', side_effect=Exception
+                ) as mock_cc:
+            bootstrap.do_operation(client)
+        assert_juju_call(self, mock_cc, client, (
+            'juju', '--show-log', 'bootstrap', '-e', 'steve',
+            '--constraints', 'mem=2G'))
+        output = yaml.safe_dump({
+            'machines': {'0': {'agent-state': 'started'}},
+            'services': {},
+            })
+        with patch('subprocess.check_output', return_value=output):
+            self.assertFalse(bootstrap.get_result(client))
+
+    def test_get_result_true(self):
+        bootstrap = BootstrapAttempt()
+        client = FakeEnvJujuClient()
+        output = yaml.safe_dump({
+            'machines': {'0': {'agent-state': 'started'}},
+            'services': {},
+            })
+        with patch('subprocess.check_output', return_value=output):
+            self.assertTrue(bootstrap.get_result(client))
+
+    def test_get_result_false(self):
+        bootstrap = BootstrapAttempt()
+        client = FakeEnvJujuClient()
+        output = yaml.safe_dump({
+            'machines': {'0': {'agent-state': 'pending'}},
+            'services': {},
+            })
+        with patch('subprocess.check_output', return_value=output):
+            self.assertFalse(bootstrap.get_result(client))
+
+
+class TestDestroyEnvironmentAttempt(TestCase):
+
+    def test_do_operation(self):
+        client = FakeEnvJujuClient()
+        bootstrap = DestroyEnvironmentAttempt()
+        with patch('subprocess.check_call') as mock_cc:
+            bootstrap.do_operation(client)
+        assert_juju_call(self, mock_cc, client, (
+            'juju', '--show-log', 'destroy-environment', '-y', 'steve'))