diff --git a/pkg/daemon/daemon.go b/pkg/daemon/daemon.go index c9e3df51db..5109507700 100644 --- a/pkg/daemon/daemon.go +++ b/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" @@ -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 { @@ -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 @@ -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 diff --git a/pkg/daemon/daemon_test.go b/pkg/daemon/daemon_test.go index 6b0b71b81e..6fa776570c 100644 --- a/pkg/daemon/daemon_test.go +++ b/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 diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index 4799bd879c..18d614c693 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -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 { @@ -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() { @@ -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. @@ -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 @@ -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) { @@ -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") @@ -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 { diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index f38e2e30fc..773f8a25fb 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -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{ @@ -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.") } }