Merge pull request #11147 from manadart/relocate-systemd-files

#11147 ### Checklist - [x] Checked if it requires a [pylibjuju](https://github.com/juju/python-libjuju) change? - [x] Added [integration tests](https://github.com/juju/juju/tree/develop/tests) for the PR? - [x] Added or updated [doc.go](https://discourse.jujucharms.com/t/readme-in-packages/451) related to packages changed? - [x] Do comments answer the question of why design decisions were made? ---- ## Description of change This patch changes the location of systemd unit files and start-up scripts. Instead of writing each service definition and it's exec-start script into a service folder in `/lib/systemd/system` and using dbus (systemd) to link them into `/etc/systemd/system`, we write them directly to `/etc/systemd/system`. This means the exec-start script is co-located with its service definition and prefixed with the service name. This change was required to support Focal Fossa (20.04) which has these two directories on different partitions, causing systemd to fail. The linking step remains for now, though it effectively does nothing - the service definitions are in the default search path, so reloading the systemd daemon should be sufficient to have them loaded. Subsequent patches will take care of any required upgrade paths, including upgrading the OS series. ## QA steps - Bootstrap to Focal: `juju bootstrap lxd systemd-test-focal --bootstrap-series focal --debug --no-gui --force --config image-stream=daily`. - SSH to the controller machine and observe the shutdown, database and machine services and scripts in `etc/systemd/system`. - `juju add-machine --series focal` - `juju deploy cs:ubuntu-13 -n 2 --to 0,0 --series focal --force` - `juju ssh 0` and observe the service units/scripts are present in `/etc/systemd/system`. - `juju remove-unit ubuntu/1` and observe that the service and script for the unit disappear. ## Documentation changes None. ## Bug reference https://bugs.launchpad.net/juju/+bug/1860432
juju · Jan 24, 2020 · e04f31b · e04f31b
2 parents a659955 + 8009b40
commit e04f31b
Show file tree

Hide file tree

Showing 12 changed files with 117 additions and 110 deletions.
diff --git a/cloudconfig/userdatacfg_unix.go b/cloudconfig/userdatacfg_unix.go
@@ -361,7 +361,7 @@ func (w *unixConfigure) ConfigureJuju() error {
 
 	// We add the machine agent's configuration info
 	// before running bootstrap-state so that bootstrap-state
-	// has a chance to rerwrite it to change the password.
+	// has a chance to rewrite it to change the password.
 	// It would be cleaner to change bootstrap-state to
 	// be responsible for starting the machine agent itself,
 	// but this would not be backwardly compatible.

diff --git a/service/agentconf.go b/service/agentconf.go
@@ -227,13 +227,7 @@ func (s *systemdServiceManager) WriteSystemdAgents(
 
 		// Otherwise we need to manually ensure the service unit links.
 		svcFileName := svcName + ".service"
-		if err = os.Symlink(path.Join(systemd.LibSystemdDir, svcName, svcFileName),
-			path.Join(symLinkSystemdDir, svcFileName)); err != nil && !os.IsExist(err) {
-			return nil, nil, nil, errors.Errorf(
-				"failed to link service file (%s) in systemd dir: %s\n", svcFileName, err)
-		}
-
-		if err = os.Symlink(path.Join(systemd.LibSystemdDir, svcName, svcFileName),
+		if err = os.Symlink(path.Join(systemd.EtcSystemdDir, svcFileName),
 			path.Join(symLinkSystemdMultiUserDir, svcFileName)); err != nil && !os.IsExist(err) {
 			return nil, nil, nil, errors.Errorf(
 				"failed to link service file (%s) in multi-user.target.wants dir: %s\n", svcFileName, err)

diff --git a/service/agentconf_test.go b/service/agentconf_test.go
@@ -25,7 +25,6 @@ import (
 	"github.com/juju/juju/service"
 	"github.com/juju/juju/service/common"
 	svctesting "github.com/juju/juju/service/common/testing"
-	"github.com/juju/juju/service/systemd"
 	"github.com/juju/juju/testing"
 	coretest "github.com/juju/juju/tools"
 	jujuversion "github.com/juju/juju/version"
@@ -314,7 +313,6 @@ func (s *agentConfSuite) TestWriteSystemdAgentsSystemdNotRunning(c *gc.C) {
 	c.Assert(startedSysServiceNames, gc.DeepEquals, append(s.agentUnitNames(), "jujud-"+s.machineName))
 	c.Assert(errAgents, gc.HasLen, 0)
 	s.assertServicesCalls(c, "WriteService", len(s.services))
-	s.assertServiceSymLinks(c)
 }
 
 func (s *agentConfSuite) TestWriteSystemdAgentsDBusErrManualLink(c *gc.C) {
@@ -372,16 +370,6 @@ func (s *agentConfSuite) assertToolsCopySymlink(c *gc.C, series string) {
 	}
 }
 
-func (s *agentConfSuite) assertServiceSymLinks(c *gc.C) {
-	for _, name := range append(s.unitNames, s.machineName) {
-		svcName := "jujud-" + name
-		svcFileName := svcName + ".service"
-		result, err := os.Readlink(path.Join(s.systemdDir, svcFileName))
-		c.Assert(err, jc.ErrorIsNil)
-		c.Assert(result, gc.Equals, path.Join(systemd.LibSystemdDir, svcName, svcFileName))
-	}
-}
-
 func (s *agentConfSuite) assertServicesCalls(c *gc.C, svc string, expectedCnt int) {
 	// Call list shared by the services
 	calls := s.services[0].Calls()

diff --git a/service/discovery.go b/service/discovery.go
@@ -23,13 +23,12 @@ import (
 // DiscoverService returns an interface to a service appropriate
 // for the current system
 func DiscoverService(name string, conf common.Conf) (Service, error) {
-	hostSeries := series.MustHostSeries()
-	initName, err := discoverInitSystem(hostSeries)
+	initName, err := discoverInitSystem(series.MustHostSeries())
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
 
-	service, err := newService(name, conf, initName, hostSeries)
+	service, err := newService(name, conf, initName)
 	if err != nil {
 		return nil, errors.Trace(err)
 	}

diff --git a/service/service.go b/service/service.go
@@ -128,11 +128,11 @@ var NewService = func(name string, conf common.Conf, series string) (Service, er
 	if err != nil {
 		return nil, errors.Trace(err)
 	}
-	return newService(name, conf, initSystem, series)
+	return newService(name, conf, initSystem)
 }
 
 // this needs to be stubbed out in some tests
-func newService(name string, conf common.Conf, initSystem, series string) (Service, error) {
+func newService(name string, conf common.Conf, initSystem string) (Service, error) {
 	var svc Service
 	var err error
 

diff --git a/service/systemd/dbusapi_mock_test.go b/service/systemd/dbusapi_mock_test.go
diff --git a/service/systemd/export_test.go b/service/systemd/export_test.go
@@ -32,8 +32,7 @@ func PatchNewChan(patcher patcher) chan string {
 
 func PatchFileOps(patcher patcher, ctrl *gomock.Controller) *MockShimFileOps {
 	mock := NewMockShimFileOps(ctrl)
-	patcher.PatchValue(&removeAll, mock.RemoveAll)
-	patcher.PatchValue(&mkdirAll, mock.MkdirAll)
+	patcher.PatchValue(&remove, mock.Remove)
 	patcher.PatchValue(&createFile, mock.CreateFile)
 	return mock
 }

diff --git a/service/systemd/service.go b/service/systemd/service.go
@@ -21,7 +21,6 @@ import (
 )
 
 const (
-	LibSystemdDir          = "/lib/systemd/system"
 	EtcSystemdDir          = "/etc/systemd/system"
 	EtcSystemdMultiUserDir = EtcSystemdDir + "/multi-user.target.wants"
 )
@@ -76,7 +75,7 @@ type Service struct {
 // NewServiceWithDefaults returns a new systemd service reference populated
 // with sensible defaults.
 func NewServiceWithDefaults(name string, conf common.Conf) (*Service, error) {
-	svc, err := NewService(name, conf, LibSystemdDir, NewDBusAPI, renderer.Join(paths.NixDataDir, "init"))
+	svc, err := NewService(name, conf, EtcSystemdDir, NewDBusAPI, renderer.Join(paths.NixDataDir, "init"))
 	return svc, errors.Trace(err)
 }
 
@@ -90,7 +89,7 @@ func NewService(
 	if conf.ExecStart != "" {
 		volName = renderer.VolumeName(common.Unquote(strings.Fields(conf.ExecStart)[0]))
 	}
-	dirName := volName + renderer.Join(dataDir, name)
+	dirName := volName + renderer.Join(dataDir)
 
 	service := &Service{
 		Service: common.Service{
@@ -184,7 +183,7 @@ func (s *Service) validate(conf common.Conf) error {
 }
 
 func (s *Service) normalize(conf common.Conf) (common.Conf, []byte) {
-	scriptPath := renderer.ScriptFilename("exec-start", s.DirName)
+	scriptPath := renderer.ScriptFilename(s.execStartFileName(), s.DirName)
 	return normalize(s.Service.Name, conf, scriptPath, &renderer)
 }
 
@@ -425,8 +424,7 @@ func (s *Service) remove() error {
 	}
 	defer conn.Close()
 
-	runtime := false
-	_, err = conn.DisableUnitFiles([]string{s.UnitName}, runtime)
+	_, err = conn.DisableUnitFiles([]string{s.UnitName}, false)
 	if err != nil {
 		return s.errorf(err, "dbus disable request failed")
 	}
@@ -435,15 +433,22 @@ func (s *Service) remove() error {
 		return s.errorf(err, "dbus post-disable daemon reload request failed")
 	}
 
-	if err := removeAll(s.DirName); err != nil {
-		return s.errorf(err, "failed to delete juju-managed conf dir")
+	// Remove the service unit file and the exec-start script.
+	if err := remove(path.Join(s.DirName, s.ConfName)); err != nil {
+		return s.errorf(err, "failed to delete service unit file")
+	}
+	if err := remove(renderer.ScriptFilename(s.execStartFileName(), s.DirName)); err != nil {
+		return s.errorf(err, "failed to delete service exec-start script")
 	}
 
 	return nil
 }
 
-var removeAll = func(name string) error {
-	return os.RemoveAll(name)
+var remove = func(name string) error {
+	if _, err := os.Stat(name); os.IsNotExist(err) {
+		return nil
+	}
+	return os.Remove(name)
 }
 
 // Install implements Service.
@@ -495,13 +500,10 @@ func (s *Service) writeConf() (string, error) {
 		return "", errors.Trace(err)
 	}
 
-	if err := mkdirAll(s.DirName); err != nil {
-		return "", s.errorf(err, "failed to create juju-managed service dir %q", s.DirName)
-	}
 	filename := path.Join(s.DirName, s.ConfName)
 
 	if s.Script != nil {
-		scriptPath := renderer.ScriptFilename("exec-start", s.DirName)
+		scriptPath := renderer.ScriptFilename(s.execStartFileName(), s.DirName)
 		if scriptPath != s.Service.Conf.ExecStart {
 			err := errors.Errorf("wrong script path: expected %q, got %q", scriptPath, s.Service.Conf.ExecStart)
 			return filename, s.errorf(err, "failed to write script at %q", scriptPath)
@@ -519,10 +521,6 @@ func (s *Service) writeConf() (string, error) {
 	return filename, nil
 }
 
-var mkdirAll = func(dirname string) error {
-	return os.MkdirAll(dirname, 0755)
-}
-
 var createFile = func(filename string, data []byte, perm os.FileMode) error {
 	return ioutil.WriteFile(filename, data, perm)
 }
@@ -541,11 +539,9 @@ func (s *Service) InstallCommands() ([]string, error) {
 		return nil, errors.Trace(err)
 	}
 
-	cmdList := []string{
-		cmds.mkdirs(dirname),
-	}
+	var cmdList []string
 	if s.Script != nil {
-		scriptName := renderer.Base(renderer.ScriptFilename("exec-start", ""))
+		scriptName := renderer.Base(renderer.ScriptFilename(s.execStartFileName(), ""))
 		cmdList = append(cmdList, []string{
 			// TODO(ericsnow) Use the renderer here.
 			cmds.writeFile(scriptName, dirname, s.Script),
@@ -606,6 +602,15 @@ func (s *Service) WriteService() error {
 	return nil
 }
 
+// scriptFileName returns the name of the file that will contain a start script
+// indicated by the service unit file `ExecStart` property.
+// This is required for non-trivial start-up logic, as bash-isms are not
+// supported.
+//See: https://www.freedesktop.org/software/systemd/man/systemd.service.html#Command%20lines
+func (s *Service) execStartFileName() string {
+	return s.Name() + "-exec-start"
+}
+
 // SysdReload reloads Service daemon.
 func SysdReload() error {
 	err := Cmdline{}.reload()