Skip to content

daemon: Use systemd-run, not machine-config-daemon-host.service #1803

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

Closed
cgwalters wants to merge 1 commit into from
Closed

daemon: Use systemd-run, not machine-config-daemon-host.service #1803

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
26 changes: 10 additions & 16 deletions pkg/daemon/rpm-ostree.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -322,23 +322,16 @@ func (r *RpmOstreeClient) RunPivot(osImageURL string) error {
return errors.Wrap(err, "deleting pivot reboot-needed file")
}

service := "machine-config-daemon-host.service"
// We need to use pivot if it's there, because machine-config-daemon-host.service
// currently has a ConditionPathExists=!/usr/lib/systemd/system/pivot.service to
// avoid having *both* pivot and MCD try to update. This code can be dropped
// once we don't need to care about compat with older RHCOS.
var err error
_, err = os.Stat("/usr/lib/systemd/system/pivot.service")
// We used to start machine-config-daemon-host here, but now we make a dynamic
// unit because that service was started in too many ways, and the systemd-run
// model of creating a unit dynamically is much clearer for what we want here;
// conceptually the service is just a dynamic child of this pod (if we could we'd
// tie the lifecycle together). Further, let's shorten our systemd unit names
// by using the mco- prefix, and we also inject the RPMOSTREE_CLIENT_ID now.
err := exec.Command("systemd-run", "--wait", "--collect", "--unit=mco-pivot",
"-E", "RPMOSTREE_CLIENT_ID=mco", "/usr/libexec/machine-config-daemon", "pivot", osImageURL).Run()
Copy link
Contributor

@sinnykumari sinnykumari Jun 10, 2020

Choose a reason for hiding this comment

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

umm, so we are directly calling m-c-d pivot here which means in case of an update pivot will run systemctl reboot (https://github.com/openshift/machine-config-operator/blob/master/cmd/machine-config-daemon/pivot.go#L253) . Can this cause system to reboot without executing remaining control flow?

Copy link
Member Author

@cgwalters cgwalters Jun 10, 2020

Choose a reason for hiding this comment

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

My original theory that somehow the pivot code was rebooting turned out to be wrong. That code only reboots if a CLI argument is passed or if a stamp file exists, but just above in this function we unlink it to be sure:

	// This is written by code injected by the MCS, but we always want the MCD to be in control of reboots
	if err := os.Remove("/run/pivot/reboot-needed"); err != nil && !os.IsNotExist(err) {
		return errors.Wrap(err, "deleting pivot reboot-needed file")
	}

Copy link
Member Author

@cgwalters cgwalters Jun 10, 2020

Choose a reason for hiding this comment

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

Also I think we could just remove that pivot reboot code now too. Nothing should be using it.

Copy link
Contributor

@sinnykumari sinnykumari Jun 10, 2020

Choose a reason for hiding this comment

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

Right, it won't have /run/pivot/reboot-needed file.

Copy link
Contributor

@sinnykumari sinnykumari Jun 10, 2020

Choose a reason for hiding this comment

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

Curious, what is the use of passing osImageURL to m-c-d pivot, does it get processed?

if err != nil {
if !os.IsNotExist(err) {
return errors.Wrapf(err, "checking pivot service")
}
} else {
service = "pivot.service"
}
err = exec.Command("systemctl", "start", service).Run()
if err != nil {
return errors.Wrapf(err, "failed to start %s", service)
return errors.Wrapf(err, "failed to run pivot")
}
return nil
}
Expand All @@ -349,6 +342,7 @@ func followPivotJournalLogs(stopCh <-chan time.Time) {
cmd := exec.Command("journalctl", "-f", "-b", "-o", "cat",
"-u", "rpm-ostreed",
"-u", "pivot",
Copy link
Contributor

@sinnykumari sinnykumari Jun 10, 2020

Choose a reason for hiding this comment

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

since we are removing pivot service, does it make sense to remove from logging?

"-u", "mco-pivot",
"-u", "machine-config-daemon-host")
Copy link
Contributor

@sinnykumari sinnykumari Jun 10, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Do we need this since mco-pivot replaces machine-config-daemon-host service

cmd.Stdout = os.Stdout
cmd.Stderr = os.Stderr
Expand Down