add error case for layout not ready, and fail earlier in many places

src/rpc/layout/helper.rs

@@ -4,6 +4,7 @@ use std::sync::atomic::{AtomicUsize, Ordering};
 use serde::{Deserialize, Serialize};
 
 use garage_util::data::*;
+use garage_util::error::Error;
 
 use super::*;
 use crate::replication_mode::*;
@@ -145,24 +146,31 @@ impl LayoutHelper {
 	}
 
 	/// Returns the current layout version
-	pub fn current(&self) -> &LayoutVersion {
-		self.inner().current()
+	pub fn current(&self) -> Result<&LayoutVersion, Error> {
+		if !self.is_check_ok {
+			return Err(Error::LayoutNotReady);
+		}
+		Ok(self.inner().current())
 	}
 
 	/// Returns all layout versions currently active in the cluster
-	pub fn versions(&self) -> &[LayoutVersion] {
-		&self.inner().versions
+	pub fn versions(&self) -> Result<&[LayoutVersion], Error> {
+		if !self.is_check_ok {
+			return Err(Error::LayoutNotReady);
+		}
+		Ok(&self.inner().versions)
 	}
 
 	/// Returns the latest layout version for which it is safe to read data from,
 	/// i.e. the version whose version number is sync_map_min
-	pub fn read_version(&self) -> &LayoutVersion {
+	pub fn read_version(&self) -> Result<&LayoutVersion, Error> {
 		let sync_min = self.sync_map_min;
-		self.versions()
+		let versions = self.versions()?;
+		Ok(versions
 			.iter()
 			.find(|x| x.version == sync_min)
-			.or(self.versions().last())
-			.unwrap()
+			.or(versions.last())
+			.unwrap())
 	}
 
 	pub fn is_check_ok(&self) -> bool {
@@ -171,14 +179,20 @@ impl LayoutHelper {
 
 	/// Return all nodes that have a role (gateway or storage)
 	/// in one of the currently active layout versions
-	pub fn all_nodes(&self) -> &[Uuid] {
-		&self.all_nodes
+	pub fn all_nodes(&self) -> Result<&[Uuid], Error> {
+		if !self.is_check_ok {
+			return Err(Error::LayoutNotReady);
+		}
+		Ok(&self.all_nodes)
 	}
 
 	/// Return all nodes that are configured to store data
 	/// in one of the currently active layout versions
-	pub fn all_nongateway_nodes(&self) -> &[Uuid] {
-		&self.all_nongateway_nodes
+	pub fn all_nongateway_nodes(&self) -> Result<&[Uuid], Error> {
+		if !self.is_check_ok {
+			return Err(Error::LayoutNotReady);
+		}
+		Ok(&self.all_nongateway_nodes)
 	}
 
 	pub fn ack_map_min(&self) -> u64 {
@@ -193,7 +207,7 @@ impl LayoutHelper {
 
 	pub fn sync_digest(&self) -> SyncLayoutDigest {
 		SyncLayoutDigest {
-			current: self.current().version,
+			current: self.inner().current().version,
 			ack_map_min: self.ack_map_min(),
 			min_stored: self.inner().min_stored(),
 		}
@@ -201,8 +215,8 @@ impl LayoutHelper {
 
 	pub(crate) fn digest(&self) -> RpcLayoutDigest {
 		RpcLayoutDigest {
-			current_version: self.current().version,
-			active_versions: self.versions().len(),
+			current_version: self.inner().current().version,
+			active_versions: self.inner().versions.len(),
 			trackers_hash: self.trackers_hash,
 			staging_hash: self.staging_hash,
 		}
@@ -246,7 +260,8 @@ impl LayoutHelper {
 
 	pub(crate) fn update_ack_to_max_free(&mut self, local_node_id: Uuid) -> bool {
 		let max_free = self
-			.versions()
+			.inner()
+			.versions
 			.iter()
 			.map(|x| x.version)
 			.skip_while(|v| {
@@ -256,7 +271,7 @@ impl LayoutHelper {
 					.unwrap_or(true)
 			})
 			.next()
-			.unwrap_or(self.current().version);
+			.unwrap_or(self.inner().current().version);
 		let changed = self.update(|layout| {
 			layout
 				.update_trackers

src/rpc/layout/manager.rs

@@ -105,7 +105,7 @@ impl LayoutManager {
 	}
 
 	pub fn add_table(&self, table_name: &'static str) {
-		let first_version = self.layout().versions().first().unwrap().version;
+		let first_version = self.layout().inner().versions.first().unwrap().version;
 
 		self.table_sync_version
 			.lock()
@@ -139,19 +139,20 @@ impl LayoutManager {
 
 	// ---- ACK LOCKING ----
 
-	pub fn write_lock_with<T, F>(self: &Arc<Self>, f: F) -> WriteLock<T>
+	pub fn write_lock_with<T, F>(self: &Arc<Self>, f: F) -> Result<WriteLock<T>, Error>
 	where
-		F: FnOnce(&LayoutHelper) -> T,
+		F: FnOnce(&[LayoutVersion]) -> T,
 	{
 		let layout = self.layout();
-		let version = layout.current().version;
-		let value = f(&layout);
+		let current_version = layout.current()?.version;
+		let versions = layout.versions()?;
+		let value = f(versions);
 		layout
 			.ack_lock
-			.get(&version)
+			.get(&current_version)
 			.unwrap()
 			.fetch_add(1, Ordering::Relaxed);
-		WriteLock::new(version, self, value)
+		Ok(WriteLock::new(current_version, self, value))
 	}
 
 	// ---- INTERNALS ---
@@ -369,7 +370,7 @@ impl<T> Drop for WriteLock<T> {
 		let layout = self.layout_manager.layout(); // acquire read lock
 		if let Some(counter) = layout.ack_lock.get(&self.layout_version) {
 			let prev_lock = counter.fetch_sub(1, Ordering::Relaxed);
-			if prev_lock == 1 && layout.current().version > self.layout_version {
+			if prev_lock == 1 && layout.current().unwrap().version > self.layout_version {
 				drop(layout); // release read lock, write lock will be acquired
 				self.layout_manager.ack_new_version();
 			}

src/rpc/layout/version.rs

@@ -118,15 +118,14 @@ impl LayoutVersion {
 	pub fn nodes_of(&self, position: &Hash) -> impl Iterator<Item = Uuid> + '_ {
 		let data = &self.ring_assignment_data;
 
-		let partition_nodes = if data.len() == self.replication_factor * (1 << PARTITION_BITS) {
-			let partition_idx = self.partition_of(position) as usize;
-			let partition_start = partition_idx * self.replication_factor;
-			let partition_end = (partition_idx + 1) * self.replication_factor;
-			&data[partition_start..partition_end]
-		} else {
-			warn!("Ring not yet ready, read/writes will be lost!");
-			&[]
-		};
+		if data.len() != self.replication_factor * (1 << PARTITION_BITS) {
+			panic!(".nodes_of() called on invalid LayoutVersion (this is a bug)");
+		}
+
+		let partition_idx = self.partition_of(position) as usize;
+		let partition_start = partition_idx * self.replication_factor;
+		let partition_end = (partition_idx + 1) * self.replication_factor;
+		let partition_nodes = &data[partition_start..partition_end];
 
 		partition_nodes
 			.iter()

src/rpc/rpc_helper.rs

@@ -345,7 +345,7 @@ impl RpcHelper {
 
 		// Reorder requests to priorize closeness / low latency
 		let request_order =
-			self.request_order(&self.0.layout.read().unwrap().current(), to.iter().copied());
+			self.request_order(self.0.layout.read().unwrap().current()?, to.iter().copied());
 		let send_all_at_once = strategy.rs_send_all_at_once.unwrap_or(false);
 
 		// Build future for each request
@@ -567,25 +567,29 @@ impl RpcHelper {
 	/// The preference order, for each layout version, is given by `request_order`,
 	/// based on factors such as nodes being in the same datacenter,
 	/// having low ping, etc.
-	pub fn block_read_nodes_of(&self, position: &Hash, rpc_helper: &RpcHelper) -> Vec<Uuid> {
+	pub fn block_read_nodes_of(
+		&self,
+		position: &Hash,
+		rpc_helper: &RpcHelper,
+	) -> Result<Vec<Uuid>, Error> {
 		let layout = self.0.layout.read().unwrap();
+		let current_layout = layout.current()?;
 
 		// Compute, for each layout version, the set of nodes that might store
 		// the block, and put them in their preferred order as of `request_order`.
-		let mut vernodes = layout.versions().iter().map(|ver| {
+		let mut vernodes = vec![];
+		for ver in layout.versions()?.iter() {
 			let nodes = ver.nodes_of(position);
-			rpc_helper.request_order(layout.current(), nodes)
-		});
+			vernodes.push(rpc_helper.request_order(current_layout, nodes))
+		}
 
-		let mut ret = if layout.versions().len() == 1 {
+		let mut ret = if vernodes.len() == 1 {
 			// If we have only one active layout version, then these are the
 			// only nodes we ask in step 1
-			vernodes.next().unwrap()
+			vernodes.into_iter().next().unwrap()
 		} else {
-			let vernodes = vernodes.collect::<Vec<_>>();
-
 			let mut nodes = Vec::<Uuid>::with_capacity(12);
-			for i in 0..layout.current().replication_factor {
+			for i in 0..current_layout.replication_factor {
 				for vn in vernodes.iter() {
 					if let Some(n) = vn.get(i) {
 						if !nodes.contains(&n) {
@@ -608,14 +612,14 @@ impl RpcHelper {
 		let old_ver_iter = layout.inner().old_versions.iter().rev();
 		for ver in old_ver_iter {
 			let nodes = ver.nodes_of(position);
-			for node in rpc_helper.request_order(layout.current(), nodes) {
+			for node in rpc_helper.request_order(current_layout, nodes) {
 				if !ret.contains(&node) {
 					ret.push(node);
 				}
 			}
 		}
 
-		ret
+		Ok(ret)
 	}
 
 	fn request_order(

src/rpc/system.rs

@@ -453,11 +453,28 @@ impl System {
 
 		// Acquire a rwlock read-lock to the current cluster layout
 		let layout = self.cluster_layout();
+		let layout_versions = match layout.versions() {
+			Ok(v) => v,
+			Err(_) => {
+				// Layout not yet configured, special case
+				return ClusterHealth {
+					status: ClusterHealthStatus::Unavailable,
+					known_nodes: nodes.len(),
+					connected_nodes,
+					storage_nodes: 0,
+					storage_nodes_ok: 0,
+					partitions: 0,
+					partitions_quorum: 0,
+					partitions_all_ok: 0,
+				};
+			}
+		};
+		let current_layout = layout_versions.last().unwrap();
 
 		// Obtain information about nodes that have a role as storage nodes
 		// in one of the active layout versions
 		let mut storage_nodes = HashSet::<Uuid>::with_capacity(16);
-		for ver in layout.versions().iter() {
+		for ver in layout_versions.iter() {
 			storage_nodes.extend(
 				ver.roles
 					.items()
@@ -471,11 +488,11 @@ impl System {
 		// Determine the number of partitions that have:
 		// - a quorum of up nodes for all write sets (i.e. are available)
 		// - for which all nodes in all write sets are up (i.e. are fully healthy)
-		let partitions = layout.current().partitions().collect::<Vec<_>>();
+		let partitions = current_layout.partitions().collect::<Vec<_>>();
 		let mut partitions_quorum = 0;
 		let mut partitions_all_ok = 0;
 		for (_, hash) in partitions.iter() {
-			let mut write_sets = layout.versions().iter().map(|x| x.nodes_of(hash));
+			let mut write_sets = layout_versions.iter().map(|x| x.nodes_of(hash));
 			let has_quorum = write_sets
 				.clone()
 				.all(|set| set.filter(|x| node_up(x)).count() >= quorum);
@@ -630,21 +647,37 @@ impl System {
 
 	async fn discovery_loop(self: &Arc<Self>, mut stop_signal: watch::Receiver<bool>) {
 		while !*stop_signal.borrow() {
-			let n_connected = self
+			let peers_up = self
 				.peering
 				.get_peer_list()
 				.iter()
 				.filter(|p| p.is_up())
-				.count();
+				.map(|p| Uuid::from(p.id))
+				.collect::<Vec<_>>();
 
-			let not_configured = !self.cluster_layout().is_check_ok();
-			let no_peers = n_connected < self.replication_factor.into();
-			let expected_n_nodes = self.cluster_layout().all_nodes().len();
-			let bad_peers = n_connected != expected_n_nodes;
-
-			if not_configured || no_peers || bad_peers {
-				info!("Doing a bootstrap/discovery step (not_configured: {}, no_peers: {}, bad_peers: {})", not_configured, no_peers, bad_peers);
+			let do_bootstrap = match self.cluster_layout().all_nodes() {
+				Err(_) => {
+					debug!("doing bootstrap/discovery step (layout not configured)");
+					true
+				}
+				Ok(all_nodes) => {
+					// Do bootstrap if we have fewer peers than the replication
+					// factor,
+					// or if some peers in the layout are not connected
+					let do_bootstrap = peers_up.len() < self.replication_factor.into()
+						|| all_nodes.iter().any(|x| !peers_up.contains(x));
+					if do_bootstrap {
+						debug!(
+							"doing bootstrap/discovery step (peers_up: {}, all_nodes: {})",
+							peers_up.len(),
+							all_nodes.len()
+						);
+					}
+					do_bootstrap
+				}
+			};
 
+			if do_bootstrap {
 				let mut ping_list = resolve_peers(&self.bootstrap_peers).await;
 
 				// Add peer list from list stored on disk
@@ -687,12 +720,13 @@ impl System {
 					}
 				}
 
-				if !not_configured && !no_peers {
-					// If the layout is configured, and we already have some connections
-					// to other nodes in the cluster, we can skip trying to connect to
-					// nodes that are not in the cluster layout.
-					let layout = self.cluster_layout();
-					ping_list.retain(|(id, _)| layout.all_nodes().contains(&(*id).into()));
+				if let Ok(all_nodes) = self.cluster_layout().all_nodes() {
+					if peers_up.len() >= self.replication_factor.into() {
+						// If the layout is configured, and we already have some connections
+						// to other nodes in the cluster, we can skip trying to connect to
+						// nodes that are not in the cluster layout.
+						ping_list.retain(|(id, _)| all_nodes.contains(&(*id).into()));
+					}
 				}
 
 				for (node_id, node_addr) in ping_list {

src/rpc/system_metrics.rs

@@ -216,10 +216,13 @@ impl SystemMetrics {
 					.u64_value_observer("cluster_layout_node_connected", move |observer| {
 						let layout = system.cluster_layout();
 						let nodes = system.get_known_nodes();
-						for id in layout.all_nodes().iter() {
+						for id in layout.all_nodes().unwrap_or_default().iter() {
 							let mut kv = vec![KeyValue::new("id", format!("{:?}", id))];
-							if let Some(role) =
-								layout.current().roles.get(id).and_then(|r| r.0.as_ref())
+							if let Some(role) = layout
+								.current()
+								.ok()
+								.and_then(|l| l.roles.get(id))
+								.and_then(|r| r.0.as_ref())
 							{
 								kv.push(KeyValue::new("role_zone", role.zone.clone()));
 								match role.capacity {
@@ -260,10 +263,13 @@ impl SystemMetrics {
 					.u64_value_observer("cluster_layout_node_disconnected_time", move |observer| {
 						let layout = system.cluster_layout();
 						let nodes = system.get_known_nodes();
-						for id in layout.all_nodes().iter() {
+						for id in layout.all_nodes().unwrap_or_default().iter() {
 							let mut kv = vec![KeyValue::new("id", format!("{:?}", id))];
-							if let Some(role) =
-								layout.current().roles.get(id).and_then(|r| r.0.as_ref())
+							if let Some(role) = layout
+								.current()
+								.ok()
+								.and_then(|l| l.roles.get(id))
+								.and_then(|r| r.0.as_ref())
 							{
 								kv.push(KeyValue::new("role_zone", role.zone.clone()));
 								match role.capacity {