LXC AutoRestart Setting

LXC AutoRestart Setting

lxc.containerManager gains “autorestart bool”, 
and sets that from container.ManagerConfig, if
not in container.ManagerConfig, then manager
default is true.

https://code.launchpad.net/~waigani/juju-core/lxc-autorestart-setting/+merge/210117

Requires: https://code.launchpad.net/~waigani/juju-core/managers-should-warn-for-any-unknown-options/+merge/210114

(do not edit description out of merge proposal)

[waigani, 2014-03-10 03:43:39]

Please take a look.

[thumper, 2014-03-10 03:56:48]

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode47
container/lxc/lxc.go:47: autorestart bool
normally camel case: "autoRestart"

yes logdir is wrong

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode80
container/lxc/lxc.go:80: func (manager *containerManager) AutoRestart() bool {
this doesn't need to be exported

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc_test.go#newc...
container/lxc/lxc_test.go:67: // How do we test autorestart ?
I have a branch that we probably want to land first, as I have tests for this.

[hduran, 2014-03-17 17:42:23]

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode70
container/lxc/lxc.go:70: }
Wouldn't this whole block be equivalent to:

autoRestart := conf["autoRestart"] == "" || conf["autoRestart"] == "true"

it seems a bit odd to me to do if true then true if false then false, but it is
just an opinion.

[waigani, 2014-03-18 00:37:24]

Please take a look.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode47
container/lxc/lxc.go:47: autorestart bool
On 2014/03/10 03:56:48, thumper wrote:
> normally camel case: "autoRestart"
> 
> yes logdir is wrong

Done.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode70
container/lxc/lxc.go:70: }
On 2014/03/17 17:42:23, hduran wrote:
> Wouldn't this whole block be equivalent to:
> 
> autoRestart := conf["autoRestart"] == "" || conf["autoRestart"] == "true"
> 
> it seems a bit odd to me to do if true then true if false then false, but it
is
> just an opinion.

Done. Much better. I didn't know you could do that in go, but now I think about
it - of course you can.

https://codereview.appspot.com/73390043/diff/1/container/lxc/lxc.go#newcode80
container/lxc/lxc.go:80: func (manager *containerManager) AutoRestart() bool {
On 2014/03/10 03:56:48, thumper wrote:
> this doesn't need to be exported

Done. It was a leftover from debugging.

[waigani, 2014-03-18 17:54:34]

Please take a look.

[thumper, 2014-03-19 04:01:23]

I think we should hold off on the local provider changes until suspend and
resume are done.

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc.go
File container/lxc/lxc.go (right):

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:93: autoRestart := conf["auto-restart"] == "" ||
conf["auto-restart"] == "true"
autoRestart, err := strconv.ParseBool(conf.PopValue("auto-restart"))
if err != nil {
  // An empty string is an error.
  autoRestart = true
}

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc.go#newco...
container/lxc/lxc.go:97: logger.Warningf(`Found unused config option with key:
"%v" and value: "%v"`, k, v)
Please delete this.  It is now handled by the

  conf.WarnAboutUnused()

below.

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc_test.go
File container/lxc/lxc_test.go (right):

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc_test.go#...
container/lxc/lxc_test.go:346: s.autoRestartFalse = true
Don't set here and reset at the end, as an assert stops the end from happening.
Instead use PatchValue...

  s.PatchValue(&s.autoRestartFalse, true)

https://codereview.appspot.com/73390043/diff/40001/container/lxc/lxc_test.go#...
container/lxc/lxc_test.go:355: s.autoRestartFalse = true
ditto.

https://codereview.appspot.com/73390043/diff/40001/provider/local/config.go
File provider/local/config.go (right):

https://codereview.appspot.com/73390043/diff/40001/provider/local/config.go#n...
provider/local/config.go:121: value, _ := c.attrs["lxc-auto-restart"].(bool)
I think we want a general auto-restart, not just an lxc- one. We can do the same
for kvm.  Which leads me to point out that you need to fix kvm.

Although I don't think we should add the ability to set it in the local provider
until suspend and resume are done.