Merge lp:~sseman/juju-ci-tools/model-change-watcher-py3 into lp:juju-ci-tools

[Seman]

This branch adds a test for 2.1 feature "Model changes needs to be in mega-watcher". It basically
   - bootstraps, deploys,
   - start listening to the watcher asynchronously,
   - make a config change,
   - verify the config change event is sent by Juju.

Since this test uses https://github.com/juju/python-libjuju, it runs under Python 3. It also updates some of the shared juju-ci-tools packages so that they work with Python 2 as well as Python 3.

If you have issue installing https://github.com/juju/python-libjuju from the source, just clone the repo and add PYTHONPATH to the lib.

Makefile has been updated to exclude this test from Python 2 lint checks but added another test for Python 3 files.

This is one of the two branches. What is missing from this branch and will address in next branch:
   - Add unit tests
   - Update makefile to run Py2 and Py3 tests.

I ran this test and it is working https://pastebin.canonical.com/172811/

[lp:~sseman/juju-ci-tools/model-change-watcher-py3]

1789. By Aaron Bentley on 2016-12-06

Remove --clean and WaitForSearch.

1790. By Curtis Hovey on 2016-12-06

Upgrade maas micro version.

1791. By Curtis Hovey on 2016-12-06

Access container networking on xenial.

1792. By Aaron Bentley on 2016-12-06

Provide progress output from wait_for.

[Martin Packman (gz)]

No detailed comments on the asyncio script, but nothing to block landing on. See comments on Python 3 related changes.

review: Needs Fixing

[lp:~sseman/juju-ci-tools/model-change-watcher-py3]

1793. By Andrew James Beach on 2016-12-07

Merged in fixes to model migration tests.

[Seman (sseman)]

Thanks for reviewing. Mostly, I updated the code per you suggestion. Some will update on my follow up branch. See inline comments.

Incremental diff https://pastebin.canonical.com/173065/

[lp:~sseman/juju-ci-tools/model-change-watcher-py3]

1794. By Aaron Bentley on 2016-12-08

Implmement wait_for_version using wait_for.

1795. By Andrew James Beach on 2016-12-08

Merged in fake_juju_client's new juju_home argument.

[Martin Packman (gz)]

Thanks! One note over a misunderstanding of a suggestion.

review: Approve

[Aaron Bentley (abentley)]

> Thanks for reviewing. Mostly, I updated the code per you suggestion. Some will update on my follow up branch. See inline comments.
>> - for model in self.models.values():
>> + for model in list(self.models.values()):
>
> I changed it back. Maybe this is something added for debugging an issue.

I wrote that code for a reason. For you. If you don't understand it, you should ask me.

Specifically, the length of self.models.values changes during iteration. You will see a warning about that when you run the tests. listifying self.models.values fixes that problem.

The length of self.models.values changes because FakeEnvironmentState.destroy_model does:
del self.controller.models[self.name]

review: Needs Fixing

[lp:~sseman/juju-ci-tools/model-change-watcher-py3]

1796. By Seman on 2016-12-08

Merged.

1797. By Seman on 2016-12-09

Python 2/3 compatibility update.

[Seman (sseman)]

I wasn't sure who updated the code, so I removed the change and ran the tests. Since the code is in fakejuju, any issue should be discovered by just running the tests. The test didn't fail but I didn't notice the warning. This code has been reverted.

Incremental diff https://pastebin.canonical.com/173183/

[Nicholas Skaggs (nskaggs)]

review: Approve

=== modified file 'Makefile'
--- Makefile	2016-11-18 20:05:05 +0000
+++ Makefile	2016-12-09 00:09:01 +0000
@@ -1,8 +1,10 @@
 p=test*.py
+py3="assess_model_change_watcher.py"
 test:
 	TMPDIR=/tmp python -m unittest discover -vv ./tests -p "$(p)"
 lint:
-	flake8 $$(find -name '*.py') --builtins xrange,basestring
+	python3 -m flake8 --builtins xrange,basestring $(py3)
+	flake8 $$(find -name '*.py') --builtins xrange,basestring --exclude $(py3)
 cover:
 	python -m coverage run --source="./" --omit "./tests/*" -m unittest discover -vv ./tests
 	python -m coverage report

=== modified file 'assess_constraints.py'
--- assess_constraints.py	2016-10-22 02:03:03 +0000
+++ assess_constraints.py	2016-12-09 00:09:01 +0000
@@ -219,7 +219,7 @@
     status = client.get_status()
     app_data = status.get_applications()[application]
     machines = [unit_data['machine'] for unit_data in
-                app_data['units'].itervalues()]
+                app_data['units'].values()]
     return machines
 
 

=== added file 'assess_model_change_watcher.py'
--- assess_model_change_watcher.py	1970-01-01 00:00:00 +0000
+++ assess_model_change_watcher.py	2016-12-09 00:09:01 +0000
@@ -0,0 +1,122 @@
+#!/usr/bin/env python3
+
+import argparse
+import asyncio
+import logging
+import os
+import sys
+
+from juju.client.connection import Connection
+from juju.client import watcher
+
+from deploy_stack import (
+    BootstrapManager,
+    until_timeout,
+    )
+from jujucharm import local_charm_path
+from utility import (
+    add_basic_testing_arguments,
+    configure_logging,
+    JujuAssertionError,
+    )
+
+
+log = logging.getLogger("assess_model_change_watcher")
+TOKEN = "1234asdf"
+
+
+def is_config_change_in_event(event):
+    message = _get_message(event)
+    return all([
+        "application" in event,
+        "change" in event,
+        is_in_dict("config", {"token": TOKEN}, message),
+    ])
+
+
+def _get_message(event):
+    for message in event:
+        if isinstance(message, dict):
+            return message
+    return None
+
+
+def is_in_dict(key, value, items):
+    return items.get(key) == value
+
+
+async def listen_to_watcher(event_found, conn, future):
+    logging.info("Starting to listen for the watcher.")
+    all_watcher = watcher.AllWatcher()
+    all_watcher.connect(conn)
+    for _ in until_timeout(120):
+        logging.info("Listening for events...")
+        change = await all_watcher.Next()
+        for delta in change.deltas:
+            logging.info("Event received: {}".format(str(delta.deltas)))
+            if event_found(delta.deltas) is True:
+                await all_watcher.Stop()
+                await conn.close()
+                logging.info("Event found: {}".format(str(delta.deltas)))
+                future.set_result(True)
+                return
+
+    await all_watcher.Stop()
+    await conn.close()
+    logging.warning("Event not found.")
+    future.set_result(False)
+
+
+def run_listener(client, event, juju_bin):
+    logging.info("Running listener.")
+    loop = asyncio.get_event_loop()
+    future = asyncio.Future()
+
+    logging.info("Connect to the current model.")
+    os.environ['JUJU_DATA'] = client.env.juju_home
+    os.environ['PATH'] = "{}{}{}".format(
+        juju_bin, os.pathsep, os.environ.get('PATH', ''))
+    conn = loop.run_until_complete(Connection.connect_current())
+    logging.info("Connected to the current model.")
+
+    asyncio.ensure_future(listen_to_watcher(event, conn, future))
+    return loop, future
+
+
+def assess_model_change_watcher(client, charm_series, juju_bin):
+    charm = local_charm_path(
+        charm='dummy-source', juju_ver=client.version, series=charm_series,
+        platform='ubuntu')
+    client.deploy(charm)
+    client.wait_for_started()
+
+    loop, future = run_listener(client, is_config_change_in_event, juju_bin)
+
+    logging.info("Making config change.")
+    client.set_config('dummy-source', {'token': TOKEN})
+
+    loop.run_until_complete(future)
+    result = future.result()
+    if result is not True:
+        raise JujuAssertionError("Config change event was not sent.")
+    loop.close()
+
+
+def parse_args(argv):
+    parser = argparse.ArgumentParser(description="Assess config change.")
+    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_model_change_watcher(
+            bs_manager.client, bs_manager.series, args.juju_bin)
+    return 0
+
+
+if __name__ == '__main__':
+    sys.exit(main())

=== modified file 'deploy_stack.py'
--- deploy_stack.py	2016-12-01 16:39:37 +0000
+++ deploy_stack.py	2016-12-09 00:09:01 +0000
@@ -3,10 +3,13 @@
 
 
 from argparse import ArgumentParser
-from contextlib import (
-    contextmanager,
-    nested,
-)
+from contextlib import contextmanager
+try:
+    from contextlib import nested
+except ImportError:
+    from contextlib import ExitStack as nested
+
+
 from datetime import (
     datetime,
 )
@@ -274,7 +277,7 @@
         if machine_id not in machines:
             machines[machine_id] = remote_from_address(address)
     # Update remote machines in place with real addresses if substrate needs.
-    resolve_remote_dns_names(client.env, machines.itervalues())
+    resolve_remote_dns_names(client.env, machines.values())
     return machines
 
 

=== modified file 'fakejuju.py'
--- fakejuju.py	2016-12-08 15:12:32 +0000
+++ fakejuju.py	2016-12-09 00:09:01 +0000
@@ -21,14 +21,11 @@
 
 __metaclass__ = type
 
-
-def check_juju_output(func):
-    def wrapper(*args, **kwargs):
-        result = func(*args, **kwargs)
-        if 'service' in result:
-            raise AssertionError('Result contained service')
-        return result
-    return wrapper
+# Python 2 and 3 compatibility
+try:
+    argtype = basestring
+except NameError:
+    argtype = str
 
 
 class ControllerOperation(Exception):
@@ -106,7 +103,7 @@
         self.state = 'registered'
 
     def destroy(self, kill=False):
-        for model in self.models.values():
+        for model in list(self.models.values()):
             model.destroy_model()
         self.models.clear()
         if kill:
@@ -146,7 +143,7 @@
 
     def add_machine(self, host_name=None, machine_id=None):
         if machine_id is None:
-            machine_id = str(self.machine_id_iter.next())
+            machine_id = str(next(self.machine_id_iter))
         self.machines.add(machine_id)
         if host_name is None:
             host_name = '{}.example.com'.format(machine_id)
@@ -732,13 +729,14 @@
         if model is not None:
             full_args.extend(['-m', model])
         full_args.extend(args)
-        self.log.log(level, ' '.join(full_args))
+        self.log.log(level, u' '.join(full_args))
 
     def juju(self, command, args, used_feature_flags,
              juju_home, model=None, check=True, timeout=None, extra_env=None):
         if 'service' in command:
             raise Exception('Command names must not contain "service".')
-        if isinstance(args, basestring):
+
+        if isinstance(args, argtype):
             args = (args,)
         self._log_command(command, args, model)
         if model is not None:
@@ -867,7 +865,6 @@
         self.juju(command, args, used_feature_flags,
                   juju_home, model, timeout=timeout)
 
-    @check_juju_output
     def get_juju_output(self, command, args, used_feature_flags, juju_home,
                         model=None, timeout=None, user_name=None,
                         merge_stderr=False):
@@ -910,7 +907,7 @@
                 status_dict = model_state.get_status_dict()
                 # Parsing JSON is much faster than parsing YAML, and JSON is a
                 # subset of YAML, so emit JSON.
-                return json.dumps(status_dict)
+                return json.dumps(status_dict).encode('utf-8')
             if command == 'create-backup':
                 self.controller_state.require_controller('backup', model)
                 return 'juju-backup-0.tar.gz'

=== modified file 'jujupy.py'
--- jujupy.py	2016-12-08 08:22:52 +0000
+++ jujupy.py	2016-12-09 00:09:01 +0000
@@ -55,6 +55,12 @@
 
 __metaclass__ = type
 
+# Python 2 and 3 compatibility
+try:
+    argtype = basestring
+except NameError:
+    argtype = str
+
 AGENTS_READY = set(['started', 'idle'])
 WIN_JUJU_CMD = os.path.join('\\', 'Progra~2', 'Juju', 'juju.exe')
 
@@ -251,11 +257,11 @@
 
 
 @contextmanager
-def temp_yaml_file(yaml_dict):
+def temp_yaml_file(yaml_dict, encoding="utf-8"):
     temp_file = NamedTemporaryFile(suffix='.yaml', delete=False)
     try:
         with temp_file:
-            yaml.safe_dump(yaml_dict, temp_file)
+            yaml.safe_dump(yaml_dict, temp_file, encoding=encoding)
         yield temp_file.name
     finally:
         os.unlink(temp_file.name)
@@ -1129,7 +1135,7 @@
 
         # If args is a string, make it a tuple. This makes writing commands
         # with one argument a bit nicer.
-        if isinstance(args, basestring):
+        if isinstance(args, argtype):
             args = (args,)
         # we split the command here so that the caller can control where the -m
         # model flag goes.  Everything in the command string is put before the
@@ -1276,7 +1282,7 @@
         enforced.
     """
     version = EnvJujuClient.get_version(juju_path)
-    client_class = get_client_class(version)
+    client_class = get_client_class(str(version))
     if config is None:
         env = client_class.config_class('', {})
     else:
@@ -1385,7 +1391,8 @@
         """
         if juju_path is None:
             juju_path = 'juju'
-        return subprocess.check_output((juju_path, '--version')).strip()
+        version = subprocess.check_output((juju_path, '--version')).strip()
+        return version.decode("utf-8")
 
     def check_timeouts(self):
         return self._backend._check_timeouts()
@@ -1799,7 +1806,7 @@
                 return self.status_class.from_text(
                     self.get_juju_output(
                         self._show_status, '--format', 'yaml',
-                        controller=controller))
+                        controller=controller).decode('utf-8'))
             except subprocess.CalledProcessError:
                 pass
         raise StatusTimeout(

=== modified file 'substrate.py'
--- substrate.py	2016-11-28 22:04:46 +0000
+++ substrate.py	2016-12-09 00:09:01 +0000
@@ -6,8 +6,10 @@
 import os
 import subprocess
 from time import sleep
-import urlparse
-
+try:
+    import urlparse
+except ImportError:
+    import urllib.parse as parser
 from boto import ec2
 from boto.exception import EC2ResponseError
 

=== modified file 'tests/__init__.py'
--- tests/__init__.py	2016-11-21 16:30:01 +0000
+++ tests/__init__.py	2016-12-09 00:09:01 +0000
@@ -5,12 +5,20 @@
 import errno
 import logging
 import os
-import StringIO
+import io
+try:
+    from StringIO import StringIO
+except ImportError:
+    from io import StringIO
 import subprocess
+import sys
 from tempfile import NamedTemporaryFile
 import unittest
 
-from mock import patch
+try:
+    from mock import patch
+except ImportError:
+    from unittest.mock import patch
 import yaml
 
 import utility
@@ -18,7 +26,10 @@
 
 @contextmanager
 def stdout_guard():
-    stdout = StringIO.StringIO()
+    if isinstance(sys.stdout, io.TextIOWrapper):
+        stdout = io.StringIO()
+    else:
+        stdout = io.BytesIO()
     with patch('sys.stdout', stdout):
         yield
     if stdout.getvalue() != '':
@@ -103,7 +114,7 @@
     log = logging.getLogger()
     testcase.addCleanup(setattr, log, 'handlers', log.handlers)
     log.handlers = []
-    testcase.log_stream = StringIO.StringIO()
+    testcase.log_stream = StringIO()
     handler = logging.StreamHandler(testcase.log_stream)
     handler.setFormatter(logging.Formatter("%(levelname)s %(message)s"))
     log.addHandler(handler)
@@ -118,7 +129,10 @@
 
 @contextmanager
 def parse_error(test_case):
-    stderr = StringIO.StringIO()
+    if isinstance(sys.stdout, io.TextIOWrapper):
+        stderr = io.StringIO()
+    else:
+        stderr = io.BytesIO()
     with test_case.assertRaises(SystemExit):
         with patch('sys.stderr', stderr):
             yield stderr

=== modified file 'utility.py'
--- utility.py	2016-11-18 20:26:28 +0000
+++ utility.py	2016-12-09 00:09:01 +0000
@@ -88,6 +88,9 @@
     def now():
         return datetime.now()
 
+    def __next__(self):
+        self.next()
+
     def next(self):
         elapsed = self.now() - self.start
         remaining = self.timeout - elapsed.total_seconds()
@@ -190,7 +193,7 @@
             else:
                 continue
         conn = socket.socket(*addrinfo[0][:3])
-        conn.settimeout(max(remaining, 5))
+        conn.settimeout(max(remaining or 0, 5))
         try:
             conn.connect(sockaddr)
         except socket.timeout: