Merge lp:~waigani/juju-core/lxc-autorestart-setting into lp:~go-bot/juju-core/trunk

[Jesse Meek]

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://codereview.appspot.com/73390043/

[Jesse Meek (waigani)]

Reviewers: mp+210117_code.launchpad.net,

Message:
Please take a look.

Description:
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)

Please review this at https://codereview.appspot.com/73390043/

Affected files (+26, -3 lines):
   A [revision details]
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go

Index: [revision details]
=== added file '[revision details]'
--- [revision details] 2012-01-01 00:00:00 +0000
+++ [revision details] 2012-01-01 00:00:00 +0000
@@ -0,0 +1,2 @@
+Old revision: <email address hidden>
+New revision: <email address hidden>

Index: container/lxc/lxc.go
=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go 2014-03-10 02:51:31 +0000
+++ container/lxc/lxc.go 2014-03-10 03:33:38 +0000
@@ -42,8 +42,9 @@
  }

  type containerManager struct {
- name string
- logdir string
+ name string
+ logdir string
+ autorestart bool
  }

  // containerManager implements container.Manager.
@@ -63,11 +64,21 @@
   if logDir == "" {
    logDir = agent.DefaultLogDir
   }
+ autoRestart := false
+ if conf["autoRestart"] == "" || conf["autoRestart"] == "true" {
+ autoRestart = true
+ }
+ delete(conf, "autoRestart")
+
   for k, v := range conf {
    logger.Warningf(`Found unused config option with key: "%v" and
value: "%v"`, k, v)
   }

- return &containerManager{name: name, logdir: logDir}, nil
+ return &containerManager{name: name, logdir: logDir, autorestart:
autoRestart}, nil
+}
+
+func (manager *containerManager) AutoRestart() bool {
+ return manager.autorestart
  }

  func (manager *containerManager) StartContainer(

Index: container/lxc/lxc_test.go
=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go 2014-03-10 02:51:31 +0000
+++ container/lxc/lxc_test.go 2014-03-10 03:33:38 +0000
@@ -58,6 +58,16 @@
   c.Assert(c.GetTestLog(), gc.Matches, `^.*WARNING juju.container.lxc Found
unused config option with key: "shazam" and value: "Captain Marvel"\n*`)
  }

+func (*LxcSuite) TestAutoRestartDefaultTrue(c *gc.C) {
+ _, err := lxc.NewContainerManager(container.ManagerConfig{
+ container.ConfigName: "startMeUp",
+ })
+ c.Assert(err, gc.IsNil)
+
+ // How do we test autorestart ?
+ // c.Assert(manager.somehowGetAutoRestart(?), gc.Equals, true)
+}
+
  func (s *LxcSuite) TestStartContainer(c *gc.C) {
   manager := s.makeManager(c, "test")
   instance := containertesting.StartContainer(c, manager, "1/lxc/0")

[Tim Penhey (thumper)]

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

https://codereview.appspot.com/73390043/

[Horacio Durán (hduran-8)]

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.

https://codereview.appspot.com/73390043/

[lp:~waigani/juju-core/lxc-autorestart-setting]

2400. By Jesse Meek on 2014-03-17

cosmetic change to if statement syntax

2401. By Jesse Meek on 2014-03-18

merged trunk

[Jesse Meek (waigani)]

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.

https://codereview.appspot.com/73390043/

[lp:~waigani/juju-core/lxc-autorestart-setting]

2402. By Jesse Meek on 2014-03-18

added tests

2403. By Jesse Meek on 2014-03-18

merged trunk

[Jesse Meek (waigani)]

Please take a look.

https://codereview.appspot.com/73390043/

[Tim Penhey (thumper)]

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#newcode93
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#newcode97
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#newcode346
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#newcode355
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#newcode121
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.

https://codereview.appspot.com/73390043/

[William Reade (fwereade)]

re thumper's comment, setting WIP

Unmerged revisions

2403. By Jesse Meek on 2014-03-18

merged trunk

2402. By Jesse Meek on 2014-03-18

added tests

2401. By Jesse Meek on 2014-03-18

merged trunk

2400. By Jesse Meek on 2014-03-17

cosmetic change to if statement syntax

2399. By Jesse Meek on 2014-03-10

LXC AutoRestart Setting

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

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2014-03-17 22:21:59 +0000
+++ container/lxc/lxc.go	2014-03-18 17:52:53 +0000
@@ -68,6 +68,7 @@
 type containerManager struct {
 	name              string
 	logdir            string
+	autoRestart       bool
 	createWithClone   bool
 	useAUFS           bool
 	backingFilesystem string
@@ -88,6 +89,14 @@
 	if logDir == "" {
 		logDir = agent.DefaultLogDir
 	}
+
+	autoRestart := conf["auto-restart"] == "" || conf["auto-restart"] == "true"
+	delete(conf, "auto-restart")
+
+	for k, v := range conf {
+		logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
+	}
+
 	// Explicitly ignore the error result from ParseBool.
 	// If it fails to parse, the value is false, and this suits
 	// us fine.
@@ -101,11 +110,13 @@
 		// call it 'unknown'.
 		backingFS = "unknown"
 	}
+
 	logger.Tracef("backing filesystem: %q", backingFS)
 	conf.WarnAboutUnused()
 	return &containerManager{
 		name:              name,
 		logdir:            logDir,
+		autoRestart:       autoRestart,
 		createWithClone:   useClone,
 		useAUFS:           useAUFS,
 		backingFilesystem: backingFS,
@@ -198,8 +209,10 @@
 		}
 		logger.Tracef("lxc container created")
 	}
-	if err := autostartContainer(name); err != nil {
-		return nil, nil, err
+	if manager.autoRestart {
+		if err := autostartContainer(name); err != nil {
+			return nil, nil, err
+		}
 	}
 	if err := mountHostLogDir(name, manager.logdir); err != nil {
 		return nil, nil, err

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2014-03-17 01:08:31 +0000
+++ container/lxc/lxc_test.go	2014-03-18 17:52:53 +0000
@@ -36,9 +36,10 @@
 type LxcSuite struct {
 	lxctesting.TestSuite
 
-	events   chan mock.Event
-	useClone bool
-	useAUFS  bool
+	events           chan mock.Event
+	useClone         bool
+	useAUFS          bool
+	autoRestartFalse bool
 }
 
 var _ = gc.Suite(&LxcSuite{})
@@ -99,6 +100,10 @@
 	if s.useAUFS {
 		params["use-aufs"] = "true"
 	}
+	if s.autoRestartFalse {
+		params["auto-restart"] = "false"
+	}
+
 	manager, err := lxc.NewContainerManager(params)
 	c.Assert(err, gc.IsNil)
 	return manager
@@ -337,6 +342,37 @@
 	instancetest.MatchInstances(c, result, bar1, bar2)
 }
 
+func (s *LxcSuite) TestStartContainerNoAutostart(c *gc.C) {
+	s.autoRestartFalse = true
+	manager := s.makeManager(c, "test")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
+	autostartLink := lxc.RestartSymlink(string(instance.Id()))
+	c.Assert(autostartLink, jc.SymlinkDoesNotExist)
+	s.autoRestartFalse = false
+}
+
+func (s *LxcSuite) TestStartContainerNoAutostartNoRestartDir(c *gc.C) {
+	s.autoRestartFalse = true
+	err := os.Remove(s.RestartDir)
+	c.Assert(err, gc.IsNil)
+
+	manager := s.makeManager(c, "test")
+	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
+	name := string(instance.Id())
+	autostartLink := lxc.RestartSymlink(name)
+	config, err := ioutil.ReadFile(lxc.ContainerConfigFilename(name))
+	c.Assert(err, gc.IsNil)
+	expected := `
+lxc.network.type = veth
+lxc.network.link = nic42
+lxc.network.flags = up
+lxc.mount.entry=/var/log/juju var/log/juju none defaults,bind 0 0
+`
+	c.Assert(string(config), gc.Equals, expected)
+	c.Assert(autostartLink, jc.DoesNotExist)
+	s.autoRestartFalse = false
+}
+
 func (s *LxcSuite) TestStartContainerAutostarts(c *gc.C) {
 	manager := s.makeManager(c, "test")
 	instance := containertesting.StartContainer(c, manager, "1/lxc/0")

=== modified file 'provider/local/config.go'
--- provider/local/config.go	2014-03-18 03:51:15 +0000
+++ provider/local/config.go	2014-03-18 17:52:53 +0000
@@ -117,6 +117,11 @@
 	return value
 }
 
+func (c *environConfig) lxcAutoRestart() bool {
+	value, _ := c.attrs["lxc-auto-restart"].(bool)
+	return value
+}
+
 func (c *environConfig) createDirs() error {
 	for _, dirname := range []string{
 		c.storageDir(),

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-03-18 03:51:15 +0000
+++ provider/local/environ.go	2014-03-18 17:52:53 +0000
@@ -220,6 +220,7 @@
 	if containerType == instance.LXC {
 		managerConfig["use-clone"] = strconv.FormatBool(env.config.lxcClone())
 		managerConfig["use-aufs"] = strconv.FormatBool(env.config.lxcCloneAUFS())
+		managerConfig["auto-restart"] = strconv.FormatBool(env.config.lxcAutoRestart())
 	}
 	env.containerManager, err = factory.NewContainerManager(
 		containerType, managerConfig)