diff --git a/pkg/daemon/update.go b/pkg/daemon/update.go index e6061536d9..4c5b604702 100644 --- a/pkg/daemon/update.go +++ b/pkg/daemon/update.go @@ -172,7 +172,7 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err newConfigName := newConfig.GetName() glog.Infof("Checking reconcilable for config %v to %v", oldConfigName, newConfigName) // make sure we can actually reconcile this state - reconcilableError := dn.reconcilable(oldConfig, newConfig) + diff, reconcilableError := dn.reconcilable(oldConfig, newConfig) if reconcilableError != nil { wrappedErr := fmt.Errorf("can't reconcile config %s with %s: %v", oldConfigName, newConfigName, reconcilableError) @@ -186,7 +186,7 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err } return errors.Wrapf(errUnreconcilable, "%v", wrappedErr) } - dn.logSystem("Starting update from %s to %s", oldConfigName, newConfigName) + dn.logSystem("Starting update from %s to %s: %+v", oldConfigName, newConfigName, diff) // update files on disk that need updating if err := dn.updateFiles(oldConfig, newConfig); err != nil { @@ -230,6 +230,17 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err return dn.updateOSAndReboot(newConfig) } +// machineConfigDiff represents an ad-hoc difference between two MachineConfig objects. +// At some point this may change into holding just the files/units that changed +// and the MCO would just operate on that. For now we're just doing this to get +// improved logging. +type machineConfigDiff struct { + osUpdate bool + passwd bool + files bool + units bool +} + // reconcilable checks the configs to make sure that the only changes requested // are ones we know how to do in-place. If we can reconcile, (nil, nil) is returned. // Otherwise, if we can't do it in place, the node is marked as degraded; @@ -238,13 +249,12 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err // we can only update machine configs that have changes to the files, // directories, links, and systemd units sections of the included ignition // config currently. - -func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error { +func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) (*machineConfigDiff, error) { // We skip out of reconcilable if there is no Kind and we are in runOnce mode. The // reason is that there is a good chance a previous state is not available to match against. if oldConfig.Kind == "" && dn.onceFrom != "" { glog.Info("Missing kind in old config. Assuming no prior state.") - return nil + return nil, nil } oldIgn := oldConfig.Spec.Config newIgn := newConfig.Spec.Config @@ -253,13 +263,13 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // First check if this is a generally valid Ignition Config rpt := validate.ValidateWithoutSource(reflect.ValueOf(newIgn)) if rpt.IsFatal() { - return errors.Errorf("invalid Ignition config found: %v", rpt) + return nil, errors.Errorf("invalid Ignition config found: %v", rpt) } // if the config versions are different, all bets are off. this probably // shouldn't happen, but if it does, we can't deal with it. if oldIgn.Ignition.Version != newIgn.Ignition.Version { - return fmt.Errorf("ignition version mismatch between old and new config: old: %s new: %s", + return nil, fmt.Errorf("ignition version mismatch between old and new config: old: %s new: %s", oldIgn.Ignition.Version, newIgn.Ignition.Version) } // everything else in the ignition section doesn't matter to us, since the @@ -272,7 +282,7 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // we don't currently configure the network in place. we can't fix it if // something changed here. if !reflect.DeepEqual(oldIgn.Networkd, newIgn.Networkd) { - return errors.New("ignition networkd section contains changes") + return nil, errors.New("ignition networkd section contains changes") } // Passwd section @@ -280,9 +290,10 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // we don't currently configure Groups in place. we don't configure Users except // for setting/updating SSHAuthorizedKeys for the only allowed user "core". // otherwise we can't fix it if something changed here. - if !reflect.DeepEqual(oldIgn.Passwd, newIgn.Passwd) { + passwdChanged := !reflect.DeepEqual(oldIgn.Passwd, newIgn.Passwd) + if passwdChanged { if !reflect.DeepEqual(oldIgn.Passwd.Groups, newIgn.Passwd.Groups) { - return errors.New("ignition Passwd Groups section contains changes") + return nil, errors.New("ignition Passwd Groups section contains changes") } if !reflect.DeepEqual(oldIgn.Passwd.Users, newIgn.Passwd.Users) { // check if the prior config is empty and that this is the first time running. @@ -292,12 +303,12 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // change to the SSHAuthorizedKeys for the user "core" for _, user := range newIgn.Passwd.Users { if user.Name != coreUserName { - return errors.New("ignition passwd user section contains unsupported changes: non-core user") + return nil, errors.New("ignition passwd user section contains unsupported changes: non-core user") } } glog.Infof("user data to be verified before ssh update: %v", newIgn.Passwd.Users[len(newIgn.Passwd.Users)-1]) if err := verifyUserFields(newIgn.Passwd.Users[len(newIgn.Passwd.Users)-1]); err != nil { - return err + return nil, err } } } @@ -308,23 +319,23 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // we can only reconcile files right now. make sure the sections we can't // fix aren't changed. if !reflect.DeepEqual(oldIgn.Storage.Disks, newIgn.Storage.Disks) { - return errors.New("ignition disks section contains changes") + return nil, errors.New("ignition disks section contains changes") } if !reflect.DeepEqual(oldIgn.Storage.Filesystems, newIgn.Storage.Filesystems) { - return errors.New("ignition filesystems section contains changes") + return nil, errors.New("ignition filesystems section contains changes") } if !reflect.DeepEqual(oldIgn.Storage.Raid, newIgn.Storage.Raid) { - return errors.New("ignition raid section contains changes") + return nil, errors.New("ignition raid section contains changes") } if !reflect.DeepEqual(oldIgn.Storage.Directories, newIgn.Storage.Directories) { - return errors.New("ignition directories section contains changes") + return nil, errors.New("ignition directories section contains changes") } if !reflect.DeepEqual(oldIgn.Storage.Links, newIgn.Storage.Links) { // This means links have been added, as opposed as being removed as it happened with // https://bugzilla.redhat.com/show_bug.cgi?id=1677198. This doesn't really change behavior // since we still don't support links but we allow old MC to remove links when upgrading. if len(newIgn.Storage.Links) != 0 { - return errors.New("ignition links section contains changes") + return nil, errors.New("ignition links section contains changes") } } @@ -332,7 +343,7 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // have to force a reprovision since it's not idempotent for _, f := range newIgn.Storage.Files { if f.Append { - return fmt.Errorf("ignition file %v includes append", f.Path) + return nil, fmt.Errorf("ignition file %v includes append", f.Path) } } @@ -342,7 +353,12 @@ func (dn *Daemon) reconcilable(oldConfig, newConfig *mcfgv1.MachineConfig) error // we made it through all the checks. reconcile away! glog.V(2).Info("Configs are reconcilable") - return nil + return &machineConfigDiff { + osUpdate: oldConfig.Spec.OSImageURL != newConfig.Spec.OSImageURL, + passwd: passwdChanged, + files: !reflect.DeepEqual(oldIgn.Storage.Files, newIgn.Storage.Files), + units: !reflect.DeepEqual(oldIgn.Systemd.Units, newIgn.Systemd.Units), + }, nil } // verifyUserFields returns nil if the user Name = "core", if 1 or more SSHKeys exist for diff --git a/pkg/daemon/update_test.go b/pkg/daemon/update_test.go index 00f0d90fbc..57faf515c8 100644 --- a/pkg/daemon/update_test.go +++ b/pkg/daemon/update_test.go @@ -91,12 +91,12 @@ func TestReconcilable(t *testing.T) { } // Verify Ignition version changes react as expected - isReconcilable := d.reconcilable(oldConfig, newConfig) + _, isReconcilable := d.reconcilable(oldConfig, newConfig) checkIrreconcilableResults(t, "Ignition", isReconcilable) // Match ignition versions oldConfig.Spec.Config.Ignition.Version = "2.2.0" - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkReconcilableResults(t, "Ignition", isReconcilable) // Verify Networkd unit changes react as expected @@ -108,13 +108,13 @@ func TestReconcilable(t *testing.T) { }, }, } - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkIrreconcilableResults(t, "Networkd", isReconcilable) // Match Networkd oldConfig.Spec.Config.Networkd = newConfig.Spec.Config.Networkd - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkReconcilableResults(t, "Networkd", isReconcilable) // Verify Disk changes react as expected @@ -124,12 +124,12 @@ func TestReconcilable(t *testing.T) { }, } - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkIrreconcilableResults(t, "Disk", isReconcilable) // Match storage disks newConfig.Spec.Config.Storage.Disks = oldConfig.Spec.Config.Storage.Disks - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkReconcilableResults(t, "Disk", isReconcilable) // Verify Filesystems changes react as expected @@ -141,12 +141,12 @@ func TestReconcilable(t *testing.T) { }, } - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkIrreconcilableResults(t, "Filesystem", isReconcilable) // Match Storage filesystems newConfig.Spec.Config.Storage.Filesystems = oldConfig.Spec.Config.Storage.Filesystems - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkReconcilableResults(t, "Filesystem", isReconcilable) // Verify Raid changes react as expected @@ -157,12 +157,12 @@ func TestReconcilable(t *testing.T) { }, } - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkIrreconcilableResults(t, "Raid", isReconcilable) // Match storage raid newConfig.Spec.Config.Storage.Raid = oldConfig.Spec.Config.Storage.Raid - isReconcilable = d.reconcilable(oldConfig, newConfig) + _, isReconcilable = d.reconcilable(oldConfig, newConfig) checkReconcilableResults(t, "Raid", isReconcilable) // Verify Passwd Groups changes unsupported @@ -177,9 +177,70 @@ func TestReconcilable(t *testing.T) { }, }, } - isReconcilable = d.reconcilable(oldConfig, newMcfg) + _, isReconcilable = d.reconcilable(oldConfig, newMcfg) checkIrreconcilableResults(t, "PasswdGroups", isReconcilable) +} + +func newTestIgnitionFile(i uint) ignv2_2types.File { + mode := 0644 + return ignv2_2types.File{Node: ignv2_2types.Node{Path: fmt.Sprintf("/etc/config%d", i), Filesystem: "root"}, + FileEmbedded1: ignv2_2types.FileEmbedded1{Contents: ignv2_2types.FileContents{Source: fmt.Sprintf("data:,config%d", i)}, Mode: &mode}} +} + +func newMachineConfigFromFiles(files []ignv2_2types.File) *mcfgv1.MachineConfig { + return &mcfgv1.MachineConfig{ + Spec: mcfgv1.MachineConfigSpec{ + Config: ignv2_2types.Config{ + Ignition: ignv2_2types.Ignition{ + Version: "2.2.0", + }, + Storage: ignv2_2types.Storage{ + Files: files, + }, + }, + }, + } +} +func TestReconcilableDiff(t *testing.T) { + d := Daemon{ + name: "nodeName", + OperatingSystem: machineConfigDaemonOSRHCOS, + NodeUpdaterClient: nil, + kubeClient: nil, + bootedOSImageURL: "test", + } + var oldFiles []ignv2_2types.File + nOldFiles := uint(10) + for i := uint(0); i < nOldFiles; i++ { + oldFiles = append(oldFiles, newTestIgnitionFile(uint(i))) + } + oldConfig := newMachineConfigFromFiles(oldFiles) + newConfig := newMachineConfigFromFiles(append(oldFiles, newTestIgnitionFile(nOldFiles + 1))) + + diff, err := d.reconcilable(oldConfig, newConfig) + checkReconcilableResults(t, "add file", err) + assert.Equal(t, diff.osUpdate, false) + assert.Equal(t, diff.passwd, false) + assert.Equal(t, diff.units, false) + assert.Equal(t, diff.files, true) + + newConfig = newMachineConfigFromFiles(nil) + diff, err = d.reconcilable(oldConfig, newConfig) + checkReconcilableResults(t, "remove all files", err) + assert.Equal(t, diff.osUpdate, false) + assert.Equal(t, diff.passwd, false) + assert.Equal(t, diff.units, false) + assert.Equal(t, diff.files, true) + + newConfig = newMachineConfigFromFiles(oldFiles) + newConfig.Spec.OSImageURL = "example.com/machine-os-content:new" + diff, err = d.reconcilable(oldConfig, newConfig) + checkReconcilableResults(t, "os update", err) + assert.Equal(t, diff.osUpdate, true) + assert.Equal(t, diff.passwd, false) + assert.Equal(t, diff.units, false) + assert.Equal(t, diff.files, false) } func TestReconcilableSSH(t *testing.T) { @@ -193,7 +254,7 @@ func TestReconcilableSSH(t *testing.T) { RunPivotReturns: []error{ // First run will return no error nil, - // Second rrun will return our expected error + // Second run will return our expected error expectedError}, } @@ -231,7 +292,7 @@ func TestReconcilableSSH(t *testing.T) { }, } - errMsg := d.reconcilable(oldMcfg, newMcfg) + _, errMsg := d.reconcilable(oldMcfg, newMcfg) checkReconcilableResults(t, "SSH", errMsg) // Check that updating User with User that is not core is not supported @@ -240,21 +301,21 @@ func TestReconcilableSSH(t *testing.T) { tempUser3 := ignv2_2types.PasswdUser{Name: "another user", SSHAuthorizedKeys: []ignv2_2types.SSHAuthorizedKey{"5678"}} newMcfg.Spec.Config.Passwd.Users[0] = tempUser3 - errMsg = d.reconcilable(oldMcfg, newMcfg) + _, errMsg = d.reconcilable(oldMcfg, newMcfg) checkIrreconcilableResults(t, "SSH", errMsg) // check that we cannot make updates if any other Passwd.User field is changed. tempUser4 := ignv2_2types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ignv2_2types.SSHAuthorizedKey{"5678"}, HomeDir: "somedir"} newMcfg.Spec.Config.Passwd.Users[0] = tempUser4 - errMsg = d.reconcilable(oldMcfg, newMcfg) + _, errMsg = d.reconcilable(oldMcfg, newMcfg) checkIrreconcilableResults(t, "SSH", errMsg) // check that we cannot add a user or have len(Passwd.Users)> 1 tempUser5 := ignv2_2types.PasswdUser{Name: "some user", SSHAuthorizedKeys: []ignv2_2types.SSHAuthorizedKey{"5678"}} newMcfg.Spec.Config.Passwd.Users = append(newMcfg.Spec.Config.Passwd.Users, tempUser5) - errMsg = d.reconcilable(oldMcfg, newMcfg) + _, errMsg = d.reconcilable(oldMcfg, newMcfg) checkIrreconcilableResults(t, "SSH", errMsg) // check that user is not attempting to remove the only sshkey from core user @@ -262,15 +323,14 @@ func TestReconcilableSSH(t *testing.T) { newMcfg.Spec.Config.Passwd.Users[0] = tempUser6 newMcfg.Spec.Config.Passwd.Users = newMcfg.Spec.Config.Passwd.Users[:len(newMcfg.Spec.Config.Passwd.Users)-1] - errMsg = d.reconcilable(oldMcfg, newMcfg) + _, errMsg = d.reconcilable(oldMcfg, newMcfg) checkIrreconcilableResults(t, "SSH", errMsg) //check that empty Users does not generate error/degrade node newMcfg.Spec.Config.Passwd.Users = nil - errMsg = d.reconcilable(oldMcfg, newMcfg) + _, errMsg = d.reconcilable(oldMcfg, newMcfg) checkReconcilableResults(t, "SSH", errMsg) - } func TestUpdateSSHKeys(t *testing.T) { @@ -374,13 +434,13 @@ func TestInvalidIgnConfig(t *testing.T) { }, }, } - err := d.reconcilable(oldMcfg, newMcfg) + _, err := d.reconcilable(oldMcfg, newMcfg) assert.NotNil(t, err, "Expected error. Relative Paths should fail general ignition validation") newMcfg.Spec.Config.Storage.Files[0].Node.Path = "/home/core/test" - err = d.reconcilable(oldMcfg, newMcfg) + diff, err := d.reconcilable(oldMcfg, newMcfg) assert.Nil(t, err, "Expected no error. Absolute paths should not fail general ignition validation") - + assert.Equal(t, diff.files, true) } // checkReconcilableResults is a shortcut for verifying results that should be reconcilable