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
1=== modified file 'container/lxc/lxc.go'
2--- container/lxc/lxc.go 2013-10-01 02:33:14 +0000
3+++ container/lxc/lxc.go 2013-10-22 22:30:22 +0000
4@@ -30,6 +30,9 @@
5
6 var (
7 defaultTemplate = "ubuntu-cloud"
8+ defaultFSSize = "8G" // Same as (most) cloud images.
9+ defaultFSType = "" // Let lxc pick a default.
10+ defaultCloneSize = "8G" // Only a hint to lxc, which may or may not get used.
11 containerDir = "/var/lib/juju/containers"
12 removedContainerDir = "/var/lib/juju/removed-containers"
13 lxcContainerDir = "/var/lib/lxc"
14@@ -120,65 +123,89 @@
15 stateInfo *state.Info,
16 apiInfo *api.Info) (instance.Instance, error) {
17
18+ var container golxc.Container
19+
20 name := names.MachineTag(machineId)
21 if manager.name != "" {
22 name = fmt.Sprintf("%s-%s", manager.name, name)
23 }
24- // Note here that the lxcObjectFacotry only returns a valid container
25+ // Note here that the lxcObjectFactory only returns a valid container
26 // object, and doesn't actually construct the underlying lxc container on
27 // disk.
28- container := lxcObjectFactory.New(name)
29+ templateName := fmt.Sprintf("%s-%s-template", manager.name, series)
30+ template := lxcObjectFactory.New(templateName)
31+ if !template.IsConstructed() {
32+ templateDirectory := jujuContainerDirectory(templateName)
33+ logger.Debugf("create directory: %s", templateDirectory)
34+ if err := os.MkdirAll(templateDirectory, 0755); err != nil {
35+ logger.Errorf("failed to create template container directory: %v", err)
36+ return nil, err
37+ }
38+ logger.Debugf("write the lxc.conf file")
39+ configFile, err := writeLxcConfig(network, templateDirectory, manager.logdir)
40+ if err != nil {
41+ logger.Errorf("failed to write config file: %v", err)
42+ return nil, err
43+ }
44+ templateParams := []string{
45+ "--debug", // Debug errors in the cloud image
46+ "--hostid", templateName, // Use the container name as the hostid
47+ "-r", series,
48+ }
49+ // Create the container.
50+ logger.Debugf("create the container template")
51+ if err := template.Create(configFile, golxc.BackingStoreAuto, defaultFSSize, defaultFSType, defaultTemplate, templateParams...); err != nil {
52+ logger.Errorf("lxc template container creation failed: %v", err)
53+ return nil, err
54+ }
55+ // Make sure that the mount dir has been created.
56+ logger.Debugf("make the mount dir for the shard logs")
57+ if err := os.MkdirAll(internalLogDir(templateName), 0755); err != nil {
58+ logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
59+ return nil, err
60+ }
61+ }
62
63 // Create the cloud-init.
64 directory := jujuContainerDirectory(name)
65- logger.Tracef("create directory: %s", directory)
66+ logger.Debugf("create directory: %s", directory)
67 if err := os.MkdirAll(directory, 0755); err != nil {
68 logger.Errorf("failed to create container directory: %v", err)
69 return nil, err
70 }
71- logger.Tracef("write cloud-init")
72+ logger.Debugf("write cloud-init")
73 userDataFilename, err := writeUserData(directory, machineId, nonce, tools, environConfig, stateInfo, apiInfo)
74 if err != nil {
75 logger.Errorf("failed to write user data: %v", err)
76 return nil, err
77 }
78- logger.Tracef("write the lxc.conf file")
79- configFile, err := writeLxcConfig(network, directory, manager.logdir)
80- if err != nil {
81- logger.Errorf("failed to write config file: %v", err)
82- return nil, err
83- }
84 templateParams := []string{
85- "--debug", // Debug errors in the cloud image
86 "--userdata", userDataFilename, // Our groovey cloud-init
87- "--hostid", name, // Use the container name as the hostid
88- "-r", series,
89- }
90- // Create the container.
91- logger.Tracef("create the container")
92- if err := container.Create(configFile, defaultTemplate, templateParams...); err != nil {
93- logger.Errorf("lxc container creation failed: %v", err)
94- return nil, err
95- }
96- // Make sure that the mount dir has been created.
97- logger.Tracef("make the mount dir for the shard logs")
98- if err := os.MkdirAll(internalLogDir(name), 0755); err != nil {
99- logger.Errorf("failed to create internal /var/log/juju mount dir: %v", err)
100- return nil, err
101- }
102- logger.Tracef("lxc container created")
103+ }
104+
105+ // Clone existing template into a new container.
106+ logger.Debugf("create the container clone")
107+ if container, err = template.Clone(name, true, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
108+ logger.Errorf("lxc container clone creation failed: %v, retrying without snapshot", err)
109+ if container, err = template.Clone(name, false, golxc.BackingStoreAuto, defaultCloneSize, templateParams...); err != nil {
110+ logger.Errorf("lxc container clone creation failed: %v", err)
111+ return nil, err
112+ }
113+ }
114+
115+ logger.Debugf("lxc container clone created")
116 // Now symlink the config file into the restart directory.
117 containerConfigFile := filepath.Join(lxcContainerDir, name, "config")
118 if err := os.Symlink(containerConfigFile, restartSymlink(name)); err != nil {
119 return nil, err
120 }
121- logger.Tracef("auto-restart link created")
122+ logger.Debugf("auto-restart link created")
123
124 // Start the lxc container with the appropriate settings for grabbing the
125 // console output and a log file.
126 consoleFile := filepath.Join(directory, "console.log")
127 container.SetLogFile(filepath.Join(directory, "container.log"), golxc.LogDebug)
128- logger.Tracef("start the container")
129+ logger.Debugf("start the container")
130 // We explicitly don't pass through the config file to the container.Start
131 // method as we have passed it through at container creation time. This
132 // is necessary to get the appropriate rootfs reference without explicitly
133@@ -187,7 +214,7 @@
134 logger.Errorf("container failed to start: %v", err)
135 return nil, err
136 }
137- logger.Tracef("container started")
138+ logger.Debugf("container started")
139 return &lxcInstance{container, name}, nil
140 }
141
142@@ -205,7 +232,7 @@
143 }
144
145 // Move the directory.
146- logger.Tracef("create old container dir: %s", removedContainerDir)
147+ logger.Debugf("create old container dir: %s", removedContainerDir)
148 if err := os.MkdirAll(removedContainerDir, 0755); err != nil {
149 logger.Errorf("failed to create removed container directory: %v", err)
150 return err
151
152=== modified file 'container/lxc/mock/mock-lxc.go'
153--- container/lxc/mock/mock-lxc.go 2013-09-30 01:50:43 +0000
154+++ container/lxc/mock/mock-lxc.go 2013-10-22 22:30:22 +0000
155@@ -68,7 +68,7 @@
156 }
157
158 // Create creates a new container based on the given template.
159-func (mock *mockContainer) Create(configFile, template string, templateArgs ...string) error {
160+func (mock *mockContainer) Create(configFile string, backingStore golxc.BackingStore, fsSize string, fsType string, template string, templateArgs ...string) error {
161 if mock.state != golxc.StateUnknown {
162 return fmt.Errorf("container is already created")
163 }
164@@ -102,7 +102,7 @@
165 }
166
167 // Clone creates a copy of the container, giving the copy the specified name.
168-func (mock *mockContainer) Clone(name string) (golxc.Container, error) {
169+func (mock *mockContainer) Clone(name string, snapshot bool, backingStore golxc.BackingStore, fsSize string, templateArgs ...string) (golxc.Container, error) {
170 container := &mockContainer{
171 factory: mock.factory,
172 name: name,

Subscribers

People subscribed via source and target branches

to status/vote changes: