Merge lp:~gz/juju-ci-tools/winrm_copy into lp:juju-ci-tools

[Martin Packman]

Enable windows deployment testing and log collection

Debugging the windows deploy test was annoying without easy access to the artifacts, so this branch adds a copy implementation on top of winrm. Also needed a bunch of refactoring of log collection methods to pass sufficient information to handle remote access correctly.

The copy scheme is mostly straight forward, there's a big blob of powershell that creates one line per file matching the given blobs, with the filename followed by the compressed-base64'd file contents. That's then parsed in python and splatted to disk.

On the powershell:
* Not easy to unit test under python on linux, I have not attempted.
* Has two functions for doing the copy, only the Binary one is used. My first pass was getting utf-16 encoded contents hence the Text version which does encoding handling. By the time I got back to comparing them, that wrinkle had mysteriously vanished, so just adapting the line-endings is not worth while.
* Needs to handle exceptions in some fashion, I want help with this. Currently you get stderr output but still a 0 return code, and unreadable files would break things.

The decode in python is pretty dumb, it would ideally stream, but that's not well supported in the pywinrm library yet. Currently it holds all files deflated but only one file inflated in memory at once, so is not completely terrible. We redo the compression later in the chain as gzip for logs, but that's fine especially as we have little control on the ratio under windows.

I do not like the way I've changed the maas address special-casing. The first attempt introduced a bug because changing the address attribute of a WinRmRemote object did not actually work, and it's still icky. Doing only one maas lookup rather than one per machine is a requirement though.

[Curtis Hovey (sinzui)]

Thank you Martin. I have a few questions and suggestions inline.

review: Needs Information (code)

[Martin Packman (gz)]

Responded to comments, new rev with updates is following.

[Curtis Hovey (sinzui)]

Oops. I forgot to add a comment to make my inline comments visible. Thank your for answering my questions and commitment to make some changes. You can merge when you are satisfied.

review: Approve (code)

[lp:~gz/juju-ci-tools/winrm_copy]

999. By Martin Packman on 2015-07-02

Update with extra tests and addresses review comments by sinzui

1000. By Martin Packman on 2015-07-02

Always skip assess_juju_run for windows charms

=== modified file 'deploy_stack.py'
--- deploy_stack.py	2015-07-01 23:08:17 +0000
+++ deploy_stack.py	2015-07-02 00:56:56 +0000
@@ -36,6 +36,7 @@
     destroy_job_instances,
     LIBVIRT_DOMAIN_RUNNING,
     MAASAccount,
+    resolve_remote_dns_names,
     run_instances,
     start_libvirt_domain,
     stop_libvirt_domain,
@@ -128,6 +129,8 @@
     # package updates.
     logging.info('Retrieving token.')
     remote = remote_from_unit(client, "dummy-sink/0")
+    # Update remote with real address if needed.
+    resolve_remote_dns_names(client.env, [remote])
     start = time.time()
     while True:
         if remote.is_windows():
@@ -150,14 +153,15 @@
 
 
 def dump_env_logs(client, bootstrap_host, directory, jenv_path=None):
-    machine_addrs = get_machines_for_logs(client, bootstrap_host)
+    remote_machines = get_remote_machines(client, bootstrap_host)
 
-    for machine_id, addr in machine_addrs.iteritems():
-        logging.info("Retrieving logs for machine-%s", machine_id)
+    for machine_id, remote in remote_machines.iteritems():
+        logging.info("Retrieving logs for machine-%s using %r", machine_id,
+                     remote)
         machine_directory = os.path.join(directory, "machine-%s" % machine_id)
         os.mkdir(machine_directory)
         local_state_server = client.env.local and machine_id == '0'
-        dump_logs(client, addr, machine_directory,
+        dump_logs(client.env, remote, machine_directory,
                   local_state_server=local_state_server)
     retain_jenv(jenv_path, directory)
 
@@ -175,47 +179,43 @@
     return False
 
 
-def get_machines_for_logs(client, bootstrap_host):
-    """Return a dict of machine_id and address.
+def get_remote_machines(client, bootstrap_host):
+    """Return a dict of machine_id to remote machines.
 
-    With a maas environment, the maas api will be queried for the real
-    ip addresses. Otherwise, bootstrap_host may be provided to
-    override the address of machine 0.
+    A bootstrap_host address may be provided as a fallback for machine 0 if
+    status fails. For some providers such as MAAS the dns-name will be
+    resolved to a real ip address using the substrate api.
     """
     # Try to get machine details from environment if possible.
-    machine_addrs = dict(get_machine_addrs(client))
-
-    if client.env.config['type'] == 'maas':
-        # Maas hostnames are not resolvable, but we can adapt them to IPs.
-        with MAASAccount.manager_from_config(client.env.config) as account:
-            allocated_ips = account.get_allocated_ips()
-        for machine_id, hostname in machine_addrs.items():
-            machine_addrs[machine_id] = allocated_ips.get(hostname, hostname)
-    elif bootstrap_host:
-        # The bootstrap host overrides the status output if provided in case
-        # status failed.
-        machine_addrs['0'] = bootstrap_host
-    return machine_addrs
-
-
-def get_machine_addrs(client):
+    machines = dict(iter_remote_machines(client))
+    # The bootstrap host is added as a fallback in case status failed.
+    if bootstrap_host and '0' not in machines:
+        machines['0'] = remote_from_address(bootstrap_host)
+    # Update remote machines in place with real addresses if substrate needs.
+    resolve_remote_dns_names(client.env, machines.itervalues())
+    return machines
+
+
+def iter_remote_machines(client):
     try:
         status = client.get_status()
     except Exception as err:
         logging.warning("Failed to retrieve status for dumping logs: %s", err)
         return
+
     for machine_id, machine in status.iter_machines():
         hostname = machine.get('dns-name')
         if hostname:
-            yield machine_id, hostname
-
-
-def dump_logs(client, host, directory, local_state_server=False):
+            remote = remote_from_address(hostname, machine.get('series'))
+            yield machine_id, remote
+
+
+def dump_logs(env, remote, directory, local_state_server=False):
     try:
         if local_state_server:
-            copy_local_logs(directory, client)
+            copy_local_logs(env, directory)
         else:
-            copy_remote_logs(host, directory)
+            copy_remote_logs(remote, directory)
         subprocess.check_call(
             ['gzip', '-f'] +
             glob.glob(os.path.join(directory, '*.log')))
@@ -227,8 +227,8 @@
 lxc_template_glob = '/var/lib/juju/containers/juju-*-lxc-template/*.log'
 
 
-def copy_local_logs(directory, client):
-    local = get_local_root(get_juju_home(), client.env)
+def copy_local_logs(env, directory):
+    local = get_local_root(get_juju_home(), env)
     log_names = [os.path.join(local, 'cloud-init-output.log')]
     log_names.extend(glob.glob(os.path.join(local, 'log', '*.log')))
     log_names.extend(glob.glob(lxc_template_glob))
@@ -237,28 +237,33 @@
     subprocess.check_call(['cp'] + log_names + [directory])
 
 
-def copy_remote_logs(host, directory):
+def copy_remote_logs(remote, directory):
     """Copy as many logs from the remote host as possible to the directory."""
     # This list of names must be in the order of creation to ensure they
     # are retrieved.
-    log_paths = [
-        '/var/log/cloud-init*.log',
-        '/var/log/juju/*.log',
-    ]
-    remote = remote_from_address(address=host)
-
-    try:
-        wait_for_port(host, 22, timeout=60)
-    except PortTimeoutError:
-        logging.warning("Could not dump logs because port 22 was closed.")
-        return
-
-    try:
-        remote.run('sudo chmod go+r /var/log/juju/*')
-    except subprocess.CalledProcessError as e:
-        # The juju log dir is not created until after cloud-init succeeds.
-        logging.warning("Could not change the permission of the juju logs:")
-        logging.warning(e.output)
+    if remote.is_windows():
+        log_paths = [
+            "%ProgramFiles(x86)%\\Cloudbase Solutions\\Cloudbase-Init\\log\\*",
+            "C:\\Juju\\log\\juju\\*.log",
+        ]
+    else:
+        log_paths = [
+            '/var/log/cloud-init*.log',
+            '/var/log/juju/*.log',
+        ]
+
+        try:
+            wait_for_port(remote.address, 22, timeout=60)
+        except PortTimeoutError:
+            logging.warning("Could not dump logs because port 22 was closed.")
+            return
+
+        try:
+            remote.run('sudo chmod go+r /var/log/juju/*')
+        except subprocess.CalledProcessError as e:
+            # The juju log dir is not created until after cloud-init succeeds.
+            logging.warning("Could not allow access to the juju logs:")
+            logging.warning(e.output)
 
     try:
         remote.copy(directory, log_paths)
@@ -284,7 +289,7 @@
     return responses
 
 
-def assess_upgrade(old_client, juju_path):
+def assess_upgrade(old_client, juju_path, skip_juju_run=False):
     client = EnvJujuClient.by_version(old_client.env, juju_path,
                                       old_client.debug)
     upgrade_juju(client)
@@ -293,7 +298,8 @@
     else:
         timeout = 600
     client.wait_for_version(client.get_matching_agent_version(), timeout)
-    assess_juju_run(client)
+    if not skip_juju_run:
+        assess_juju_run(client)
     token = get_random_string()
     client.juju('set', ('dummy-source', 'token=%s' % token))
     check_token(client, token)
@@ -502,10 +508,12 @@
             return
         client.juju('status', ())
         deploy_dummy_stack(client, charm_prefix)
-        assess_juju_run(client)
+        is_windows_charm = charm_prefix.startswith("local:win")
+        if not is_windows_charm:
+            assess_juju_run(client)
         if upgrade:
             client.juju('status', ())
-            assess_upgrade(client, juju_path)
+            assess_upgrade(client, juju_path, skip_juju_run=is_windows_charm)
 
 
 def run_deployer():

=== modified file 'remote.py'
--- remote.py	2015-06-24 15:32:05 +0000
+++ remote.py	2015-07-02 00:56:56 +0000
@@ -4,11 +4,12 @@
 
 import abc
 import logging
+import os
 import subprocess
+import zlib
 
 import winrm
 
-from substrate import MAASAccount
 import utility
 
 
@@ -38,7 +39,7 @@
 
 
 class _Remote:
-    """Remote represents a juju machine to access over the network."""
+    """_Remote represents a juju machine to access over the network."""
 
     __metaclass__ = abc.ABCMeta
 
@@ -79,6 +80,15 @@
         """Returns True if remote machine is running windows."""
         return self.series and self.series.startswith("win")
 
+    def get_address(self):
+        """Gives the address of the remote machine."""
+        self._ensure_address()
+        return self.address
+
+    def update_address(self, address):
+        """Change address of remote machine."""
+        self.address = address
+
     def _get_status(self):
         if self.status is None:
             self.status = self.client.get_status()
@@ -92,17 +102,10 @@
         status = self._get_status()
         unit = status.get_unit(self.unit)
         self.address = unit['public-address']
-        # TODO(gz): Avoid special casing MAAS here and in deploy_stack
-        if self.client.env.config['type'] == 'maas':
-            config = self.client.env.config
-            with MAASAccount.manager_from_config(config) as account:
-                allocated_ips = account.get_allocated_ips()
-            if self.address in allocated_ips:
-                self.address = allocated_ips[self.address]
 
 
 class SSHRemote(_Remote):
-    """Remote represents a juju machine to access over the network."""
+    """SSHRemote represents a juju machine to access using ssh."""
 
     _ssh_opts = [
         "-o", "User ubuntu",
@@ -158,13 +161,60 @@
                                        cert_key_pem=key, cert_pem=cert)
 
 
+_ps_copy_script = """\
+function CopyText {
+    param([String]$file, [IO.Stream]$outstream)
+    $enc = New-Object Text.UTF8Encoding($False)
+    $us = New-Object IO.StreamWriter($outstream, $enc)
+    $us.NewLine = "\n"
+    $uin = New-Object IO.StreamReader($file)
+    while (($line = $uin.ReadLine()) -ne $Null) {
+        $us.WriteLine($line)
+    }
+    $uin.Close()
+    $us.Close()
+}
+
+function CopyBinary {
+    param([String]$file, [IO.Stream]$outstream)
+    $in = New-Object IO.FileStream($file, [IO.FileMode]::Open,
+        [IO.FileAccess]::Read, [IO.FileShare]"ReadWrite,Delete")
+    $in.CopyTo($outstream)
+    $in.Close()
+}
+
+ForEach ($pattern in %s) {
+    $path = [Environment]::ExpandEnvironmentVariables($pattern)
+    $files = Get-Item -path $path
+    ForEach ($file in $files) {
+        [Console]::Out.Write($file.name + "|")
+        $trans = New-Object Security.Cryptography.ToBase64Transform
+        $out = [Console]::OpenStandardOutput()
+        $bs = New-Object Security.Cryptography.CryptoStream($out, $trans,
+            [Security.Cryptography.CryptoStreamMode]::Write)
+        $zs = New-Object IO.Compression.DeflateStream($bs,
+            [IO.Compression.CompressionMode]::Compress)
+        CopyBinary -file $file -outstream $zs
+        $zs.Close()
+        [Console]::Out.Write("\n")
+    }
+}
+"""
+
+
 class WinRmRemote(_Remote):
+    """WinRmRemote represents a juju machine to access using winrm."""
 
     def __init__(self, *args, **kwargs):
         super(WinRmRemote, self).__init__(*args, **kwargs)
         self._ensure_address()
-        certs = utility.get_winrm_certs()
-        self.session = _SSLSession(self.address, certs)
+        self.certs = utility.get_winrm_certs()
+        self.session = _SSLSession(self.address, self.certs)
+
+    def update_address(self, address):
+        """Change address of remote machine, refreshes the winrm session."""
+        self.address = address
+        self.session = _SSLSession(self.address, self.certs)
 
     _escape = staticmethod(subprocess.list2cmdline)
 
@@ -196,4 +246,40 @@
 
     def copy(self, destination_dir, source_globs):
         """Copy files from the remote machine."""
-        raise NotImplementedError
+        # Encode globs into script to run on remote machine and return result.
+        script = _ps_copy_script % ",".join(s.join('""') for s in source_globs)
+        result = self.run_ps(script)
+        if result.std_err:
+            logging.warning("winrm copy stderr:\n%s", result.std_err)
+        if result.status_code:
+            raise subprocess.CalledProcessError(result.status_code,
+                                                "powershell", result)
+        self._encoded_copy_to_dir(destination_dir, result.std_out)
+
+    @staticmethod
+    def _encoded_copy_to_dir(destination_dir, output):
+        """Write remote files from powershell script to disk.
+
+        The given output from the powershell script is one line per file, with
+        the filename first, then a pipe, then the base64 encoded deflated file
+        contents. This method reverses that process and creates the files in
+        the given destination_dir.
+        """
+        start = 0
+        while True:
+            end = output.find("\n", start)
+            if end == -1:
+                break
+            mid = output.find("|", start, end)
+            if mid == -1:
+                if not output[start:end].rstrip("\r\n"):
+                    break
+                raise ValueError("missing filename in encoded copy data")
+            filename = output[start:mid]
+            if "/" in filename:
+                # Just defense against path traversal bugs, should never reach.
+                raise ValueError("path not filename {!r}".format(filename))
+            with open(os.path.join(destination_dir, filename), "wb") as f:
+                f.write(zlib.decompress(output[mid+1:end].decode("base64"),
+                                        -zlib.MAX_WBITS))
+            start = end + 1

=== modified file 'substrate.py'
--- substrate.py	2015-07-01 21:30:33 +0000
+++ substrate.py	2015-07-02 00:56:56 +0000
@@ -588,3 +588,16 @@
     if len(instances) == 0:
         return
     subprocess.check_call(['euca-terminate-instances'] + instances)
+
+
+def resolve_remote_dns_names(env, remote_machines):
+    """Update addresses of given remote_machines as needed by providers."""
+    if env.config['type'] != 'maas':
+        # Only MAAS requires special handling at prsent.
+        return
+    # MAAS hostnames are not resolvable, but we can adapt them to IPs.
+    with MAASAccount.manager_from_config(env.config) as account:
+        allocated_ips = account.get_allocated_ips()
+    for remote in remote_machines:
+        if remote.get_address() in allocated_ips:
+            remote.update_address(allocated_ips[remote.address])

=== modified file 'test_deploy_stack.py'
--- test_deploy_stack.py	2015-07-01 23:08:17 +0000
+++ test_deploy_stack.py	2015-07-02 00:56:56 +0000
@@ -30,8 +30,8 @@
     dump_logs,
     get_juju_path,
     get_log_level,
-    get_machine_addrs,
-    get_machines_for_logs,
+    iter_remote_machines,
+    get_remote_machines,
     GET_TOKEN_SCRIPT,
     prepare_environment,
     assess_upgrade,
@@ -47,6 +47,9 @@
     SimpleEnvironment,
     Status,
 )
+from remote import (
+    remote_from_address,
+)
 from test_jujupy import (
     assert_juju_call,
 )
@@ -65,14 +68,6 @@
     return write_dumped_files
 
 
-def get_machine_addresses():
-    return {
-        '0': '10.10.0.1',
-        '1': '10.10.0.11',
-        '2': '10.10.0.22',
-    }
-
-
 class ArgParserTestCase(TestCase):
 
     def test_add_path_args(self):
@@ -228,14 +223,24 @@
     def setUp(self):
         setup_test_logging(self, level=logging.DEBUG)
 
+    def assert_machines(self, expected, got):
+        self.assertEqual(expected, dict((k, got[k].address) for k in got))
+
+    r0 = remote_from_address('10.10.0.1')
+    r1 = remote_from_address('10.10.0.11')
+    r2 = remote_from_address('10.10.0.22')
+
+    @classmethod
+    def fake_remote_machines(cls):
+        return {'0': cls.r0, '1': cls.r1, '2': cls.r2}
+
     def test_dump_env_logs_non_local_env(self):
         with temp_dir() as artifacts_dir:
-            with patch('deploy_stack.get_machines_for_logs', autospec=True,
-                       return_value=get_machine_addresses()) as gm_mock:
+            with patch('deploy_stack.get_remote_machines', autospec=True,
+                       return_value=self.fake_remote_machines()) as gm_mock:
                 with patch('deploy_stack.dump_logs', autospec=True) as dl_mock:
-                    client = EnvJujuClient(
-                        SimpleEnvironment(
-                            'foo', {'type': 'nonlocal'}), '1.234-76', None)
+                    env = SimpleEnvironment('foo', {'type': 'nonlocal'})
+                    client = EnvJujuClient(env, '1.234-76', None)
                     dump_env_logs(client, '10.10.0.1', artifacts_dir)
             self.assertEqual(
                 ['machine-0', 'machine-1', 'machine-2'],
@@ -244,75 +249,73 @@
             (client, '10.10.0.1'), gm_mock.call_args[0])
         call_list = sorted((cal[0], cal[1]) for cal in dl_mock.call_args_list)
         self.assertEqual(
-            [((client, '10.10.0.1', '%s/machine-0' % artifacts_dir),
-              {'local_state_server': False}),
-             ((client, '10.10.0.11', '%s/machine-1' % artifacts_dir),
-              {'local_state_server': False}),
-             ((client, '10.10.0.22', '%s/machine-2' % artifacts_dir),
+            [((env, self.r0, '%s/machine-0' % artifacts_dir),
+              {'local_state_server': False}),
+             ((env, self.r1, '%s/machine-1' % artifacts_dir),
+              {'local_state_server': False}),
+             ((env, self.r2, '%s/machine-2' % artifacts_dir),
               {'local_state_server': False})],
             call_list)
         self.assertEqual(
-            ['INFO Retrieving logs for machine-0',
-             'INFO Retrieving logs for machine-1',
-             'INFO Retrieving logs for machine-2'],
+            ['INFO Retrieving logs for machine-0 using ' + repr(self.r0),
+             'INFO Retrieving logs for machine-1 using ' + repr(self.r1),
+             'INFO Retrieving logs for machine-2 using ' + repr(self.r2)],
             sorted(self.log_stream.getvalue().splitlines()))
 
     def test_dump_env_logs_local_env(self):
         with temp_dir() as artifacts_dir:
-            with patch('deploy_stack.get_machines_for_logs', autospec=True,
-                       return_value=get_machine_addresses()):
+            with patch('deploy_stack.get_remote_machines', autospec=True,
+                       return_value=self.fake_remote_machines()):
                 with patch('deploy_stack.dump_logs', autospec=True) as dl_mock:
-                    client = EnvJujuClient(
-                        SimpleEnvironment(
-                            'foo', {'type': 'local'}), '1.234-76', None)
+                    env = SimpleEnvironment('foo', {'type': 'local'})
+                    client = EnvJujuClient(env, '1.234-76', None)
                     dump_env_logs(client, '10.10.0.1', artifacts_dir)
         call_list = sorted((cal[0], cal[1]) for cal in dl_mock.call_args_list)
         self.assertEqual(
-            [((client, '10.10.0.1', '%s/machine-0' % artifacts_dir),
+            [((env, self.r0, '%s/machine-0' % artifacts_dir),
               {'local_state_server': True}),
-             ((client, '10.10.0.11', '%s/machine-1' % artifacts_dir),
+             ((env, self.r1, '%s/machine-1' % artifacts_dir),
               {'local_state_server': False}),
-             ((client, '10.10.0.22', '%s/machine-2' % artifacts_dir),
+             ((env, self.r2, '%s/machine-2' % artifacts_dir),
               {'local_state_server': False})],
             call_list)
 
     def test_dump_logs_with_local_state_server_false(self):
         # copy_remote_logs is called for non-local envs.
-        client = EnvJujuClient(
-            SimpleEnvironment('foo', {'type': 'nonlocal'}), '1.234-76', None)
+        env = SimpleEnvironment('foo', {'type': 'nonlocal'})
+        remote = object()
         with temp_dir() as log_dir:
             with patch('deploy_stack.copy_local_logs',
                        autospec=True) as cll_mock:
                 with patch('deploy_stack.copy_remote_logs', autospec=True,
                            side_effect=make_logs(log_dir)) as crl_mock:
-                    dump_logs(client, '10.10.0.1', log_dir,
+                    dump_logs(env, remote, log_dir,
                               local_state_server=False)
             self.assertEqual(['cloud.log.gz', 'extra'],
                              sorted(os.listdir(log_dir)))
         self.assertEqual(0, cll_mock.call_count)
-        self.assertEqual(('10.10.0.1', log_dir), crl_mock.call_args[0])
+        self.assertEqual((remote, log_dir), crl_mock.call_args[0])
 
     def test_dump_logs_with_local_state_server_true(self):
         # copy_local_logs is called for machine 0 in a local env.
-        client = EnvJujuClient(
-            SimpleEnvironment('foo', {'type': 'local'}), '1.234-76', None)
+        env = SimpleEnvironment('foo', {'type': 'local'})
+        remote = object()
         with temp_dir() as log_dir:
             with patch('deploy_stack.copy_local_logs', autospec=True,
                        side_effect=make_logs(log_dir)) as cll_mock:
                 with patch('deploy_stack.copy_remote_logs',
                            autospec=True) as crl_mock:
-                    dump_logs(client, '10.10.0.1', log_dir,
+                    dump_logs(env, remote, log_dir,
                               local_state_server=True)
             self.assertEqual(['cloud.log.gz', 'extra'],
                              sorted(os.listdir(log_dir)))
-        self.assertEqual((log_dir, client), cll_mock.call_args[0])
+        self.assertEqual((env, log_dir), cll_mock.call_args[0])
         self.assertEqual(0, crl_mock.call_count)
 
     def test_copy_local_logs(self):
         # Relevent local log files are copied, after changing their permissions
         # to allow access by non-root user.
-        client = EnvJujuClient(
-            SimpleEnvironment('a-local', {'type': 'local'}), '1.234-76', None)
+        env = SimpleEnvironment('a-local', {'type': 'local'})
         with temp_dir() as juju_home_dir:
             log_dir = os.path.join(juju_home_dir, "a-local", "log")
             os.makedirs(log_dir)
@@ -325,7 +328,7 @@
                 with patch('deploy_stack.lxc_template_glob',
                            os.path.join(template_dir, "*.log")):
                     with patch('subprocess.check_call') as cc_mock:
-                        copy_local_logs('/destination/dir', client)
+                        copy_local_logs(env, '/destination/dir')
         expected_files = [os.path.join(juju_home_dir, *p) for p in (
             ('a-local', 'cloud-init-output.log'),
             ('a-local', 'log', 'all-machines.log'),
@@ -342,7 +345,7 @@
         # to ensure errors do not prevent some logs from being retrieved.
         with patch('deploy_stack.wait_for_port', autospec=True):
             with patch('subprocess.check_output') as cc_mock:
-                copy_remote_logs('10.10.0.1', '/foo')
+                copy_remote_logs(remote_from_address('10.10.0.1'), '/foo')
         self.assertEqual(
             (['timeout', '5m', 'ssh',
               '-o', 'User ubuntu',
@@ -361,6 +364,16 @@
               '/foo'],),
             cc_mock.call_args_list[1][0])
 
+    def test_copy_remote_logs_windows(self):
+        remote = remote_from_address('10.10.0.1', series="win2012hvr2")
+        with patch.object(remote, "copy", autospec=True) as copy_mock:
+            copy_remote_logs(remote, '/foo')
+        paths = [
+            "%ProgramFiles(x86)%\\Cloudbase Solutions\\Cloudbase-Init\\log\\*",
+            "C:\\Juju\\log\\juju\\*.log",
+        ]
+        copy_mock.assert_called_once_with("/foo", paths)
+
     def test_copy_remote_logs_with_errors(self):
         # Ssh and scp errors will happen when /var/log/juju doesn't exist yet,
         # but we log the case anc continue to retrieve as much as we can.
@@ -372,10 +385,10 @@
 
         with patch('subprocess.check_output', side_effect=remote_op) as co:
             with patch('deploy_stack.wait_for_port', autospec=True):
-                copy_remote_logs('10.10.0.1', '/foo')
+                copy_remote_logs(remote_from_address('10.10.0.1'), '/foo')
         self.assertEqual(2, co.call_count)
         self.assertEqual(
-            ['WARNING Could not change the permission of the juju logs:',
+            ['WARNING Could not allow access to the juju logs:',
              'WARNING None',
              'WARNING Could not retrieve some or all logs:',
              'WARNING None'],
@@ -393,9 +406,9 @@
             """)
         with patch.object(client, 'get_status', autospec=True,
                           return_value=status):
-            machine_addrs = get_machines_for_logs(client, None)
-        self.assertEqual(
-            {'0': '10.11.12.13', '1': '10.11.12.14'}, machine_addrs)
+            machines = get_remote_machines(client, None)
+        self.assert_machines(
+            {'0': '10.11.12.13', '1': '10.11.12.14'}, machines)
 
     def test_get_machines_for_logs_with_boostrap_host(self):
         client = EnvJujuClient(
@@ -403,23 +416,23 @@
         status = Status.from_text("""\
             machines:
               "0":
-                dns-name: 10.11.12.13
+                instance-id: pending
             """)
         with patch.object(client, 'get_status', autospec=True,
                           return_value=status):
-            machine_addrs = get_machines_for_logs(client, '10.11.111.222')
-        self.assertEqual({'0': '10.11.111.222'}, machine_addrs)
+            machines = get_remote_machines(client, '10.11.111.222')
+        self.assert_machines({'0': '10.11.111.222'}, machines)
 
     def test_get_machines_for_logs_with_no_addresses(self):
         client = EnvJujuClient(
             SimpleEnvironment('cloud', {'type': 'ec2'}), '1.23.4', None)
         with patch.object(client, 'get_status', autospec=True,
                           side_effect=Exception):
-            machine_addrs = get_machines_for_logs(client, '10.11.111.222')
-        self.assertEqual({'0': '10.11.111.222'}, machine_addrs)
+            machines = get_remote_machines(client, '10.11.111.222')
+        self.assert_machines({'0': '10.11.111.222'}, machines)
 
     @patch('subprocess.check_call')
-    def test_get_machines_for_logs_with_maas(self, cc_mock):
+    def test_get_remote_machines_with_maas(self, cc_mock):
         config = {
             'type': 'maas',
             'name': 'foo',
@@ -442,25 +455,46 @@
             }
             with patch('deploy_stack.MAASAccount.get_allocated_ips',
                        autospec=True, return_value=allocated_ips):
-                machine_addrs = get_machines_for_logs(client, 'node1.maas')
-        self.assertEqual(
-            {'0': '10.11.12.13', '1': '10.11.12.14'}, machine_addrs)
-
-    def test_get_machine_addrs(self):
-        client = EnvJujuClient(
-            SimpleEnvironment('cloud', {'type': 'ec2'}), '1.23.4', None)
-        status = Status.from_text("""\
-            machines:
-              "0":
-                dns-name: 10.11.12.13
-              "1":
-                dns-name: 10.11.12.14
-            """)
-        with patch.object(client, 'get_status', autospec=True,
-                          return_value=status):
-            machine_addrs = [ma for ma in get_machine_addrs(client)]
-        self.assertEqual(
-            [('0', '10.11.12.13'), ('1', '10.11.12.14')], machine_addrs)
+                machines = get_remote_machines(client, 'node1.maas')
+        self.assert_machines(
+            {'0': '10.11.12.13', '1': '10.11.12.14'}, machines)
+
+    def test_iter_remote_machines(self):
+        client = EnvJujuClient(
+            SimpleEnvironment('cloud', {'type': 'ec2'}), '1.23.4', None)
+        status = Status.from_text("""\
+            machines:
+              "0":
+                dns-name: 10.11.12.13
+              "1":
+                dns-name: 10.11.12.14
+            """)
+        with patch.object(client, 'get_status', autospec=True,
+                          return_value=status):
+            machines = [(m, r.address)
+                        for m, r in iter_remote_machines(client)]
+        self.assertEqual(
+            [('0', '10.11.12.13'), ('1', '10.11.12.14')], machines)
+
+    def test_iter_remote_machines_with_series(self):
+        client = EnvJujuClient(
+            SimpleEnvironment('cloud', {'type': 'ec2'}), '1.23.4', None)
+        status = Status.from_text("""\
+            machines:
+              "0":
+                dns-name: 10.11.12.13
+                series: trusty
+              "1":
+                dns-name: 10.11.12.14
+                series: win2012hvr2
+            """)
+        with patch.object(client, 'get_status', autospec=True,
+                          return_value=status):
+            machines = [(m, r.address, r.series)
+                        for m, r in iter_remote_machines(client)]
+        self.assertEqual(
+            [('0', '10.11.12.13', 'trusty'),
+             ('1', '10.11.12.14', 'win2012hvr2')], machines)
 
     def test_retain_jenv(self):
         with temp_dir() as jenv_dir:
@@ -485,7 +519,8 @@
         setup_test_logging(self)
 
     def test_deploy_dummy_stack(self):
-        client = EnvJujuClient(SimpleEnvironment('foo', {}), None, '/foo/juju')
+        env = SimpleEnvironment('foo', {'type': 'nonlocal'})
+        client = EnvJujuClient(env, None, '/foo/juju')
         status = yaml.safe_dump({
             'machines': {'0': {'agent-state': 'started'}},
             'services': {

=== modified file 'test_remote.py'
--- test_remote.py	2015-06-20 17:32:27 +0000
+++ test_remote.py	2015-07-02 00:56:56 +0000
@@ -2,6 +2,7 @@
 
 import logging
 from mock import patch
+import os
 from StringIO import StringIO
 import subprocess
 import unittest
@@ -16,6 +17,10 @@
 from remote import (
     remote_from_address,
     remote_from_unit,
+    WinRmRemote,
+)
+from utility import (
+    temp_dir,
 )
 
 
@@ -198,6 +203,27 @@
             "/local/path",
         ])
 
+    def test_copy_on_windows(self):
+        env = SimpleEnvironment("an-env", {"type": "nonlocal"})
+        client = EnvJujuClient(env, None, None)
+        unit = "a-service/0"
+        dest = "/local/path"
+        with patch.object(client, "get_status", autospec=True) as st:
+            st.return_value = Status.from_text(self.win2012hvr2_status_output)
+            response = winrm.Response(("fake output", "",  0))
+            remote = remote_from_unit(client, unit)
+            with patch.object(remote.session, "run_ps", autospec=True,
+                              return_value=response) as mock_run:
+                with patch.object(remote, "_encoded_copy_to_dir",
+                                  autospec=True) as mock_cpdir:
+                    remote.copy(dest, ["C:\\logs\\*", "%APPDATA%\\*.log"])
+        mock_cpdir.assert_called_once_with(dest, "fake output")
+        st.assert_called_once_with()
+        self.assertEquals(mock_run.call_count, 1)
+        self.assertRegexpMatches(
+            mock_run.call_args[0][0],
+            r'.*"C:\\logs\\[*]","%APPDATA%\\[*].log".*')
+
     def test_run_cmd(self):
         env = SimpleEnvironment("an-env", {"type": "nonlocal"})
         client = EnvJujuClient(env, None, None)
@@ -214,3 +240,24 @@
         st.assert_called_once_with()
         mock_run.assert_called_once_with(
             '"C:\\Program Files\\bin.exe"', ['/IN "Bob\'s Stuff"'])
+
+    def test_encoded_copy_to_dir_one(self):
+        output = "testfile|K0ktLuECAA==\r\n"
+        with temp_dir() as dest:
+            WinRmRemote._encoded_copy_to_dir(dest, output)
+            with open(os.path.join(dest, "testfile")) as f:
+                self.assertEqual(f.read(), "test\n")
+
+    def test_encoded_copy_to_dir_many(self):
+        output = "test one|K0ktLuECAA==\r\ntest two|K0ktLuECAA==\r\n\r\n"
+        with temp_dir() as dest:
+            WinRmRemote._encoded_copy_to_dir(dest, output)
+            for name in ("test one", "test two"):
+                with open(os.path.join(dest, name)) as f:
+                    self.assertEqual(f.read(), "test\n")
+
+    def test_encoded_copy_traversal_guard(self):
+        output = "../../../etc/passwd|K0ktLuECAA==\r\n"
+        with temp_dir() as dest:
+            with self.assertRaises(ValueError):
+                WinRmRemote._encoded_copy_to_dir(dest, output)