Code review comment for lp:~sidnei/juju-core/lxc-clone-with-overlayfs

Sidnei da Silva (sidnei) wrote :

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.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, 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.BackingStoreOverlayFS, 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 +207,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 +225,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

Index: container/lxc/mock/mock-lxc.go
=== modified file 'container/lxc/mock/mock-lxc.go'
--- container/lxc/mock/mock-lxc.go 2013-06-27 04:48:10 +0000
+++ container/lxc/mock/mock-lxc.go 2013-08-15 21:38:57 +0000
@@ -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, templateArgs ...string) (golxc.Container, error) {
   container := &mockContainer{
    factory: mock.factory,
    name: name,

« Back to merge proposal