Fix for systemd instantiated services when enabled via ignition

config/shared/errors/errors.go

@@ -74,6 +74,8 @@ var (
 	// Systemd section errors
 	ErrInvalidSystemdExt       = errors.New("invalid systemd unit extension")
 	ErrInvalidSystemdDropinExt = errors.New("invalid systemd drop-in extension")
+	ErrNoSystemdExt            = errors.New("no systemd unit extension")
+	ErrInvalidInstantiatedUnit = errors.New("invalid systemd instantiated unit")
 
 	// Misc errors
 	ErrInvalidScheme       = errors.New("invalid url scheme")

internal/exec/stages/files/units.go

@@ -19,35 +19,72 @@ import (
 	"path/filepath"
 	"strings"
 
+	"github.com/coreos/ignition/v2/config/shared/errors"
 	"github.com/coreos/ignition/v2/config/v3_1_experimental/types"
 	"github.com/coreos/ignition/v2/internal/distro"
 	"github.com/coreos/ignition/v2/internal/exec/util"
+	"github.com/coreos/ignition/v2/internal/systemd"
 )
 
+// Preset holds the information about
+// a given systemd unit.
+type Preset struct {
+	unit           string
+	enabled        bool
+	instantiatable bool
+	instances      []string
+}
+
+// warnOnOldSystemdVersion checks the version of Systemd
+// in a given system and prints a warning if older than 240.
+func (s *stage) warnOnOldSystemdVersion() error {
+	systemdVersion, err := systemd.GetSystemdVersion()
+	if err != nil {
+		return err
+	}
+	if systemdVersion < 240 {
+		if err := s.Logger.Warning("The version of systemd (%q) is less than 240. Enabling/disabling instantiated units may not work. See https://github.com/coreos/ignition/issues/586 for more information.", systemdVersion); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 // createUnits creates the units listed under systemd.units.
 func (s *stage) createUnits(config types.Config) error {
-	enabledOneUnit := false
+	presets := make(map[string]*Preset)
 	for _, unit := range config.Systemd.Units {
 		if err := s.writeSystemdUnit(unit, false); err != nil {
 			return err
 		}
 		if unit.Enabled != nil {
+			// identifier keyword is used to distinguish systemd units
+			// which are either enabled or disabled. Appending
+			// it to a unitName will avoid overwriting the existing
+			// unitName's instance if the state of the unit is different.
+			identifier := "disabled"
 			if *unit.Enabled {
-				if err := s.Logger.LogOp(
-					func() error { return s.EnableUnit(unit) },
-					"enabling unit %q", unit.Name,
-				); err != nil {
+				identifier = "enabled"
+			}
+			if strings.Contains(unit.Name, "@") {
+				unitName, instance, err := parseInstanceUnit(unit)
+				if err != nil {
 					return err
 				}
+				key := fmt.Sprintf("%s-%s", unitName, identifier)
+				if _, ok := presets[key]; ok {
+					presets[key].instances = append(presets[key].instances, instance)
+				} else {
+					presets[key] = &Preset{unitName, *unit.Enabled, true, []string{instance}}
+				}
 			} else {
-				if err := s.Logger.LogOp(
-					func() error { return s.DisableUnit(unit) },
-					"disabling unit %q", unit.Name,
-				); err != nil {
-					return err
+				key := fmt.Sprintf("%s-%s", unit.Name, identifier)
+				if _, ok := presets[unit.Name]; !ok {
+					presets[key] = &Preset{unit.Name, *unit.Enabled, false, []string{}}
+				} else {
+					return fmt.Errorf("%q key is already present in the presets map", key)
 				}
 			}
-			enabledOneUnit = true
 		}
 		if unit.Mask != nil && *unit.Mask {
 			relabelpath := ""
@@ -64,10 +101,71 @@ func (s *stage) createUnits(config types.Config) error {
 			s.relabel(relabelpath)
 		}
 	}
-	// and relabel the preset file itself if we enabled/disabled something
-	if enabledOneUnit {
-		s.relabel(util.PresetPath)
+	// if we have presets then create the systemd preset file.
+	if len(presets) != 0 {
+		if err := s.createSystemdPresetFile(presets); err != nil {
+			return err
+		}
+	}
+
+	return nil
+}
+
+// parseInstanceUnit extracts the name and a corresponding instance
+// for a given instantiated unit.
+// e.g: [email protected] ==> [email protected] & instance=bar
+func parseInstanceUnit(unit types.Unit) (string, string, error) {
+	at := strings.Index(unit.Name, "@")
+	if at == -1 {
+		return "", "", errors.ErrInvalidInstantiatedUnit
+	}
+	dot := strings.LastIndex(unit.Name, ".")
+	if dot == -1 {
+		return "", "", errors.ErrNoSystemdExt
+	}
+	instance := unit.Name[at+1 : dot]
+	serviceInstance := unit.Name[0:at+1] + unit.Name[dot:len(unit.Name)]
+	return serviceInstance, instance, nil
+}
+
+// createSystemdPresetFile creates the presetfile for enabled/disabled
+// systemd units.
+func (s *stage) createSystemdPresetFile(presets map[string]*Preset) error {
+	hasInstanceUnit := false
+	for _, value := range presets {
+		unitString := value.unit
+		if value.instantiatable {
+			hasInstanceUnit = true
+			// Let's say we have two instantiated enabled units listed under
+			// the systemd units i.e. [email protected], [email protected]
+			// then the unitString will look like "[email protected] foo bar"
+			unitString = fmt.Sprintf("%s %s", unitString, strings.Join(value.instances, " "))
+		}
+		if value.enabled {
+			if err := s.Logger.LogOp(
+				func() error { return s.EnableUnit(unitString) },
+				"setting preset to enabled for %q", unitString,
+			); err != nil {
+				return err
+			}
+		} else {
+			if err := s.Logger.LogOp(
+				func() error { return s.DisableUnit(unitString) },
+				"setting preset to disabled for %q", unitString,
+			); err != nil {
+				return err
+			}
+		}
+	}
+	// Print the warning if there's an instantiated unit present under
+	// the systemd units and the version of systemd in a given system
+	// is older than 240.
+	if hasInstanceUnit {
+		if err := s.warnOnOldSystemdVersion(); err != nil {
+			return err
+		}
 	}
+	s.relabel(util.PresetPath)
 	return nil
 }
 

internal/exec/stages/files/units_test.go

@@ -0,0 +1,76 @@
+// Copyright 2020 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package files
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/coreos/ignition/v2/config/shared/errors"
+	"github.com/coreos/ignition/v2/config/v3_1_experimental/types"
+)
+
+func TestParseInstanceUnit(t *testing.T) {
+	type in struct {
+		unit types.Unit
+	}
+	type out struct {
+		unitName string
+		instance string
+		parseErr error
+	}
+	tests := []struct {
+		in  in
+		out out
+	}{
+		{in: in{types.Unit{Name: "[email protected]"}},
+			out: out{unitName: "[email protected]", instance: "bar",
+				parseErr: nil},
+		},
+
+		{in: in{types.Unit{Name: "[email protected]"}},
+			out: out{unitName: "[email protected]", instance: "foo",
+				parseErr: nil},
+		},
+		{in: in{types.Unit{Name: "echo.service"}},
+			out: out{unitName: "", instance: "",
+				parseErr: errors.ErrInvalidInstantiatedUnit},
+		},
+		{in: in{types.Unit{Name: "echo@fooservice"}},
+			out: out{unitName: "", instance: "",
+				parseErr: errors.ErrNoSystemdExt},
+		},
+		{in: in{types.Unit{Name: "[email protected]"}},
+			out: out{unitName: "[email protected]", instance: "",
+				parseErr: nil},
+		},
+		{in: in{types.Unit{Name: "[email protected]"}},
+			out: out{unitName: "[email protected]", instance: "9.3-main",
+				parseErr: nil},
+		},
+	}
+	for i, test := range tests {
+		unitName, instance, err := parseInstanceUnit(test.in.unit)
+		if test.out.parseErr != err {
+			t.Errorf("#%d: bad error: want %v, got %v", i, test.out.parseErr, err)
+		}
+		if !reflect.DeepEqual(test.out.unitName, unitName) {
+			t.Errorf("#%d: bad unitName: want %v, got %v", i, test.out.unitName, unitName)
+		}
+		if !reflect.DeepEqual(test.out.instance, instance) {
+			t.Errorf("#%d: bad instance: want %v, got %v", i, test.out.instance, instance)
+		}
+	}
+}

internal/exec/util/unit.go

@@ -110,8 +110,8 @@ func (ut Util) MaskUnit(unit types.Unit) (string, error) {
 	return filepath.Join("/", SystemdUnitsPath(), unit.Name), nil
 }
 
-func (ut Util) EnableUnit(unit types.Unit) error {
-	return ut.appendLineToPreset(fmt.Sprintf("enable %s", unit.Name))
+func (ut Util) EnableUnit(enabledUnit string) error {
+	return ut.appendLineToPreset(fmt.Sprintf("enable %s", enabledUnit))
 }
 
 // presets link in /etc, which doesn't make sense for runtime units
@@ -145,8 +145,8 @@ func (ut Util) EnableRuntimeUnit(unit types.Unit, target string) error {
 	return ut.WriteLink(link)
 }
 
-func (ut Util) DisableUnit(unit types.Unit) error {
-	return ut.appendLineToPreset(fmt.Sprintf("disable %s", unit.Name))
+func (ut Util) DisableUnit(disabledUnit string) error {
+	return ut.appendLineToPreset(fmt.Sprintf("disable %s", disabledUnit))
 }
 
 func (ut Util) appendLineToPreset(data string) error {

internal/systemd/systemd.go

@@ -16,6 +16,8 @@ package systemd
 
 import (
 	"fmt"
+	"regexp"
+	"strconv"
 
 	"github.com/coreos/go-systemd/dbus"
 	"github.com/coreos/go-systemd/unit"
@@ -48,3 +50,27 @@ func WaitOnDevices(devs []string, stage string) error {
 
 	return nil
 }
+
+// GetSystemdVersion fetches the version of Systemd
+// in a given system.
+func GetSystemdVersion() (uint, error) {
+	conn, err := dbus.NewSystemdConnection()
+	if err != nil {
+		return 0, err
+	}
+	version, err := conn.GetManagerProperty("Version")
+	if err != nil {
+		return 0, err
+	}
+	// Handle different systemd versioning schemes that are being returned.
+	// for e.g:
+	// - Fedora 31: `"v243.5-1.fc31"`
+	// - RHEL 8: `"239"`
+	re := regexp.MustCompile(`\d+`)
+	systemdVersion := re.FindString(version)
+	value, err := strconv.Atoi(systemdVersion)
+	if err != nil {
+		return 0, err
+	}
+	return uint(value), nil
+}

tests/positive/files/units.go

@@ -0,0 +1,74 @@
+// Copyright 2020 CoreOS, Inc.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package files
+
+import (
+	"github.com/coreos/ignition/v2/tests/register"
+	"github.com/coreos/ignition/v2/tests/types"
+)
+
+func init() {
+	register.Register(register.PositiveTest, CreateInstantiatedService())
+}
+
+func CreateInstantiatedService() types.Test {
+	name := "instantiated.unit.create"
+	in := types.GetBaseDisk()
+	out := types.GetBaseDisk()
+	config := `{
+		"ignition": { "version": "$version" },
+		"systemd": {
+			"units": [
+			  {
+				"name": "[email protected]"
+				"contents": "[Unit]\nDescription=f\n[Service]\nType=oneshot\nExecStart=/bin/echo %i\nRemainAfterExit=yes\n[Install]\nWantedBy=multi-user.target\n",
+			  },
+			  {
+				"enabled": true,
+				"name": "[email protected]"
+			  },
+			  {
+				"enabled": true,
+				"name": "[email protected]"
+			  },
+			]
+		  }
+		}`
+	configMinVersion := "3.0.0"
+	out[0].Partitions.AddFiles("ROOT", []types.File{
+		{
+			Node: types.Node{
+				Name:      "[email protected]",
+				Directory: "etc/systemd/system",
+			},
+			Contents: "[Unit]\nDescription=f\n[Service]\nType=oneshot\nExecStart=/bin/echo %i\nRemainAfterExit=yes\n[Install]\nWantedBy=multi-user.target\n",
+		},
+		{
+			Node: types.Node{
+				Name:      "20-ignition.preset",
+				Directory: "etc/systemd/system-preset",
+			},
+			Contents: "enable [email protected] bar foo\n",
+		},
+	})
+
+	return types.Test{
+		Name:             name,
+		In:               in,
+		Out:              out,
+		Config:           config,
+		ConfigMinVersion: configMinVersion,
+	}
+}