Container Manager Interface

Container Manager Interface

Add StartContainer and StopContainer to
containerManger interface and implement
for lxc and kvm.

https://code.launchpad.net/~waigani/juju-core/add-start-stop-to-container-manager-interface/+merge/211832

(do not edit description out of merge proposal)

[waigani, 2014-03-19 22:07:04]

Please take a look.

[waigani, 2014-03-21 08:31:59]

Please take a look.

[fwereade, 2014-03-24 09:51:56]

I'm a bit nervous about the consistency of start/stop vs create/destroy, because
the InstanceBroker interface has different meanings. Please make that interface
consistent -- assuming you can do so completely mechanically, LGTM; the rest are
just trivials.

https://codereview.appspot.com/77970043/diff/20001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/interface.go#new...
container/interface.go:31: StartContainer(instance.Instance) error
Blank lines between methods, please, it's getting harder to read.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (left):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:20: }
One concern about caching these values: how will they hold up if someone messes
with our containers out-of-band? I'm mainly worried about the local provider
here.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:19: exists *bool
and between fields as well, actually

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:96: logger.Debugf("Start %s", c.name)
s/Start/start/

[waigani, 2014-03-24 22:37:21]

Please take a look.

https://codereview.appspot.com/77970043/diff/20001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/interface.go#new...
container/interface.go:31: StartContainer(instance.Instance) error
On 2014/03/24 09:51:56, fwereade wrote:
> Blank lines between methods, please, it's getting harder to read.

Done.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (left):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:20: }
On 2014/03/24 09:51:56, fwereade wrote:
> One concern about caching these values: how will they hold up if someone
messes
> with our containers out-of-band? I'm mainly worried about the local provider
> here.

Good point. What is the cache actually giving us? How often do we call the
container methods Start, Stop, Create and Destroy and does the expense of
calling down to virsh and uvt-kvm each time have any significant consequence? <-
Questions for thumper.

Currently the container methods reset the values to nil after a successful cache
check. We could also reset the cache to nil if the cache is not as expected and
then rerun the check. But really, this is just circumventing the cache.

On the other hand, what is our policy if people do mess with containers
out-of-band? Is that something we need to seamlessly recover from? We could
reset the cache to nil if the container methods error out for any reason, so the
next time they are called it will work. BUT this is just adding more complexity
at the expense of user experience. 

Sigh, thinking out loud here ... ditching the cache seems simplest to me.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:19: exists *bool
On 2014/03/24 09:51:56, fwereade wrote:
> and between fields as well, actually

Done.

https://codereview.appspot.com/77970043/diff/20001/container/kvm/container.go...
container/kvm/container.go:96: logger.Debugf("Start %s", c.name)
On 2014/03/24 09:51:56, fwereade wrote:
> s/Start/start/

Done.

[waigani, 2014-03-25 10:00:16]

Please take a look.

[waigani, 2014-03-25 23:03:39]

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
File container/kvm/mock/mock-kvm_test.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
container/kvm/mock/mock-kvm_test.go:181: // first.Start()
The commented lines cause a go routine timeout. I'm not sure why?

[thumper, 2014-03-26 02:25:57]

NOT LGTM yet

https://codereview.appspot.com/77970043/diff/60001/container/interface.go
File container/interface.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/interface.go#new...
container/interface.go:35: // StopCOntainer stops a container
s/StopCOntainer/StopContainer/

StopContainer stops a started container?
What if the container is already stopped? Do we error?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go
File container/kvm/container.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go...
container/kvm/container.go:21: exists *bool
We should move from a tree state boolean (which was sufficient), to an actual
state.

type kvmState string
var (
  unknown kvmState = "unknown"
  created kvmState = "created"
  running kvmState = "running"
  ...
)

https://codereview.appspot.com/77970043/diff/60001/container/kvm/container.go...
container/kvm/container.go:41: c.exists = nil
wat?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go
File container/kvm/interface.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go...
container/kvm/interface.go:32: Destroy() error
What happens if the container has stopped?

https://codereview.appspot.com/77970043/diff/60001/container/kvm/interface.go...
container/kvm/interface.go:34: Start() error
need at least minimal docstrings on the methods

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go
File container/kvm/kvm.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newco...
container/kvm/kvm.go:82: return nil
pretty sure that start container shouldn't do nothing

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newco...
container/kvm/kvm.go:88: return kvmContainer.Stop()
doc string should at least say what happens in the error conditions.

what if the container doesn't exist, what if it is already stopped, etc

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newco...
container/kvm/kvm.go:131: if err := kvmContainer.Create(startParams); err != nil
{
Worth noting that that Create both creates and starts the container.

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm.go#newco...
container/kvm/kvm.go:141: if err := kvmContainer.Destroy(); err != nil {
equally worth noting that Destroy both stops and destroys the container.

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm_test.go
File container/kvm/kvm_test.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/kvm_test.go#...
container/kvm/kvm_test.go:113: c.Assert(containertesting.StopContainer(c,
s.manager, instance), gc.IsNil)
What about starting a started container, or stopping a stopped container? the
mock should as closely as possible mirror what the virsh commands return

https://codereview.appspot.com/77970043/diff/60001/container/kvm/libvirt.go
File container/kvm/libvirt.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/libvirt.go#n...
container/kvm/libvirt.go:112: func StartMachine(hostname string) error {
doc strings, and expected behaviour...

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
File container/kvm/mock/mock-kvm.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
container/kvm/mock/mock-kvm.go:69: exists  bool
should mirror the kvmContainer state

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
File container/kvm/mock/mock-kvm_test.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
container/kvm/mock/mock-kvm_test.go:78: err = container.Start()
again, test starting started containers, stopping stopped containers etc.

https://codereview.appspot.com/77970043/diff/60001/container/kvm/mock/mock-kv...
container/kvm/mock/mock-kvm_test.go:181: // first.Start()
On 2014/03/25 23:03:39, waigani wrote:
> The commented lines cause a go routine timeout. I'm not sure why?

increase the buffer in the channel above

https://codereview.appspot.com/77970043/diff/60001/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:119: directory, err := container.NewDirectory(manager.name)
wrong name...

should just be "name"

https://codereview.appspot.com/77970043/diff/60001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:126: //lxcContainer.SetLogFile(filepath.Join(directory,
"container.log"), golxc.LogDebug)
why did you comment this out?

https://codereview.appspot.com/77970043/diff/60001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:238: manager.StartContainer(&lxcInstance{lxcContainer,
name})
don't create a new instance here for the parameter, create it before the
function call, and pass it in, and return the same instance.

https://codereview.appspot.com/77970043/diff/60001/container/testing/common.go
File container/testing/common.go (right):

https://codereview.appspot.com/77970043/diff/60001/container/testing/common.g...
container/testing/common.go:27: 
Sorry, but these two methods are pointless.

[thumper, 2014-03-26 02:27:50]

On 2014/03/24 09:51:56, fwereade wrote:
> I'm a bit nervous about the consistency of start/stop vs create/destroy,
because
> the InstanceBroker interface has different meanings. Please make that
interface
> consistent -- assuming you can do so completely mechanically, LGTM; the rest
are
> just trivials.

The InstanceBroker is aimed at the provisioner, and for the provider to support
the methods
that will be called by the provisioner.

We have the lxcBroker and kvmBroker to do this mapping for us in the provisioner
code.

At the container level, it makes sense to have Create, Start, Stop, and Destroy.
This is
quite independent from the Broker.

[fwereade, 2014-03-27 12:32:39]

On 2014/03/26 02:27:50, thumper wrote:
> On 2014/03/24 09:51:56, fwereade wrote:
> > I'm a bit nervous about the consistency of start/stop vs create/destroy,
> because
> > the InstanceBroker interface has different meanings. Please make that
> interface
> > consistent -- assuming you can do so completely mechanically, LGTM; the rest
> are
> > just trivials.
> 
> The InstanceBroker is aimed at the provisioner, and for the provider to
support
> the methods
> that will be called by the provisioner.
> 
> We have the lxcBroker and kvmBroker to do this mapping for us in the
provisioner
> code.
> 
> At the container level, it makes sense to have Create, Start, Stop, and
Destroy.
> This is
> quite independent from the Broker.

As discussed live: I'd like the Start/Stop/Create/Destroy vocabulary to have a
consistent meaning across the codebase -- at least when talking about such
similar things as different implementations of Instance. If that means the
Broker methods start talking Create/Destroy, that works just fine. I'm not
asking for any sort of rearchitecting, just naming consistency :).

[waigani, 2014-03-31 06:15:26]

Please take a look.

[waigani, 2014-03-31 06:20:49]

On 2014/03/31 06:15:26, waigani wrote:
> Please take a look.

I missed container/lxc/lxc.go:238, passing in instance. I'll follow up with
that. Should I make a new branch for updating the Broker interface?

[waigani, 2014-03-31 22:44:15]

Please take a look.