diff --git a/CHANGELOG.md b/CHANGELOG.md index 34f4d3e5..6633fee1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,10 +15,12 @@ All notable changes to this project will be documented in this file. the CSI node driver DaemonSet previously ignored in favour of a hardcoded name. `serviceAccount.create=false` now requires `serviceAccount.name`; it used to fall back to the namespace default ServiceAccount, which lacks the operator ClusterRole ([#736]). +- `CreateVolume` no longer returns gRPC codes that make external-provisioner retry indefinitely ([#743]). [#730]: https://github.com/stackabletech/secret-operator/pull/730 [#735]: https://github.com/stackabletech/secret-operator/pull/735 [#736]: https://github.com/stackabletech/secret-operator/pull/736 +[#743]: https://github.com/stackabletech/secret-operator/pull/743 ## [26.7.0] - 2026-07-21 diff --git a/rust/operator-binary/src/csi_server/controller.rs b/rust/operator-binary/src/csi_server/controller.rs index 5de1fcf1..6e7ace45 100644 --- a/rust/operator-binary/src/csi_server/controller.rs +++ b/rust/operator-binary/src/csi_server/controller.rs @@ -6,9 +6,10 @@ use stackable_operator::{ k8s_openapi::api::core::v1::{PersistentVolumeClaim, Pod}, kube::runtime::reflector::ObjectRef, }; -use tonic::{Request, Response, Status}; +use tonic::{Code, Request, Response, Status}; use uuid::Uuid; +use super::log_if_endpoint_error; use crate::{ backend::{ self, InternalSecretVolumeSelectorParams, SecretBackendError, SecretVolumeSelector, @@ -70,23 +71,64 @@ enum CreateVolumeError { NoMatchingNode, } +/// Rewrites the gRPC codes that `external-provisioner` treats as "the operation may still be +/// running, call CreateVolume again later" into a terminal code. +/// +/// Such a code makes the provisioner park the PVC in an in-memory map that is only ever cleared +/// on success or on a terminal code. +/// +/// [`Controller::create_volume`] never leaves anything running in the background. It creates no +/// state per volume, so none of these codes may escape it. +/// The backends still use them for [`Controller::node_publish_volume`], +/// where they are correct, and so must be rewritten here rather than at their source. +fn make_terminal(code: Code) -> Code { + match code { + Code::Unavailable | Code::Aborted | Code::Cancelled | Code::DeadlineExceeded => { + Code::Internal + } + code => code, + } +} + impl From for Status { fn from(err: CreateVolumeError) -> Self { let full_msg = error_full_message(&err); // Convert to an appropriate tonic::Status representation and include full error message - match err { - CreateVolumeError::InvalidParams { .. } => Status::invalid_argument(full_msg), - CreateVolumeError::FindPvc { .. } => Status::unavailable(full_msg), - CreateVolumeError::ResolveOwnerPod { .. } => Status::failed_precondition(full_msg), - CreateVolumeError::GetPod { .. } => Status::unavailable(full_msg), - CreateVolumeError::ParsePod { .. } => Status::failed_precondition(full_msg), - CreateVolumeError::InvalidSecretSelector { .. } => { - Status::failed_precondition(full_msg) - } - CreateVolumeError::InitBackend { source } => Status::new(source.grpc_code(), full_msg), - CreateVolumeError::FindNodes { source } => Status::new(source.grpc_code(), full_msg), - CreateVolumeError::NoMatchingNode => Status::unavailable(full_msg), + let raw_code = match err { + CreateVolumeError::InvalidParams { .. } => Code::InvalidArgument, + CreateVolumeError::FindPvc { .. } => Code::FailedPrecondition, + CreateVolumeError::ResolveOwnerPod { .. } => Code::FailedPrecondition, + CreateVolumeError::GetPod { .. } => Code::FailedPrecondition, + CreateVolumeError::ParsePod { .. } => Code::FailedPrecondition, + CreateVolumeError::InvalidSecretSelector { .. } => Code::FailedPrecondition, + CreateVolumeError::InitBackend { source } => source.grpc_code(), + CreateVolumeError::FindNodes { source } => source.grpc_code(), + // CSI defines ResourceExhausted as "unable to provision in accessible_topology", which + // describes this case, but external-provisioner turns it into ProvisioningReschedule + // whenever the volume has a selected node, so it strips the node annotation and the + // scheduler picks another one. No node ever satisfies the scopes here, so the retry is + // futile. + // Additionally that path is not rate limited: the claim is forgotten rather than + // requeued, leaving the scheduler to re-trigger it immediately. + // + // ResourceExhausted would only stay dormant today because the operator has no `update` on + // persistentvolumeclaims and the annotation delete therefore fails (see roles.yaml). + // FailedPrecondition does not depend on that. + CreateVolumeError::NoMatchingNode => Code::FailedPrecondition, + }; + + // Applied to every variant, including the codes inherited from the backends (which are + // only known at runtime), so that no future variant can reintroduce the retry loop. + let code = make_terminal(raw_code); + if code != raw_code { + tracing::debug!( + grpc.code.original = ?raw_code, + grpc.code.returned = ?code, + "Mapped CreateVolume error code:" + ); } + + Status::new(code, full_msg) } } @@ -184,52 +226,59 @@ impl Controller for SecretProvisionerController { request: Request, ) -> Result, Status> { use create_volume_error::*; - let request = request.into_inner(); - let params = CreateVolumeParams::deserialize(request.parameters.into_deserializer()) - .context(InvalidParamsSnafu)?; - let (pvc_selector, selector) = self.get_pvc_secret_selector(¶ms).await?; - - let pod = self - .client - .get::(&selector.pod, &selector.namespace) - .await - .context(GetPodSnafu)?; - let pod_info = SchedulingPodInfo::from_pod(&self.client, &pod, &selector.scope) - .await - .context(ParsePodSnafu)?; - - let backend = backend::dynamic::from_selector(&self.client, &selector) - .await - .context(create_volume_error::InitBackendSnafu)?; - let accessible_topology = match backend - .get_qualified_node_names(&selector, pod_info) - .await - .context(create_volume_error::FindNodesSnafu)? - { - // No node constraints apply to this volume, so allow any topology - None => Vec::new(), - // No nodes match the constraints on this volume, so fail - Some(nodes) if nodes.is_empty() => { - return Err(create_volume_error::NoMatchingNodeSnafu.build().into()); + log_if_endpoint_error( + "failed to create volume", + async move { + let request = request.into_inner(); + let params = + CreateVolumeParams::deserialize(request.parameters.into_deserializer()) + .context(InvalidParamsSnafu)?; + let (pvc_selector, selector) = self.get_pvc_secret_selector(¶ms).await?; + + let pod = self + .client + .get::(&selector.pod, &selector.namespace) + .await + .context(GetPodSnafu)?; + let pod_info = SchedulingPodInfo::from_pod(&self.client, &pod, &selector.scope) + .await + .context(ParsePodSnafu)?; + + let backend = backend::dynamic::from_selector(&self.client, &selector) + .await + .context(create_volume_error::InitBackendSnafu)?; + let accessible_topology = match backend + .get_qualified_node_names(&selector, pod_info) + .await + .context(create_volume_error::FindNodesSnafu)? + { + // No node constraints apply to this volume, so allow any topology + None => Vec::new(), + // No nodes match the constraints on this volume, so fail + Some(nodes) if nodes.is_empty() => { + return Err(create_volume_error::NoMatchingNodeSnafu.build().into()); + } + // Matching nodes were found, only allow scheduling to them + Some(nodes) => nodes + .into_iter() + .map(|node| Topology { + segments: [(TOPOLOGY_NODE.to_string(), node)].into(), + }) + .collect(), + }; + Ok(Response::new(CreateVolumeResponse { + volume: Some(Volume { + // We don't care about the volume ID ourselves, but generate something unique + // in case anyone else relies on it for some kind of deduplication + volume_id: Uuid::new_v4().to_string(), + accessible_topology, + volume_context: pvc_selector.into_iter().collect(), + ..Volume::default() + }), + })) } - // Matching nodes were found, only allow scheduling to them - Some(nodes) => nodes - .into_iter() - .map(|node| Topology { - segments: [(TOPOLOGY_NODE.to_string(), node)].into(), - }) - .collect(), - }; - Ok(Response::new(CreateVolumeResponse { - volume: Some(Volume { - // We don't care about the volume ID ourselves, but generate something unique - // in case anyone else relies on it for some kind of deduplication - volume_id: Uuid::new_v4().to_string(), - accessible_topology, - volume_context: pvc_selector.into_iter().collect(), - ..Volume::default() - }), - })) + .await, + ) } async fn delete_volume( @@ -325,3 +374,44 @@ struct CreateVolumeParams { #[serde(rename = "csi.storage.k8s.io/pvc/namespace")] pvc_namespace: String, } + +#[cfg(test)] +mod tests { + use tonic::Code; + + use super::make_terminal; + + /// Every code `external-provisioner` reads as "may still be running in the background". + const RETRIED_FOREVER: [Code; 4] = [ + Code::Unavailable, + Code::Aborted, + Code::Cancelled, + Code::DeadlineExceeded, + ]; + + /// Every variant of [`CreateVolumeError`] funnels through [`make_terminal`], including the + /// codes inherited from the backends, so covering the whole code space here covers every + /// error `CreateVolume` can produce. + #[test] + fn create_volume_never_returns_a_retried_forever_code() { + for raw in 0..=16 { + let code = Code::from_i32(raw); + assert!( + !RETRIED_FOREVER.contains(&make_terminal(code)), + "{code:?} must not be returned by CreateVolume, it makes external-provisioner \ + retry the PVC forever (issue #722)" + ); + } + } + + /// The rewrite must only touch the codes that cause the retry loop. + #[test] + fn make_terminal_leaves_other_codes_alone() { + for raw in 0..=16 { + let code = Code::from_i32(raw); + if !RETRIED_FOREVER.contains(&code) { + assert_eq!(make_terminal(code), code); + } + } + } +} diff --git a/rust/operator-binary/src/csi_server/mod.rs b/rust/operator-binary/src/csi_server/mod.rs index c3af2f16..739db6ae 100644 --- a/rust/operator-binary/src/csi_server/mod.rs +++ b/rust/operator-binary/src/csi_server/mod.rs @@ -1,3 +1,17 @@ pub mod controller; pub mod identity; pub mod node; + +/// Logs the error returned by a CSI endpoint, if any. +/// +/// CSI errors are otherwise only visible to whoever called us (the Kubelet, or the +/// external-provisioner sidecar), never in our own logs. +fn log_if_endpoint_error( + error_msg: &str, + res: Result, +) -> Result { + if let Err(err) = &res { + tracing::warn!(error = err as &dyn std::error::Error, "{error_msg}"); + } + res +} diff --git a/rust/operator-binary/src/csi_server/node.rs b/rust/operator-binary/src/csi_server/node.rs index 4094b3b7..822a7a06 100644 --- a/rust/operator-binary/src/csi_server/node.rs +++ b/rust/operator-binary/src/csi_server/node.rs @@ -20,7 +20,7 @@ use tokio::{ }; use tonic::{Request, Response, Status}; -use super::controller::TOPOLOGY_NODE; +use super::{controller::TOPOLOGY_NODE, log_if_endpoint_error}; use crate::{ backend::{ self, SecretBackendError, SecretContents, SecretVolumeSelector, @@ -496,13 +496,3 @@ impl Node for SecretProvisionerNode { })) } } - -fn log_if_endpoint_error( - error_msg: &str, - res: Result, -) -> Result { - if let Err(err) = &res { - tracing::warn!(error = err as &dyn std::error::Error, "{error_msg}"); - } - res -}