pkg/daemon: allow reboots from external componenents #576

runcom · 2019-03-25T18:13:37Z

This is a temporary stopgap to allow other componenents ask for a reboot
since the MCD is the one in charge of draining a node and rebooting it.

The logic is simple with this:

if an update is already coming, we just go through the update and if
successful, the machine is gonna reboot just fine
if we're not updating through MachineConfigs, then we check for a file
and drain+reboot
the "reboot file" is the removed on MCD boot

Signed-off-by: Antonio Murdaca [email protected]

openshift-ci-robot · 2019-03-25T18:13:52Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: runcom

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [runcom]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

runcom · 2019-03-25T18:15:56Z

/hold

cc @ashcrow @crawford

cgwalters · 2019-03-25T18:16:54Z

pkg/daemon/daemon.go

@@ -319,9 +319,15 @@ func (dn *Daemon) bootstrapNode() error {
 		return err
 	}
 	dn.node = node
+	if _, err := os.Stat("/etc/defaults/rhcos/reboot-needed"); err == nil {


IMO, /etc/defaults is a legacy of SySV init; no reason to put anything new there.

The right place for this is more likely /run/machine-config-operator/reboot-needed or so. (We also have /etc/machine-config-daemon used for /etc/machine-config-daemon/state.json but I think /run makes more sense since it goes away automatically if the machine is rebooted)

cc @ashcrow

ashcrow · 2019-03-25T18:16:47Z

pkg/daemon/daemon.go

@@ -400,6 +406,15 @@ func (dn *Daemon) syncNode(key string) error {
 				return err
 			}
 		}
+		// FIXME: this is a temporary workaround, remove once an MCD signaling system is in place


ashcrow · 2019-03-25T18:16:57Z

pkg/daemon/daemon.go

@@ -400,6 +406,15 @@ func (dn *Daemon) syncNode(key string) error {
 				return err
 			}
 		}
+		// FIXME: this is a temporary workaround, remove once an MCD signaling system is in place
+		if _, err := os.Stat("/etc/defaults/rhcos/reboot-needed"); err == nil {


cgwalters · 2019-03-25T18:19:51Z

What other components are going to be asking for a reboot?

This is a temporary stopgap to allow other componenents ask for a reboot since the MCD is the one in charge of draining a node and rebooting it. The logic is simple with this: - if an update is already coming, we just go through the update and if successful, the machine is gonna reboot just fine - if we're not updating through MachineConfigs, then we check for a file and drain+reboot - the "reboot file" is the removed on MCD boot Signed-off-by: Antonio Murdaca <[email protected]>

runcom · 2019-03-25T18:21:16Z

What other components are going to be asking for a reboot?

hopefully just the one @ashcrow needs for 4.1

ashcrow · 2019-03-25T18:22:18Z

Yeah, it's just RHCOS and Installer team for 4.1 (same use case and underlying tool)

Context: https://url.corp.redhat.com/bc7c76c

cgwalters · 2019-03-25T18:27:32Z

I don't understand why that work isn't driven by the MCD. We already have other needs to adjust kernel arguments (per #388 ) - and further it's going to be problematic to have some other service running rpm-ostree kargs which will adjust deployments at possibly the same time as the MCD is possibly executing pivot.

As is today AFAICS it will race; there's no ordering relationship between that service and pivot.service which runs to do the early pivot.

Further it overlaps heavily with ostreedev/ostree#479 which Robert is working on to aid in dropping Anaconda and removing the dependency on grub2 with ostree in general which will solve various other issues.

cgwalters · 2019-03-25T18:40:38Z

Short term hack, we can make this new service Before=pivot.service to address the race.

ashcrow · 2019-03-25T18:45:39Z

@cgwalters I'll update the host level service with

Before=pivot.service

cgwalters · 2019-03-25T18:56:09Z

Remember that the "early pivot" happens before the node has joined the cluster. So the MCD isn't involved here - no need to drain anything.

Can we de-scope this from "arbitrary kernel arguments are supported changing at any time" to "If /etc/openshift-nosmt is present during ConditionFirstBoot, then nosmt is added to kernel arguments` at "early pivot" time?

Today, we can assume that pivot.service will itself cause a reboot, so kernel args handling doesn't need to additionally do so.

Maybe the simplest is actually to add this to pivot.service itself as /etc/pivot/kernel-args. It's already in a place to do all of this transactionally anyways.

(Though that leaves the "BYO host" scenario, but will they really be expecting us to inject kernel args and reboot traditional hosts? This seems like more in the territory of "you brought it, you manage it"?)

cgwalters · 2019-03-25T19:12:49Z

To elaborate on this a bit...it looks like the current prototype service runs every boot. That greatly increases the scope; if we only have to care about it during early pivot, thereafter the nosmt kernel argument will be correctly carried forwards across upgrades etc. We don't have a concern about the "source of truth" for kernel arguments. (The source of truth is the BLS config files as is the original ostree design - while BLS didn't take over the world it's at least a somewhat agreed upon place for kernel arguments implemented by multiple bootloaders. Invoking rpm-ostree kargs is unfortunately still necessary though to regenerate the bootloader config)

ashcrow · 2019-03-25T19:18:26Z

I'm OK with the above proposal for the near term. I'm going to bounce it off other folks who were part of the design before we commit to change.

The POC today runs whenever the /etc/default/rhcos/kargs file changes on an RHCOS or RHEL machine. This can be right after boot, or whenever the restricted file is written to.

ashcrow · 2019-03-25T19:52:02Z

/cc @abhinavdahiya

abhinavdahiya · 2019-03-25T20:21:26Z

/cc @abhinavdahiya

I'm fine if we support nosmt thorugh a special file.. but I would like decide the file location as that would unblock openshift/installer#1392 .

ashcrow · 2019-03-25T20:50:10Z

Can we de-scope this from "arbitrary kernel arguments are supported changing at any time" to "If /etc/openshift-nosmt is present during ConditionFirstBoot, then nosmt is added to kernel arguments` at "early pivot" time?

I spoke with others and they are cool with this for 4.1. How about /etc/pivot/kernel-args. Whatever is put in there will be applied via rpm-ostree kargs? The file change will come from one of the following locations at a time:

openshift-installer
MCD (via an MC)

If this sounds good I'll shelve my system service API for now and throw something together starting in the morning.

runcom · 2019-03-26T12:28:45Z

I spoke with others and they are cool with this for 4.1. How about /etc/pivot/kernel-args. Whatever is put in there will be applied via rpm-ostree kargs? The file change will come from one of the following locations at a time:

openshift-installer

MCD (via an MC)

isn't this allowing "arbitrary kernel arguments"? for 4.1, we just want nosmt to take effect at early pivot.

For the future, then yeah, I agree with what you wrote, that can be a path towards allowing general and arbitrary kargs.

ashcrow · 2019-03-26T12:57:17Z

Isn't this allowing "arbitrary kernel arguments"?

It can be expanded to be arbitrary. For now it will only be the subset for tuning.

for 4.1, we just want nosmt to take effect at early pivot.

Almost. It needs to be in early pivot or toggled later.

ashcrow · 2019-03-26T13:03:55Z

@runcom thank you for putting this PR together, but I think it can be closed. I'll hopefully have something up today or tomorrow in it's place.

runcom · 2019-03-26T13:26:26Z

Thanks!!!

cgwalters · 2019-03-26T13:28:52Z

It needs to be in early pivot or toggled later.

If we really need to support toggling it later I don't see how we avoid implementing #388 right?

ashcrow · 2019-03-26T13:35:01Z

If we really need to support toggling it later I don't see how we avoid implementing #388 right?

For a later version, yes. But if we scope this to the few tuning parameters that are needed we the current MCO/MC and update to pivot should work. The installer can lay down a file at install time as needed and, if the cluster admin wants to modify the supported tuning options, they can lay down an updated /etc/pivot/kernel-args (which will only white list the options we're allowing for now).

ashcrow · 2019-03-26T15:32:43Z

~~Actually, I see what you mean now @cgwalters. Since pivot ONLY runs when MCD gets a new machine-os-content.~~

Checks /etc/pivot/kernel-args for argument changes. This early version only supports bare arguments that are in the whitelist. See conversation at openshift/machine-config-operator#576 Signed-off-by: Steve Milner <[email protected]>

openshift-ci-robot requested review from jlebon and kikisdeliveryservice March 25, 2019 18:13

openshift-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Mar 25, 2019

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 25, 2019

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 25, 2019

cgwalters reviewed Mar 25, 2019

View reviewed changes

ashcrow reviewed Mar 25, 2019

View reviewed changes

runcom force-pushed the queue-reboots branch from 0431ab1 to 147e607 Compare March 25, 2019 18:18

runcom force-pushed the queue-reboots branch from 147e607 to 97eacbd Compare March 25, 2019 18:20

openshift-ci-robot requested a review from abhinavdahiya March 25, 2019 19:52

runcom closed this Mar 26, 2019

runcom deleted the queue-reboots branch March 26, 2019 13:59

ashcrow mentioned this pull request Mar 26, 2019

Add basic kernel tuning functionality openshift/pivot#47

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/daemon: allow reboots from external componenents #576

pkg/daemon: allow reboots from external componenents #576

runcom commented Mar 25, 2019

openshift-ci-robot commented Mar 25, 2019

runcom commented Mar 25, 2019

cgwalters Mar 25, 2019

runcom Mar 25, 2019

ashcrow Mar 25, 2019

ashcrow Mar 25, 2019

cgwalters commented Mar 25, 2019

runcom commented Mar 25, 2019

ashcrow commented Mar 25, 2019 •

edited

Loading

cgwalters commented Mar 25, 2019

cgwalters commented Mar 25, 2019

ashcrow commented Mar 25, 2019 •

edited

Loading

cgwalters commented Mar 25, 2019

cgwalters commented Mar 25, 2019

ashcrow commented Mar 25, 2019

ashcrow commented Mar 25, 2019

abhinavdahiya commented Mar 25, 2019 •

edited

Loading

ashcrow commented Mar 25, 2019

runcom commented Mar 26, 2019

ashcrow commented Mar 26, 2019

ashcrow commented Mar 26, 2019

runcom commented Mar 26, 2019

cgwalters commented Mar 26, 2019

ashcrow commented Mar 26, 2019

ashcrow commented Mar 26, 2019 •

edited

Loading

pkg/daemon: allow reboots from external componenents #576

pkg/daemon: allow reboots from external componenents #576

Conversation

runcom commented Mar 25, 2019

openshift-ci-robot commented Mar 25, 2019

runcom commented Mar 25, 2019

cgwalters Mar 25, 2019

Choose a reason for hiding this comment

runcom Mar 25, 2019

Choose a reason for hiding this comment

ashcrow Mar 25, 2019

Choose a reason for hiding this comment

ashcrow Mar 25, 2019

Choose a reason for hiding this comment

cgwalters commented Mar 25, 2019

runcom commented Mar 25, 2019

ashcrow commented Mar 25, 2019 • edited Loading

cgwalters commented Mar 25, 2019

cgwalters commented Mar 25, 2019

ashcrow commented Mar 25, 2019 • edited Loading

cgwalters commented Mar 25, 2019

cgwalters commented Mar 25, 2019

ashcrow commented Mar 25, 2019

ashcrow commented Mar 25, 2019

abhinavdahiya commented Mar 25, 2019 • edited Loading

ashcrow commented Mar 25, 2019

runcom commented Mar 26, 2019

ashcrow commented Mar 26, 2019

ashcrow commented Mar 26, 2019

runcom commented Mar 26, 2019

cgwalters commented Mar 26, 2019

ashcrow commented Mar 26, 2019

ashcrow commented Mar 26, 2019 • edited Loading

ashcrow commented Mar 25, 2019 •

edited

Loading

ashcrow commented Mar 25, 2019 •

edited

Loading

abhinavdahiya commented Mar 25, 2019 •

edited

Loading

ashcrow commented Mar 26, 2019 •

edited

Loading