[MCO-126] Have capability to consume new format base OS image

Makefile

@@ -98,7 +98,7 @@ Dockerfile.rhel7: Dockerfile Makefile
 
 # This was copied from https://github.com/openshift/cluster-image-registry-operator
 test-e2e:
-	go test -tags=$(GOTAGS) -failfast -timeout 90m -v$${WHAT:+ -run="$$WHAT"} ./test/e2e/
+	go test -tags=$(GOTAGS) -failfast -timeout 100m -v$${WHAT:+ -run="$$WHAT"} ./test/e2e/
 
 test-e2e-single-node:
 	go test -tags=$(GOTAGS) -failfast -timeout 90m -v$${WHAT:+ -run="$$WHAT"} ./test/e2e-single-node/

cmd/machine-config-daemon/pivot.go

@@ -10,7 +10,6 @@ import (
 	// Enable sha256 in container image references
 	_ "crypto/sha256"
 
-	"github.com/golang/glog"
 	daemon "github.com/openshift/machine-config-operator/pkg/daemon"
 	errors "github.com/pkg/errors"
 	"github.com/spf13/cobra"
@@ -59,17 +58,17 @@ func run(_ *cobra.Command, args []string) (retErr error) {
 		container = args[0]
 	}
 
-	client := daemon.NewNodeUpdaterClient()
+	// I'm pretty sure we no longer use pivot directly anymore, although we have recommended users to
+	// use this utility in the past.
+	// Perhaps with coreos layering we should just remove this entirely.
 
-	osImageContentDir, err := daemon.ExtractOSImage(container)
-	if err != nil {
-		return err
-	}
-	changed, err := client.Rebase(container, osImageContentDir)
+	osImageContentDir, err := daemon.ExtractAndUpdateOS(container)
 	if err != nil {
 		return err
 	}
 
+	defer os.RemoveAll(osImageContentDir)
+
 	// Delete the file now that we successfully rebased
 	if fromEtcPullSpec {
 		if err := os.Remove(etcPivotFile); err != nil {
@@ -79,10 +78,6 @@ func run(_ *cobra.Command, args []string) (retErr error) {
 		}
 	}
 
-	if !changed {
-		glog.Info("No changes; already at target oscontainer")
-	}
-
 	return nil
 }
 

go.mod

@@ -45,6 +45,7 @@ require (
 	k8s.io/component-base v0.23.0
 	k8s.io/kubectl v0.23.0
 	k8s.io/kubelet v0.23.0
+	k8s.io/kubernetes v1.13.0
 	k8s.io/utils v0.0.0-20210930125809-cb0fa318a74b
 	sigs.k8s.io/controller-runtime v0.11.0
 )

go.sum

@@ -2051,6 +2051,7 @@ k8s.io/kubectl v0.23.0 h1:WABWfj+Z4tC3SfKBCtZr5sIVHsFtkU9Azii4DR9IT6Y=
 k8s.io/kubectl v0.23.0/go.mod h1:TfcGEs3u4dkmoC2eku1GYymdGaMtPMcaLLFrX/RB2kI=
 k8s.io/kubelet v0.23.0 h1:hHdHe/Hp3R2HzxnYI8/f173gDUOTRYERd7S7+to9MZw=
 k8s.io/kubelet v0.23.0/go.mod h1:A4DxfIt5Ka+rz54HAFhs1bgiFjJT6lcaAYUcACZl1/k=
+k8s.io/kubernetes v1.23.0 h1:r2DrryCpnmFfBuelpUNSWXHtD6Zy7SdwaCcycV5DsJE=
 k8s.io/kubernetes v1.23.0/go.mod h1:sgD3+Qzb8FHlRKlZnNCN+np3zZuHEAb/0PKLJkYyCUI=
 k8s.io/legacy-cloud-providers v0.23.0/go.mod h1:tM5owPlhLyEYJC2FLHgcGu1jks5ANvH2JlY03mnUYU4=
 k8s.io/metrics v0.23.0/go.mod h1:NDiZTwppEtAuKJ1Rxt3S4dhyRzdp6yUcJf0vo023dPo=

pkg/daemon/daemon.go

@@ -129,6 +129,11 @@ type Daemon struct {
 	configDriftMonitor ConfigDriftMonitor
 }
 
+// CoreOSDaemon protects the methods that should only be called on CoreOS variants
+type CoreOSDaemon struct {
+	*Daemon
+}
+
 const (
 	// pathSystemd is the path systemd modifiable units, services, etc.. reside
 	pathSystemd = "/etc/systemd/system"
@@ -983,6 +988,10 @@ func (dn *Daemon) LogSystemData() {
 	} else {
 		glog.Info("systemd service state: OK")
 	}
+
+	if err := checkNodeRpmOstreeVersion(); err != nil {
+		glog.Errorf("Failed to check rpm-ostree version: %v", err)
+	}
 }
 
 const (
@@ -1140,15 +1149,22 @@ func (dn *Daemon) checkStateOnFirstRun() error {
 		targetOSImageURL := state.currentConfig.Spec.OSImageURL
 		osMatch := dn.checkOS(targetOSImageURL)
 		if !osMatch {
+			// TODO: I am actually not sure when this should happen. Bootstrap updates should
+			// in theory be going through the firstboot-complete-machineconfig path before the
+			// sync ever happens. So I don't know what constitutes the bootstrap pivot here.
+
+			// In case of the OS not fully being provisioned, I am unsure if this is the correct
+			// path either, since it doesn't consider any other OS related updates, such as kargs,
+			// extensions, etc.
+
+			// We should look into this and look to remove. In the meantime, I will just use the
+			// new update format.
 			glog.Infof("Bootstrap pivot required to: %s", targetOSImageURL)
 			// This only returns on error
-			osImageContentDir, err := ExtractOSImage(targetOSImageURL)
+			osImageContentDir, err := ExtractAndUpdateOS(targetOSImageURL)
 			if err != nil {
 				return err
 			}
-			if err := dn.updateOS(state.currentConfig, osImageContentDir); err != nil {
-				return err
-			}
 			if err := os.RemoveAll(osImageContentDir); err != nil {
 				return err
 			}
@@ -1502,16 +1518,6 @@ func (dn *Daemon) validateOnDiskState(currentConfig *mcfgv1.MachineConfig) error
 	return validateOnDiskState(currentConfig, pathSystemd)
 }
 
-// compareOSImageURL checks whether the current and desired
-// URL are the same.  This used to do more, but now the
-// only special casing is to support an empty desired URL
-// as meaning "keep current OS" which we probably don't need
-// anymore either.
-func compareOSImageURL(current, desired string) bool {
-	// A desired "" is special cased
-	return desired == "" || current == desired
-}
-
 // checkOS determines whether the booted system matches the target
 // osImageURL and if not whether we need to take action.  This function
 // returns `true` if no action is required, which is the case if we're
@@ -1525,7 +1531,7 @@ func (dn *Daemon) checkOS(osImageURL string) bool {
 		return true
 	}
 
-	return compareOSImageURL(dn.bootedOSImageURL, osImageURL)
+	return dn.bootedOSImageURL == osImageURL
 }
 
 // Close closes all the connections the node agent has open for it's lifetime

pkg/daemon/daemon_test.go

@@ -11,7 +11,6 @@ import (
 
 	ign2types "github.com/coreos/ignition/config/v2_2/types"
 	ign3types "github.com/coreos/ignition/v2/config/v3_2/types"
-	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"github.com/vincent-petithory/dataurl"
 	corev1 "k8s.io/api/core/v1"
@@ -110,26 +109,6 @@ func TestValidateFiles(t *testing.T) {
 	}
 }
 
-func TestCompareOSImageURL(t *testing.T) {
-	refA := "registry.example.com/foo/bar@sha256:0743a3cc3bcf3b4aabb814500c2739f84cb085ff4e7ec7996aef7977c4c19c7f"
-	refB := "registry.example.com/foo/baz@sha256:0743a3cc3bcf3b4aabb814500c2739f84cb085ff4e7ec7996aef7977c4c19c7f"
-	refC := "registry.example.com/foo/bar@sha256:2a76681fd15bfc06fa4aa0ff6913ba17527e075417fc92ea29f6bcc2afca24ff"
-	m := compareOSImageURL(refA, refA)
-	if !m {
-		t.Fatalf("Expected refA ident")
-	}
-	m = compareOSImageURL(refA, refB)
-	if m {
-		t.Fatalf("Expected refA != refB")
-	}
-	m = compareOSImageURL(refA, refC)
-	if m {
-		t.Fatalf("Expected refA != refC")
-	}
-	m = compareOSImageURL("", refA)
-	assert.False(t, m)
-}
-
 type fixture struct {
 	t *testing.T
 

pkg/daemon/rpm-ostree.go

@@ -5,10 +5,12 @@ import (
 	"fmt"
 	"os"
 	"os/exec"
+	"strconv"
 	"strings"
 	"time"
 
 	"github.com/containers/image/v5/types"
+	yaml "github.com/ghodss/yaml"
 	"github.com/golang/glog"
 	"github.com/opencontainers/go-digest"
 	pivotutils "github.com/openshift/machine-config-operator/pkg/daemon/pivot/utils"
@@ -20,8 +22,21 @@ const (
 	numRetriesNetCommands = 5
 	// Pull secret.  Written by the machine-config-operator
 	kubeletAuthFile = "/var/lib/kubelet/config.json"
+
+	// rpmOstreeVersionMinimum is the minimum required version
+	rpmOstreeVersionMinimum = "2021.14"
 )
 
+// rpmOstreeVersionOuter is YAML output by `rpm-ostree --version`
+type rpmOstreeVersionOuter struct {
+	Root rpmOstreeVersionData `json:"rpm-ostree"`
+}
+
+type rpmOstreeVersionData struct {
+	Version  string   `json:"Version"`
+	Features []string `json:"Features"`
+}
+
 // rpmOstreeState houses zero or more RpmOstreeDeployments
 // Subset of `rpm-ostree status --json`
 // https://github.com/projectatomic/rpm-ostree/blob/bce966a9812df141d38e3290f845171ec745aa4e/src/daemon/rpmostreed-deployment-utils.c#L227
@@ -32,15 +47,16 @@ type rpmOstreeState struct {
 
 // RpmOstreeDeployment represents a single deployment on a node
 type RpmOstreeDeployment struct {
-	ID           string   `json:"id"`
-	OSName       string   `json:"osname"`
-	Serial       int32    `json:"serial"`
-	Checksum     string   `json:"checksum"`
-	Version      string   `json:"version"`
-	Timestamp    uint64   `json:"timestamp"`
-	Booted       bool     `json:"booted"`
-	Origin       string   `json:"origin"`
-	CustomOrigin []string `json:"custom-origin"`
+	ID                      string   `json:"id"`
+	OSName                  string   `json:"osname"`
+	Serial                  int32    `json:"serial"`
+	Checksum                string   `json:"checksum"`
+	Version                 string   `json:"version"`
+	Timestamp               uint64   `json:"timestamp"`
+	Booted                  bool     `json:"booted"`
+	Origin                  string   `json:"origin"`
+	CustomOrigin            []string `json:"custom-origin"`
+	ContainerImageReference string   `json:"container-image-reference"`
 }
 
 // imageInspection is a public implementation of
@@ -100,6 +116,60 @@ func (r *RpmOstreeClient) loadStatus() (*rpmOstreeState, error) {
 	return &rosState, nil
 }
 
+func parseVer(s string) ([]int, error) {
+	r := []int{}
+	for _, s := range strings.Split(s, ".") {
+		n, err := strconv.Atoi(s)
+		if err != nil {
+			return nil, fmt.Errorf("Failed to parse %s: %v", s, err)
+		}
+		r = append(r, n)
+	}
+	return r, nil
+}
+
+func validateVersion(current rpmOstreeVersionOuter) error {
+	requiredVer, err := parseVer(rpmOstreeVersionMinimum)
+	if err != nil {
+		return err
+	}
+	curVer, err := parseVer(current.Root.Version)
+	if err != nil {
+		return err
+	}
+	if len(curVer) < len(requiredVer) {
+		return fmt.Errorf("Too few components in %s, expected to match %s", current.Root.Version, rpmOstreeVersionMinimum)
+	}
+	for i, v := range requiredVer {
+		if curVer[i] < v {
+			return fmt.Errorf("Too old %s, expected to match %s", current.Root.Version, rpmOstreeVersionMinimum)
+		}
+		// Shortcut for e.g. 2022 > 2021
+		if curVer[i] > v {
+			return nil
+		}
+	}
+
+	return nil
+}
+
+func checkNodeRpmOstreeVersion() error {
+	versionBytes, err := runGetOut("rpm-ostree", "--version")
+	if err != nil {
+		return err
+	}
+	var versionData rpmOstreeVersionOuter
+	if err := yaml.Unmarshal(versionBytes, &versionData); err != nil {
+		return fmt.Errorf("failed to parse rpm-ostree --version as YAML: %v", err)
+	}
+
+	if err := validateVersion(versionData); err != nil {
+		return fmt.Errorf("Too old rpm-ostree: %v", err)
+	}
+
+	return nil
+}
+
 func (r *RpmOstreeClient) Initialize() error {
 	// This replicates https://github.com/coreos/rpm-ostree/pull/2945
 	// and can be removed when we have a new enough rpm-ostree with
@@ -180,6 +250,11 @@ func (r *RpmOstreeClient) GetBootedOSImageURL() (string, string, error) {
 		}
 	}
 
+	// Not sure if ContainerImageReference is canonical
+	if bootedDeployment.ContainerImageReference != "" {
+		osImageURL = bootedDeployment.ContainerImageReference[len("ostree-unverified-registry:"):]
+	}
+
 	return osImageURL, bootedDeployment.Version, nil
 }
 

pkg/daemon/rpm-ostree_test.go

@@ -1,5 +1,13 @@
 package daemon
 
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+	"k8s.io/apimachinery/pkg/util/yaml"
+)
+
 /*
  * This file contains test code for the rpm-ostree client. It is meant to be used when
  * testing the daemon and mocking the responses that would normally be executed by the
@@ -46,3 +54,39 @@ func (r RpmOstreeClientMock) GetStatus() (string, error) {
 func (r RpmOstreeClientMock) GetBootedDeployment() (*RpmOstreeDeployment, error) {
 	return &RpmOstreeDeployment{}, nil
 }
+
+func TestParseVersion(t *testing.T) {
+	s := `
+rpm-ostree:
+  Version: '2021.14'
+  Git: v2021.14
+  Features:
+   - bin-unit-tests
+   - compose
+   - rust
+   - fedora-integration
+   `
+	var outer rpmOstreeVersionOuter
+	assert.Nil(t, yaml.Unmarshal([]byte(s), &outer))
+	fmt.Printf("%v", outer)
+	assert.Equal(t, outer.Root.Version, "2021.14")
+}
+
+func TestValidateVersion(t *testing.T) {
+	for _, old := range []string{"2019.5", "2021.6"} {
+		v := rpmOstreeVersionOuter{
+			Root: rpmOstreeVersionData{
+				Version: old,
+			},
+		}
+		assert.NotNil(t, validateVersion(v))
+	}
+	for _, newver := range []string{"2021.14", "2021.15", "2022.1"} {
+		v := rpmOstreeVersionOuter{
+			Root: rpmOstreeVersionData{
+				Version: newver,
+			},
+		}
+		assert.Nil(t, validateVersion(v))
+	}
+}