Rework locking to avoid holding instance-map lock for a long time

Rather than making the instance-map lock async, and holding it for the
duration of the zone bundle operation, we keep it sync. Instead, clone
the instance itself, which is pretty cheap since it's in an `Arc`. This
keeps the code simpler and should improve overall throughput.

Also makes a small change to the zone bundle filename for zone-specific
commands, to ensure they don't collide.

Author: bnaecker

sled-agent/src/instance.rs

@@ -13,7 +13,6 @@ use crate::nexus::NexusClientWithResolver;
 use crate::params::{
     InstanceHardware, InstanceMigrationSourceParams,
     InstanceMigrationTargetParams, InstanceStateRequested, VpcFirewallRule,
-    ZoneBundleCause,
 };
 use crate::profile::*;
 use crate::storage_manager::StorageResources;
@@ -35,6 +34,7 @@ use omicron_common::api::internal::shared::{
 };
 use omicron_common::backoff;
 //use propolis_client::generated::DiskRequest;
+use crate::params::ZoneBundleCause;
 use crate::params::ZoneBundleMetadata;
 use crate::zone_bundle;
 use crate::zone_bundle::BundleError;
@@ -155,7 +155,9 @@ fn fmri_name() -> String {
     format!("{}:default", service_name())
 }
 
-fn propolis_zone_name(id: &Uuid) -> String {
+/// Return the expected name of a Propolis zone managing an instace with the
+/// provided ID.
+pub fn propolis_zone_name(id: &Uuid) -> String {
     format!("{}{}", PROPOLIS_ZONE_PREFIX, id)
 }
 
@@ -589,20 +591,6 @@ impl InstanceInner {
     }
 }
 
-/// A result asking an instance to generate a zone bundle.
-#[derive(Debug)]
-pub enum InstanceZoneBundleResult {
-    /// A zone bundle was operation was performed on the requested zone.
-    Ok(Result<ZoneBundleMetadata, BundleError>),
-
-    /// A request to create a bundle for a zone whose name does not match this
-    /// one's Propolis zone.
-    NotMe,
-
-    /// The zone is not currently running.
-    NotRunning,
-}
-
 /// A reference to a single instance running a running Propolis server.
 ///
 /// Cloning this object clones the reference - it does not create another
@@ -675,22 +663,15 @@ impl Instance {
         Ok(Instance { inner })
     }
 
-    /// Create bundle from a zone matching the provided name.
-    ///
-    /// This returns an [`InstanceZoneBundleResult`] to indicate whether the
-    /// bundle could be correctly taken. See that enum for details.
+    /// Create bundle from an instance zone.
     pub async fn request_zone_bundle(
         &self,
-        name: &str,
-    ) -> InstanceZoneBundleResult {
+    ) -> Result<ZoneBundleMetadata, BundleError> {
         let inner = self.inner.lock().await;
-        let zone_name = propolis_zone_name(inner.propolis_id());
-        if zone_name != name {
-            return InstanceZoneBundleResult::NotMe;
-        }
+        let name = propolis_zone_name(inner.propolis_id());
         match &*inner {
             InstanceInner { running_state: None, .. } => {
-                InstanceZoneBundleResult::NotRunning
+                Err(BundleError::Unavailable { name })
             }
             InstanceInner {
                 ref log,
@@ -699,11 +680,12 @@ impl Instance {
             } => {
                 let context = inner
                     .storage
-                    .zone_bundle_context(name, ZoneBundleCause::ExplicitRequest)
+                    .zone_bundle_context(
+                        &name,
+                        ZoneBundleCause::ExplicitRequest,
+                    )
                     .await;
-                InstanceZoneBundleResult::Ok(
-                    zone_bundle::create(log, running_zone, &context).await,
-                )
+                zone_bundle::create(log, running_zone, &context).await
             }
         }
     }

sled-agent/src/instance_manager.rs

@@ -4,8 +4,8 @@
 
 //! API for controlling multiple instances on a sled.
 
+use crate::instance::propolis_zone_name;
 use crate::instance::Instance;
-use crate::instance::InstanceZoneBundleResult;
 use crate::nexus::NexusClientWithResolver;
 use crate::params::ZoneBundleMetadata;
 use crate::params::{
@@ -23,7 +23,6 @@ use omicron_common::api::internal::nexus::InstanceRuntimeState;
 use slog::Logger;
 use std::collections::BTreeMap;
 use std::sync::{Arc, Mutex};
-use tokio::sync::Mutex as TokioMutex;
 use uuid::Uuid;
 
 #[derive(thiserror::Error, Debug)]
@@ -58,7 +57,7 @@ struct InstanceManagerInternal {
     // instance, we could avoid the methods within "instance.rs" that panic
     // if the Propolis client hasn't been initialized.
     /// A mapping from a Sled Agent "Instance ID" to ("Propolis ID", [Instance]).
-    instances: TokioMutex<BTreeMap<Uuid, (Uuid, Instance)>>,
+    instances: Mutex<BTreeMap<Uuid, (Uuid, Instance)>>,
 
     vnic_allocator: VnicAllocator<Etherstub>,
     port_manager: PortManager,
@@ -86,7 +85,7 @@ impl InstanceManager {
 
                 // no reservoir size set on startup
                 reservoir_size: Mutex::new(ByteCount::from_kibibytes_u32(0)),
-                instances: TokioMutex::new(BTreeMap::new()),
+                instances: Mutex::new(BTreeMap::new()),
                 vnic_allocator: VnicAllocator::new("Instance", etherstub),
                 port_manager,
                 storage,
@@ -176,7 +175,7 @@ impl InstanceManager {
         );
 
         let instance = {
-            let mut instances = self.inner.instances.lock().await;
+            let mut instances = self.inner.instances.lock().unwrap();
             if let Some((existing_propolis_id, existing_instance)) =
                 instances.get(&instance_id)
             {
@@ -233,7 +232,7 @@ impl InstanceManager {
         instance_id: Uuid,
     ) -> Result<InstanceUnregisterResponse, Error> {
         let instance = {
-            let instances = self.inner.instances.lock().await;
+            let instances = self.inner.instances.lock().unwrap();
             let instance = instances.get(&instance_id);
             if let Some((_, instance)) = instance {
                 instance.clone()
@@ -257,7 +256,7 @@ impl InstanceManager {
         target: InstanceStateRequested,
     ) -> Result<InstancePutStateResponse, Error> {
         let instance = {
-            let instances = self.inner.instances.lock().await;
+            let instances = self.inner.instances.lock().unwrap();
             let instance = instances.get(&instance_id);
 
             if let Some((_, instance)) = instance {
@@ -300,7 +299,7 @@ impl InstanceManager {
             .inner
             .instances
             .lock()
-            .await
+            .unwrap()
             .get(&instance_id)
             .ok_or_else(|| Error::NoSuchInstance(instance_id))?
             .clone();
@@ -315,7 +314,7 @@ impl InstanceManager {
         snapshot_id: Uuid,
     ) -> Result<(), Error> {
         let instance = {
-            let instances = self.inner.instances.lock().await;
+            let instances = self.inner.instances.lock().unwrap();
             let (_, instance) = instances
                 .get(&instance_id)
                 .ok_or(Error::NoSuchInstance(instance_id))?;
@@ -333,20 +332,26 @@ impl InstanceManager {
         &self,
         name: &str,
     ) -> Result<ZoneBundleMetadata, BundleError> {
-        for (_propolis_id, instance) in
-            self.inner.instances.lock().await.values()
-        {
-            match instance.request_zone_bundle(name).await {
-                InstanceZoneBundleResult::NotMe => continue,
-                InstanceZoneBundleResult::NotRunning => {
-                    return Err(BundleError::Unavailable {
-                        name: name.to_string(),
-                    });
-                }
-                InstanceZoneBundleResult::Ok(result) => return result,
-            }
-        }
-        Err(BundleError::NoSuchZone { name: name.to_string() })
+        // We need to find the instance and take its lock, but:
+        //
+        // 1. The instance-map lock is sync, and
+        // 2. we don't want to hold the instance-map lock for the entire
+        //    bundling duration.
+        //
+        // Instead, we cheaply clone the instance through its `Arc` around the
+        // `InstanceInner`, which is ultimately what we want.
+        let Some((_propolis_id, instance)) = self
+            .inner
+            .instances
+            .lock()
+            .unwrap()
+            .values()
+            .find(|(propolis_id, _instance)| name == propolis_zone_name(propolis_id))
+            .cloned()
+        else {
+            return Err(BundleError::NoSuchZone { name: name.to_string() });
+        };
+        instance.request_zone_bundle().await
     }
 }
 
@@ -368,15 +373,7 @@ impl InstanceTicket {
     /// themselves after stopping.
     pub fn terminate(&mut self) {
         if let Some(inner) = self.inner.take() {
-            // NOTE: We cannot call methods like `Mutex::blocking_lock()` from
-            // an asynchronous context. We wrap this in a call to
-            // `block_in_place()`. This does block the current thread (and any
-            // concurrent tasks in the same task), but that's not a distinction
-            // from the previous implementation which called
-            // `std::sync::Mutex::lock()` here.
-            tokio::task::block_in_place(|| {
-                inner.instances.blocking_lock().remove(&self.id);
-            });
+            inner.instances.lock().unwrap().remove(&self.id);
         }
     }
 }

sled-agent/src/zone_bundle.rs

@@ -214,7 +214,7 @@ pub async fn create(
     }
 
     // Run any caller-requested zone-specific commands.
-    for cmd in context.zone_specific_commands.iter() {
+    for (i, cmd) in context.zone_specific_commands.iter().enumerate() {
         if cmd.is_empty() {
             continue;
         }
@@ -229,7 +229,12 @@ pub async fn create(
             Err(e) => format!("{}", e),
         };
         let contents = format!("Command: {:?}\n{}", cmd, output).into_bytes();
-        if let Err(e) = insert_data(&mut builder, &cmd[0], &contents) {
+
+        // We'll insert the index into the filename as well, since it's
+        // plausible that users will run multiple executions of the same
+        // command.
+        let filename = format!("zone-specific-{}-{}", i, &cmd[0]);
+        if let Err(e) = insert_data(&mut builder, &filename, &contents) {
             error!(
                 log,
                 "failed to save zone bundle command output";