Merge lp:~chipaca/snappy/config-modules into lp:~snappy-dev/snappy/snappy-moved-to-github

[John Lenton]

"modules" ubuntu-core config handling

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/ubuntu-core.ubuntu/config

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

Oh, I also made some non-related changes to config. Please ignore those: switching ioutil.WriteFile to helpers.AtomicWriteFile, and its->it's.

[John Lenton (chipaca)]

(unless you're actually reviewing and not feedback'ing on the feature; in that case, do not ignore)

[John Lenton (chipaca)]

I'll go have breakfast now.

[Michael Vogt (mvo)]

Thanks, this looks good, one nitpick inline.

review: Approve

[Oliver Grawert (ogra)]

i wouldnt make the module a boolean but a list i.e.:

echo "config: {ubuntu-core: {modules: {potato, banana, apple}}}" | uPOST /1.0/packages/ubuntu-core.ubuntu/config

to result in /etc/modules-load.d/ubuntu-core.conf like:

# 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)]

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/ubuntu-core.ubuntu/config

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/ubuntu-core.ubuntu/config

The alternative api would be, to set them up:

echo "config: {ubuntu-core: {modules: [potato, banana, apple]}}" | uPOST /1.0/packages/ubuntu-core.ubuntu/config

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/ubuntu-core.ubuntu/config

* i'd need to double-check that yaml, but you get the idea

[John Lenton (chipaca)]

oh, in all the above, "snappy config ubuntu-core -" instead of uPOST works too ;)

[Oliver Grawert (ogra)]

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

review: Approve

[John Lenton (chipaca)]

Please do not top-approve until the relevant ubuntu-core-config branch lands.

[John Lenton (chipaca)]

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

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

gustavo, snaps don't call this, system integrators and administrators call this

[John Lenton (chipaca)]

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

More inline responses..

[Jamie Strandboge (jdstrand)]

FWIW, I much prefer the additive list over the boolean approach.

[lp:~chipaca/snappy/config-modules]

782. By John Lenton on 2015-10-20

couple more tweaks

=== modified file 'coreconfig/config.go'
--- coreconfig/config.go	2015-09-18 15:16:17 +0000
+++ coreconfig/config.go	2015-10-20 20:00:45 +0000
@@ -20,12 +20,15 @@
 package coreconfig
 
 import (
+	"bufio"
+	"bytes"
 	"errors"
 	"io/ioutil"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"reflect"
+	"sort"
 	"strings"
 	"syscall"
 
@@ -52,6 +55,7 @@
 
 var (
 	modprobePath        = "/etc/modprobe.d/ubuntu-core.conf"
+	modulesPath         = "/etc/modules-load.d/ubuntu-core.conf"
 	interfacesRoot      = "/etc/network/interfaces.d/"
 	pppRoot             = "/etc/ppp/"
 	watchdogConfigPath  = "/etc/watchdog.conf"
@@ -73,6 +77,7 @@
 	Timezone  *string         `yaml:"timezone,omitempty"`
 	Hostname  *string         `yaml:"hostname,omitempty"`
 	Modprobe  *string         `yaml:"modprobe,omitempty"`
+	Modules   []string        `yaml:"load-kernel-modules,omitempty"`
 	Network   *networkConfig  `yaml:"network,omitempty"`
 	Watchdog  *watchdogConfig `yaml:"watchdog,omitempty"`
 }
@@ -119,6 +124,10 @@
 	if err != nil {
 		return nil, err
 	}
+	modules, err := getModules()
+	if err != nil {
+		return nil, err
+	}
 	interfaces, err := getInterfaces()
 	if err != nil {
 		return nil, err
@@ -145,6 +154,7 @@
 		Timezone:  &tz,
 		Hostname:  &hostname,
 		Modprobe:  &modprobe,
+		Modules:   modules,
 		Network:   network,
 		Watchdog:  watchdog,
 	}
@@ -244,6 +254,10 @@
 			if err := setModprobe(*newConfig.Modprobe); err != nil {
 				return "", err
 			}
+		case "Modules":
+			if err := setModules(newConfig.Modules); err != nil {
+				return "", err
+			}
 		case "Network":
 			if oldConfig.Network == nil || !passthroughEqual(oldConfig.Network.Interfaces, newConfig.Network.Interfaces) {
 				if err := setInterfaces(newConfig.Network.Interfaces); err != nil {
@@ -297,7 +311,7 @@
 		return err
 	}
 
-	return ioutil.WriteFile(tzFile(), []byte(timezone), 0644)
+	return helpers.AtomicWriteFile(tzFile(), []byte(timezone), 0644)
 }
 
 func getPassthrough(rootDir string) (pc []passthroughConfig, err error) {
@@ -326,7 +340,7 @@
 			os.Remove(path)
 			continue
 		}
-		if err := ioutil.WriteFile(path, []byte(c.Content), 0644); err != nil {
+		if err := helpers.AtomicWriteFile(path, []byte(c.Content), 0644); err != nil {
 			return err
 		}
 	}
@@ -362,7 +376,94 @@
 
 // setModprobe sets the specified modprobe config
 var setModprobe = func(modprobe string) error {
-	return ioutil.WriteFile(modprobePath, []byte(modprobe), 0644)
+	return helpers.AtomicWriteFile(modprobePath, []byte(modprobe), 0644)
+}
+
+func getModules() ([]string, error) {
+	f, err := os.Open(modulesPath)
+	if err != nil {
+		if os.IsNotExist(err) {
+			return nil, nil
+		}
+
+		return nil, err
+	}
+
+	// there's a warning at the top of the file
+	// but you know they're just going to edit it anyway
+	// so be kind
+
+	var modules []string
+	scanner := bufio.NewScanner(f)
+	for scanner.Scan() {
+		line := strings.TrimSpace(scanner.Text())
+		if len(line) == 0 {
+			continue
+		}
+		if line[0] == '#' || line[0] == ';' {
+			continue
+		}
+
+		modules = append(modules, line)
+	}
+	if err := scanner.Err(); err != nil {
+		return nil, err
+	}
+
+	// doing the sort on get makes testing easier
+	sort.Strings(modules)
+
+	return modules, nil
+}
+
+const modulesHeader = `#
+# DO NOT EDIT THIS FILE
+# it is auto-generated, and will be overwritten.
+`
+
+func setModules(modules []string) error {
+	oldModules, err := getModules()
+	if err != nil {
+		return err
+	}
+
+	for i := range modules {
+		m := strings.TrimSpace(modules[i])
+		if len(m) == 0 {
+			continue
+		}
+
+		if m[0] == '-' {
+			m = m[1:]
+			idx := sort.SearchStrings(oldModules, m)
+			if idx == len(oldModules) || oldModules[idx] != m {
+				// not found
+				continue
+			}
+			oldModules = append(oldModules[:idx], oldModules[idx+1:]...)
+		} else {
+			idx := sort.SearchStrings(oldModules, m)
+			if idx < len(oldModules) && oldModules[idx] == m {
+				// already got it
+				continue
+			}
+			oldModules = append(oldModules, "")
+			copy(oldModules[idx+1:], oldModules[idx:])
+			oldModules[idx] = m
+		}
+	}
+
+	var buf bytes.Buffer
+
+	// bytes' Write* methods always return nil error
+	buf.WriteString(modulesHeader)
+
+	for i := range oldModules {
+		buf.WriteString(oldModules[i])
+		buf.WriteByte('\n')
+	}
+
+	return helpers.AtomicWriteFile(modulesPath, buf.Bytes(), 0644)
 }
 
 // getWatchdog returns the current watchdog config
@@ -388,11 +489,11 @@
 
 // setWatchdog sets the specified watchdog config
 var setWatchdog = func(wf *watchdogConfig) error {
-	if err := ioutil.WriteFile(watchdogStartupPath, []byte(wf.Startup), 0644); err != nil {
+	if err := helpers.AtomicWriteFile(watchdogStartupPath, []byte(wf.Startup), 0644); err != nil {
 		return err
 	}
 
-	return ioutil.WriteFile(watchdogConfigPath, []byte(wf.Config), 0644)
+	return helpers.AtomicWriteFile(watchdogConfigPath, []byte(wf.Config), 0644)
 }
 
 // for testing purposes
@@ -467,5 +568,5 @@
 		return err
 	}
 
-	return ioutil.WriteFile(hostnamePath, hostnameB, 0644)
+	return helpers.AtomicWriteFile(hostnamePath, hostnameB, 0644)
 }
 
=== modified file 'coreconfig/config_test.go'
--- coreconfig/config_test.go	2015-09-21 15:14:41 +0000
+++ coreconfig/config_test.go	2015-10-20 20:00:45 +0000
@@ -51,6 +51,7 @@
 	originalCmdSystemctl        = cmdSystemctl
 	originalHostnamePath        = hostnamePath
 	originalModprobePath        = modprobePath
+	originalModulesPath         = modulesPath
 	originalInterfacesRoot      = interfacesRoot
 	originalPppRoot             = pppRoot
 	originalWatchdogStartupPath = watchdogStartupPath
@@ -107,6 +108,7 @@
 	cmdAutopilotEnabled = originalCmdAutopilotEnabled
 	cmdSystemctl = originalCmdSystemctl
 	modprobePath = originalModprobePath
+	modulesPath = originalModulesPath
 	interfacesRoot = originalInterfacesRoot
 	pppRoot = originalPppRoot
 	watchdogStartupPath = originalWatchdogStartupPath
@@ -489,12 +491,195 @@
 	_, err := Set(input)
 	c.Assert(err, IsNil)
 
-	// ensure its really there
+	// ensure it's really there
 	content, err := ioutil.ReadFile(modprobePath)
 	c.Assert(err, IsNil)
 	c.Assert(string(content), Equals, "blacklist floppy\nsoftdep mlx4_core post: mlx4_en\n")
 }
 
+func (cts *ConfigTestSuite) TestModules(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, HasLen, 0)
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+
+	modules, err = getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+
+	c.Assert(setModules([]string{"bar"}), IsNil)
+
+	modules, err = getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"bar", "foo"})
+
+	c.Assert(setModules([]string{"-foo"}), IsNil)
+
+	modules, err = getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"bar"})
+}
+
+func (cts *ConfigTestSuite) TestModulesRemoveAbsent(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{"-bar"}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesRemoveEmpty(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{"-"}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesRemoveBlank(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{"- "}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesAddDupe(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{"foo"}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesAddEmpty(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{""}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesAddBlank(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+	c.Assert(setModules([]string{" "}), IsNil)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"foo"})
+}
+
+func (cts *ConfigTestSuite) TestModulesHasWarning(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+
+	bs, err := ioutil.ReadFile(modulesPath)
+	c.Assert(err, IsNil)
+	c.Check(string(bs), Matches, `(?s).*DO NOT EDIT.*`)
+}
+
+func (cts *ConfigTestSuite) TestModulesIsKind(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+	c.Assert(ioutil.WriteFile(modulesPath, []byte(`# hello
+# this is what happens when soembody comes and edits the file
+# just to be sure
+; modules-load.d(5) says comments can also start with a ;
+  ; actually not even start
+  # it's the first non-whitespace that counts
+#    also here's an empty line:
+
+# and here's a module with spurious whitespace:
+  oops
+# that is all. Have a good day.
+`), 0644), IsNil)
+
+	modules, err := getModules()
+	c.Check(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"oops"})
+}
+
+func (cts *ConfigTestSuite) TestModulesYaml(c *C) {
+	modulesPath = filepath.Join(c.MkDir(), "test.conf")
+
+	c.Assert(setModules([]string{"foo"}), IsNil)
+
+	cfg, err := newSystemConfig()
+	c.Assert(err, IsNil)
+	c.Check(cfg.Modules, DeepEquals, []string{"foo"})
+
+	input := `config:
+  ubuntu-core:
+    load-kernel-modules: [-foo, bar]
+`
+	_, err = Set(input)
+	c.Assert(err, IsNil)
+
+	// ensure it's really there
+	content, err := ioutil.ReadFile(modulesPath)
+	c.Assert(err, IsNil)
+	c.Assert(string(content), Matches, `(?sm).*^bar$.*`)
+
+	modules, err := getModules()
+	c.Assert(err, IsNil)
+	c.Check(modules, DeepEquals, []string{"bar"})
+}
+
+func (cts *ConfigTestSuite) TestModulesErrorWrite(c *C) {
+	// modulesPath is not writable, but only notexist read error
+	modulesPath = filepath.Join(c.MkDir(), "not-there", "test.conf")
+
+	c.Check(setModules([]string{"bar"}), NotNil)
+
+	input := `config:
+  ubuntu-core:
+    load-kernel-modules: [foo]
+`
+	_, err := Set(input)
+	c.Check(err, NotNil)
+
+	_, err = getModules()
+	c.Check(err, IsNil)
+
+	_, err = newSystemConfig()
+	c.Check(err, IsNil)
+}
+
+func (cts *ConfigTestSuite) TestModulesErrorRW(c *C) {
+	modulesPath = c.MkDir()
+
+	modules, err := getModules()
+	c.Check(err, NotNil)
+	c.Check(modules, HasLen, 0)
+	c.Check(setModules([]string{"bar"}), NotNil)
+
+	_, err = newSystemConfig()
+	c.Check(err, NotNil)
+
+	_, err = Set("config: {ubuntu-core: {modules: [foo]}}")
+	c.Check(err, NotNil)
+}
+
 func (cts *ConfigTestSuite) TestNetworkGet(c *C) {
 	path := filepath.Join(interfacesRoot, "eth0")
 	content := "auto eth0"
@@ -573,7 +758,7 @@
 	_, err := Set(input)
 	c.Assert(err, IsNil)
 
-	// ensure its really there
+	// ensure it's really there
 	content, err := ioutil.ReadFile(filepath.Join(interfacesRoot, "eth0"))
 	c.Assert(err, IsNil)
 	c.Assert(string(content), Equals, "auto dhcp")
@@ -593,7 +778,7 @@
 	_, err := Set(input)
 	c.Assert(err, IsNil)
 
-	// ensure its really there
+	// ensure it's really there
 	content, err := ioutil.ReadFile(filepath.Join(pppRoot, "chap-secret"))
 	c.Assert(err, IsNil)
 	c.Assert(string(content), Equals, "password")
@@ -670,7 +855,7 @@
 	_, err := Set(input)
 	c.Assert(err, IsNil)
 
-	// ensure its really there
+	// ensure it's really there
 	content, err := ioutil.ReadFile(watchdogStartupPath)
 	c.Assert(err, IsNil)
 	c.Assert(string(content), Equals, "some startup")