juju-core

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

  1. lxc-autorestart-setting
  2. Merge into trunk
Proposed by Jesse Meek
Status: Work in progress
Proposed branch: lp:~waigani/juju-core/lxc-autorestart-setting
Merge into: lp:~go-bot/juju-core/trunk
Prerequisite: lp:~waigani/juju-core/managers-should-warn-for-any-unknown-options
Diff against target: 148 lines (+60/-5)
4 files modified
container/lxc/lxc.go (+15/-2)
container/lxc/lxc_test.go (+39/-3)
provider/local/config.go (+5/-0)
provider/local/environ.go (+1/-0)
To merge this branch: bzr merge lp:~waigani/juju-core/lxc-autorestart-setting
Reviewer Review Type Date Requested Status
Juju Engineering Pending
Review via email: mp+210117@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Revision history for this message
Jesse Meek (waigani) wrote :

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

Reply
Revision history for this message
Tim Penhey (thumper) wrote :

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/

Reply
Revision history for this message
Horacio Durán (hduran-8) wrote :

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/

Reply
lp:~waigani/juju-core/lxc-autorestart-setting updated
2400. By Jesse Meek

cosmetic change to if statement syntax

2401. By Jesse Meek

merged trunk

Revision history for this message
Jesse Meek (waigani) wrote :

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/

Reply
lp:~waigani/juju-core/lxc-autorestart-setting updated
2402. By Jesse Meek

added tests

2403. By Jesse Meek

merged trunk

Revision history for this message
Jesse Meek (waigani) wrote :

Please take a look.

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

Reply
Revision history for this message
Tim Penhey (thumper) wrote :

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/

Reply
Revision history for this message
William Reade (fwereade) wrote :

re thumper's comment, setting WIP

Reply

Unmerged revisions

2403. By Jesse Meek

merged trunk

2402. By Jesse Meek

added tests

2401. By Jesse Meek

merged trunk

2400. By Jesse Meek

cosmetic change to if statement syntax

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
68type containerManager struct {68type containerManager struct {
69 name string69 name string
70 logdir string70 logdir string
71 autoRestart bool
71 createWithClone bool72 createWithClone bool
72 useAUFS bool73 useAUFS bool
73 backingFilesystem string74 backingFilesystem string
@@ -88,6 +89,14 @@
88 if logDir == "" {89 if logDir == "" {
89 logDir = agent.DefaultLogDir90 logDir = agent.DefaultLogDir
90 }91 }
92
93 autoRestart := conf["auto-restart"] == "" || conf["auto-restart"] == "true"
94 delete(conf, "auto-restart")
95
96 for k, v := range conf {
97 logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
98 }
99
91 // Explicitly ignore the error result from ParseBool.100 // Explicitly ignore the error result from ParseBool.
92 // If it fails to parse, the value is false, and this suits101 // If it fails to parse, the value is false, and this suits
93 // us fine.102 // us fine.
@@ -101,11 +110,13 @@
101 // call it 'unknown'.110 // call it 'unknown'.
102 backingFS = "unknown"111 backingFS = "unknown"
103 }112 }
113
104 logger.Tracef("backing filesystem: %q", backingFS)114 logger.Tracef("backing filesystem: %q", backingFS)
105 conf.WarnAboutUnused()115 conf.WarnAboutUnused()
106 return &containerManager{116 return &containerManager{
107 name: name,117 name: name,
108 logdir: logDir,118 logdir: logDir,
119 autoRestart: autoRestart,
109 createWithClone: useClone,120 createWithClone: useClone,
110 useAUFS: useAUFS,121 useAUFS: useAUFS,
111 backingFilesystem: backingFS,122 backingFilesystem: backingFS,
@@ -198,8 +209,10 @@
198 }209 }
199 logger.Tracef("lxc container created")210 logger.Tracef("lxc container created")
200 }211 }
201 if err := autostartContainer(name); err != nil {212 if manager.autoRestart {
202 return nil, nil, err213 if err := autostartContainer(name); err != nil {
214 return nil, nil, err
215 }
203 }216 }
204 if err := mountHostLogDir(name, manager.logdir); err != nil {217 if err := mountHostLogDir(name, manager.logdir); err != nil {
205 return nil, nil, err218 return nil, nil, err
206219
=== 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 @@
36type LxcSuite struct {36type LxcSuite struct {
37 lxctesting.TestSuite37 lxctesting.TestSuite
3838
39 events chan mock.Event39 events chan mock.Event
40 useClone bool40 useClone bool
41 useAUFS bool41 useAUFS bool
42 autoRestartFalse bool
42}43}
4344
44var _ = gc.Suite(&LxcSuite{})45var _ = gc.Suite(&LxcSuite{})
@@ -99,6 +100,10 @@
99 if s.useAUFS {100 if s.useAUFS {
100 params["use-aufs"] = "true"101 params["use-aufs"] = "true"
101 }102 }
103 if s.autoRestartFalse {
104 params["auto-restart"] = "false"
105 }
106
102 manager, err := lxc.NewContainerManager(params)107 manager, err := lxc.NewContainerManager(params)
103 c.Assert(err, gc.IsNil)108 c.Assert(err, gc.IsNil)
104 return manager109 return manager
@@ -337,6 +342,37 @@
337 instancetest.MatchInstances(c, result, bar1, bar2)342 instancetest.MatchInstances(c, result, bar1, bar2)
338}343}
339344
345func (s *LxcSuite) TestStartContainerNoAutostart(c *gc.C) {
346 s.autoRestartFalse = true
347 manager := s.makeManager(c, "test")
348 instance := containertesting.StartContainer(c, manager, "1/lxc/0")
349 autostartLink := lxc.RestartSymlink(string(instance.Id()))
350 c.Assert(autostartLink, jc.SymlinkDoesNotExist)
351 s.autoRestartFalse = false
352}
353
354func (s *LxcSuite) TestStartContainerNoAutostartNoRestartDir(c *gc.C) {
355 s.autoRestartFalse = true
356 err := os.Remove(s.RestartDir)
357 c.Assert(err, gc.IsNil)
358
359 manager := s.makeManager(c, "test")
360 instance := containertesting.StartContainer(c, manager, "1/lxc/0")
361 name := string(instance.Id())
362 autostartLink := lxc.RestartSymlink(name)
363 config, err := ioutil.ReadFile(lxc.ContainerConfigFilename(name))
364 c.Assert(err, gc.IsNil)
365 expected := `
366lxc.network.type = veth
367lxc.network.link = nic42
368lxc.network.flags = up
369lxc.mount.entry=/var/log/juju var/log/juju none defaults,bind 0 0
370`
371 c.Assert(string(config), gc.Equals, expected)
372 c.Assert(autostartLink, jc.DoesNotExist)
373 s.autoRestartFalse = false
374}
375
340func (s *LxcSuite) TestStartContainerAutostarts(c *gc.C) {376func (s *LxcSuite) TestStartContainerAutostarts(c *gc.C) {
341 manager := s.makeManager(c, "test")377 manager := s.makeManager(c, "test")
342 instance := containertesting.StartContainer(c, manager, "1/lxc/0")378 instance := containertesting.StartContainer(c, manager, "1/lxc/0")
343379
=== 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 @@
117 return value117 return value
118}118}
119119
120func (c *environConfig) lxcAutoRestart() bool {
121 value, _ := c.attrs["lxc-auto-restart"].(bool)
122 return value
123}
124
120func (c *environConfig) createDirs() error {125func (c *environConfig) createDirs() error {
121 for _, dirname := range []string{126 for _, dirname := range []string{
122 c.storageDir(),127 c.storageDir(),
123128
=== 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 @@
220 if containerType == instance.LXC {220 if containerType == instance.LXC {
221 managerConfig["use-clone"] = strconv.FormatBool(env.config.lxcClone())221 managerConfig["use-clone"] = strconv.FormatBool(env.config.lxcClone())
222 managerConfig["use-aufs"] = strconv.FormatBool(env.config.lxcCloneAUFS())222 managerConfig["use-aufs"] = strconv.FormatBool(env.config.lxcCloneAUFS())
223 managerConfig["auto-restart"] = strconv.FormatBool(env.config.lxcAutoRestart())
223 }224 }
224 env.containerManager, err = factory.NewContainerManager(225 env.containerManager, err = factory.NewContainerManager(
225 containerType, managerConfig)226 containerType, managerConfig)

Subscribers

People subscribed via source and target branches

to status/vote changes: