Issue 5764043: Go port for lxc
|
Created:
12 years, 1 month ago by andrewsmedina Modified:
12 years, 1 month ago Reviewers:
mp+96266 Visibility:
Public. |
Description
https://code.launchpad.net/~andrewsmedina/juju/go-lxc/+merge/96266
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 11
Patch Set 2 : Go port for lxc #
Total comments: 10
Patch Set 3 : Go port for lxc #Patch Set 4 : Go port for lxc #MessagesTotal messages: 18
Please take a look.
Sign in to reply to this message.
Looks nice, thanks Andrew. A few details before we can submit: https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go File environs/lxc/lxc.go (right): https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode9 environs/lxc/lxc.go:9: type Container struct { Please add docs in the type and in all methods below. https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode13 environs/lxc/lxc.go:13: func (c *Container) Rootfs() string { s/Rootfs/RootPath/ https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode18 environs/lxc/lxc.go:18: return exec.Command("sudo", "lxc-create", "-n", c.Name).Run() Not sure about what we'll want to do with stdout and stderr, but looks ok for now. https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode34 environs/lxc/lxc.go:34: return strings.Contains(Ls(), c.Name) This must be adapted to verify the elements item by item, but see the comments on Ls. https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode37 environs/lxc/lxc.go:37: func Ls() string { s/Ls/List/ Then, we need it to make it return a []string with the container names. https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc_test.go File environs/lxc/lxc_test.go (right): https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc_test.go#newcode10 environs/lxc/lxc_test.go:10: DefaultContainer = "lxc_test" Please put this in a single line: const DefaultContainer ... https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc_test.go#newcode17 environs/lxc/lxc_test.go:17: var _ = Suite(&S{}) Please add a SetUpSuite resembling this: func (s *S) SetUpSuite(c *C) { if !*lxcEnabled { c.Skip("lxc tests need sudo access (-lxc to enable)") } } and define a flag at the top: var lxcEnabled = flag.Bool("lxc", false, "enable LXC tests that require sudo")
Sign in to reply to this message.
s/Andrew/Andrews/, sorry.
Sign in to reply to this message.
thanks very much for starting on this. i'm very excited to see the beginnings of a new Environ implementation! https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go File environs/lxc/lxc.go (right): https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode1 environs/lxc/lxc.go:1: package lxc package local (as mentioned below) https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode9 environs/lxc/lxc.go:9: type Container struct { i think that this type and its methods can remain private until we (perhaps) want to factor it into its own package. this package is about implementing environs.Environ, not exposing an lxc interface. also, as gustavo pointed out, the package should be named "environs/local" not "environs/lxc". https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode30 environs/lxc/lxc.go:30: return exec.Command("sudo", "lxc-destroy", "-n", c.Name).Run() i'm wondering if you want to save the output of these commands (all the calls to Run) and return it when there's an error. otherwise the diagnostics when things go wrong will be really not very useful :-) https://codereview.appspot.com/5764043/diff/1/environs/lxc/lxc.go#newcode42 environs/lxc/lxc.go:42: return out.String() you'll want to check the error too. you could use exec.Command("sudo", "lxc-ls").Output() here, except that i think you want to save error output as mentioned above.
Sign in to reply to this message.
I've marked the merge proposal as Work In Progress. I believe lbox isn't actually moving the proposal back to Needs Review. Can you please make sure you check the merge proposal after you next "lbox propose" it? I'll fix lbox to not need that.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode8 environs/local/lxc.go:8: // Represents a lxc container. // container represents an LXC container with the given name. (doc comments in Go should always be full sentences) https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode9 environs/local/lxc.go:9: type container struct { the methods of this type are still exported, even though the type isn't. i'd make everything in this file unexported to save confusion. https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode18 environs/local/lxc.go:18: // Create the container executing lxc-create this should be a full sentence. also, i'm not sure it's useful to mention the names of the command that's being executed. // Create creates the LXC container. i'm not sure it's useful to return the output of the command as a []byte (particularly given that it's not currently even returning stuff that's printed to stderr). i think that the output should be returned as part of the error message if the command fails. how about a function like this: // runLXCCommand runs an LXC command with the given arguments, // sending the standard output to stdout if stdout is non-nil. func runLXCCommand(stdout io.Writer, args ...string) error { var buf bytes.Buffer cmd := exec.Command(args[0], args...) cmd.Stdout = stdout cmd.Stderr = &buf err := cmd.Run() if err == nil { return nil } msg := buf.String() if len(msg) == 0 { return err } // The useful error message is in the first // line of the LXC command's output. The other // lines contain the usage message which // we don't want to see. if i := strings.Index(msg, "\n"); i > 0 { msg = msg[0:i] } // Add a prefix mentioning the command name if // there isn't one already (for instance if sudo fails). if !strings.HasPrefix(msg, args[0]) { msg = args[0] + ": " + msg } return errors.New(msg) } then you could define Create and friends like this: func (c *container) create() error { return runLXCCommand(nil, "lxc-create", "-n", c.Name) } https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode58 environs/local/lxc.go:58: return strings.Fields(string(output)) this would cause problems if lxc names were allowed to contain spaces. as it is, it seems that lxc names with spaces cause problems - perhaps we should check somewhere that the name does not contain any white space. the check doesn't necessarily have to be in this file though.
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode18 environs/local/lxc.go:18: // Create the container executing lxc-create On 2012/03/14 09:31:26, rog wrote: > // The useful error message is in the first > // line of the LXC command's output. The other > // lines contain the usage message which > // we don't want to see. Roger, we already talked about that in a different context. Arbitrarily selecting a line out of some commands output is not a good approach, for the same reasons we debated before.
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode18 environs/local/lxc.go:18: // Create the container executing lxc-create On 2012/03/14 10:21:25, niemeyer wrote: > On 2012/03/14 09:31:26, rog wrote: > > // The useful error message is in the first > > // line of the LXC command's output. The other > > // lines contain the usage message which > > // we don't want to see. > > Roger, we already talked about that in a different context. Arbitrarily > selecting a line out of some commands output is not a good approach, for the > same reasons we debated before. i appreciate that point of view, but when we can experimentally verify that all the useful information is in the first line of the error output, do we really want an error message that looks like this? lxc-start: option requires an argument -- 'n' Usage: lxc-start --name=NAME -- COMMAND lxc-start start COMMAND in specified container NAME Options : -n, --name=NAME NAME for name of the container -d, --daemon daemonize the container -f, --rcfile=FILE Load configuration file FILE -c, --console=FILE Set the file output for the container console -s, --define KEY=VAL Assign VAL to configuration variable KEY Common options : -o, --logfile=FILE Output log to FILE instead of stderr -l, --logpriority=LEVEL Set log priority to LEVEL -q, --quiet Don't produce any output -?, --help Give this help list --usage Give a short usage message Mandatory or optional arguments to long options are also mandatory or optional for any corresponding short options. See the lxc-start man page for further information.
Sign in to reply to this message.
On 2012/03/14 10:49:30, rog wrote: > https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go > File environs/local/lxc.go (right): > > https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode18 > environs/local/lxc.go:18: // Create the container executing lxc-create > On 2012/03/14 10:21:25, niemeyer wrote: > > On 2012/03/14 09:31:26, rog wrote: > > > // The useful error message is in the first > > > // line of the LXC command's output. The other > > > // lines contain the usage message which > > > // we don't want to see. > > > > Roger, we already talked about that in a different context. Arbitrarily > > selecting a line out of some commands output is not a good approach, for the > > same reasons we debated before. > > i appreciate that point of view, but when we can experimentally verify that all > the useful information is in the first line of the error output, do we really > want an error message that looks like this? > > > lxc-start: option requires an argument -- 'n' > Usage: lxc-start --name=NAME -- COMMAND > > lxc-start start COMMAND in specified container NAME > > Options : > -n, --name=NAME NAME for name of the container > -d, --daemon daemonize the container > -f, --rcfile=FILE Load configuration file FILE > -c, --console=FILE Set the file output for the container console > -s, --define KEY=VAL Assign VAL to configuration variable KEY > > Common options : > -o, --logfile=FILE Output log to FILE instead of stderr > -l, --logpriority=LEVEL Set log priority to LEVEL > -q, --quiet Don't produce any output > -?, --help Give this help list > --usage Give a short usage message > > Mandatory or optional arguments to long options are also mandatory or optional > for any corresponding short options. > > See the lxc-start man page for further information. another option might be to truncate before the first line beginning with "[uU]sage:"
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode18 environs/local/lxc.go:18: // Create the container executing lxc-create On 2012/03/14 10:49:30, rog wrote: > i appreciate that point of view, but when we can We already went over that discussion and I already explained in practice why this isn't fine to do. I won't go over the trouble of proving to you again, but commands can fail with multiple lines, and the lxc command set is no exception. It's fine to strip all-but-the-first-line if the second line starts with "Usage:". It is NOT fine to grab the first line and drop the rest blindly as suggested.
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode58 environs/local/lxc.go:58: return strings.Fields(string(output)) this isn't a problem because the lxc not allow space in the name of a container.
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode58 environs/local/lxc.go:58: return strings.Fields(string(output)) On 2012/03/14 14:16:46, andrewsmedina wrote: > this isn't a problem because the lxc not allow space in the name of a container. if that were true, it would be ok. but experimentally, this is what happens: % sudo lxc-create -n 'a b' 'a b' created % lxc-ls a foo % lxc-start --daemon -n 'a' lxc-start: no configuration file for '/sbin/init' (may crash the host) %
Sign in to reply to this message.
https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go File environs/local/lxc.go (right): https://codereview.appspot.com/5764043/diff/9001/environs/local/lxc.go#newcode58 environs/local/lxc.go:58: return strings.Fields(string(output)) On 2012/03/14 14:30:41, rog wrote: > % sudo lxc-create -n 'a b' > 'a b' created > % lxc-ls > a foo This looks like a bug in lxc tools. Please feel free to report it, Roger, but we don't have to fix ourselves. The man page is clear: -n, --name=NAME Use container identifier NAME. The container identifier format is an alphanumeric string. The approach from Andrews is fine.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
I'm submitting this, thanks Andrews.
Sign in to reply to this message.
*** Submitted: environs/local: initial lxc commands wrapper R=niemeyer, rog CC= https://codereview.appspot.com/5764043
Sign in to reply to this message.
|
