diff --git a/pkg/daemon/file_writers.go b/pkg/daemon/file_writers.go index f955277dc9..8df8f44089 100644 --- a/pkg/daemon/file_writers.go +++ b/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 { diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index e9d0546ebd..75dd01c73d 100644 --- a/pkg/daemon/update.go +++ b/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 { diff --git a/test/e2e-1of2/mcd_test.go b/test/e2e-1of2/mcd_test.go index c762f9793c..37874dd792 100644 --- a/test/e2e-1of2/mcd_test.go +++ b/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{}) diff --git a/test/e2e-1of2/systemdhelpers_test.go b/test/e2e-1of2/systemdhelpers_test.go new file mode 100644 index 0000000000..8b6cf24459 --- /dev/null +++ b/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 " 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 "" +}