Merge lp:~sidnei/juju-core/lxc-clone-with-overlayfs into lp:~go-bot/juju-core/trunk

[Sidnei da Silva]

Faster container creation via lxc-clone

Use the newer Clone arguments in golxc to request a snapshot container with a
specific backing store, create base template for selected distro if one
doesn't exist yet.

https://codereview.appspot.com/12801044/

[Ian Booth (wallyworld)]

Looks like a nice change.

I'd like to see some tests to ensure the new template stuff is processed
correctly. eg is the container template created as expected, is it only
done once, is the filename done correctly if manager name is "" etc etc.

Also, maybe a comment as to why the newly added HUP is needed in the
upstart script.

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

https://codereview.appspot.com/12801044/diff/1/container/lxc/lxc.go#newcode133
container/lxc/lxc.go:133: template := lxcObjectFactory.New(templateName)

What if manager.name is ""? There's a few lines of code above this
accounts for the fact that manager name might be empty so I think
something similar is needed for making the template name also? Otherwise
we will end up with a template name like "-precise-template" if
manager.name is ""

https://codereview.appspot.com/12801044/

[William Reade (fwereade)]

Yeah, definitely needs tests, otherwise very nice. I guess the notify
problems with overlayfs aren't a big deal if we're never updating the
template underneath a container?

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

https://codereview.appspot.com/12801044/diff/1/container/lxc/lxc.go#newcode134
container/lxc/lxc.go:134: if !template.IsConstructed() {
Would it be reasonable to extract this block as a method?

https://codereview.appspot.com/12801044/

[Sidnei da Silva (sidnei)]

Reviewers: mp+180445_code.launchpad.net, wallyworld, fwereade,

Message:
On 2013/11/06 12:28:44, fwereade wrote:
> Yeah, definitely needs tests, otherwise very nice. I guess the notify
problems
> with overlayfs aren't a big deal if we're never updating the template
underneath
> a container?

They still are. I've abandoned the overlayfs idea entirely until its
issues are fixed in kernel. We'll default to -Bbest in lxc-create which
will use either btrfs or lvm (or zfs even) depending on how you
configured lxc itself.

Description:
Faster container creation via lxc-clone

Use the newer Clone arguments in golxc to request a snapshot container
with a
specific backing store, create base template for selected distro if one
doesn't exist yet.

https://code.launchpad.net/~sidnei/juju-core/lxc-clone-with-overlayfs/+merge/180445

(do not edit description out of merge proposal)

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

Affected files (+56, -32 lines):
   A [revision details]
   M container/lxc/lxc.go
   M container/lxc/mock/mock-lxc.go
   M upstart/upstart.go
   M upstart/upstart_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: tarmac-20130815174953-arsxsn7541v1jxs5
+New revision: <email address hidden>

Index: upstart/upstart.go
=== modified file 'upstart/upstart.go'
--- upstart/upstart.go 2013-05-02 15:55:42 +0000
+++ upstart/upstart.go 2013-08-15 22:58:14 +0000
@@ -176,6 +176,7 @@
   }
   return []string{
    fmt.Sprintf("cat >> %s << 'EOF'\n%sEOF\n", c.confPath(), conf),
+ "kill -HUP 1",
    "start " + c.Name,
   }, nil
  }

Index: upstart/upstart_test.go
=== modified file 'upstart/upstart_test.go'
--- upstart/upstart_test.go 2013-07-09 10:32:23 +0000
+++ upstart/upstart_test.go 2013-08-15 22:58:14 +0000
@@ -169,6 +169,7 @@
   c.Assert(err, IsNil)
   c.Assert(cmds, DeepEquals, []string{
    "cat >> " + expectPath + " << 'EOF'\n" + expectContent + "EOF\n",
+ "kill -HUP 1",
    "start some-service",
   })

Index: container/lxc/lxc.go
=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go 2013-08-12 17:18:37 +0000
+++ container/lxc/lxc.go 2013-08-15 22:18:16 +0000
@@ -120,65 +120,85 @@
   stateInfo *state.Info,
   apiInfo *api.Info) (instance.Instance, error) {

+ var container golxc.Container
+
   name := names.MachineTag(machineId)
   if manager.name != "" {
    name = fmt.Sprintf("%s-%s", manager.name, name)
   }
- // Note here that the lxcObjectFacotry only returns a valid container
+ // Note here that the lxcObjectFactory only returns a valid container
   // object, and doesn't actually construct the underlying lxc container on
   // disk.
- container := lxcObjectFactory.New(name)
+ templateName := fmt.Sprintf("%s-%s-template", manager.name, series)
+ template := lxcObjectFactory.New(templateName)
+ if !template.IsConstructed() {
+ templateDirectory := jujuContainerDirectory(templateName)
+ logger.Debugf("create directory: %s", templateDirectory)
+ if err := os.MkdirAll(templateDirectory, 0755); err != nil {
+ logger...

[Sidnei da Silva (sidnei)]

https://codereview.appspot.com/12801044/

[William Reade (fwereade)]

I still don't see tests for container/lxc -- surely with these new
params we should be testing at least that we get what's expected?

https://codereview.appspot.com/12801044/

Unmerged revisions

1676. By Sidnei da Silva on 2013-10-22

- Revert upstart change

1675. By Sidnei da Silva on 2013-10-22

- Default to auto

1674. By Sidnei da Silva on 2013-10-21

- Merge from trunk

1673. By Sidnei da Silva on 2013-08-20

- Expose more settings via create and clone

1672. By Sidnei da Silva on 2013-08-19

- Fallback to no snapshot.

1671. By Sidnei da Silva on 2013-08-16

- Switch to inictl instead

1670. By Sidnei da Silva on 2013-08-15

- kill -HUP upstart so it picks up the new service.

1669. By Sidnei da Silva on 2013-08-15

- Some IRL fixes

1668. By Sidnei da Silva on 2013-08-15

- Minimal hack to get started

=== modified file 'container/lxc/lxc.go'
--- container/lxc/lxc.go	2013-10-01 02:33:14 +0000
+++ container/lxc/lxc.go	2013-10-22 22:30:22 +0000
@@ -30,6 +30,9 @@
 
 var (
 	defaultTemplate     = "ubuntu-cloud"
+	defaultFSSize       = "8G" // Same as (most) cloud images.
+	defaultFSType       = ""   // Let lxc pick a default.
+	defaultCloneSize    = "8G" // Only a hint to lxc, which may or may not get used.
 	containerDir        = "/var/lib/juju/containers"
 	removedContainerDir = "/var/lib/juju/removed-containers"
 	lxcContainerDir     = "/var/lib/lxc"
@@ -120,65 +123,89 @@
 	stateInfo *state.Info,
 	apiInfo *api.Info) (instance.Instance, error) {
 
+	var container golxc.Container
+
 	name := names.MachineTag(machineId)
 	if manager.name != "" {
 		name = fmt.Sprintf("%s-%s", manager.name, name)
 	}
-	// Note here that the lxcObjectFacotry only returns a valid container
+	// Note here that the lxcObjectFactory only returns a valid container
 	// object, and doesn't actually construct the underlying lxc container on
 	// disk.
-	container := lxcObjectFactory.New(name)
+	templateName := fmt.Sprintf("%s-%s-template", manager.name, series)
+	template := lxcObjectFactory.New(templateName)
+	if !template.IsConstructed() {
+		templateDirectory := jujuContainerDirectory(templateName)
+		logger.Debugf("create directory: %s", templateDirectory)
+		if err := os.MkdirAll(templateDirectory, 0755); err != nil {
+			logger.Errorf("failed to create template container directory: %v", err)
+			return nil, err
+		}
+		logger.Debugf("write the lxc.conf file")
+		configFile, err := writeLxcConfig(network, templateDirectory, manager.logdir)
+		if err != nil {
+			logger.Errorf("failed to write config file: %v", err)
+			return nil, err
+		}
+		templateParams := []string{
+			"--debug",                // Debug errors in the cloud image
+			"--hostid", templateName, // Use the container name as the hostid
+			"-r", series,
+		}
+		// Create the container.
+		logger.Debugf("create the container template")
+		if err := template.Create(configFile, golxc.BackingStoreAuto, defaultFSSize, defaultFSType, defaultTemplate, templateParams...); err != nil {
+			logger.Errorf("lxc template container creation failed: %v", err)
+			return nil, err
+		}
+		// Make sure that the mount dir has been created.
+		logger.Debugf("make the mount dir for the shard logs")
+		if err := os.MkdirAll(internalLogDir(templateName), 0755); err != nil {
+			logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
+			return nil, err
+		}
+	}
 
 	// Create the cloud-init.
 	directory := jujuContainerDirectory(name)
-	logger.Tracef("create directory: %s", directory)
+	logger.Debugf("create directory: %s", directory)
 	if err := os.MkdirAll(directory, 0755); err != nil {
 		logger.Errorf("failed to create container directory: %v", err)
 		return nil, err
 	}
-	logger.Tracef("write cloud-init")
+	logger.Debugf("write cloud-init")
 	userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)
 	if err != nil {
 		logger.Errorf("failed to write user data: %v", err)
 		return nil, err
 	}
-	logger.Tracef("write the lxc.conf file")
-	configFile, err := writeLxcConfig(network, directory, manager.logdir)
-	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,
-	}
-	// Create the container.
-	logger.Tracef("create the container")
-	if err := container.Create(configFile, defaultTemplate, 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 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, err
-	}
-	logger.Tracef("lxc container created")
+	}
+
+	// Clone existing template into a new container.
+	logger.Debugf("create the container clone")
+	if container, err = template.Clone(name, true, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
+		logger.Errorf("lxc container clone creation failed: %v, retrying without snapshot", err)
+		if container, err = template.Clone(name, false, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
+			logger.Errorf("lxc container clone creation failed: %v", err)
+			return nil, err
+		}
+	}
+
+	logger.Debugf("lxc container clone created")
 	// Now symlink the config file into the restart directory.
 	containerConfigFile := filepath.Join(lxcContainerDir, name, "config")
 	if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
 		return nil, err
 	}
-	logger.Tracef("auto-restart link created")
+	logger.Debugf("auto-restart link created")
 
 	// Start the lxc container with the appropriate settings for grabbing the
 	// console output and a log file.
 	consoleFile := filepath.Join(directory, "console.log")
 	container.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
-	logger.Tracef("start the container")
+	logger.Debugf("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
@@ -187,7 +214,7 @@
 		logger.Errorf("container failed to start: %v", err)
 		return nil, err
 	}
-	logger.Tracef("container started")
+	logger.Debugf("container started")
 	return &lxcInstance{container, name}, nil
 }
 
@@ -205,7 +232,7 @@
 	}
 
 	// Move the directory.
-	logger.Tracef("create old container dir: %s", removedContainerDir)
+	logger.Debugf("create old container dir: %s", removedContainerDir)
 	if err := os.MkdirAll(removedContainerDir, 0755); err != nil {
 		logger.Errorf("failed to create removed container directory: %v", err)
 		return err
 
=== 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	2013-10-22 22:30:22 +0000
@@ -68,7 +68,7 @@
 }
 
 // Create creates a new container based on the given template.
-func (mock *mockContainer) Create(configFile, template string, templateArgs ...string) error {
+func (mock *mockContainer) Create(configFile string, backingStore golxc.BackingStore, fsSize string, fsType string, template string, templateArgs ...string) error {
 	if mock.state != golxc.StateUnknown {
 		return fmt.Errorf("container is already created")
 	}
@@ -102,7 +102,7 @@
 }
 
 // Clone creates a copy of the container, giving the copy the specified name.
-func (mock *mockContainer) Clone(name string) (golxc.Container, error) {
+func (mock *mockContainer) Clone(name string, snapshot bool, backingStore golxc.BackingStore, fsSize string, templateArgs ...string) (golxc.Container, error) {
 	container := &mockContainer{
 		factory:  mock.factory,
 		name:     name,