Code review comment for lp:~yellow/launchpad/lxcsetup

Revision history for this message
Benji York (benji) wrote :

Here's the rest of my review:

The places that have comments like "This requires Oneiric or later." or
"This script is run as root." could assert those preconditions and
generate errors if they are not met.

A question about file_insert and file_append: are the files being
modified long-lived? If we are using file_append on a file and the user
adds a new line to the end of the file then we will re-append another
copy of our line(s) to that file. Do those lines really have to be at
the very beginning and very end? Could we instead check to see if they
exist anywhere in the file and append/prepend only if they don't?

Writing the above made me realize that file_insert should probably be
named file_prepend since its behavior is the mirror of file_append and
doesn't insert the line at an arbitrary point.

Tiny suggestion: I think it would read better if you used

    with ssh_connection(lxcname) as ssh:
        ...

instead of

    with ssh(lxcname) as sshcall:
        ...

in initialize_lxc. The later "ssh(...)" calls would read better as
well.

We should have some error checking/reporting for all the commands we're
running (via SSH). Just something simple to stderr would help
tremendously for that inevitable time in the future when the script
fails for one reason or another.

What purpose does the time.sleep(5) in stop_lxc serve? Is it intended
as a deadline for the instance to stop cleanly? If so, it seems fine
(if a little short). We could increase it to account for the inevitable
slow shutdowns that will sometimes happen and add a probe in a loop to
see if the instance has actually stopped or not that can exit early if
the instance stops before the timeout value is reached (much as you do
in create_lxc).

On the other hand, since these are ephemeral instances, perhaps we don't
really care to let them shut down cleanly, in that case we could just
lxc-stop them without the poweroff.

Name of the Namespace class could be a bit more specific (line 584 of
the diff). Perhaps "Configuration".

« Back to merge proposal