From ce6bb764afd35577b134b1ad50c2d18230a19818 Mon Sep 17 00:00:00 2001 From: John Baublitz Date: Thu, 23 Mar 2023 15:46:46 -0400 Subject: [PATCH] Change StopPool for partially constructed pools This commit makes two changes: * Switch D-Bus API from D-Bus object path to pool UUID or name to allow stopping partially constructed pools. * Modify engine to tear down partially constructed pools for r6 and later. Otherwise treat partially constructed pools as stopped. --- src/dbus_api/api/manager_3_2/methods.rs | 10 +- src/dbus_api/api/manager_3_6/api.rs | 29 +++++ src/dbus_api/api/manager_3_6/methods.rs | 116 ++++++++++++++++++ src/dbus_api/api/manager_3_6/mod.rs | 4 + src/dbus_api/api/mod.rs | 3 +- src/engine/engine.rs | 6 +- src/engine/sim_engine/engine.rs | 40 ++++-- src/engine/strat_engine/engine.rs | 60 ++++++--- src/engine/strat_engine/liminal/liminal.rs | 14 +++ src/engine/types/actions.rs | 5 + src/jsonrpc/server/pool.rs | 13 +- .../src/stratisd_client_dbus/_introspect.py | 3 +- tests/client-dbus/tests/udev/test_udev.py | 21 ++-- 13 files changed, 269 insertions(+), 55 deletions(-) create mode 100644 src/dbus_api/api/manager_3_6/api.rs create mode 100644 src/dbus_api/api/manager_3_6/methods.rs create mode 100644 src/dbus_api/api/manager_3_6/mod.rs diff --git a/src/dbus_api/api/manager_3_2/methods.rs b/src/dbus_api/api/manager_3_2/methods.rs index a341e48c13..cdee879107 100644 --- a/src/dbus_api/api/manager_3_2/methods.rs +++ b/src/dbus_api/api/manager_3_2/methods.rs @@ -45,6 +45,7 @@ where return Ok(vec![return_message.append3(default_return, rc, rs)]); } }; + let unlock_method = { let unlock_method_tup: (bool, &str) = get_next_arg(&mut iter, 1)?; match tuple_to_option(unlock_method_tup) { @@ -79,7 +80,7 @@ where } }; - let (pool_name, _, pool) = guard.as_tuple(); + let (pool_name, pool_uuid, pool) = guard.as_tuple(); let pool_path = create_dbus_pool(dbus_context, base_path.clone(), &pool_name, pool_uuid, pool); let mut bd_paths = Vec::new(); @@ -169,7 +170,11 @@ where }) .unwrap_or(false); - let msg = match handle_action!(block_on(dbus_context.engine.stop_pool(pool_uuid))) { + let msg = match handle_action!(block_on( + dbus_context + .engine + .stop_pool(PoolIdentifier::Uuid(pool_uuid), false) + )) { Ok(StopAction::Stopped(_)) => { dbus_context.push_remove(&pool_path, consts::pool_interface_list()); if send_locked_signal { @@ -182,6 +187,7 @@ where OK_STRING.to_string(), ) } + Ok(StopAction::CleanedUp(_)) => unreachable!("!has_partially_constructed above"), Ok(StopAction::Identity) => return_message.append3( default_return, DbusErrorEnum::OK as u16, diff --git a/src/dbus_api/api/manager_3_6/api.rs b/src/dbus_api/api/manager_3_6/api.rs new file mode 100644 index 0000000000..6ad03cf865 --- /dev/null +++ b/src/dbus_api/api/manager_3_6/api.rs @@ -0,0 +1,29 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +use dbus_tree::{Factory, MTSync, Method}; + +use crate::{ + dbus_api::{api::manager_3_6::methods::stop_pool, types::TData}, + engine::Engine, +}; + +pub fn stop_pool_method( + f: &Factory>, TData>, +) -> Method>, TData> +where + E: 'static + Engine, +{ + f.method("StopPool", (), stop_pool) + .in_arg(("id", "s")) + .in_arg(("id_type", "s")) + // In order from left to right: + // b: true if the pool was newly stopped + // s: string representation of UUID of stopped pool + // + // Rust representation: (bool, String) + .out_arg(("result", "(bs)")) + .out_arg(("return_code", "q")) + .out_arg(("return_string", "s")) +} diff --git a/src/dbus_api/api/manager_3_6/methods.rs b/src/dbus_api/api/manager_3_6/methods.rs new file mode 100644 index 0000000000..980efc3950 --- /dev/null +++ b/src/dbus_api/api/manager_3_6/methods.rs @@ -0,0 +1,116 @@ +// This Source Code Form is subject to the terms of the Mozilla Public +// License, v. 2.0. If a copy of the MPL was not distributed with this +// file, You can obtain one at http://mozilla.org/MPL/2.0/. + +use dbus::Message; +use dbus_tree::{MTSync, MethodInfo, MethodResult}; +use futures::executor::block_on; + +use crate::{ + dbus_api::{ + consts, + types::{DbusErrorEnum, TData, OK_STRING}, + util::{engine_to_dbus_err_tuple, get_next_arg}, + }, + engine::{Engine, Name, Pool, PoolIdentifier, PoolUuid, StopAction, StratisUuid}, + stratis::StratisError, +}; + +pub fn stop_pool(m: &MethodInfo<'_, MTSync>, TData>) -> MethodResult +where + E: 'static + Engine, +{ + let message: &Message = m.msg; + let mut iter = message.iter_init(); + let dbus_context = m.tree.get_data(); + let default_return = (false, String::new()); + let return_message = message.method_return(); + + let id_str: &str = get_next_arg(&mut iter, 0)?; + let pool_id = { + let id_type_str: &str = get_next_arg(&mut iter, 1)?; + match id_type_str { + "uuid" => match PoolUuid::parse_str(id_str) { + Ok(u) => PoolIdentifier::Uuid(u), + Err(e) => { + let (rc, rs) = engine_to_dbus_err_tuple(&e); + return Ok(vec![return_message.append3(default_return, rc, rs)]); + } + }, + "name" => PoolIdentifier::Name(Name::new(id_str.to_string())), + _ => { + let (rc, rs) = engine_to_dbus_err_tuple(&StratisError::Msg(format!( + "ID type {id_type_str} not recognized" + ))); + return Ok(vec![return_message.append3(default_return, rc, rs)]); + } + } + }; + + // If Some(_), send a locked pool property change signal only if the pool is + // encrypted. If None, the pool may already be stopped or not exist at all. + // Both of these cases are handled by stop_pool and the value we provide + // for send_locked_signal does not matter as send_locked_signal is only + // used when a pool is newly stopped which can only occur if the pool is found + // here. + let send_locked_signal = block_on(dbus_context.engine.get_pool(pool_id.clone())) + .map(|g| { + let (_, _, p) = g.as_tuple(); + p.is_encrypted() + }) + .unwrap_or(false); + + let msg = match handle_action!(block_on(dbus_context.engine.stop_pool(pool_id, true))) { + Ok(StopAction::Stopped(pool_uuid)) => { + match m.tree.iter().find_map(|opath| { + opath + .get_data() + .as_ref() + .and_then(|op_cxt| match op_cxt.uuid { + StratisUuid::Pool(u) => { + if u == pool_uuid { + Some(opath.get_name()) + } else { + None + } + } + StratisUuid::Fs(_) => None, + StratisUuid::Dev(_) => None, + }) + }) { + Some(pool_path) => { + dbus_context.push_remove(pool_path, consts::pool_interface_list()); + if send_locked_signal { + dbus_context + .push_locked_pools(block_on(dbus_context.engine.locked_pools())); + } + dbus_context.push_stopped_pools(block_on(dbus_context.engine.stopped_pools())); + } + None => { + warn!("Could not find pool D-Bus path for the pool that was just stopped"); + } + } + return_message.append3( + (true, uuid_to_string!(pool_uuid)), + DbusErrorEnum::OK as u16, + OK_STRING.to_string(), + ) + } + Ok(StopAction::CleanedUp(pool_uuid)) => return_message.append3( + (true, uuid_to_string!(pool_uuid)), + DbusErrorEnum::OK as u16, + OK_STRING.to_string(), + ), + Ok(StopAction::Identity) => return_message.append3( + default_return, + DbusErrorEnum::OK as u16, + OK_STRING.to_string(), + ), + Err(e) => { + let (rc, rs) = engine_to_dbus_err_tuple(&e); + return Ok(vec![return_message.append3(default_return, rc, rs)]); + } + }; + + Ok(vec![msg]) +} diff --git a/src/dbus_api/api/manager_3_6/mod.rs b/src/dbus_api/api/manager_3_6/mod.rs new file mode 100644 index 0000000000..b50d39ede6 --- /dev/null +++ b/src/dbus_api/api/manager_3_6/mod.rs @@ -0,0 +1,4 @@ +mod api; +mod methods; + +pub use api::stop_pool_method; diff --git a/src/dbus_api/api/mod.rs b/src/dbus_api/api/mod.rs index 6d057aff17..89a71efc1a 100644 --- a/src/dbus_api/api/mod.rs +++ b/src/dbus_api/api/mod.rs @@ -16,6 +16,7 @@ mod manager_3_0; mod manager_3_2; mod manager_3_4; mod manager_3_5; +mod manager_3_6; pub mod prop_conv; mod report_3_0; mod shared; @@ -135,7 +136,7 @@ where .add_m(manager_3_0::destroy_pool_method(&f)) .add_m(manager_3_0::engine_state_report_method(&f)) .add_m(manager_3_4::start_pool_method(&f)) - .add_m(manager_3_2::stop_pool_method(&f)) + .add_m(manager_3_6::stop_pool_method(&f)) .add_m(manager_3_2::refresh_state_method(&f)) .add_p(manager_3_0::version_property(&f)) .add_p(manager_3_2::stopped_pools_property(&f)), diff --git a/src/engine/engine.rs b/src/engine/engine.rs index e5db6b7a23..3f56810249 100644 --- a/src/engine/engine.rs +++ b/src/engine/engine.rs @@ -448,7 +448,11 @@ pub trait Engine: Debug + Report + Send + Sync { /// Stop and tear down a pool, storing the information for it to be started /// again later. - async fn stop_pool(&self, pool_uuid: PoolUuid) -> StratisResult>; + async fn stop_pool( + &self, + pool_id: PoolIdentifier, + has_partially_constructed: bool, + ) -> StratisResult>; /// Refresh the state of all pools and liminal devices. async fn refresh_state(&self) -> StratisResult<()>; diff --git a/src/engine/sim_engine/engine.rs b/src/engine/sim_engine/engine.rs index 4426d2c201..9778c8c66c 100644 --- a/src/engine/sim_engine/engine.rs +++ b/src/engine/sim_engine/engine.rs @@ -342,16 +342,34 @@ impl Engine for SimEngine { } } - async fn stop_pool(&self, pool_uuid: PoolUuid) -> StratisResult> { - if self - .stopped_pools - .read() - .await - .get_by_uuid(pool_uuid) - .is_some() - { - Ok(StopAction::Identity) - } else if let Some((name, pool)) = self.pools.write_all().await.remove_by_uuid(pool_uuid) { + async fn stop_pool( + &self, + pool_id: PoolIdentifier, + _: bool, + ) -> StratisResult> { + let is_stopped = match pool_id { + PoolIdentifier::Name(ref n) => self.stopped_pools.read().await.get_by_name(n).is_some(), + PoolIdentifier::Uuid(u) => self.stopped_pools.read().await.get_by_uuid(u).is_some(), + }; + if is_stopped { + return Ok(StopAction::Identity); + } + + let pool_entry = match pool_id { + PoolIdentifier::Name(ref n) => self + .pools + .write_all() + .await + .remove_by_name(n) + .map(|(u, p)| (n.clone(), u, p)), + PoolIdentifier::Uuid(u) => self + .pools + .write_all() + .await + .remove_by_uuid(u) + .map(|(n, p)| (n, u, p)), + }; + if let Some((name, pool_uuid, pool)) = pool_entry { self.stopped_pools .write() .await @@ -359,7 +377,7 @@ impl Engine for SimEngine { Ok(StopAction::Stopped(pool_uuid)) } else { Err(StratisError::Msg(format!( - "Pool with UUID {pool_uuid} was not found and cannot be stopped" + "Pool with ID {pool_id} was not found and cannot be stopped" ))) } } diff --git a/src/engine/strat_engine/engine.rs b/src/engine/strat_engine/engine.rs index 215786230a..4a1645dd3f 100644 --- a/src/engine/strat_engine/engine.rs +++ b/src/engine/strat_engine/engine.rs @@ -657,29 +657,49 @@ impl Engine for StratEngine { } } - async fn stop_pool(&self, pool_uuid: PoolUuid) -> StratisResult> { + async fn stop_pool( + &self, + pool_id: PoolIdentifier, + has_partially_constructed: bool, + ) -> StratisResult> { + let id_str = pool_id.to_string(); + + let stopped_pools = self.liminal_devices.read().await.stopped_pools(); + let pool_uuid = match pool_id { + PoolIdentifier::Name(ref n) => stopped_pools.name_to_uuid.get(n), + PoolIdentifier::Uuid(ref u) => Some(u), + }; + if let Some(pool_uuid) = pool_uuid { + if has_partially_constructed { + if stopped_pools.stopped.get(pool_uuid).is_some() { + return Ok(StopAction::Identity); + } else if stopped_pools.partially_constructed.get(pool_uuid).is_some() { + self.liminal_devices + .write() + .await + .stop_partially_constructed_pool(*pool_uuid)?; + return Ok(StopAction::CleanedUp(*pool_uuid)); + } + } else if stopped_pools.stopped.get(pool_uuid).is_some() + || stopped_pools.partially_constructed.get(pool_uuid).is_some() + { + return Ok(StopAction::Identity); + } + } + let mut pools = self.pools.write_all().await; - if let Some((name, pool)) = pools.remove_by_uuid(pool_uuid) { + if let Some((name, pool_uuid, pool)) = match pool_id { + PoolIdentifier::Name(n) => pools.remove_by_name(&n).map(|(u, p)| (n, u, p)), + PoolIdentifier::Uuid(u) => pools.remove_by_uuid(u).map(|(n, p)| (n, u, p)), + } { self.liminal_devices .write() .await .stop_pool(&mut pools, name, pool_uuid, pool)?; - return Ok(StopAction::Stopped(pool_uuid)); - } - - drop(pools); - - let stopped_pools = self.liminal_devices.read().await.stopped_pools(); - if stopped_pools.stopped.get(&pool_uuid).is_some() - || stopped_pools - .partially_constructed - .get(&pool_uuid) - .is_some() - { - Ok(StopAction::Identity) + Ok(StopAction::Stopped(pool_uuid)) } else { Err(StratisError::Msg(format!( - "Pool with UUID {pool_uuid} could not be found and cannot be stopped" + "Pool with UUID {id_str} could not be found and cannot be stopped" ))) } } @@ -938,7 +958,7 @@ mod test { let res = init_res.and_then(|_| needs_clean_up(&mut pool, fail_device, operation)); drop(pool); - test_async!(engine.stop_pool(uuid))?; + test_async!(engine.stop_pool(PoolIdentifier::Uuid(uuid), true))?; res?; test_async!(engine.start_pool(PoolIdentifier::Uuid(uuid), Some(unlock_method)))?; @@ -1360,7 +1380,11 @@ mod test { .unwrap() .changed() .unwrap(); - assert!(test_async!(engine.stop_pool(uuid)).unwrap().is_changed()); + assert!( + test_async!(engine.stop_pool(PoolIdentifier::Uuid(uuid), true)) + .unwrap() + .is_changed() + ); assert_eq!(test_async!(engine.stopped_pools()).stopped.len(), 1); assert_eq!(test_async!(engine.pools()).len(), 0); diff --git a/src/engine/strat_engine/liminal/liminal.rs b/src/engine/strat_engine/liminal/liminal.rs index a6d75c6b8d..f93a6bdae1 100644 --- a/src/engine/strat_engine/liminal/liminal.rs +++ b/src/engine/strat_engine/liminal/liminal.rs @@ -327,6 +327,20 @@ impl LiminalDevices { } } + /// Tear down a partially constructed pool. + pub fn stop_partially_constructed_pool(&mut self, pool_uuid: PoolUuid) -> StratisResult<()> { + if let Some(device_set) = self.partially_constructed_pools.get(&pool_uuid) { + stop_partially_constructed_pool( + pool_uuid, + &device_set + .iter() + .map(|(dev_uuid, _)| *dev_uuid) + .collect::>(), + )?; + } + Ok(()) + } + /// Get a mapping of pool UUIDs from all of the LUKS2 devices that are currently /// locked to their encryption info in the set of pools that are not yet set up. pub fn locked_pools(&self) -> LockedPoolsInfo { diff --git a/src/engine/types/actions.rs b/src/engine/types/actions.rs index 42dbfe00ee..eef38c5c9a 100644 --- a/src/engine/types/actions.rs +++ b/src/engine/types/actions.rs @@ -678,6 +678,7 @@ impl EngineAction for StartAction { pub enum StopAction { Identity, Stopped(T), + CleanedUp(T), } impl Display for StopAction { @@ -687,6 +688,10 @@ impl Display for StopAction { f, "The requested pool is already stopped; no action was taken" ), + StopAction::CleanedUp(uuid) => write!( + f, + "The pool with UUID {uuid} was partially constructed and cleaned up successfully", + ), StopAction::Stopped(uuid) => { write!(f, "The pool with UUID {uuid} was successfully stopped") } diff --git a/src/jsonrpc/server/pool.rs b/src/jsonrpc/server/pool.rs index c697fe1a39..5273d53881 100644 --- a/src/jsonrpc/server/pool.rs +++ b/src/jsonrpc/server/pool.rs @@ -40,18 +40,7 @@ pub async fn pool_stop(engine: Arc, id: PoolIdentifier) -> Strat where E: Engine, { - let pool_uuid = match id { - PoolIdentifier::Uuid(u) => u, - PoolIdentifier::Name(n) => { - engine - .pools() - .await - .get_by_name(&n) - .ok_or_else(|| StratisError::Msg(format!("Pool with name {n} not found")))? - .0 - } - }; - Ok(engine.stop_pool(pool_uuid).await?.is_changed()) + Ok(engine.stop_pool(id, true).await?.is_changed()) } // stratis-min pool create diff --git a/tests/client-dbus/src/stratisd_client_dbus/_introspect.py b/tests/client-dbus/src/stratisd_client_dbus/_introspect.py index f3681959ca..841ad7d562 100644 --- a/tests/client-dbus/src/stratisd_client_dbus/_introspect.py +++ b/tests/client-dbus/src/stratisd_client_dbus/_introspect.py @@ -53,7 +53,8 @@ - + + diff --git a/tests/client-dbus/tests/udev/test_udev.py b/tests/client-dbus/tests/udev/test_udev.py index 4a7e6ec3fc..a013c93465 100644 --- a/tests/client-dbus/tests/udev/test_udev.py +++ b/tests/client-dbus/tests/udev/test_udev.py @@ -601,12 +601,14 @@ def _simple_stop_test(self): num_devices = 2 device_tokens = self._lb_mgr.create_devices(num_devices) devnodes = self._lb_mgr.device_files(device_tokens) + pool_name1 = random_string(5) + pool_name2 = random_string(5) with OptionalKeyServiceContextManager(): self.wait_for_pools(0) - (_, (pool_object_path, _)) = create_pool(random_string(5), devnodes[:1]) - create_pool(random_string(5), devnodes[1:]) + create_pool(pool_name1, devnodes[:1]) + create_pool(pool_name2, devnodes[1:]) self.wait_for_pools(2) @@ -621,7 +623,10 @@ def _simple_stop_test(self): ((changed, _), exit_code, _) = Manager.Methods.StopPool( get_object(TOP_OBJECT), - {"pool": pool_object_path}, + { + "id": pool_name1, + "id_type": "name", + }, ) self.assertEqual(exit_code, 0) self.assertEqual(changed, True) @@ -684,22 +689,20 @@ def _simple_start_by_name_test(self): ) as key_desc: self.wait_for_pools(0) - (_, (pool_object_path_enc, _)) = create_pool( - "encrypted", devnodes[:2], key_description=key_desc[0] - ) - (_, (pool_object_path, _)) = create_pool("unencrypted", devnodes[2:]) + create_pool("encrypted", devnodes[:2], key_description=key_desc[0]) + create_pool("unencrypted", devnodes[2:]) self.wait_for_pools(2) ((changed, _), exit_code, _) = Manager.Methods.StopPool( get_object(TOP_OBJECT), - {"pool": pool_object_path_enc}, + {"id": "encrypted", "id_type": "name"}, ) self.assertEqual(exit_code, 0) self.assertEqual(changed, True) ((changed, _), exit_code, _) = Manager.Methods.StopPool( get_object(TOP_OBJECT), - {"pool": pool_object_path}, + {"id": "unencrypted", "id_type": "name"}, ) self.assertEqual(exit_code, 0) self.assertEqual(changed, True)