Simplify systemd detection and use the upstream method instead of cop…

…ying its logic. Includes some drive-by comment and test assertion fixes.
google38438 · Nov 27, 2018 · 7c3f0f2 · 7c3f0f2
1 parent 294ca9f
commit 7c3f0f2
Show file tree

Hide file tree

Showing 4 changed files with 29 additions and 44 deletions.
diff --git a/service/agentconf.go b/service/agentconf.go
@@ -9,7 +9,7 @@
 // 	- copy all tools and related to agents and setup the links
 // 	- start all the agents
 // These routines can be used by any tools/cmds trying to implement the above
-// functionality as part of the process, eg. juju-updateseries command.
+// functionality as part of the process, eg. upgrade-series.
 
 // TODO (manadart 2018-07-31) This module is specific to systemd and should
 // reside in the service/systemd package.
@@ -63,7 +63,7 @@ type SystemdServiceManager interface {
 }
 
 type systemdServiceManager struct {
-	isRunning  func() (bool, error)
+	isRunning  func() bool
 	newService func(string, common.Conf) (Service, error)
 }
 
@@ -81,7 +81,7 @@ func NewServiceManagerWithDefaults() SystemdServiceManager {
 // NewServiceManager allows creation of a new SystemdServiceManager from
 // custom dependencies.
 func NewServiceManager(
-	isRunning func() (bool, error),
+	isRunning func() bool,
 	newService func(string, common.Conf) (Service, error),
 ) SystemdServiceManager {
 	return &systemdServiceManager{
@@ -192,6 +192,7 @@ func (s *systemdServiceManager) WriteSystemdAgents(
 		if !ok {
 			return nil, nil, nil, errors.Errorf("%s service not of type UpgradableService", svcName)
 		}
+
 		if err = uSvc.WriteService(); err != nil {
 			logger.Infof("failed to write service for %s: %s", agentName, err)
 			errAgentNames = append(errAgentNames, agentName)
@@ -201,11 +202,7 @@ func (s *systemdServiceManager) WriteSystemdAgents(
 			logger.Infof("successfully wrote service for %s:", agentName)
 		}
 
-		running, err := s.isRunning()
-		switch {
-		case err != nil:
-			return nil, nil, nil, errors.Errorf("failed to determine if systemd is running: %#v\n", err)
-		case running:
+		if s.isRunning() {
 			// If systemd is the running init system on this host, then the
 			// call to DBusAPI.LinkUnitFiles in WriteService above will have
 			// automatically updated sym-links for the file. We are done.
@@ -339,25 +336,21 @@ func (s *systemdServiceManager) CopyAgentBinary(
 func (s *systemdServiceManager) StartAllAgents(
 	machineAgent string, unitAgents []string, dataDir string,
 ) (string, []string, error) {
-	running, err := s.isRunning()
-	if err != nil {
-		return "", nil, errors.Trace(err)
-	}
-	if !running {
+	if !s.isRunning() {
 		return "", nil, errors.Errorf("cannot interact with systemd; reboot to start agents")
 	}
 
 	var startedUnits []string
 	for _, unit := range unitAgents {
-		if err = s.startAgent(unit, AgentKindUnit, dataDir); err != nil {
+		if err := s.startAgent(unit, AgentKindUnit, dataDir); err != nil {
 			return "", startedUnits, errors.Annotatef(err, "failed to start %s service", serviceName(unit))
 		}
 		startedUnits = append(startedUnits, serviceName(unit))
 		logger.Infof("started %s service", serviceName(unit))
 	}
 
 	machineService := serviceName(machineAgent)
-	err = s.startAgent(machineAgent, AgentKindMachine, dataDir)
+	err := s.startAgent(machineAgent, AgentKindMachine, dataDir)
 	if err == nil {
 		logger.Infof("started %s service", machineService)
 		return machineService, startedUnits, nil

diff --git a/service/agentconf_test.go b/service/agentconf_test.go
@@ -1,7 +1,7 @@
 // Copyright 2018 Canonical Ltd.
 // Licensed under the AGPLv3, see LICENCE file for details.
 
-//The unit testcases in this file do not pertain to an specific command.
+// The test cases in this file do not pertain to a specific command.
 
 package service_test
 
@@ -38,8 +38,8 @@ type agentConfSuite struct {
 	dataDir             string
 	machineName         string
 	unitNames           []string
-	systemdDir          string // updateseries.systemDir
-	systemdMultiUserDir string // updateseries.systemMultiUserDir
+	systemdDir          string
+	systemdMultiUserDir string
 	systemdDataDir      string // service.SystemdDataDir
 	manager             service.SystemdServiceManager
 
@@ -57,14 +57,14 @@ func (s *agentConfSuite) SetUpTest(c *gc.C) {
 	s.dataDir = c.MkDir()
 	s.systemdDir = path.Join(s.dataDir, "etc", "systemd", "system")
 	s.systemdMultiUserDir = path.Join(s.systemdDir, "multi-user.target.wants")
-	os.MkdirAll(s.systemdMultiUserDir, os.ModeDir|os.ModePerm)
+	c.Assert(os.MkdirAll(s.systemdMultiUserDir, os.ModeDir|os.ModePerm), jc.ErrorIsNil)
 	s.systemdDataDir = path.Join(s.dataDir, "lib", "systemd", "system")
 
 	s.machineName = "machine-0"
 	s.unitNames = []string{"unit-ubuntu-0", "unit-mysql-0"}
 
 	s.manager = service.NewServiceManager(
-		func() (bool, error) { return true, nil },
+		func() bool { return true },
 		s.newService,
 	)
 
@@ -108,15 +108,16 @@ func (s *agentConfSuite) setUpAgentConf(c *gc.C) {
 
 func (s *agentConfSuite) setUpServices(c *gc.C) {
 	for _, fake := range append(s.unitNames, s.machineName) {
-		s.addService("jujud-" + fake)
+		s.addService(c, "jujud-"+fake)
 	}
 	s.PatchValue(&service.ListServices, s.listServices)
 }
 
-func (s *agentConfSuite) addService(name string) {
-	svc, _ := s.newService(name, common.Conf{})
-	svc.Install()
-	svc.Start()
+func (s *agentConfSuite) addService(c *gc.C, name string) {
+	svc, err := s.newService(name, common.Conf{})
+	c.Assert(err, jc.ErrorIsNil)
+	c.Assert(svc.Install(), jc.ErrorIsNil)
+	c.Assert(svc.Start(), jc.ErrorIsNil)
 }
 
 func (s *agentConfSuite) listServices() ([]string, error) {
@@ -258,7 +259,7 @@ func (s *agentConfSuite) TestStartAllAgentsFailMachine(c *gc.C) {
 
 func (s *agentConfSuite) TestStartAllAgentsSystemdNotRunning(c *gc.C) {
 	s.manager = service.NewServiceManager(
-		func() (bool, error) { return false, nil },
+		func() bool { return false },
 		s.newService,
 	)
 
@@ -289,8 +290,6 @@ func (s *agentConfSuite) TestCopyAgentBinaryOriginalAgentBinariesNotFound(c *gc.
 }
 
 func (s *agentConfSuite) TestWriteSystemdAgents(c *gc.C) {
-	s.PatchValue(&systemd.SystemPath, s.dataDir)
-
 	startedSymLinkAgents, startedSysServiceNames, errAgents, err := s.manager.WriteSystemdAgents(
 		s.machineName, s.unitNames, s.systemdDataDir, s.systemdDir, s.systemdMultiUserDir)
 
@@ -303,7 +302,7 @@ func (s *agentConfSuite) TestWriteSystemdAgents(c *gc.C) {
 
 func (s *agentConfSuite) TestWriteSystemdAgentsSystemdNotRunning(c *gc.C) {
 	s.manager = service.NewServiceManager(
-		func() (bool, error) { return false, nil },
+		func() bool { return false },
 		s.newService,
 	)
 
@@ -319,7 +318,6 @@ func (s *agentConfSuite) TestWriteSystemdAgentsSystemdNotRunning(c *gc.C) {
 }
 
 func (s *agentConfSuite) TestWriteSystemdAgentsWriteServiceFail(c *gc.C) {
-	s.PatchValue(&systemd.SystemPath, s.dataDir)
 	s.services[0].SetErrors(
 		nil,
 		nil,

diff --git a/service/discovery.go b/service/discovery.go
@@ -100,7 +100,7 @@ type discoveryCheck struct {
 
 var discoveryFuncs = []discoveryCheck{
 	{InitSystemUpstart, upstart.IsRunning},
-	{InitSystemSystemd, systemd.IsRunning},
+	{InitSystemSystemd, func() (bool, error) { return systemd.IsRunning(), nil }},
 	{InitSystemWindows, windows.IsRunning},
 }
 

diff --git a/service/systemd/service.go b/service/systemd/service.go
@@ -11,6 +11,7 @@ import (
 	"strings"
 
 	"github.com/coreos/go-systemd/dbus"
+	"github.com/coreos/go-systemd/util"
 	"github.com/juju/errors"
 	"github.com/juju/loggo"
 	"github.com/juju/utils/shell"
@@ -30,19 +31,12 @@ var (
 
 	renderer = shell.BashRenderer{}
 	cmds     = commands{renderer, executable}
-
-	SystemPath = "/run/systemd/system"
 )
 
 // IsRunning returns whether or not systemd is the local init system.
-func IsRunning() (bool, error) {
-	if _, err := os.Stat(SystemPath); err == nil {
-		return true, nil
-	} else if os.IsNotExist(err) {
-		return false, nil
-	} else {
-		return false, errors.Trace(err)
-	}
+// It has a signature consistent with similar methods in other service modules.
+func IsRunning() bool {
+	return util.IsRunningSystemd()
 }
 
 // ListServices returns the list of installed service names.
@@ -584,9 +578,9 @@ func (s *Service) WriteService() error {
 		return errors.Trace(err)
 	}
 
-	if running, err := IsRunning(); err != nil {
-		return errors.Trace(err)
-	} else if !running {
+	// If systemd is not the running init system,
+	// then do not attempt to use it for linking unit files.
+	if !IsRunning() {
 		return nil
 	}