system: clean up UnitManager interface

initialize/config.go

@@ -195,66 +195,66 @@ func Apply(cfg config.CloudConfig, env *Environment) error {
 // commands against units. It returns any error encountered.
 func processUnits(units []system.Unit, root string, um system.UnitManager) error {
 	type action struct {
-		unit    string
+		unit    system.Unit
 		command string
 	}
 	actions := make([]action, 0, len(units))
 	reload := false
 	for _, unit := range units {
-		dst := unit.Destination(root)
 		if unit.Content != "" {
-			log.Printf("Writing unit %s to filesystem at path %s", unit.Name, dst)
+			log.Printf("Writing unit %q to filesystem", unit.Name)
-			if err := um.PlaceUnit(&unit, dst); err != nil {
+			if err := um.PlaceUnit(unit); err != nil {
 				return err
 			}
-			log.Printf("Placed unit %s at %s", unit.Name, dst)
+			log.Printf("Wrote unit %q", unit.Name)
 			reload = true
 		}
 
 		if unit.Mask {
-			log.Printf("Masking unit file %s", unit.Name)
+			log.Printf("Masking unit file %q", unit.Name)
-			if err := um.MaskUnit(&unit); err != nil {
+			if err := um.MaskUnit(unit); err != nil {
 				return err
 			}
 		} else if unit.Runtime {
-			log.Printf("Ensuring runtime unit file %s is unmasked", unit.Name)
+			log.Printf("Ensuring runtime unit file %q is unmasked", unit.Name)
-			if err := um.UnmaskUnit(&unit); err != nil {
+			if err := um.UnmaskUnit(unit); err != nil {
 				return err
 			}
 		}
 
 		if unit.Enable {
 			if unit.Group() != "network" {
-				log.Printf("Enabling unit file %s", unit.Name)
+				log.Printf("Enabling unit file %q", unit.Name)
-				if err := um.EnableUnitFile(unit.Name, unit.Runtime); err != nil {
+				if err := um.EnableUnitFile(unit); err != nil {
 					return err
 				}
-				log.Printf("Enabled unit %s", unit.Name)
+				log.Printf("Enabled unit %q", unit.Name)
 			} else {
-				log.Printf("Skipping enable for network-like unit %s", unit.Name)
+				log.Printf("Skipping enable for network-like unit %q", unit.Name)
 			}
 		}
 
 		if unit.Group() == "network" {
-			actions = append(actions, action{"systemd-networkd.service", "restart"})
+			networkd := system.Unit{Unit: config.Unit{Name: "systemd-networkd.service"}}
+			actions = append(actions, action{networkd, "restart"})
 		} else if unit.Command != "" {
-			actions = append(actions, action{unit.Name, unit.Command})
+			actions = append(actions, action{unit, unit.Command})
 		}
 	}
 
 	if reload {
 		if err := um.DaemonReload(); err != nil {
-			return errors.New(fmt.Sprintf("failed systemd daemon-reload: %v", err))
+			return errors.New(fmt.Sprintf("failed systemd daemon-reload: %s", err))
 		}
 	}
 
 	for _, action := range actions {
-		log.Printf("Calling unit command '%s %s'", action.command, action.unit)
+		log.Printf("Calling unit command %q on %q'", action.command, action.unit.Name)
-		res, err := um.RunUnitCommand(action.command, action.unit)
+		res, err := um.RunUnitCommand(action.unit, action.command)
 		if err != nil {
 			return err
 		}
-		log.Printf("Result of '%s %s': %s", action.command, action.unit, res)
+		log.Printf("Result of %q on %q': %s", action.command, action.unit.Name, res)
 	}
 
 	return nil

initialize/config_test.go

@@ -32,29 +32,29 @@ type TestUnitManager struct {
 	reload   bool
 }
 
-func (tum *TestUnitManager) PlaceUnit(unit *system.Unit, dst string) error {
+func (tum *TestUnitManager) PlaceUnit(u system.Unit) error {
-	tum.placed = append(tum.placed, unit.Name)
+	tum.placed = append(tum.placed, u.Name)
 	return nil
 }
-func (tum *TestUnitManager) EnableUnitFile(unit string, runtime bool) error {
+func (tum *TestUnitManager) EnableUnitFile(u system.Unit) error {
-	tum.enabled = append(tum.enabled, unit)
+	tum.enabled = append(tum.enabled, u.Name)
 	return nil
 }
-func (tum *TestUnitManager) RunUnitCommand(command, unit string) (string, error) {
+func (tum *TestUnitManager) RunUnitCommand(u system.Unit, c string) (string, error) {
 	tum.commands = make(map[string]string)
-	tum.commands[unit] = command
+	tum.commands[u.Name] = c
 	return "", nil
 }
 func (tum *TestUnitManager) DaemonReload() error {
 	tum.reload = true
 	return nil
 }
-func (tum *TestUnitManager) MaskUnit(unit *system.Unit) error {
+func (tum *TestUnitManager) MaskUnit(u system.Unit) error {
-	tum.masked = append(tum.masked, unit.Name)
+	tum.masked = append(tum.masked, u.Name)
 	return nil
 }
-func (tum *TestUnitManager) UnmaskUnit(unit *system.Unit) error {
+func (tum *TestUnitManager) UnmaskUnit(u system.Unit) error {
-	tum.unmasked = append(tum.unmasked, unit.Name)
+	tum.unmasked = append(tum.unmasked, u.Name)
 	return nil
 }
 

system/file.go

@@ -19,6 +19,7 @@ package system
 import (
 	"fmt"
 	"io/ioutil"
+	"log"
 	"os"
 	"os/exec"
 	"path"
@@ -47,13 +48,14 @@ func (f *File) Permissions() (os.FileMode, error) {
 }
 
 func WriteFile(f *File, root string) (string, error) {
+	fullpath := path.Join(root, f.Path)
+	dir := path.Dir(fullpath)
+	log.Printf("Writing file to %q", fullpath)
+
 	if f.Encoding != "" {
 		return "", fmt.Errorf("Unable to write file with encoding %s", f.Encoding)
 	}
 
-	fullpath := path.Join(root, f.Path)
-	dir := path.Dir(fullpath)
-
 	if err := EnsureDirectoryExists(dir); err != nil {
 		return "", err
 	}
@@ -94,6 +96,7 @@ func WriteFile(f *File, root string) (string, error) {
 		return "", err
 	}
 
+	log.Printf("Wrote file to %q", fullpath)
 	return fullpath, nil
 }
 

system/networkd.go

@@ -96,7 +96,8 @@ func maybeProbeBonding(interfaces []network.InterfaceGenerator) error {
 
 func restartNetworkd() error {
 	log.Printf("Restarting networkd.service\n")
-	_, err := NewUnitManager("").RunUnitCommand("restart", "systemd-networkd.service")
+	networkd := Unit{config.Unit{Name: "systemd-networkd.service"}}
+	_, err := NewUnitManager("").RunUnitCommand(networkd, "restart")
 	return err
 }
 

system/systemd.go

@@ -23,7 +23,6 @@ import (
 	"os"
 	"os/exec"
 	"path"
-	"path/filepath"
 	"strings"
 
 	"github.com/coreos/coreos-cloudinit/Godeps/_workspace/src/github.com/coreos/go-systemd/dbus"
@@ -42,49 +41,38 @@ type systemd struct {
 // never be used as a true MachineID
 const fakeMachineID = "42000000000000000000000000000042"
 
-// PlaceUnit writes a unit file at the provided destination, creating
+// PlaceUnit writes a unit file at its desired destination, creating parent
-// parent directories as necessary.
+// directories as necessary.
-func (s *systemd) PlaceUnit(u *Unit, dst string) error {
+func (s *systemd) PlaceUnit(u Unit) error {
-	dir := filepath.Dir(dst)
-	if _, err := os.Stat(dir); os.IsNotExist(err) {
-		if err := os.MkdirAll(dir, os.FileMode(0755)); err != nil {
-			return err
-		}
-	}
-
 	file := File{config.File{
-		Path:               filepath.Base(dst),
+		Path:               u.Destination(s.root),
 		Content:            u.Content,
 		RawFilePermissions: "0644",
 	}}
 
-	_, err := WriteFile(&file, dir)
+	_, err := WriteFile(&file, "/")
-	if err != nil {
+	return err
-		return err
-	}
-
-	return nil
 }
 
-func (s *systemd) EnableUnitFile(unit string, runtime bool) error {
+func (s *systemd) EnableUnitFile(u Unit) error {
 	conn, err := dbus.New()
 	if err != nil {
 		return err
 	}
 
-	units := []string{unit}
+	units := []string{u.Name}
-	_, _, err = conn.EnableUnitFiles(units, runtime, true)
+	_, _, err = conn.EnableUnitFiles(units, u.Runtime, true)
 	return err
 }
 
-func (s *systemd) RunUnitCommand(command, unit string) (string, error) {
+func (s *systemd) RunUnitCommand(u Unit, c string) (string, error) {
 	conn, err := dbus.New()
 	if err != nil {
 		return "", err
 	}
 
 	var fn func(string, string) (string, error)
-	switch command {
+	switch c {
 	case "start":
 		fn = conn.StartUnit
 	case "stop":
@@ -100,10 +88,10 @@ func (s *systemd) RunUnitCommand(command, unit string) (string, error) {
 	case "reload-or-try-restart":
 		fn = conn.ReloadOrTryRestartUnit
 	default:
-		return "", fmt.Errorf("Unsupported systemd command %q", command)
+		return "", fmt.Errorf("Unsupported systemd command %q", c)
 	}
 
-	return fn(unit, "replace")
+	return fn(u.Name, "replace")
 }
 
 func (s *systemd) DaemonReload() error {
@@ -119,8 +107,8 @@ func (s *systemd) DaemonReload() error {
 // /dev/null, analogous to `systemctl mask`.
 // N.B.: Unlike `systemctl mask`, this function will *remove any existing unit
 // file at the location*, to ensure that the mask will succeed.
-func (s *systemd) MaskUnit(unit *Unit) error {
+func (s *systemd) MaskUnit(u Unit) error {
-	masked := unit.Destination(s.root)
+	masked := u.Destination(s.root)
 	if _, err := os.Stat(masked); os.IsNotExist(err) {
 		if err := os.MkdirAll(path.Dir(masked), os.FileMode(0755)); err != nil {
 			return err
@@ -134,8 +122,8 @@ func (s *systemd) MaskUnit(unit *Unit) error {
 // UnmaskUnit is analogous to systemd's unit_file_unmask. If the file
 // associated with the given Unit is empty or appears to be a symlink to
 // /dev/null, it is removed.
-func (s *systemd) UnmaskUnit(unit *Unit) error {
+func (s *systemd) UnmaskUnit(u Unit) error {
-	masked := unit.Destination(s.root)
+	masked := u.Destination(s.root)
 	ne, err := nullOrEmpty(masked)
 	if os.IsNotExist(err) {
 		return nil

system/systemd_test.go

@@ -51,7 +51,7 @@ Address=10.209.171.177/19
 		t.Fatalf("unit.Destination returned %s, expected %s", dst, expectDst)
 	}
 
-	if err := sd.PlaceUnit(&u, dst); err != nil {
+	if err := sd.PlaceUnit(u); err != nil {
 		t.Fatalf("PlaceUnit failed: %v", err)
 	}
 
@@ -128,7 +128,7 @@ Where=/media/state
 		t.Fatalf("unit.Destination returned %s, expected %s", dst, expectDst)
 	}
 
-	if err := sd.PlaceUnit(&u, dst); err != nil {
+	if err := sd.PlaceUnit(u); err != nil {
 		t.Fatalf("PlaceUnit failed: %v", err)
 	}
 
@@ -180,7 +180,7 @@ func TestMaskUnit(t *testing.T) {
 	sd := &systemd{dir}
 
 	// Ensure mask works with units that do not currently exist
-	uf := &Unit{config.Unit{Name: "foo.service"}}
+	uf := Unit{config.Unit{Name: "foo.service"}}
 	if err := sd.MaskUnit(uf); err != nil {
 		t.Fatalf("Unable to mask new unit: %v", err)
 	}
@@ -194,7 +194,7 @@ func TestMaskUnit(t *testing.T) {
 	}
 
 	// Ensure mask works with unit files that already exist
-	ub := &Unit{config.Unit{Name: "bar.service"}}
+	ub := Unit{config.Unit{Name: "bar.service"}}
 	barPath := path.Join(dir, "etc", "systemd", "system", "bar.service")
 	if _, err := os.Create(barPath); err != nil {
 		t.Fatalf("Error creating new unit file: %v", err)
@@ -220,12 +220,12 @@ func TestUnmaskUnit(t *testing.T) {
 
 	sd := &systemd{dir}
 
-	nilUnit := &Unit{config.Unit{Name: "null.service"}}
+	nilUnit := Unit{config.Unit{Name: "null.service"}}
 	if err := sd.UnmaskUnit(nilUnit); err != nil {
 		t.Errorf("unexpected error from unmasking nonexistent unit: %v", err)
 	}
 
-	uf := &Unit{config.Unit{Name: "foo.service", Content: "[Service]\nExecStart=/bin/true"}}
+	uf := Unit{config.Unit{Name: "foo.service", Content: "[Service]\nExecStart=/bin/true"}}
 	dst := uf.Destination(dir)
 	if err := os.MkdirAll(path.Dir(dst), os.FileMode(0755)); err != nil {
 		t.Fatalf("Unable to create unit directory: %v", err)
@@ -245,7 +245,7 @@ func TestUnmaskUnit(t *testing.T) {
 		t.Errorf("unmask of non-empty unit mutated unit contents unexpectedly")
 	}
 
-	ub := &Unit{config.Unit{Name: "bar.service"}}
+	ub := Unit{config.Unit{Name: "bar.service"}}
 	dst = ub.Destination(dir)
 	if err := os.Symlink("/dev/null", dst); err != nil {
 		t.Fatalf("Unable to create masked unit: %v", err)

system/unit.go

@@ -27,12 +27,12 @@ import (
 const cloudConfigDropIn = "20-cloudinit.conf"
 
 type UnitManager interface {
-	PlaceUnit(unit *Unit, dst string) error
+	PlaceUnit(unit Unit) error
-	EnableUnitFile(unit string, runtime bool) error
+	EnableUnitFile(unit Unit) error
-	RunUnitCommand(command, unit string) (string, error)
+	RunUnitCommand(unit Unit, command string) (string, error)
+	MaskUnit(unit Unit) error
+	UnmaskUnit(unit Unit) error
 	DaemonReload() error
-	MaskUnit(unit *Unit) error
-	UnmaskUnit(unit *Unit) error
 }
 
 // Unit is a top-level structure which embeds its underlying configuration,
@@ -41,10 +41,10 @@ type Unit struct {
 	config.Unit
 }
 
-// Destination builds the appropriate absolute file path for
+// Destination builds the appropriate absolute file path for the Unit. The root
-// the Unit. The root argument indicates the effective base
+// argument indicates the effective base directory of the system (similar to a
-// directory of the system (similar to a chroot).
+// chroot).
-func (u *Unit) Destination(root string) string {
+func (u Unit) Destination(root string) string {
 	dir := "etc"
 	if u.Runtime {
 		dir = "run"