allow layered nodes to revert to non-layered

This adds code that reverts from a layered MachineConfigPool to a non-layered MachineConfigPool.

Why this was so troublesome is:
- When a MachineConfig is written to the node, it is placed in the portions of the filesystem that are mutable according to ostree.
- When a container image containing those MachineConfigs is written onto the node using rpm-ostree, it technically overwrites those preexisting MachineConfigs. In doing so, the container is now claiming (for lack of a better term) ownership of those files.
- The "factory" OS image does not contain these MachineConfigs.
- So when we roll back from the customized image to the "factory" image, because the MachineConfig files on disk are now owned by the customized container, they are removed when the factory OS image is rebased.

If an ad-hoc file is written to a mutable part of the filesystem after the container has been applied, provided that the container does not claim ownership of a file with the same name, the ad-hoc file will persist after a reboot. To take full advantage of this fact, this does the following:

1. Introduces a `machine-config-daemon-revert.service` systemd service, which is disabled by default. The contents of this are similar to the `machine-config-daemon-firstboot.service`, with the exception being that it is required by a default system target.
2. In the event that a revert is detected, this file is cloned to a different service name in the systemd root (`/etc/systemd/system`).
3. The systemd service is then enabled, the new MachineConfig is written to disk under `/etc/ignition-machine-config-encapsulated.json`.
4. The node reboots.
5. During the bootup, the new service detects the presence of `/etc/ignition-machine-config-encapsulated.json` and runs the MCD in bootstrap mode to rewrite all of the configs to disk. This (unfortunately) includes a second node reboot.
6. Following the second node reboot, the node should be in the reverted configuration.

pkg/daemon/daemon.go

@@ -1292,6 +1292,13 @@ func (dn *Daemon) RunFirstbootCompleteMachineconfig() error {
 		return fmt.Errorf("failed to rename encapsulated MachineConfig after processing on firstboot: %w", err)
 	}
 
+	// If we re-bootstrapped the node, we should disable and remove the systemd
+	// unit that we used to do that. This function will no-op and return nil if
+	// the systemd unit is not present.
+	if err := dn.disableRevertSystemdUnit(); err != nil {
+		return fmt.Errorf("cannot disable revert systemd unit %s: %w", clonedLayeringRevertSystemdUnitName, err)
+	}
+
 	dn.skipReboot = false
 	return dn.reboot(fmt.Sprintf("Completing firstboot provisioning to %s", mc.GetName()))
 }
@@ -2456,16 +2463,6 @@ func (dn *Daemon) triggerUpdate(currentConfig, desiredConfig *mcfgv1.MachineConf
 		return dn.triggerUpdateWithMachineConfig(currentConfig, desiredConfig, true)
 	}
 
-	// If the desired image annotation is empty, but the current image is not
-	// empty, this should be a regular MachineConfig update.
-	//
-	// However, the node will not roll back from a layered config to a
-	// non-layered config without admin intervention; so we should emit an error
-	// for now.
-	if desiredImage == "" && currentImage != "" {
-		return fmt.Errorf("rolling back from a layered to non-layered configuration is not currently supported")
-	}
-
 	// Shut down the Config Drift Monitor since we'll be performing an update
 	// and the config will "drift" while the update is occurring.
 	dn.stopConfigDriftMonitor()

pkg/daemon/update.go

@@ -41,6 +41,7 @@ import (
 	pivottypes "github.com/openshift/machine-config-operator/pkg/daemon/pivot/types"
 	pivotutils "github.com/openshift/machine-config-operator/pkg/daemon/pivot/utils"
 	"github.com/openshift/machine-config-operator/pkg/upgrademonitor"
+	"github.com/openshift/machine-config-operator/test/helpers"
 )
 
 const (
@@ -71,6 +72,10 @@ const (
 	// ImageRegistryDrainOverrideConfigmap is the name of the Configmap a user can apply to force all
 	// image registry changes to not drain
 	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"
+	clonedLayeringRevertSystemdUnitName = "machine-config-daemon-revert-layered.service"
 )
 
 func getNodeRef(node *corev1.Node) *corev1.ObjectReference {
@@ -880,14 +885,27 @@ func (dn *Daemon) updateOnClusterBuild(oldConfig, newConfig *mcfgv1.MachineConfi
 	// If the new image pullspec is already on disk, do not attempt to re-apply
 	// it. rpm-ostree will throw an error as a result.
 	// See: https://issues.redhat.com/browse/OCPBUGS-18414.
-	if oldImage != newImage && newImage != "" {
+	if oldImage != newImage {
+		// If the new image field is empty, set it to the OS image URL value
+		// provided by the MachineConfig to do a rollback.
+		if newImage == "" {
+			klog.Infof("%s empty, reverting to osImageURL %s from MachineConfig %s", constants.DesiredImageAnnotationKey, newConfig.Spec.OSImageURL, newConfig.Name)
+			newImage = newConfig.Spec.OSImageURL
+		}
 		if err := dn.updateLayeredOSToPullspec(newImage); err != nil {
 			return err
 		}
 	} else {
 		klog.Infof("Image pullspecs equal, skipping rpm-ostree rebase")
 	}
 
+	// If the new OS image equals the OS image URL value, this means we're in a
+	// revert-from-layering situation. This also means we can return early after
+	// taking a different path.
+	if newImage == newConfig.Spec.OSImageURL {
+		return dn.finalizeRevertToNonLayering(newConfig)
+	}
+
 	// update files on disk that need updating
 	if err := dn.updateFiles(oldIgnConfig, newIgnConfig, skipCertificateWrite); err != nil {
 		return err
@@ -975,6 +993,60 @@ func (dn *Daemon) updateOnClusterBuild(oldConfig, newConfig *mcfgv1.MachineConfi
 	return dn.reboot(fmt.Sprintf("Node will reboot into image %s / MachineConfig %s", newImage, newConfigName))
 }
 
+// Finalizes the revert process by enabling a special systemd unit prior to
+// rebooting the node.
+//
+// After we write the original factory image to the node, none of the files
+// specified in the MachineConfig will be written due to how rpm-ostree handles
+// file writes. Because those files are owned by the layered container image,
+// they are not present after reboot; even if we were to write them to the node
+// before rebooting. Consequently, after reverting back to the original image,
+// the node will lose contact with the control plane and the easiest way to
+// reestablish contact is to rebootstrap the node.
+//
+// By comparison, if we write a file that is _not_ owned by the layered
+// container image, this file will persist after reboot. So what we do is write
+// a special systemd unit that will rebootstrap the node upon reboot.
+// Unfortunately, this will incur a second reboot during the rollback process,
+// so there is room for improvement here.
+func (dn *Daemon) finalizeRevertToNonLayering(newConfig *mcfgv1.MachineConfig) error {
+	// First, we write the MachineConfig-encapsulated Ignition file. This is both
+	// the signal that the revert systemd unit should fire as well as the desired
+	// source of truth.
+	outBytes, err := json.Marshal(newConfig)
+	if err != nil {
+		return fmt.Errorf("could not marshal MachineConfig %q to JSON: %w", newConfig.Name, err)
+	}
+
+	if err := writeFileAtomicallyWithDefaults(constants.MachineConfigEncapsulatedPath, outBytes); err != nil {
+		return fmt.Errorf("could not write MachineConfig %q to %q: %w", newConfig.Name, constants.MachineConfigEncapsulatedPath, err)
+	}
+
+	klog.Infof("Wrote MachineConfig %q to %q", newConfig.Name, constants.MachineConfigEncapsulatedPath)
+
+	// Next, we enable the revert systemd unit. This takes the current
+	// MachineConfig, searches for the machine-config-daemon-revert.service
+	// systemd unit, clones it, and writes it to disk. The reason for doing it
+	// this way is because it will persist after the reboot since it was not
+	// written or mutated by the rpm-ostree process.
+	if err := dn.enableRevertSystemdUnit(newConfig); err != nil {
+		return err
+	}
+
+	// Clear the current image field so that after reboot, the node will clear
+	// the currentImage annotation.
+	odc := &onDiskConfig{
+		currentImage:  "",
+		currentConfig: newConfig,
+	}
+
+	if err := dn.storeCurrentConfigOnDisk(odc); err != nil {
+		return err
+	}
+
+	return dn.reboot(fmt.Sprintf("Node will reboot into image %s / MachineConfig %s", newConfig.Spec.OSImageURL, newConfig.Name))
+}
+
 // Update the node to the provided node configuration.
 // This function should be de-duped with dn.updateHypershift() and
 // dn.updateOnClusterBuild(). See: https://issues.redhat.com/browse/MCO-810 for
@@ -2848,3 +2920,82 @@ func (dn *Daemon) hasImageRegistryDrainOverrideConfigMap() (bool, error) {
 
 	return false, fmt.Errorf("Error fetching image registry drain override configmap: %w", err)
 }
+
+// Enables the revert layering systemd unit.
+//
+// This requires the current MachineConfig because it has the original unit
+// that we use to create the cloned unit.
+//
+// To enable the unit, it performs the following operations:
+// 1. It gets the disabled revert systemd unit from the MachineConfig, renames
+// it, enables it, and unmasks it.
+// 2. It writes the new systemd unit to disk using the default MCD code paths
+// for that process.
+func (dn *Daemon) enableRevertSystemdUnit(mc *mcfgv1.MachineConfig) error {
+	clonedUnit, err := getRevertSystemdUnitFromMachineConfig(mc)
+	if err != nil {
+		return err
+	}
+
+	clonedUnit.Name = clonedLayeringRevertSystemdUnitName
+	clonedUnit.Enabled = helpers.BoolToPtr(true)
+	clonedUnit.Mask = nil
+
+	if err := dn.writeUnits([]ign3types.Unit{*clonedUnit}); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+// Disables the revert layering systemd unit, if it is present.
+//
+// To disable the unit, it performs the following operations:
+// 1. Checks for the presence of the cloned systemd unit file. If not present,
+// it will no-op.
+// 2. If the unit file is present, it will disable the unit using the default
+// MCD code paths for that purpose.
+// 3. It will ensure that the unit file is removed.
+func (dn *Daemon) disableRevertSystemdUnit() error {
+	// First, check if the unit file is present.
+	unitPath := filepath.Join(pathSystemd, clonedLayeringRevertSystemdUnitName)
+	_, err := os.Lstat(unitPath)
+	if err != nil {
+		// If the unit file is not present, there is nothing to do.
+		if errors.Is(err, os.ErrNotExist) {
+			return nil
+		}
+
+		return err
+	}
+
+	// If we've reached this point, we know that the unit file is still present,
+	// which means the unit may still be enabled.
+	if err := dn.disableUnits([]string{clonedLayeringRevertSystemdUnitName}); err != nil {
+		return err
+	}
+
+	// systemd removes the unit file, but there is no harm in calling
+	// os.RemoveAll() since it will return nil if the file does not exist.
+	if err := os.RemoveAll(unitPath); err != nil {
+		return err
+	}
+
+	return nil
+}
+
+func getRevertSystemdUnitFromMachineConfig(mc *mcfgv1.MachineConfig) (*ign3types.Unit, error) {
+	ignConfig, err := ctrlcommon.ParseAndConvertConfig(mc.Spec.Config.Raw)
+	if err != nil {
+		return nil, err
+	}
+
+	for _, unit := range ignConfig.Systemd.Units {
+		unit := unit
+		if unit.Name == layeringRevertSystemdUnitName {
+			return &unit, nil
+		}
+	}
+
+	return nil, fmt.Errorf("machineconfig %s does not contain systemd unit %s", mc.Name, layeringRevertSystemdUnitName)
+}

templates/common/_base/units/machine-config-daemon-revert.service.yaml

@@ -0,0 +1,26 @@
+name: machine-config-daemon-revert.service
+mask: true
+enabled: false
+contents: |
+  [Unit]
+  Description=Machine Config Daemon Revert
+  # Make sure it runs only on OSTree booted system
+  ConditionPathExists=/run/ostree-booted
+  # Removal of this file signals firstboot completion
+  ConditionPathExists=/etc/ignition-machine-config-encapsulated.json
+  After=network.target
+
+  [Service]
+  Type=oneshot
+  RemainAfterExit=yes
+  # Disable existing repos (if any) so that OS extensions would use embedded RPMs only
+  ExecStartPre=-/usr/bin/sh -c "sed -i 's/enabled=1/enabled=0/' /etc/yum.repos.d/*.repo"
+  # Run this via podman because we want to use the nmstatectl binary in our container
+  ExecStart=/usr/bin/podman run --authfile=/var/lib/kubelet/config.json --rm --privileged --net=host -v /:/rootfs  --entrypoint machine-config-daemon '{{ .Images.machineConfigOperator }}' firstboot-complete-machineconfig --persist-nics
+  ExecStart=/usr/bin/podman run --authfile=/var/lib/kubelet/config.json --rm --privileged --pid=host --net=host -v /:/rootfs  --entrypoint machine-config-daemon '{{ .Images.machineConfigOperator }}' firstboot-complete-machineconfig
+  {{if .Proxy -}}
+  EnvironmentFile=/etc/mco/proxy.env
+  {{end -}}
+
+  [Install]
+  RequiredBy=multi-user.target

test/e2e-techpreview/onclusterbuild_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/openshift/machine-config-operator/pkg/controller/build"
 	"github.com/openshift/machine-config-operator/test/framework"
 	"github.com/openshift/machine-config-operator/test/helpers"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 
 	ctrlcommon "github.com/openshift/machine-config-operator/pkg/controller/common"
@@ -122,14 +123,30 @@ func TestOnClusterBuildRollsOutImage(t *testing.T) {
 	cs := framework.NewClientSet("")
 	node := helpers.GetRandomNode(t, cs, "worker")
 
-	t.Cleanup(makeIdempotentAndRegister(t, func() {
-		helpers.DeleteNodeAndMachine(t, cs, node)
-	}))
-
-	helpers.LabelNode(t, cs, node, helpers.MCPNameToRole(layeredMCPName))
+	unlabelFunc := makeIdempotentAndRegister(t, helpers.LabelNode(t, cs, node, helpers.MCPNameToRole(layeredMCPName)))
 	helpers.WaitForNodeImageChange(t, cs, node, imagePullspec)
 
+	assert.Contains(t, helpers.GetRPMOstreeStatus(t, cs, node), imagePullspec, "node %q did not get new image %q", node.Name, imagePullspec)
+	t.Logf("Node %s is booted into image %q", node.Name, imagePullspec)
+
 	t.Log(helpers.ExecCmdOnNode(t, cs, node, "chroot", "/rootfs", "cowsay", "Moo!"))
+
+	unlabelFunc()
+
+	assertNodeRevertsToNonLayered(t, cs, node)
+}
+
+func assertNodeRevertsToNonLayered(t *testing.T, cs *framework.ClientSet, node corev1.Node) {
+	workerMCName := helpers.GetMcName(t, cs, "worker")
+	workerMC, err := cs.MachineConfigs().Get(context.TODO(), workerMCName, metav1.GetOptions{})
+	require.NoError(t, err)
+
+	helpers.WaitForNodeConfigAndImageChange(t, cs, node, workerMCName, "")
+
+	assert.Contains(t, helpers.GetRPMOstreeStatus(t, cs, node), workerMC.Spec.OSImageURL, "node %q did not rollback to OS image %q", node.Name, workerMC.Spec.OSImageURL)
+	t.Logf("Node %s has reverted to OS image %q", node.Name, workerMC.Spec.OSImageURL)
+
+	helpers.AssertFileNotOnNode(t, cs, node, "/etc/systemd/system/machine-config-daemon-revert-layered.service")
 }
 
 // This test extracts the /etc/yum.repos.d and /etc/pki/rpm-gpg content from a

test/e2e/osimageurl_override_test.go

@@ -76,12 +76,8 @@ func assertNodeDoesNotHaveBinaries(t *testing.T, cs *framework.ClientSet, node c
 // Creates a new MachineConfigPool, adds the given node to it, and overrides
 // the osImageURL with the provided OS image name.
 func applyCustomOSToNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, osImageURL, poolName string) func() {
-	getRpmOstreeStatus := func() string {
-		return helpers.ExecCmdOnNode(t, cs, node, "chroot", "/rootfs", "rpm-ostree", "status")
-	}
-
 	// Do a pre-run assertion to ensure that we are not in the new OS image.
-	assert.NotContains(t, getRpmOstreeStatus(), osImageURL, fmt.Sprintf("node %q already booted into %q", node.Name, osImageURL))
+	assert.NotContains(t, helpers.GetRPMOstreeStatus(t, cs, node), osImageURL, fmt.Sprintf("node %q already booted into %q", node.Name, osImageURL))
 
 	mc := helpers.NewMachineConfig("custom-os-image", helpers.MCLabelForRole(poolName), osImageURL, []ign3types.File{})
 
@@ -90,7 +86,7 @@ func applyCustomOSToNode(t *testing.T, cs *framework.ClientSet, node corev1.Node
 	undoFunc := helpers.CreatePoolAndApplyMCToNode(t, cs, poolName, node, mc)
 
 	// Assert that we've booted into the new custom OS image.
-	assert.Contains(t, getRpmOstreeStatus(), osImageURL, fmt.Sprintf("node %q did not boot into %q", node.Name, osImageURL))
+	assert.Contains(t, helpers.GetRPMOstreeStatus(t, cs, node), osImageURL, fmt.Sprintf("node %q did not boot into %q", node.Name, osImageURL))
 
 	t.Logf("Node %q has booted into %q", node.Name, osImageURL)
 
@@ -99,7 +95,7 @@ func applyCustomOSToNode(t *testing.T, cs *framework.ClientSet, node corev1.Node
 		undoFunc()
 
 		// Assert that rpm-ostree indicates we're not running the custom OS image anymore.
-		assert.NotContains(t, getRpmOstreeStatus(), osImageURL, fmt.Sprintf("node %q did not roll back to previous OS image", node.Name))
+		assert.NotContains(t, helpers.GetRPMOstreeStatus(t, cs, node), osImageURL, fmt.Sprintf("node %q did not roll back to previous OS image", node.Name))
 
 		t.Logf("Node %q has returned to its previous OS image", node.Name)
 	})

test/helpers/utils.go

@@ -811,6 +811,10 @@ func ExecCmdOnNode(t *testing.T, cs *framework.ClientSet, node corev1.Node, subA
 	return string(out)
 }
 
+func GetRPMOstreeStatus(t *testing.T, cs *framework.ClientSet, node corev1.Node) string {
+	return ExecCmdOnNode(t, cs, node, "chroot", "/rootfs", "rpm-ostree", "status")
+}
+
 // ExecCmdOnNodeWithError behaves like ExecCmdOnNode, with the exception that
 // any errors are returned to the caller for inspection. This allows one to
 // execute a command that is expected to fail; e.g., stat /nonexistant/file.