we cannot rely on the systemd unit being present during firstboot

pkg/daemon/daemon.go

@@ -1292,7 +1292,7 @@ func (dn *Daemon) RunFirstbootCompleteMachineconfig() error {
 		return fmt.Errorf("failed to rename encapsulated MachineConfig after processing on firstboot: %w", err)
 	}
 
-	if err := dn.toggleRevertSystemdUnit(&mc, false); err != nil {
+	if err := dn.disableRevertSystemdUnit(); err != nil {
 		return err
 	}
 

pkg/daemon/update.go

@@ -74,7 +74,8 @@ const (
 	ImageRegistryDrainOverrideConfigmap = "image-registry-override-drain"
 
 	// name of the systemd unit that re-bootstraps a node after reverting from layered to non-layered
-	layeringRevertSystemdUnitName = "machine-config-daemon-revert.service"
+	layeringRevertSystemdUnitName       = "machine-config-daemon-revert.service"
+	clonedLayeringRevertSystemdUnitName = "machine-config-daemon-revert-layered.service"
 )
 
 func getNodeRef(node *corev1.Node) *corev1.ObjectReference {
@@ -1007,7 +1008,7 @@ func (dn *Daemon) finalizeRevertToNonLayering(newConfig *mcfgv1.MachineConfig) e
 
 	klog.Infof("Wrote MachineConfig %q to %q", newConfig.Name, constants.MachineConfigEncapsulatedPath)
 
-	if err := dn.toggleRevertSystemdUnit(newConfig, true); err != nil {
+	if err := dn.enableRevertSystemdUnit(newConfig); err != nil {
 		return err
 	}
 
@@ -2900,69 +2901,62 @@ func (dn *Daemon) hasImageRegistryDrainOverrideConfigMap() (bool, error) {
 	return false, fmt.Errorf("Error fetching image registry drain override configmap: %w", err)
 }
 
-// Enables or disables the revert layering systemd unit.
-// This unit is a copy of the one provided by the MachineConfig so that it will
-// persist after rebooting.
+// Enables the revert layering systemd unit.
+//
+// This requires the current MachineConfig because it has the original unit
+// that we clone in place.
+//
 // To enable the unit, it performs the following operations:
-// 1. It gets the disabled systemd unit, renames it, and enables it.
-// 2. It writes the new systemd unit to disk.
+// 1. It gets the disabled systemd unit, renames it, enables it, and unmasks it.
+// 2. It writes the new systemd unit to disk using the MCD helpers.
 // 3. It reloads the systemd daemon.
-//
-// To disable the unit, it does the following:
-// 1. Sets the unit to be disabled.
-// 2. Writes the disabled unit to disk.
-// 3. Reloads the systemd daemon.
-// 4. Removes the new unit file.
-func (dn *Daemon) toggleRevertSystemdUnit(mc *mcfgv1.MachineConfig, enabled bool) error {
-	clonedUnit, err := getRevertSystemdUnitFromMachineConfig(mc)
+func (dn *Daemon) enableRevertSystemdUnit(mc *mcfgv1.MachineConfig) error {
+	clonedUnit, err := getRevertRevertSystemdUnitFromMachineConfig(mc)
 	if err != nil {
 		return err
 	}
 
-	unitFilePath := filepath.Join(pathSystemd, clonedUnit.Name)
+	clonedUnit.Name = clonedLayeringRevertSystemdUnitName
+	clonedUnit.Enabled = helpers.BoolToPtr(true)
+	clonedUnit.Mask = nil
 
-	_, err = os.Stat(unitFilePath)
-	if err != nil && !errors.Is(err, os.ErrNotExist) {
+	if err := dn.writeUnits([]ign3types.Unit{*clonedUnit}); err != nil {
 		return err
 	}
 
-	// If os.Stat() returns nil, it means the file exists. If it returns
-	// os.ErrNotExist, it means that the file does not exist. If it returns any
-	// other error, something else is wrong and we can't determine whether the
-	// file exists or not.
-	unitFileExists := err == nil
-
-	// If the unit file does not exist and the systemd unit should not be
-	// enabled, we should no-op.
-	if !unitFileExists && !enabled {
-		return nil
-	}
-
-	// If the unit file does exists and the systemd unit should be enabled,
-	// then we should no-op here.
-	if unitFileExists && enabled {
-		return nil
+	if err := reloadDaemon(); err != nil {
+		return err
 	}
 
-	clonedUnit.Enabled = helpers.BoolToPtr(enabled)
-	clonedUnit.Mask = nil
+	return nil
+}
 
-	if err := dn.writeUnits([]ign3types.Unit{*clonedUnit}); err != nil {
+// Disables the revert layering systemd unit.
+//
+// In the interest of thoroughness, the way that this works is it will create
+// its own disabled Ignition unit and write it to disk using the MCD's paths
+// for doing so. This will ensure that all symlinks are cleaned up and that
+// nothing is left dangling.
+func (dn *Daemon) disableRevertSystemdUnit() error {
+	unit := ign3types.Unit{
+		Name:     clonedLayeringRevertSystemdUnitName,
+		Mask:     nil,
+		Enabled:  helpers.BoolToPtr(false),
+		Contents: nil,
+	}
+
+	if err := dn.writeUnits([]ign3types.Unit{unit}); err != nil {
 		return err
 	}
 
 	if err := reloadDaemon(); err != nil {
 		return err
 	}
 
-	if !enabled {
-		return os.RemoveAll(unitFilePath)
-	}
-
-	return nil
+	return os.RemoveAll(filepath.Join(pathSystemd, clonedLayeringRevertSystemdUnitName))
 }
 
-func getRevertSystemdUnitFromMachineConfig(mc *mcfgv1.MachineConfig) (*ign3types.Unit, error) {
+func getRevertRevertSystemdUnitFromMachineConfig(mc *mcfgv1.MachineConfig) (*ign3types.Unit, error) {
 	ignConfig, err := ctrlcommon.ParseAndConvertConfig(mc.Spec.Config.Raw)
 	if err != nil {
 		return nil, err
@@ -2971,11 +2965,9 @@ func getRevertSystemdUnitFromMachineConfig(mc *mcfgv1.MachineConfig) (*ign3types
 	for _, unit := range ignConfig.Systemd.Units {
 		unit := unit
 		if unit.Name == layeringRevertSystemdUnitName {
-			unit.Name = "machine-config-daemon-revert-layered.service"
-			unit.Mask = nil
 			return &unit, nil
 		}
 	}
 
-	return nil, fmt.Errorf("could not find %s in MachineConfig %s", layeringRevertSystemdUnitName, mc.Name)
+	return nil, fmt.Errorf("machineconfig %s does not contain systemd unit %s", mc.Name, layeringRevertSystemdUnitName)
 }