Add state duration to DDM FSM (#689)

* ddm: cleanup peer tracking and add state durations

Db.peers was only used as a cache for reporting peers to ddm-api clients
calling GET /peers, and the contents of the cache were a bit misleading.
For example, there is no such thing as a peer state in DDM. DDM FSMs
have states, but they are instantiated per interface not per peer. Not
only does the state reported by GET /peers not actually map to a peer,
but it doesn't even directly map to the FSM of the peer's interface.
Instead, that state was populated by manual db updates scattered
throughout the FSM implementation. This removes Db.peers and introduces
a new shared InterfaceState struct that consolidates peer/FSM info for a
given interface. GET /peers now reads from each InterfaceState and
passes along any peers it finds along with interface state.

Also fixes a pre-existing bug in oxstats where interface names
were always empty due to reading from a stale SmContext clone.

* ddmadm: sort output of get-peers

* ddm: remove dupe peer_address, add fsm helpers

After adding PeerIdentity, the peer_address field of SessionStats is now
redundant (and also isn't a stat) so this removes it. The peer address
is tracked in PeerIdentity, so we can just rely on that field behind a
single mutex instead of locking/juggling both stats and identity.
Also adds a couple helper methods for transitioning FSM state and
updating the peer state.

* ddmadm: show fsm state duration in separate column

* cargo fmt

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* ddmadm: reuse mg_common::format_duration_human

Drop the local seconds-precision format_duration helper in favor of the
shared helper already used by mgadm for BGP FSM durations, so ddmadm and
mgadm display durations consistently.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* ddm: add InterfaceState::set_if_info helper

Encapsulate the paired writes to if_index and if_name behind a single
method on InterfaceState so the two fields stay in sync at the one site
that updates them.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* ddm: hoist if_name lock out of per-sample oximeter macros

The produce() loop locked peer.iface.if_name twelve times per peer per
scrape. Take the lock once at the top of the loop body and reuse the
cloned String for each sample.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

* ddm: compute peer_status before locking other iface fields

peer_status() internally locks fsm_state and last_fsm_state_change. Call
it before acquiring if_index and peer_identity in get_peers so it remains
deadlock-safe if it ever takes additional InterfaceState mutexes.

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

---------

Signed-off-by: Trey Aspelund <trey@oxidecomputer.com>

Author: taspelund

ddm-admin-client/src/lib.rs

@@ -23,5 +23,8 @@ progenitor::generate_api!(
         IpPrefix = ddm_api_types_versions::latest::net::IpPrefix,
         Ipv4Prefix = ddm_api_types_versions::latest::net::Ipv4Prefix,
         Ipv6Prefix = ddm_api_types_versions::latest::net::Ipv6Prefix,
+        PeerInfo = ddm_api_types_versions::latest::db::PeerInfo,
+        PeerStatus = ddm_api_types_versions::latest::db::PeerStatus,
+        Duration = std::time::Duration,
     }
 );

ddm-api-types/versions/src/latest.rs

@@ -11,10 +11,11 @@ pub mod admin {
 }
 
 pub mod db {
-    pub use crate::v1::db::PeerInfo;
-    pub use crate::v1::db::PeerStatus;
     pub use crate::v1::db::RouterKind;
     pub use crate::v1::db::TunnelRoute;
+
+    pub use crate::v2::db::PeerInfo;
+    pub use crate::v2::db::PeerStatus;
 }
 
 pub mod exchange {

ddm-api-types/versions/src/lib.rs

@@ -32,3 +32,5 @@
 pub mod latest;
 #[path = "initial/mod.rs"]
 pub mod v1;
+#[path = "peer_durations/mod.rs"]
+pub mod v2;

ddm-api-types/versions/src/peer_durations/db.rs

@@ -0,0 +1,48 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+use std::net::Ipv6Addr;
+use std::time::Duration;
+
+use crate::v1::db::RouterKind;
+use schemars::JsonSchema;
+use serde::{Deserialize, Serialize};
+
+/// Status of a DDM peer, including how long the peer has been in its
+/// current state.
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
+#[serde(tag = "type", content = "duration")]
+pub enum PeerStatus {
+    Init(Duration),
+    Solicit(Duration),
+    Exchange(Duration),
+    Expired(Duration),
+}
+
+/// Information about a DDM peer.
+#[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize, JsonSchema)]
+pub struct PeerInfo {
+    pub status: PeerStatus,
+    pub addr: Ipv6Addr,
+    pub host: String,
+    pub kind: RouterKind,
+}
+
+// Response backwards-compat: convert v2 PeerInfo to v1 PeerInfo.
+impl From<PeerInfo> for crate::v1::db::PeerInfo {
+    fn from(value: PeerInfo) -> Self {
+        Self {
+            status: match value.status {
+                PeerStatus::Init(_) | PeerStatus::Solicit(_) => {
+                    crate::v1::db::PeerStatus::NoContact
+                }
+                PeerStatus::Exchange(_) => crate::v1::db::PeerStatus::Active,
+                PeerStatus::Expired(_) => crate::v1::db::PeerStatus::Expired,
+            },
+            addr: value.addr,
+            host: value.host,
+            kind: value.kind,
+        }
+    }
+}

ddm-api-types/versions/src/peer_durations/mod.rs

@@ -0,0 +1,10 @@
+// This Source Code Form is subject to the terms of the Mozilla Public
+// License, v. 2.0. If a copy of the MPL was not distributed with this
+// file, You can obtain one at https://mozilla.org/MPL/2.0/.
+
+//! Version `PEER_DURATIONS` of the DDM Admin API.
+//!
+//! Tracks how long each DDM peer has been in its current state and exposes
+//! that through the `/peers` endpoint with duration information.
+
+pub mod db;

ddm-api/src/lib.rs

@@ -3,6 +3,7 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 use ddm_api_types_versions::latest;
+use ddm_api_types_versions::v1;
 use dropshot::HttpError;
 use dropshot::HttpResponseOk;
 use dropshot::HttpResponseUpdatedNoContent;
@@ -25,6 +26,7 @@ api_versions!([
     // |  example for the next person.
     // v
     // (next_int, IDENT),
+    (2, PEER_DURATIONS),
     (1, INITIAL),
 ]);
 
@@ -44,11 +46,21 @@ api_versions!([
 pub trait DdmAdminApi {
     type Context;
 
-    #[endpoint { method = GET, path = "/peers" }]
+    #[endpoint { method = GET, path = "/peers", versions = VERSION_PEER_DURATIONS.. }]
     async fn get_peers(
         ctx: RequestContext<Self::Context>,
     ) -> Result<HttpResponseOk<HashMap<u32, latest::db::PeerInfo>>, HttpError>;
 
+    #[endpoint { method = GET, path = "/peers", versions = ..VERSION_PEER_DURATIONS }]
+    async fn get_peers_v1(
+        ctx: RequestContext<Self::Context>,
+    ) -> Result<HttpResponseOk<HashMap<u32, v1::db::PeerInfo>>, HttpError> {
+        let resp = Self::get_peers(ctx).await?;
+        let converted: HashMap<u32, v1::db::PeerInfo> =
+            resp.0.into_iter().map(|(k, v)| (k, v.into())).collect();
+        Ok(HttpResponseOk(converted))
+    }
+
     #[endpoint { method = DELETE, path = "/peers/{addr}" }]
     async fn expire_peer(
         ctx: RequestContext<Self::Context>,

ddm/src/admin.rs

@@ -122,7 +122,26 @@ impl DdmAdminApi for DdmAdminApiImpl {
         ctx: RequestContext<Self::Context>,
     ) -> Result<HttpResponseOk<HashMap<u32, PeerInfo>>, HttpError> {
         let ctx = lock!(ctx.context());
-        Ok(HttpResponseOk(ctx.db.peers()))
+        let mut result = HashMap::new();
+        for sm in &ctx.peers {
+            // Compute status first so peer_status() never runs while we hold
+            // any of the InterfaceState mutexes below.
+            let status = sm.iface.peer_status();
+            let if_index = *lock!(sm.iface.if_index);
+            let Some(peer) = lock!(sm.iface.peer_identity).clone() else {
+                continue;
+            };
+            result.insert(
+                if_index,
+                PeerInfo {
+                    status,
+                    addr: peer.addr,
+                    host: peer.hostname,
+                    kind: peer.kind,
+                },
+            );
+        }
+        Ok(HttpResponseOk(result))
     }
 
     async fn expire_peer(

ddm/src/db.rs

@@ -2,7 +2,7 @@
 // License, v. 2.0. If a copy of the MPL was not distributed with this
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
-use ddm_api_types::db::{PeerInfo, TunnelRoute};
+use ddm_api_types::db::TunnelRoute;
 use ddm_api_types::net::TunnelOrigin;
 use mg_common::lock;
 use oxnet::{IpNet, Ipv6Net};
@@ -45,7 +45,6 @@ pub struct Db {
 
 #[derive(Default, Clone)]
 pub struct DbData {
-    pub peers: HashMap<u32, PeerInfo>,
     pub imported: HashSet<Route>,
     pub imported_tunnel: HashSet<TunnelRoute>,
 }
@@ -65,10 +64,6 @@ impl Db {
         lock!(self.data).clone()
     }
 
-    pub fn peers(&self) -> HashMap<u32, PeerInfo> {
-        lock!(self.data).peers.clone()
-    }
-
     pub fn imported(&self) -> HashSet<Route> {
         lock!(self.data).imported.clone()
     }
@@ -221,15 +216,6 @@ impl Db {
         Ok(())
     }
 
-    /// Set peer info at the given index. Returns true if peer information was
-    /// changed.
-    pub fn set_peer(&self, index: u32, info: PeerInfo) -> bool {
-        match lock!(self.data).peers.insert(index, info.clone()) {
-            Some(previous) => previous == info,
-            None => true,
-        }
-    }
-
     pub fn remove_nexthop_routes(
         &self,
         nexthop: Ipv6Addr,
@@ -259,10 +245,6 @@ impl Db {
         (removed, tnl_removed)
     }
 
-    pub fn remove_peer(&self, index: u32) {
-        lock!(self.data).peers.remove(&index);
-    }
-
     pub fn routes_by_vector(
         &self,
         dst: Ipv6Net,

ddm/src/discovery/runtime.rs

@@ -8,10 +8,11 @@
 //! illumos-only.
 
 use super::{DiscoveryError, Version};
-use crate::db::Db;
-use crate::sm::{Config, Event, NeighborEvent, SessionStats};
+use crate::sm::{
+    Config, Event, InterfaceState, NeighborEvent, PeerIdentity, SessionStats,
+};
 use crate::{dbg, err, inf, trc, wrn};
-use ddm_api_types::db::{PeerInfo, PeerStatus, RouterKind};
+use ddm_api_types::db::RouterKind;
 use mg_common::lock;
 use serde::{Deserialize, Serialize};
 use slog::Logger;
@@ -80,7 +81,7 @@ struct HandlerContext {
     nbr: Arc<RwLock<Option<Neighbor>>>,
     log: Logger,
     event: Sender<Event>,
-    db: Db,
+    iface: Arc<InterfaceState>,
 }
 
 struct Neighbor {
@@ -94,7 +95,7 @@ pub(crate) fn handler(
     hostname: String,
     config: Config,
     event: Sender<Event>,
-    db: Db,
+    iface: Arc<InterfaceState>,
     stats: Arc<SessionStats>,
     log: Logger,
 ) -> Result<(), DiscoveryError> {
@@ -134,7 +135,7 @@ pub(crate) fn handler(
         hostname,
         event,
         config,
-        db,
+        iface,
     };
 
     let stop = Arc::new(AtomicBool::new(false));
@@ -426,17 +427,15 @@ fn handle_advertisement(
         }
     };
     drop(guard);
-    let updated = ctx.db.set_peer(
-        ctx.config.if_index,
-        PeerInfo {
-            status: PeerStatus::Active,
-            addr: *sender,
-            host: hostname,
-            kind,
-        },
-    );
-    if updated {
-        lock!(stats.peer_address).replace(*sender);
+    let new_peer = PeerIdentity {
+        addr: *sender,
+        hostname,
+        kind,
+    };
+    let mut info = lock!(ctx.iface.peer_identity);
+    if info.as_ref() != Some(&new_peer) {
+        *info = Some(new_peer);
+        drop(info);
         emit_nbr_update(ctx, sender, version);
     }
 }