Skip to content
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

daemon: skip or perform node reboot based on rebootAction #2254

Closed
wants to merge 1 commit into from
Closed

daemon: skip or perform node reboot based on rebootAction #2254

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 45 additions & 26 deletions pkg/daemon/daemon.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -165,6 +165,14 @@ const (
onceFromRemoteConfig
)

type rebootAction int

const (
rebootActionReboot rebootAction = iota
rebootActionNone
rebootActionReloadCrio
)

var (
defaultRebootTimeout = 24 * time.Hour
)
Expand Down Expand Up @@ -1015,7 +1023,10 @@ func (dn *Daemon) checkStateOnFirstRun() error {
if err := dn.drain(); err != nil {
return err
}
return dn.finalizeAndReboot(state.pendingConfig)
if err := dn.finalizeBeforeReboot(state.pendingConfig); err != nil {
return err
}
return dn.reboot(fmt.Sprintf("Node will reboot into config %v", state.pendingConfig.GetName()))
}

if err := dn.detectEarlySSHAccessesFromBoot(); err != nil {
Expand Down Expand Up @@ -1043,7 +1054,10 @@ func (dn *Daemon) checkStateOnFirstRun() error {
if err := os.RemoveAll(osImageContentDir); err != nil {
return err
}
return dn.finalizeAndReboot(state.currentConfig)
if err := dn.finalizeBeforeReboot(state.currentConfig); err != nil {
return err
}
return dn.reboot(fmt.Sprintf("Node will reboot into config %v", state.currentConfig.GetName()))
}
glog.Info("No bootstrap pivot required; unlinking bootstrap node annotations")

Expand Down Expand Up @@ -1103,33 +1117,44 @@ func (dn *Daemon) checkStateOnFirstRun() error {
return dn.triggerUpdateWithMachineConfig(state.currentConfig, state.desiredConfig)
}

// We've validated our state. In the case where we had a pendingConfig,
// make that now currentConfig. We update the node annotation, delete the
// state file, etc.
//
// However, it may be the case that desiredConfig changed while we
// were coming up, so we next look at that before uncordoning the node (so
// we don't uncordon and then immediately re-cordon)
// We've validated state. Now, ensure that node is in desired state
var inDesiredConfig bool
if inDesiredConfig, err = dn.updateConfigAndState(state); err != nil {
return err
}
if inDesiredConfig {
return nil
}

if dn.recorder != nil {
dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeNormal, "BootResync", fmt.Sprintf("Booting node %s, currentConfig %s, desiredConfig %s", dn.node.Name, state.currentConfig.GetName(), state.desiredConfig.GetName()))
}
// currentConfig != desiredConfig, and we're not booting up into the desiredConfig.
// Kick off an update.
return dn.triggerUpdateWithMachineConfig(state.currentConfig, state.desiredConfig)
}

// updateConfigAndState updates node to desired state, labels nodes as done and uncordon
func (dn *Daemon) updateConfigAndState(state *stateAndConfigs) (bool, error) {
// In the case where we had a pendingConfig, make that now currentConfig.
// We update the node annotation, delete the state file, etc.
if state.pendingConfig != nil {
if dn.recorder != nil {
dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeNormal, "NodeDone", fmt.Sprintf("Setting node %s, currentConfig %s to Done", dn.node.Name, state.pendingConfig.GetName()))
}
if err := dn.nodeWriter.SetDone(dn.kubeClient.CoreV1().Nodes(), dn.nodeLister, dn.name, state.pendingConfig.GetName()); err != nil {
return errors.Wrap(err, "error setting node's state to Done")
return true, errors.Wrap(err, "error setting node's state to Done")
}
if out, err := dn.storePendingState(state.pendingConfig, 0); err != nil {
return errors.Wrapf(err, "failed to reset pending config: %s", string(out))
return true, errors.Wrapf(err, "failed to reset pending config: %s", string(out))
}

state.currentConfig = state.pendingConfig
}

if state.bootstrapping {
if err := dn.storeCurrentConfigOnDisk(state.currentConfig); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this no longer needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch, seems like it got lost during function split up.

return err
}
}

// In case of node reboot, it may be the case that desiredConfig changed while we
// were coming up, so we next look at that before uncordoning the node (so
// we don't uncordon and then immediately re-cordon)
inDesiredConfig := state.currentConfig.GetName() == state.desiredConfig.GetName()
if inDesiredConfig {
if state.pendingConfig != nil {
Expand All @@ -1138,7 +1163,7 @@ func (dn *Daemon) checkStateOnFirstRun() error {
glog.Infof("Completing pending config %s", state.pendingConfig.GetName())
if err := dn.completeUpdate(dn.node, state.pendingConfig.GetName()); err != nil {
MCDUpdateState.WithLabelValues("", err.Error()).SetToCurrentTime()
return err
return inDesiredConfig, err
}
}
// If we're degraded here, it means we got an error likely on startup and we retried.
Expand All @@ -1147,22 +1172,16 @@ func (dn *Daemon) checkStateOnFirstRun() error {
if err := dn.nodeWriter.SetDone(dn.kubeClient.CoreV1().Nodes(), dn.nodeLister, dn.name, state.currentConfig.GetName()); err != nil {
errLabelStr := fmt.Sprintf("error setting node's state to Done: %v", err)
MCDUpdateState.WithLabelValues("", errLabelStr).SetToCurrentTime()
return errors.Wrap(err, "error setting node's state to Done")
return inDesiredConfig, errors.Wrap(err, "error setting node's state to Done")
}
}

glog.Infof("In desired config %s", state.currentConfig.GetName())
MCDUpdateState.WithLabelValues(state.currentConfig.GetName(), "").SetToCurrentTime()

// All good!
return nil
}
if dn.recorder != nil {
dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeNormal, "BootResync", fmt.Sprintf("Booting node %s, currentConfig %s, desiredConfig %s", dn.node.Name, state.currentConfig.GetName(), state.desiredConfig.GetName()))
}
// currentConfig != desiredConfig, and we're not booting up into the desiredConfig.
// Kick off an update.
return dn.triggerUpdateWithMachineConfig(state.currentConfig, state.desiredConfig)
return inDesiredConfig, nil
}

// runOnceFromMachineConfig utilizes a parsed machineConfig and executes in onceFrom
Expand Down
70 changes: 64 additions & 6 deletions pkg/daemon/update.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,56 @@ func getNodeRef(node *corev1.Node) *corev1.ObjectReference {
}
}

// finalizeAndReboot is the last step in an update(), and it can also
// be called as a special case for the "bootstrap pivot".
func (dn *Daemon) finalizeAndReboot(newConfig *mcfgv1.MachineConfig) (retErr error) {
func reloadCrioConfig() error {
_, err := runGetOut("pkill", "-HUP", "crio")
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice if we could use systemctl reload crio.service instead. That way, crio can specify how exactly it needs to be reloaded (ExecReload) without the MCD needing to know the details. I don't think this will work today, since crio.service doesn't define an ExecReload (systemd will restart the service in this case).

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed but I think for this release we are ok with this operation as it stands

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As Jerry said we can but we are trying to keep impact as minimal as possible. Since, crio supports live configuration reload for container registry, we are making use of it in our usecase.

return err
}

// performRebootAction takes action based on what rebootAction has been asked.
// For non-reboot action, it applies configuration, updates node's config and state.
// In the end uncordon node to schedule workload.
// If at any point an error occurs, we reboot the node so that node has correct configuration.
func (dn *Daemon) performRebootAction(action rebootAction, configName string) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

I would rename this so that it doesn't include "reboot" in the name (since we might not be rebooting). I'm also not sure that an enum is going to be expressive enough (feel free to tell me to stop over-complicating things and to come back in six months). I had envisioned this being expressed as a list of actions that needed to be taken (e.g. [ ] (rebootActionNone), [ reboot ] (rebootActionReboot), [ reload crio.service, reload foobar.service] (rebootActionReloadCrio + reload foobar) so that we could build up the list as we enumerate the diffed config (e.g. any changes to a particular set of paths triggers a reload of crio, any unrecognized change triggers a reboot).

Copy link
Contributor

Choose a reason for hiding this comment

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

I've changed it to "PostConfigChangeAction" in an attempt to be more descriptive but that doesn't quite feel right either. Naming open to suggestions.

As for the enum, I've changed it to a list. I initially created the enum as a skeleton structure since we were handling very limited cases, but I agree that parsing a list will be needed in the future. I've updated that at: #2259

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree with better regrouping of actions.

switch action {
case rebootActionNone:
dn.logSystem("Node has Desired Config %s, skipping reboot", configName)
case rebootActionReloadCrio:
if err := reloadCrioConfig(); err != nil {
dn.logSystem("Reloading crio configuration failed, rebooting: %v", err)
dn.reboot(fmt.Sprintf("Node will reboot into config %s", configName))
Copy link
Contributor

Choose a reason for hiding this comment

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

One thing I'm debating is whether we want to hard-reboot here or block + degrade. It seems to me that if we are explicitly supporting "rebootless updates" that we should not reboot unexpectedly because of a failure. We should probably do e.g. what we do when a file write fails. This might also allow us not to drain (although recovery is another thing we need to consider) What do you think @sinnykumari ?

Copy link
Contributor

Choose a reason for hiding this comment

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

f you degrade, the customer can't really recover from it and do something to make it work on a retry... so degrading might mean that it can't proceed. not sure how that cuts but since it's an optimization it feels like rebooting should be the fall back.

Copy link
Contributor

Choose a reason for hiding this comment

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

(at least to me rebootless upgrades isn't a promise, it's a best effort optimization in certain cases where it can be safely done)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can but isn't it better to reboot (crio will restart on reboot) and fix the problem by itself rather than degrade? I think we are not promising rebootless update rather it is a best effort basis i.e. MCO tries its best not to reboot for certain config change but if an error occur it reboots. As this is new feature, we can frame documentation accordingly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can definitely become smarter in future.

Copy link
Contributor

Choose a reason for hiding this comment

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

ha jinx! sinny and i both wrote our responses at the same time 😆

}
dn.logSystem("crio config reloaded successfully! Desired config %s has been applied, skipping reboot", configName)
default:
// Defaults to rebooting node
dn.logSystem("Rebooting node")
return dn.reboot(fmt.Sprintf("Node will reboot into config %s", configName))
}

// We are here, which means reboot was not needed to apply the configuration.

// Get current state of node, in case of an error reboot
state, err := dn.getStateAndConfigs(configName)
if err != nil {
glog.Errorf("Error processing state and configs, rebooting: %v", err)
return dn.reboot(fmt.Sprintf("Node will reboot into config %s", configName))
}

var inDesiredConfig bool
if inDesiredConfig, err = dn.updateConfigAndState(state); err != nil {
glog.Errorf("Setting node's state to Done failed, rebooting: %v", err)
return dn.reboot(fmt.Sprintf("Node will reboot into config %s", configName))
}
if inDesiredConfig {
return nil
}

// currentConfig != desiredConfig, kick off an update
return dn.triggerUpdateWithMachineConfig(state.currentConfig, state.desiredConfig)
}

// finalizeBeforeReboot is the last step in an update() and then we take appropriate rebootAction.
// It can also be called as a special case for the "bootstrap pivot".
func (dn *Daemon) finalizeBeforeReboot(newConfig *mcfgv1.MachineConfig) (retErr error) {
if out, err := dn.storePendingState(newConfig, 1); err != nil {
return errors.Wrapf(err, "failed to log pending config: %s", string(out))
}
Expand All @@ -114,8 +161,7 @@ func (dn *Daemon) finalizeAndReboot(newConfig *mcfgv1.MachineConfig) (retErr err
dn.recorder.Eventf(getNodeRef(dn.node), corev1.EventTypeNormal, "PendingConfig", fmt.Sprintf("Written pending config %s", newConfig.GetName()))
}

// reboot. this function shouldn't actually return.
return dn.reboot(fmt.Sprintf("Node will reboot into config %v", newConfig.GetName()))
return nil
}

func (dn *Daemon) drain() error {
Expand Down Expand Up @@ -515,7 +561,19 @@ func (dn *Daemon) update(oldConfig, newConfig *mcfgv1.MachineConfig) (retErr err
glog.Info("Updated kernel tuning arguments")
}

return dn.finalizeAndReboot(newConfig)
if err := dn.finalizeBeforeReboot(newConfig); err != nil {
return err
}

// TODO: Need Jerry's work to determine exact reboot action
var action rebootAction
action = dn.getRebootAction()
return dn.performRebootAction(action, newConfig.GetName())
}

func (dn *Daemon) getRebootAction() rebootAction {
// Until we have logic, always reboot
return rebootActionReboot
}

// removeRollback removes the rpm-ostree rollback deployment. It
Expand Down