Merge lp:~mskalka/juju-ci-tools/assess-destroy-model into lp:juju-ci-tools

[Michael Skalka]

In response to: https://bugs.launchpad.net/juju-ci-tools/+bug/1582021

Adds a test that bootstraps, gets the controller ID, adds a model, destroys it, then gets the controller ID again and asserts the IDs are equal. It will fail if juju can't get the controller ID or if they differ.

Additionally it adds an optional arg to client.destroy_model to destroy a model other than the one it's currently pointed at. The original behavior made it impossible to destroy a model without tracking that model through a client object, which is silly.

Also changes the syntax of client.switch() to default to the client's controller if none is specified. In the past switching back to the default jujupy model used in tests was ambiguous as the model and controller had the same name. Although it wasn't used in this test it was bugging me to no end.

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1975. By Michael Skalka on 2017-04-03

docstring edit

[Curtis Hovey (sinzui)]

Hi Michael.

I don't think this test is recreating what happened, nor is it verifying how we know juju is correct. I have many thoughts in line, much regarding the extra changes the branch has.

review: Needs Information (code)

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1976. By Michael Skalka on 2017-04-05

backed out client.py changes

1977. By Michael Skalka on 2017-04-05

restructured test, unittests, backed out client.py changes

[Michael Skalka (mskalka)]

Curtis,

I backed out the changes made to client.py. I will rework and resubmit those as their own MP.

I changed how the test behaves to better align with the committed bug fix. The test now checks to see if Juju drops its current model when that model is destroyed, and raises errors if it doesn't. Is this more in line with what you had in mind for this test?

[Curtis Hovey (sinzui)]

Thank you Michael. This test needs another round of changes. In general never use "assert" and always raise JujuAssertionError when juju fails. Juju errors are being logged as info, but the test will still pass. You might want to consider using
    with logged_exception(logging):
to log exceptions without needing to write log.error.

review: Needs Fixing (code)

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1978. By Michael Skalka on 2017-04-05

changes as per mp comments

1979. By Michael Skalka on 2017-04-05

added unittest for failure cases

[Michael Skalka (mskalka)]

Made the requested changes in regards to the asserts. Also added unittests for failure cases where they were missing. I'm not sure what you meant exactly by "Drop this since you didn't write tests for it." but I assumed that was it.

[Aaron Bentley (abentley)]

get_current_model and get_current_controller appear to reimplement Juju2Backend.get_active_model. If they do not, I recommend explaining the difference in the docstring.

I recommend using --format json or --format yaml in general. It is much more precise than the default textual output. For example, with list-models, parsed['current-model'] gives you the current model.

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1980. By Michael Skalka on 2017-04-05

changes as per mp comments

[Michael Skalka (mskalka)]

I modified the functions to use --format yaml. I didn't know that was even an option with list-models. It's definitely a lot cleaner, so thank you for the suggestion. As for why I this uses list-models instead of switch, the bug specifies list-models and I wanted to keep it as close to the original case as possible.

[Aaron Bentley (abentley)]

--format json or --format yaml is an option with basically every query command (show-x, list-x). I prefer json because the parser is much faster (which can affect unit test speed), you don't need to tell it to be safe, and it's part of the standard library. Unfortunately, juju's json output is not human-readable. (You can pretty-print it by piping it to jq, though.)

I still recommend explaining the difference between get_current_model, get_current_controller, and Juju2Backend.get_active_model in the docstring. Otherwise, someone will "helpfully" remove the code duplication sooner or later.

However, this looks like a response to bug #1505504. Cheryl adjusted the scope of bug #1582021 to be just the timing hole: https://bugs.launchpad.net/juju-ci-tools/+bug/1582021/comments/1

[Michael Skalka (mskalka)]

Hmm. I don't see a report linked for the timing hole, but I'll search through Juju's bug reports. I'm not sure how we could adequately or reliably test for that with the tools we have right now however. Possibly repeatedly adding and deleting a model or series of models in rapid succession and listing each time? I'm happy to update the test if you have a better idea of what we should be attempting.

[Aaron Bentley (abentley)]

> Hmm. I don't see a report linked for the timing hole

The report of the timing hole is bug #1582021 itself.

It is the fact that "list-models" errors with "model not found". It is not erroring because the current-model is wrong. It is erroring because "we first get all models, then query each one for more details"

"bigdata" was in the first list, but was not accessible when queried, producing the "model not found" error.

A correct test for this would be something like:
juju bootstrap --default-model foobar
juju destroy-model foobar
# Avoid bug 1505504.
juju switch controller
# If we have a timing hole, this fails with "model not found"
juju list-models

This test should be run against 2.0-beta7 (or earlier) to prove that it fails, with 2.0-beta8 to prove that it fails even with bug #1505504 fixed, and then with current juju to prove that it passes.

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1981. By Michael Skalka on 2017-04-06

cleanup

1982. By Michael Skalka on 2017-04-06

more cleanup as per mp

1983. By Michael Skalka on 2017-04-06

lint

[Michael Skalka (mskalka)]

Ok. I restructured the test a little to reflect what you think is the proper procedure and swapped to json at your recommendation.

Now it:
boostraps and stores that as a ModelClient object
adds a model which is captured as a new ModelClient object
destroys that model using that object
lists model so we know we avoid bug 1505504
switches back to the original model using our first client object
runs list-model on that client object to try to catch that timing hole

I confirmed the test fails on 2.0-beta8; it throws a called process error on list-models which is the expected outcome. It passes on current juju versions.

[Aaron Bentley (abentley)]

This looks good, though please update the error message as noted below. Also, Curtis should re-review before you merge to make sure his concerns are dealt with.

A few notes about testing:
assertRaisesRegexp (aka assertRaisesRegex in Python3) is much nicer than assertRaises followed by checking message content.

Also, assertIn should be used in preference to assertTrue(x in y).

review: Approve

[lp:~mskalka/juju-ci-tools/assess-destroy-model]

1984. By Michael Skalka on 2017-04-07

unittest changes as per mp comments

1985. By Michael Skalka on 2017-04-07

docstring edit

[Curtis Hovey (sinzui)]

Thank you.

review: Approve (code)

=== added file 'assess_destroy_model.py'
--- assess_destroy_model.py	1970-01-01 00:00:00 +0000
+++ assess_destroy_model.py	2017-04-07 20:13:21 +0000
@@ -0,0 +1,164 @@
+#!/usr/bin/env python
+"""Assess if Juju tracks the model when the current model is destroyed."""
+
+from __future__ import print_function
+
+import argparse
+import logging
+import sys
+import subprocess
+import json
+
+from deploy_stack import (
+    BootstrapManager,
+    )
+from utility import (
+    add_basic_testing_arguments,
+    configure_logging,
+    JujuAssertionError,
+    )
+
+
+__metaclass__ = type
+
+
+log = logging.getLogger("assess_destroy_model")
+
+TEST_MODEL = 'test-tmp-env'
+
+
+def assess_destroy_model(client):
+    """Tests if Juju tracks the model properly through deletion.
+
+    In normal behavior Juju should drop the current model selection if that
+    model is destroyed. This will fail if Juju does not drop it's current
+    selection.
+
+    :param client: Jujupy ModelClient object
+    """
+
+    current_model = get_current_model(client)
+    controller = get_current_controller(client)
+    log.info('Current model: {}'.format(current_model))
+
+    new_client = add_model(client)
+    destroy_model(client, new_client)
+
+    log.info('Juju successfully dropped its current model. '
+             'Switching to {} to complete test'.format(current_model))
+    switch_model(client, current_model, controller)
+
+    log.info('SUCCESS')
+
+
+def add_model(client):
+    """Adds a model to the current juju environment then destroys it.
+
+    Will raise an exception if the Juju does not deselect the current model.
+
+    :param client: Jujupy ModelClient object
+    """
+    log.info('Adding model "{}" to current controller'.format(TEST_MODEL))
+    new_client = client.add_model(TEST_MODEL)
+    new_model = get_current_model(new_client)
+    if new_model == TEST_MODEL:
+        log.info('Current model and newly added model match')
+    else:
+        error = ('Juju failed to switch to new model after creation. '
+                 'Expected {} got {}'.format(TEST_MODEL, new_model))
+        raise JujuAssertionError(error)
+    return new_client
+
+
+def destroy_model(client, new_client):
+    log.info('Destroying model "{}"'.format(TEST_MODEL))
+    new_client.destroy_model()
+    new_model = get_current_model(client)
+    if new_model:
+        error = 'Juju failed to unset model after it was destroyed'
+        raise JujuAssertionError(error)
+
+
+def switch_model(client, current_model, current_controller):
+    """Switches back to the old model.
+
+    :param client: Jujupy ModelClient object
+    :param current_model: String name of initial testing model
+    :param current_controller: String name of testing controller
+    """
+    client.switch(model=current_model, controller=current_controller)
+    new_model = get_current_model(client)
+    if new_model == current_model:
+        log.info('Current model and switch target match')
+    else:
+        error = ('Juju failed to switch back to existing model. '
+                 'Expected {} got {}'.format(TEST_MODEL, new_model))
+        raise JujuAssertionError(error)
+
+
+def get_current_controller(client):
+    """Gets the current controller from Juju's list-models command.
+
+    :param client: Jujupy ModelClient object
+    :return: String name of current controller
+    """
+    raw = client.get_juju_output('switch', include_e=False).decode('utf-8')
+    raw = raw.split(':')[0]
+    return raw
+
+
+def get_current_model(client):
+    """Gets the current model from Juju's list-models command.
+
+    :param client: Jujupy ModelClient object
+    :return: String name of current model
+    """
+    raw = list_models(client)
+    try:
+        return raw['current-model']
+    except KeyError as e:
+        log.warning('No model is currently selected.')
+        return None
+
+
+def list_models(client):
+    """Helper function to get the output of juju's list-models command.
+
+    Instead of using 'juju switch' or client.backend.get_active_model() we use
+    list-models because that was what the bug report this test was generated
+    around used. It also allows for flexiblity in the future to get more
+    detailed information about the models that Juju thinks it has
+    if we need it.
+
+    :param client: Jujupy ModelClient object
+    :return: Dict of list-models command
+    """
+    try:
+        raw = client.get_juju_output('list-models', '--format', 'json',
+                                     include_e=False).decode('utf-8')
+    except subprocess.CalledProcessError as e:
+        log.error('Failed to list current models due to error: {}'.format(e))
+        raise e
+    return json.loads(raw)
+
+
+def parse_args(argv):
+    """Parse all arguments."""
+    parser = argparse.ArgumentParser(
+        description='Test if juju drops selection of the current model '
+        'when that model is destroyed.')
+    add_basic_testing_arguments(parser)
+    return parser.parse_args(argv)
+
+
+def main(argv=None):
+    args = parse_args(argv)
+    configure_logging(args.verbose)
+    bs_manager = BootstrapManager.from_args(args)
+    with bs_manager.booted_context(args.upload_tools):
+        assess_destroy_model(bs_manager.client)
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())

=== added file 'tests/test_assess_destroy_model.py'
--- tests/test_assess_destroy_model.py	1970-01-01 00:00:00 +0000
+++ tests/test_assess_destroy_model.py	2017-04-07 20:13:21 +0000
@@ -0,0 +1,175 @@
+"""Tests for assess_destroy_model module."""
+
+import logging
+import StringIO
+from textwrap import dedent
+
+from mock import (
+    Mock,
+    call,
+    patch,
+    )
+
+from assess_destroy_model import (
+    add_model,
+    destroy_model,
+    switch_model,
+    parse_args,
+    main,
+    )
+from jujupy import (
+    fake_juju_client,
+    )
+from tests import (
+    parse_error,
+    TestCase,
+    )
+from utility import (
+    JujuAssertionError
+    )
+
+list_model_initial = dedent(
+    """{
+    "models": [
+        {"name": "controller",
+         "controller-name": "foo-tmp-env"},
+        {"name": "foo-tmp-env",
+         "controller-name": "foo-tmp-env"}],
+    "current-model": "foo-tmp-env"
+    }""")
+
+list_model_destroy = dedent(
+    """{
+    "models": [
+        {"name": "controller",
+         "controller-name": "foo-tmp-env"},
+        {"name": "foo-tmp-env",
+         "controller-name": "foo-tmp-env"}]
+    }""")
+
+list_model_add = dedent(
+    """{
+    "models": [
+        {"name": "controller",
+         "controller-name": "foo-tmp-env"},
+        {"name": "test-tmp-env",
+         "controller-name": "foo-tmp-env"},
+        {"name": "foo-tmp-env",
+         "controller-name": "foo-tmp-env"}],
+    "current-model": "test-tmp-env"
+    }""")
+
+list_model_switch = dedent(
+    """{
+    "models": [
+        {"name": "controller",
+         "controller-name": "foo-tmp-env"},
+        {"name": "foo-tmp-env",
+         "controller-name": "foo-tmp-env"}],
+    "current-model": "bar-tmp-env"
+    }""")
+
+
+class TestParseArgs(TestCase):
+
+    def test_common_args(self):
+        args = parse_args(["an-env", "/bin/juju", "/tmp/logs", "an-env-mod"])
+        self.assertEqual("an-env", args.env)
+        self.assertEqual("/bin/juju", args.juju_bin)
+        self.assertEqual("/tmp/logs", args.logs)
+        self.assertEqual("an-env-mod", args.temp_env_name)
+        self.assertEqual(False, args.debug)
+
+    def test_help(self):
+        fake_stdout = StringIO.StringIO()
+        with parse_error(self) as fake_stderr:
+            with patch("sys.stdout", fake_stdout):
+                parse_args(["--help"])
+        self.assertEqual("", fake_stderr.getvalue())
+        self.assertNotIn("TODO", fake_stdout.getvalue())
+
+
+class TestMain(TestCase):
+
+    def test_main(self):
+        argv = ["an-env", "/bin/juju", "/tmp/logs", "an-env-mod", "--verbose"]
+        client = Mock(spec=["is_jes_enabled"])
+        with patch("assess_destroy_model.configure_logging",
+                   autospec=True) as mock_cl:
+            with patch("assess_destroy_model.BootstrapManager.booted_context",
+                       autospec=True) as mock_bc:
+                with patch('deploy_stack.client_from_config',
+                           return_value=client) as mock_cfc:
+                    with patch("assess_destroy_model.assess_destroy_model",
+                               autospec=True) as mock_assess:
+                        main(argv)
+        mock_cl.assert_called_once_with(logging.DEBUG)
+        mock_cfc.assert_called_once_with('an-env', "/bin/juju", debug=False,
+                                         soft_deadline=None)
+        self.assertEqual(mock_bc.call_count, 1)
+        mock_assess.assert_called_once_with(client)
+
+
+class TestAssess(TestCase):
+
+    def test_add_model(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_add
+        fake_client.add_model.return_value = fake_client
+        add_model(fake_client)
+        self.assertEqual(
+            [call.bootstrap(),
+             call.add_model('test-tmp-env'),
+             call.get_juju_output('list-models', '--format', 'json',
+                                  include_e=False)],
+            fake_client.mock_calls)
+
+    def test_add_model_fails(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_initial
+        fake_client.add_model.return_value = fake_client
+        pattern = 'Expected test-tmp-env got foo-tmp-env'
+        with self.assertRaisesRegexp(JujuAssertionError, pattern):
+            add_model(fake_client)
+
+    def test_destroy_model(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_destroy
+        destroy_model(fake_client, fake_client)
+        self.assertEqual(
+            [call.bootstrap(),
+             call.destroy_model(),
+             call.get_juju_output('list-models', '--format', 'json',
+                                  include_e=False)],
+            fake_client.mock_calls)
+
+    def test_destroy_model_fails(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_initial
+        pattern = 'Juju failed to unset model after it was destroyed'
+        with self.assertRaisesRegexp(JujuAssertionError, pattern):
+            destroy_model(fake_client, fake_client)
+
+    def test_switch_model(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_initial
+        switch_model(fake_client, 'foo-tmp-env', 'foo-tmp-env')
+        self.assertEqual(
+            [call.bootstrap(),
+             call.switch(controller='foo-tmp-env', model='foo-tmp-env'),
+             call.get_juju_output('list-models', '--format', 'json',
+                                  include_e=False)],
+            fake_client.mock_calls)
+
+    def test_switch_model_fails(self):
+        fake_client = Mock(wraps=fake_juju_client())
+        fake_client.bootstrap()
+        fake_client.get_juju_output.return_value = list_model_switch
+        pattern = 'Expected test-tmp-env got bar-tmp-env'
+        with self.assertRaisesRegexp(JujuAssertionError, pattern):
+            switch_model(fake_client, 'foo-tmp-env', 'test-tmp-env')