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

Author: smklein

Cargo.lock

@@ -71,6 +71,7 @@ Nexus Internal API (client: nexus-client)
     consumed by: propolis-server (propolis/bin/propolis-server) via 3 paths
 
 NTP Admin (client: ntp-admin-client)
+    consumed by: omicron-sled-agent (omicron/sled-agent) via 2 paths
 
 External API (client: oxide-client)
 

dev-tools/ls-apis/tests/api_dependencies.out

@@ -1464,29 +1464,6 @@
         }
       }
     },
-    "/timesync": {
-      "get": {
-        "operationId": "timesync_get",
-        "responses": {
-          "200": {
-            "description": "successful operation",
-            "content": {
-              "application/json": {
-                "schema": {
-                  "$ref": "#/components/schemas/TimeSync"
-                }
-              }
-            }
-          },
-          "4XX": {
-            "$ref": "#/components/responses/Error"
-          },
-          "5XX": {
-            "$ref": "#/components/responses/Error"
-          }
-        }
-      }
-    },
     "/v2p": {
       "get": {
         "summary": "List v2p mappings present on sled",
@@ -7326,50 +7303,6 @@
           "uplinks"
         ]
       },
-      "TimeSync": {
-        "type": "object",
-        "properties": {
-          "correction": {
-            "description": "The current offset between the NTP clock and system clock.",
-            "type": "number",
-            "format": "double"
-          },
-          "ip_addr": {
-            "description": "The NTP reference IP address.",
-            "type": "string",
-            "format": "ip"
-          },
-          "ref_id": {
-            "description": "The NTP reference ID.",
-            "type": "integer",
-            "format": "uint32",
-            "minimum": 0
-          },
-          "ref_time": {
-            "description": "The NTP reference time (i.e. what chrony thinks the current time is, not necessarily the current system time).",
-            "type": "number",
-            "format": "double"
-          },
-          "stratum": {
-            "description": "The NTP stratum (our upstream's stratum plus one).",
-            "type": "integer",
-            "format": "uint8",
-            "minimum": 0
-          },
-          "sync": {
-            "description": "The synchronization state of the sled, true when the system clock and the NTP clock are in sync (to within a small window).",
-            "type": "boolean"
-          }
-        },
-        "required": [
-          "correction",
-          "ip_addr",
-          "ref_id",
-          "ref_time",
-          "stratum",
-          "sync"
-        ]
-      },
       "TxEqConfig": {
         "description": "Per-port tx-eq overrides.  This can be used to fine-tune the transceiver equalization settings to improve signal integrity.",
         "type": "object",

openapi/sled-agent.json

@@ -59,6 +59,7 @@ nexus-client.workspace = true
 nexus-config.workspace = true
 nexus-sled-agent-shared.workspace = true
 nexus-types.workspace = true
+ntp-admin-client.workspace = true
 omicron-common.workspace = true
 omicron-ddm-admin-client.workspace = true
 omicron-uuid-kinds.workspace = true

sled-agent/Cargo.toml

@@ -42,7 +42,6 @@ use sled_agent_types::{
         VmmPutStateResponse, VmmUnregisterResponse,
     },
     sled::AddSledRequest,
-    time_sync::TimeSync,
     zone_bundle::{
         BundleUtilization, CleanupContext, CleanupCount, PriorityOrder,
         ZoneBundleId, ZoneBundleMetadata,
@@ -477,14 +476,6 @@ pub trait SledAgentApi {
         rqctx: RequestContext<Self::Context>,
     ) -> Result<HttpResponseOk<Vec<VirtualNetworkInterfaceHost>>, HttpError>;
 
-    #[endpoint {
-        method = GET,
-        path = "/timesync",
-    }]
-    async fn timesync_get(
-        rqctx: RequestContext<Self::Context>,
-    ) -> Result<HttpResponseOk<TimeSync>, HttpError>;
-
     #[endpoint {
         method = POST,
         path = "/switch-ports",

sled-agent/api/src/lib.rs

@@ -26,6 +26,7 @@ illumos-utils.workspace = true
 installinator-common.workspace = true
 key-manager.workspace = true
 nexus-sled-agent-shared.workspace = true
+ntp-admin-client.workspace = true
 omicron-common.workspace = true
 omicron-uuid-kinds.workspace = true
 serde.workspace = true

sled-agent/config-reconciler/Cargo.toml

@@ -503,7 +503,7 @@ impl ReconcilerTask {
 
         // Collect the current timesync status (needed to start any new zones,
         // and also we want to report it as part of each reconciler result).
-        let timesync_status = self.zones.check_timesync().await;
+        let timesync_status = self.zones.check_timesync(&self.log).await;
 
         // Call back into sled-agent and let it do any work that needs to happen
         // once time is sync'd (e.g., rewrite `uptime`).

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

@@ -24,9 +24,9 @@ use nexus_sled_agent_shared::inventory::ConfigReconcilerInventoryResult;
 use nexus_sled_agent_shared::inventory::OmicronZoneConfig;
 use nexus_sled_agent_shared::inventory::OmicronZoneImageSource;
 use nexus_sled_agent_shared::inventory::OmicronZoneType;
+use ntp_admin_client::types::TimeSync;
 use omicron_common::address::Ipv6Subnet;
 use omicron_uuid_kinds::OmicronZoneUuid;
-use sled_agent_types::time_sync::TimeSync;
 use sled_agent_types::zone_bundle::ZoneBundleCause;
 use sled_agent_types::zone_images::ResolverStatus;
 use sled_storage::config::MountConfig;
@@ -36,10 +36,7 @@ use slog::warn;
 use slog_error_chain::InlineErrorChain;
 use std::borrow::Cow;
 use std::collections::BTreeMap;
-use std::net::IpAddr;
-use std::net::Ipv6Addr;
 use std::num::NonZeroUsize;
-use std::str::FromStr as _;
 use std::sync::Arc;
 
 use super::OmicronDatasets;
@@ -70,6 +67,8 @@ pub enum TimeSyncError {
     NoRunningNtpZone,
     #[error("multiple running NTP zones - this should never happen!")]
     MultipleRunningNtpZones,
+    #[error("failed to communicate with NTP admin server")]
+    NtpAdmin(#[from] ntp_admin_client::Error<ntp_admin_client::types::Error>),
     #[error("failed to execute chronyc within NTP zone")]
     ExecuteChronyc(#[source] RunCommandError),
     #[error(
@@ -465,10 +464,10 @@ impl OmicronZones {
     }
 
     /// Check the timesync status from a running NTP zone (if it exists)
-    pub(super) async fn check_timesync(&self) -> TimeSyncStatus {
+    pub(super) async fn check_timesync(&self, log: &Logger) -> TimeSyncStatus {
         match &self.timesync_config {
             TimeSyncConfig::Normal => {
-                match self.timesync_status_from_ntp_zone().await {
+                match self.timesync_status_from_ntp_zone(log).await {
                     Ok(timesync) => TimeSyncStatus::TimeSync(timesync),
                     Err(err) => {
                         TimeSyncStatus::FailedToGetSyncStatus(Arc::new(err))
@@ -481,83 +480,45 @@ impl OmicronZones {
 
     async fn timesync_status_from_ntp_zone(
         &self,
+        log: &Logger,
     ) -> Result<TimeSync, TimeSyncError> {
         // Get the one and only running NTP zone, or return an error.
-        let mut running_ntp_zones = self.zones.iter().filter_map(|z| {
+        let mut ntp_admin_addresses = self.zones.iter().filter_map(|z| {
             if !z.config.zone_type.is_ntp() {
                 return None;
             }
+            // TODO(https://github.com/oxidecomputer/omicron/issues/6796):
+            //
+            // We could avoid hard-coding the port here if the zone was fully
+            // specified to include both NTP and Admin server addresses.
+            let mut addr = match z.config.zone_type {
+                OmicronZoneType::BoundaryNtp { address, .. } => address,
+                OmicronZoneType::InternalNtp { address, .. } => address,
+                _ => return None,
+            };
+            addr.set_port(omicron_common::address::NTP_ADMIN_PORT);
 
             match &z.state {
-                ZoneState::Running(running_zone) => Some(running_zone),
+                ZoneState::Running(_) => Some(addr),
                 ZoneState::PartiallyShutDown { .. }
                 | ZoneState::FailedToStart(_) => None,
             }
         });
-        let running_ntp_zone =
-            running_ntp_zones.next().ok_or(TimeSyncError::NoRunningNtpZone)?;
-        if running_ntp_zones.next().is_some() {
+        let ntp_admin_address = ntp_admin_addresses
+            .next()
+            .ok_or(TimeSyncError::NoRunningNtpZone)?;
+        if ntp_admin_addresses.next().is_some() {
             return Err(TimeSyncError::MultipleRunningNtpZones);
         }
 
-        // 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 client = ntp_admin_client::Client::new(
+            &format!("http://{ntp_admin_address}"),
+            log.clone(),
+        );
 
-        let sync = stratum < 10
-            && ref_time > 1234567890.0
-            && peer_sync
-            && correction.abs() <= 0.05;
+        let timesync = client.timesync().await?.into_inner();
 
-        Ok(TimeSync { sync, ref_id, ip_addr, stratum, ref_time, correction })
+        Ok(timesync)
     }
 }
 
@@ -1111,6 +1072,7 @@ mod tests {
     use sled_agent_types::zone_images::ZoneManifestStatus;
     use std::collections::BTreeSet;
     use std::collections::VecDeque;
+    use std::net::Ipv6Addr;
     use std::sync::Mutex;
     use tufaceous_artifact::ArtifactHash;
 

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

@@ -36,7 +36,6 @@ use sled_agent_types::instance::{
     VmmPutStateResponse, VmmUnregisterResponse,
 };
 use sled_agent_types::sled::AddSledRequest;
-use sled_agent_types::time_sync::TimeSync;
 use sled_agent_types::zone_bundle::{
     BundleUtilization, CleanupContext, CleanupCount, CleanupPeriod,
     StorageLimit, ZoneBundleId, ZoneBundleMetadata,
@@ -708,13 +707,6 @@ impl SledAgentApi for SledAgentImpl {
         Ok(HttpResponseOk(vnics))
     }
 
-    async fn timesync_get(
-        rqctx: RequestContext<Self::Context>,
-    ) -> Result<HttpResponseOk<TimeSync>, HttpError> {
-        let sa = rqctx.context();
-        Ok(HttpResponseOk(sa.timesync_get().await.map_err(|e| Error::from(e))?))
-    }
-
     async fn uplink_ensure(
         rqctx: RequestContext<Self::Context>,
         body: TypedBody<SwitchPorts>,