From afc564e7cfbfa15bac6a14e69b83469629afae3a Mon Sep 17 00:00:00 2001 From: Rain Date: Mon, 12 Aug 2024 11:14:03 -0700 Subject: [PATCH] [sled-agent] remove NexusClientWithResolver (#6283) Previously, we would carry around a DNS resolver -- however, the resolver has been unused for a while. And in fact there's a warning about being careful around using it. Because the type was public, the rustc dead code detector didn't figure that out until my change to switch to an API trait in #6159 exposed this. Remove `NexusClientWithResolver`, adding a couple of free functions to make a `NexusClient` instead. --- sled-agent/src/instance.rs | 27 +++++----- sled-agent/src/instance_manager.rs | 8 +-- sled-agent/src/nexus.rs | 80 ++++++++++-------------------- sled-agent/src/probe_manager.rs | 8 ++- sled-agent/src/server.rs | 5 +- sled-agent/src/sled_agent.rs | 12 ++--- sled-agent/src/updates.rs | 2 +- 7 files changed, 53 insertions(+), 89 deletions(-) diff --git a/sled-agent/src/instance.rs b/sled-agent/src/instance.rs index 631f2b83f6..5dc4e1e6a2 100644 --- a/sled-agent/src/instance.rs +++ b/sled-agent/src/instance.rs @@ -12,7 +12,7 @@ use crate::instance_manager::{ Error as ManagerError, InstanceManagerServices, InstanceTicket, }; use crate::metrics::MetricsRequestQueue; -use crate::nexus::NexusClientWithResolver; +use crate::nexus::NexusClient; use crate::params::ZoneBundleMetadata; use crate::params::{InstanceExternalIpBody, ZoneBundleCause}; use crate::params::{ @@ -349,7 +349,7 @@ struct InstanceRunner { running_state: Option, // Connection to Nexus - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, // Storage resources storage: StorageHandle, @@ -528,7 +528,6 @@ impl InstanceRunner { ); self.nexus_client - .client() .cpapi_instances_put( &self.id().into_untyped_uuid(), &state.into(), @@ -1568,6 +1567,7 @@ mod tests { use super::*; use crate::fakes::nexus::{FakeNexusServer, ServerContext}; use crate::metrics; + use crate::nexus::make_nexus_client_with_port; use crate::vmm_reservoir::VmmReservoirManagerHandle; use crate::zone_bundle::CleanupContext; use camino_tempfile::Utf8TempDir; @@ -1634,7 +1634,7 @@ mod tests { } struct FakeNexusParts { - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, _nexus_server: HttpServer, state_rx: Receiver, _dns_server: TransientServer, @@ -1662,12 +1662,11 @@ mod tests { .unwrap(), ); - let nexus_client = - NexusClientWithResolver::new_from_resolver_with_port( - &log, - resolver, - _nexus_server.local_addr().port(), - ); + let nexus_client = make_nexus_client_with_port( + &log, + resolver, + _nexus_server.local_addr().port(), + ); Self { nexus_client, _nexus_server, state_rx, _dns_server } } @@ -1760,7 +1759,7 @@ mod tests { async fn instance_struct( log: &Logger, propolis_addr: SocketAddr, - nexus_client_with_resolver: NexusClientWithResolver, + nexus_client: NexusClient, storage_handle: StorageHandle, temp_dir: &String, ) -> (Instance, MetricsRx) { @@ -1774,7 +1773,7 @@ mod tests { let (services, rx) = fake_instance_manager_services( log, storage_handle, - nexus_client_with_resolver, + nexus_client, temp_dir, ); @@ -1850,7 +1849,7 @@ mod tests { fn fake_instance_manager_services( log: &Logger, storage_handle: StorageHandle, - nexus_client_with_resolver: NexusClientWithResolver, + nexus_client: NexusClient, temp_dir: &String, ) -> (InstanceManagerServices, MetricsRx) { let vnic_allocator = @@ -1869,7 +1868,7 @@ mod tests { let (metrics_queue, rx) = MetricsRequestQueue::for_test(); let services = InstanceManagerServices { - nexus_client: nexus_client_with_resolver, + nexus_client, vnic_allocator, port_manager, storage: storage_handle, diff --git a/sled-agent/src/instance_manager.rs b/sled-agent/src/instance_manager.rs index 1b2fb204d0..fe070464ad 100644 --- a/sled-agent/src/instance_manager.rs +++ b/sled-agent/src/instance_manager.rs @@ -7,7 +7,7 @@ use crate::instance::propolis_zone_name; use crate::instance::Instance; use crate::metrics::MetricsRequestQueue; -use crate::nexus::NexusClientWithResolver; +use crate::nexus::NexusClient; use crate::params::InstanceExternalIpBody; use crate::params::InstanceMetadata; use crate::params::ZoneBundleMetadata; @@ -74,7 +74,7 @@ pub enum Error { } pub(crate) struct InstanceManagerServices { - pub nexus_client: NexusClientWithResolver, + pub nexus_client: NexusClient, pub vnic_allocator: VnicAllocator, pub port_manager: PortManager, pub storage: StorageHandle, @@ -103,7 +103,7 @@ impl InstanceManager { #[allow(clippy::too_many_arguments)] pub fn new( log: Logger, - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, etherstub: Etherstub, port_manager: PortManager, storage: StorageHandle, @@ -422,7 +422,7 @@ struct InstanceManagerRunner { terminate_tx: mpsc::UnboundedSender, terminate_rx: mpsc::UnboundedReceiver, - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, // TODO: If we held an object representing an enum of "Created OR Running" // instance, we could avoid the methods within "instance.rs" that panic diff --git a/sled-agent/src/nexus.rs b/sled-agent/src/nexus.rs index 9565e34b1f..26b0e3de59 100644 --- a/sled-agent/src/nexus.rs +++ b/sled-agent/src/nexus.rs @@ -2,12 +2,11 @@ // 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/. -pub use nexus_client::Client as NexusClient; use omicron_common::api::external::Generation; use omicron_common::disk::DiskVariant; use crate::vmm_reservoir::VmmReservoirManagerHandle; -use internal_dns::resolver::{ResolveError, Resolver}; +use internal_dns::resolver::Resolver; use internal_dns::ServiceName; use nexus_client::types::SledAgentInfo; use omicron_common::address::NEXUS_INTERNAL_PORT; @@ -19,62 +18,33 @@ use tokio::sync::{broadcast, mpsc, oneshot, Notify}; use tokio::time::{interval, Duration, MissedTickBehavior}; use uuid::Uuid; -/// A thin wrapper over a progenitor-generated NexusClient. -/// -/// Also attaches the "DNS resolver" for historical reasons. -#[derive(Clone)] -pub struct NexusClientWithResolver { - client: NexusClient, +// Re-export the nexus_client::Client crate. (Use a type alias to be more +// rust-analyzer friendly.) +pub(crate) type NexusClient = nexus_client::Client; + +pub(crate) fn make_nexus_client( + log: &Logger, resolver: Arc, +) -> NexusClient { + make_nexus_client_with_port(log, resolver, NEXUS_INTERNAL_PORT) } -impl NexusClientWithResolver { - pub fn new( - log: &Logger, - resolver: Arc, - ) -> Result { - Ok(Self::new_from_resolver_with_port( - log, - resolver, - NEXUS_INTERNAL_PORT, - )) - } - - pub fn new_from_resolver_with_port( - log: &Logger, - resolver: Arc, - port: u16, - ) -> Self { - let client = reqwest::ClientBuilder::new() - .dns_resolver(resolver.clone()) - .build() - .expect("Failed to build client"); - - let dns_name = ServiceName::Nexus.srv_name(); - Self { - client: NexusClient::new_with_client( - &format!("http://{dns_name}:{port}"), - client, - log.new(o!("component" => "NexusClient")), - ), - resolver, - } - } - - /// Access the progenitor-based Nexus Client. - pub fn client(&self) -> &NexusClient { - &self.client - } - - /// Access the DNS resolver used by the Nexus Client. - /// - /// WARNING: If you're using this resolver to access an IP address of - /// another service, be aware that it might change if that service moves - /// around! Be cautious when accessing and persisting IP addresses of other - /// services. - pub fn resolver(&self) -> &Arc { - &self.resolver - } +pub(crate) fn make_nexus_client_with_port( + log: &Logger, + resolver: Arc, + port: u16, +) -> NexusClient { + let client = reqwest::ClientBuilder::new() + .dns_resolver(resolver) + .build() + .expect("Failed to build client"); + + let dns_name = ServiceName::Nexus.srv_name(); + NexusClient::new_with_client( + &format!("http://{dns_name}:{port}"), + client, + log.new(o!("component" => "NexusClient")), + ) } pub fn d2n_params( diff --git a/sled-agent/src/probe_manager.rs b/sled-agent/src/probe_manager.rs index fa2e9dfa3d..529ef392b7 100644 --- a/sled-agent/src/probe_manager.rs +++ b/sled-agent/src/probe_manager.rs @@ -1,5 +1,5 @@ use crate::metrics::MetricsRequestQueue; -use crate::nexus::NexusClientWithResolver; +use crate::nexus::NexusClient; use anyhow::{anyhow, Result}; use illumos_utils::dladm::Etherstub; use illumos_utils::link::VnicAllocator; @@ -54,7 +54,7 @@ struct RunningProbes { pub(crate) struct ProbeManagerInner { join_handle: Mutex>>, - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, log: Logger, sled_id: Uuid, vnic_allocator: VnicAllocator, @@ -67,7 +67,7 @@ pub(crate) struct ProbeManagerInner { impl ProbeManager { pub(crate) fn new( sled_id: Uuid, - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, etherstub: Etherstub, storage: StorageHandle, port_manager: PortManager, @@ -248,7 +248,6 @@ impl ProbeManagerInner { if n_added > 0 { if let Err(e) = self .nexus_client - .client() .bgtask_activate(&BackgroundTasksActivateRequest { bgtask_names: vec!["vpc_route_manager".into()], }) @@ -439,7 +438,6 @@ impl ProbeManagerInner { async fn target_state(self: &Arc) -> Result> { Ok(self .nexus_client - .client() .probes_get( &self.sled_id, None, //limit diff --git a/sled-agent/src/server.rs b/sled-agent/src/server.rs index ec86066096..43fb64914f 100644 --- a/sled-agent/src/server.rs +++ b/sled-agent/src/server.rs @@ -9,7 +9,7 @@ use super::http_entrypoints::api as http_api; use super::sled_agent::SledAgent; use crate::bootstrap::params::StartSledAgentRequest; use crate::long_running_tasks::LongRunningTaskHandles; -use crate::nexus::NexusClientWithResolver; +use crate::nexus::make_nexus_client; use crate::services::ServiceManager; use internal_dns::resolver::Resolver; use slog::Logger; @@ -52,8 +52,7 @@ impl Server { .map_err(|e| e.to_string())?, ); - let nexus_client = NexusClientWithResolver::new(&log, resolver) - .map_err(|e| e.to_string())?; + let nexus_client = make_nexus_client(&log, resolver); let sled_agent = SledAgent::new( &config, diff --git a/sled-agent/src/sled_agent.rs b/sled-agent/src/sled_agent.rs index f8454a0f7b..d87df0d7c5 100644 --- a/sled-agent/src/sled_agent.rs +++ b/sled-agent/src/sled_agent.rs @@ -13,8 +13,7 @@ use crate::instance_manager::InstanceManager; use crate::long_running_tasks::LongRunningTaskHandles; use crate::metrics::MetricsManager; use crate::nexus::{ - NexusClientWithResolver, NexusNotifierHandle, NexusNotifierInput, - NexusNotifierTask, + NexusClient, NexusNotifierHandle, NexusNotifierInput, NexusNotifierTask, }; use crate::params::{ DiskStateRequested, InstanceExternalIpBody, InstanceHardware, @@ -320,7 +319,7 @@ struct SledAgentInner { services: ServiceManager, // Connection to Nexus. - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, // A mechanism for notifiying nexus about sled-agent updates nexus_notifier: NexusNotifierHandle, @@ -365,7 +364,7 @@ impl SledAgent { pub async fn new( config: &Config, log: Logger, - nexus_client: NexusClientWithResolver, + nexus_client: NexusClient, request: StartSledAgentRequest, services: ServiceManager, long_running_task_handles: LongRunningTaskHandles, @@ -552,7 +551,7 @@ impl SledAgent { let nexus_notifier_input = NexusNotifierInput { sled_id: request.body.id, sled_address: get_sled_address(request.body.subnet), - nexus_client: nexus_client.client().clone(), + nexus_client: nexus_client.clone(), hardware: long_running_task_handles.hardware_manager.clone(), vmm_reservoir_manager: vmm_reservoir_manager.clone(), }; @@ -688,7 +687,6 @@ impl SledAgent { self.inner .nexus_client - .client() .sled_firewall_rules_request(&sled_id) .await .map_err(|err| Error::FirewallRequest(err))?; @@ -1074,7 +1072,7 @@ impl SledAgent { ) -> Result<(), Error> { self.inner .updates - .download_artifact(artifact, &self.inner.nexus_client.client()) + .download_artifact(artifact, &self.inner.nexus_client) .await?; Ok(()) } diff --git a/sled-agent/src/updates.rs b/sled-agent/src/updates.rs index 9193a855b0..a928abe9b3 100644 --- a/sled-agent/src/updates.rs +++ b/sled-agent/src/updates.rs @@ -252,8 +252,8 @@ impl UpdateManager { mod test { use super::*; use crate::fakes::nexus::FakeNexusServer; + use crate::nexus::NexusClient; use flate2::write::GzEncoder; - use nexus_client::Client as NexusClient; use omicron_common::api::external::{Error, SemverVersion}; use omicron_common::api::internal::nexus::UpdateArtifactId; use omicron_test_utils::dev::test_setup_log;