Merge pull request #5086 from cheesesashimi/zzlotnik/OCPBUGS-56648

OCPBUGS-56648: fixes systemd unit creation for empty units

Author: openshift-merge-bot[bot]

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

@@ -2124,10 +2124,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-1of2/systemdhelpers_test.go

@@ -0,0 +1,131 @@
+package e2e_1of2_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 ""
+}