From 6291acd77ff637a9161df4f90edc7782111b9081 Mon Sep 17 00:00:00 2001 From: Tiago Castro Date: Thu, 14 Mar 2024 15:48:47 +0000 Subject: [PATCH 1/2] fix(agents/core): use tracing api Incorrect log api was used for error logging. Also include error source in the service error. Signed-off-by: Tiago Castro --- .../agents/src/bin/core/nexus/operations_helper.rs | 9 ++++----- control-plane/agents/src/bin/core/node/wrapper.rs | 14 ++++---------- control-plane/agents/src/common/errors.rs | 8 ++++++-- 3 files changed, 14 insertions(+), 17 deletions(-) diff --git a/control-plane/agents/src/bin/core/nexus/operations_helper.rs b/control-plane/agents/src/bin/core/nexus/operations_helper.rs index 6bc5e5964..57ffe7b7d 100644 --- a/control-plane/agents/src/bin/core/nexus/operations_helper.rs +++ b/control-plane/agents/src/bin/core/nexus/operations_helper.rs @@ -20,8 +20,6 @@ use stor_port::types::v0::{ }, }; -use tracing::log::error; - impl OperationGuardArc { /// Attach the specified replica to the volume nexus /// The replica might need to be shared/unshared so it can be opened by the nexus @@ -143,9 +141,10 @@ impl OperationGuardArc { replica_state.node.clone(), ); if let Err(error) = node.set_replica_entity_id(&set_entity_id).await { - error!( - "Failed to set entity_id for the replica {}", - error.to_string() + tracing::error!( + replica.uuid = %replica_state.uuid, + %error, + "Failed to set entity_id", ); } } diff --git a/control-plane/agents/src/bin/core/node/wrapper.rs b/control-plane/agents/src/bin/core/node/wrapper.rs index 8d3c02ddb..d033887ef 100644 --- a/control-plane/agents/src/bin/core/node/wrapper.rs +++ b/control-plane/agents/src/bin/core/node/wrapper.rs @@ -1283,22 +1283,15 @@ impl ReplicaApi for Arc> { Ok(replica) } - Err(SvcError::GrpcRequestError { source, .. }) - if source.code() == tonic::Code::DataLoss => - { - Err(SvcError::ReplicaSetPropertyFailed { - attribute: "entity_id".to_string(), - replica: request.uuid.to_string(), - }) - } Err(error) => { let mut ctx = dataplane.reconnect(GETS_TIMEOUT).await?; self.update_pool_replica_states(ctx.deref_mut()).await?; match self.read().await.replicas().into_iter().find(|replica| { - replica.uuid == request.uuid + (replica.uuid == request.uuid || replica.name - == ReplicaName::from_opt_uuid(request.name.as_ref(), &request.uuid) + == ReplicaName::from_opt_uuid(request.name.as_ref(), &request.uuid)) + && replica.entity_id == request.entity_id }) { Some(replica) => Ok(replica), None => Err(error), @@ -1420,6 +1413,7 @@ impl ReplicaApi for Arc> { Err(SvcError::ReplicaSetPropertyFailed { attribute: "entity_id".to_string(), replica: request.uuid().to_string(), + source, }) } Err(error) => Err(error), diff --git a/control-plane/agents/src/common/errors.rs b/control-plane/agents/src/common/errors.rs index 41cf63e56..e0a7557e7 100644 --- a/control-plane/agents/src/common/errors.rs +++ b/control-plane/agents/src/common/errors.rs @@ -402,8 +402,12 @@ pub enum SvcError { replicas: u8, id: String, }, - #[snafu(display("Cannot set '{attribute}' on Replica '{replica}'"))] - ReplicaSetPropertyFailed { attribute: String, replica: String }, + #[snafu(display("Cannot set '{attribute}' on Replica '{replica}: {source}'"))] + ReplicaSetPropertyFailed { + attribute: String, + replica: String, + source: tonic::Status, + }, } impl SvcError { From 251f682c22500cfca0d132f4e98cde0fa0f624b8 Mon Sep 17 00:00:00 2001 From: Tiago Castro Date: Thu, 14 Mar 2024 17:55:13 +0000 Subject: [PATCH 2/2] feat(platform): support custom namespace When we run outside of the cluster, we can't just use the env to determine the correct namespace.. For now we expose an additional method which allows overriding it. A better solution is perhaps to simply ensure we create clients with the correct namespace as default? Signed-off-by: Tiago Castro --- utils/platform/src/k8s/mod.rs | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/utils/platform/src/k8s/mod.rs b/utils/platform/src/k8s/mod.rs index 658c2becd..279afd8a2 100644 --- a/utils/platform/src/k8s/mod.rs +++ b/utils/platform/src/k8s/mod.rs @@ -19,12 +19,24 @@ impl K8s { /// Create new `Self` from a given `Client`. pub async fn from(client: Client) -> Result { let platform_uuid = Self::fetch_platform_uuid(client).await?; + // todo: should we use the default namespace from the client? let platform_ns = std::env::var("MY_POD_NAMESPACE").unwrap_or_else(|_| "default".into()); Ok(Self { platform_uuid, platform_ns, }) } + /// Create new `Self` from a given `Client` and namespace. + /// This may be necessary if we're running outside of the cluster itself, which + /// means that MY_POD_NAMESPACE itself is not available... Perhaps using a client + /// configured with the namespace as default would be better. + pub async fn from_custom(client: Client, namespace: &str) -> Result { + let platform_uuid = Self::fetch_platform_uuid(client).await?; + Ok(Self { + platform_uuid, + platform_ns: namespace.to_string(), + }) + } async fn fetch_platform_uuid(client: Client) -> Result { let namespaces: Api = Api::all(client); let ns = namespaces