Merge lp://staging/~themue/golxc/001-added-sudo-and-first-test into lp://staging/golxc

lots of comments but nothing major.
i'm not familiar with lxc, so all superficial, i'm afraid.

https://codereview.appspot.com/6852065/diff/6001/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode9
golxc.go:9: // Frank Mueller <email address hidden>
do we need individual attributions?

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode29
golxc.go:29: ContainerStarting = 1 << iota
you can omit the "1 << iota" from here and the following constants.

however, ISTM that there's no particular reason for these to be bit
masks. the only place that we make use of their bitmask nature is in the
unexported Container.wait, and it would be just as easy (actually
easier, i think) to create a bitmask explicitly there.

also, how about:

type State uint

and using that as the state type, perhaps renaming the constants to
StateRunning, etc.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode52
golxc.go:52: var go2state = map[int]string{
if the states weren't bitmasks, this could just be []string and you'd
get the same result.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode82
golxc.go:82: var go2log = map[int]string{
[]string

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode111
golxc.go:111: outbuf, _, err := run("lxc-ls", args...)
run("lxc-ls", "-1")

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode113
golxc.go:113: log.Fatalf("cannot list containers: %v", err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode117
golxc.go:117: containers := []*Container{}
containers := make([]*Container, len(names)) ?

and then containers[i] = New(name) below.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode142
golxc.go:142: "-t", template,
you could add "--" here unconditionally.
then the below lines could be simply:

args = append(args, tmplArgs)

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode150
golxc.go:150: log.Fatalf("cannot create container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode173
golxc.go:173: log.Fatalf("cannot start container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode189
golxc.go:189: log.Fatalf("cannot stop container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode212
golxc.go:212: log.Fatalf("cannot clone container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode231
golxc.go:231: log.Fatalf("cannot freeze container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode250
golxc.go:250: log.Fatalf("cannot unfreeze container %q: %v", c.name,
err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode269
golxc.go:269: log.Fatalf("cannot destroy container %q: %v", c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode282
golxc.go:282: log.Fatalf("cannot retrieve container info for %q: %v",
c.name, err)
d

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode289
golxc.go:289: return -1, -1, err
a m...

LGTM apart from the error messages. i think we don't want to discard the
important information printed to stderr.

https://codereview.appspot.com/6852065/diff/6001/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode9
golxc.go:9: // Frank Mueller <email address hidden>
On 2012/11/20 16:34:04, TheMue wrote:
> On 2012/11/20 15:20:11, rog wrote:
> > do we need individual attributions?

> Got it from packages like goamz. But personally don't need it.

i think goamz is different because it really is almost all gustavo's
work. YMMV, whatever.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode370
golxc.go:370: log.Fatalf("error executing %q: %v",
strings.Join(cmd.Args, " "), err)
On 2012/11/20 16:34:04, TheMue wrote:
> On 2012/11/20 15:20:11, rog wrote:
> > d
> > but i wonder whether the error message here should actually
incorporate the
> > error printed to stderr. nothing that calls run actually uses the
stderr
> > returned, and the error returned by cmd.Run is, in most cases,
almost
> > meaningless.

> The err here is an error of Command executing the command, not the
stderr of the
> executed command. I changed it to mode it more clear. So far we have
no usage
> for stderr, but it's also not everything implemented. If there stays
no need it
> will be removed.

my point is that if a command fails, the only message we'll get will be
"exit status 1" or whatever, and the useful error message that the
command has printed to stderr will be discarded.

i think we should put at least some of that information into the err
return.

https://codereview.appspot.com/6852065/diff/11001/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode35
golxc.go:35: // LogLevel represents a containers log level.
s/containers/container's/

https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode86
golxc.go:86: c.logLevel = logLevel
is it really worth having the setter method here?
could we just export the relevant fields?

https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode273
golxc.go:273: func (c *Container) wait(states ...State) error {
given that nothing is actually making use of the functionality yet, we
could just have wait(state State) and not bother with combining states
at all. just a then this function is only 5 lines. just a thought.

https://codereview.appspot.com/6852065/

Not sure about the tests -- it looks like there's plenty of opportunity
from pollution from failures. Is there some motivation for this
behaviour that I've missed?

Also, it kinda looks as though the tests will fail if your system is
already running a container. Can you see any way around this?

https://codereview.appspot.com/6852065/diff/7007/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/7007/golxc.go#newcode217
golxc.go:217: _, _, err := run("lxc-wait", "-n", c.name, "-s",
combinedStates)
I guess LXC will complain about impossible state combinations?

https://codereview.appspot.com/6852065/diff/7007/golxc.go#newcode257
golxc.go:257: // String returns a short information about the container.
s/a short/brief/

perhaps?

https://codereview.appspot.com/6852065/diff/7007/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6852065/diff/7007/golxc_test.go#newcode20
golxc_test.go:20: lc.Destroy()
Shouldn't these be destroyed inline via defer?

https://codereview.appspot.com/6852065/

A few more comments...

https://codereview.appspot.com/6852065/diff/6002/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode180
golxc.go:180: // Unfreeze thaws all a froozen container's processes.
s/rooz/roz/

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode186
golxc.go:186: return fmt.Errorf("container %q is not froozen", c.name)
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode219
golxc.go:219: switch len(states) {
I'm not sure the switch is a big win over a simple err return when no
states are supplied -- the one-state case may be more efficient but I'm
not sure it really aids clarity. Up to you though.

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode271
golxc.go:271: // String returns a brief information about the container.
s/a //

?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode25
golxc_test.go:25: c.Assert(lc.IsConstructed(), Equals, true)
Shouldn't we defer the Destroy at this point?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode40
golxc_test.go:40: defer lc1.Destroy()
This should be either 1 or 2 lines down, shouldn't it? And surely we
should be asserting no error?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode74
golxc_test.go:74: defer lc.Destroy()
Handle errors? (after checking no error on preceding Create)

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode87
golxc_test.go:87: defer lc1.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode91
golxc_test.go:91: defer lc2.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode116
golxc_test.go:116: defer lc.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode137
golxc_test.go:137: defer lc.Destroy()
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode138
golxc_test.go:138: go lc.Start("", "")
Destroy works even if the container is running?

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode161
golxc_test.go:161: // Test that a not running container can't be
froozen.
s/rooz/roz/

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode171
golxc_test.go:171: // Test that a non-existing container can't be
froozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode179
golxc_test.go:179: // Test that a non-existing container can't be
unfroozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode186
golxc_test.go:186: func (l *LXCSuite) TestUnfreezeNotFroozen(c *C) {
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode187
golxc_test.go:187: // Test that a running container can't be unfroozen.
ditto

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode193
golxc_test.go:193: defer lc.Stop()
error checking

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode195
golxc_test.go:195: c.Assert(err, ErrorMatches, "container .* is not
froozen...

LGTM!

https://codereview.appspot.com/6852065/

looks good to me apart from (still) the error messages.
a few minor suggestions in addition to that.

https://codereview.appspot.com/6852065/diff/6002/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode219
golxc.go:219: switch len(states) {
On 2012/11/22 14:35:57, fwereade wrote:
> I'm not sure the switch is a big win over a simple err return when no
states are
> supplied -- the one-state case may be more efficient but I'm not sure
it really
> aids clarity. Up to you though.

i think i agree here. we don't mind about a single non-escaping
allocation. i'd lose the "case 1" and make the "case 0" into an if that
returns the error.

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode271
golxc.go:271: // String returns a brief information about the container.
On 2012/11/22 14:35:57, fwereade wrote:
> s/a //

> ?

s/a brief // ?

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode277
golxc.go:277: return fmt.Sprintf("container %q has state %q and pid %d",
c.name, state, pid)
i'd be tempted to go a little more terse than this.

something like this, as an example, perhaps?

container "foo" (STARTING, pid 2353)

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode304
golxc.go:304: return outbuf.String(), errbuf.String(), err
i'm still not happy with this. if we call Container.Start and get the
error "exit status 1", that's an extremely unhelpful error. for an
example of what might be a reasonable approach, see how lbox deals with
error returns from running bzr. (search for bzrError in the lbox
source). you might want to tailor that for the way that lxc tends to
print error messages.

https://codereview.appspot.com/6852065/diff/9003/golxc_test.go
File golxc_test.go (right):

https://codereview.appspot.com/6852065/diff/9003/golxc_test.go#newcode12
golxc_test.go:12: type LXCSuite struct{}
i wonder if it might be worth Skipping the suite (with a warning,
perhaps) if we're not running as root.

https://codereview.appspot.com/6852065/

much better, but still not *quite* there.

i'd like to see at least one test that verifies the new error logic.

https://codereview.appspot.com/6852065/diff/5003/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/5003/golxc.go#newcode29
golxc.go:29: if strings.HasPrefix(e.StdErr, e.Name) {
perhaps slightly more robust to do: HasPrefix(e.StdErr, e.Name + ": ")
and more obvious to the reader what the extra 2 chars are too.

also, what if the error output is more than one line? i'm not sure if
it's best to discard all but the first line in that case, or remove the
prefix from all lines. you could do the latter and join the lines with
semicolons, i suppose. might be worth discussing with gustavo, as he has
had definite opinions on this subject in the past.

https://codereview.appspot.com/6852065/

LGTM with one suggestion below.

i'm worried i might have led you up the garden path with this one though
- please check with niemeyer that this logic is acceptable.

thanks for bearing with me!

https://codereview.appspot.com/6852065/diff/18/golxc.go
File golxc.go (right):

https://codereview.appspot.com/6852065/diff/18/golxc.go#newcode326
golxc.go:326: if len(output) == 0 {
i think this might be better as something like:

     e := &Error{name, err, nil}
     for _, l := range strings.Split(string(output), "\n") {
         if l != "" {
             e.Output = append(e.Output, l)
         }
     }
     return e

i think that's equivalent, except it also removes all blank lines, which
could be considered a good thing.

if you wanted, this would also be a good place to strip the command
prefix from all the lines.

https://codereview.appspot.com/6852065/