Merge lp:~waigani/juju-core/lxc-trusty-autostart into lp:~go-bot/juju-core/trunk
- lxc-trusty-autostart
- Merge into trunk
Status: | Merged | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Approved by: | Jesse Meek | ||||||||||||
Approved revision: | no longer in the source branch. | ||||||||||||
Merged at revision: | 2262 | ||||||||||||
Proposed branch: | lp:~waigani/juju-core/lxc-trusty-autostart | ||||||||||||
Merge into: | lp:~go-bot/juju-core/trunk | ||||||||||||
Diff against target: |
126 lines (+68/-12) 3 files modified
container/lxc/lxc.go (+16/-6) container/lxc/lxc_test.go (+36/-6) container/lxc/restart.go (+16/-0) |
||||||||||||
To merge this branch: | bzr merge lp:~waigani/juju-core/lxc-trusty-autostart | ||||||||||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Juju Engineering | Pending | ||
Review via email: mp+202974@code.launchpad.net |
Commit message
Auto start for lxc containers
Auto start for lxc containers should now work on Trusty. Symlink is created if /etc/lxc/auto exists, otherwise auto start is set to true in lxc config.
Description of the change
Auto start for lxc containers
Auto start for lxc containers should now work on Trusty. Symlink is created if /etc/lxc/auto exists, otherwise auto start is set to true in lxc config.
Jesse Meek (waigani) wrote : | # |
Andrew Wilkins (axwalk) wrote : | # |
LGTM with comment embellishment
https:/
File container/
https:/
container/
restart directory, if it exists
I think it'd be good to add a comment as to when it does/doesn't exist.
i.e. that this is for backwards-
longer exists from trusty.
William Reade (fwereade) wrote : | # |
Nice, LGTM, please remember to link the bug to your branch:
https:/
William Reade (fwereade) wrote : | # |
On 2014/01/24 14:52:20, fwereade wrote:
> Nice, LGTM, please remember to link the bug to your branch:
> https:/
Except; wait. Do we know what happens on a machine that has a restart
dir from an earlier LXC version, but currently does not? I think it
might be better to depend on an explicit LXC version than on this
heuristic...
Jesse Meek (waigani) wrote : | # |
On 2014/01/24 14:53:50, fwereade wrote:
> On 2014/01/24 14:52:20, fwereade wrote:
> > Nice, LGTM, please remember to link the bug to your branch:
> > https:/
> Except; wait. Do we know what happens on a machine that has a restart
dir from
> an earlier LXC version, but currently does not? I think it might be
better to
> depend on an explicit LXC version than on this heuristic...
Good point. Any idea from what version LXC stopped using the restart
directory? Should we just use the version currently used in Trusty?
Tim Penhey (thumper) wrote : | # |
On 2014/01/24 14:53:50, fwereade wrote:
> On 2014/01/24 14:52:20, fwereade wrote:
> > Nice, LGTM, please remember to link the bug to your branch:
> > https:/
> Except; wait. Do we know what happens on a machine that has a restart
dir from
> an earlier LXC version, but currently does not? I think it might be
better to
> depend on an explicit LXC version than on this heuristic...
I talked with stgraber and hallyn about this, and they both said that
the
best approach is to use the /etc/lxc/auto directory if it is there.
As lxc gets upgraded, it updates all of the containers currently
auto-starting
using the directory and then removes the directory.
There will be no /etc/lxc/auto directory if it is not used by lxc.
Tim Penhey (thumper) wrote : | # |
https:/
File container/
https:/
container/
restart directory, if it exists
On 2014/01/24 02:19:17, axw wrote:
> I think it'd be good to add a comment as to when it does/doesn't
exist. i.e.
> that this is for backwards-
exists
> from trusty.
I agree that there should be more of a comment here to explain
intent.
Jesse Meek (waigani) wrote : | # |
https:/
File container/
https:/
container/
restart directory, if it exists
On 2014/01/24 02:19:17, axw wrote:
> I think it'd be good to add a comment as to when it does/doesn't
exist. i.e.
> that this is for backwards-
exists
> from trusty.
Done.
Go Bot (go-bot) wrote : | # |
No approved revision specified.
Go Bot (go-bot) wrote : | # |
The attempt to merge lp:~waigani/juju-core/lxc-trusty-autostart into lp:juju-core failed. Below is the output from the failed tests.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
-------
FAIL: lxc_test.go:251: NetworkSuite.
lxc_test.go:258:
c.Assert(
... obtained string = "" +
... "\n" +
... "lxc.network.type = foo\n" +
... "lxc.network.link = bar\n" +
... "lxc.network.flags = up\n" +
... "lxc.start.auto = 1\n"
... expected string = "" +
... "\n" +
... "lxc.network.type = foo\n" +
... "lxc.network.link = bar\n" +
... "lxc.network.flags = up\n"
OOPS: 10 passed, 1 FAILED
--- FAIL: Test (0.08 seconds)
FAIL
FAIL launchpad.
? launchpad.
? launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
? launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
ok launchpad.
Jesse Meek (waigani) wrote : | # |
All tests pass for me. What am I missing? Here are my steps:
$ cd ~/go/src/
$ bzr switch lxc-trusty-
$ go test
OK: 11 passed
PASS
ok launchpad.
$ go test -gocheck.f TestNetworkConf
OK: 1 passed
PASS
ok launchpad.
On Mon, Jan 27, 2014 at 3:09 PM, Go Bot <email address hidden> wrote:
> The attempt to merge lp:~waigani/juju-core/lxc-trusty-autostart into
> lp:juju-core failed. Below is the output from the failed tests.
>
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ? launchpad.
> ? launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ? launchpad.
> ? launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ? launchpad.
> files]
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ? launchpad.
>
> -------
> FAIL: lxc_test.go:251: NetworkSuite.
>
> lxc_test.go:258:
> c.Assert(config, gc.Equals, expected)
> ... obtained string = "" +
> ... "\n" +
> ... "lxc.network.type = foo\n" +
> ... "lxc.network.link = bar\n" +
> ... "lxc.network.flags = up\n" +
> ... "lxc.start.auto = 1\n"
> ... expected string = "" +
> ... "\n" +
> ... "lxc.network.type = foo\n" +
> ... "lxc.network.link = bar\n" +
> ... "lxc.network.flags = up\n"
>
> OOPS: 10 passed, 1 FAILED
> --- FAIL: Test (0.08 seconds)
> FAIL
> FAIL launchpad.
> ? launchpad.
> ? launchpad.
> ? launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
> ok launchpad.
Dimiter Naydenov (dimitern) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 27.01.2014 04:27, Jesse Meek wrote:
> All tests pass for me. What am I missing? Here are my steps:
The bot is running precise, not trusty, which might imply you've
written the tests assuming they should be run on trusty?
Dimiter
>
> $ cd ~/go/src/
> lxc-trusty-
> launchpad.
> TestNetworkConf
> launchpad.
>
>
>
> On Mon, Jan 27, 2014 at 3:09 PM, Go Bot
> <email address hidden> wrote:
>
>> The attempt to merge lp:~waigani/juju-core/lxc-trusty-autostart
>> into lp:juju-core failed. Below is the output from the failed
>> tests.
>>
>> ok launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> files] ok launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>>
>> -------
>>
>>
FAIL: lxc_test.go:251: NetworkSuite.
>>
>> lxc_test.go:258: c.Assert(config, gc.Equals, expected) ...
>> obtained string = "" + ... "\n" + ... "lxc.network.type =
>> foo\n" + ... "lxc.network.link = bar\n" + ...
>> "lxc.network.flags = up\n" + ... "lxc.start.auto = 1\n" ...
>> expected string = "" + ... "\n" + ... "lxc.network.type =
>> foo\n" + ... "lxc.network.link = bar\n" + ...
>> "lxc.network.flags = up\n"
>>
>> OOPS: 10 passed, 1 FAILED --- FAIL: Test (0.08 seconds) FAIL FAIL
>> launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
>> ok launchpad.
>> launchpad.
>> launchpad.
>> launchpad.
Tim Penhey (thumper) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
On 27/01/14 15:30, Dimiter Naydenov wrote:
> On 27.01.2014 04:27, Jesse Meek wrote:
>> All tests pass for me. What am I missing? Here are my steps:
>
> The bot is running precise, not trusty, which might imply you've
> written the tests assuming they should be run on trusty?
>
> Dimiter
No... I think it is just a test isolation thing.
I'll work with Jesse on it.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.14 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://
iEYEARECAAYFAlL
kAIAnAyomeR/
=/ZOW
-----END PGP SIGNATURE-----
Tim Penhey (thumper) wrote : | # |
The test failed because the bot doesn't have lxc installed, and thus the lxc.RemoveDir doesn't exist.
The NetworkSuite doesn't contain the lxc suite, so there is no isolation of the RemoveDir.
In the past, the entire lxc.conf file was just networking. With the addition of the auto start, we now have to have better isolation with this test.
There are a few options, but the one that I think fits best, is to parse the conf template results and just get the results that start with 'lxc.network' as that is what the test cares about.
So, something like
obtained = []string{}
for _, value := range strings.
if strings.
obtained := append(obtained, value)
}
}
expected := []string{
"lxc.network.type = foo",
"lxc.network.link = bar",
"lxc.
}
c.Assert(obtained, gc.DeepEquals, expected)
William Reade (fwereade) wrote : | # |
LGTM in light of the explanation, thanks.
Preview Diff
1 | === modified file 'container/lxc/lxc.go' |
2 | --- container/lxc/lxc.go 2013-12-04 02:37:05 +0000 |
3 | +++ container/lxc/lxc.go 2014-01-27 07:18:13 +0000 |
4 | @@ -109,12 +109,17 @@ |
5 | return nil, nil, err |
6 | } |
7 | logger.Tracef("lxc container created") |
8 | - // Now symlink the config file into the restart directory. |
9 | - containerConfigFile := filepath.Join(LxcContainerDir, name, "config") |
10 | - if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil { |
11 | - return nil, nil, err |
12 | + // Now symlink the config file into the restart directory, if it exists. |
13 | + // This is for backwards compatiblity. From Trusty onwards, the auto start |
14 | + // option should be set in the LXC config file, this is done in the networkConfigTemplate |
15 | + // function below. |
16 | + if useRestartDir() { |
17 | + containerConfigFile := filepath.Join(LxcContainerDir, name, "config") |
18 | + if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil { |
19 | + return nil, nil, err |
20 | + } |
21 | + logger.Tracef("auto-restart link created") |
22 | } |
23 | - logger.Tracef("auto-restart link created") |
24 | |
25 | // Start the lxc container with the appropriate settings for grabbing the |
26 | // console output and a log file. |
27 | @@ -198,7 +203,12 @@ |
28 | ` |
29 | |
30 | func networkConfigTemplate(networkType, networkLink string) string { |
31 | - return fmt.Sprintf(networkTemplate, networkType, networkLink) |
32 | + networkConfig := fmt.Sprintf(networkTemplate, networkType, networkLink) |
33 | + if !useRestartDir() { |
34 | + networkConfig += "lxc.start.auto = 1\n" |
35 | + logger.Tracef("Setting auto start to true in lxc config.") |
36 | + } |
37 | + return networkConfig |
38 | } |
39 | |
40 | func generateNetworkConfig(network *container.NetworkConfig) string { |
41 | |
42 | === modified file 'container/lxc/lxc_test.go' |
43 | --- container/lxc/lxc_test.go 2013-12-16 07:20:01 +0000 |
44 | +++ container/lxc/lxc_test.go 2014-01-27 07:18:13 +0000 |
45 | @@ -8,6 +8,7 @@ |
46 | "io/ioutil" |
47 | "os" |
48 | "path/filepath" |
49 | + "strings" |
50 | stdtesting "testing" |
51 | |
52 | gc "launchpad.net/gocheck" |
53 | @@ -186,6 +187,25 @@ |
54 | c.Assert(autostartLink, jc.IsSymlink) |
55 | } |
56 | |
57 | +func (s *LxcSuite) TestStartContainerNoRestartDir(c *gc.C) { |
58 | + err := os.Remove(s.RestartDir) |
59 | + c.Assert(err, gc.IsNil) |
60 | + |
61 | + manager := lxc.NewContainerManager(container.ManagerConfig{}) |
62 | + instance := containertesting.StartContainer(c, manager, "1/lxc/0") |
63 | + autostartLink := lxc.RestartSymlink(string(instance.Id())) |
64 | + |
65 | + config := lxc.NetworkConfigTemplate("foo", "bar") |
66 | + expected := ` |
67 | +lxc.network.type = foo |
68 | +lxc.network.link = bar |
69 | +lxc.network.flags = up |
70 | +lxc.start.auto = 1 |
71 | +` |
72 | + c.Assert(config, gc.Equals, expected) |
73 | + c.Assert(autostartLink, jc.DoesNotExist) |
74 | +} |
75 | + |
76 | func (s *LxcSuite) TestStopContainerRemovesAutostartLink(c *gc.C) { |
77 | manager := lxc.NewContainerManager(container.ManagerConfig{}) |
78 | instance := containertesting.StartContainer(c, manager, "1/lxc/0") |
79 | @@ -231,10 +251,20 @@ |
80 | |
81 | func (*NetworkSuite) TestNetworkConfigTemplate(c *gc.C) { |
82 | config := lxc.NetworkConfigTemplate("foo", "bar") |
83 | - expected := ` |
84 | -lxc.network.type = foo |
85 | -lxc.network.link = bar |
86 | -lxc.network.flags = up |
87 | -` |
88 | - c.Assert(config, gc.Equals, expected) |
89 | + //In the past, the entire lxc.conf file was just networking. With the addition |
90 | + //of the auto start, we now have to have better isolate this test. As such, we |
91 | + //parse the conf template results and just get the results that start with |
92 | + //'lxc.network' as that is what the test cares about. |
93 | + obtained := []string{} |
94 | + for _, value := range strings.Split(config, "\n") { |
95 | + if strings.HasPrefix(value, "lxc.network") { |
96 | + obtained = append(obtained, value) |
97 | + } |
98 | + } |
99 | + expected := []string{ |
100 | + "lxc.network.type = foo", |
101 | + "lxc.network.link = bar", |
102 | + "lxc.network.flags = up", |
103 | + } |
104 | + c.Assert(obtained, gc.DeepEquals, expected) |
105 | } |
106 | |
107 | === added file 'container/lxc/restart.go' |
108 | --- container/lxc/restart.go 1970-01-01 00:00:00 +0000 |
109 | +++ container/lxc/restart.go 2014-01-27 07:18:13 +0000 |
110 | @@ -0,0 +1,16 @@ |
111 | +package lxc |
112 | + |
113 | +import ( |
114 | + "os" |
115 | +) |
116 | + |
117 | +//If restart dir exists, return true. Otherwise, return false. |
118 | +//TODO Should this test LXC version, not existence of restart dir? |
119 | +func useRestartDir() bool { |
120 | + if _, err := os.Stat(LxcRestartDir); err != nil { |
121 | + if os.IsNotExist(err) { |
122 | + return false |
123 | + } |
124 | + } |
125 | + return true |
126 | +} |
Reviewers: mp+202974_ code.launchpad. net,
Message:
Please take a look.
Description:
Auto start for lxc containers
Auto start for lxc containers should now work on Trusty. Symlink is
created if /etc/lxc/auto exists, otherwise auto start is set to true in
lxc config.
https:/ /code.launchpad .net/~waigani/ juju-core/ lxc-trusty- autostart/ +merge/ 202974
(do not edit description out of merge proposal)
Please review this at https:/ /codereview. appspot. com/56370043/
Affected files (+48, -6 lines): lxc/lxc. go lxc/lxc_ test.go lxc/restart. go
A [revision details]
M container/
M container/
A container/
Index: [revision details] 20140124005805- 5wkfrpv3q7zaknx v
=== 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: tarmac-
+New revision: <email address hidden>
Index: container/ lxc/lxc. go lxc/lxc. go' lxc/lxc. go 2013-12-04 02:37:05 +0000 lxc/lxc. go 2014-01-24 01:19:25 +0000 Tracef( "lxc container created") Join(LxcContain erDir, name, "config") containerConfig File, restartSymlink( name)); err != Join(LxcContain erDir, name, "config") containerConfig File, restartSymlink( name)); err != Tracef( "auto-restart link created") Tracef( "auto-restart link created")
=== modified file 'container/
--- container/
+++ container/
@@ -109,12 +109,14 @@
return nil, nil, err
}
logger.
- // Now symlink the config file into the restart directory.
- containerConfigFile := filepath.
- if err := os.Symlink(
nil {
- return nil, nil, err
+ // Now symlink the config file into the restart directory, if it exists
+ if restartDirExists() {
+ containerConfigFile := filepath.
+ if err := os.Symlink(
nil {
+ return nil, nil, err
+ }
+ logger.
}
- logger.
// Start the lxc container with the appropriate settings for grabbing the
// console output and a log file.
@@ -198,7 +200,12 @@
`
func networkConfigTe mplate( networkType, networkLink string) string { networkTemplate , networkType, networkLink) networkTemplate , networkType, networkLink) Tracef( "Setting auto start to true in lxc config.")
- return fmt.Sprintf(
+ networkConfig := fmt.Sprintf(
+ if !restartDirExists() {
+ networkConfig += "lxc.start.auto = 1\n"
+ logger.
+ }
+ return networkConfig
}
func generateNetwork Config( network *container. NetworkConfig) string {
Index: container/ lxc/lxc_ test.go lxc/lxc_ test.go' lxc/lxc_ test.go 2013-12-16 07:20:01 +0000 lxc/lxc_ test.go 2014-01-24 01:19:25 +0000 autostartLink, jc.IsSymlink)
=== modified file 'container/
--- container/
+++ container/
@@ -186,6 +186,25 @@
c.Assert(
}
+func (s *LxcSuite) TestStartContai nerNoRestartDir (c *gc.C) { s.RestartDir) rManager( container. ManagerConfig{ }) g.StartContaine r(c, manager, "1/lxc/0") ink(string( instance. Id())) igTemplate( "foo", "bar") autostartLink, jc.Does...
+ err := os.Remove(
+ c.Assert(err, gc.IsNil)
+
+ manager := lxc.NewContaine
+ instance := containertestin
+ autostartLink := lxc.RestartSyml
+
+ config := lxc.NetworkConf
+ expected := `
+lxc.network.type = foo
+lxc.network.link = bar
+lxc.network.flags = up
+lxc.start.auto = 1
+`
+ c.Assert(config, gc.Equals, expected)
+ c.Assert(