From dcaabe4d4a8589e1bd73c506dc6f03c8302787fc Mon Sep 17 00:00:00 2001 From: Alex Crawford Date: Mon, 24 Nov 2014 16:42:31 -0800 Subject: [PATCH] system: clean up UnitManager interface --- initialize/config.go | 38 ++++++++++++++++---------------- initialize/config_test.go | 20 ++++++++--------- system/file.go | 9 +++++--- system/networkd.go | 3 ++- system/systemd.go | 46 +++++++++++++++------------------------ system/systemd_test.go | 14 ++++++------ system/unit.go | 18 +++++++-------- 7 files changed, 70 insertions(+), 78 deletions(-) diff --git a/initialize/config.go b/initialize/config.go index a2024ce..9e169ce 100644 --- a/initialize/config.go +++ b/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) - if err := um.PlaceUnit(&unit, dst); err != nil { + log.Printf("Writing unit %q to filesystem", unit.Name) + 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) - if err := um.MaskUnit(&unit); err != nil { + log.Printf("Masking unit file %q", unit.Name) + if err := um.MaskUnit(unit); err != nil { return err } } else if unit.Runtime { - log.Printf("Ensuring runtime unit file %s is unmasked", unit.Name) - if err := um.UnmaskUnit(&unit); err != nil { + log.Printf("Ensuring runtime unit file %q is unmasked", unit.Name) + if err := um.UnmaskUnit(unit); err != nil { return err } } if unit.Enable { if unit.Group() != "network" { - log.Printf("Enabling unit file %s", unit.Name) - if err := um.EnableUnitFile(unit.Name, unit.Runtime); err != nil { + log.Printf("Enabling unit file %q", unit.Name) + 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) - res, err := um.RunUnitCommand(action.command, action.unit) + log.Printf("Calling unit command %q on %q'", action.command, action.unit.Name) + 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 diff --git a/initialize/config_test.go b/initialize/config_test.go index fd67d7e..10a5ae3 100644 --- a/initialize/config_test.go +++ b/initialize/config_test.go @@ -32,29 +32,29 @@ type TestUnitManager struct { reload bool } -func (tum *TestUnitManager) PlaceUnit(unit *system.Unit, dst string) error { - tum.placed = append(tum.placed, unit.Name) +func (tum *TestUnitManager) PlaceUnit(u system.Unit) error { + tum.placed = append(tum.placed, u.Name) return nil } -func (tum *TestUnitManager) EnableUnitFile(unit string, runtime bool) error { - tum.enabled = append(tum.enabled, unit) +func (tum *TestUnitManager) EnableUnitFile(u system.Unit) error { + 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 { - tum.masked = append(tum.masked, unit.Name) +func (tum *TestUnitManager) MaskUnit(u system.Unit) error { + tum.masked = append(tum.masked, u.Name) return nil } -func (tum *TestUnitManager) UnmaskUnit(unit *system.Unit) error { - tum.unmasked = append(tum.unmasked, unit.Name) +func (tum *TestUnitManager) UnmaskUnit(u system.Unit) error { + tum.unmasked = append(tum.unmasked, u.Name) return nil } diff --git a/system/file.go b/system/file.go index f56ab10..ad8673f 100644 --- a/system/file.go +++ b/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 } diff --git a/system/networkd.go b/system/networkd.go index 528aaa2..056eb6a 100644 --- a/system/networkd.go +++ b/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 } diff --git a/system/systemd.go b/system/systemd.go index 8200333..2c346a5 100644 --- a/system/systemd.go +++ b/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 -// parent directories as necessary. -func (s *systemd) PlaceUnit(u *Unit, dst string) 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 - } - } - +// PlaceUnit writes a unit file at its desired destination, creating parent +// directories as necessary. +func (s *systemd) PlaceUnit(u Unit) error { file := File{config.File{ - Path: filepath.Base(dst), + Path: u.Destination(s.root), Content: u.Content, RawFilePermissions: "0644", }} - _, err := WriteFile(&file, dir) - if err != nil { - return err - } - - return nil + _, err := WriteFile(&file, "/") + return err } -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} - _, _, err = conn.EnableUnitFiles(units, runtime, true) + units := []string{u.Name} + _, _, 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 { - masked := unit.Destination(s.root) +func (s *systemd) MaskUnit(u Unit) error { + 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 { - masked := unit.Destination(s.root) +func (s *systemd) UnmaskUnit(u Unit) error { + masked := u.Destination(s.root) ne, err := nullOrEmpty(masked) if os.IsNotExist(err) { return nil diff --git a/system/systemd_test.go b/system/systemd_test.go index 2d4f1d5..1a728f6 100644 --- a/system/systemd_test.go +++ b/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) diff --git a/system/unit.go b/system/unit.go index 26e9bbc..b48db7b 100644 --- a/system/unit.go +++ b/system/unit.go @@ -27,12 +27,12 @@ import ( const cloudConfigDropIn = "20-cloudinit.conf" type UnitManager interface { - PlaceUnit(unit *Unit, dst string) error - EnableUnitFile(unit string, runtime bool) error - RunUnitCommand(command, unit string) (string, error) + PlaceUnit(unit Unit) error + EnableUnitFile(unit Unit) 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 -// the Unit. The root argument indicates the effective base -// directory of the system (similar to a chroot). -func (u *Unit) Destination(root string) string { +// Destination builds the appropriate absolute file path for the Unit. The root +// argument indicates the effective base directory of the system (similar to a +// chroot). +func (u Unit) Destination(root string) string { dir := "etc" if u.Runtime { dir = "run"