Skip to content

daemon.go/update.go: various cleanup surrounding OS updates #2973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
openshift-merge-robot merged 4 commits into from
Mar 10, 2022
Merged

daemon.go/update.go: various cleanup surrounding OS updates #2973

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
83f511c
Remove redundant checks in updateOS
mkenigs Dec 23, 2021
8e30e61
Consolidate IsCoreOSVariant calls
mkenigs Dec 23, 2021
f5552dc
Remove compareOSImageURL
mkenigs Dec 23, 2021
a5bd3e5
Add CoreOSDaemon struct
mkenigs Dec 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 7 additions & 12 deletions pkg/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,11 @@ type Daemon struct {
configDriftMonitor ConfigDriftMonitor
}

// CoreOSDaemon protects the methods that should only be called on CoreOS variants

@cheesesashimi cheesesashimi Mar 1, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we anticipate the need to support additional variants? If we do, this may be better expressed as an interface:

// New type to avoid an argument ordering error
type Updates struct {
	OldConfig *mcfgv1.MachineConfig
	NewConfig *mcfgv1.MachineConfig
}

func (u Updates) Diff() machineConfigDiff {
	// I don't know how this is computed
	return machineConfigDiff
}

func (u Updates) Rollback() Updates {
	return Updates{
		OldConfig: u.NewConfig,
		NewConfig: u.OldConfig,
	}
}

// Interface definition
type OSUpdater interface {
	ApplyOSChanges(machineConfigDiff, Updates) error
	ApplyExtensions(Updates) error
	SwitchKernel(Updates) error
}

// CoreOS specific implementation gets hung off of this struct
type CoreOSUpdater struct {
	*Daemon
}

// RHEL-specific implementation gets hung off of this struct
type RHELUpdater struct {
	*Daemon
}

func NewOSUpdater(dn *Daemon) OSUpdater {
	if dn.os.IsCoreOSVariant() {
		return &CoreOSUpdater{dn}
	}

	return &RHELUpdater{dn}
}

@mkenigs mkenigs Mar 2, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're definitely going to want more variants. At least (Legacy)Daemon, (Legacy)CoreOSDaemon, and LayeredDaemon. I've thought briefly about what that could look like but just haven't put the effort into coming up with something that works yet. It is a little tricky because the interface can't just be update() because a legacy update would need update(oldMachineConfig, newMachineConfig), and (I don't think) the MachineConfigs are always calculated the same way for current code paths. Whereas a layered update will just need the image annotation.

@cheesesashimi cheesesashimi Mar 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that makes sense. There's definitely more stuff to flesh out than my admittedly-naïve example includes. But in general, it would be nice to have a common interface for all the variants since each variant can have its own independent implementation so we don't have a bunch of duplicated logic.

@mkenigs mkenigs Mar 2, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Completely agreed! I'm thinking it will be nuanced enough that we can wrap back to it once we have a little more time. Would you agree this is in a state that it's okay to merge and come back to later? And I'll make a card

@cheesesashimi cheesesashimi Mar 2, 2022

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's fine for now. I do want to figure out why the CI runs failed first before this gets merged.

type CoreOSDaemon struct {
*Daemon
}

const (
// pathSystemd is the path systemd modifiable units, services, etc.. reside
pathSystemd = "/etc/systemd/system"
Expand Down Expand Up @@ -1146,7 +1151,7 @@ func (dn *Daemon) checkStateOnFirstRun() error {
if err != nil {
return err
}
if err := dn.updateOS(state.currentConfig, osImageContentDir); err != nil {
if err := updateOS(state.currentConfig, osImageContentDir); err != nil {
return err
}
if err := os.RemoveAll(osImageContentDir); err != nil {
Expand Down Expand Up @@ -1502,16 +1507,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
Expand All @@ -1525,7 +1520,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
Expand Down
21 changes: 0 additions & 21 deletions pkg/daemon/daemon_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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

Expand Down
77 changes: 31 additions & 46 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -337,7 +337,7 @@ func removePendingDeployment() error {
return runRpmOstree("cleanup", "-p")
}

func (dn *Daemon) applyOSChanges(mcDiff machineConfigDiff, oldConfig, newConfig *mcfgv1.MachineConfig) (retErr error) {
func (dn *CoreOSDaemon) applyOSChanges(mcDiff machineConfigDiff, oldConfig, newConfig *mcfgv1.MachineConfig) (retErr error) {
// Extract image and add coreos-extensions repo if we have either OS update or package layering to perform

if dn.recorder != nil {
Expand Down Expand Up @@ -371,22 +371,22 @@ func (dn *Daemon) applyOSChanges(mcDiff machineConfigDiff, oldConfig, newConfig
// Delete extracted OS image once we are done.
defer os.RemoveAll(osImageContentDir)

if dn.os.IsCoreOSVariant() {
if err := addExtensionsRepo(osImageContentDir); err != nil {
return err
}
defer os.Remove(extensionsRepo)
if err := addExtensionsRepo(osImageContentDir); err != nil {
return err
}
defer os.Remove(extensionsRepo)
}

// Update OS
if err := dn.updateOS(newConfig, osImageContentDir); err != nil {
nodeName := ""
if dn.node != nil {
nodeName = dn.node.Name
if mcDiff.osUpdate {
if err := updateOS(newConfig, osImageContentDir); err != nil {
nodeName := ""
if dn.node != nil {
nodeName = dn.node.Name
}
MCDPivotErr.WithLabelValues(nodeName, newConfig.Spec.OSImageURL, err.Error()).SetToCurrentTime()
return err
}
MCDPivotErr.WithLabelValues(nodeName, newConfig.Spec.OSImageURL, err.Error()).SetToCurrentTime()
return err
}

defer func() {
Expand Down Expand Up @@ -575,18 +575,23 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err
}
}()

if err := dn.applyOSChanges(*diff, oldConfig, newConfig); err != nil {
return err
}
if dn.os.IsCoreOSVariant() {
coreOSDaemon := CoreOSDaemon{dn}
if err := coreOSDaemon.applyOSChanges(*diff, oldConfig, newConfig); err != nil {
return err
}

defer func() {
if retErr != nil {
if err := dn.applyOSChanges(*diff, newConfig, oldConfig); err != nil {
retErr = errors.Wrapf(retErr, "error rolling back changes to OS %v", err)
return
defer func() {
if retErr != nil {
if err := coreOSDaemon.applyOSChanges(*diff, newConfig, oldConfig); err != nil {
retErr = errors.Wrapf(retErr, "error rolling back changes to OS %v", err)
return
}
}
}
}()
}()
} else {
glog.Info("updating the OS on non-CoreOS nodes is not supported")
}

// Ideally we would want to update kernelArguments only via MachineConfigs.
// We are keeping this to maintain compatibility and OKD requirement.
Expand Down Expand Up @@ -930,12 +935,7 @@ func generateKargs(oldConfig, newConfig *mcfgv1.MachineConfig) []string {
}

// updateKernelArguments adjusts the kernel args
func (dn *Daemon) updateKernelArguments(oldConfig, newConfig *mcfgv1.MachineConfig) error {
if !dn.os.IsCoreOSVariant() {
glog.Info("updating kargs on non-CoreOS nodes is not supported")
return nil
}

func (dn *CoreOSDaemon) updateKernelArguments(oldConfig, newConfig *mcfgv1.MachineConfig) error {
kargs := generateKargs(oldConfig, newConfig)
if len(kargs) == 0 {
return nil
Expand Down Expand Up @@ -1035,13 +1035,7 @@ func validateExtensions(exts []string) error {

}

func (dn *Daemon) applyExtensions(oldConfig, newConfig *mcfgv1.MachineConfig) error {
// Right now, we support extensions only on CoreOS nodes
if !dn.os.IsCoreOSVariant() {
glog.Info("applying extensions on non-CoreOS nodes is not supported")
return nil
}

func (dn *CoreOSDaemon) applyExtensions(oldConfig, newConfig *mcfgv1.MachineConfig) error {
extensionsEmpty := len(oldConfig.Spec.Extensions) == 0 && len(newConfig.Spec.Extensions) == 0
if (extensionsEmpty) ||
(reflect.DeepEqual(oldConfig.Spec.Extensions, newConfig.Spec.Extensions) && oldConfig.Spec.OSImageURL == newConfig.Spec.OSImageURL) {
Expand All @@ -1060,7 +1054,7 @@ func (dn *Daemon) applyExtensions(oldConfig, newConfig *mcfgv1.MachineConfig) er

// switchKernel updates kernel on host with the kernelType specified in MachineConfig.
// Right now it supports default (traditional) and realtime kernel
func (dn *Daemon) switchKernel(oldConfig, newConfig *mcfgv1.MachineConfig) error {
func (dn *CoreOSDaemon) switchKernel(oldConfig, newConfig *mcfgv1.MachineConfig) error {
// We support Kernel update only on RHCOS nodes
if !dn.os.IsRHCOS() {
glog.Info("updating kernel on non-RHCOS nodes is not supported")
Expand Down Expand Up @@ -1804,17 +1798,8 @@ func (dn *Daemon) updateSSHKeys(newUsers []ign3types.PasswdUser) error {
}

// updateOS updates the system OS to the one specified in newConfig
func (dn *Daemon) updateOS(config *mcfgv1.MachineConfig, osImageContentDir string) error {
if !dn.os.IsCoreOSVariant() {
glog.Info("Updating of non-CoreOS nodes are not supported")
return nil
}

func updateOS(config *mcfgv1.MachineConfig, osImageContentDir string) error {
newURL := config.Spec.OSImageURL
if compareOSImageURL(dn.bootedOSImageURL, newURL) {
return nil
}

glog.Infof("Updating OS to %s", newURL)
client := NewNodeUpdaterClient()
if _, err := client.Rebase(newURL, osImageContentDir); err != nil {
Expand Down
12 changes: 2 additions & 10 deletions pkg/daemon/update_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,10 +91,6 @@ func TestUpdateOS(t *testing.T) {
// expectedError is the error we will use when expecting an error to return
expectedError := fmt.Errorf("broken")

d := newMockDaemon()

// Set up machineconfigs to pass to updateOS.
mcfg := &mcfgv1.MachineConfig{}
// differentMcfg has a different OSImageURL so it will force Daemon.UpdateOS
// to trigger an update of the operatingsystem (as fronted by our testClient)
differentMcfg := &mcfgv1.MachineConfig{
Expand All @@ -103,12 +99,8 @@ func TestUpdateOS(t *testing.T) {
},
}

// This should be a no-op
if err := d.updateOS(mcfg, ""); err != nil {
t.Errorf("Expected no error. Got %s.", err)
}
// Second call should return an error
if err := d.updateOS(differentMcfg, ""); err == expectedError {
// should return an error
if err := updateOS(differentMcfg, ""); err == expectedError {
t.Error("Expected an error. Got none.")
}
}
Expand Down