Move chrony config from generator to systemd service #2271

gursewak1997 · 2023-03-02T01:23:13Z

With the latest changes from systemd, we now run certain steps before NetworkManager.service to access and write to /etc/sysconfig/network. We completely got rid of the usage of generators.

Issue: coreos/fedora-coreos-tracker#1402

dustymabe · 2023-03-02T03:36:19Z

Unfortunately this is getting complicated :(

Long-term, we are looking to eliminate that file entirely and just use systemd unit service.

Can you expand on the strategy there?

I'm thinking 1. maybe we should push harder to get everything out of a generator now OR 2. take a more tactical approach to updating /etc/sysconfig/network and leave most of the logic in the generator for now (until we can get rid of it completely.

I actually have ideas on how we can achieve either 1. or 2.. Maybe we can discuss it tomorrow.

overlay.d/20platform-chrony/usr/lib/systemd/system/coreos-platform-chrony-config.service

dustymabe

A few small comment fixups.. otherwise LGTM

How did this perform in testing?

...lay.d/20platform-chrony/usr/lib/systemd/system/chronyd.service.d/coreos-platform-chrony.conf

dustymabe · 2023-03-03T04:22:53Z

looking at the CI failure we probably need to delete the coreos-platform-chrony-generator-permissions test now that we no longer have a generator.

dustymabe

LGTM - tagging in @jlebon for a second pair of eyes.

jlebon · 2023-03-03T17:38:57Z

overlay.d/20platform-chrony/usr/lib/systemd/system/coreos-platform-chrony-config.service

@@ -0,0 +1,13 @@
+[Unit]
+Description=Configure chrony based on the platform


Minor: we usually prefix our units with "CoreOS". Also, the convention for systemd unit descriptions is Title Case.

Can you elaborate on the description part? Also, don't we already have "coreos" as the prefix for this service?

I think he's saying that we usually prefix our units with CoreOS in the description so that in the logs it's clear to us that it's one of our systemd units we defined.

Based on his suggestion something like:

CoreOS Configure Chrony Based On The Platform

Would work.

jlebon · 2023-03-03T17:46:02Z

overlay.d/20platform-chrony/usr/libexec/coreos-platform-chrony-config

+confpath=/run/coreos-platform-chrony.conf
+altenvfilepath=/run/sysconfig-coreos-chrony


While we're here, I wonder if we should put these under /run/coreos instead? (And drop the coreos in the basename.) Now that it's two files, it starts to look a bit messy putting it in the top-level.

jlebon · 2023-03-03T17:48:32Z

overlay.d/20platform-chrony/usr/libexec/coreos-platform-chrony-config

+# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chrony
+# and write out a new $OPTIONS variable (with specified new configuration path)
+# to /run/sysconfig-coreos-chrony
+source ${altenvfilepath}


Shouldn't this be /etc/sysconfig/chronyd?

this still needs to be updated

jlebon · 2023-03-03T17:50:11Z

overlay.d/20platform-chrony/usr/libexec/coreos-platform-chrony-config

-ExecStart=
-ExecStart=/usr/sbin/chronyd -f ${confpath} \$OPTIONS
-EOF
+# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chrony


Suggested change

# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chrony

# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chronyd

jlebon · 2023-03-03T18:56:06Z

Just noticed this PR is targeting the rawhide branch. It needs to target testing-devel. Were there concerns with doing this change in f37?

dustymabe · 2023-03-03T19:22:37Z

Just noticed this PR is targeting the rawhide branch. It needs to target testing-devel. Were there concerns with doing this change in f37?

good catch. Yes, this should target testing-devel.

gursewak1997 · 2023-03-05T19:51:51Z

With the recent changes, the basic test passes but the ext.config.ntp.chrony.coreos-platform-chrony-generator fails. The CI here isn't testing that as this test only runs on aws azure gce. Working on the fix as the logs aren't helping much.

dustymabe · 2023-03-06T03:27:38Z

With the recent changes, the basic test passes but the ext.config.ntp.chrony.coreos-platform-chrony-generator fails. The CI here isn't testing that as this test only runs on aws azure gce. Working on the fix as the logs aren't helping much.

For one, this still needs to be addressed: https://github.com/coreos/fedora-coreos-config/pull/2271/files#r1124818300

dustymabe · 2023-03-06T19:33:44Z

overlay.d/20platform-chrony/usr/lib/systemd/system/chronyd.service.d/platform-chrony.conf

+[Service]
+EnvironmentFile=-/etc/sysconfig/chronyd


this should go back to the value it was before.

dustymabe · 2023-03-06T19:35:08Z

overlay.d/20platform-chrony/usr/libexec/coreos-platform-chrony-config

 if ! cmp {/usr,}/etc/chrony.conf >/dev/null; then
    echo "$self: /etc/chrony.conf is modified; not changing the default"
    exit 0
 fi

+confpath=/run/coreos/platform-chrony.conf
+altenvfilepath=/etc/sysconfig/chronyd


This should go back to the value it was before

dustymabe · 2023-03-06T19:35:25Z

overlay.d/20platform-chrony/usr/libexec/coreos-platform-chrony-config

+# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chronyd
+# and write out a new $OPTIONS variable (with specified new configuration path)
+# to /etc/sysconfig/chronyd
+source ${altenvfilepath}


This should say source /etc/sysconfig/chronyd

With latest changes from systemd, we now run certain steps before NetworkManager.service to access and write to /etc/sysconfig/network. Issue: coreos/fedora-coreos-tracker#1402

dustymabe

LGTM if the tests pass!

gursewak1997 requested review from dustymabe and jlebon March 2, 2023 01:23

dustymabe reviewed Mar 3, 2023

View reviewed changes

overlay.d/20platform-chrony/usr/lib/systemd/system/coreos-platform-chrony-config.service Outdated Show resolved Hide resolved

dustymabe previously approved these changes Mar 3, 2023

View reviewed changes

...lay.d/20platform-chrony/usr/lib/systemd/system/chronyd.service.d/coreos-platform-chrony.conf Outdated Show resolved Hide resolved

gursewak1997 dismissed dustymabe’s stale review via 8080055 March 3, 2023 07:54

dustymabe previously approved these changes Mar 3, 2023

View reviewed changes

jlebon reviewed Mar 3, 2023

View reviewed changes

gursewak1997 dismissed dustymabe’s stale review via 15ffcec March 3, 2023 22:13

gursewak1997 changed the base branch from rawhide to testing-devel March 3, 2023 22:14

dustymabe reviewed Mar 6, 2023

View reviewed changes

Move chrony config from generator to systemd service

8d55980

With latest changes from systemd, we now run certain steps before NetworkManager.service to access and write to /etc/sysconfig/network. Issue: coreos/fedora-coreos-tracker#1402

dustymabe approved these changes Mar 6, 2023

View reviewed changes

gursewak1997 enabled auto-merge (rebase) March 6, 2023 20:55

gursewak1997 merged commit 5e4f40c into coreos:testing-devel Mar 6, 2023

This was referenced Mar 6, 2023

rawhide: kola: coreos-platform-chrony systemd generator failure coreos/fedora-coreos-tracker#1402

Closed

Bump fedora-coreos-config openshift/os#1185

Merged

gursewak1997 deleted the chrony-generator-fix branch March 7, 2023 08:37

jlebon mentioned this pull request Mar 28, 2023

Platform chrony configuration intolerant of user modified config coreos/fedora-coreos-tracker#1449

Closed

dustymabe mentioned this pull request Apr 23, 2023

overlay.d/20platform-chrony: accept ec2 and gce platform IDs #2396

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move chrony config from generator to systemd service #2271

Move chrony config from generator to systemd service #2271

gursewak1997 commented Mar 2, 2023 •

edited

dustymabe commented Mar 2, 2023

dustymabe left a comment

dustymabe commented Mar 3, 2023

dustymabe left a comment

jlebon Mar 3, 2023

gursewak1997 Mar 3, 2023

dustymabe Mar 3, 2023

jlebon Mar 3, 2023

jlebon Mar 3, 2023

dustymabe Mar 4, 2023

jlebon Mar 3, 2023

jlebon commented Mar 3, 2023

dustymabe commented Mar 3, 2023

gursewak1997 commented Mar 5, 2023

dustymabe commented Mar 6, 2023

dustymabe Mar 6, 2023

dustymabe Mar 6, 2023

dustymabe Mar 6, 2023

dustymabe left a comment

		@@ -0,0 +1,13 @@
		[Unit]
		Description=Configure chrony based on the platform

		confpath=/run/coreos-platform-chrony.conf
		altenvfilepath=/run/sysconfig-coreos-chrony

	# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chrony
	# Read in the existing $OPTIONS variable setting from /etc/sysconfig/chronyd

		[Service]
		EnvironmentFile=-/etc/sysconfig/chronyd

Move chrony config from generator to systemd service #2271

Move chrony config from generator to systemd service #2271

Conversation

gursewak1997 commented Mar 2, 2023 • edited

dustymabe commented Mar 2, 2023

dustymabe left a comment

Choose a reason for hiding this comment

dustymabe commented Mar 3, 2023

dustymabe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jlebon commented Mar 3, 2023

dustymabe commented Mar 3, 2023

gursewak1997 commented Mar 5, 2023

dustymabe commented Mar 6, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustymabe left a comment

Choose a reason for hiding this comment

gursewak1997 commented Mar 2, 2023 •

edited