Merge lp:~chipaca/snappy/config-modules into lp:~snappy-dev/snappy/snappy-moved-to-github
- config-modules
- Merge into snappy-moved-to-github
Status: | Merged |
---|---|
Approved by: | John Lenton |
Approved revision: | 782 |
Merged at revision: | 791 |
Proposed branch: | lp:~chipaca/snappy/config-modules |
Merge into: | lp:~snappy-dev/snappy/snappy-moved-to-github |
Diff against target: |
444 lines (+296/-10) 2 files modified
coreconfig/config.go (+107/-6) coreconfig/config_test.go (+189/-4) |
To merge this branch: | bzr merge lp:~chipaca/snappy/config-modules |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Oliver Grawert | Approve | ||
Michael Vogt (community) | Approve | ||
Review via email: mp+274987@code.launchpad.net |
Commit message
"modules" ubuntu-core config handling
Description of the change
This adds the "config" option to ubuntu-core config.
It's the first one of these that isn't just a blob of the file, so please comment on the api as exposed.
You add a module to be loaded at boot via:
echo "config: {ubuntu-core: {modules: {potato: true}}}" | uPOST /1.0/packages/
and you remove it by setting it to "false".
Another api that was considered was simply setting it to a list. Downside is, adding a single module to a system that already has a few gets tedious; upside, simpler.
Please do comment.
John Lenton (chipaca) wrote : | # |
John Lenton (chipaca) wrote : | # |
(unless you're actually reviewing and not feedback'ing on the feature; in that case, do not ignore)
John Lenton (chipaca) wrote : | # |
I'll go have breakfast now.
Michael Vogt (mvo) wrote : | # |
Thanks, this looks good, one nitpick inline.
Oliver Grawert (ogra) wrote : | # |
i wouldnt make the module a boolean but a list i.e.:
echo "config: {ubuntu-core: {modules: {potato, banana, apple}}}" | uPOST /1.0/packages/
to result in /etc/modules-
# DO NOT EDIT THIS FILE
# it is auto-generated, and will be overwritten.
potato
banana
apple
i imagine that we have the case more often that you need to load more than one module (especially for firewalling there are a ton of options to manually enable by loading the respective module), the list saves you from having to use multiple commands for this.
John Lenton (chipaca) wrote : | # |
it's a map, so with this api you'd do*
echo "config: {ubuntu-core: {modules: {potato: true, banana: true, apple: true}}}" | uPOST /1.0/packages/
to have that conf.
The advantage of this is that if you later want to change banana for ip6_banana, you do
echo "config: {ubuntu-core: {modules: {banana: false, ip6_banana: true}}}" | uPOST /1.0/packages/
The alternative api would be, to set them up:
echo "config: {ubuntu-core: {modules: [potato, banana, apple]}}" | uPOST /1.0/packages/
and then to change it you've got to specify them all again:
echo "config: {ubuntu-core: {modules: [potato, ip6_banana, apple]}}" | uPOST /1.0/packages/
* i'd need to double-check that yaml, but you get the idea
John Lenton (chipaca) wrote : | # |
oh, in all the above, "snappy config ubuntu-core -" instead of uPOST works too ;)
Oliver Grawert (ogra) wrote : | # |
oh, thanks, i didnt get that you can make a list out of the booleans :)
we should make sure thats clear in the docs ;)
Oliver Grawert (ogra) : | # |
John Lenton (chipaca) wrote : | # |
Please do not top-approve until the relevant ubuntu-core-config branch lands.
John Lenton (chipaca) wrote : | # |
Gustavo has indicated that it would be better to expose this as an additive list, with optional removal being indicated with a prefixed dash.
Gustavo Niemeyer (niemeyer) wrote : | # |
Indeed.. it feels like an additive list would be more friendly to write and read, and it would also allow snappy itself to make changes to that list without it being unintendedly overwritten by snaps that were not even aware that they were killing them.
I've also added a few more notes inline:
John Lenton (chipaca) wrote : | # |
gustavo, snaps don't call this, system integrators and administrators call this
John Lenton (chipaca) : | # |
John Lenton (chipaca) wrote : | # |
Refactored into an additive list thing.
echo "config: {ubuntu-core: {modules: [potato, banana, apple]}}" | sudo snappy config ubuntu-core -
to add those to the list, and
echo "config: {ubuntu-core: {modules: [-banana, ip6_banana]}}" | sudo snappy config ubuntu-core -
to remove them
Gustavo Niemeyer (niemeyer) wrote : | # |
More inline responses..
Jamie Strandboge (jdstrand) wrote : | # |
FWIW, I much prefer the additive list over the boolean approach.
- 782. By John Lenton
-
couple more tweaks
Preview Diff
1 | === modified file 'coreconfig/config.go' |
2 | --- coreconfig/config.go 2015-09-18 15:16:17 +0000 |
3 | +++ coreconfig/config.go 2015-10-20 20:00:45 +0000 |
4 | @@ -20,12 +20,15 @@ |
5 | package coreconfig |
6 | |
7 | import ( |
8 | + "bufio" |
9 | + "bytes" |
10 | "errors" |
11 | "io/ioutil" |
12 | "os" |
13 | "os/exec" |
14 | "path/filepath" |
15 | "reflect" |
16 | + "sort" |
17 | "strings" |
18 | "syscall" |
19 | |
20 | @@ -52,6 +55,7 @@ |
21 | |
22 | var ( |
23 | modprobePath = "/etc/modprobe.d/ubuntu-core.conf" |
24 | + modulesPath = "/etc/modules-load.d/ubuntu-core.conf" |
25 | interfacesRoot = "/etc/network/interfaces.d/" |
26 | pppRoot = "/etc/ppp/" |
27 | watchdogConfigPath = "/etc/watchdog.conf" |
28 | @@ -73,6 +77,7 @@ |
29 | Timezone *string `yaml:"timezone,omitempty"` |
30 | Hostname *string `yaml:"hostname,omitempty"` |
31 | Modprobe *string `yaml:"modprobe,omitempty"` |
32 | + Modules []string `yaml:"load-kernel-modules,omitempty"` |
33 | Network *networkConfig `yaml:"network,omitempty"` |
34 | Watchdog *watchdogConfig `yaml:"watchdog,omitempty"` |
35 | } |
36 | @@ -119,6 +124,10 @@ |
37 | if err != nil { |
38 | return nil, err |
39 | } |
40 | + modules, err := getModules() |
41 | + if err != nil { |
42 | + return nil, err |
43 | + } |
44 | interfaces, err := getInterfaces() |
45 | if err != nil { |
46 | return nil, err |
47 | @@ -145,6 +154,7 @@ |
48 | Timezone: &tz, |
49 | Hostname: &hostname, |
50 | Modprobe: &modprobe, |
51 | + Modules: modules, |
52 | Network: network, |
53 | Watchdog: watchdog, |
54 | } |
55 | @@ -244,6 +254,10 @@ |
56 | if err := setModprobe(*newConfig.Modprobe); err != nil { |
57 | return "", err |
58 | } |
59 | + case "Modules": |
60 | + if err := setModules(newConfig.Modules); err != nil { |
61 | + return "", err |
62 | + } |
63 | case "Network": |
64 | if oldConfig.Network == nil || !passthroughEqual(oldConfig.Network.Interfaces, newConfig.Network.Interfaces) { |
65 | if err := setInterfaces(newConfig.Network.Interfaces); err != nil { |
66 | @@ -297,7 +311,7 @@ |
67 | return err |
68 | } |
69 | |
70 | - return ioutil.WriteFile(tzFile(), []byte(timezone), 0644) |
71 | + return helpers.AtomicWriteFile(tzFile(), []byte(timezone), 0644) |
72 | } |
73 | |
74 | func getPassthrough(rootDir string) (pc []passthroughConfig, err error) { |
75 | @@ -326,7 +340,7 @@ |
76 | os.Remove(path) |
77 | continue |
78 | } |
79 | - if err := ioutil.WriteFile(path, []byte(c.Content), 0644); err != nil { |
80 | + if err := helpers.AtomicWriteFile(path, []byte(c.Content), 0644); err != nil { |
81 | return err |
82 | } |
83 | } |
84 | @@ -362,7 +376,94 @@ |
85 | |
86 | // setModprobe sets the specified modprobe config |
87 | var setModprobe = func(modprobe string) error { |
88 | - return ioutil.WriteFile(modprobePath, []byte(modprobe), 0644) |
89 | + return helpers.AtomicWriteFile(modprobePath, []byte(modprobe), 0644) |
90 | +} |
91 | + |
92 | +func getModules() ([]string, error) { |
93 | + f, err := os.Open(modulesPath) |
94 | + if err != nil { |
95 | + if os.IsNotExist(err) { |
96 | + return nil, nil |
97 | + } |
98 | + |
99 | + return nil, err |
100 | + } |
101 | + |
102 | + // there's a warning at the top of the file |
103 | + // but you know they're just going to edit it anyway |
104 | + // so be kind |
105 | + |
106 | + var modules []string |
107 | + scanner := bufio.NewScanner(f) |
108 | + for scanner.Scan() { |
109 | + line := strings.TrimSpace(scanner.Text()) |
110 | + if len(line) == 0 { |
111 | + continue |
112 | + } |
113 | + if line[0] == '#' || line[0] == ';' { |
114 | + continue |
115 | + } |
116 | + |
117 | + modules = append(modules, line) |
118 | + } |
119 | + if err := scanner.Err(); err != nil { |
120 | + return nil, err |
121 | + } |
122 | + |
123 | + // doing the sort on get makes testing easier |
124 | + sort.Strings(modules) |
125 | + |
126 | + return modules, nil |
127 | +} |
128 | + |
129 | +const modulesHeader = `# |
130 | +# DO NOT EDIT THIS FILE |
131 | +# it is auto-generated, and will be overwritten. |
132 | +` |
133 | + |
134 | +func setModules(modules []string) error { |
135 | + oldModules, err := getModules() |
136 | + if err != nil { |
137 | + return err |
138 | + } |
139 | + |
140 | + for i := range modules { |
141 | + m := strings.TrimSpace(modules[i]) |
142 | + if len(m) == 0 { |
143 | + continue |
144 | + } |
145 | + |
146 | + if m[0] == '-' { |
147 | + m = m[1:] |
148 | + idx := sort.SearchStrings(oldModules, m) |
149 | + if idx == len(oldModules) || oldModules[idx] != m { |
150 | + // not found |
151 | + continue |
152 | + } |
153 | + oldModules = append(oldModules[:idx], oldModules[idx+1:]...) |
154 | + } else { |
155 | + idx := sort.SearchStrings(oldModules, m) |
156 | + if idx < len(oldModules) && oldModules[idx] == m { |
157 | + // already got it |
158 | + continue |
159 | + } |
160 | + oldModules = append(oldModules, "") |
161 | + copy(oldModules[idx+1:], oldModules[idx:]) |
162 | + oldModules[idx] = m |
163 | + } |
164 | + } |
165 | + |
166 | + var buf bytes.Buffer |
167 | + |
168 | + // bytes' Write* methods always return nil error |
169 | + buf.WriteString(modulesHeader) |
170 | + |
171 | + for i := range oldModules { |
172 | + buf.WriteString(oldModules[i]) |
173 | + buf.WriteByte('\n') |
174 | + } |
175 | + |
176 | + return helpers.AtomicWriteFile(modulesPath, buf.Bytes(), 0644) |
177 | } |
178 | |
179 | // getWatchdog returns the current watchdog config |
180 | @@ -388,11 +489,11 @@ |
181 | |
182 | // setWatchdog sets the specified watchdog config |
183 | var setWatchdog = func(wf *watchdogConfig) error { |
184 | - if err := ioutil.WriteFile(watchdogStartupPath, []byte(wf.Startup), 0644); err != nil { |
185 | + if err := helpers.AtomicWriteFile(watchdogStartupPath, []byte(wf.Startup), 0644); err != nil { |
186 | return err |
187 | } |
188 | |
189 | - return ioutil.WriteFile(watchdogConfigPath, []byte(wf.Config), 0644) |
190 | + return helpers.AtomicWriteFile(watchdogConfigPath, []byte(wf.Config), 0644) |
191 | } |
192 | |
193 | // for testing purposes |
194 | @@ -467,5 +568,5 @@ |
195 | return err |
196 | } |
197 | |
198 | - return ioutil.WriteFile(hostnamePath, hostnameB, 0644) |
199 | + return helpers.AtomicWriteFile(hostnamePath, hostnameB, 0644) |
200 | } |
201 | |
202 | === modified file 'coreconfig/config_test.go' |
203 | --- coreconfig/config_test.go 2015-09-21 15:14:41 +0000 |
204 | +++ coreconfig/config_test.go 2015-10-20 20:00:45 +0000 |
205 | @@ -51,6 +51,7 @@ |
206 | originalCmdSystemctl = cmdSystemctl |
207 | originalHostnamePath = hostnamePath |
208 | originalModprobePath = modprobePath |
209 | + originalModulesPath = modulesPath |
210 | originalInterfacesRoot = interfacesRoot |
211 | originalPppRoot = pppRoot |
212 | originalWatchdogStartupPath = watchdogStartupPath |
213 | @@ -107,6 +108,7 @@ |
214 | cmdAutopilotEnabled = originalCmdAutopilotEnabled |
215 | cmdSystemctl = originalCmdSystemctl |
216 | modprobePath = originalModprobePath |
217 | + modulesPath = originalModulesPath |
218 | interfacesRoot = originalInterfacesRoot |
219 | pppRoot = originalPppRoot |
220 | watchdogStartupPath = originalWatchdogStartupPath |
221 | @@ -489,12 +491,195 @@ |
222 | _, err := Set(input) |
223 | c.Assert(err, IsNil) |
224 | |
225 | - // ensure its really there |
226 | + // ensure it's really there |
227 | content, err := ioutil.ReadFile(modprobePath) |
228 | c.Assert(err, IsNil) |
229 | c.Assert(string(content), Equals, "blacklist floppy\nsoftdep mlx4_core post: mlx4_en\n") |
230 | } |
231 | |
232 | +func (cts *ConfigTestSuite) TestModules(c *C) { |
233 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
234 | + |
235 | + modules, err := getModules() |
236 | + c.Assert(err, IsNil) |
237 | + c.Check(modules, HasLen, 0) |
238 | + |
239 | + c.Assert(setModules([]string{"foo"}), IsNil) |
240 | + |
241 | + modules, err = getModules() |
242 | + c.Assert(err, IsNil) |
243 | + c.Check(modules, DeepEquals, []string{"foo"}) |
244 | + |
245 | + c.Assert(setModules([]string{"bar"}), IsNil) |
246 | + |
247 | + modules, err = getModules() |
248 | + c.Assert(err, IsNil) |
249 | + c.Check(modules, DeepEquals, []string{"bar", "foo"}) |
250 | + |
251 | + c.Assert(setModules([]string{"-foo"}), IsNil) |
252 | + |
253 | + modules, err = getModules() |
254 | + c.Assert(err, IsNil) |
255 | + c.Check(modules, DeepEquals, []string{"bar"}) |
256 | +} |
257 | + |
258 | +func (cts *ConfigTestSuite) TestModulesRemoveAbsent(c *C) { |
259 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
260 | + |
261 | + c.Assert(setModules([]string{"foo"}), IsNil) |
262 | + c.Assert(setModules([]string{"-bar"}), IsNil) |
263 | + |
264 | + modules, err := getModules() |
265 | + c.Assert(err, IsNil) |
266 | + c.Check(modules, DeepEquals, []string{"foo"}) |
267 | +} |
268 | + |
269 | +func (cts *ConfigTestSuite) TestModulesRemoveEmpty(c *C) { |
270 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
271 | + |
272 | + c.Assert(setModules([]string{"foo"}), IsNil) |
273 | + c.Assert(setModules([]string{"-"}), IsNil) |
274 | + |
275 | + modules, err := getModules() |
276 | + c.Assert(err, IsNil) |
277 | + c.Check(modules, DeepEquals, []string{"foo"}) |
278 | +} |
279 | + |
280 | +func (cts *ConfigTestSuite) TestModulesRemoveBlank(c *C) { |
281 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
282 | + |
283 | + c.Assert(setModules([]string{"foo"}), IsNil) |
284 | + c.Assert(setModules([]string{"- "}), IsNil) |
285 | + |
286 | + modules, err := getModules() |
287 | + c.Assert(err, IsNil) |
288 | + c.Check(modules, DeepEquals, []string{"foo"}) |
289 | +} |
290 | + |
291 | +func (cts *ConfigTestSuite) TestModulesAddDupe(c *C) { |
292 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
293 | + |
294 | + c.Assert(setModules([]string{"foo"}), IsNil) |
295 | + c.Assert(setModules([]string{"foo"}), IsNil) |
296 | + |
297 | + modules, err := getModules() |
298 | + c.Assert(err, IsNil) |
299 | + c.Check(modules, DeepEquals, []string{"foo"}) |
300 | +} |
301 | + |
302 | +func (cts *ConfigTestSuite) TestModulesAddEmpty(c *C) { |
303 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
304 | + |
305 | + c.Assert(setModules([]string{"foo"}), IsNil) |
306 | + c.Assert(setModules([]string{""}), IsNil) |
307 | + |
308 | + modules, err := getModules() |
309 | + c.Assert(err, IsNil) |
310 | + c.Check(modules, DeepEquals, []string{"foo"}) |
311 | +} |
312 | + |
313 | +func (cts *ConfigTestSuite) TestModulesAddBlank(c *C) { |
314 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
315 | + |
316 | + c.Assert(setModules([]string{"foo"}), IsNil) |
317 | + c.Assert(setModules([]string{" "}), IsNil) |
318 | + |
319 | + modules, err := getModules() |
320 | + c.Assert(err, IsNil) |
321 | + c.Check(modules, DeepEquals, []string{"foo"}) |
322 | +} |
323 | + |
324 | +func (cts *ConfigTestSuite) TestModulesHasWarning(c *C) { |
325 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
326 | + |
327 | + c.Assert(setModules([]string{"foo"}), IsNil) |
328 | + |
329 | + bs, err := ioutil.ReadFile(modulesPath) |
330 | + c.Assert(err, IsNil) |
331 | + c.Check(string(bs), Matches, `(?s).*DO NOT EDIT.*`) |
332 | +} |
333 | + |
334 | +func (cts *ConfigTestSuite) TestModulesIsKind(c *C) { |
335 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
336 | + c.Assert(ioutil.WriteFile(modulesPath, []byte(`# hello |
337 | +# this is what happens when soembody comes and edits the file |
338 | +# just to be sure |
339 | +; modules-load.d(5) says comments can also start with a ; |
340 | + ; actually not even start |
341 | + # it's the first non-whitespace that counts |
342 | +# also here's an empty line: |
343 | + |
344 | +# and here's a module with spurious whitespace: |
345 | + oops |
346 | +# that is all. Have a good day. |
347 | +`), 0644), IsNil) |
348 | + |
349 | + modules, err := getModules() |
350 | + c.Check(err, IsNil) |
351 | + c.Check(modules, DeepEquals, []string{"oops"}) |
352 | +} |
353 | + |
354 | +func (cts *ConfigTestSuite) TestModulesYaml(c *C) { |
355 | + modulesPath = filepath.Join(c.MkDir(), "test.conf") |
356 | + |
357 | + c.Assert(setModules([]string{"foo"}), IsNil) |
358 | + |
359 | + cfg, err := newSystemConfig() |
360 | + c.Assert(err, IsNil) |
361 | + c.Check(cfg.Modules, DeepEquals, []string{"foo"}) |
362 | + |
363 | + input := `config: |
364 | + ubuntu-core: |
365 | + load-kernel-modules: [-foo, bar] |
366 | +` |
367 | + _, err = Set(input) |
368 | + c.Assert(err, IsNil) |
369 | + |
370 | + // ensure it's really there |
371 | + content, err := ioutil.ReadFile(modulesPath) |
372 | + c.Assert(err, IsNil) |
373 | + c.Assert(string(content), Matches, `(?sm).*^bar$.*`) |
374 | + |
375 | + modules, err := getModules() |
376 | + c.Assert(err, IsNil) |
377 | + c.Check(modules, DeepEquals, []string{"bar"}) |
378 | +} |
379 | + |
380 | +func (cts *ConfigTestSuite) TestModulesErrorWrite(c *C) { |
381 | + // modulesPath is not writable, but only notexist read error |
382 | + modulesPath = filepath.Join(c.MkDir(), "not-there", "test.conf") |
383 | + |
384 | + c.Check(setModules([]string{"bar"}), NotNil) |
385 | + |
386 | + input := `config: |
387 | + ubuntu-core: |
388 | + load-kernel-modules: [foo] |
389 | +` |
390 | + _, err := Set(input) |
391 | + c.Check(err, NotNil) |
392 | + |
393 | + _, err = getModules() |
394 | + c.Check(err, IsNil) |
395 | + |
396 | + _, err = newSystemConfig() |
397 | + c.Check(err, IsNil) |
398 | +} |
399 | + |
400 | +func (cts *ConfigTestSuite) TestModulesErrorRW(c *C) { |
401 | + modulesPath = c.MkDir() |
402 | + |
403 | + modules, err := getModules() |
404 | + c.Check(err, NotNil) |
405 | + c.Check(modules, HasLen, 0) |
406 | + c.Check(setModules([]string{"bar"}), NotNil) |
407 | + |
408 | + _, err = newSystemConfig() |
409 | + c.Check(err, NotNil) |
410 | + |
411 | + _, err = Set("config: {ubuntu-core: {modules: [foo]}}") |
412 | + c.Check(err, NotNil) |
413 | +} |
414 | + |
415 | func (cts *ConfigTestSuite) TestNetworkGet(c *C) { |
416 | path := filepath.Join(interfacesRoot, "eth0") |
417 | content := "auto eth0" |
418 | @@ -573,7 +758,7 @@ |
419 | _, err := Set(input) |
420 | c.Assert(err, IsNil) |
421 | |
422 | - // ensure its really there |
423 | + // ensure it's really there |
424 | content, err := ioutil.ReadFile(filepath.Join(interfacesRoot, "eth0")) |
425 | c.Assert(err, IsNil) |
426 | c.Assert(string(content), Equals, "auto dhcp") |
427 | @@ -593,7 +778,7 @@ |
428 | _, err := Set(input) |
429 | c.Assert(err, IsNil) |
430 | |
431 | - // ensure its really there |
432 | + // ensure it's really there |
433 | content, err := ioutil.ReadFile(filepath.Join(pppRoot, "chap-secret")) |
434 | c.Assert(err, IsNil) |
435 | c.Assert(string(content), Equals, "password") |
436 | @@ -670,7 +855,7 @@ |
437 | _, err := Set(input) |
438 | c.Assert(err, IsNil) |
439 | |
440 | - // ensure its really there |
441 | + // ensure it's really there |
442 | content, err := ioutil.ReadFile(watchdogStartupPath) |
443 | c.Assert(err, IsNil) |
444 | c.Assert(string(content), Equals, "some startup") |
Oh, I also made some non-related changes to config. Please ignore those: switching ioutil.WriteFile to helpers. AtomicWriteFile , and its->it's.