chore: Fix TODOs and add unit tests

Author: siegfriedweber

rust/operator-binary/src/controller.rs

@@ -124,7 +124,7 @@ type OpenSearchRoleGroupConfig =
 type OpenSearchNodeResources =
     stackable_operator::commons::resources::Resources<v1alpha1::StorageConfig>;
 
-/// The validated [`v1alpha1::OpenSearchConfig`]
+/// Validated [`v1alpha1::OpenSearchConfig`]
 #[derive(Clone, Debug, PartialEq)]
 pub struct ValidatedOpenSearchConfig {
     pub affinity: StackableAffinity,
@@ -135,6 +135,7 @@ pub struct ValidatedOpenSearchConfig {
     pub termination_grace_period_seconds: i64,
 }
 
+/// Validated log configuration per container
 #[derive(Clone, Debug, PartialEq)]
 pub struct ValidatedLogging {
     pub opensearch_container: ValidatedContainerLogConfigChoice,

rust/operator-binary/src/controller/build/node_config.rs

@@ -493,11 +493,6 @@ mod tests {
             ..TestConfig::default()
         });
 
-        println!(
-            "node_config_single_node: {:?}",
-            node_config_single_node.role_group_config.config
-        );
-
         let node_config_multiple_nodes = node_config(TestConfig {
             replicas: 2,
             ..TestConfig::default()

rust/operator-binary/src/controller/build/product_logging/config.rs

@@ -1,13 +1,21 @@
-use std::cmp;
+//! OpenSearch specific log configuration
+
+use std::{cmp, collections::BTreeMap};
 
 use stackable_operator::{
     memory::{BinaryMultiple, MemoryQuantity},
-    product_logging::spec::AutomaticContainerLogConfig,
+    product_logging::spec::{AppenderConfig, AutomaticContainerLogConfig, LogLevel, LoggerConfig},
 };
 
-use crate::{crd::v1alpha1, framework::product_logging::framework::STACKABLE_LOG_DIR};
+use crate::{
+    crd::v1alpha1::{self},
+    framework::{
+        builder::pod::container::{EnvVarName, EnvVarSet},
+        product_logging::framework::STACKABLE_LOG_DIR,
+    },
+};
 
-/// The log configuration file
+/// OpenSearch log configuration file
 pub const CONFIGURATION_FILE_LOG4J2_PROPERTIES: &str = "log4j2.properties";
 
 const OPENSEARCH_SERVER_LOG_FILE: &str = "opensearch_server.json";
@@ -17,24 +25,39 @@ pub const MAX_OPENSEARCH_SERVER_LOG_FILES_SIZE: MemoryQuantity = MemoryQuantity
     unit: BinaryMultiple::Mebi,
 };
 
+/// Create a log4j2 configuration from the given automatic log configuration
 pub fn create_log4j2_config(config: &AutomaticContainerLogConfig) -> String {
-    let log_path = format!(
-        "{STACKABLE_LOG_DIR}/{container}/{OPENSEARCH_SERVER_LOG_FILE}",
-        container = v1alpha1::Container::OpenSearch.to_container_name()
-    );
+    [
+        log4j2_root_logger_config(&config.root_log_level()),
+        log4j2_loggers_config(&config.loggers),
+        log4j2_console_appender_config(&config.console),
+        log4j2_file_appender_config(&config.file),
+    ]
+    .iter()
+    .flatten()
+    .map(|(key, value)| format!("{key} = {value}\n"))
+    .collect()
+}
 
-    let number_of_archived_log_files = 1;
-    let max_log_files_size_in_mib = MAX_OPENSEARCH_SERVER_LOG_FILES_SIZE
-        .scale_to(BinaryMultiple::Mebi)
-        .floor()
-        .value as u32;
-    let max_log_file_size_in_mib = cmp::max(
-        1,
-        max_log_files_size_in_mib / (1 + number_of_archived_log_files),
-    );
+fn log4j2_root_logger_config(root_log_level: &LogLevel) -> Vec<(String, String)> {
+    vec![
+        (
+            "rootLogger.level".to_owned(),
+            root_log_level.to_log4j2_literal(),
+        ),
+        (
+            "rootLogger.appenderRef.CONSOLE.ref".to_owned(),
+            "CONSOLE".to_owned(),
+        ),
+        (
+            "rootLogger.appenderRef.FILE.ref".to_owned(),
+            "FILE".to_owned(),
+        ),
+    ]
+}
 
-    let loggers = config
-        .loggers
+fn log4j2_loggers_config(loggers_config: &BTreeMap<String, LoggerConfig>) -> Vec<(String, String)> {
+    loggers_config
         .iter()
         .filter(|(name, _)| name.as_str() != AutomaticContainerLogConfig::ROOT_LOGGER)
         .enumerate()
@@ -50,24 +73,13 @@ pub fn create_log4j2_config(config: &AutomaticContainerLogConfig) -> String {
                 ),
             ]
         })
-        .collect::<Vec<_>>();
-
-    let root_logger = vec![
-        (
-            "rootLogger.level".to_owned(),
-            config.root_log_level().to_log4j2_literal(),
-        ),
-        (
-            "rootLogger.appenderRef.CONSOLE.ref".to_owned(),
-            "CONSOLE".to_owned(),
-        ),
-        (
-            "rootLogger.appenderRef.FILE.ref".to_owned(),
-            "FILE".to_owned(),
-        ),
-    ];
+        .collect::<Vec<_>>()
+}
 
-    let console_appender = vec![
+fn log4j2_console_appender_config(
+    console_appender_config: &Option<AppenderConfig>,
+) -> Vec<(String, String)> {
+    vec![
         ("appender.CONSOLE.type".to_owned(), "Console".to_owned()),
         ("appender.CONSOLE.name".to_owned(), "CONSOLE".to_owned()),
         (
@@ -90,16 +102,34 @@ pub fn create_log4j2_config(config: &AutomaticContainerLogConfig) -> String {
         ),
         (
             "appender.CONSOLE.filter.threshold.level".to_owned(),
-            config
-                .console
+            console_appender_config
                 .as_ref()
                 .and_then(|console| console.level)
                 .unwrap_or_default()
                 .to_log4j2_literal(),
         ),
-    ];
+    ]
+}
+
+fn log4j2_file_appender_config(
+    file_appender_config: &Option<AppenderConfig>,
+) -> Vec<(String, String)> {
+    let log_path = format!(
+        "{STACKABLE_LOG_DIR}/{container}/{OPENSEARCH_SERVER_LOG_FILE}",
+        container = v1alpha1::Container::OpenSearch.to_container_name()
+    );
+
+    let number_of_archived_log_files = 1;
+    let max_log_files_size_in_mib = MAX_OPENSEARCH_SERVER_LOG_FILES_SIZE
+        .scale_to(BinaryMultiple::Mebi)
+        .floor()
+        .value as u32;
+    let max_log_file_size_in_mib = cmp::max(
+        1,
+        max_log_files_size_in_mib / (1 + number_of_archived_log_files),
+    );
 
-    let file_appender = vec![
+    vec![
         ("appender.FILE.type".to_owned(), "RollingFile".to_owned()),
         ("appender.FILE.name".to_owned(), "FILE".to_owned()),
         ("appender.FILE.fileName".to_owned(), log_path.to_owned()),
@@ -141,33 +171,38 @@ pub fn create_log4j2_config(config: &AutomaticContainerLogConfig) -> String {
         ),
         (
             "appender.FILE.filter.threshold.level".to_owned(),
-            config
-                .file
+            file_appender_config
                 .as_ref()
                 .and_then(|file| file.level)
                 .unwrap_or_default()
                 .to_log4j2_literal(),
         ),
-    ];
-
-    [root_logger, loggers, console_appender, file_appender]
-        .iter()
-        .flatten()
-        .map(|(key, value)| format!("{key} = {value}\n"))
-        .collect()
+    ]
 }
 
+/// Returns the Vector configuration file content as YAML
 pub fn vector_config_file_content() -> String {
     include_str!("vector.yaml").to_owned()
 }
 
+/// Returns the OpenSearch specific environment variables used in the Vector configuration file
+///
+/// The common environment variables are already set in
+/// [`crate::framework::product_logging::framework::vector_container`].
+pub fn vector_config_file_extra_env_vars() -> EnvVarSet {
+    EnvVarSet::new().with_value(
+        &EnvVarName::from_str_unsafe("OPENSEARCH_SERVER_LOG_FILE"),
+        "opensearch_server.json",
+    )
+}
+
 #[cfg(test)]
 mod tests {
     use stackable_operator::product_logging::spec::{
         AppenderConfig, AutomaticContainerLogConfig, LogLevel, LoggerConfig,
     };
 
-    use super::create_log4j2_config;
+    use super::{create_log4j2_config, vector_config_file_extra_env_vars};
 
     #[test]
     pub fn test_create_log4j2_config() {
@@ -227,4 +262,10 @@ mod tests {
 
         assert_eq!(expected_config, log4j2_config);
     }
+
+    #[test]
+    pub fn test_vector_config_file_extra_env_vars() {
+        // Test that the function does not panic
+        vector_config_file_extra_env_vars();
+    }
 }

rust/operator-binary/src/controller/build/role_group_builder.rs

@@ -18,7 +18,7 @@ use stackable_operator::{
         },
         apimachinery::pkg::{apis::meta::v1::LabelSelector, util::intstr::IntOrString},
     },
-    kvp::{Annotations, Label, Labels},
+    kvp::{Annotation, Annotations, Label, Labels},
     product_logging::framework::{
         VECTOR_CONFIG_FILE, calculate_log_volume_size_limit, create_vector_shutdown_file_command,
         remove_vector_shutdown_file_command,
@@ -36,7 +36,9 @@ use crate::{
     constant,
     controller::{
         ContextNames, OpenSearchRoleGroupConfig, ValidatedCluster,
-        build::product_logging::config::MAX_OPENSEARCH_SERVER_LOG_FILES_SIZE,
+        build::product_logging::config::{
+            MAX_OPENSEARCH_SERVER_LOG_FILES_SIZE, vector_config_file_extra_env_vars,
+        },
     },
     crd::v1alpha1,
     framework::{
@@ -217,6 +219,13 @@ impl<'a> RoleGroupBuilder<'a> {
         let metadata = ObjectMetaBuilder::new()
             .with_labels(self.recommended_labels())
             .with_labels(node_role_labels)
+            .with_annotation(
+                Annotation::try_from((
+                    "kubectl.kubernetes.io/default-container".to_owned(),
+                    v1alpha1::Container::OpenSearch.to_container_name(),
+                ))
+                .expect("should be a valid annotation"),
+            )
             .build();
 
         let opensearch_container = self.build_opensearch_container();
@@ -229,13 +238,12 @@ impl<'a> RoleGroupBuilder<'a> {
             .map(|vector_container_log_config| {
                 vector_container(
                     &v1alpha1::Container::Vector.to_container_name(),
-                    vector_container_log_config,
-                    &self.resource_names.cluster_name,
-                    &self.resource_names.role_name,
-                    &self.resource_names.role_group_name,
                     &self.cluster.image,
+                    vector_container_log_config,
+                    &self.resource_names,
                     &CONFIG_VOLUME_NAME,
                     &LOG_VOLUME_NAME,
+                    vector_config_file_extra_env_vars(),
                 )
             });
 
@@ -294,7 +302,6 @@ impl<'a> RoleGroupBuilder<'a> {
                         .pod_anti_affinity
                         .clone(),
                 }),
-                // TODO Add annotation that the opensearch container is the main one
                 containers: [Some(opensearch_container), vector_container]
                     .into_iter()
                     .flatten()
@@ -768,7 +775,7 @@ mod tests {
             .expect("should be serializable");
 
         // The content of log4j2.properties is already tested in the
-        // `conrtoller::build::product_logging::config` module.
+        // `controller::build::product_logging::config` module.
         config_map["data"]["log4j2.properties"].take();
         // The content of opensearch.yml is already tested in the `controller::build::node_config`
         // module.
@@ -860,6 +867,9 @@ mod tests {
                     "serviceName": "my-opensearch-cluster-nodes-default-headless",
                     "template": {
                         "metadata": {
+                            "annotations": {
+                                "kubectl.kubernetes.io/default-container": "opensearch",
+                            },
                             "labels": {
                                 "app.kubernetes.io/component": "nodes",
                                 "app.kubernetes.io/instance": "my-opensearch-cluster",
@@ -912,8 +922,12 @@ mod tests {
                                             "\n",
                                             "rm -f /stackable/log/_vector/shutdown\n",
                                             "prepare_signal_handlers\n",
+                                            "if command --search containerdebug >/dev/null 2>&1; then\n",
                                             "containerdebug --output=/stackable/log/containerdebug-state.json --loop &\n",
-                                            "/stackable/opensearch/opensearch-docker-entrypoint.sh  &\n",
+                                            "else\n",
+                                            "echo >&2 \"containerdebug not installed; Proceed without it.\"\n",
+                                            "fi\n",
+                                            "./opensearch-docker-entrypoint.sh  &\n",
                                             "wait_for_termination $!\n",
                                             "mkdir -p /stackable/log/_vector && touch /stackable/log/_vector/shutdown"
                                         )
@@ -1092,7 +1106,7 @@ mod tests {
                                     "volumeMounts": [
                                         {
                                             "mountPath": "/stackable/config/vector.yaml",
-                                            "name": "log-config",
+                                            "name": "config",
                                             "readOnly": true,
                                             "subPath": "vector.yaml",
                                         },

rust/operator-binary/src/crd/mod.rs

@@ -152,10 +152,13 @@ pub mod versioned {
         #[fragment_attrs(serde(default))]
         pub graceful_shutdown_timeout: Duration,
 
-        /// This field controls which [ListenerClass](https://docs.stackable.tech/home/nightly/listener-operator/listenerclass.html) is used to expose the HTTP communication.
+        /// This field controls which
+        /// [ListenerClass](https://docs.stackable.tech/home/nightly/listener-operator/listenerclass.html)
+        /// is used to expose the HTTP communication.
         #[fragment_attrs(serde(default))]
         pub listener_class: ListenerClassName,
 
+        /// Logging configuration
         #[fragment_attrs(serde(default))]
         pub logging: Logging<Container>,
 
@@ -169,7 +172,6 @@ pub mod versioned {
         pub resources: Resources<StorageConfig>,
     }
 
-    // TODO All derives required?
     #[derive(
         Clone,
         Debug,
@@ -258,10 +260,10 @@ impl v1alpha1::OpenSearchConfig {
             graceful_shutdown_timeout: Some(
                 Duration::from_str("2m").expect("should be a valid duration"),
             ),
-            // Defaults taken from the Helm chart, see
-            // https://github.com/opensearch-project/helm-charts/blob/opensearch-3.0.0/charts/opensearch/values.yaml#L16-L20
             listener_class: Some(DEFAULT_LISTENER_CLASS.to_owned()),
             logging: product_logging::spec::default_logging(),
+            // Defaults taken from the Helm chart, see
+            // https://github.com/opensearch-project/helm-charts/blob/opensearch-3.0.0/charts/opensearch/values.yaml#L16-L20
             node_roles: Some(NodeRoles(vec![
                 v1alpha1::NodeRole::ClusterManager,
                 v1alpha1::NodeRole::Ingest,