Merge lp:~hazmat/pyjuju/local-origin-passthrough into lp:pyjuju

[Kapil Thangavelu]

Local provider should respect juju-origin

Minor fixes to pass juju origin to the machine agent, and to juju-create
to utilize jujuorigin for lp branch names.

Depends on env-origin AND unit-relation-with-addr...

the latter is probably immaterial.. just happened to be in the pipeline, i can extract if its a problem.

[Jim Baker (jimbaker)]

The implementation looks good.

However, there were two problems:

1. I was unable to run this with our wordpress example. None of the service units start up. Are there any branches that this work implicitly depends on?

2. Probably related, this error was reported:

[ERROR]
Traceback (most recent call last):
  File "/usr/lib/python2.7/dist-packages/twisted/internet/defer.py", line 1020, in _inlineCallbacks
    result = g.send(result)
  File "/home/jbaker/canonical/juju/hazmat/local-origin-passthrough/juju/machine/tests/test_unit_deployment.py", line 405, in test_origin_usage
    self.unit_name, self.juju_home, customize_log=customize_log)
exceptions.TypeError: __init__() got an unexpected keyword argument 'customize_log'

juju.machine.tests.test_unit_deployment.UnitContainerDeploymentTest.test_origin_usage

review: Needs Information

[Kapil Thangavelu (hazmat)]

i'm still in progress on functionally testing this and the overall local provider end 2 end. with drawing from the review queue till that's completed.

[Kapil Thangavelu (hazmat)]

> The implementation looks good.
>
> However, there were two problems:
>
> 1. I was unable to run this with our wordpress example. None of the service
> units start up. Are there any branches that this work implicitly depends on?
>

works pretty well for me now with juju-origin to a branch, the ppa still seems to hit the protocol version issue (i guess the ppa hasn't updated), i'd be happy to debug with you what's happening on your system. the master-customize.log is pretty critical to understanding most issues.

> 2. Probably related, this error was reported:
>

fixed.

thanks for having a look.

[Gustavo Niemeyer (niemeyer)]

LGTM. Just a couple of trivials:

[1]

149 - elif [ $JUJU_ORIGIN = "branch" ]; then
150 - echo "Using juju Branch $JUJU_SOURCE"
151 + elif [[ $JUJU_ORIGIN == lp:* ]]; then
152 + echo "Using juju branch $JUJU_SOURCE"

Hmmm.. What'd be the reason to have $JUJU_SOURCE if $JUJU_ORIGIN
already has the branch?

Also, is there a difference between == and = in this context? They're
both being used with the same purpose in the two different occurrences
of this test.

[2]

177 bash -c "cd /usr/lib/juju/juju && sudo python setup.py develop"
178 + chmod -R a+r /usr/lib/juju

Indentation is slightly off here.

review: Approve

[Benjamin Saller (bcsaller)]

[3]

132 - _customize_container(self.customize_script, self.rootfs)
133 + _, output = _customize_container(self.customize_script, self.rootfs)
134 +
135 + if self.customize_log:
136 + with open(self.customize_log, "w") as fh:
137 + fh.write(output)

Its very possible to write a customize_log location which is in the container but not writeable by the caller. In cases like this opening a temp file and using [sudo mv tmp customize_log] might make sense. Then again I suppose there is a single supported use of this and its already written so I don't feel strongly.

[4]

+ apt-get install --force-yes -y bzr tmux sudo python-software-properties python-yaml

I am reminded reminded that we have a bug around hook execution needing the DEBIAN_FRONTEND=noninteractive. I think we should do this in this file as well

DEBIAN_FRONTEND=noninteractive
APT_OPTIONS="-o Dpkg::Options::="--force-confnew --force-yes -fuy"

apt-get install $APT_OPTIONS ...

might make sense in this file as well.

[5]
175 + # light weight checkout is significantly faster, no history though
176 + bzr co --lightweight $JUJU_ORIGIN /usr/lib/juju/juju

This strikes me as very much the correct thing to so here.

[6] The private address relation stuff seems like a good idea, but I'll be more pleased when the unit-get stuff you did lands. This branch still can't run the examples as they stand now.

review: Approve

[Kapil Thangavelu (hazmat)]

Excerpts from Benjamin Saller's message of Tue Oct 04 20:26:25 UTC 2011:
> Review: Approve
>
> [3]
>
> 132 - _customize_container(self.customize_script, self.rootfs)
> 133 + _, output = _customize_container(self.customize_script, self.rootfs)
> 134 +
> 135 + if self.customize_log:
> 136 + with open(self.customize_log, "w") as fh:
> 137 + fh.write(output)
>
> Its very possible to write a customize_log location which is in the container but not writeable by the caller. In cases like this opening a temp file and using [sudo mv tmp customize_log] might make sense. Then again I suppose there is a single supported use of this and its already written so I don't feel strongly.

Customize is only invoked once for master template container, not per container
in practice since its not used for clones. To make it easy to discover and find
this file we end up putting it into data-dir/units/master-customize.log. Putting
it in the container would detract from easy discovery. In practice most of the
issues with setting up the local provider where localized to the output from
this script, and keeping it outside the container helps ensure it survives
a destroy-environment if its needed for additional debugging. iotw, i think its
in a good spot for now.

>
> [4]
>
> + apt-get install --force-yes -y bzr tmux sudo python-software-properties python-yaml
>
> I am reminded reminded that we have a bug around hook execution needing the DEBIAN_FRONTEND=noninteractive. I think we should do this in this file as well
>
> DEBIAN_FRONTEND=noninteractive
> APT_OPTIONS="-o Dpkg::Options::="--force-confnew --force-yes -fuy"
>
> apt-get install $APT_OPTIONS ...
>
> might make sense in this file as well.
>

Sounds good, thanks.

>
>
> [5]
> 175 + # light weight checkout is significantly faster, no history though
> 176 + bzr co --lightweight $JUJU_ORIGIN /usr/lib/juju/juju
>
> This strikes me as very much the correct thing to so here.
>

Indeed, if needed they can be easily bound on a per container basis. I'm a
little concerned that this will end up stale because customize is only invoked
once, but i don't see any other easy options here, and it has a nice side
benefit ensuring consistency across a deploy.

>
> [6] The private address relation stuff seems like a good idea, but I'll be more pleased when the unit-get stuff you did lands. This branch still can't run the examples as they stand now.

Yeah.. but they where already broken for local deploy before the branch.. they
unit-get cli branch should be merged shortly.

thanks for the review.
cheers,
Kapil

[lp:~hazmat/pyjuju/local-origin-passthrough]

418. By Kapil Thangavelu on 2011-10-04

merge pipeline, resolve conflict

[Kapil Thangavelu (hazmat)]

Excerpts from Gustavo Niemeyer's message of Tue Oct 04 02:08:27 UTC 2011:
> Review: Approve
>
> LGTM. Just a couple of trivials:
>
> [1]
>
> 149 - elif [ $JUJU_ORIGIN = "branch" ]; then
> 150 - echo "Using juju Branch $JUJU_SOURCE"
> 151 + elif [[ $JUJU_ORIGIN == lp:* ]]; then
> 152 + echo "Using juju branch $JUJU_SOURCE"
>
> Hmmm.. What'd be the reason to have $JUJU_SOURCE if $JUJU_ORIGIN
> already has the branch?

Leftover from previous usage, replaced with JUJU_ORIGIN

>
> Also, is there a difference between == and = in this context? They're
> both being used with the same purpose in the two different occurrences
> of this test.

Sounds good, both work, i'll use '=' for consistency.

>
> [2]
>
> 177 bash -c "cd /usr/lib/juju/juju && sudo python setup.py develop"
> 178 + chmod -R a+r /usr/lib/juju
>
> Indentation is slightly off here.

Fixed, thanks

=== modified file 'juju/environment/config.py'
--- juju/environment/config.py	2011-10-04 19:44:11 +0000
+++ juju/environment/config.py	2011-10-04 22:53:25 +0000
@@ -55,8 +55,9 @@
                               "storage-url": String(),
                               "placement": String(),
                               "default-series": String()},
-                             optional=["storage-url", "placement"]),
-        "lxc": KeyDict({"admin-secret": String(),
+                             optional=["storage-url", "placement",
+                                       "default-series"]),
+        "local": KeyDict({"admin-secret": String(),
                         "data-dir": String(),
                         "placement": Constant("local"),
                         "default-series": String()},

=== modified file 'juju/environment/tests/test_config.py'
--- juju/environment/tests/test_config.py	2011-10-04 19:44:11 +0000
+++ juju/environment/tests/test_config.py	2011-10-04 22:53:25 +0000
@@ -43,7 +43,7 @@
 
 environments:
    sample:
-     type: lxc
+     type: local
      admin-secret: sekret
      default-series: oneiric
 """

=== modified file 'juju/lib/lxc/__init__.py'
--- juju/lib/lxc/__init__.py	2011-09-29 22:04:55 +0000
+++ juju/lib/lxc/__init__.py	2011-10-04 22:53:25 +0000
@@ -152,7 +152,9 @@
                  origin_source=None,
                  network_name="virbr0",
                  customize_script=None,
-                 debug_log=None, console_log=None):
+                 debug_log=None,
+                 console_log=None,
+                 customize_log=None):
         """Create an LXCContainer
 
         :param container_name: should be unique within the system
@@ -174,7 +176,7 @@
         self.container_name = container_name
         self.debug_log = debug_log
         self.console_log = console_log
-
+        self.customize_log = customize_log
         if customize_script is None:
             customize_script = os.path.join(DATA_PATH,
                                             "juju-create")
@@ -240,7 +242,11 @@
                 print >>fp, escape("JUJU_SOURCE", self.source)
 
         _cmd(["sudo", "mv", fn, target])
-        _customize_container(self.customize_script, self.rootfs)
+        _, output = _customize_container(self.customize_script, self.rootfs)
+
+        if self.customize_log:
+            with open(self.customize_log, "w") as fh:
+                fh.write(output)
 
     def execute(self, args):
         if not isinstance(args, (list, tuple)):

=== modified file 'juju/lib/lxc/data/juju-create'
--- juju/lib/lxc/data/juju-create	2011-09-30 01:57:00 +0000
+++ juju/lib/lxc/data/juju-create	2011-10-04 22:53:25 +0000
@@ -69,8 +69,8 @@
 
     if [ $JUJU_ORIGIN = "ppa" ]; then
         echo "Using juju PPA for container"
-    elif [ $JUJU_ORIGIN = "branch" ]; then
-        echo "Using juju Branch $JUJU_SOURCE"
+    elif [[ $JUJU_ORIGIN == lp:* ]]; then
+        echo "Using juju branch $JUJU_SOURCE"
     elif [ $JUJU_ORIGIN = "distro" ]; then
         echo "Using juju distribution packages"
     else
@@ -79,7 +79,7 @@
     fi
 
     echo "Setting up juju in container"
-    apt-get install --force-yes -y bzr tmux sudo python-software-properties
+    apt-get install --force-yes -y bzr tmux sudo python-software-properties python-yaml
 
     if [ $JUJU_ORIGIN = "ppa" ]; then
         # the echo forces an enter to get around the interactive
@@ -87,11 +87,13 @@
         echo y | apt-add-repository ppa:juju/pkgs
         apt-get update
         apt-get install --force-yes -y juju python-txzookeeper
-    elif [ $JUJU_ORIGIN = "branch" ]; then
-        apt-get install --force-yes -y python-txzookeeper
+    elif [[ $JUJU_ORIGIN = lp:* ]]; then
+        apt-get install --force-yes -y python-txzookeeper python-setuptools
         mkdir /usr/lib/juju
-        bzr branch $JUJU_SOURCE /usr/lib/juju/juju
+        # light weight checkout is significantly faster, no history though
+        bzr co --lightweight $JUJU_ORIGIN /usr/lib/juju/juju
         bash -c "cd /usr/lib/juju/juju && sudo python setup.py develop"
+	chmod -R a+r /usr/lib/juju
     elif [ $JUJU_ORIGIN = "distro" ]; then
         apt-get install --force-yes -y juju python-txzookeeper
     fi

=== modified file 'juju/lib/lxc/tests/test_lxc.py'
--- juju/lib/lxc/tests/test_lxc.py	2011-09-28 18:04:36 +0000
+++ juju/lib/lxc/tests/test_lxc.py	2011-10-04 22:53:25 +0000
@@ -76,14 +76,16 @@
     @inlineCallbacks
     def test_lxc_deferred(self):
         self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
-        yield deferToThread(_lxc_create, DEFAULT_CONTAINER, config_file=self.config)
+        yield deferToThread(
+            _lxc_create, DEFAULT_CONTAINER, config_file=self.config)
         yield deferToThread(_lxc_start, DEFAULT_CONTAINER)
 
     @inlineCallbacks
     def test_lxc_container(self):
         self.addCleanup(self.clean_container, DEFAULT_CONTAINER)
-
-        c = LXCContainer(DEFAULT_CONTAINER, "dsa...", "ppa")
+        customize_log = self.makeFile()
+        c = LXCContainer(
+            DEFAULT_CONTAINER, "dsa...", "ppa", customize_log=customize_log)
 
         running = yield c.is_running()
         self.assertFalse(running)
@@ -123,6 +125,9 @@
         containers = yield get_containers(None)
         self.assertNotIn(DEFAULT_CONTAINER, containers)
 
+        # Verify the customize log file.
+        self.assertTrue(os.path.exists(customize_log))
+
         # and its gone
         output = _lxc_ls()
         self.assertNotIn(DEFAULT_CONTAINER, output)
@@ -240,7 +245,9 @@
         # we'd have to run customize_constainer again which removes
         # some of the point of the clone support to repair this.
         # droppping assertion for now and replacing with a lax one
-        #XXX::: self.assertIn("127.0.0.1 %s localhost" % c.container_name, sudo_get("etc/hosts"))
+        #XXX::: self.assertIn("127.0.0.1 %s localhost" % c.container_name,
+        #XXX    sudo_get("etc/hosts"))
+
         self.assertIn(c.container_name, sudo_get("etc/hosts"))
 
         # nameserver
@@ -263,7 +270,3 @@
             self.assertEqual(1, run(["dpkg-query", "-s", "juju"]))
             # but the branch is checked out
             self.asssertTrue(os.path.exists(p("usr/lib/juju/juju")))
-
-
-
-

=== modified file 'juju/machine/tests/test_unit_deployment.py'
--- juju/machine/tests/test_unit_deployment.py	2011-09-29 22:04:55 +0000
+++ juju/machine/tests/test_unit_deployment.py	2011-10-04 22:53:25 +0000
@@ -386,6 +386,30 @@
         self.assertIn("Destroyed container for riak/0", output)
 
     @inlineCallbacks
+    def test_origin_usage(self):
+        """The machine agent is started with a origin environment variable
+        """
+        mock_container = self.mocker.patch(LXCContainer)
+        mock_container.is_constructed()
+        self.mocker.result(True)
+        mock_container.is_constructed()
+        self.mocker.result(True)
+        self.mocker.replay()
+
+        environ = dict(os.environ)
+        environ["JUJU_ORIGIN"] = "lp:~juju/foobar"
+
+        self.change_environment(**environ)
+        unit_deploy = UnitContainerDeployment(
+            self.unit_name, self.juju_home)
+        container = yield unit_deploy._get_master_template(
+            "local", "127.0.0.1:1", "abc")
+        self.assertEqual(container.origin, "lp:~juju/foobar")
+        self.assertEqual(
+            container.customize_log,
+            os.path.join(self.juju_home, "units", "master-customize.log"))
+
+    @inlineCallbacks
     def test_start(self):
         container = LXCContainer(self.unit_name, None, None, None)
         rootfs = self.makeDir()
@@ -429,6 +453,12 @@
             self.unit_deploy.directory, "var", "lib", "juju", "units",
             self.unit_deploy.unit_path_name, "charm", "metadata.yaml")))
 
+        # Verify the directory structure in the unit.
+        self.assertTrue(os.path.exists(os.path.join(
+            self.unit_deploy.directory, "var", "lib", "juju", "state")))
+        self.assertTrue(os.path.exists(os.path.join(
+            self.unit_deploy.directory, "var", "log", "juju")))
+
         # Verify log output
         output = self.output.getvalue()
         self.assertIn("Charm extracted into container", output)

=== modified file 'juju/machine/unit.py'
--- juju/machine/unit.py	2011-09-30 03:20:11 +0000
+++ juju/machine/unit.py	2011-10-04 22:53:25 +0000
@@ -188,6 +188,7 @@
         super(UnitContainerDeployment, self).__init__(unit_name, juju_home)
 
         self._unit_namespace = os.environ.get("JUJU_UNIT_NS")
+        self._juju_origin = os.environ.get("JUJU_ORIGIN")
         assert self._unit_namespace is not None, "Required unit ns not found"
 
         self.pid_file = None
@@ -248,7 +249,13 @@
             self._unit_namespace, machine_id)
 
         master_template = LXCContainer(
-            container_template_name, origin="ppa", public_key=public_key)
+            container_template_name, origin=self._juju_origin,
+            public_key=public_key)
+
+        # Debug log for the customize script, customize is only run on master.
+        customize_log_path = os.path.join(
+            self.juju_home, "units", "master-customize.log")
+        master_template.customize_log = customize_log_path
 
         if not master_template.is_constructed():
             log.debug("Creating master container...")
@@ -286,7 +293,7 @@
         # Build a template container that can be cloned in deploy
         # we leave the loosely initialized self.container in place for
         # the class as thats all we need for methods other than start.
-        container, directory = yield self._get_container(machine_id,
+        self.container, self.directory = yield self._get_container(machine_id,
                                                          zookeeper_hosts,
                                                          bundle,
                                                          public_key)
@@ -295,14 +302,14 @@
 
         # Extract the charm bundle
         charm_path = os.path.join(
-            directory, "var", "lib", "juju", "units",
+            self.directory, "var", "lib", "juju", "units",
             self.unit_path_name, "charm")
         bundle.extract_to(charm_path)
         log.debug("Charm extracted into container")
 
         # Write upstart file for the agent into the container
         upstart_path = os.path.join(
-            directory, "etc", "init",
+            self.directory, "etc", "init",
             "%s-unit-agent.conf" % self.unit_path_name)
         with open(upstart_path, "w") as fh:
             fh.write(self.get_upstart_unit_job(machine_id, zookeeper_hosts))
@@ -312,12 +319,17 @@
             self.juju_home, "units", self.unit_path_name, "unit.log")
         if not os.path.lexists(unit_log_path_host):
             os.symlink(
-                os.path.join(directory, "var", "log", "juju",
+                os.path.join(self.directory, "var", "log", "juju",
                              "unit-%s.log" % self.unit_path_name),
                 unit_log_path_host)
 
+        # Debug log for the container
+        container_log_path = os.path.join(
+            self.juju_home, "units", self.unit_path_name, "container.log")
+        self.container.debug_log = container_log_path
+
         log.debug("Starting container...")
-        yield container.run()
+        yield self.container.run()
         log.info("Started container for %s" % self.unit_name)
 
     @inlineCallbacks

=== modified file 'juju/providers/local/__init__.py'
--- juju/providers/local/__init__.py	2011-09-30 03:20:11 +0000
+++ juju/providers/local/__init__.py	2011-10-04 22:53:25 +0000
@@ -125,11 +125,14 @@
         log.info("Starting machine agent...")
         pid_file = os.path.join(self._directory, "machine-agent.pid")
         log_file = os.path.join(self._directory, "machine-agent.log")
+
+        juju_origin = self.config.get("juju-origin")
         agent = ManagedMachineAgent(pid_file,
                                     zookeeper_hosts=zookeeper.address,
                                     machine_id="0",
                                     juju_directory=self._directory,
                                     log_file=log_file,
+                                    juju_origin=juju_origin,
                                     juju_unit_namespace=self._qualified_name,
                                     public_key=public_key)
         yield agent.start()

=== modified file 'juju/providers/local/agent.py'
--- juju/providers/local/agent.py	2011-09-27 16:51:50 +0000
+++ juju/providers/local/agent.py	2011-10-04 22:53:25 +0000
@@ -7,6 +7,8 @@
 from twisted.internet.defer import inlineCallbacks, returnValue
 from twisted.internet.threads import deferToThread
 
+from juju.providers.common.cloudinit import get_default_origin
+
 
 class ManagedMachineAgent(object):
 
@@ -15,8 +17,7 @@
     def __init__(
         self, pid_file, zookeeper_hosts=None, machine_id="0",
         log_file=None, juju_directory="/var/lib/juju",
-        juju_unit_namespace="",
-        public_key=None):
+        juju_unit_namespace="", public_key=None, juju_origin="ppa"):
         """
         :param pid_file: Path to file used to store process id.
         :param machine_id: machine id for the local machine.
@@ -37,6 +38,10 @@
         self._juju_unit_namespace = juju_unit_namespace
         self._log_file = log_file
         self._public_key = public_key
+        self._juju_origin = juju_origin
+
+        if self._juju_origin is None:
+            self._juju_origin = get_default_origin()
 
     @inlineCallbacks
     def start(self):
@@ -51,6 +56,7 @@
         # to the command.
         args = ["sudo",
                 "JUJU_ZOOKEEPER=%s" % self._zookeeper_hosts,
+                "JUJU_ORIGIN=%s" % self._juju_origin,
                 "JUJU_MACHINE_ID=%s" % self._machine_id,
                 "JUJU_HOME=%s" % self._juju_directory,
                 "JUJU_UNIT_NS=%s" % self._juju_unit_namespace,
@@ -60,7 +66,8 @@
                 "--logfile", self._log_file]
 
         if self._public_key:
-            args.insert(1, "JUJU_PUBLIC_KEY=%s" % pipes.quote(self._public_key))
+            args.insert(
+                1, "JUJU_PUBLIC_KEY=%s" % pipes.quote(self._public_key))
 
         yield deferToThread(subprocess.check_call, args)
 

=== modified file 'juju/providers/local/tests/test_agent.py'
--- juju/providers/local/tests/test_agent.py	2011-09-27 16:51:50 +0000
+++ juju/providers/local/tests/test_agent.py	2011-10-04 22:53:25 +0000
@@ -47,6 +47,7 @@
             dict(JUJU_ZOOKEEPER=get_test_zookeeper_address(),
                  JUJU_MACHINE_ID="0",
                  JUJU_HOME=juju_directory,
+                 JUJU_ORIGIN="ppa",
                  JUJU_UNIT_NS="ns1"))
 
     @inlineCallbacks

=== modified file 'juju/unit/address.py'
--- juju/unit/address.py	2011-09-25 06:47:08 +0000
+++ juju/unit/address.py	2011-10-04 22:53:25 +0000
@@ -16,7 +16,7 @@
     provider_type = yield settings.get_provider_type()
     if provider_type == "ec2":
         returnValue(EC2UnitAddress())
-    elif provider_type == "lxc":
+    elif provider_type == "local":
         returnValue(LocalUnitAddress())
     elif provider_type == "orchestra":
         returnValue(OrchestraUnitAddress())

=== modified file 'juju/unit/tests/test_address.py'
--- juju/unit/tests/test_address.py	2011-09-26 17:11:52 +0000
+++ juju/unit/tests/test_address.py	2011-10-04 22:53:25 +0000
@@ -33,7 +33,7 @@
 
     @inlineCallbacks
     def test_get_local_address(self):
-        address = yield self.get_address_for("lxc")
+        address = yield self.get_address_for("local")
         self.assertTrue(isinstance(address, LocalUnitAddress))
 
     @inlineCallbacks