Add pivot.service as a host API

[jlebon]

Add a new systemd service that simply executes pivot. But note that we
don't have a pivot.path unit to auto-trigger it: this needs to be
triggered explicitly by whatever drives pivot. Making it a service and
standizing on a path in /etc means that we can trigger a pivot on boot
from Ignition (patch to preset enable the service is pending), as well
as from the MCD by directly starting the unit.

By running it directly from the host context rather than e.g. directly
by the MCD, we also avoid potential issues with SELinux as described in:
openshift/machine-config-operator#314

[ashcrow]

lint is unhappy and there looks to be a missing in file referenced.

[jlebon]

Ahh yes, the good ol' "forgot to git add" phenomenon.

[jlebon]

OK, split the Makefile & spec tweaks in #26 for now.

[jlebon]

So, the downside with this approach is that it's trickier for the MCD to keep track of what's happening during pivot. It'd have to do something like:

1. write to /etc/pivot/image-pull-spec
2. wait for service to start (use systemd API?)
3. optional: proxy journal logs (use journal API?)
4. wait for service to end and check if it ended successfully or not (use systemd API?)

One thing we could do (if we're going to be writing godbus code anyway) is add a D-Bus interface to pivot and make it D-Bus activated. Then, the MCD could do a synchronous method call with the exact osImageURL it wants and get much cleaner error-handling & logging.

We'd still need something like what this patch is doing though for the pivot on first boot case. (In fact, another reason I'm thinking about a D-Bus interface is that this latter case wants --reboot, whereas the MCD wants to be in control of the reboot).

[cgwalters]

One thing we could do (if we're going to be writing godbus code anyway) is add a D-Bus interface to pivot and make it D-Bus activated.

Eek. I would like to try to do the minimum possible we need to work here right now. So my vote is less good logging and smaller more obviously correct patch.

[cgwalters]

Maybe one approach for the reboot handling is to have pivot just create a /run/pivot-reboot-needed file if one is needed. Then we can have a separate transient unit in the "early pivot" path that reacts to it. The MCD case would ignore it?

[cgwalters]

(I'm not fully against a DBus interface, but I personally don't have much experience implementing them in Go. Actually this doesn't even need to be DBus, we can use whatever other local IPC we want over a socket; there may be more convenient ones? HTTP-over-Unix-socket like Docker is a well known path...)

[cgwalters]

wait for service to start (use systemd API?)

Maybe drop the .path unit and use systemctl start --wait pivot.service
Nope...argh --wait isn't in rhel7 systemd 😢

[cgwalters]

OK my current thought is to add a .socket unit ListenSequentialPacket and have pivot support socket activation; the packet (via SOCK_SEQPACKET) is a NUL separated array of strings that are the same as the current command line. Our response is one other packet which should be a single string success/failure; other logs go to the journal?

Then in the MCD it's just connecting to that unix socket and writing the args.

[jlebon]

Hmm, OK one way to make this "synchronous" is we drop the .path unit and the MCD does:

1. drop file in /etc/...
2. systemd Subscribe()
3. systemd StartUnit()
4. wait for JobRemoved() signal and inspect result string

The same unit can still cover the pivot on first boot path.

Maybe one approach for the reboot handling is to have pivot just create a /run/pivot-reboot-needed file if one is needed. Then we can have a separate transient unit in the "early pivot" path that reacts to it. The MCD case would ignore it?

Or tweak it slightly: make the Ignition config drop a /run/pivot-reboot-needed and pivot reboots based on that (or deletes it if no pivot is needed).

[ashcrow]

I'd like to have pivot be as simple as possible while still providing the required functionality. I'm not against adding dbus into the mix, but only if it fixes a problematic bug or adds a feature we need.

[cgwalters]

Here's another thought; the MCD can easily ship code to run directly on the host. We just embed whatever binary we want, copy it to the host and systemd-run it. In fact we could even move the pivot functionality into the MCD binary itself and have it write itself to /rootfs/run/machine-config-daemon-binary and then pivot is systemd-run /rootfs/run/machine-config-daemon-binary pivot ....

[jlebon]

Hmm, yeah that's true. Though the main issue I'm trying to solve here is waiting and error-handling. Doing systemd-run is not much different from just dropping a file in that respect, right? Unless I'm misunderstanding something.

The StartUnit() approach returns a specific Job object we can match against JobRemoved() signals. So we know exactly when the job was started (we did!) and when it finished, as well as the exit status (it's in the JobRemoved() signal), so we can e.g. have some retry logic etc...

[jlebon]

(That's not incompatible in any way with the idea of merging pivot into the MCD of course.)

[jlebon]

Oh wow, just realized we're already vendoring this in MCO for the login1 stuff: https://github.com/coreos/go-systemd/blob/master/dbus/methods.go.

[cgwalters]

MCD can easily ship code to run directly on the host.

🤔 more while I think it's a nice idea in general actually for this case it wouldn't work because we need this code also for the early-pivot case before we've even pulled down the MCD. And while it's kind of tempting to embed the MCD container in the host by default...let's not go down that road right now.

[cgwalters]

The StartUnit() approach returns a specific Job object we can match against JobRemoved() signals. So we know exactly when the job was started (we did!) and when it finished, as well as the exit status (it's in the JobRemoved() signal), so we can e.g. have some retry logic etc...

OK yep.

[jlebon]

OK, I updated this! I still stuck with the systemd approach even though now that the MCD will take care of the initial pivot, I'd like to leave the door open for the ability to pivot immediately on boot before joining the cluster. The service unit here allows us to do this by configuring it from Ignition.

[jlebon]

OK, I now rebased this on top of #29. Edited first post with updated commit message.

Still testing the MCD path.
/hold

[jlebon]

OK, this is ready for review now! Successfully tested with openshift/machine-config-operator#335.
/hold cancel

[jlebon]

Related: openshift/redhat-release-coreos#19

[ashcrow]

@jlebon is this ready for updated reviews and possibly merge?

[jlebon]

This is now blocked on coreos/rpm-ostree#1732.

[jlebon]

OK, this is now ready for final review! Together with openshift/machine-config-operator#335 and coreos/rpm-ostree#1732, I successfully tested a rebase to ootpa.

This can go in now, but of course we'll have to wait for a new RHCOS before we actually make use of it in the MCD.

[ashcrow]

Two notes/comments, otherwise 👍

[ashcrow]

LGTM

[ashcrow]

@yuqi-zhang Shortly after this PR a new release will be cut.

[jlebon]

Latest RHCOS now includes coreos/rpm-ostree#1732! Let's get this in soon-ish? I'd like to have all this bake for a bit before the end of the week.

[yuqi-zhang]

Do you know if that rpm-ostree change has also propagated to brew? I'd like to pull this into the new internal build for pivot before end of week as well.

[jlebon]

Do you know if that rpm-ostree change has also propagated to brew?

Not yet. Will either backport or do a new release soon for it!

[cgwalters]

LGTM

[jlebon]

Do you know if that rpm-ostree change has also propagated to brew?

Not yet. Will either backport or do a new release soon for it!

This is done now.

[jlebon]

Thanks for the reviews, merging!