Merge pull request #5576 from neisw/revert-5527-ocpbugs-58023

Revert "OCPBUGS-58023: Prevent unnecessary systemd unit disable"

Author: openshift-merge-bot[bot]

pkg/controller/common/helpers.go

@@ -945,25 +945,12 @@ func CalculateConfigFileDiffs(oldIgnConfig, newIgnConfig *ign3types.Config) []st
 	return diffFileSet
 }
 
-type UnitDiff struct {
-	Added   []ign3types.Unit
-	Removed []ign3types.Unit
-	Updated []ign3types.Unit
-}
-
-// GetChangedConfigUnitsByType compares the units present in two ignition configurations, one
-// old config and the other new, a struct with units mapped to the type of change:
-//   - New unit added: "Added"
-//   - Unit previously existing removed: "Removed"
-//   - Existing unit changed in some way: "Updated"
-func GetChangedConfigUnitsByType(oldIgnConfig, newIgnConfig *ign3types.Config) (unitDiffs UnitDiff) {
-	diffUnit := UnitDiff{
-		Added:   []ign3types.Unit{},
-		Removed: []ign3types.Unit{},
-		Updated: []ign3types.Unit{},
-	}
-
-	// Get the sets of the old and new units from the ignition configurations
+// CalculateConfigUnitDiffs compares the units present in two ignition configurations and returns the list of units
+// that are different between them
+//
+//nolint:dupl
+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 {
 		oldUnitSet[u.Name] = u
@@ -972,25 +959,26 @@ func GetChangedConfigUnitsByType(oldIgnConfig, newIgnConfig *ign3types.Config) (
 	for _, u := range newIgnConfig.Systemd.Units {
 		newUnitSet[u.Name] = u
 	}
+	diffUnitSet := []string{}
 
 	// First check if any units were removed
 	for unit := range oldUnitSet {
 		_, ok := newUnitSet[unit]
 		if !ok {
-			diffUnit.Removed = append(diffUnit.Removed, oldUnitSet[unit])
+			diffUnitSet = append(diffUnitSet, unit)
 		}
 	}
 
-	// Now check if any units were added or updated
+	// Now check if any units were added/changed
 	for name, newUnit := range newUnitSet {
 		oldUnit, ok := oldUnitSet[name]
 		if !ok {
-			diffUnit.Added = append(diffUnit.Added, newUnitSet[name])
+			diffUnitSet = append(diffUnitSet, name)
 		} else if !reflect.DeepEqual(oldUnit, newUnit) {
-			diffUnit.Updated = append(diffUnit.Updated, newUnitSet[name])
+			diffUnitSet = append(diffUnitSet, name)
 		}
 	}
-	return diffUnit
+	return diffUnitSet
 }
 
 // NewIgnFile returns a simple ignition3 file from just path and file contents.

pkg/daemon/on_disk_validation.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"os"
 	"path/filepath"
-	"strings"
 
 	ign2types "github.com/coreos/ignition/config/v2_2/types"
 	ign3types "github.com/coreos/ignition/v2/config/v3_5/types"
@@ -102,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
@@ -237,29 +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
-	}
-	outBytes, _ := runGetOut("systemctl", "is-enabled", name)
-	out := strings.TrimSpace(string(outBytes))
-
-	switch {
-	case *expectedEnabled:
-		// If expected to be enabled, reject known "not enabled" states
-		if out == "disabled" || out == "masked" || out == "masked-runtime" || out == "not-found" {
-			return fmt.Errorf("unit %q expected enabled=true, but systemd reports %q", name, out)
-		}
-	case !*expectedEnabled:
-		// If expected to be disabled, reject any of the enabled-like states
-		if out == "enabled" || out == "enabled-runtime" {
-			return fmt.Errorf("unit %q expected enabled=false, but systemd reports %q", name, 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

@@ -12,7 +12,6 @@ import (
 	"path/filepath"
 	"reflect"
 	goruntime "runtime"
-	"slices"
 	"strconv"
 	"strings"
 	"sync"
@@ -1014,20 +1013,13 @@ 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)
-	// Get the added and updated units
-	unitDiff := ctrlcommon.GetChangedConfigUnitsByType(&oldIgnConfig, &newIgnConfig)
-	addedOrChangedUnits := slices.Concat(unitDiff.Added, unitDiff.Updated)
-	// Get the names of all units changed in some way (added, removed, or updated)
-	var allChangedUnitNames []string
-	for _, unit := range append(addedOrChangedUnits, unitDiff.Removed...) {
-		allChangedUnitNames = append(allChangedUnitNames, unit.Name)
-	}
+	diffUnitSet := ctrlcommon.CalculateConfigUnitDiffs(&oldIgnConfig, &newIgnConfig)
 
 	var nodeDisruptionActions []opv1.NodeDisruptionPolicyStatusAction
 	var actions []string
 	// Node Disruption Policies cannot be used during firstboot as API is not accessible.
 	if !firstBoot {
-		nodeDisruptionActions, err = dn.calculatePostConfigChangeNodeDisruptionAction(diff, diffFileSet, allChangedUnitNames)
+		nodeDisruptionActions, err = dn.calculatePostConfigChangeNodeDisruptionAction(diff, diffFileSet, diffUnitSet)
 	} else {
 		actions, err = calculatePostConfigChangeAction(diff, diffFileSet)
 		klog.Infof("Skipping node disruption polciies as node is executing first boot.")
@@ -1133,13 +1125,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
@@ -1274,18 +1266,15 @@ func (dn *Daemon) updateHypershift(oldConfig, newConfig *mcfgv1.MachineConfig, d
 		return fmt.Errorf("parsing new Ignition config failed: %w", err)
 	}
 
-	unitDiff := ctrlcommon.GetChangedConfigUnitsByType(&oldIgnConfig, &newIgnConfig)
-	addedOrChangedUnits := slices.Concat(unitDiff.Added, unitDiff.Updated)
-
 	// 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
@@ -1810,12 +1799,12 @@ func (dn *CoreOSDaemon) getKernelPackagesForTargetRelease() (releaseKernelPackag
 // 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)