Merge pull request #5213 from stbenjam/revert-5148-1754043225154

TRT-2232: Revert #5148 "OCPBUGS-58023: Prevent unnecessary systemd unit disable"

Author: openshift-merge-bot[bot]

pkg/controller/common/helpers.go

@@ -926,7 +926,7 @@ func CalculateConfigFileDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) []st
 // that are different between them
 //
 //nolint:dupl
-func CalculateConfigUnitDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) ([]string, []ign3types.Unit) {
+func CalculateConfigUnitDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) []string {
 	// Go through the units and see what is new or different
 	oldUnitSet := make(map[string]ign3types.Unit)
 	for _, u := range oldIgnConfig.Systemd.Units {
@@ -946,19 +946,16 @@ func CalculateConfigUnitDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) ([]s
 		}
 	}
 
-	addedOrChangedUnits := []ign3types.Unit{}
 	// Now check if any units were added/changed
 	for name, newUnit := range newUnitSet {
 		oldUnit, ok := oldUnitSet[name]
 		if !ok {
 			diffUnitSet = append(diffUnitSet, name)
-			addedOrChangedUnits = append(addedOrChangedUnits, newUnit)
 		} else if !reflect.DeepEqual(oldUnit, newUnit) {
 			diffUnitSet = append(diffUnitSet, name)
-			addedOrChangedUnits = append(addedOrChangedUnits, newUnit)
 		}
 	}
-	return diffUnitSet, addedOrChangedUnits
+	return diffUnitSet
 }
 
 // NewIgnFile returns a simple ignition3 file from just path and file contents.

pkg/daemon/on_disk_validation.go

@@ -4,9 +4,7 @@ import (
 	"bytes"
 	"fmt"
 	"os"
-	"os/exec"
 	"path/filepath"
-	"strings"
 
 	ign2types "github.com/coreos/ignition/config/v2_2/types"
 	ign3types "github.com/coreos/ignition/v2/config/v3_5/types"
@@ -103,12 +101,7 @@ func checkV3Unit(unit ign3types.Unit, systemdPath string) error {
 		return nil
 	}
 
-	err := checkFileContentsAndMode(path, []byte(*unit.Contents), defaultFilePermissions)
-	if err != nil {
-		return err
-	}
-
-	return checkUnitEnabled(unit.Name, unit.Enabled)
+	return checkFileContentsAndMode(path, []byte(*unit.Contents), defaultFilePermissions)
 }
 
 // checkV3Units validates the contents of all the units in the
@@ -238,24 +231,6 @@ func checkV2Files(files []ign2types.File) error {
 	return nil
 }
 
-// checkUnitEnabled checks whether a systemd unit is enabled as expected.
-func checkUnitEnabled(name string, expectedEnabled *bool) error {
-	if expectedEnabled == nil {
-		return nil
-	}
-	cmd := exec.Command("systemctl", "is-enabled", name)
-	outBytes, err := cmd.CombinedOutput()
-	out := strings.TrimSpace(string(outBytes))
-
-	// units types considered not enabled (linked, linked-runtime, masked, masked-runtime, disabled, bad)
-	isEnabled := err == nil
-	if isEnabled != *expectedEnabled {
-		return fmt.Errorf("unit %q expected enabled=%t, but systemd reports %q", name, *expectedEnabled, out)
-	}
-
-	return nil
-}
-
 // checkFileContentsAndMode reads the file from the filepath and compares its
 // contents and mode with the expectedContent and mode parameters. It logs an
 // error in case of an error or mismatch and returns the status of the

pkg/daemon/update.go

@@ -1005,7 +1005,7 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig, skipCertifi
 	logSystem("Starting update from %s to %s: %+v", oldConfigName, newConfigName, diff)
 
 	diffFileSet := ctrlcommon.CalculateConfigFileDiffs(&oldIgnConfig, &newIgnConfig)
-	diffUnitSet, addedOrChangedUnits := ctrlcommon.CalculateConfigUnitDiffs(&oldIgnConfig, &newIgnConfig)
+	diffUnitSet := ctrlcommon.CalculateConfigUnitDiffs(&oldIgnConfig, &newIgnConfig)
 
 	var nodeDisruptionActions []opv1.NodeDisruptionPolicyStatusAction
 	var actions []string
@@ -1117,13 +1117,13 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig, skipCertifi
 	}
 
 	// update files on disk that need updating
-	if err := dn.updateFiles(oldIgnConfig, newIgnConfig, addedOrChangedUnits, skipCertificateWrite); err != nil {
+	if err := dn.updateFiles(oldIgnConfig, newIgnConfig, skipCertificateWrite); err != nil {
 		return err
 	}
 
 	defer func() {
 		if retErr != nil {
-			if err := dn.updateFiles(newIgnConfig, oldIgnConfig, addedOrChangedUnits, skipCertificateWrite); err != nil {
+			if err := dn.updateFiles(newIgnConfig, oldIgnConfig, skipCertificateWrite); err != nil {
 				errs := kubeErrs.NewAggregate([]error{err, retErr})
 				retErr = fmt.Errorf("error rolling back files writes: %w", errs)
 				return
@@ -1258,17 +1258,15 @@ func (dn *Daemon) updateHypershift(oldConfig, newConfig *mcfgv1.MachineConfig, d
 		return fmt.Errorf("parsing new Ignition config failed: %w", err)
 	}
 
-	_, addedOrChangedUnits := ctrlcommon.CalculateConfigUnitDiffs(&oldIgnConfig, &newIgnConfig)
-
 	// update files on disk that need updating
 	// We should't skip the certificate write in HyperShift since it does not run the extra daemon process
-	if err := dn.updateFiles(oldIgnConfig, newIgnConfig, addedOrChangedUnits, false); err != nil {
+	if err := dn.updateFiles(oldIgnConfig, newIgnConfig, false); err != nil {
 		return err
 	}
 
 	defer func() {
 		if retErr != nil {
-			if err := dn.updateFiles(newIgnConfig, oldIgnConfig, addedOrChangedUnits, false); err != nil {
+			if err := dn.updateFiles(newIgnConfig, oldIgnConfig, false); err != nil {
 				errs := kubeErrs.NewAggregate([]error{err, retErr})
 				retErr = fmt.Errorf("error rolling back files writes: %w", errs)
 				return
@@ -1773,12 +1771,12 @@ func (dn *CoreOSDaemon) switchKernel(oldConfig, newConfig *mcfgv1.MachineConfig)
 // whatever has been written is picked up by the appropriate daemons, if
 // required. in particular, a daemon-reload and restart for any unit files
 // touched.
-func (dn *Daemon) updateFiles(oldIgnConfig, newIgnConfig ign3types.Config, addedOrChangedUnits []ign3types.Unit, skipCertificateWrite bool) error {
+func (dn *Daemon) updateFiles(oldIgnConfig, newIgnConfig ign3types.Config, skipCertificateWrite bool) error {
 	klog.Info("Updating files")
 	if err := dn.writeFiles(newIgnConfig.Storage.Files, skipCertificateWrite); err != nil {
 		return err
 	}
-	if err := dn.writeUnits(addedOrChangedUnits); err != nil {
+	if err := dn.writeUnits(newIgnConfig.Systemd.Units); err != nil {
 		return err
 	}
 	return dn.deleteStaleData(oldIgnConfig, newIgnConfig)