Skip to content

Commit

Permalink
Merge pull request openshift#752 from cgwalters/reconcile-diff
Browse files Browse the repository at this point in the history 
daemon: Log high level diff between MCs
  • Loading branch information
@openshift-merge-robot
openshift-merge-robot authored May 14, 2019
2 parents 0b76a36 + 448088e commit df41ae9
Show file tree
Hide file tree
Showing 2 changed files with 117 additions and 41 deletions.
54 changes: 35 additions & 19 deletions pkg/daemon/update.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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 {
Expand Down Expand Up @@ -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;
Expand All @@ -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
Expand All @@ -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
Expand All @@ -272,17 +282,18 @@ 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

// 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.
Expand All @@ -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
}
}
}
Expand All @@ -308,31 +319,31 @@ 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")
}
}

// Special case files append: if the new config wants us to append, then we
// 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)
}
}

Expand All @@ -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
Expand Down
104 changes: 82 additions & 22 deletions pkg/daemon/update_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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
Expand All @@ -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) {
Expand All @@ -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},
}

Expand Down Expand Up @@ -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
Expand All @@ -240,37 +301,36 @@ 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
tempUser6 := ignv2_2types.PasswdUser{Name: "core", SSHAuthorizedKeys: []ignv2_2types.SSHAuthorizedKey{}}
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) {
Expand Down Expand Up @@ -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
Expand Down

0 comments on commit df41ae9

Please sign in to comment.