Merge lp:~thumper/juju-core/fast-lxc-no-apt-upgrade into lp:~go-bot/juju-core/trunk

[Tim Penhey]

Use lxc clone on trusty

This branch speeds up lxc containers for the local
provider only by creating a template as needed that
persists between environments, and this template is
then used as a basis for the machines created in the
environment using lxc-clone.

If /var/lib/lxc is a btrfs filesystem, the containers
use btrfs snapshots. If not, the containers are
created using aufs snapshots.

In addition to this, the apt-get update/upgrade step
is skipped during cloud-init for the cloned machines.

https://codereview.appspot.com/74370044/

Use lxc clone on trusty

This branch speeds up lxc containers for the local
provider only by creating a template as needed that
persists between environments, and this template is
then used as a basis for the machines created in the
environment using lxc-clone.

If /var/lib/lxc is a btrfs filesystem, the containers
use btrfs snapshots. If not, the containers are
created using aufs snapshots.

In addition to this, the apt-get update/upgrade step
is skipped during cloud-init for the cloned machines.

Since the testing required a go routine to shut down
the template container, the mock containers had to be
made go routine safe for the state checking.

Certain extra info was also added for the mock containers,
being the config file and console log.

https://codereview.appspot.com/74370044/

[Tim Penhey (thumper)]

Reviewers: mp+210536_code.launchpad.net,

Message:
Please take a look.

Description:
Use lxc clone on trusty

This branch speeds up lxc containers for the local
provider only by creating a template as needed that
persists between environments, and this template is
then used as a basis for the machines created in the
environment using lxc-clone.

If /var/lib/lxc is a btrfs filesystem, the containers
use btrfs snapshots. If not, the containers are
created using aufs snapshots.

In addition to this, the apt-get update/upgrade step
is skipped during cloud-init for the cloned machines.

Since the testing required a go routine to shut down
the template container, the mock containers had to be
made go routine safe for the state checking.

Certain extra info was also added for the mock containers,
being the config file and console log.

https://code.launchpad.net/~thumper/juju-core/fast-lxc-no-apt-upgrade/+merge/210536

Requires:
https://code.launchpad.net/~thumper/juju-core/update-golxc-version/+merge/210492

(do not edit description out of merge proposal)

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

Affected files (+485, -46 lines):
   A [revision details]
   A container/lxc/clonetemplate.go
   M container/lxc/lxc.go
   M container/lxc/lxc_test.go
   M container/lxc/mock/mock-lxc.go
   M container/userdata.go
   M provider/local/environ.go
   M worker/provisioner/lxc-broker_test.go

[Ian Booth (wallyworld)]

I'm not fully across the finer details about lxc cloning. I think this
looks good. Perhaps a little more test coverage though. Specifically:
1. EnsureCloneTemplate behaves correctly if called when a template
already exists
2. The cloud init contents are devoid of apt stuff

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc.go#newcode92
container/lxc/lxc.go:92: if conf["use-clone"] == "true" {
Is there a helper function for str -> bool?

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc_test.go#newcode165
container/lxc/lxc_test.go:165: func (s *LxcSuite) AssertEvent(c *gc.C,
event mock.Event, expected mock.Action, id string) {
does this need to be exported?

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc_test.go#newcode203
container/lxc/lxc_test.go:203: c.Assert(template.Name(), gc.Equals,
name)
Should we be checking (some key aspects of) the contents of the
template?

https://codereview.appspot.com/74370044/

[Sidnei da Silva (sidnei)]

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/clonetemplate.go#newcode142
container/lxc/clonetemplate.go:142: if backingFilesystem == Btrfs {
I find it odd that backingFilesystem is special-cased for btrfs, since
the version of lxc in trusty has support for picking the right backing
filesystem if -Bbest is specified, which would work for either btrfs or
lvm thin provisioning.

https://codereview.appspot.com/74370044/

[Tim Penhey (thumper)]

Please take a look.

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/clonetemplate.go#newcode142
container/lxc/clonetemplate.go:142: if backingFilesystem == Btrfs {
On 2014/03/12 08:31:38, sidnei.da.silva wrote:
> I find it odd that backingFilesystem is special-cased for btrfs, since
the
> version of lxc in trusty has support for picking the right backing
filesystem if
> -Bbest is specified, which would work for either btrfs or lvm thin
provisioning.

man lxc-create doesn't show this.

Post chat: yes -B best does exist, but not documented. However, it
looks for the following in order: "btrfs", "zfs", "lvm", "dir". Since
we have no easy way to determine when to use "aufs" if we are allowing
"best", it is easier to be explicit in our support with btrfs now, and
have all others use aufs.
I'll talk with hallyn

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc.go#newcode92
container/lxc/lxc.go:92: if conf["use-clone"] == "true" {
On 2014/03/12 05:42:25, wallyworld wrote:
> Is there a helper function for str -> bool?

Yes there is. Using it now.

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

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc_test.go#newcode165
container/lxc/lxc_test.go:165: func (s *LxcSuite) AssertEvent(c *gc.C,
event mock.Event, expected mock.Action, id string) {
On 2014/03/12 05:42:25, wallyworld wrote:
> does this need to be exported?

No it doesn't, but it fits with the other Assert statements.

Since "exporting" a test function has no meaning, I went with
consistency.

https://codereview.appspot.com/74370044/diff/1/container/lxc/lxc_test.go#newcode203
container/lxc/lxc_test.go:203: c.Assert(template.Name(), gc.Equals,
name)
On 2014/03/12 05:42:25, wallyworld wrote:
> Should we be checking (some key aspects of) the contents of the
template?

We could make sure that it isn't autostarting nor mounting the logdir by
looking at the config file. Worth it?

https://codereview.appspot.com/74370044/

[Ian Booth (wallyworld)]

LGTM. Let's get this landed, but I'd still like to see tests that the
template doesn't start etc.

https://codereview.appspot.com/74370044/

=== modified file 'container/interface.go'
--- container/interface.go	2014-03-03 02:02:25 +0000
+++ container/interface.go	2014-03-13 02:11:05 +0000
@@ -39,3 +39,19 @@
 	// that the host machine can run containers.
 	Initialise() error
 }
+
+// PopValue returns the requested key from the config map. If the value
+// doesn't exist, the function returns the empty string. If the value does
+// exist, the value is returned, and the element removed from the map.
+func (m ManagerConfig) PopValue(key string) string {
+	value := m[key]
+	delete(m, key)
+	return value
+}
+
+// WarnAboutUnused emits a warning about each value in the map.
+func (m ManagerConfig) WarnAboutUnused() {
+	for key, value := range m {
+		logger.Warningf("unused config option: %q -> %q", key, value)
+	}
+}

=== modified file 'container/kvm/kvm.go'
--- container/kvm/kvm.go	2014-03-11 01:17:39 +0000
+++ container/kvm/kvm.go	2014-03-13 02:11:05 +0000
@@ -56,20 +56,15 @@
 // containers. The containers that are created are namespaced by the name
 // parameter.
 func NewContainerManager(conf container.ManagerConfig) (container.Manager, error) {
-	name := conf[container.ConfigName]
-	delete(conf, container.ConfigName)
+	name := conf.PopValue(container.ConfigName)
 	if name == "" {
 		return nil, fmt.Errorf("name is required")
 	}
-	logDir := conf[container.ConfigLogDir]
-	delete(conf, container.ConfigLogDir)
+	logDir := conf.PopValue(container.ConfigLogDir)
 	if logDir == "" {
 		logDir = agent.DefaultLogDir
 	}
-	for k, v := range conf {
-		logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
-	}
-
+	conf.WarnAboutUnused()
 	return &containerManager{name: name, logdir: logDir}, nil
 }
 

=== modified file 'container/kvm/kvm_test.go'
--- container/kvm/kvm_test.go	2014-03-11 01:17:39 +0000
+++ container/kvm/kvm_test.go	2014-03-13 02:11:05 +0000
@@ -46,7 +46,7 @@
 		"shazam":             "Captain Marvel",
 	})
 	c.Assert(err, gc.IsNil)
-	c.Assert(c.GetTestLog(), gc.Matches, `^.*WARNING juju.container.kvm Found unused config option with key: "shazam" and value: "Captain Marvel"\n*`)
+	c.Assert(c.GetTestLog(), jc.Contains, `WARNING juju.container unused config option: "shazam" -> "Captain Marvel"`)
 }
 
 func (s *KVMSuite) TestListInitiallyEmpty(c *gc.C) {

=== added file 'container/lxc/clonetemplate.go'
--- container/lxc/clonetemplate.go	1970-01-01 00:00:00 +0000
+++ container/lxc/clonetemplate.go	2014-03-13 02:11:05 +0000
@@ -0,0 +1,219 @@
+// Copyright 2014 Canonical Ltd.
+// Licensed under the AGPLv3, see LICENCE file for details.
+
+package lxc
+
+import (
+	"fmt"
+	"io"
+	"os"
+	"path/filepath"
+	"sync"
+	"time"
+
+	"launchpad.net/golxc"
+
+	coreCloudinit "launchpad.net/juju-core/cloudinit"
+	"launchpad.net/juju-core/container"
+	"launchpad.net/juju-core/environs/cloudinit"
+	"launchpad.net/juju-core/juju/osenv"
+	"launchpad.net/juju-core/utils"
+	"launchpad.net/juju-core/utils/fslock"
+	"launchpad.net/juju-core/utils/tailer"
+)
+
+const (
+	templateShutdownUpstartFilename = "/etc/init/juju-template-restart.conf"
+	templateShutdownUpstartScript   = `
+description "Juju lxc template shutdown job"
+author "Juju Team <[e-mail]>"
+start on stopped cloud-final
+
+script
+  shutdown -h now
+end script
+
+post-stop script
+  rm ` + templateShutdownUpstartFilename + `
+end script
+`
+)
+
+var (
+	TemplateLockDir = "/var/lib/juju/locks"
+
+	TemplateStopTimeout = 5 * time.Minute
+)
+
+// templateUserData returns a minimal user data necessary for the template.
+// This should have the authorized keys, base packages, the cloud archive if
+// necessary,  initial apt proxy config, and it should do the apt-get
+// update/upgrade initially.
+func templateUserData(
+	series string,
+	authorizedKeys string,
+	aptProxy osenv.ProxySettings,
+) ([]byte, error) {
+	config := coreCloudinit.New()
+	config.AddScripts(
+		"set -xe", // ensure we run all the scripts or abort.
+	)
+	config.AddSSHAuthorizedKeys(authorizedKeys)
+	cloudinit.MaybeAddCloudArchiveCloudTools(config, series)
+	cloudinit.AddAptCommands(aptProxy, config)
+	config.AddScripts(
+		fmt.Sprintf(
+			"printf '%%s\n' %s > %s",
+			utils.ShQuote(templateShutdownUpstartScript),
+			templateShutdownUpstartFilename,
+		))
+	data, err := config.Render()
+	if err != nil {
+		return nil, err
+	}
+	return data, nil
+}
+
+func AcquireTemplateLock(name, message string) (*fslock.Lock, error) {
+	logger.Infof("wait for fslock on %v", name)
+	lock, err := fslock.NewLock(TemplateLockDir, name)
+	if err != nil {
+		logger.Tracef("failed to create fslock for template: %v", err)
+		return nil, err
+	}
+	err = lock.Lock(message)
+	if err != nil {
+		logger.Tracef("failed to acquire lock for template: %v", err)
+		return nil, err
+	}
+	return lock, nil
+}
+
+// Make sure a template exists that we can clone from.
+func EnsureCloneTemplate(
+	backingFilesystem string,
+	series string,
+	network *container.NetworkConfig,
+	authorizedKeys string,
+	aptProxy osenv.ProxySettings,
+) (golxc.Container, error) {
+	name := fmt.Sprintf("juju-%s-template", series)
+	containerDirectory, err := container.NewDirectory(name)
+	if err != nil {
+		return nil, err
+	}
+
+	lock, err := AcquireTemplateLock(name, "ensure clone exists")
+	if err != nil {
+		return nil, err
+	}
+	defer lock.Unlock()
+
+	lxcContainer := LxcObjectFactory.New(name)
+	// Early exit if the container has been constructed before.
+	if lxcContainer.IsConstructed() {
+		logger.Infof("template exists, continuing")
+		return lxcContainer, nil
+	}
+	logger.Infof("template does not exist, creating")
+
+	userData, err := templateUserData(series, authorizedKeys, aptProxy)
+	if err != nil {
+		logger.Tracef("filed to create tempalte user data for template: %v", err)
+		return nil, err
+	}
+	userDataFilename, err := container.WriteCloudInitFile(containerDirectory, userData)
+	if err != nil {
+		return nil, err
+	}
+
+	configFile, err := writeLxcConfig(network, containerDirectory)
+	if err != nil {
+		logger.Errorf("failed to write config file: %v", err)
+		return nil, err
+	}
+	templateParams := []string{
+		"--debug",                      // Debug errors in the cloud image
+		"--userdata", userDataFilename, // Our groovey cloud-init
+		"--hostid", name, // Use the container name as the hostid
+		"-r", series,
+	}
+	var extraCreateArgs []string
+	if backingFilesystem == Btrfs {
+		extraCreateArgs = append(extraCreateArgs, "-B", Btrfs)
+	}
+	// Create the container.
+	logger.Tracef("create the container")
+	if err := lxcContainer.Create(configFile, defaultTemplate, extraCreateArgs, templateParams); err != nil {
+		logger.Errorf("lxc container creation failed: %v", err)
+		return nil, err
+	}
+	// 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.Tracef("failed to create internal /var/log/juju mount dir: %v", err)
+		return nil, err
+	}
+
+	// Start the lxc container with the appropriate settings for grabbing the
+	// console output and a log file.
+	consoleFile := filepath.Join(containerDirectory, "console.log")
+	lxcContainer.SetLogFile(filepath.Join(containerDirectory, "container.log"), golxc.LogDebug)
+	logger.Tracef("start the container")
+	// We explicitly don't pass through the config file to the container.Start
+	// method as we have passed it through at container creation time.  This
+	// is necessary to get the appropriate rootfs reference without explicitly
+	// setting it ourselves.
+	if err = lxcContainer.Start("", consoleFile); err != nil {
+		logger.Errorf("container failed to start: %v", err)
+		return nil, err
+	}
+	logger.Infof("template container started, now wait for it to stop")
+	// Perhaps we should wait for it to finish, and the question becomes "how
+	// long do we wait for it to complete?"
+
+	console, err := os.Open(consoleFile)
+	if err != nil {
+		// can't listen
+		return nil, err
+	}
+
+	tailWriter := &logTail{tick: time.Now()}
+	consoleTailer := tailer.NewTailer(console, tailWriter, 0, nil)
+	defer consoleTailer.Stop()
+
+	// We should wait maybe 1 minute between output?
+	// if no output check to see if stopped
+	// If we have no output and still running, something has probably gone wrong
+	for lxcContainer.IsRunning() {
+		if tailWriter.lastTick().Before(time.Now().Add(-TemplateStopTimeout)) {
+			logger.Infof("not heard anything from the template log for five minutes")
+			return nil, fmt.Errorf("template container %q did not stop", name)
+		}
+		time.Sleep(time.Second)
+	}
+
+	return lxcContainer, nil
+}
+
+type logTail struct {
+	tick  time.Time
+	mutex sync.Mutex
+}
+
+var _ io.Writer = (*logTail)(nil)
+
+func (t *logTail) Write(data []byte) (int, error) {
+	logger.Tracef(string(data))
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
+	t.tick = time.Now()
+	return len(data), nil
+}
+
+func (t *logTail) lastTick() time.Time {
+	t.mutex.Lock()
+	defer t.mutex.Unlock()
+	tick := t.tick
+	return tick
+}

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2014-03-12 02:13:38 +0000
+++ container/lxc/lxc.go	2014-03-13 02:11:05 +0000
@@ -9,7 +9,9 @@
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strconv"
 	"strings"
+	"time"
 
 	"github.com/juju/loggo"
 	"launchpad.net/golxc"
@@ -66,6 +68,7 @@
 type containerManager struct {
 	name              string
 	logdir            string
+	createWithClone   bool
 	backingFilesystem string
 }
 
@@ -76,16 +79,18 @@
 // containers. The containers that are created are namespaced by the name
 // parameter.
 func NewContainerManager(conf container.ManagerConfig) (container.Manager, error) {
-	name := conf[container.ConfigName]
-	delete(conf, container.ConfigName)
+	name := conf.PopValue(container.ConfigName)
 	if name == "" {
 		return nil, fmt.Errorf("name is required")
 	}
-	logDir := conf[container.ConfigLogDir]
-	delete(conf, container.ConfigLogDir)
+	logDir := conf.PopValue(container.ConfigLogDir)
 	if logDir == "" {
 		logDir = agent.DefaultLogDir
 	}
+	// Explicitly ignore the error result from ParseBool.
+	// If it fails to parse, the value is false, and this suits
+	// us fine.
+	useClone, _ := strconv.ParseBool(conf.PopValue("use-clone"))
 	backingFS, err := containerDirFilesystem()
 	if err != nil {
 		// Especially in tests, or a bot, the lxc dir may not exist
@@ -95,12 +100,11 @@
 		backingFS = "unknown"
 	}
 	logger.Tracef("backing filesystem: %q", backingFS)
-	for k, v := range conf {
-		logger.Warningf(`Found unused config option with key: "%v" and value: "%v"`, k, v)
-	}
+	conf.WarnAboutUnused()
 	return &containerManager{
 		name:              name,
 		logdir:            logDir,
+		createWithClone:   useClone,
 		backingFilesystem: backingFS,
 	}, nil
 }
@@ -108,23 +112,23 @@
 func (manager *containerManager) StartContainer(
 	machineConfig *cloudinit.MachineConfig,
 	series string,
-	network *container.NetworkConfig) (instance.Instance, *instance.HardwareCharacteristics, error) {
-
+	network *container.NetworkConfig,
+) (instance.Instance, *instance.HardwareCharacteristics, error) {
+	start := time.Now()
 	name := names.MachineTag(machineConfig.MachineId)
 	if manager.name != "" {
 		name = fmt.Sprintf("%s-%s", manager.name, name)
 	}
-	// Note here that the lxcObjectFacotry only returns a valid container
-	// object, and doesn't actually construct the underlying lxc container on
-	// disk.
-	lxcContainer := LxcObjectFactory.New(name)
-
 	// Create the cloud-init.
 	directory, err := container.NewDirectory(name)
 	if err != nil {
 		return nil, nil, err
 	}
 	logger.Tracef("write cloud-init")
+	if manager.createWithClone {
+		// If we are using clone, disable the apt-get steps
+		machineConfig.DisablePackageCommands = true
+	}
 	userDataFilename, err := container.WriteUserData(machineConfig, directory)
 	if err != nil {
 		logger.Errorf("failed to write user data: %v", err)
@@ -136,20 +140,58 @@
 		logger.Errorf("failed to write config file: %v", err)
 		return nil, nil, err
 	}
-	templateParams := []string{
-		"--debug",                      // Debug errors in the cloud image
-		"--userdata", userDataFilename, // Our groovey cloud-init
-		"--hostid", name, // Use the container name as the hostid
-		"-r", series,
-	}
-	// Create the container.
-	logger.Tracef("create the container")
-	if err := lxcContainer.Create(configFile, defaultTemplate, nil, templateParams); err != nil {
-		logger.Errorf("lxc container creation failed: %v", err)
-		return nil, nil, err
-	}
-	logger.Tracef("lxc container created")
-
+
+	var lxcContainer golxc.Container
+	if manager.createWithClone {
+		templateContainer, err := EnsureCloneTemplate(
+			manager.backingFilesystem,
+			series,
+			network,
+			machineConfig.AuthorizedKeys,
+			machineConfig.AptProxySettings,
+		)
+		if err != nil {
+			return nil, nil, err
+		}
+		templateParams := []string{
+			"--debug",                      // Debug errors in the cloud image
+			"--userdata", userDataFilename, // Our groovey cloud-init
+			"--hostid", name, // Use the container name as the hostid
+		}
+		extraCloneArgs := []string{"--snapshot"}
+		if manager.backingFilesystem != Btrfs {
+			extraCloneArgs = append(extraCloneArgs, "--backingstore", "aufs")
+		}
+
+		lock, err := AcquireTemplateLock(templateContainer.Name(), "clone")
+		if err != nil {
+			return nil, nil, fmt.Errorf("failed to acquire lock on template: %v", err)
+		}
+		defer lock.Unlock()
+		lxcContainer, err = templateContainer.Clone(name, extraCloneArgs, templateParams)
+		if err != nil {
+			logger.Errorf("lxc container cloning failed: %v", err)
+			return nil, nil, err
+		}
+	} else {
+		// Note here that the lxcObjectFacotry only returns a valid container
+		// object, and doesn't actually construct the underlying lxc container on
+		// disk.
+		lxcContainer = LxcObjectFactory.New(name)
+		templateParams := []string{
+			"--debug",                      // Debug errors in the cloud image
+			"--userdata", userDataFilename, // Our groovey cloud-init
+			"--hostid", name, // Use the container name as the hostid
+			"-r", series,
+		}
+		// Create the container.
+		logger.Tracef("create the container")
+		if err := lxcContainer.Create(configFile, defaultTemplate, nil, templateParams); err != nil {
+			logger.Errorf("lxc container creation failed: %v", err)
+			return nil, nil, err
+		}
+		logger.Tracef("lxc container created")
+	}
 	if err := autostartContainer(name); err != nil {
 		return nil, nil, err
 	}
@@ -173,7 +215,7 @@
 	hardware := &instance.HardwareCharacteristics{
 		Arch: &arch,
 	}
-	logger.Tracef("container started")
+	logger.Tracef("container %q started: %v", name, time.Now().Sub(start))
 	return &lxcInstance{lxcContainer, name}, hardware, nil
 }
 
@@ -222,6 +264,7 @@
 }
 
 func (manager *containerManager) StopContainer(instance instance.Instance) error {
+	start := time.Now()
 	name := string(instance.Id())
 	lxcContainer := LxcObjectFactory.New(name)
 	if useRestartDir() {
@@ -236,7 +279,9 @@
 		return err
 	}
 
-	return container.RemoveDirectory(name)
+	err := container.RemoveDirectory(name)
+	logger.Tracef("container %q stopped: %v", name, time.Now().Sub(start))
+	return err
 }
 
 func (manager *containerManager) ListContainers() (result []instance.Instance, err error) {

=== modified file 'container/lxc/lxc_test.go'
--- container/lxc/lxc_test.go	2014-03-12 01:27:57 +0000
+++ container/lxc/lxc_test.go	2014-03-13 02:11:05 +0000
@@ -10,6 +10,7 @@
 	"path/filepath"
 	"strings"
 	stdtesting "testing"
+	"time"
 
 	"github.com/juju/loggo"
 	gc "launchpad.net/gocheck"
@@ -19,9 +20,11 @@
 	"launchpad.net/juju-core/agent"
 	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/container/lxc"
+	"launchpad.net/juju-core/container/lxc/mock"
 	lxctesting "launchpad.net/juju-core/container/lxc/testing"
 	containertesting "launchpad.net/juju-core/container/testing"
 	instancetest "launchpad.net/juju-core/instance/testing"
+	"launchpad.net/juju-core/juju/osenv"
 	jc "launchpad.net/juju-core/testing/checkers"
 	"launchpad.net/juju-core/testing/testbase"
 )
@@ -32,6 +35,9 @@
 
 type LxcSuite struct {
 	lxctesting.TestSuite
+
+	events   chan mock.Event
+	useClone bool
 }
 
 var _ = gc.Suite(&LxcSuite{})
@@ -39,6 +45,16 @@
 func (s *LxcSuite) SetUpTest(c *gc.C) {
 	s.TestSuite.SetUpTest(c)
 	loggo.GetLogger("juju.container.lxc").SetLogLevel(loggo.TRACE)
+	s.events = make(chan mock.Event, 25)
+	s.TestSuite.Factory.AddListener(s.events)
+	s.PatchValue(&lxc.TemplateLockDir, c.MkDir())
+	s.PatchValue(&lxc.TemplateStopTimeout, 500*time.Millisecond)
+}
+
+func (s *LxcSuite) TearDownTest(c *gc.C) {
+	s.TestSuite.Factory.RemoveListener(s.events)
+	close(s.events)
+	s.TestSuite.TearDownTest(c)
 }
 
 func (s *LxcSuite) TestContainerDirFilesystem(c *gc.C) {
@@ -73,9 +89,13 @@
 }
 
 func (s *LxcSuite) makeManager(c *gc.C, name string) container.Manager {
-	manager, err := lxc.NewContainerManager(container.ManagerConfig{
+	params := container.ManagerConfig{
 		container.ConfigName: name,
-	})
+	}
+	if s.useClone {
+		params["use-clone"] = "true"
+	}
+	manager, err := lxc.NewContainerManager(params)
 	c.Assert(err, gc.IsNil)
 	return manager
 }
@@ -86,7 +106,7 @@
 		"shazam":             "Captain Marvel",
 	})
 	c.Assert(err, gc.IsNil)
-	c.Assert(c.GetTestLog(), jc.Contains, `WARNING juju.container.lxc Found unused config option with key: "shazam" and value: "Captain Marvel"`)
+	c.Assert(c.GetTestLog(), jc.Contains, `WARNING juju.container unused config option: "shazam" -> "Captain Marvel"`)
 }
 
 func (s *LxcSuite) TestStartContainer(c *gc.C) {
@@ -130,6 +150,93 @@
 	c.Assert(location, gc.Equals, expectedTarget)
 }
 
+func (s *LxcSuite) ensureTemplateStopped(name string) {
+	go func() {
+		for {
+			template := s.Factory.New(name)
+			if template.IsRunning() {
+				template.Stop()
+			}
+			time.Sleep(50 * time.Millisecond)
+		}
+	}()
+}
+
+func (s *LxcSuite) AssertEvent(c *gc.C, event mock.Event, expected mock.Action, id string) {
+	c.Assert(event.Action, gc.Equals, expected)
+	c.Assert(event.InstanceId, gc.Equals, id)
+}
+
+func (s *LxcSuite) TestStartContainerEvents(c *gc.C) {
+	manager := s.makeManager(c, "test")
+	instance := containertesting.StartContainer(c, manager, "1")
+	id := string(instance.Id())
+	s.AssertEvent(c, <-s.events, mock.Created, id)
+	s.AssertEvent(c, <-s.events, mock.Started, id)
+}
+
+func (s *LxcSuite) TestStartContainerEventsWithClone(c *gc.C) {
+	s.PatchValue(&s.useClone, true)
+	// The template containers are created with an upstart job that
+	// stops them once cloud init has finished.  We emulate that here.
+	template := "juju-series-template"
+	s.ensureTemplateStopped(template)
+	manager := s.makeManager(c, "test")
+	instance := containertesting.StartContainer(c, manager, "1")
+	id := string(instance.Id())
+	s.AssertEvent(c, <-s.events, mock.Created, template)
+	s.AssertEvent(c, <-s.events, mock.Started, template)
+	s.AssertEvent(c, <-s.events, mock.Stopped, template)
+	s.AssertEvent(c, <-s.events, mock.Cloned, template)
+	s.AssertEvent(c, <-s.events, mock.Started, id)
+}
+
+func (s *LxcSuite) createTemplate(c *gc.C) golxc.Container {
+	name := "juju-series-template"
+	s.ensureTemplateStopped(name)
+	network := container.BridgeNetworkConfig("nic42")
+	authorizedKeys := "authorized keys list"
+	aptProxy := osenv.ProxySettings{}
+	template, err := lxc.EnsureCloneTemplate(
+		"ext4", "series", network, authorizedKeys, aptProxy)
+	c.Assert(err, gc.IsNil)
+	c.Assert(template.Name(), gc.Equals, name)
+	s.AssertEvent(c, <-s.events, mock.Created, name)
+	s.AssertEvent(c, <-s.events, mock.Started, name)
+	s.AssertEvent(c, <-s.events, mock.Stopped, name)
+
+	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
+`
+	// NOTE: no autostart, no mounting the log dir
+	c.Assert(string(config), gc.Equals, expected)
+	c.Assert(autostartLink, jc.DoesNotExist)
+
+	return template
+}
+
+func (s *LxcSuite) TestStartContainerEventsWithCloneExistingTemplate(c *gc.C) {
+	s.createTemplate(c)
+	s.PatchValue(&s.useClone, true)
+	manager := s.makeManager(c, "test")
+	instance := containertesting.StartContainer(c, manager, "1")
+	name := string(instance.Id())
+	s.AssertEvent(c, <-s.events, mock.Cloned, "juju-series-template")
+	s.AssertEvent(c, <-s.events, mock.Started, name)
+
+	autostartLink := lxc.RestartSymlink(name)
+	config, err := ioutil.ReadFile(lxc.ContainerConfigFilename(name))
+	c.Assert(err, gc.IsNil)
+	mountLine := "lxc.mount.entry=/var/log/juju var/log/juju none defaults,bind 0 0"
+	c.Assert(string(config), jc.Contains, mountLine)
+	c.Assert(autostartLink, jc.IsSymlink)
+}
+
 func (s *LxcSuite) TestContainerState(c *gc.C) {
 	manager := s.makeManager(c, "test")
 	c.Logf("%#v", manager)

=== modified file 'container/lxc/mock/mock-lxc.go'
--- container/lxc/mock/mock-lxc.go	2014-03-11 03:49:20 +0000
+++ container/lxc/mock/mock-lxc.go	2014-03-13 02:11:05 +0000
@@ -5,17 +5,23 @@
 
 import (
 	"fmt"
+	"io/ioutil"
 	"os"
 	"path/filepath"
+	"sync"
 
+	"github.com/juju/loggo"
 	"launchpad.net/golxc"
 
+	"launchpad.net/juju-core/container"
 	"launchpad.net/juju-core/utils"
 )
 
 // This file provides a mock implementation of the golxc interfaces
 // ContainerFactory and Container.
 
+var logger = loggo.GetLogger("juju.container.lxc.mock")
+
 type Action int
 
 const (
@@ -23,6 +29,12 @@
 	Started Action = iota
 	// A container has been stopped.
 	Stopped
+	// A container has been created.
+	Created
+	// A container has been destroyed.
+	Destroyed
+	// A container has been cloned.
+	Cloned
 )
 
 func (action Action) String() string {
@@ -31,6 +43,12 @@
 		return "Started"
 	case Stopped:
 		return "Stopped"
+	case Created:
+		return "Created"
+	case Destroyed:
+		return "Destroyed"
+	case Cloned:
+		return "Cloned"
 	}
 	return "unknown"
 }
@@ -51,6 +69,7 @@
 	containerDir string
 	instances    map[string]golxc.Container
 	listeners    []chan<- Event
+	mutex        sync.Mutex
 }
 
 func MockFactory(containerDir string) ContainerFactory {
@@ -68,53 +87,85 @@
 	logLevel golxc.LogLevel
 }
 
+func (mock *mockContainer) getState() golxc.State {
+	mock.factory.mutex.Lock()
+	defer mock.factory.mutex.Unlock()
+	return mock.state
+}
+
+func (mock *mockContainer) setState(newState golxc.State) {
+	mock.factory.mutex.Lock()
+	defer mock.factory.mutex.Unlock()
+	mock.state = newState
+	logger.Debugf("container %q state change to %s", mock.name, string(newState))
+}
+
 // Name returns the name of the container.
 func (mock *mockContainer) Name() string {
 	return mock.name
 }
 
+func (mock *mockContainer) configFilename() string {
+	return filepath.Join(mock.factory.containerDir, mock.name, "config")
+}
+
 // Create creates a new container based on the given template.
 func (mock *mockContainer) Create(configFile, template string, extraArgs []string, templateArgs []string) error {
-	if mock.state != golxc.StateUnknown {
+	if mock.getState() != golxc.StateUnknown {
 		return fmt.Errorf("container is already created")
 	}
-	mock.state = golxc.StateStopped
 	mock.factory.instances[mock.name] = mock
 	// 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)
+	if err := utils.CopyFile(mock.configFilename(), configFile); err != nil {
+		return err
+	}
+	mock.setState(golxc.StateStopped)
+	mock.factory.notify(Created, mock.name)
+	return nil
 }
 
 // Start runs the container as a daemon.
 func (mock *mockContainer) Start(configFile, consoleFile string) error {
-	if mock.state == golxc.StateUnknown {
+	state := mock.getState()
+	if state == golxc.StateUnknown {
 		return fmt.Errorf("container has not been created")
-	} else if mock.state == golxc.StateRunning {
+	} else if state == golxc.StateRunning {
 		return fmt.Errorf("container is already running")
 	}
-	mock.state = golxc.StateRunning
+	ioutil.WriteFile(
+		filepath.Join(container.ContainerDir, mock.name, "console.log"),
+		[]byte("fake console.log"), 0644)
+	mock.setState(golxc.StateRunning)
 	mock.factory.notify(Started, mock.name)
 	return nil
 }
 
 // Stop terminates the running container.
 func (mock *mockContainer) Stop() error {
-	if mock.state == golxc.StateUnknown {
+	state := mock.getState()
+	if state == golxc.StateUnknown {
 		return fmt.Errorf("container has not been created")
-	} else if mock.state == golxc.StateStopped {
+	} else if state == golxc.StateStopped {
 		return fmt.Errorf("container is already stopped")
 	}
-	mock.state = golxc.StateStopped
+	mock.setState(golxc.StateStopped)
 	mock.factory.notify(Stopped, mock.name)
 	return nil
 }
 
 // Clone creates a copy of the container, giving the copy the specified name.
 func (mock *mockContainer) Clone(name string, extraArgs []string, templateArgs []string) (golxc.Container, error) {
+	state := mock.getState()
+	if state == golxc.StateUnknown {
+		return nil, fmt.Errorf("container has not been created")
+	} else if state == golxc.StateRunning {
+		return nil, fmt.Errorf("container is running, clone not possible")
+	}
+
 	container := &mockContainer{
 		factory:  mock.factory,
 		name:     name,
@@ -122,6 +173,17 @@
 		logLevel: golxc.LogWarning,
 	}
 	mock.factory.instances[name] = container
+
+	// Create the container directory.
+	containerDir := filepath.Join(mock.factory.containerDir, name)
+	if err := os.MkdirAll(containerDir, 0755); err != nil {
+		return nil, err
+	}
+	if err := utils.CopyFile(container.configFilename(), mock.configFilename()); err != nil {
+		return nil, err
+	}
+
+	mock.factory.notify(Cloned, mock.name)
 	return container, nil
 }
 
@@ -137,15 +199,17 @@
 
 // Destroy stops and removes the container.
 func (mock *mockContainer) Destroy() error {
+	state := mock.getState()
 	// golxc destroy will stop the machine if it is running.
-	if mock.state == golxc.StateRunning {
+	if state == golxc.StateRunning {
 		mock.Stop()
 	}
-	if mock.state == golxc.StateUnknown {
+	if state == golxc.StateUnknown {
 		return fmt.Errorf("container has not been created")
 	}
-	mock.state = golxc.StateUnknown
 	delete(mock.factory.instances, mock.name)
+	mock.setState(golxc.StateUnknown)
+	mock.factory.notify(Destroyed, mock.name)
 	return nil
 }
 
@@ -157,27 +221,28 @@
 // Info returns the status and the process id of the container.
 func (mock *mockContainer) Info() (golxc.State, int, error) {
 	pid := -1
-	if mock.state == golxc.StateRunning {
+	state := mock.getState()
+	if state == golxc.StateRunning {
 		pid = 42
 	}
-	return mock.state, pid, nil
+	return state, pid, nil
 }
 
 // IsConstructed checks if the container image exists.
 func (mock *mockContainer) IsConstructed() bool {
-	return mock.state != golxc.StateUnknown
+	return mock.getState() != golxc.StateUnknown
 }
 
 // IsRunning checks if the state of the container is 'RUNNING'.
 func (mock *mockContainer) IsRunning() bool {
-	return mock.state == golxc.StateRunning
+	return mock.getState() == golxc.StateRunning
 }
 
 // String returns information about the container, like the name, state,
 // and process id.
 func (mock *mockContainer) String() string {
-	_, pid, _ := mock.Info()
-	return fmt.Sprintf("<MockContainer %q, state: %s, pid %d>", mock.name, string(mock.state), pid)
+	state, pid, _ := mock.Info()
+	return fmt.Sprintf("<MockContainer %q, state: %s, pid %d>", mock.name, string(state), pid)
 }
 
 // LogFile returns the current filename used for the LogFile.
@@ -202,6 +267,8 @@
 }
 
 func (mock *mockFactory) New(name string) golxc.Container {
+	mock.mutex.Lock()
+	defer mock.mutex.Unlock()
 	container, ok := mock.instances[name]
 	if ok {
 		return container
@@ -212,6 +279,7 @@
 		state:    golxc.StateUnknown,
 		logLevel: golxc.LogWarning,
 	}
+	mock.instances[name] = container
 	return container
 }
 

=== modified file 'container/userdata.go'
--- container/userdata.go	2014-03-12 02:28:30 +0000
+++ container/userdata.go	2014-03-13 02:11:05 +0000
@@ -9,7 +9,6 @@
 
 	"github.com/juju/loggo"
 
-	"launchpad.net/juju-core/agent"
 	coreCloudinit "launchpad.net/juju-core/cloudinit"
 	"launchpad.net/juju-core/environs/cloudinit"
 )
@@ -18,12 +17,21 @@
 	logger = loggo.GetLogger("juju.container")
 )
 
+// WriteUserData generates the cloud init for the specified machine config,
+// and writes the serialized form out to a cloud-init file in the directory
+// specified.
 func WriteUserData(machineConfig *cloudinit.MachineConfig, directory string) (string, error) {
 	userData, err := cloudInitUserData(machineConfig)
 	if err != nil {
 		logger.Errorf("failed to create user data: %v", err)
 		return "", err
 	}
+	return WriteCloudInitFile(directory, userData)
+}
+
+// WriteCloudInitFile writes the data out to a cloud-init file in the
+// directory specified, and returns the filename.
+func WriteCloudInitFile(directory string, userData []byte) (string, error) {
 	userDataFilename := filepath.Join(directory, "cloud-init")
 	if err := ioutil.WriteFile(userDataFilename, userData, 0644); err != nil {
 		logger.Errorf("failed to write user data: %v", err)
@@ -33,8 +41,6 @@
 }
 
 func cloudInitUserData(machineConfig *cloudinit.MachineConfig) ([]byte, error) {
-	// consider not having this line hardcoded...
-	machineConfig.DataDir = agent.DefaultDataDir
 	cloudConfig := coreCloudinit.New()
 	err := cloudinit.Configure(machineConfig, cloudConfig)
 	if err != nil {

=== modified file 'provider/local/environ.go'
--- provider/local/environ.go	2014-03-09 20:53:17 +0000
+++ provider/local/environ.go	2014-03-13 02:11:05 +0000
@@ -10,6 +10,7 @@
 	"os/exec"
 	"path/filepath"
 	"regexp"
+	"strconv"
 	"strings"
 	"sync"
 	"syscall"
@@ -219,6 +220,7 @@
 		container.ManagerConfig{
 			container.ConfigName:   env.config.namespace(),
 			container.ConfigLogDir: env.config.logDir(),
+			"use-clone":            strconv.FormatBool(env.fastLXC),
 		})
 	if err != nil {
 		return err

=== modified file 'worker/provisioner/lxc-broker_test.go'
--- worker/provisioner/lxc-broker_test.go	2014-03-07 03:00:24 +0000
+++ worker/provisioner/lxc-broker_test.go	2014-03-13 02:11:05 +0000
@@ -204,6 +204,8 @@
 func (s *lxcProvisionerSuite) expectStarted(c *gc.C, machine *state.Machine) string {
 	s.State.StartSync()
 	event := <-s.events
+	c.Assert(event.Action, gc.Equals, mock.Created)
+	event = <-s.events
 	c.Assert(event.Action, gc.Equals, mock.Started)
 	err := machine.Refresh()
 	c.Assert(err, gc.IsNil)
@@ -215,6 +217,8 @@
 	s.State.StartSync()
 	event := <-s.events
 	c.Assert(event.Action, gc.Equals, mock.Stopped)
+	event = <-s.events
+	c.Assert(event.Action, gc.Equals, mock.Destroyed)
 	c.Assert(event.InstanceId, gc.Equals, instId)
 }