more review fixes

Author: andrewjstone

nexus/src/app/background/tasks/trust_quorum.rs

@@ -251,11 +251,10 @@ async fn drive_reconfiguration(
     }
 
     // For each unacked node, need to send a `Commit` or `PrepareAndCommit'
-    if config.state.is_committing() {
-        return commit(log, opctx, datastore, config).await;
-    }
-
-    Ok(Status::ConfigInactive)
+    //
+    // At this point we know that the configuration is active and we are not
+    // preparing. Therefore we must be committing.
+    commit(log, opctx, datastore, config).await
 }
 
 async fn prepare(
@@ -347,6 +346,13 @@ async fn commit(
         }
     }
 
+    if ops.is_empty() {
+        bail!(
+            "Unexpected error. No members to commit, \
+            even though configuration was active."
+        );
+    }
+
     //
     // Get a set of random clients. We can proxy requests if these clients are
     // different from the members that need committing.
@@ -395,7 +401,7 @@ async fn commit(
     let ops_per_client = ops.len() / clients.len();
     let num_clients = clients.len();
     let mut client_destinations_by_worker_task_id = BTreeMap::new();
-    for (i, client) in clients.into_iter().enumerate() {
+    for (i, (client_dest, client)) in clients.into_iter().enumerate() {
         let config = config.clone();
         let client_ops = if i + 1 == num_clients {
             // Take all remaining ops for the last client.
@@ -406,9 +412,9 @@ async fn commit(
             // always drain from the front.
             ops.drain(..ops_per_client).collect()
         };
-        let client_dest = client.0.clone();
+        let client_dest2 = client_dest.clone();
         let handle = workers.spawn(async move {
-            run_commit_ops(client.0, client.1, client_ops, config).await
+            run_commit_ops(client_dest2, client, client_ops, config).await
         });
         client_destinations_by_worker_task_id.insert(handle.id(), client_dest);
     }

nexus/src/app/trust_quorum.rs

@@ -80,7 +80,7 @@ impl super::Nexus {
                         the abort to the database also failed with error: {e}. \
                         Abort will have to be performed explicitly by the \
                         operator.",
-                        msg.clone()
+                        msg
                     ))
                 })?;
 
@@ -137,7 +137,7 @@ impl super::Nexus {
         if !intersection.is_empty() {
             return Err(Error::invalid_request(format!(
                 "The following sleds are already members of the trust quorum: \
-                 {intersection:?}. Is there a problem with their sled agents?"
+                 {intersection:?}."
             )));
         }
 

nexus/types/src/external_api/views.rs

@@ -637,15 +637,22 @@ pub struct RackMembershipChange {
 
 impl From<TrustQuorumConfig> for RackMembershipChange {
     fn from(value: TrustQuorumConfig) -> Self {
+        // `Unacked` means that a member has not received and acked a `Prepare`
+        // yet. `Prepared` means that a member has acknolwedged the prepare but
+        // not the commit. `Committed` is when the member starts participating
+        // in the new group.
+        //
+        // Since we don't want to expose trust quorum specific knowledge to
+        // the operator, and they really only want to know when the membership
+        // change has started to take effect, we say that any member that hasn't
+        // yet committed is unacknowledged.
         let unacknowledged_members = value
             .members
             .iter()
-            .filter_map(|(id, data)| {
-                if data.state == TrustQuorumMemberState::Committed {
-                    Some(id.clone())
-                } else {
-                    None
-                }
+            .filter_map(|(id, data)| match data.state {
+                TrustQuorumMemberState::Unacked => Some(id.clone()),
+                TrustQuorumMemberState::Prepared => Some(id.clone()),
+                TrustQuorumMemberState::Committed => None,
             })
             .collect();
         let state = if value.state.is_committed() {