Merge lp:~thumper/juju-core/autostart-containers-after-creation into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Set lxc autostart after creation.

Currently for a recent LXC version, the autostart is controlled
by a setting in the lxc config. We were setting this in the
config file that we use for creation. However this is not clone
friendly, as we do not specify a config file for clone.

However it is perfectly valid to modify the config for the container
once it has been created. This then becomes common for both
normally created containers and those that have been cloned.

A key here is that our template container that we want to use
as a base to clone off we do not want auto-starting, however
the containers that we clone off that may well want to auto-start
(in fact all will until we add the autostart policy for local).

https://codereview.appspot.com/73300043/

Set lxc autostart after creation.

Currently for a recent LXC version, the autostart is controlled
by a setting in the lxc config. We were setting this in the
config file that we use for creation. However this is not clone
friendly, as we do not specify a config file for clone.

However it is perfectly valid to modify the config for the container
once it has been created. This then becomes common for both
normally created containers and those that have been cloned.

A key here is that our template container that we want to use
as a base to clone off we do not want auto-starting, however
the containers that we clone off that may well want to auto-start
(in fact all will until we add the autostart policy for local).

https://codereview.appspot.com/73300043/

[Tim Penhey (thumper)]

Reviewers: mp+210099_code.launchpad.net,

Message:
Please take a look.

Description:
Set lxc autostart after creation.

Currently for a recent LXC version, the autostart is controlled
by a setting in the lxc config. We were setting this in the
config file that we use for creation. However this is not clone
friendly, as we do not specify a config file for clone.

However it is perfectly valid to modify the config for the container
once it has been created. This then becomes common for both
normally created containers and those that have been cloned.

A key here is that our template container that we want to use
as a base to clone off we do not want auto-starting, however
the containers that we clone off that may well want to auto-start
(in fact all will until we add the autostart policy for local).

https://code.launchpad.net/~thumper/juju-core/autostart-containers-after-creation/+merge/210099

Requires:
https://code.launchpad.net/~thumper/juju-core/fast-lxc/+merge/209801

(do not edit description out of merge proposal)

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

Affected files (+71, -35 lines):
   A [revision details]
   M container/lxc/export_test.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M container/lxc/mock/mock-lxc.go
   M container/lxc/testing/test.go

[Jesse Meek (waigani)]

On 2014/03/09 21:29:35, thumper wrote:
> Please take a look.

LGTM

https://codereview.appspot.com/73300043/

[Andrew Wilkins (axwalk)]

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

https://codereview.appspot.com/73300043/diff/1/container/lxc/lxc_test.go#newcode179
container/lxc/lxc_test.go:179: name := string(instance.Id())
It's not clear why this has changed. Has something caused the contents
of the file to be different (apart from the lxc.start.auto)? I would
think it's preferable to match exactly if it's not too onerous.

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

https://codereview.appspot.com/73300043/diff/1/container/lxc/mock/mock-lxc.go#newcode87
container/lxc/mock/mock-lxc.go:87: data, err :=
ioutil.ReadFile(configFile)
There's utils.CopyFile, if you didn't know.

https://codereview.appspot.com/73300043/diff/1/container/lxc/mock/mock-lxc.go#newcode92
container/lxc/mock/mock-lxc.go:92: return
ioutil.WriteFile(configFilename, data, 0755)
0644?

https://codereview.appspot.com/73300043/

[Tim Penhey (thumper)]

Please take a look.

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

https://codereview.appspot.com/73300043/diff/1/container/lxc/lxc_test.go#newcode179
container/lxc/lxc_test.go:179: name := string(instance.Id())
On 2014/03/11 02:24:13, axw wrote:
> It's not clear why this has changed. Has something caused the contents
of the
> file to be different (apart from the lxc.start.auto)? I would think
it's
> preferable to match exactly if it's not too onerous.

Doing an exact match now.

In order to make the match easily, I have also added in the part where
the mounting of the log dir is done after create.

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

https://codereview.appspot.com/73300043/diff/1/container/lxc/mock/mock-lxc.go#newcode87
container/lxc/mock/mock-lxc.go:87: data, err :=
ioutil.ReadFile(configFile)
On 2014/03/11 02:24:13, axw wrote:
> There's utils.CopyFile, if you didn't know.

No I didn't. Using it now.

https://codereview.appspot.com/73300043/

[Andrew Wilkins (axwalk)]

On 2014/03/11 03:46:16, thumper wrote:
> Please take a look.

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

https://codereview.appspot.com/73300043/diff/1/container/lxc/lxc_test.go#newcode179
> container/lxc/lxc_test.go:179: name := string(instance.Id())
> On 2014/03/11 02:24:13, axw wrote:
> > It's not clear why this has changed. Has something caused the
contents of the
> > file to be different (apart from the lxc.start.auto)? I would think
it's
> > preferable to match exactly if it's not too onerous.

> Doing an exact match now.

> In order to make the match easily, I have also added in the part where
the
> mounting of the log dir is done after create.

https://codereview.appspot.com/73300043/diff/1/container/lxc/mock/mock-lxc.go
> File container/lxc/mock/mock-lxc.go (right):

https://codereview.appspot.com/73300043/diff/1/container/lxc/mock/mock-lxc.go#newcode87
> container/lxc/mock/mock-lxc.go:87: data, err :=
ioutil.ReadFile(configFile)
> On 2014/03/11 02:24:13, axw wrote:
> > There's utils.CopyFile, if you didn't know.

> No I didn't. Using it now.

LGTM, thanks

https://codereview.appspot.com/73300043/

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/autostart-containers-after-creation into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.014s
ok launchpad.net/juju-core/agent 1.039s
ok launchpad.net/juju-core/agent/mongo 0.530s
ok launchpad.net/juju-core/agent/tools 0.172s
ok launchpad.net/juju-core/bzr 5.107s
ok launchpad.net/juju-core/cert 2.457s
ok launchpad.net/juju-core/charm 0.393s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.029s
ok launchpad.net/juju-core/cloudinit/sshinit 0.856s
ok launchpad.net/juju-core/cmd 0.152s
ok launchpad.net/juju-core/cmd/charm-admin 0.728s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 188.242s
ok launchpad.net/juju-core/cmd/jujud 63.233s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.291s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.186s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.029s
ok launchpad.net/juju-core/container 0.044s
ok launchpad.net/juju-core/container/factory 0.048s
ok launchpad.net/juju-core/container/kvm 0.183s
ok launchpad.net/juju-core/container/kvm/mock 0.041s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.264s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.236s
ok launchpad.net/juju-core/environs 2.510s
ok launchpad.net/juju-core/environs/bootstrap 2.981s
ok launchpad.net/juju-core/environs/cloudinit 0.460s
ok launchpad.net/juju-core/environs/config 2.174s
ok launchpad.net/juju-core/environs/configstore 0.037s
ok launchpad.net/juju-core/environs/filestorage 0.024s
ok launchpad.net/juju-core/environs/httpstorage 0.712s
ok launchpad.net/juju-core/environs/imagemetadata 0.462s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.042s
ok launchpad.net/juju-core/environs/jujutest 0.202s
ok launchpad.net/juju-core/environs/manual 11.863s
ok launchpad.net/juju-core/environs/simplestreams 0.218s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.948s
ok launchpad.net/juju-core/environs/storage 1.024s
ok launchpad.net/juju-core/environs/sync 23.103s
ok launchpad.net/juju-core/environs/testing 0.179s
ok launchpad.net/juju-core/environs/tools 4.606s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.012s
ok launchpad.net/juju-core/instance 0.017s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 17.731s
ok launchpad.net/juju-core/juj...

[Go Bot (go-bot)]

The attempt to merge lp:~thumper/juju-core/autostart-containers-after-creation into lp:juju-core failed. Below is the output from the failed tests.

ok launchpad.net/juju-core 0.012s
ok launchpad.net/juju-core/agent 0.987s
ok launchpad.net/juju-core/agent/mongo 0.561s
ok launchpad.net/juju-core/agent/tools 0.191s
ok launchpad.net/juju-core/bzr 4.808s
ok launchpad.net/juju-core/cert 2.287s
ok launchpad.net/juju-core/charm 0.392s
? launchpad.net/juju-core/charm/hooks [no test files]
? launchpad.net/juju-core/charm/testing [no test files]
ok launchpad.net/juju-core/cloudinit 0.033s
ok launchpad.net/juju-core/cloudinit/sshinit 0.799s
ok launchpad.net/juju-core/cmd 0.170s
ok launchpad.net/juju-core/cmd/charm-admin 0.730s
? launchpad.net/juju-core/cmd/charmd [no test files]
? launchpad.net/juju-core/cmd/charmload [no test files]
ok launchpad.net/juju-core/cmd/juju 194.479s
ok launchpad.net/juju-core/cmd/jujud 62.786s
ok launchpad.net/juju-core/cmd/plugins/juju-metadata 9.924s
? launchpad.net/juju-core/cmd/plugins/juju-restore [no test files]
ok launchpad.net/juju-core/cmd/plugins/local 0.151s
? launchpad.net/juju-core/cmd/plugins/local/juju-local [no test files]
ok launchpad.net/juju-core/constraints 0.019s
ok launchpad.net/juju-core/container 0.038s
ok launchpad.net/juju-core/container/factory 0.050s
ok launchpad.net/juju-core/container/kvm 0.236s
ok launchpad.net/juju-core/container/kvm/mock 0.041s
? launchpad.net/juju-core/container/kvm/testing [no test files]
ok launchpad.net/juju-core/container/lxc 0.251s
? launchpad.net/juju-core/container/lxc/mock [no test files]
? launchpad.net/juju-core/container/lxc/testing [no test files]
? launchpad.net/juju-core/container/testing [no test files]
ok launchpad.net/juju-core/downloader 5.221s
ok launchpad.net/juju-core/environs 2.491s
ok launchpad.net/juju-core/environs/bootstrap 3.082s
ok launchpad.net/juju-core/environs/cloudinit 0.444s
ok launchpad.net/juju-core/environs/config 3.589s
ok launchpad.net/juju-core/environs/configstore 0.028s
ok launchpad.net/juju-core/environs/filestorage 0.026s
ok launchpad.net/juju-core/environs/httpstorage 0.592s
ok launchpad.net/juju-core/environs/imagemetadata 0.405s
? launchpad.net/juju-core/environs/imagemetadata/testing [no test files]
ok launchpad.net/juju-core/environs/instances 0.049s
ok launchpad.net/juju-core/environs/jujutest 0.156s
ok launchpad.net/juju-core/environs/manual 10.074s
ok launchpad.net/juju-core/environs/simplestreams 0.226s
? launchpad.net/juju-core/environs/simplestreams/testing [no test files]
ok launchpad.net/juju-core/environs/sshstorage 0.961s
ok launchpad.net/juju-core/environs/storage 0.910s
ok launchpad.net/juju-core/environs/sync 23.657s
ok launchpad.net/juju-core/environs/testing 0.176s
ok launchpad.net/juju-core/environs/tools 4.828s
? launchpad.net/juju-core/environs/tools/testing [no test files]
ok launchpad.net/juju-core/errors 0.011s
ok launchpad.net/juju-core/instance 0.016s
? launchpad.net/juju-core/instance/testing [no test files]
ok launchpad.net/juju-core/juju 20.596s
ok launchpad.net/juju-core/juj...

=== modified file 'container/lxc/export_test.go'
--- container/lxc/export_test.go	2013-10-01 02:21:52 +0000
+++ container/lxc/export_test.go	2014-03-11 03:19:30 +0000
@@ -4,7 +4,8 @@
 package lxc
 
 var (
-	NetworkConfigTemplate = networkConfigTemplate
-	GenerateNetworkConfig = generateNetworkConfig
-	RestartSymlink        = restartSymlink
+	NetworkConfigTemplate   = networkConfigTemplate
+	GenerateNetworkConfig   = generateNetworkConfig
+	RestartSymlink          = restartSymlink
+	ContainerConfigFilename = containerConfigFilename
 )

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2014-03-10 02:51:31 +0000
+++ container/lxc/lxc.go	2014-03-11 03:19:30 +0000
@@ -96,7 +96,7 @@
 		return nil, nil, err
 	}
 	logger.Tracef("write the lxc.conf file")
-	configFile, err := writeLxcConfig(network, directory, manager.logdir)
+	configFile, err := writeLxcConfig(network, directory)
 	if err != nil {
 		logger.Errorf("failed to write config file: %v", err)
 		return nil, nil, err
@@ -113,25 +113,14 @@
 		logger.Errorf("lxc container creation failed: %v", err)
 		return nil, nil, err
 	}
-	// Make sure that the mount dir has been created.
-	logger.Tracef("make the mount dir for the shard logs")
-	if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {
-		logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
-		return nil, nil, err
-	}
 	logger.Tracef("lxc container created")
-	// Now symlink the config file into the restart directory, if it exists.
-	// This is for backwards compatiblity. From Trusty onwards, the auto start
-	// option should be set in the LXC config file, this is done in the networkConfigTemplate
-	// function below.
-	if useRestartDir() {
-		containerConfigFile := filepath.Join(LxcContainerDir, name, "config")
-		if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
-			return nil, nil, err
-		}
-		logger.Tracef("auto-restart link created")
-	}
 
+	if err := autostartContainer(name); err != nil {
+		return nil, nil, err
+	}
+	if err := mountHostLogDir(name, manager.logdir); err != nil {
+		return nil, nil, err
+	}
 	// Start the lxc container with the appropriate settings for grabbing the
 	// console output and a log file.
 	consoleFile := filepath.Join(directory, "console.log")
@@ -153,6 +142,50 @@
 	return &lxcInstance{lxcContainer, name}, hardware, nil
 }
 
+func appendToContainerConfig(name, line string) error {
+	file, err := os.OpenFile(
+		containerConfigFilename(name), os.O_RDWR|os.O_APPEND, 0644)
+	if err != nil {
+		return err
+	}
+	defer file.Close()
+	_, err = file.WriteString(line)
+	return err
+}
+
+func autostartContainer(name string) error {
+	// Now symlink the config file into the restart directory, if it exists.
+	// This is for backwards compatiblity. From Trusty onwards, the auto start
+	// option should be set in the LXC config file, this is done in the networkConfigTemplate
+	// function below.
+	if useRestartDir() {
+		if err := os.Symlink(
+			containerConfigFilename(name),
+			restartSymlink(name),
+		); err != nil {
+			return err
+		}
+		logger.Tracef("auto-restart link created")
+	} else {
+		logger.Tracef("Setting auto start to true in lxc config.")
+		return appendToContainerConfig(name, "lxc.start.auto = 1\n")
+	}
+	return nil
+}
+
+func mountHostLogDir(name, logDir string) error {
+	// Make sure that the mount dir has been created.
+	logger.Tracef("make the mount dir for the shared logs")
+	if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {
+		logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
+		return err
+	}
+	line := fmt.Sprintf(
+		"lxc.mount.entry=%s var/log/juju none defaults,bind 0 0\n",
+		logDir)
+	return appendToContainerConfig(name, line)
+}
+
 func (manager *containerManager) StopContainer(instance instance.Instance) error {
 	name := string(instance.Id())
 	lxcContainer := LxcObjectFactory.New(name)
@@ -205,9 +238,9 @@
 	return filepath.Join(LxcRestartDir, name+".conf")
 }
 
-const localConfig = `%s
-lxc.mount.entry=%s var/log/juju none defaults,bind 0 0
-`
+func containerConfigFilename(name string) string {
+	return filepath.Join(LxcContainerDir, name, "config")
+}
 
 const networkTemplate = `
 lxc.network.type = %s
@@ -216,12 +249,7 @@
 `
 
 func networkConfigTemplate(networkType, networkLink string) string {
-	networkConfig := fmt.Sprintf(networkTemplate, networkType, networkLink)
-	if !useRestartDir() {
-		networkConfig += "lxc.start.auto = 1\n"
-		logger.Tracef("Setting auto start to true in lxc config.")
-	}
-	return networkConfig
+	return fmt.Sprintf(networkTemplate, networkType, networkLink)
 }
 
 func generateNetworkConfig(network *container.NetworkConfig) string {
@@ -240,11 +268,10 @@
 	}
 }
 
-func writeLxcConfig(network *container.NetworkConfig, directory, logdir string) (string, error) {
+func writeLxcConfig(network *container.NetworkConfig, directory string) (string, error) {
 	networkConfig := generateNetworkConfig(network)
 	configFilename := filepath.Join(directory, "lxc.conf")
-	configContent := fmt.Sprintf(localConfig, networkConfig, logdir)
-	if err := ioutil.WriteFile(configFilename, []byte(configContent), 0644); err != nil {
+	if err := ioutil.WriteFile(configFilename, []byte(networkConfig), 0644); err != nil {
 		return "", err
 	}
 	return configFilename, nil

=== 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-11 03:19:30 +0000
@@ -185,16 +185,18 @@
 
 	manager := s.makeManager(c, "test")
 	instance := containertesting.StartContainer(c, manager, "1/lxc/0")
-	autostartLink := lxc.RestartSymlink(string(instance.Id()))
-
-	config := lxc.NetworkConfigTemplate("foo", "bar")
+	name := string(instance.Id())
+	autostartLink := lxc.RestartSymlink(name)
+	config, err := ioutil.ReadFile(lxc.ContainerConfigFilename(name))
+	c.Assert(err, gc.IsNil)
 	expected := `
-lxc.network.type = foo
-lxc.network.link = bar
+lxc.network.type = veth
+lxc.network.link = nic42
 lxc.network.flags = up
 lxc.start.auto = 1
+lxc.mount.entry=/var/log/juju var/log/juju none defaults,bind 0 0
 `
-	c.Assert(config, gc.Equals, expected)
+	c.Assert(string(config), gc.Equals, expected)
 	c.Assert(autostartLink, jc.DoesNotExist)
 }
 

=== modified file 'container/lxc/mock/mock-lxc.go'
--- container/lxc/mock/mock-lxc.go	2013-09-30 01:50:43 +0000
+++ container/lxc/mock/mock-lxc.go	2014-03-11 03:19:30 +0000
@@ -5,8 +5,12 @@
 
 import (
 	"fmt"
+	"os"
+	"path/filepath"
 
 	"launchpad.net/golxc"
+
+	"launchpad.net/juju-core/utils"
 )
 
 // This file provides a mock implementation of the golxc interfaces
@@ -44,13 +48,15 @@
 }
 
 type mockFactory struct {
-	instances map[string]golxc.Container
-	listeners []chan<- Event
+	containerDir string
+	instances    map[string]golxc.Container
+	listeners    []chan<- Event
 }
 
-func MockFactory() ContainerFactory {
+func MockFactory(containerDir string) ContainerFactory {
 	return &mockFactory{
-		instances: make(map[string]golxc.Container),
+		containerDir: containerDir,
+		instances:    make(map[string]golxc.Container),
 	}
 }
 
@@ -74,7 +80,13 @@
 	}
 	mock.state = golxc.StateStopped
 	mock.factory.instances[mock.name] = mock
-	return nil
+	// Create the container directory.
+	containerDir := filepath.Join(mock.factory.containerDir, mock.name)
+	if err := os.MkdirAll(containerDir, 0755); err != nil {
+		return err
+	}
+	containerConfig := filepath.Join(containerDir, "config")
+	return utils.CopyFile(containerConfig, configFile)
 }
 
 // Start runs the container as a daemon.

=== modified file 'container/lxc/testing/test.go'
--- container/lxc/testing/test.go	2013-11-11 22:45:29 +0000
+++ container/lxc/testing/test.go	2014-03-11 03:19:30 +0000
@@ -33,6 +33,6 @@
 	s.PatchValue(&lxc.LxcContainerDir, s.LxcDir)
 	s.RestartDir = c.MkDir()
 	s.PatchValue(&lxc.LxcRestartDir, s.RestartDir)
-	s.Factory = mock.MockFactory()
+	s.Factory = mock.MockFactory(s.LxcDir)
 	s.PatchValue(&lxc.LxcObjectFactory, s.Factory)
 }