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

Quite a few details, but it looks very nice overall.V

+1 with these sorted.

[1]

=== added file 'ensemble/lib/lxc/data/ensemble-create'

Nicely organized script, thanks.

[2]

+ ENSEMBLE_SOURCE="ppa:ensemble/ppa"
+ elif [ $ENSEMBLE_ORIGIN = "lp" ]; then
+ echo "Using Ensemble Branch $ensemble_source"

s/$ensemble_source/$ENSEMBLE_SOURCE/

Environment variables are case sensitive.

Also, let's s/lp/branch/ please. Seems more clear and PPAs
are also in Launchpad.

As a minor, s/Branch/branch/

[3]

+ elif [ $ENSEMBLE_ORIGIN = "distro" ]; then
+ echo "Using Ensemble distribution packages"
+ fi

Should error if $ENSEMBLE_ORIGIN contains garbage.

[4]

+ ENSEMBLE_SOURCE="ppa:ensemble/ppa"
(...)
+ apt-add-repository $ENSEMBLE_SOURCE

Seems unnecessary to use the variable if the script is
hardcoding the behavior. Just use the value itself in
the second line.

[5]

- env_defaults = environment.get_serialization_data()
- options.placement = env_defaults[environment.name].get(
- "placement", UNASSIGNED_POLICY)
+ options.placement = environment.placement

Nice.

[6]

+BASE_PATH = os.path.normpath(
+ os.path.abspath(
+ os.path.join(os.path.dirname(__file__), "data")))

s/BASE_PATH/DATA_PATH/?

Also, abspath calls normpath internally.

[7]

+class LXCError(EnsembleError):
+ """Indicates a low level error with an LXC container"""
+ pass

/pass/d

[8]

+ p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.PIPE,
+ env=dict(os.environ))
+ r = p.wait()
+ if r != 0:
+ # read the stdout/err streams and show the user
+ print >>sys.stderr, p.stderr.read()
+ raise LXCError(p.stdout.read())

This logic is suboptimal in a few ways:

- The stderr and stdout will be separated out, which means depending on
  the output it will be hard to interpret what the error was about.

- In case stdout or stderr get enough data to fill the kernel buffer,
  the p.wait() will hang completely and never return.

- The argument of env provided is the default behavior if not provided.

- The stderr printed has no context.

To fix all of these, use something along these lines:

    p = subprocess.Popen(args, stdout=subprocess.PIPE, stderr=subprocess.STDOUT)
    output = p.communicate()[0]
    if p.wait() != 0:
        raise LXCError(output)

Note the argument of stderr (STDOUT, not PIPE). This will make them share
the same file descriptor and intersperse the output as it happens, fixing
the first point. communicate() avoids the second problem.

[9]

+ "-f", config_file,
+ "--",
+ "-r", release]

Weird.. what's the -- about for lxc-create? Can you please add a comment
in the code?

[10]

+ return _cmd(args)
+
+
+def _lxc_start(container_name, console_log=None):

As a suggestion only, I'd remove the two spaces between functions. I know
about PEP8, but this is one of those cases where it feels detrimental to
readability.

[11]

+ subprocess.Popen(["sudo", "lxc-stop", "-n",
+ container_name],
(...)
+ output = subprocess.Popen(["lxc-ls"],
+ stdout=subprocess.PIPE).communicate()[0]

Please use _cmd across the board. You can add an argument if you need
a slightly different behavior. We don't want to have to remember and
think about the problems mentioned across the whole file.

[12]

+ in_path = os.path.join(container_root, "tmp", base_script)

Writing something to a globally writable location with a known name
is a security bug.

/usr/local/bin might be an interesting location for this.

[13]

+ shutil.copy(customize_script, in_path)
+ os.chmod(in_path, 0770)

Is there a specific reason for the 0770? If not, please use the
traditional 0755.

[14]

+ if not os.access(pathname, os.R_OK):
+ raise LXCError("Invalid file %s" % pathname)

The user will appreciate a hint about why we think the file is invalid.
Also, please use a colon before the filename ("This sucks: %s").

[15]

+class LXCContainer(object):
+ def __init__(self, container_name, configuration=None, network_name="virbr0",
+ customize_script=None):
+ """Create an LXC container

s/LXC container/LXCContainer/. It won't create the LXC container itself.

[16]

+ template = open(lxc_config, "r").read()

Trusting garbage collection to close files is a bad practice

   with open(lxc...) as f:

etc.

[17]

+ # open the template file and create a new temp processed
+ # version
+ self.lxc_config = self._make_lxc_config(network_name)
(...)
+ fd, output_fn = tempfile.mkstemp(suffix=".conf")

The constructor is creating a new temporary file every time the
constructor is called. This file should be created when necessary
only, and should be removed once it's not necessary anymore.

Also, have a look at tempfile.TemporaryFile for a way to potentially
simplify it a bit.

[18]

+ def _injectConfiguration(self):

Not the naming convention for the project (at least not for the
Python side of it ;-).

[19]

+ if not os.path.exists(config_dir):
+ _cmd(["sudo", "mkdir", config_dir])

It's nice that we're not expecting the user to run the script itself
as root, but rather just asking for the access when needed. Well done.

+ _cmd(["sudo", "mv", fn,
+ self._p("etc/ensemble/ensemble.conf")])

Ditto.

[20]

+ @inlineCallbacks
+ def _customize_container(self):
+ self._injectConfiguration()
+ yield deferToThread(_customize_container, self.customize_script, self.rootfs)
+
+ @inlineCallbacks
+ def create(self):
+ yield deferToThread(_lxc_create, self.container_name,
+ config_file=self.lxc_config)
+ yield self._customize_container()

Looks like the only user of self._customize_container is create(). If so,
this can be simplified by renaming it to self._create_container, putting
both commands in it, and having deferToThread just once.

[21]

+ def run(self):
+ def stop(self):
+ def destroy(self):

Nice API. Thanks for that.

[22]

+ # expect a processed config file above the rootfs
+ # -- this requires root to read
+ #network = open(os.path.join(c.rootfs, "..", "config"), "r").read()
+ #self.assertIn("lxc.network.link=virbr0", network)

Aren't tests running as root?