Skip to content

Don't enable machine-config-daemon-host.service by default #1366

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

Jump to bottom
Merged
openshift-merge-robot merged 1 commit into from
Jan 22, 2020
Merged

Don't enable machine-config-daemon-host.service by default #1366

openshift-merge-robot merged 1 commit into from
Jan 22, 2020

Conversation

@cgwalters
Copy link
Member

@cgwalters cgwalters commented Jan 10, 2020 •
edited
Loading

Today the MCS serves both /etc/pivot/image-pullspec and
/etc/ignition-machine-config-encapsulated.json - and we have
to serve both so that using 4.1 bootimages works.

However, we absolutely shouldn't start both services on a modern
4.3+ system on firstboot - they will race, both trying to update,
and one doing a reboot.

Only enable the -firstboot.service which will itself currently
invoke -host.service, though in the future we should clean this
up by having the firstboot code run rpm-ostree directly, since
we're already on the host.

Motivated by discussions around RT kernel handling.

@openshift-ci-robot openshift-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jan 10, 2020
@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

I didn't test that we're running both today yet, but @sinnykumari said she'd seen it. Investigating too.

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 10, 2020
@kikisdeliveryservice
Copy link
Contributor

kikisdeliveryservice commented Jan 10, 2020

Logic makes sense to me, we'd have to backport this to 4.3 right?

@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

Oh yeah, they're totally racing today. Look at the master journal here from an aws run on the main openshift-release (not this PR):

Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: Started RPM-OSTree System Management Daemon.
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: client(id:cli dbus:1.9 unit:machine-config-daemon-firstboot.service uid:0) added; new total=1
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: client(id:cli dbus:1.10 unit:machine-config-daemon-host.service uid:0) added; new total=2
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: client(id:cli dbus:1.9 unit:machine-config-daemon-firstboot.service uid:0) vanished; remaining=1
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: client(id:cli dbus:1.10 unit:machine-config-daemon-host.service uid:0) vanished; remaining=0
...
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2245]: I0110 00:06:48.766882    2245 run.go:16] Running: podman pull -q --authfile /var/lib/kubelet/config.json quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:3bada34ebed01542891c576954844afa164f087b6d8081e23f6f1724600b1f2e
Jan 10 00:06:48 ip-10-0-139-114 machine-config-daemon[2242]: I0110 00:06:48.766882    2245 run.go:16] Running: podman pull -q --authfile /var/lib/kubelet/config.json quay.io/openshift-release-dev/ocp-v4.0-art-dev@sha256:3bada34ebed01542891c576954844afa164f087b6d8081e23f6f1724600b1f2e

We're pulling the image twice, etc! Eww! Let's get this in!

@jlebon
Copy link
Member

jlebon commented Jan 10, 2020

Hmm, I think what's going on here is just that the -host service is how the MCD runs the pivot on the host, no?

Notice the timestamps matching; I think it's just from the -firstboot service proxying the journal.

@jlebon
Copy link
Member

jlebon commented Jan 10, 2020

IOW, -firstboot sees it needs to pivot, and runs the -host service to actually do it.

One thing we need to make sure though is that the -host service isn't actually getting enabled by Ignition.

@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

Hmm, I think what's going on here is just that the -host service is how the MCD runs the pivot on the host, no?

OK you really made me pause here and verify. But no. The firstboot service doesn't do the "systemctl start" dance. That bit is only in the MCD. (When I say "MCD", I mean the thing running as a daemonset)

Look at e.g.

Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: Started coreos-regenerate-iscsi-initiatorname.service.
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: mcd-write-pivot-reboot.service: Consumed 3ms CPU time
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: Starting Machine Config Daemon Firstboot...
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal chronyd[2244]: chronyd version 3.5 starting (+CMDMON +NTP +REFCLOCK +RTC +PRIVDROP +SCFILTER +SIGND +ASYNCDNS +SECHASH +IPV6 +DEBUG)
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: Starting Machine Config Daemon Initial...
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal chronyd[2244]: Using right/UTC timezone to obtain leap second data
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: Started OpenSSH ed25519 Server Key Generation.
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: [email protected]: Consumed 21ms CPU time
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: Started OpenSSH ecdsa Server Key Generation.
Jan 10 00:06:45 ip-10-0-139-114.ec2.internal systemd[1]: [email protected]: Consumed 17ms CPU time

Both services were started up quite early on.

@jlebon
Copy link
Member

jlebon commented Jan 10, 2020

OK you really made me pause here and verify. But no. The firstboot service doesn't do the "systemctl start" dance.

Hmm OK, that's weird. Reading the code, here's the stack I see:

  • RunFirstbootCompleteMachineconfig ->
  • dn.update ->
  • dn.updateOSAndReboot ->
  • dn.updateOS ->
  • RunPivot
  • err = exec.Command("systemctl", "start", service).Run()

right? (But full disclosure, I didn't actually run this code.)

Both services were started up quite early on.

Ahh right, that's what I mean by:

One thing we need to make sure though is that the -host service isn't actually getting enabled by Ignition.

I.e. it seems like we should just have the -host service not be enabled at all and just manually started via systemctl start for the daemonset to use?

And then also we can have -firstboot indeed directly do the pivot since it's running on the host anyway.

@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

OK I think you're right and I was wrong - the "don't systemctl in -firstboot" was something I had planned to do and had in my head we were doing.

We're still starting both services though. One thing I quickly checked is that systemctl start foo.service on a oneshot wait for the outstanding service if it's already running.

So...in the current state, in fact I think the race here is that -firstboot might be killed by the reboot initated by -host before it has a chance to unlink(/etc/ignition-machine-config-encapsulated.json)?

I.e. it seems like we should just have the -host service not be enabled at all and just manually started via systemctl start for the daemonset to use?

Yeah I think so.

@cgwalters cgwalters force-pushed the fix-firstboot-double branch from e49c1fa to a086336 Compare January 10, 2020 20:43
@cgwalters cgwalters changed the title Only run machine-config-daemon-host.service if not firstboot Don't enable machine-config-daemon-host.service by default Jan 10, 2020
@jlebon
Copy link
Member

jlebon commented Jan 10, 2020

/approve

@cgwalters
Don't enable machine-config-daemon-host.service by default
57e4c67 
Today the MCS serves both `/etc/pivot/image-pullspec` *and*
`/etc/ignition-machine-config-encapsulated.json` - and we have
to serve both so that using 4.1 bootimages works.

However, we absolutely shouldn't start *both* services on a modern
4.3+ system on firstboot - they will race, both trying to update,
and one doing a reboot.

Only enable the `-firstboot.service` which will itself currently
invoke `-host.service`, though in the future we should clean this
up by having the firstboot code run rpm-ostree *directly*, since
we're already on the host.

Motivated by discussions around RT kernel handling.
@cgwalters cgwalters force-pushed the fix-firstboot-double branch from a086336 to 57e4c67 Compare January 10, 2020 21:18
@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

Bumped to force retesting.

@cgwalters
Copy link
Member Author

cgwalters commented Jan 10, 2020

/hold
I think this is fine, I just want to get a chance to read through the journals from the CI runs.

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 10, 2020
@cgwalters
Copy link
Member Author

cgwalters commented Jan 11, 2020

/retest

@sinnykumari
Copy link
Contributor

sinnykumari commented Jan 13, 2020

Keeping m-c-d-host disabled seems right way to handle this.

Verified this by looking at service log and their timestamps on a local cluster. During firstboot, m-c-d-host service doesn't get started by default. It gets started by m-c-d-firstboot service

# journalctl -u machine-config-daemon-firstboot.service 
-- Logs begin at Mon 2020-01-13 07:51:29 UTC, end at Mon 2020-01-13 09:24:22 UTC. --
Jan 13 07:52:11 ip-10-0-135-195.ec2.internal systemd[1]: Starting Machine Config Daemon Firstboot...
...
Jan 13 07:53:15 ip-10-0-135-195 machine-config-daemon[1811]: I0113 07:53:15.766939    1811 update.go:1093] Updating OS to registry.svc.ci.openshift.org/ocp/4.4@sha256:235d616048c2728c55b4aa5f91ad3601980f60f0910adc8c62dda4bb17f4de8c
...

# journalctl -u machine-config-daemon-host.service
-- Logs begin at Mon 2020-01-13 07:51:29 UTC, end at Mon 2020-01-13 09:26:25 UTC. --
Jan 13 07:53:15 ip-10-0-135-195 systemd[1]: Starting Machine Config Daemon Initial...
Jan 13 07:53:15 ip-10-0-135-195 machine-config-daemon[2139]: I0113 07:53:15.794572    2139 rpm-ostree.go:434] Running captured: rpm-ostree status --json
Jan 13 07:53:15 ip-10-0-135-195 machine-config-daemon[2139]: I0113 07:53:15.882161    2139 rpm-ostree.go:159] Current origin is not custom
Jan 13 07:53:15 ip-10-0-135-195 machine-config-daemon[2139]: I0113 07:53:15.882459    2139 run.go:16] Running: podman pull -q --authfile /var/lib/kubelet/config.json registry.svc.ci.openshift.org/ocp/4.4@sha256:235d616048c2728c55b4aa5f91ad3601980f60f0910adc8c62dda4bb17f4de8c
...

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 13, 2020
@sinnykumari
Copy link
Contributor

sinnykumari commented Jan 13, 2020

/retest

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 13, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, sinnykumari

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

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [cgwalters,sinnykumari]

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

@sinnykumari
Copy link
Contributor

sinnykumari commented Jan 14, 2020

@cgwalters Shall we get this merged?

@cgwalters
Copy link
Member Author

cgwalters commented Jan 14, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 14, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

25 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 20, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 21, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 22, 2020

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit d25e2bd into openshift:master Jan 22, 2020
@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Jan 22, 2020

@cgwalters: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-aws 57e4c67 link /test e2e-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

sinnykumari added a commit to sinnykumari/machine-config-operator that referenced this pull request May 21, 2020
@sinnykumari
templates: Don't enable machine-config-daemon-host.service by default
872e48c 
Also, add machine-config-daemon-firstboot-v42.service so that new
nodes through machine-api comes up as expected on a 4.2 to 4.3
upgraded cluster

Backport of PRs:
- openshift#1366
- openshift#1706
@sinnykumari sinnykumari mentioned this pull request May 21, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashcrow ashcrow Awaiting requested review from ashcrow

@runcom runcom Awaiting requested review from runcom

@sinnykumari sinnykumari Awaiting requested review from sinnykumari

Assignees

@sinnykumari sinnykumari

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@cgwalters @kikisdeliveryservice @jlebon @sinnykumari @openshift-ci-robot @openshift-bot @ashcrow @openshift-merge-robot