Merge lp:~frankban/lpsetup/use-lxcip into lp:lpsetup

[Francesco Banconi]

== Changes ==

lpsetup no longer changes resolv.conf and dhcp files.
Each ssh connection is made using the container's ip address, obtained from *lp-lxc-ip*.

Changed the lxc template filename, and how the bridge interface is retrieved: now the script looks for interface existence: lxcbr0 first, virbr0 as a fallback, error otherwise (see get_lxc_gateway).

Fixed the script error handling: now lpsetup exists with the expected return code.

Removed the `wait_for_lxc` step: each ssh call systematically retries to obtain the ip address and establish the connection, for ~30 seconds.

== Tests ==

$ nosetests
........................................................
Name Stmts Miss Cover Missing
---------------------------------------------------
lpsetup 6 1 83% 16
lpsetup.argparser 125 6 95% 113, 221, 278-279, 298, 307
lpsetup.exceptions 6 0 100%
lpsetup.handlers 55 1 98% 55
lpsetup.settings 29 0 100%
lpsetup.subcommands 0 0 100%
lpsetup.utils 125 28 78% 80, 114-124, 132-133, 148, 199, 209, 230-232, 250-256, 275-277
---------------------------------------------------
TOTAL 346 36 90%
----------------------------------------------------------------------
Ran 56 tests in 0.579s

OK

[Gary Poster (gary)]

Another great branch, frankban.

A summary of our IRC conversation follows. I requested some small changes.

 * This is a great improvement to simplicity and robustness.
 * Let's not catch MemoryError in cli.main(): the traceback might in fact be a valuable diagnostic.
 * I don't think we should retry sshlxc, because we only want to retry the lxcip call and verify that the sshd is up. Once the sshd is up, we should not retry a failed command. It may have had an effect before it blew up, and there are no guarantees that a retry is appropriate. Instead, we probably should retry the lxcip call, and use wait_for_lxc to verify that the sshd is ready before we try the command (once).

Thank you!

review: Approve

[lp:~frankban/lpsetup/use-lxcip]

25. By Francesco Banconi on 2012-05-02

Changes from review.

=== modified file 'lp-setup'
--- lp-setup	2012-03-12 10:11:31 +0000
+++ lp-setup	2012-05-02 16:37:17 +0000
@@ -4,8 +4,10 @@
 
 """Command line interface entry point for lpsetup."""
 
+import sys
+
 from lpsetup import cli
 
 
 if __name__ == '__main__':
-    cli.main()
+    sys.exit(cli.main())

=== modified file 'lpsetup/cli.py'
--- lpsetup/cli.py	2012-03-12 10:11:31 +0000
+++ lpsetup/cli.py	2012-05-02 16:37:17 +0000
@@ -12,6 +12,7 @@
 from lpsetup import (
     __doc__ as description,
     argparser,
+    exceptions,
     )
 from lpsetup.subcommands import (
     install,
@@ -26,4 +27,7 @@
     parser.register_subcommand('update', update.SubCommand)
     parser.register_subcommand('lxc-install', lxcinstall.SubCommand)
     args = parser.parse_args()
-    return args.main(args)
+    try:
+        return args.main(args)
+    except (exceptions.ExecutionError, KeyboardInterrupt) as err:
+        return err

=== modified file 'lpsetup/exceptions.py'
--- lpsetup/exceptions.py	2012-03-09 10:14:32 +0000
+++ lpsetup/exceptions.py	2012-05-02 16:37:17 +0000
@@ -6,14 +6,18 @@
 
 __metaclass__ = type
 __all__ = [
-    'LaunchpadError',
+    'LPSetupError',
     'ValidationError',
     ]
 
 
-class LaunchpadError(Exception):
+class LPSetupError(Exception):
     """Base exception for lpsetup."""
 
 
-class ValidationError(LaunchpadError):
+class ValidationError(LPSetupError):
     """Argparse invalid arguments."""
+
+
+class ExecutionError(LPSetupError):
+    """Error occurring during script execution."""

=== modified file 'lpsetup/settings.py'
--- lpsetup/settings.py	2012-04-20 13:49:21 +0000
+++ lpsetup/settings.py	2012-05-02 16:37:17 +0000
@@ -15,7 +15,6 @@
 BASE_PACKAGES = ['ssh', 'bzr']
 CHECKOUT_DIR = '~/launchpad/branches'
 DEPENDENCIES_DIR = '~/launchpad/dependencies'
-DHCP_FILE = '/etc/dhcp/dhclient.conf'
 HOSTS_CONTENT = (
     ('127.0.0.88',
         'launchpad.dev answers.launchpad.dev archive.launchpad.dev '
@@ -57,10 +56,11 @@
 LP_SOURCE_DEPS = (
     'http://bazaar.launchpad.net/~launchpad/lp-source-dependencies/trunk')
 LPSETUP_PPA = 'ppa:yellow/ppa'
-LXC_CONFIG_TEMPLATE = '/etc/lxc/local.conf'
+LXC_CONFIG_TEMPLATE = '/etc/lxc/lp-setup.conf'
 LXC_GUEST_ARCH = 'i386'
 LXC_GUEST_CHOICES = ('lucid', 'oneiric', 'precise')
 LXC_GUEST_OS = LXC_GUEST_CHOICES[0]
+LXC_IP_COMMAND = ('/usr/bin/lp-lxc-ip', '-s', '-i', 'eth0', '-n')
 LXC_LEASES = (
     '/var/lib/dhcp3/dhclient.eth0.leases',
     '/var/lib/dhcp/dhclient.eth0.leases',
@@ -75,6 +75,5 @@
 """
 LXC_PACKAGES = ('lxc', 'libvirt-bin')
 LXC_PATH = '/var/lib/lxc/'
-RESOLV_FILE = '/etc/resolv.conf'
 SCRIPTS = ('lp-setup-lxc-build', 'lp-setup-lxc-cleanup', 'lp-setup-lxc-test')
 SSH_KEY_NAME = 'id_rsa'

=== modified file 'lpsetup/subcommands/lxcinstall.py'
--- lpsetup/subcommands/lxcinstall.py	2012-04-20 14:37:23 +0000
+++ lpsetup/subcommands/lxcinstall.py	2012-05-02 16:37:17 +0000
@@ -13,26 +13,24 @@
     'setup_launchpad_lxc',
     'start_lxc',
     'stop_lxc',
-    'wait_for_lxc',
     ]
 
 import os
 import shutil
 import subprocess
-import time
 
 from shelltoolbox import (
     apt_get_install,
     file_append,
-    file_prepend,
     mkdirs,
-    ssh,
     )
 
-from lpsetup import handlers
+from lpsetup import (
+    exceptions,
+    handlers,
+    )
 from lpsetup.settings import (
     BASE_PACKAGES,
-    DHCP_FILE,
     LPSETUP_PPA,
     LXC_CONFIG_TEMPLATE,
     LXC_GUEST_ARCH,
@@ -42,7 +40,6 @@
     LXC_NAME,
     LXC_OPTIONS,
     LXC_PACKAGES,
-    RESOLV_FILE,
     SCRIPTS,
     )
 from lpsetup.subcommands import install
@@ -52,6 +49,8 @@
     get_lxc_gateway,
     lxc_stopped,
     render_to_file,
+    retry,
+    sshlxc as ssh,
     this_command,
     )
 
@@ -111,15 +110,12 @@
     call('ln', '-s',
          '/etc/apparmor.d/usr.bin.lxc-start', '/etc/apparmor.d/disable/')
     call('apparmor_parser', '-R', '/etc/apparmor.d/usr.bin.lxc-start')
-    # Update resolv file in order to get the ability to ssh into the LXC
-    # container using its name.
-    lxc_gateway_name, lxc_gateway_address = get_lxc_gateway()
-    file_prepend(RESOLV_FILE, 'nameserver {0}\n'.format(lxc_gateway_address))
-    file_append(
-        DHCP_FILE,
-        'prepend domain-name-servers {0};\n'.format(lxc_gateway_address))
     # Container configuration template.
-    content = LXC_OPTIONS.format(interface=lxc_gateway_name)
+    lxc_gateway = get_lxc_gateway()
+    if lxc_gateway is None:
+        raise exceptions.ExecutionError(
+            'Error: LXC bridge interface not found.')
+    content = LXC_OPTIONS.format(interface=lxc_gateway)
     with open(LXC_CONFIG_TEMPLATE, 'w') as f:
         f.write(content)
     # Creating container.
@@ -152,37 +148,31 @@
     call('lxc-start', '-n', lxc_name, '-d')
 
 
-def wait_for_lxc(lxc_name, ssh_key_path, trials=60, sleep_seconds=1):
-    """Try to ssh as `user` into the LXC container named `lxc_name`."""
-    sshcall = ssh(lxc_name, key=ssh_key_path)
-    while True:
-        trials -= 1
-        try:
-            sshcall('true')
-        except subprocess.CalledProcessError:
-            if not trials:
-                raise
-            time.sleep(sleep_seconds)
-        else:
-            break
+def wait_for_lxc(lxc_name, ssh_key_path):
+    """Try to ssh into the LXC container named `lxc_name`."""
+    retry_ssh = retry(subprocess.CalledProcessError)(ssh)
+    retry_ssh(lxc_name, 'true', key=ssh_key_path)
 
 
 def initialize_lxc(lxc_name, ssh_key_path, lxc_os):
     """Initialize LXC container."""
     base_packages = list(BASE_PACKAGES) + ['python-software-properties']
-    sshcall = ssh(lxc_name, key=ssh_key_path)
-    sshcall(
+    ssh(lxc_name,
         'DEBIAN_FRONTEND=noninteractive '
-        'apt-get install -y ' + ' '.join(base_packages))
+        'apt-get install -y ' + ' '.join(base_packages),
+        key=ssh_key_path)
     args = {
         'assume_yes': '' if lxc_os == 'lucid' else '-y',
         'repository': LPSETUP_PPA,
         }
-    sshcall('apt-add-repository {assume_yes} {repository}'.format(**args))
-    sshcall('apt-get clean && apt-get update')
-    sshcall(
+    ssh(lxc_name,
+        'apt-add-repository {assume_yes} {repository}'.format(**args),
+        key=ssh_key_path)
+    ssh(lxc_name, 'apt-get clean && apt-get update', key=ssh_key_path)
+    ssh(lxc_name,
         'DEBIAN_FRONTEND=noninteractive '
-        'apt-get upgrade -y && apt-get install -y lpsetup')
+        'apt-get upgrade -y && apt-get install -y lpsetup',
+        key=ssh_key_path)
 
 
 def setup_launchpad_lxc(
@@ -194,12 +184,12 @@
         '-d', dependencies_dir, '-c', directory
         ]
     cmd = this_command(directory, args)
-    ssh(lxc_name, key=ssh_key_path)(cmd)
+    ssh(lxc_name, cmd, key=ssh_key_path)
 
 
 def stop_lxc(lxc_name, ssh_key_path):
     """Stop the lxc instance named `lxc_name`."""
-    ssh(lxc_name, key=ssh_key_path)('poweroff')
+    ssh(lxc_name, 'poweroff', key=ssh_key_path)
     if not lxc_stopped(lxc_name):
         subprocess.call(['lxc-stop', '-n', lxc_name])
 

=== modified file 'lpsetup/tests/test_utils.py'
--- lpsetup/tests/test_utils.py	2012-04-11 10:40:05 +0000
+++ lpsetup/tests/test_utils.py	2012-05-02 16:37:17 +0000
@@ -11,8 +11,6 @@
 import tempfile
 import unittest
 
-from shelltoolbox import run
-
 from lpsetup.settings import (
     LXC_LP_DIR_PATTERN,
     LXC_LP_TEST_DIR_PATTERN,
@@ -20,8 +18,11 @@
     )
 from lpsetup.utils import (
     get_container_path,
+    get_lxc_gateway,
+    get_network_interfaces,
     get_running_containers,
     render_to_file,
+    retry,
     Scrubber,
     this_command,
     )
@@ -45,6 +46,55 @@
             get_container_path('mycontainer', 'home'))
 
 
+class GetLXCGatewayTest(unittest.TestCase):
+
+    interfaces = ('eth0', 'virbr0', 'lxcbr0', 'lo')
+
+    def test_found(self):
+        # Ensure the first gateway found is correctly returned.
+        gw = get_lxc_gateway(interface_lookup=lambda: self.interfaces)
+        self.assertEqual(self.interfaces[2], gw)
+
+    def test_fallback(self):
+        # If the first interface can not be found, a fallback is returned.
+        gw = get_lxc_gateway(interface_lookup=lambda: self.interfaces[:2])
+        self.assertEqual(self.interfaces[1], gw)
+
+    def test_not_found(self):
+        # Ensure the function returns None if the gateway can not be found.
+        gw = get_lxc_gateway(interface_lookup=lambda: self.interfaces[:1])
+        self.assertIsNone(gw)
+
+
+class GetNetworkInterfacesTest(unittest.TestCase):
+
+    interfaces = ('eth0', 'eth1', 'lo')
+
+    def create_path(self, interfaces):
+        path = tempfile.mkdtemp()
+        for interface in interfaces:
+            os.mkdir(os.path.join(path, interface))
+        return path
+
+    def test_all(self):
+        # Ensure all interfaces are correctly returned.
+        path = self.create_path(self.interfaces)
+        interfaces = get_network_interfaces(path=path)
+        self.assertItemsEqual(self.interfaces, interfaces)
+
+    def test_exclude(self):
+        # Ensure interfaces can be excluded from the returned list.
+        path = self.create_path(self.interfaces)
+        interfaces = get_network_interfaces(path=path, exclude=['lo'])
+        self.assertItemsEqual(self.interfaces[:-1], interfaces)
+
+    def test_none(self):
+        # An empty list is returned if interfaces are not found.
+        path = self.create_path([])
+        interfaces = get_network_interfaces(path=path, exclude=['lo'])
+        self.assertEqual([], interfaces)
+
+
 class GetRunningContainersTest(unittest.TestCase):
 
     def assertRunning(self, expected, containers):
@@ -86,7 +136,38 @@
             self.assertEqual(expected, f.read())
 
 
-class TestScrubber(unittest.TestCase):
+class RetryTest(unittest.TestCase):
+
+    error = 'error after {} tries'
+
+    def setUp(self):
+        self.tries = 0
+
+    @retry(OSError, tries=10, delay=0)
+    def _success(self):
+        self.tries += 1
+        if self.tries == 5:
+            return True
+        raise OSError
+
+    @retry(OSError, tries=10, delay=0)
+    def _failure(self):
+        self.tries += 1
+        raise OSError(self.error.format(self.tries))
+
+    def test_success(self):
+        # Ensure the decorated function correctly returns without errors
+        # after several tries.
+        self.assertTrue(self._success())
+
+    def test_failure(self):
+        # Ensure the decorated function raises the last error.
+        with self.assertRaises(OSError) as cm:
+            self._failure()
+        self.assertEqual(self.error.format(10), str(cm.exception))
+
+
+class ScrubberTest(unittest.TestCase):
 
     def make_dirs(self, tmp_path, pattern, num=10, extra=None):
         dir_ = os.path.split(pattern)[0]

=== modified file 'lpsetup/utils.py'
--- lpsetup/utils.py	2012-04-11 10:40:05 +0000
+++ lpsetup/utils.py	2012-05-02 16:37:17 +0000
@@ -9,18 +9,24 @@
     'call',
     'get_container_path',
     'get_lxc_gateway',
+    'get_network_interfaces',
+    'get_running_containers',
     'lxc_in_state',
-    'lxc_running',
+    'lxc_ip',
     'lxc_stopped',
     'render_to_file',
+    'retry',
     'Scrubber',
+    'sshlxc',
     'this_command',
     ]
 
-from functools import partial
+from functools import (
+    partial,
+    wraps,
+    )
 import glob
 import os
-import platform
 import re
 import subprocess
 import shutil
@@ -28,18 +34,24 @@
 import time
 
 from shelltoolbox import (
+    command,
     join_command,
     run,
+    ssh,
     )
 
+from lpsetup import exceptions
 from lpsetup.settings import (
     LXC_LP_DIR_PATTERN,
     LXC_LP_TEST_DIR_PATTERN,
+    LXC_IP_COMMAND,
     LXC_PATH,
     )
 
 
 PID_RE = re.compile("pid:\s+(\d+)")
+
+
 call = partial(run, stdout=None)
 
 
@@ -58,17 +70,29 @@
     return os.path.join(base_path, lxc_name, 'rootfs', path.lstrip('/'))
 
 
-def get_lxc_gateway():
-    """Return a tuple of gateway name and address.
+def get_lxc_gateway(candidates=('lxcbr0', 'virbr0'), interface_lookup=None):
+    """Return the gateway bridge name to be used by LXC containers.
 
-    The gateway name and address will change depending on which version
-    of Ubuntu the script is running on.
+    The gateway name will change depending on which version of Ubuntu the
+    script is running on: if *lxcbr0* does not exist, *virbr0* will be used.
+    If both interfaces don't exist, None is returned.
     """
-    release_name = platform.linux_distribution()[2]
-    if release_name == 'oneiric':
-        return 'virbr0', '192.168.122.1'
+    if interface_lookup is None:
+        interfaces = get_network_interfaces()
     else:
-        return 'lxcbr0', '10.0.3.1'
+        interfaces = interface_lookup()
+    for interface in candidates:
+        if interface in interfaces:
+            return interface
+
+
+def get_network_interfaces(path='/sys/class/net/', exclude=()):
+    """Return a list of the network interfaces up in this system."""
+    return [
+        i for i in os.listdir(path)
+        if i not in exclude and
+        os.path.isdir(os.path.join(path, i))
+        ]
 
 
 def get_running_containers(containers=None):
@@ -101,7 +125,6 @@
     return False
 
 
-lxc_running = partial(lxc_in_state, 'RUNNING')
 lxc_stopped = partial(lxc_in_state, 'STOPPED')
 
 
@@ -123,6 +146,29 @@
     return contents
 
 
+def retry(exception, tries=60, delay=0.5):
+    """If the decorated function raises `exception`, wait and try it again.
+
+    Raise the exception raised by the last call if the function does not
+    exit normally after 100 tries.
+
+    Original from http://wiki.python.org/moin/PythonDecoratorLibrary#Retry.
+    """
+    def decorator(func):
+        @wraps(func)
+        def decorated(*args, **kwargs):
+            mtries = tries
+            while mtries:
+                try:
+                    return func(*args, **kwargs)
+                except exception as err:
+                    time.sleep(delay)
+                    mtries -= 1
+            raise err
+        return decorated
+    return decorator
+
+
 class Scrubber(object):
     """Scrubber will cleanup after lxc ephemeral uncleanliness.
 
@@ -207,6 +253,35 @@
         self.scrub(self.lxc_lp_dir_pattern)
 
 
+@retry(subprocess.CalledProcessError)
+def lxc_ip(name):
+    """Return the ip address of the LXC container called `name`.
+
+    This function will try to obtain the ip address of the container for
+    ~30 seconds before raising a `subprocess.CalledProcessError`.
+    """
+    cmd = command(*LXC_IP_COMMAND)
+    return cmd(name).strip()
+
+
+def sshlxc(name, cmd, **kwargs):
+    """Run ssh shell commands.
+
+    The ssh connection is made resolving the ip address of the LXC
+    container called `name`.
+
+    Raise `lpsetup.exceptions.ExecutionError` if the ip address is not found.
+    Raise `subprocess.CalledProcessError` if the ssh command returns an error.
+    """
+    try:
+        ip = lxc_ip(name)
+    except subprocess.CalledProcessError as err:
+        msg = 'Error: unable to find the ip address of the LXC container: '
+        raise exceptions.ExecutionError(msg + str(err))
+    sshcall = ssh(ip, **kwargs)
+    return sshcall(cmd)
+
+
 def this_command(directory, args):
     """Return a command line to re-launch this script using given `args`.