Code review comment for lp:~bcsaller/pyjuju/lxc-omega

There are a few details, but this looks generally nice, thanks.

[2]

+ self.master_template = LXCContainer(
+ container_template_name,
+ origin="ppa",

Shouldn't this be dependent on the source of juju itself?

Please check out the env-origin branch that hopefully Jim is landing tomorrow.
Should be trivial to hook on it.

[3]

-hostname=`curl http://169.254.169.254/latest/meta-data/local-hostname`
-
+hostname=`ip -f inet addr |grep 192.168| grep -v '192\.168\.1\.' | awk '{print $2;}'|cut -f 1 -d '/'`

Please drop the example changes for now. It break the examples in EC2, and consequently our waterfall.

[4]

I haven't followed carefully, but my gut feeling is that the branch is
a bit shallow on testing. Can you please verify if the following logic
is well covered:

- get_master_template
- start using get_master_template
- JUJU_PUBLIC_KEY handling in agent.py
- etc

[5]

+ log.debug("Created master container %s" %
+ container_template_name)

Indentation is off.

[6]

+ try:
+ lockfile.lock()

The locking should be outside the try.. if it fails for whatever
reason, it shouldn't be unlocked.

[7]

Considering that this is using Twisted, shouldn't the lock and unlock
be yielding? I suspect they might be a NOOP at the moment.

[8]

+ if public_key.startswith("'"):
+ public_key = public_key[1:-1]

Please strip it rather than assuming the position blindly. This logic
breaks if e.g. there are spaces at the end of the line.

[9]

+ lockfile = FilesystemLock(os.path.join(
+ "/tmp/", "%s.lock" % container_template_name))

That's a security bug. One should never write any files to a publicly
writable location with a known name.

[10]

After you do these changes, can you please ask for another review from
Kapil. I'll be traveling tomorrow, but if he's happy with the branch
I'm +1 on it too. Thank you!