Merge lp:~hazmat/pyjuju/local-respect-series into lp:pyjuju

[Kapil Thangavelu]

Modify local provider to respect the environment's default-series

Fixes issues with creating precise containers.

[Benjamin Saller (bcsaller)]

[1] There are now a few places that precise and oneiric are included in the code. It might be worth it to make these imported constants from a single file CURRENT_UBUNTU_LTS_SERIES="", CURRENT_UBUNTU_SERIES="" and use those imported symbols through the codebase.

[2] You assert juju series in machine unit.
    [2.1] No validation that the series name is valid is preformed, though this might be YAGNI
    [2.2] It seems series is required, we might want to update the docs to reflect that a value must be provided

review: Approve

[Clint Byrum (clint-fewbar)]

Excerpts from Kapil Thangavelu's message of Wed Jan 25 18:31:29 UTC 2012:
> Kapil Thangavelu has proposed merging lp:~hazmat/juju/local-respect-series into lp:juju.
>
> Requested reviews:
> juju hackers (juju)
> Related bugs:
> Bug #914392 in juju: "LXC local provider does not respect 'series' (only installs oneiric)"
> https://bugs.launchpad.net/juju/+bug/914392
>
> For more details, see:
> https://code.launchpad.net/~hazmat/juju/local-respect-series/+merge/90182
>
> Modify local provider to respect the environment's default-series
>
> Fixes issues with creating precise containers.

I'm a little nervous about the fact that some functions have "oneiric"
and some "precise" as defaults.

It seems like it would be safer to *always* require the user to specify,
or at the very least, take this from the environment default-series.

[Kapil Thangavelu (hazmat)]

Excerpts from Clint Byrum's message of Thu Jan 26 01:58:23 UTC 2012:
> Excerpts from Kapil Thangavelu's message of Wed Jan 25 18:31:29 UTC 2012:
> > Kapil Thangavelu has proposed merging lp:~hazmat/juju/local-respect-series into lp:juju.
> >
> > Requested reviews:
> > juju hackers (juju)
> > Related bugs:
> > Bug #914392 in juju: "LXC local provider does not respect 'series' (only installs oneiric)"
> > https://bugs.launchpad.net/juju/+bug/914392
> >
> > For more details, see:
> > https://code.launchpad.net/~hazmat/juju/local-respect-series/+merge/90182
> >
> > Modify local provider to respect the environment's default-series
> >
> > Fixes issues with creating precise containers.
>
> I'm a little nervous about the fact that some functions have "oneiric"
> and some "precise" as defaults.
>
> It seems like it would be safer to *always* require the user to specify,
> or at the very least, take this from the environment default-series.
>

The value is required and always passed, but there are some defaults in place
which where just to ease impl. I'll remove those and make it required
throughout. Most of the precise/oneiric values are in place for testing which
would remain regardless.

-k

[Jim Baker (jimbaker)]

+1, looks good to me. The only thing I found problematic was this signature:

=== modified file 'juju/lib/lxc/__init__.py'
--- juju/lib/lxc/__init__.py 2011-10-04 23:38:52 +0000
+++ juju/lib/lxc/__init__.py 2012-02-03 00:27:50 +0000
@@ -154,7 +154,8 @@
                  customize_script=None,
                  debug_log=None,
                  console_log=None,
- customize_log=None):
+ customize_log=None,
+ series="oneiric"):

But according to the earlier comments, you are planning to make this arg required for non-tests anyway.

review: Approve

[lp:~hazmat/pyjuju/local-respect-series]

451. By Kapil Thangavelu on 2012-02-11

remove series default values inline, series is required

=== modified file 'juju/lib/lxc/__init__.py'
--- juju/lib/lxc/__init__.py	2011-10-04 23:38:52 +0000
+++ juju/lib/lxc/__init__.py	2012-02-11 15:52:20 +0000
@@ -148,6 +148,7 @@
     def __init__(self,
                  container_name,
                  public_key,
+                 series,
                  origin,
                  origin_source=None,
                  network_name="virbr0",
@@ -161,6 +162,8 @@
 
         :param public_key: SSH public key
 
+        :param series: distro release series (oneiric, precise, etc)
+
         :param origin: distro|ppa|branch
 
         :param origin_source: when origin is branch supply a valid bzr branch
@@ -186,6 +189,7 @@
         self.public_key = public_key
         self.origin = origin
         self.source = origin_source
+        self.series = series
         self.network_name = network_name
 
     @property
@@ -260,7 +264,10 @@
         """Create the container synchronously."""
         if not self.is_constructed():
             lxc_config = self._make_lxc_config(self.network_name)
-            _lxc_create(self.container_name, config_file=lxc_config)
+            _lxc_create(
+                self.container_name,
+                config_file=lxc_config,
+                release=self.series)
             os.unlink(lxc_config)
             self._customize_container()
 
@@ -286,7 +293,8 @@
                                  debug_log=self.debug_log,
                                  console_log=self.console_log,
                                  customize_script=self.customize_script,
-                                 network_name=self.network_name)
+                                 network_name=self.network_name,
+                                 series=self.series)
 
         if not container.is_constructed():
             _lxc_clone(self.container_name, container_name)
 
=== modified file 'juju/lib/lxc/data/juju-create'
--- juju/lib/lxc/data/juju-create	2011-10-12 18:08:30 +0000
+++ juju/lib/lxc/data/juju-create	2012-02-11 15:52:20 +0000
@@ -1,6 +1,8 @@
 #!/bin/bash
 set -x
 
+APT_OPTIONS="-o Dpkg::Options::= --force-confnew --force-yes -fuy"
+
 setup_apt_cache()
 {
 # We ask the host to run apt-cache-server by default speeding the deployment of n units
@@ -17,6 +19,9 @@
 
 setup_networking()
 {
+    # Ensure we have the resolvconf installed before configuring it.
+    apt-get install $APT_OPTIONS resolvconf
+
     # Use dnsmasq to resolve names from the bridge
     # This script executes from a chroot, so directly modify the output file.
     cat <<EOF > /etc/resolvconf/run/resolv.conf
@@ -89,8 +94,6 @@
     fi
 
     export DEBIAN_FRONTEND=noninteractive
-    APT_OPTIONS="-o Dpkg::Options::= --force-confnew --force-yes -fuy"
-
     echo "Setting up juju in container"
     apt-get install $APT_OPTIONS bzr tmux sudo python-software-properties python-yaml
 
 
=== modified file 'juju/lib/lxc/tests/test_lxc.py'
--- juju/lib/lxc/tests/test_lxc.py	2011-10-01 00:04:14 +0000
+++ juju/lib/lxc/tests/test_lxc.py	2012-02-11 15:52:20 +0000
@@ -69,7 +69,6 @@
         self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
 
         _lxc_create(DEFAULT_CONTAINER, config_file=self.config)
-
         _lxc_start(DEFAULT_CONTAINER)
         _lxc_stop(DEFAULT_CONTAINER)
 
@@ -84,12 +83,15 @@
     def test_lxc_container(self):
         self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
         customize_log = self.makeFile()
-        c = LXCContainer(
-            DEFAULT_CONTAINER, "dsa...", "ppa", customize_log=customize_log)
-
+        c = LXCContainer(DEFAULT_CONTAINER,
+                         "dsa...",
+                         "precise",
+                         "ppa",
+                         customize_log=customize_log)
         running = yield c.is_running()
         self.assertFalse(running)
         self.assertFalse(c.is_constructed())
+
         # verify we can't run a non-constructed container
         failure = c.run()
         yield self.assertFailure(failure, LXCError)
@@ -107,13 +109,13 @@
         output = _lxc_ls()
         self.assertIn(DEFAULT_CONTAINER, output)
 
-        # verify we have a path into the container
+        # Verify we have a path into the container
         self.assertTrue(os.path.exists(c.rootfs))
         self.assertTrue(c.is_constructed())
 
-        self.verify_container(c, "dsa...", "ppa")
+        self.verify_container(c, "dsa...", "precise", "ppa")
 
-        # verify that we are in containers
+        # Verify that we are in containers
         containers = yield get_containers(None)
         self.assertEqual(containers[DEFAULT_CONTAINER], True)
 
@@ -158,7 +160,8 @@
 
         master_container = LXCContainer(DEFAULT_CONTAINER,
                                         origin="ppa",
-                                        public_key="dsa...")
+                                        public_key="dsa...",
+                                        series="oneiric")
 
         # verify that we cannot clone an unconstructed container
         failure = master_container.clone("test_lxc_fail")
@@ -182,7 +185,7 @@
         output = _lxc_ls()
         self.assertIn(DEFAULT_CONTAINER, output)
 
-        self.verify_container(c, "dsa...", "ppa")
+        self.verify_container(c, "dsa...", "oneiric", "ppa")
 
         # verify that we are in containers
         containers = yield get_containers(None)
@@ -202,7 +205,7 @@
 
         yield master_container.destroy()
 
-    def verify_container(self, c, public_key, origin):
+    def verify_container(self, c, public_key, series, origin):
         """Verify properties of an LXCContainer"""
 
         def p(path):
@@ -259,6 +262,11 @@
         self.assertIn('Acquire::http { Proxy "http://192.168.122.1:3142"; };',
                       apt_proxy)
 
+        # Verify the container release series.
+        with open(os.path.join(c.rootfs, "etc", "lsb-release")) as fh:
+            lsb_info = fh.read()
+        self.assertIn(series, lsb_info)
+
         # check basic juju installation
         # these could be more through
         if origin == "ppa":
 
=== modified file 'juju/machine/tests/test_unit_deployment.py'
--- juju/machine/tests/test_unit_deployment.py	2011-10-05 13:59:44 +0000
+++ juju/machine/tests/test_unit_deployment.py	2012-02-11 15:52:20 +0000
@@ -348,6 +348,8 @@
         # Setup unit namespace
         environ = dict(os.environ)
         environ["JUJU_UNIT_NS"] = "ns1"
+        environ["JUJU_SERIES"] = "precise"
+
         self.change_environment(**environ)
 
         self.unit_deploy = UnitContainerDeployment(
@@ -411,7 +413,8 @@
 
     @inlineCallbacks
     def test_start(self):
-        container = LXCContainer(self.unit_name, None, None, None)
+        container = LXCContainer(
+            self.unit_name, None, None, None, series="precise")
         rootfs = self.makeDir()
         env = dict(os.environ)
         env["JUJU_PUBLIC_KEY"] = "dsa ..."
@@ -442,7 +445,6 @@
         self.assertIn('JUJU_ZOOKEEPER="127.0.1.1:2181"', job)
         self.assertIn('JUJU_MACHINE_ID="0"', job)
         self.assertIn('JUJU_UNIT_NAME="riak/0"', job)
-
         # Verify the symlink exists
         self.assertTrue(os.path.lexists(os.path.join(
             self.unit_deploy.juju_home, "units",
@@ -468,7 +470,8 @@
     @inlineCallbacks
     def test_get_container(self):
         rootfs = self.makeDir()
-        container = LXCContainer(self.unit_name, None, None, None)
+        container = LXCContainer(
+            self.unit_name, None, None, None, series="precise")
 
         mock_deploy = self.mocker.patch(self.unit_deploy)
         mock_deploy._get_master_template(ANY, ANY, ANY)
 
=== modified file 'juju/machine/unit.py'
--- juju/machine/unit.py	2011-10-01 00:04:14 +0000
+++ juju/machine/unit.py	2012-02-11 15:52:20 +0000
@@ -189,10 +189,13 @@
 
         self._unit_namespace = os.environ.get("JUJU_UNIT_NS")
         self._juju_origin = os.environ.get("JUJU_ORIGIN")
+        self._juju_series = os.environ.get("JUJU_SERIES")
         assert self._unit_namespace is not None, "Required unit ns not found"
+        assert self._juju_series is not None, "Required juju series not found"
 
         self.pid_file = None
-        self.container = LXCContainer(self.container_name, None, None, None)
+        self.container = LXCContainer(
+            self.container_name, None, None, None, series=self._juju_series)
 
     @property
     def container_name(self):
@@ -250,7 +253,7 @@
 
         master_template = LXCContainer(
             container_template_name, origin=self._juju_origin,
-            public_key=public_key)
+            public_key=public_key, series=self._juju_series)
 
         # Debug log for the customize script, customize is only run on master.
         customize_log_path = os.path.join(
 
=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py	2011-11-16 13:56:03 +0000
+++ juju/providers/local/__init__.py	2012-02-11 15:52:20 +0000
@@ -141,7 +141,8 @@
                                     log_file=log_file,
                                     juju_origin=juju_origin,
                                     juju_unit_namespace=self._qualified_name,
-                                    public_key=public_key)
+                                    public_key=public_key,
+                                    juju_series=self.config["default-series"])
         log.info(
             "Starting machine agent (origin: %s)... ", agent.juju_origin)
         yield agent.start()
 
=== modified file 'juju/providers/local/agent.py'
--- juju/providers/local/agent.py	2011-10-05 12:14:41 +0000
+++ juju/providers/local/agent.py	2012-02-11 15:52:20 +0000
@@ -15,11 +15,12 @@
     agent_module = "juju.agents.machine"
 
     def __init__(
-        self, pid_file, zookeeper_hosts=None, machine_id="0",
+        self, pid_file, juju_series=None, zookeeper_hosts=None, machine_id="0",
         log_file=None, juju_directory="/var/lib/juju",
         juju_unit_namespace="", public_key=None, juju_origin="ppa"):
         """
         :param pid_file: Path to file used to store process id.
+        :param juju_series: The release series to use (maverick, natty, etc).
         :param machine_id: machine id for the local machine.
         :param zookeeper_hosts: Zookeeper hosts to connect.
         :param log_file: A file to use for the agent logs.
@@ -39,6 +40,7 @@
         self._log_file = log_file
         self._public_key = public_key
         self._juju_origin = juju_origin
+        self._juju_series = juju_series
 
         if self._juju_origin is None:
             origin, source = get_default_origin()
@@ -54,7 +56,7 @@
     def start(self):
         """Start the machine agent.
         """
-        assert self._zookeeper_hosts and self._log_file
+        assert self._zookeeper_hosts and self._log_file and self._juju_series
 
         if (yield self.is_running()):
             return
@@ -67,6 +69,7 @@
                 "JUJU_MACHINE_ID=%s" % self._machine_id,
                 "JUJU_HOME=%s" % self._juju_directory,
                 "JUJU_UNIT_NS=%s" % self._juju_unit_namespace,
+                "JUJU_SERIES=%s" % self._juju_series,
                 "PYTHONPATH=%s" % ":".join(sys.path),
                 sys.executable, "-m", self.agent_module,
                 "-n", "--pidfile", self._pid_file,
 
=== modified file 'juju/providers/local/tests/test_agent.py'
--- juju/providers/local/tests/test_agent.py	2011-10-05 12:14:41 +0000
+++ juju/providers/local/tests/test_agent.py	2012-02-11 15:52:20 +0000
@@ -27,7 +27,7 @@
         log_file = self.makeFile()
 
         agent = ManagedMachineAgent(
-            pid_file, get_test_zookeeper_address(),
+            pid_file, "precise", get_test_zookeeper_address(),
             juju_directory=juju_directory,
             log_file=log_file, juju_unit_namespace="ns1",
             juju_origin="lp:juju/trunk")
@@ -50,7 +50,8 @@
                  JUJU_MACHINE_ID="0",
                  JUJU_HOME=juju_directory,
                  JUJU_ORIGIN="lp:juju/trunk",
-                 JUJU_UNIT_NS="ns1"))
+                 JUJU_UNIT_NS="ns1",
+                 JUJU_SERIES="precise"))
 
     @inlineCallbacks
     def test_managed_agent_root(self):
@@ -66,14 +67,13 @@
         self.addCleanup(cleanup_root_file, log_file)
 
         agent = ManagedMachineAgent(
-            pid_file, machine_id="0", log_file=log_file,
+            pid_file, "precise", machine_id="0", log_file=log_file,
             zookeeper_hosts=get_test_zookeeper_address(),
             juju_directory=juju_directory)
 
         agent.agent_module = "juju.agents.dummy"
         self.assertFalse((yield agent.is_running()))
         yield agent.start()
-
         # Give a moment for the process to start and write its config
         yield self.sleep(0.1)
         self.assertTrue((yield agent.is_running()))
 
=== modified file 'juju/providers/local/tests/test_provider.py'
--- juju/providers/local/tests/test_provider.py	2011-10-07 18:40:48 +0000
+++ juju/providers/local/tests/test_provider.py	2012-02-11 15:52:20 +0000
@@ -29,7 +29,7 @@
         self.provider = MachineProvider(
             "local-test", {
                 "admin-secret": "admin:abc", "data-dir": self.makeDir(),
-                "authorized-keys": "fooabc123"})
+                "authorized-keys": "fooabc123", "default-series": "oneiric"})
         self.output = self.capture_logging(
             "juju.local-dev", level=logging.DEBUG)
         zookeeper.set_debug_level(0)