fixes, docs

nexus/db-queries/src/db/datastore/dns.rs

@@ -873,7 +873,7 @@ mod test {
                 NonZeroU32::new(1).unwrap(),
                 &DnsVersion {
                     dns_group: DnsGroup::Internal,
-                    version: Generation(1.try_into().unwrap()),
+                    version: Generation(1u32.try_into().unwrap()),
                     time_created: dns_config.time_created,
                     creator: "unused".to_string(),
                     comment: "unused".to_string(),
@@ -938,9 +938,9 @@ mod test {
         let z2_id = Uuid::new_v4();
         let z3_id = Uuid::new_v4();
         let zinternal_id = Uuid::new_v4();
-        let g1 = Generation(1.try_into().unwrap());
-        let g2 = Generation(2.try_into().unwrap());
-        let g3 = Generation(3.try_into().unwrap());
+        let g1 = Generation(1u32.try_into().unwrap());
+        let g2 = Generation(2u32.try_into().unwrap());
+        let g3 = Generation(3u32.try_into().unwrap());
         let v1 = DnsVersion {
             dns_group: DnsGroup::External,
             version: g1,
@@ -1320,8 +1320,8 @@ mod test {
             use crate::db::schema::dns_name::dsl;
             let dns_zone_id = Uuid::new_v4();
             let name = "n1".to_string();
-            let g1 = Generation(1.try_into().unwrap());
-            let g2 = Generation(2.try_into().unwrap());
+            let g1 = Generation(1u32.try_into().unwrap());
+            let g2 = Generation(2u32.try_into().unwrap());
             let r1 = DnsRecord::Aaaa("fe80::1:2:3:4".parse().unwrap());
             let r2 = DnsRecord::Aaaa("fe80::1:1:1:1".parse().unwrap());
 

nexus/src/app/background/common.rs

@@ -283,68 +283,39 @@ impl Driver {
         self.tasks.keys()
     }
 
-    // XXX-dap helper function
-
-    /// Returns a summary of what this task does (for developers)
-    pub fn task_description(&self, task: &TaskHandle) -> &str {
+    fn task_required(&self, task: &TaskHandle) -> &Task {
         // It should be hard to hit this in practice, since you'd have to have
         // gotten a TaskHandle from somewhere.  It would have to be another
-        // Driver instance.
-        let task = self.tasks.get(task).unwrap_or_else(|| {
-            panic!(
-                "attempted to get docs for non-existent background task: {:?}",
-                task
-            )
-        });
+        // Driver instance.  But it's generally a singleton.
+        self.tasks.get(task).unwrap_or_else(|| {
+            panic!("attempted to get non-existent background task: {:?}", task)
+        })
+    }
 
-        &task.description
+    /// Returns a summary of what this task does (for developers)
+    pub fn task_description(&self, task: &TaskHandle) -> &str {
+        &self.task_required(task).description
     }
 
     /// Returns the configured period of the task
     pub fn task_period(&self, task: &TaskHandle) -> Duration {
-        let task = self.tasks.get(task).unwrap_or_else(|| {
-            panic!(
-                "attempted to get period of non-existent background task: {:?}",
-                task
-            )
-        });
-        task.period
+        self.task_required(task).period
     }
 
     /// Activate the specified background task
     ///
     /// If the task is currently running, it will be activated again when it
     /// finishes.
     pub fn activate(&self, task: &TaskHandle) {
-        // It should be hard to hit this in practice, since you'd have to have
-        // gotten a TaskHandle from somewhere.  It would have to be another
-        // Driver instance.
-        let task = self.tasks.get(task).unwrap_or_else(|| {
-            panic!(
-                "attempted to wake up non-existent background task: {:?}",
-                task
-            )
-        });
-
-        task.notify.notify_one();
+        self.task_required(task).notify.notify_one();
     }
 
     /// Returns the runtime status of the background task
     pub fn task_status(&self, task: &TaskHandle) -> TaskStatus {
-        // It should be hard to hit this in practice, since you'd have to have
-        // gotten a TaskHandle from somewhere.  It would have to be another
-        // Driver instance.
-        let task = self.tasks.get(task).unwrap_or_else(|| {
-            panic!(
-                "attempted to get status of non-existent background task: {:?}",
-                task
-            )
-        });
-
         // Borrowing from a watch channel's receiver blocks the sender.  Clone
         // the status to avoid an errant caller gumming up the works by hanging
         // on to a reference.
-        task.status.borrow().clone()
+        self.task_required(task).status.borrow().clone()
     }
 }
 

nexus/src/app/background/init.rs

@@ -380,7 +380,7 @@ pub mod test {
                         diesel::insert_into(dsl::dns_version)
                             .values(DnsVersion {
                                 dns_group: DnsGroup::Internal,
-                                version: Generation(2.try_into().unwrap()),
+                                version: Generation(2u32.try_into().unwrap()),
                                 time_created: chrono::Utc::now(),
                                 creator: String::from("test suite"),
                                 comment: String::from("test suite"),
@@ -399,7 +399,7 @@ pub mod test {
                                 DnsName::new(
                                     internal_dns_zone_id,
                                     String::from("we-got-beets"),
-                                    Generation(2.try_into().unwrap()),
+                                    Generation(2u32.try_into().unwrap()),
                                     None,
                                     vec![nexus_params::DnsRecord::Aaaa(
                                         "fe80::3".parse().unwrap(),

nexus/src/app/external_endpoints.rs

@@ -73,7 +73,6 @@ use uuid::Uuid;
 pub struct ExternalEndpoints {
     by_dns_name: BTreeMap<String, Arc<ExternalEndpoint>>,
     warnings: Vec<ExternalEndpointError>,
-    // XXX-dap should serialize some other way?
     default_endpoint: Option<Arc<ExternalEndpoint>>,
 }
 

omdb/src/bin/omdb/db.rs

@@ -3,6 +3,14 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 //! omdb commands that query or update the database
+//!
+//! GROUND RULES: There aren't many ground rules (see top-level docs).  But
+//! where possible, stick to operations provided by `DataStore` rather than
+//! querying the database directly.  The DataStore operations generally provide
+//! a safer level of abstraction.  But there are cases where we want to do
+//! things that really don't need to be in the DataStore -- i.e., where `omdb`
+//! would be the only consumer -- and in that case it's okay to query the
+//! database directly.
 
 use anyhow::anyhow;
 use anyhow::bail;
@@ -435,7 +443,6 @@ async fn cmd_db_sleds(
 }
 
 // DNS
-// XXX-dap add "history" command?
 
 /// Run `omdb db dns show`.
 async fn cmd_db_dns_show(

omdb/src/bin/omdb/main.rs

@@ -3,6 +3,21 @@
 // file, You can obtain one at https://mozilla.org/MPL/2.0/.
 
 //! CLI for debugging Omicron internal state
+//!
+//! GROUND RULES:
+//!
+//! 1. There aren't a lot of ground rules here.  At least for now, this is a
+//!    place to put any kind of runtime tooling for Omicron that seems useful.
+//!    You can query the database directly (see notes in db.rs), use internal
+//!    APIs, etc.  To the degree that we can stick to stable interfaces, great.
+//!    But at this stage we'd rather have tools that work on latest than not
+//!    have them because we couldn't prioritize keeping them stable.
+//!
+//! 2. Where possible, when the tool encounters something unexpected, it should
+//!    print what it can (including the error message and bad data) and then
+//!    continue.  It generally shouldn't stop on the first error.  (We often
+//!    find strange things when debugging but we need our tools to tell us as
+//!    much as they can!)
 
 use anyhow::Context;
 use clap::Parser;