error handling

Author: sbernauer

crates/stackable-operator/src/builder/pod/container.rs

@@ -9,7 +9,8 @@ use k8s_openapi::api::core::v1::{
     LifecycleHandler, ObjectFieldSelector, Probe, ResourceRequirements, SecretKeySelector,
     SecurityContext, VolumeMount,
 };
-use snafu::{ensure, ResultExt as _, Snafu};
+use snafu::{ResultExt as _, Snafu};
+use tracing::instrument;
 
 use crate::{
     commons::product_image_selection::ResolvedProductImage,
@@ -27,14 +28,11 @@ pub enum Error {
     },
 
     #[snafu(display(
-        "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content. \
-            The clashing mountPath is {clashing_mount_path:?}, \
-            the existing mount's volume name is {existing_volume_name:?} \
-            and the new mount's volume name is {new_volume_name:?}. \
-            Please have a look at the log/traces for details"
+        "Colliding mountPath {mount_path:?} in volumeMounts with different content. \
+            Existing volume name {existing_volume_name:?}, new volume name {new_volume_name:?}"
     ))]
-    ClashingMountPath {
-        clashing_mount_path: String,
+    MountPathCollision {
+        mount_path: String,
         existing_volume_name: String,
         new_volume_name: String,
     },
@@ -219,25 +217,24 @@ impl ContainerBuilder {
     /// Historically this function unconditionally added volumeMounts, which resulted in invalid
     /// [`k8s_openapi::api::core::v1::PodSpec`]s, as volumeMounts where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`]
     /// two times, resulting in e.g. the s3 credentials being mounted twice as the same volumeMount.
+    #[instrument(skip(self))]
     fn add_volume_mount_impl(&mut self, volume_mount: VolumeMount) -> Result<&mut Self> {
         if let Some(existing_volume_mount) = self.volume_mounts.get(&volume_mount.mount_path) {
-            // We don't want to include the details in the error message, but instead trace them
             if existing_volume_mount != &volume_mount {
+                let colliding_mount_path = &volume_mount.mount_path;
+                // We don't want to include the details in the error message, but instead trace them
                 tracing::error!(
-                    clashing_mount_path = &volume_mount.mount_path,
+                    colliding_mount_path,
                     ?existing_volume_mount,
-                    new_volume_mount = ?volume_mount,
-                    "The volumeMount is clashing with an already existing volumeMount with the same mountPath but a different content"
+                    "Colliding mountPath {colliding_mount_path:?} in volumeMounts with different content"
                 );
             }
-            ensure!(
-                existing_volume_mount == &volume_mount,
-                ClashingMountPathSnafu {
-                    clashing_mount_path: volume_mount.mount_path,
-                    existing_volume_name: existing_volume_mount.name.clone(),
-                    new_volume_name: volume_mount.name,
-                }
-            );
+            MountPathCollisionSnafu {
+                mount_path: &volume_mount.mount_path,
+                existing_volume_name: &existing_volume_mount.name,
+                new_volume_name: &volume_mount.name,
+            }
+            .fail()?;
         } else {
             self.volume_mounts
                 .insert(volume_mount.mount_path.clone(), volume_mount);

crates/stackable-operator/src/builder/pod/mod.rs

@@ -9,8 +9,8 @@ use k8s_openapi::{
     },
     apimachinery::pkg::{api::resource::Quantity, apis::meta::v1::ObjectMeta},
 };
-use snafu::{ensure, OptionExt, ResultExt, Snafu};
-use tracing::warn;
+use snafu::{OptionExt, ResultExt, Snafu};
+use tracing::{instrument, warn};
 
 use crate::kvp::Labels;
 use crate::{
@@ -52,9 +52,8 @@ pub enum Error {
     #[snafu(display("object is missing key {key:?}"))]
     MissingObjectKey { key: &'static str },
 
-    #[snafu(display("The volume {clashing_volume_name:?} clashes with an existing volume of the same name but with different content. \
-        Please have a look at the log/traces for details."))]
-    ClashingVolumeName { clashing_volume_name: String },
+    #[snafu(display("Colliding volume name {volume_name:?} in volumes with different content"))]
+    VolumeNameCollision { volume_name: String },
 }
 
 /// A builder to build [`Pod`] or [`PodTemplateSpec`] objects.
@@ -286,24 +285,23 @@ impl PodBuilder {
     /// Historically this function unconditionally added volumes, which resulted in invalid [`PodSpec`]s, as volumes
     /// where added multiple times - think of Trino using the same [`crate::commons::s3::S3Connection`] two times,
     /// resulting in e.g. the s3 credentials being mounted twice as the sake volume.
+    #[instrument(skip(self))]
     pub fn add_volume(&mut self, volume: Volume) -> Result<&mut Self> {
         if let Some(existing_volume) = self.volumes.get(&volume.name) {
-            // We don't want to include the details in the error message, but instead trace them
             if existing_volume != &volume {
-                let clashing_name = &volume.name;
+                let colliding_name = &volume.name;
+                // We don't want to include the details in the error message, but instead trace them
                 tracing::error!(
-                    clashing_name,
+                    colliding_name,
                     ?existing_volume,
-                    new_volume = ?volume,
-                    "The volume {clashing_name:?} clashes with an existing volume of the same name but with different content",
+                    "Colliding volume name {colliding_name:?} in volumes with different content"
                 );
-            }
-            ensure!(
-                existing_volume == &volume,
-                ClashingVolumeNameSnafu {
-                    clashing_volume_name: volume.name.clone(),
+
+                VolumeNameCollisionSnafu {
+                    volume_name: &volume.name,
                 }
-            );
+                .fail()?;
+            }
         } else {
             self.volumes.insert(volume.name.clone(), volume);
         }