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?