golxc: add first container tests

golxc: add first container tests

The package now has the first functions for the
testing of the container. Two external scripts
help to perform the tests as a root.

https://code.launchpad.net/~themue/golxc/001-added-sudo-and-first-test/+merge/134956

(do not edit description out of merge proposal)

[TheMue, 2012-11-19 16:14:59]

Please take a look.

[TheMue, 2012-11-20 07:45:07]

Please take a look.

[TheMue, 2012-11-20 09:47:37]

Please take a look.

[rog, 2012-11-20 15:20:11]

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 <frank.mueller@canonical.com>
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 more informative error message which mentions pid might be good here.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode333
golxc.go:333: func (c *Container) wait(status int) error {
if state was not a bitmask, this could be something like:

for i := State(0); i < StateMax; i++ {
    if status & (1<<i) {
         lxcStatus = append(lxcStatus, i.String())
    }
}

(i've also assumed a String method on State)

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode355
golxc.go:355: _, _, err := run("lxc-wait", args...)
inline the args?

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode370
golxc.go:370: log.Fatalf("error executing %q: %v", strings.Join(cmd.Args, " "),
err)
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.

https://codereview.appspot.com/6852065/diff/6001/golxc_test.sh
File golxc_test.sh (right):

https://codereview.appspot.com/6852065/diff/6001/golxc_test.sh#newcode2
golxc_test.sh:2: sudo ./golxc_test_sudo.sh $GOPATH $GOPKGDIR
sudo sh -c "
    export GOMAXPROCS=\"$GOMAXPROCS\"
    export GOPATH=\"$GOPATH\"
    export GOROOT=\"$GOROOT\"
    export PATH=\"$PATH\"
    go test
"

and lose golxc_test_sudo.sh ?

[TheMue, 2012-11-20 16:34:04]

Please take a look.

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 <frank.mueller@canonical.com>
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.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode29
golxc.go:29: ContainerStarting = 1 << iota
On 2012/11/20 15:20:11, rog wrote:
> 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.

Done. Thx for this important hint, simplified it a different way, but a lot.

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

Done. See above.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode82
golxc.go:82: var go2log = map[int]string{
On 2012/11/20 15:20:11, rog wrote:
> []string

Done. See above.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode111
golxc.go:111: outbuf, _, err := run("lxc-ls", args...)
On 2012/11/20 15:20:11, rog wrote:
> run("lxc-ls", "-1")

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode113
golxc.go:113: log.Fatalf("cannot list containers: %v", err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode117
golxc.go:117: containers := []*Container{}
On 2012/11/20 15:20:11, rog wrote:
> containers := make([]*Container, len(names)) ?
> 
> and then containers[i] = New(name) below.

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode142
golxc.go:142: "-t", template,
On 2012/11/20 15:20:11, rog wrote:
> you could add "--" here unconditionally.
> then the below lines could be simply:
> 
> args = append(args, tmplArgs)

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode150
golxc.go:150: log.Fatalf("cannot create container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode173
golxc.go:173: log.Fatalf("cannot start container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode189
golxc.go:189: log.Fatalf("cannot stop container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode212
golxc.go:212: log.Fatalf("cannot clone container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode231
golxc.go:231: log.Fatalf("cannot freeze container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode250
golxc.go:250: log.Fatalf("cannot unfreeze container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode269
golxc.go:269: log.Fatalf("cannot destroy container %q: %v", c.name, err)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

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)
On 2012/11/20 15:20:11, rog wrote:
> d

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode289
golxc.go:289: return -1, -1, err
On 2012/11/20 15:20:11, rog wrote:
> a more informative error message which mentions pid might be good here.

Done.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode333
golxc.go:333: func (c *Container) wait(status int) error {
On 2012/11/20 15:20:11, rog wrote:
> if state was not a bitmask, this could be something like:
> 
> for i := State(0); i < StateMax; i++ {
>     if status & (1<<i) {
>          lxcStatus = append(lxcStatus, i.String())
>     }
> }
> 
> (i've also assumed a String method on State)

Done. Simplified it a different way, thx.

https://codereview.appspot.com/6852065/diff/6001/golxc.go#newcode355
golxc.go:355: _, _, err := run("lxc-wait", args...)
On 2012/11/20 15:20:11, rog wrote:
> inline the args?

Done.

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 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.

https://codereview.appspot.com/6852065/diff/6001/golxc_test.sh
File golxc_test.sh (right):

https://codereview.appspot.com/6852065/diff/6001/golxc_test.sh#newcode2
golxc_test.sh:2: sudo ./golxc_test_sudo.sh $GOPATH $GOPKGDIR
On 2012/11/20 15:20:11, rog wrote:
> sudo sh -c "
>     export GOMAXPROCS=\"$GOMAXPROCS\"
>     export GOPATH=\"$GOPATH\"
>     export GOROOT=\"$GOROOT\"
>     export PATH=\"$PATH\"
>     go test
> "
> 
> and lose golxc_test_sudo.sh ?

Done. Fantastic, by far more elegant.

[rog, 2012-11-20 16:49:53]

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 <frank.mueller@canonical.com>
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.

[TheMue, 2012-11-21 08:36:56]

Please take a look.

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.
On 2012/11/20 16:49:53, rog wrote:
> s/containers/container's/

Done.

https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode86
golxc.go:86: c.logLevel = logLevel
On 2012/11/20 16:49:53, rog wrote:
> is it really worth having the setter method here?
> could we just export the relevant fields?

Done.

https://codereview.appspot.com/6852065/diff/11001/golxc.go#newcode273
golxc.go:273: func (c *Container) wait(states ...State) error {
On 2012/11/20 16:49:53, rog wrote:
> 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.

This package is intended to be a generic one also support other projects. So I'm
exporting Wait() now.

[fwereade, 2012-11-21 17:32:53]

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?

[TheMue, 2012-11-22 12:53:23]

Please take a look.

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)
On 2012/11/21 17:32:53, fwereade wrote:
> I guess LXC will complain about impossible state combinations?

To LXC there are no impossible combinations. It's an "or", so it waits until any
one of those states. But I renamed it to "waitStates" beside checking for the
case that no state is passed (which indeed is an error). Thx.

https://codereview.appspot.com/6852065/diff/7007/golxc.go#newcode257
golxc.go:257: // String returns a short information about the container.
On 2012/11/21 17:32:53, fwereade wrote:
> s/a short/brief/
> 
> perhaps?

Done.

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()
On 2012/11/21 17:32:53, fwereade wrote:
> Shouldn't these be destroyed inline via defer?

Done.

[fwereade, 2012-11-22 14:35:57]

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")
ditto

[TheMue, 2012-11-22 15:20:50]

Please take a look.

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.
On 2012/11/22 14:35:57, fwereade wrote:
> s/rooz/roz/

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc.go#newcode186
golxc.go:186: return fmt.Errorf("container %q is not froozen", c.name)
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

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.

Done.

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 //
> 
> ?

Done.

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)
On 2012/11/22 14:35:57, fwereade wrote:
> Shouldn't we defer the Destroy at this point?

Done.

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

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode74
golxc_test.go:74: defer lc.Destroy()
On 2012/11/22 14:35:57, fwereade wrote:
> Handle errors? (after checking no error on preceding Create)

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode87
golxc_test.go:87: defer lc1.Destroy()
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode91
golxc_test.go:91: defer lc2.Destroy()
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode116
golxc_test.go:116: defer lc.Destroy()
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode137
golxc_test.go:137: defer lc.Destroy()
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode138
golxc_test.go:138: go lc.Start("", "")
On 2012/11/22 14:35:57, fwereade wrote:
> Destroy works even if the container is running?

Yes, it automatically stops it.

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.
On 2012/11/22 14:35:57, fwereade wrote:
> s/rooz/roz/

Done.

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.
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

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.
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode186
golxc_test.go:186: func (l *LXCSuite) TestUnfreezeNotFroozen(c *C) {
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode187
golxc_test.go:187: // Test that a running container can't be unfroozen.
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

https://codereview.appspot.com/6852065/diff/6002/golxc_test.go#newcode195
golxc_test.go:195: c.Assert(err, ErrorMatches, "container .* is not froozen")
On 2012/11/22 14:35:57, fwereade wrote:
> ditto

Done.

[fwereade, 2012-11-22 15:36:46]

LGTM!

[rog, 2012-11-22 15:38:31]

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.

[TheMue, 2012-11-22 16:55:02]

Please take a look.

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{}
On 2012/11/22 15:38:31, rog wrote:
> i wonder if it might be worth Skipping the suite (with a warning, perhaps) if
> we're not running as root.

Done.

[rog, 2012-11-23 10:58:19]

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.

[TheMue, 2012-11-23 13:46:48]

Please take a look.

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) {
On 2012/11/23 10:58:19, rog wrote:
> 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.

Done.

[rog, 2012-11-23 14:02:55]

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.

[TheMue, 2012-11-23 15:59:19]

*** Submitted:

golxc: add first container tests

The package now has the first functions for the
testing of the container. Two external scripts
help to perform the tests as a root.

R=rog, fwereade
CC=
https://codereview.appspot.com/6852065

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 {
On 2012/11/23 14:02:55, rog wrote:
> 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.

Done.