(part 2 of 2) add test that Nexus propagates DNS on startup/rack initialization

Author: davepacheco

Cargo.lock

@@ -90,6 +90,7 @@ oximeter-producer.workspace = true
 [dev-dependencies]
 assert_matches.workspace = true
 criterion.workspace = true
+dns-server.workspace = true
 expectorate.workspace = true
 hyper-rustls.workspace = true
 itertools.workspace = true

nexus/Cargo.toml

@@ -1041,8 +1041,9 @@ mod test {
         // Create a sled on which the service should exist.
         let sled_id = create_test_sled(&datastore).await;
 
-        // Create a new service to exist on this sled.
-        let service_id = Uuid::new_v4();
+        // Create a few new service to exist on this sled.
+        let service1_id =
+            "ab7bd7fd-7c37-48ab-a84a-9c09a90c4c7f".parse().unwrap();
         let addr = SocketAddrV6::new(Ipv6Addr::LOCALHOST, 123, 0, 0);
         let kind = ServiceKind::Nexus;
 
@@ -1053,7 +1054,8 @@ mod test {
         assert_eq!(service1.ip, result.ip);
         assert_eq!(service1.kind, result.kind);
 
-        let service2_id = Uuid::new_v4();
+        let service2_id =
+            "fe5b6e3d-dfee-47b4-8719-c54f78912c0b".parse().unwrap();
         let service2 = Service::new(service2_id, sled_id, addr, kind);
         let result =
             datastore.service_upsert(&opctx, service2.clone()).await.unwrap();

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

@@ -24,25 +24,26 @@ pub fn init(
     opctx: &OpContext,
     datastore: Arc<DataStore>,
     config: &BackgroundTaskConfig,
-) -> (common::Driver, common::TaskHandle) {
+) -> (common::Driver, common::TaskHandle, common::TaskHandle, common::TaskHandle)
+{
     let mut driver = common::Driver::new();
 
-    let rv = init_dns(
+    let (task_config, task_servers) = init_dns(
         &mut driver,
         opctx,
         datastore.clone(),
         DnsGroup::Internal,
         &config.dns_internal,
     );
-    let _ = init_dns(
+    let (_, external_task_servers) = init_dns(
         &mut driver,
         opctx,
         datastore,
         DnsGroup::External,
         &config.dns_external,
     );
 
-    (driver, rv)
+    (driver, task_config, task_servers, external_task_servers)
 }
 
 fn init_dns(
@@ -51,28 +52,26 @@ fn init_dns(
     datastore: Arc<DataStore>,
     dns_group: DnsGroup,
     config: &DnsTasksConfig,
-) -> common::TaskHandle {
+) -> (common::TaskHandle, common::TaskHandle) {
     let dns_group_name = dns_group.to_string();
     let metadata = BTreeMap::from([("dns_group".to_string(), dns_group_name)]);
 
     // Background task: DNS config watcher
     let dns_config =
         dns_config::DnsConfigWatcher::new(Arc::clone(&datastore), dns_group);
     let dns_config_watcher = dns_config.watcher();
-    let rv = driver
-        .register(
-            format!("dns_config_{}", dns_group),
-            config.period_secs_config,
-            Box::new(dns_config),
-            opctx.child(metadata.clone()),
-            vec![],
-        )
-        .clone();
+    let task_config = driver.register(
+        format!("dns_config_{}", dns_group),
+        config.period_secs_config,
+        Box::new(dns_config),
+        opctx.child(metadata.clone()),
+        vec![],
+    );
 
     // Background task: DNS server list watcher
     let dns_servers = dns_servers::DnsServersWatcher::new(datastore, dns_group);
     let dns_servers_watcher = dns_servers.watcher();
-    driver.register(
+    let task_servers = driver.register(
         format!("dns_servers_{}", dns_group),
         config.period_secs_servers,
         Box::new(dns_servers),
@@ -94,7 +93,7 @@ fn init_dns(
         vec![Box::new(dns_config_watcher), Box::new(dns_servers_watcher)],
     );
 
-    rv
+    (task_config, task_servers)
 }
 
 #[cfg(test)]
@@ -107,21 +106,28 @@ mod test {
     use nexus_db_queries::context::OpContext;
     use nexus_test_utils_macros::nexus_test;
     use nexus_types::internal_api::params as nexus_params;
+    use nexus_types::internal_api::params::ServiceKind;
     use omicron_common::api::external::DataPageParams;
     use omicron_test_utils::dev::poll;
+    use std::net::SocketAddr;
     use std::num::NonZeroU32;
     use std::time::Duration;
+    use tempfile::TempDir;
+    use uuid::Uuid;
 
     type ControlPlaneTestContext =
         nexus_test_utils::ControlPlaneTestContext<crate::Server>;
 
+    // Nexus is supposed to automatically propagate DNS configuration to all the
+    // DNS servers it knows about.  We'll test two things here:
+    //
+    // (1) create a new DNS server and ensure that it promptly gets the
+    //     existing DNS configuration
+    //
+    // (2) create a new configuration and ensure that both servers promptly get
+    //     the new DNS configuration
     #[nexus_test(server = crate::Server)]
     async fn test_dns_propagation_basic(cptestctx: &ControlPlaneTestContext) {
-        // Nexus is supposed to automatically propagate DNS configuration to all
-        // the DNS servers it knows about.  To test that, we'll first write an
-        // initial DNS configuration to the datastore.  Then we'll wake up the
-        // background task responsible for monitoring that.  We should shortly
-        // see that DNS configuration propagated to the existing DNS server.
         let nexus = &cptestctx.server.apictx().nexus;
         let datastore = nexus.datastore();
         let opctx = OpContext::for_tests(
@@ -142,9 +148,10 @@ mod test {
         // Verify that the DNS server is on version 1.  This should already be
         // the case because it was configured with version 1 when the simulated
         // sled agent started up.
-        let dns_dropshot_server = &cptestctx.sled_agent.dns_dropshot_server;
+        let initial_dns_dropshot_server =
+            &cptestctx.sled_agent.dns_dropshot_server;
         let dns_config_client = dns_service_client::Client::new(
-            &format!("http://{}", dns_dropshot_server.local_addr()),
+            &format!("http://{}", initial_dns_dropshot_server.local_addr()),
             cptestctx.logctx.log.clone(),
         );
         let config = dns_config_client
@@ -173,6 +180,65 @@ mod test {
         );
         let internal_dns_zone_id = dns_zones[0].id;
 
+        // Now spin up another DNS server, add it to the list of servers, and
+        // make sure that DNS gets propagated to it.  Note that we shouldn't
+        // have to explicitly activate the background task because inserting a
+        // new service ought to do that for us.
+        let log = &cptestctx.logctx.log;
+        let storage_path =
+            TempDir::new().expect("Failed to create temporary directory");
+        let config_store = dns_server::storage::Config {
+            keep_old_generations: 3,
+            storage_path: storage_path
+                .path()
+                .to_string_lossy()
+                .into_owned()
+                .into(),
+        };
+        let store = dns_server::storage::Store::new(
+            log.new(o!("component" => "DnsStore")),
+            &config_store,
+        )
+        .unwrap();
+
+        let (_, new_dns_dropshot_server) = dns_server::start_servers(
+            log.clone(),
+            store,
+            &dns_server::dns_server::Config {
+                bind_address: "[::1]:0".parse().unwrap(),
+            },
+            &dropshot::ConfigDropshot {
+                bind_address: "[::1]:0".parse().unwrap(),
+                request_body_max_bytes: 8 * 1024,
+                ..Default::default()
+            },
+        )
+        .await
+        .unwrap();
+
+        let new_dns_addr = match new_dns_dropshot_server.local_addr() {
+            SocketAddr::V4(_) => panic!("expected v6 address"),
+            SocketAddr::V6(a) => a,
+        };
+        nexus
+            .upsert_service(
+                &opctx,
+                Uuid::new_v4(),
+                cptestctx.sled_agent.sled_agent.id,
+                new_dns_addr,
+                ServiceKind::InternalDNSConfig.into(),
+            )
+            .await
+            .unwrap();
+
+        wait_propagate_dns(
+            &cptestctx.logctx.log,
+            "new",
+            new_dns_dropshot_server.local_addr(),
+            1,
+        )
+        .await;
+
         // Now, write version 2 of the internal DNS configuration with one
         // additional record.
         type TxnError = TransactionError<()>;
@@ -225,17 +291,52 @@ mod test {
         }
 
         // Activate the internal DNS propagation pipeline.
-        nexus._background_tasks.activate(&nexus._task_internal_dns_config);
+        nexus.background_tasks.activate(&nexus.task_internal_dns_config);
+
+        // Wait for the new generation to get propagated to both servers.
+        wait_propagate_dns(
+            &cptestctx.logctx.log,
+            "initial",
+            initial_dns_dropshot_server.local_addr(),
+            2,
+        )
+        .await;
 
+        wait_propagate_dns(
+            &cptestctx.logctx.log,
+            "new",
+            new_dns_dropshot_server.local_addr(),
+            2,
+        )
+        .await;
+    }
+
+    /// Verify that DNS gets propagated to the specified server
+    async fn wait_propagate_dns(
+        log: &slog::Logger,
+        label: &str,
+        addr: SocketAddr,
+        generation: u64,
+    ) {
+        println!(
+            "waiting for propagation of generation {} to {} DNS server ({})",
+            generation, label, addr
+        );
+
+        let client = dns_service_client::Client::new(
+            &format!("http://{}", addr),
+            log.clone(),
+        );
         poll::wait_for_condition(
             || async {
-                match dns_config_client.dns_config_get().await {
+                match client.dns_config_get().await {
                     Err(error) => {
-                        // The DNS server is already up.  This shouldn't happen.
+                        // The DNS server is already up.  This shouldn't
+                        // happen.
                         Err(poll::CondCheckError::Failed(error))
                     }
                     Ok(config) => {
-                        if config.generation == 2 {
+                        if config.generation == generation {
                             Ok(())
                         } else {
                             Err(poll::CondCheckError::NotYet)
@@ -244,12 +345,9 @@ mod test {
                 }
             },
             &Duration::from_millis(50),
-            &Duration::from_secs(15),
+            &Duration::from_secs(30),
         )
         .await
         .expect("DNS config not propagated in expected time");
-
-        // XXX-dap then add a second part to the test that adds a new DNS
-        // server.
     }
 }

nexus/src/app/background/init.rs

@@ -168,12 +168,16 @@ pub struct Nexus {
     dpd_client: Arc<dpd_client::Client>,
 
     /// Background tasks
-    // This field is not currently used, but will be when we start activating
-    // background tasks from elsewhere in Nexus.
-    _background_tasks: background::Driver,
+    background_tasks: background::Driver,
 
     /// task handle for the internal DNS config background task
-    _task_internal_dns_config: background::TaskHandle,
+    task_internal_dns_config: background::TaskHandle,
+
+    /// task handle for the internal DNS servers background task
+    task_internal_dns_servers: background::TaskHandle,
+
+    /// task handle for the external DNS servers background task
+    task_external_dns_servers: background::TaskHandle,
 }
 
 impl Nexus {
@@ -253,7 +257,12 @@ impl Nexus {
             authn::Context::internal_api(),
             Arc::clone(&db_datastore),
         );
-        let (background_tasks, task_internal_dns_config) = background::init(
+        let (
+            background_tasks,
+            task_internal_dns_config,
+            task_internal_dns_servers,
+            task_external_dns_servers,
+        ) = background::init(
             &background_ctx,
             Arc::clone(&db_datastore),
             &config.pkg.background_tasks,
@@ -291,8 +300,10 @@ impl Nexus {
             samael_max_issue_delay: std::sync::Mutex::new(None),
             resolver,
             dpd_client,
-            _background_tasks: background_tasks,
-            _task_internal_dns_config: task_internal_dns_config,
+            background_tasks,
+            task_internal_dns_config,
+            task_internal_dns_servers,
+            task_external_dns_servers,
         };
 
         // TODO-cleanup all the extra Arcs here seems wrong
@@ -318,6 +329,31 @@ impl Nexus {
         );
 
         *nexus.recovery_task.lock().unwrap() = Some(recovery_task);
+
+        // Kick all background tasks once the populate step finishes.  Among
+        // other things, the populate step installs role assignments for
+        // internal identities that are used by the background tasks.  If we
+        // don't do this here, those tasks might fail spuriously on startup and
+        // not be retried for a while.
+        let task_nexus = nexus.clone();
+        let task_log = nexus.log.clone();
+        tokio::spawn(async move {
+            match task_nexus.wait_for_populate().await {
+                Ok(_) => {
+                    info!(
+                        task_log,
+                        "populate complete; activating background tasks"
+                    );
+                    for task in task_nexus.background_tasks.tasks() {
+                        task_nexus.background_tasks.activate(task);
+                    }
+                }
+                Err(_) => {
+                    error!(task_log, "populate failed");
+                }
+            }
+        });
+
         nexus
     }
 

nexus/src/app/mod.rs

@@ -155,6 +155,11 @@ impl super::Nexus {
             )
             .await?;
 
+        // We've potentially updated both the list of DNS servers and the DNS
+        // configuration.  Activate both background tasks.
+        self.background_tasks.activate(&self.task_internal_dns_config);
+        self.background_tasks.activate(&self.task_internal_dns_servers);
+
         Ok(())
     }