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

Proposed by Sidnei da Silva on 2013-08-15
Status: Work in progress
Proposed branch: lp:~sidnei/juju-core/lxc-clone-with-overlayfs
Merge into: lp:~go-bot/juju-core/trunk
Diff against target: 172 lines (+60/-33)
2 files modified
container/lxc/lxc.go (+58/-31)
container/lxc/mock/mock-lxc.go (+2/-2)
To merge this branch: bzr merge lp:~sidnei/juju-core/lxc-clone-with-overlayfs
Reviewer Review Type Date Requested Status
Juju Engineering 2013-08-15 Pending
Review via email: mp+180445@code.launchpad.net

Description of the change

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/

To post a comment you must log in.
Ian Booth (wallyworld) wrote :

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) 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?

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) wrote :
Download full text (8.2 KiB)

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...

Read more...

William Reade (fwereade) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
3030
31var (31var (
32 defaultTemplate = "ubuntu-cloud"32 defaultTemplate = "ubuntu-cloud"
33 defaultFSSize = "8G" // Same as (most) cloud images.
34 defaultFSType = "" // Let lxc pick a default.
35 defaultCloneSize = "8G" // Only a hint to lxc, which may or may not get used.
33 containerDir = "/var/lib/juju/containers"36 containerDir = "/var/lib/juju/containers"
34 removedContainerDir = "/var/lib/juju/removed-containers"37 removedContainerDir = "/var/lib/juju/removed-containers"
35 lxcContainerDir = "/var/lib/lxc"38 lxcContainerDir = "/var/lib/lxc"
@@ -120,65 +123,89 @@
120 stateInfo *state.Info,123 stateInfo *state.Info,
121 apiInfo *api.Info) (instance.Instance, error) {124 apiInfo *api.Info) (instance.Instance, error) {
122125
126 var container golxc.Container
127
123 name := names.MachineTag(machineId)128 name := names.MachineTag(machineId)
124 if manager.name != "" {129 if manager.name != "" {
125 name = fmt.Sprintf("%s-%s", manager.name, name)130 name = fmt.Sprintf("%s-%s", manager.name, name)
126 }131 }
127 // Note here that the lxcObjectFacotry only returns a valid container132 // Note here that the lxcObjectFactory only returns a valid container
128 // object, and doesn't actually construct the underlying lxc container on133 // object, and doesn't actually construct the underlying lxc container on
129 // disk.134 // disk.
130 container := lxcObjectFactory.New(name)135 templateName := fmt.Sprintf("%s-%s-template", manager.name, series)
136 template := lxcObjectFactory.New(templateName)
137 if !template.IsConstructed() {
138 templateDirectory := jujuContainerDirectory(templateName)
139 logger.Debugf("create directory: %s", templateDirectory)
140 if err := os.MkdirAll(templateDirectory, 0755); err != nil {
141 logger.Errorf("failed to create template container directory: %v", err)
142 return nil, err
143 }
144 logger.Debugf("write the lxc.conf file")
145 configFile, err := writeLxcConfig(network, templateDirectory, manager.logdir)
146 if err != nil {
147 logger.Errorf("failed to write config file: %v", err)
148 return nil, err
149 }
150 templateParams := []string{
151 "--debug", // Debug errors in the cloud image
152 "--hostid", templateName, // Use the container name as the hostid
153 "-r", series,
154 }
155 // Create the container.
156 logger.Debugf("create the container template")
157 if err := template.Create(configFile, golxc.BackingStoreAuto, defaultFSSize, defaultFSType, defaultTemplate, templateParams...); err != nil {
158 logger.Errorf("lxc template container creation failed: %v", err)
159 return nil, err
160 }
161 // Make sure that the mount dir has been created.
162 logger.Debugf("make the mount dir for the shard logs")
163 if err := os.MkdirAll(internalLogDir(templateName), 0755); err != nil {
164 logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
165 return nil, err
166 }
167 }
131168
132 // Create the cloud-init.169 // Create the cloud-init.
133 directory := jujuContainerDirectory(name)170 directory := jujuContainerDirectory(name)
134 logger.Tracef("create directory: %s", directory)171 logger.Debugf("create directory: %s", directory)
135 if err := os.MkdirAll(directory, 0755); err != nil {172 if err := os.MkdirAll(directory, 0755); err != nil {
136 logger.Errorf("failed to create container directory: %v", err)173 logger.Errorf("failed to create container directory: %v", err)
137 return nil, err174 return nil, err
138 }175 }
139 logger.Tracef("write cloud-init")176 logger.Debugf("write cloud-init")
140 userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)177 userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)
141 if err != nil {178 if err != nil {
142 logger.Errorf("failed to write user data: %v", err)179 logger.Errorf("failed to write user data: %v", err)
143 return nil, err180 return nil, err
144 }181 }
145 logger.Tracef("write the lxc.conf file")
146 configFile, err := writeLxcConfig(network, directory, manager.logdir)
147 if err != nil {
148 logger.Errorf("failed to write config file: %v", err)
149 return nil, err
150 }
151 templateParams := []string{182 templateParams := []string{
152 "--debug", // Debug errors in the cloud image
153 "--userdata", userDataFilename, // Our groovey cloud-init183 "--userdata", userDataFilename, // Our groovey cloud-init
154 "--hostid", name, // Use the container name as the hostid184 }
155 "-r", series,185
156 }186 // Clone existing template into a new container.
157 // Create the container.187 logger.Debugf("create the container clone")
158 logger.Tracef("create the container")188 if container, err = template.Clone(name, true, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
159 if err := container.Create(configFile, defaultTemplate, templateParams...); err != nil {189 logger.Errorf("lxc container clone creation failed: %v, retrying without snapshot", err)
160 logger.Errorf("lxc container creation failed: %v", err)190 if container, err = template.Clone(name, false, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
161 return nil, err191 logger.Errorf("lxc container clone creation failed: %v", err)
162 }192 return nil, err
163 // Make sure that the mount dir has been created.193 }
164 logger.Tracef("make the mount dir for the shard logs")194 }
165 if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {195
166 logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)196 logger.Debugf("lxc container clone created")
167 return nil, err
168 }
169 logger.Tracef("lxc container created")
170 // Now symlink the config file into the restart directory.197 // Now symlink the config file into the restart directory.
171 containerConfigFile := filepath.Join(lxcContainerDir, name, "config")198 containerConfigFile := filepath.Join(lxcContainerDir, name, "config")
172 if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {199 if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
173 return nil, err200 return nil, err
174 }201 }
175 logger.Tracef("auto-restart link created")202 logger.Debugf("auto-restart link created")
176203
177 // Start the lxc container with the appropriate settings for grabbing the204 // Start the lxc container with the appropriate settings for grabbing the
178 // console output and a log file.205 // console output and a log file.
179 consoleFile := filepath.Join(directory, "console.log")206 consoleFile := filepath.Join(directory, "console.log")
180 container.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)207 container.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
181 logger.Tracef("start the container")208 logger.Debugf("start the container")
182 // We explicitly don't pass through the config file to the container.Start209 // We explicitly don't pass through the config file to the container.Start
183 // method as we have passed it through at container creation time. This210 // method as we have passed it through at container creation time. This
184 // is necessary to get the appropriate rootfs reference without explicitly211 // is necessary to get the appropriate rootfs reference without explicitly
@@ -187,7 +214,7 @@
187 logger.Errorf("container failed to start: %v", err)214 logger.Errorf("container failed to start: %v", err)
188 return nil, err215 return nil, err
189 }216 }
190 logger.Tracef("container started")217 logger.Debugf("container started")
191 return &lxcInstance{container, name}, nil218 return &lxcInstance{container, name}, nil
192}219}
193220
@@ -205,7 +232,7 @@
205 }232 }
206233
207 // Move the directory.234 // Move the directory.
208 logger.Tracef("create old container dir: %s", removedContainerDir)235 logger.Debugf("create old container dir: %s", removedContainerDir)
209 if err := os.MkdirAll(removedContainerDir, 0755); err != nil {236 if err := os.MkdirAll(removedContainerDir, 0755); err != nil {
210 logger.Errorf("failed to create removed container directory: %v", err)237 logger.Errorf("failed to create removed container directory: %v", err)
211 return err238 return err
212239
=== 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 @@
68}68}
6969
70// Create creates a new container based on the given template.70// Create creates a new container based on the given template.
71func (mock *mockContainer) Create(configFile, template string, templateArgs ...string) error {71func (mock *mockContainer) Create(configFile string, backingStore golxc.BackingStore, fsSize string, fsType string, template string, templateArgs ...string) error {
72 if mock.state != golxc.StateUnknown {72 if mock.state != golxc.StateUnknown {
73 return fmt.Errorf("container is already created")73 return fmt.Errorf("container is already created")
74 }74 }
@@ -102,7 +102,7 @@
102}102}
103103
104// Clone creates a copy of the container, giving the copy the specified name.104// Clone creates a copy of the container, giving the copy the specified name.
105func (mock *mockContainer) Clone(name string) (golxc.Container, error) {105func (mock *mockContainer) Clone(name string, snapshot bool, backingStore golxc.BackingStore, fsSize string, templateArgs ...string) (golxc.Container, error) {
106 container := &mockContainer{106 container := &mockContainer{
107 factory: mock.factory,107 factory: mock.factory,
108 name: name,108 name: name,

Subscribers

People subscribed via source and target branches

to status/vote changes: