pyjuju

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

lxc-lib
Merge into trunk

Revision history for this message

Benjamin Saller (bcsaller) wrote on 2011-09-14:

Responded to these point and Kapils later one. All changes have been
addressed. Re [22] the tests are not run as root, they ask for
permissions the first time they need them. When not run as root they
require interactivity but they don't run by default in the test suite
(as they can be quite expensive timewise and do need perms).

On Sat, Sep 3, 2011 at 9:25 AM, Gustavo Niemeyer <email address hidden> wrote:
> Review: Approve
> 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?
>
> --
> https://code.launchpad.net/~bcsaller/ensemble/lxc-lib/+merge/71396
> You are the owner of lp:~bcsaller/ensemble/lxc-lib.
>

On Sat, Sep 3, 2011 at 9:25 AM, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Review: Approve
> 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?
>
> --
> https://code.launchpad.net/~bcsaller/ensemble/lxc-lib/+merge/71396
> You are the owner of lp:~bcsaller/ensemble/lxc-lib.
>

« Back to merge proposal