fixes systemd unit creation for empty units

If a MachineConfig is applied with empty systemd unit contents, the MCD
will degrade because it skips writing the file in that particular
situation. For parity with the CoreOS Ignition implementation, we should
not attempt to enable or disable any systemd units where the unit file
does not have any contents.

Author: cheesesashimi

pkg/daemon/file_writers.go

@@ -290,7 +290,8 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error
 		return nil
 	}
 
-	if u.Contents != nil && *u.Contents != "" {
+	switch {
+	case unitHasContent(u):
 		klog.Infof("Writing systemd unit %q", u.Name)
 		if _, err := os.Stat(withUsrPath(fpath)); err == nil &&
 			isCoreOSVariant {
@@ -315,7 +316,7 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error
 		}
 
 		klog.V(2).Infof("Successfully wrote systemd unit %q: ", u.Name)
-	} else if u.Mask != nil && !*u.Mask {
+	case u.Mask != nil && !*u.Mask:
 		// if mask is explicitly set to false, make sure to remove a previous mask
 		// see https://bugzilla.redhat.com/show_bug.cgi?id=1966445
 		// Note that this does not catch all cleanup cases; for example, if the previous machine config specified
@@ -325,11 +326,20 @@ func writeUnit(u ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error
 		if err := os.RemoveAll(fpath); err != nil {
 			return fmt.Errorf("failed to cleanup %s: %w", fpath, err)
 		}
+	default:
+		klog.Infof("Unit %q has no content, skipping write", u.Name)
 	}
 
 	return nil
 }
 
+// Determines if a systemd unit has contents by first checking whether the
+// contents field is nil and then checking if the contents field is an empty
+// string.
+func unitHasContent(u ign3types.Unit) bool {
+	return u.Contents != nil && *u.Contents != ""
+}
+
 // writeUnits writes systemd units and their dropins to disk
 func writeUnits(units []ign3types.Unit, systemdRoot string, isCoreOSVariant bool) error {
 	for _, u := range units {

pkg/daemon/update.go

@@ -2119,10 +2119,20 @@ func (dn *Daemon) writeUnits(units []ign3types.Unit) error {
 		// to not go through.
 
 		if u.Enabled != nil {
-			if *u.Enabled {
-				enabledUnits = append(enabledUnits, u.Name)
+			// Only when a unit has contents should we attempt to enable or disable it.
+			// See: https://issues.redhat.com/browse/OCPBUGS-56648
+			if unitHasContent(u) {
+				if *u.Enabled {
+					enabledUnits = append(enabledUnits, u.Name)
+				} else {
+					disabledUnits = append(disabledUnits, u.Name)
+				}
 			} else {
-				disabledUnits = append(disabledUnits, u.Name)
+				action := "disable"
+				if *u.Enabled {
+					action = "enable"
+				}
+				klog.Infof("Could not %s unit %q, because it has no contents, skipping", action, u.Name)
 			}
 		} else {
 			if err := dn.presetUnit(u); err != nil {

test/e2e-1of2/mcd_test.go

@@ -620,7 +620,7 @@ func TestDontDeleteRPMFiles(t *testing.T) {
 func TestIgn3Cfg(t *testing.T) {
 	cs := framework.NewClientSet("")
 
-	delete := helpers.CreateMCP(t, cs, "infra")
+	deleteFunc := helpers.CreateMCP(t, cs, "infra")
 	workerOldMc := helpers.GetMcName(t, cs, "worker")
 
 	unlabelFunc := helpers.LabelRandomNodeFromPool(t, cs, "worker", "node-role.kubernetes.io/infra")
@@ -631,7 +631,7 @@ func TestIgn3Cfg(t *testing.T) {
 		if err := helpers.WaitForPoolComplete(t, cs, "worker", workerOldMc); err != nil {
 			t.Fatal(err)
 		}
-		delete()
+		deleteFunc()
 	})
 	// create a dummy MC with an sshKey for user Core
 	mcName := fmt.Sprintf("99-ign3cfg-infra-%s", uuid.NewUUID())
@@ -650,6 +650,59 @@ func TestIgn3Cfg(t *testing.T) {
 	tempFile := ign3types.File{Node: ign3types.Node{Path: "/etc/testfileconfig"},
 		FileEmbedded1: ign3types.FileEmbedded1{Contents: ign3types.Resource{Source: &testfiledata}, Mode: &mode}}
 	testIgn3Config.Storage.Files = append(testIgn3Config.Storage.Files, tempFile)
+
+	overrideName := "override.conf"
+	testIgn3Config.Systemd.Units = []ign3types.Unit{
+		{
+			Name:     "new-svc.service",
+			Enabled:  helpers.BoolToPtr(true),
+			Contents: getSystemdUnitContents("New Service"),
+		},
+		{
+			Name:     "new-svc-with-dropin.service",
+			Enabled:  helpers.BoolToPtr(true),
+			Contents: getSystemdUnitContents("New Service With Dropin"),
+			Dropins: []ign3types.Dropin{
+				{
+					Name:     overrideName,
+					Contents: helpers.StrToPtr("[Unit]\nDescription=Dropin Override"),
+				},
+			},
+		},
+		{
+			Name:    "new-empty-svc.service",
+			Enabled: helpers.BoolToPtr(true),
+		},
+		{
+			Name:    "new-empty-svc-with-empty-dropin.service",
+			Enabled: helpers.BoolToPtr(true),
+			Dropins: []ign3types.Dropin{
+				{
+					Name: overrideName,
+				},
+			},
+		},
+		{
+			Name:    "new-empty-svc-with-dropin.service",
+			Enabled: helpers.BoolToPtr(true),
+			Dropins: []ign3types.Dropin{
+				{
+					Name:     overrideName,
+					Contents: getSystemdUnitContents("Empty Service With Dropin Override"),
+				},
+			},
+		},
+		{
+			Name:    "new-empty-disabled-svc.service",
+			Enabled: helpers.BoolToPtr(false),
+		},
+		{
+			Name:     "new-disabled-svc.service",
+			Enabled:  helpers.BoolToPtr(false),
+			Contents: getSystemdUnitContents("New Disabled Service"),
+		},
+	}
+
 	rawIgnConfig := helpers.MarshalOrDie(testIgn3Config)
 	mcadd.Spec.Config.Raw = rawIgnConfig
 
@@ -682,6 +735,46 @@ func TestIgn3Cfg(t *testing.T) {
 	}
 	t.Logf("Node %s has file", infraNode.Name)
 
+	systemdUnitAssertions := map[string]func(*testing.T, string){
+		"new-svc.service": func(t *testing.T, name string) {
+			assertSystemdUnitFileExists(t, cs, infraNode, name)
+			assertSystemdUnitIsEnabled(t, cs, infraNode, name)
+		},
+		"new-svc-with-dropin.service": func(t *testing.T, name string) {
+			assertSystemdUnitFileExists(t, cs, infraNode, name)
+			assertSystemdUnitIsEnabled(t, cs, infraNode, name)
+			assertSystemdUnitDropinFileExists(t, cs, infraNode, name, overrideName)
+			assertSystemdUnitHasDropins(t, cs, infraNode, name, []string{overrideName})
+		},
+		"new-empty-svc.service": func(t *testing.T, name string) {
+			assertSystemdUnitFileDoesNotExist(t, cs, infraNode, name)
+			assertSystemdUnitDoesNotExist(t, cs, infraNode, name)
+		},
+		"new-empty-svc-with-empty-dropin.service": func(t *testing.T, name string) {
+			assertSystemdUnitDoesNotExist(t, cs, infraNode, name)
+			assertSystemdUnitFileDoesNotExist(t, cs, infraNode, name)
+			assertSystemdUnitDropinFileDoesNotExist(t, cs, infraNode, name, overrideName)
+		},
+		"new-empty-svc-with-dropin.service": func(t *testing.T, name string) {
+			assertSystemdUnitFileDoesNotExist(t, cs, infraNode, name)
+			assertSystemdUnitDropinFileExists(t, cs, infraNode, name, overrideName)
+		},
+		"new-empty-disabled-svc.service": func(t *testing.T, name string) {
+			assertSystemdUnitDoesNotExist(t, cs, infraNode, name)
+			assertSystemdUnitFileDoesNotExist(t, cs, infraNode, name)
+		},
+		"new-disabled-svc.service": func(t *testing.T, name string) {
+			assertSystemdUnitFileExists(t, cs, infraNode, name)
+			assertSystemdUnitExists(t, cs, infraNode, name)
+			assertSystemdUnitIsDisabled(t, cs, infraNode, name)
+		},
+	}
+
+	for name, assertionFunc := range systemdUnitAssertions {
+		t.Logf("Running assertion(s) for %s", name)
+		assertionFunc(t, name)
+	}
+
 	unlabelFunc()
 
 	workerMCP, err := cs.MachineConfigPools().Get(context.TODO(), "worker", metav1.GetOptions{})

test/e2e-2of2/systemdhelpers_test.go

@@ -0,0 +1,131 @@
+package e2e_test
+
+import (
+	"fmt"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	corev1 "k8s.io/api/core/v1"
+
+	"github.com/openshift/machine-config-operator/test/framework"
+	"github.com/openshift/machine-config-operator/test/helpers"
+)
+
+// Creates a dummy systemd unit file with a customizable description.
+func getSystemdUnitContents(desc string) *string {
+	contents := []string{
+		"[Unit]",
+		fmt.Sprintf("Description=%s", desc),
+		"",
+		"[Service]",
+		`ExecStart=/bin/bash -c "sleep infinity"`,
+		"Restart=on-failure",
+		"",
+		"[Install]",
+		"WantedBy=multi-user.target",
+	}
+
+	return helpers.StrToPtr(strings.Join(contents, "\n"))
+}
+
+// Asserts that a given systemd unit is enabled.
+func assertSystemdUnitIsEnabled(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+
+	output := showSystemdUnit(t, cs, node, unitName)
+	assert.Contains(t, output, "UnitFileState=enabled")
+	assert.Contains(t, output, "ActiveState=active")
+	assert.Contains(t, output, "LoadState=loaded")
+}
+
+// Asserts that a given systemd unit is disabled.
+func assertSystemdUnitIsDisabled(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+
+	output := showSystemdUnit(t, cs, node, unitName)
+	assert.Contains(t, output, "UnitFileState=disabled")
+	assert.Contains(t, output, "ActiveState=inactive")
+	assert.Contains(t, output, "LoadState=loaded")
+}
+
+// Asserts that a given systemd unit has the specified dropins.
+func assertSystemdUnitHasDropins(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string, dropins []string) {
+	t.Helper()
+
+	output := showSystemdUnit(t, cs, node, unitName)
+	dropinPaths := getSystemdLineFromOutput(output, "DropInPaths")
+	for _, dropin := range dropins {
+		assert.Contains(t, dropinPaths, dropin)
+	}
+}
+
+const (
+	// The root path for where systemd stores its units, dropins, etc.
+	systemdRootPath string = "/etc/systemd/system"
+)
+
+// Asserts that a dropin file exists for a given systemd unit.
+func assertSystemdUnitDropinFileExists(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName, dropinName string) {
+	t.Helper()
+	helpers.AssertFileOnNode(t, cs, node, filepath.Join(systemdRootPath, unitName+".d", dropinName))
+}
+
+// Asserts tha a dropin file does not exist for a given systemd unit.
+func assertSystemdUnitDropinFileDoesNotExist(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName, dropinName string) {
+	t.Helper()
+	helpers.AssertFileNotOnNode(t, cs, node, filepath.Join(systemdRootPath, unitName+".d", dropinName))
+}
+
+// Asserts that the unit file for a given systemd unit exists.
+func assertSystemdUnitFileExists(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+	helpers.AssertFileOnNode(t, cs, node, filepath.Join(systemdRootPath, unitName))
+}
+
+// Asserts that the given systemd unit exists by querying systemd.
+func assertSystemdUnitExists(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+	output := showSystemdUnit(t, cs, node, unitName)
+	loadState := getSystemdLineFromOutput(output, "LoadState")
+	assert.Equal(t, loadState, "LoadState=loaded")
+}
+
+// Asserts that the given systemd unit does not exist by querying systemd.
+func assertSystemdUnitDoesNotExist(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+
+	output := showSystemdUnit(t, cs, node, unitName)
+
+	loadState := getSystemdLineFromOutput(output, "LoadState")
+	assert.Equal(t, "LoadState=not-found", loadState)
+
+	loadError := getSystemdLineFromOutput(output, "LoadError")
+	assert.Contains(t, loadError, "NoSuchUnit")
+}
+
+// Asserts that the given systemd unit file does not exist.
+func assertSystemdUnitFileDoesNotExist(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) {
+	t.Helper()
+	helpers.AssertFileNotOnNode(t, cs, node, filepath.Join("/rootfs", systemdRootPath, unitName))
+}
+
+// Runs "systemctl show <unit>" on a given node. Note: This command should
+// always returns zero even when the unit does not exist. Assertions are done
+// by interrogating its output.
+func showSystemdUnit(t *testing.T, cs *framework.ClientSet, node corev1.Node, unitName string) string {
+	return helpers.ExecCmdOnNode(t, cs, node, "chroot", "/rootfs", "systemctl", "show", unitName)
+}
+
+// TODO: This does not handle the case where a key may appear multiple times;
+// e.g., multiple ExecStart lines.
+func getSystemdLineFromOutput(output, prefix string) string {
+	for _, line := range strings.Split(output, "\n") {
+		if strings.HasPrefix(line, prefix) {
+			return line
+		}
+	}
+
+	return ""
+}