Create a new NTP-admin service #8555

smklein · 2025-07-08T22:54:56Z

This exposes the same access to chrony that exists in config-reconciler/src/reconciler_task/zones.rs, but over an HTTP interface instead.

Fixes #8552

hawkw · 2025-07-11T02:57:42Z

ntp-admin/src/bin/ntp-admin.rs

+}
+
+fn main() {
+    if let Err(err) = oxide_tokio_rt::run(main_impl()) {


Do we think this service really needs 128 worker threads, or should we maybe consider building a single-threaded runtime here?

Definitely not, but I was just copying the structure of the cockroach-admin server (which is also apparently launching 128 threads -- I doubt it needs that many)

Updated to use 8

hawkw · 2025-07-11T02:58:58Z

ntp-admin/src/http_entrypoints.rs

+        }
+
+        let Ok(ref_id) = u32::from_str_radix(v[0], 16) else {
+            error!(log, "failed to parse ref_id");


would it be worthwhile to include the invalid value here (and subsequently)?

karencfv

Looks good! There are a few chronyc-specific bits that I don't really know about, so I just took a look at the other parts of this PR :)

karencfv · 2025-07-14T01:11:36Z

ntp-admin/src/http_entrypoints.rs

+        if v.len() < 10 {
+            error!(log, "too few fields"; "stdout" => &stdout);
+            return Err(TimeSyncError::FailedToParse {
+                reason: "too few fields",
+                stdout,
+            });
+        }
+
+        let Ok(ref_id) = u32::from_str_radix(v[0], 16) else {
+            error!(log, "failed to parse ref_id"; "stdout" => &stdout);
+            return Err(TimeSyncError::FailedToParse {
+                reason: "bad ref_id",
+                stdout,
+            });
+        };
+        let ip_addr =
+            IpAddr::from_str(v[1]).unwrap_or(Ipv6Addr::UNSPECIFIED.into());
+        let Ok(stratum) = u8::from_str(v[2]) else {
+            error!(log, "bad stratum"; "stdout" => &stdout);
+            return Err(TimeSyncError::FailedToParse {
+                reason: "bad stratum",
+                stdout,
+            });
+        };
+        let Ok(ref_time) = f64::from_str(v[3]) else {
+            error!(log, "bad ref_time"; "stdout" => &stdout);
+            return Err(TimeSyncError::FailedToParse {
+                reason: "bad ref_time",
+                stdout,
+            });
+        };
+        let Ok(correction) = f64::from_str(v[4]) else {
+            error!(log, "bad correction"; "stdout" => &stdout);
+            return Err(TimeSyncError::FailedToParse {
+                reason: "bad correction",
+                stdout,
+            });
+        };
+
+        // Per `chronyc waitsync`'s implementation, if either the
+        // reference IP address is not unspecified or the reference
+        // ID is not 0 or 0x7f7f0101, we are synchronized to a peer.
+        let peer_sync =
+            !ip_addr.is_unspecified() || (ref_id != 0 && ref_id != 0x7f7f0101);
+
+        let sync = stratum < 10
+            && ref_time > 1234567890.0
+            && peer_sync
+            && correction.abs() <= 0.05;


Could be good to get some tests for this?

FYI, this is largely being lifted from

omicron/sled-agent/config-reconciler/src/reconciler_task/zones.rs

Lines 503 to 560 in 248e7ed

// XXXNTP - This could be replaced with a direct connection to the

// daemon using a patched version of the chrony_candm crate to allow

// a custom server socket path. From the GZ, it should be possible to

// connect to the UNIX socket at

// format!("{}/var/run/chrony/chronyd.sock", ntp_zone.root())

let stdout = running_ntp_zone

.run_cmd(&["/usr/bin/chronyc", "-c", "tracking"])

.map_err(TimeSyncError::ExecuteChronyc)?;

let v: Vec<&str> = stdout.split(',').collect();

if v.len() < 10 {

return Err(TimeSyncError::FailedToParse {

reason: "too few fields",

stdout,

});

}

let Ok(ref_id) = u32::from_str_radix(v[0], 16) else {

return Err(TimeSyncError::FailedToParse {

reason: "bad ref_id",

stdout,

});

};

let ip_addr =

IpAddr::from_str(v[1]).unwrap_or(Ipv6Addr::UNSPECIFIED.into());

let Ok(stratum) = u8::from_str(v[2]) else {

return Err(TimeSyncError::FailedToParse {

reason: "bad stratum",

stdout,

});

};

let Ok(ref_time) = f64::from_str(v[3]) else {

return Err(TimeSyncError::FailedToParse {

reason: "bad ref_time",

stdout,

});

};

let Ok(correction) = f64::from_str(v[4]) else {

return Err(TimeSyncError::FailedToParse {

reason: "bad correction",

stdout,

});

};

// Per `chronyc waitsync`'s implementation, if either the

// reference IP address is not unspecified or the reference

// ID is not 0 or 0x7f7f0101, we are synchronized to a peer.

let peer_sync =

!ip_addr.is_unspecified() || (ref_id != 0 && ref_id != 0x7f7f0101);

let sync = stratum < 10

&& ref_time > 1234567890.0

&& peer_sync

&& correction.abs() <= 0.05;

Ok(TimeSync { sync, ref_id, ip_addr, stratum, ref_time, correction })

, but I'll add tests regardless.

Ah! Thanks for adding the tests though!

sled-agent/types/src/time_sync.rs

smf/ntp-admin/manifest.xml

karencfv · 2025-07-14T01:28:01Z

smf/ntp-admin/manifest.xml

+  <exec_method type='method' name='start'
+    exec='/opt/oxide/lib/svc/manifest/ntp-admin.sh'
+    timeout_seconds='0' />
+  <exec_method type='method' name='stop' exec=':kill' timeout_seconds='0' />


Do we need an entire method_script.sh file for just running a single command (exec /opt/oxide/ntp-admin/bin/ntp-admin "${args[@]}" &)?

How about adding the command directly here?

Suggested change

<exec_method type='method' name='start'

exec='/opt/oxide/lib/svc/manifest/ntp-admin.sh'

timeout_seconds='0' />

<exec_method type='method' name='stop' exec=':kill' timeout_seconds='0' />

<exec_method type='method' name='start'

exec='ctrun -l child -o noorphan,regent /opt/oxide/ntp-admin/bin/ntp-admin run --config-file-path /opt/oxide/lib/svc/ntp-admin/config.toml --address %{config/address} &'

timeout_seconds='0' />

<exec_method type='method' name='stop' exec=':kill' timeout_seconds='0' />

If it's cool with you, I'd still rather keep the method script. IMO it's more consistent with other SMF services, easier to read, and easy to modify the moment we need to add additional configs.

I'm not sure consistency with other SMF services would be a huge factor here, I took a look and only the clickhouse, cockroach-admin, and cockroachdb services have a separate method_script.sh file. That said, I don't have strong feelings about this, so I'll leave it up to you :)

smf/ntp-admin/method_script.sh

karencfv

Thanks for addressing my comments, looks good to me!

karencfv · 2025-07-15T00:07:18Z

smf/ntp-admin/manifest.xml

+  <exec_method type='method' name='start'
+    exec='/opt/oxide/lib/svc/manifest/ntp-admin.sh'
+    timeout_seconds='0' />
+  <exec_method type='method' name='stop' exec=':kill' timeout_seconds='0' />


I'm not sure consistency with other SMF services would be a huge factor here, I took a look and only the clickhouse, cockroach-admin, and cockroachdb services have a separate method_script.sh file. That said, I don't have strong feelings about this, so I'll leave it up to you :)

followup from #8555 (comment)

jgallagher

Thanks, LGTM.

Not as a part of this PR - should we add an internal DNS entry for this server? (I think we discussed wanting that for cockroach-admin too, but I'm not finding an issue for it - maybe it was only in PR comments?)

smklein · 2025-07-15T15:53:14Z

Thanks, LGTM.

Not as a part of this PR - should we add an internal DNS entry for this server? (I think we discussed wanting that for cockroach-admin too, but I'm not finding an issue for it - maybe it was only in PR comments?)

I think #6796 and #8496 is kinda a related issue too -- these are cases of:

We have a zone, with a service, with an IP + Port combo in-use and known to DNS
We have a second service in that zone, at a well-known port
Is it described by OmicronZoneType + Internal DNS? (in this case, "no, not yet")

My plan was "add the service, use it, ensure that works, then add it to the zone type + DNS for completeness". I kinda wanted to separate "does the service work" from "ensuring we can find this thing in a backwards compatible way".

WDYT? I could try to fix both at once, if we prefer

jgallagher · 2025-07-15T15:55:00Z

My plan was "add the service, use it, ensure that works, then add it to the zone type + DNS for completeness". I kinda wanted to separate "does the service work" from "ensuring we can find this thing in a backwards compatible way".

100% on board with this plan. It might be right to take "ensuring we can find this thing in a backwards compatible way" on as tech debt intentionally; it's worth fixing but doesn't seem particularly urgent to me?

followup from #8555 (comment)

smklein force-pushed the ntp-admin branch 5 times, most recently from ee07393 to d2e15ed Compare July 10, 2025 22:42

hawkw reviewed Jul 11, 2025

View reviewed changes

smklein force-pushed the ntp-admin branch from d2e15ed to 6930f1b Compare July 11, 2025 19:14

smklein marked this pull request as ready for review July 11, 2025 19:46

karencfv reviewed Jul 14, 2025

View reviewed changes

smklein force-pushed the ntp-admin branch 2 times, most recently from 5cdb66b to cd0aeb3 Compare July 14, 2025 22:00

Create a new NTP-admin service

8ad59a1

smklein force-pushed the ntp-admin branch from cd0aeb3 to 8ad59a1 Compare July 14, 2025 22:02

karencfv approved these changes Jul 15, 2025

View reviewed changes

smklein mentioned this pull request Jul 15, 2025

Use NTP admin API instead of sled agent, remove sled agent time_sync API #8597

Draft

jgallagher added a commit that referenced this pull request Jul 15, 2025

cockroach-admin-server: use a smaller tokio worker pool

78d730a

followup from #8555 (comment)

jgallagher mentioned this pull request Jul 15, 2025

cockroach-admin-server: use a smaller tokio worker pool #8599

Merged

jgallagher approved these changes Jul 15, 2025

View reviewed changes

jgallagher added a commit that referenced this pull request Jul 15, 2025

cockroach-admin-server: use a smaller tokio worker pool (#8599)

809ec24

followup from #8555 (comment)

	// XXXNTP - This could be replaced with a direct connection to the
	// daemon using a patched version of the chrony_candm crate to allow
	// a custom server socket path. From the GZ, it should be possible to
	// connect to the UNIX socket at
	// format!("{}/var/run/chrony/chronyd.sock", ntp_zone.root())

	let stdout = running_ntp_zone
	.run_cmd(&["/usr/bin/chronyc", "-c", "tracking"])
	.map_err(TimeSyncError::ExecuteChronyc)?;

	let v: Vec<&str> = stdout.split(',').collect();

	if v.len() < 10 {
	return Err(TimeSyncError::FailedToParse {
	reason: "too few fields",
	stdout,
	});
	}

	let Ok(ref_id) = u32::from_str_radix(v[0], 16) else {
	return Err(TimeSyncError::FailedToParse {
	reason: "bad ref_id",
	stdout,
	});
	};
	let ip_addr =
	IpAddr::from_str(v[1]).unwrap_or(Ipv6Addr::UNSPECIFIED.into());
	let Ok(stratum) = u8::from_str(v[2]) else {
	return Err(TimeSyncError::FailedToParse {
	reason: "bad stratum",
	stdout,
	});
	};
	let Ok(ref_time) = f64::from_str(v[3]) else {
	return Err(TimeSyncError::FailedToParse {
	reason: "bad ref_time",
	stdout,
	});
	};
	let Ok(correction) = f64::from_str(v[4]) else {
	return Err(TimeSyncError::FailedToParse {
	reason: "bad correction",
	stdout,
	});
	};

	// Per `chronyc waitsync`'s implementation, if either the
	// reference IP address is not unspecified or the reference
	// ID is not 0 or 0x7f7f0101, we are synchronized to a peer.
	let peer_sync =
	!ip_addr.is_unspecified() \|\| (ref_id != 0 && ref_id != 0x7f7f0101);

	let sync = stratum < 10
	&& ref_time > 1234567890.0
	&& peer_sync
	&& correction.abs() <= 0.05;

	Ok(TimeSync { sync, ref_id, ip_addr, stratum, ref_time, correction })

Create a new NTP-admin service #8555

Are you sure you want to change the base?

Create a new NTP-admin service #8555

Conversation

smklein commented Jul 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

karencfv left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jgallagher left a comment

Choose a reason for hiding this comment

Uh oh!

smklein commented Jul 15, 2025

Uh oh!

jgallagher commented Jul 15, 2025

Uh oh!

Uh oh!

smklein commented Jul 8, 2025 •

edited

Loading