Set initial_cluster_manager_nodes only on cluster_manager nodes; Add opensearch-discovery scope only on cluster_manager nodes

Author: siegfriedweber

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

@@ -258,7 +258,7 @@ impl NodeConfig {
     /// The environment variables should only contain node-specific configuration options.
     /// Cluster-wide options should be added to the configuration file.
     pub fn environment_variables(&self) -> EnvVarSet {
-        EnvVarSet::new()
+        let mut env_vars = EnvVarSet::new()
             // Set the OpenSearch node name to the Pod name.
             // The node name is used e.g. for INITIAL_CLUSTER_MANAGER_NODES.
             .with_field_path(
@@ -269,10 +269,6 @@ impl NodeConfig {
                 &EnvVarName::from_str_unsafe(CONFIG_OPTION_DISCOVERY_SEED_HOSTS),
                 &self.discovery_service_name,
             )
-            .with_value(
-                &EnvVarName::from_str_unsafe(CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES),
-                self.initial_cluster_manager_nodes(),
-            )
             .with_value(
                 &EnvVarName::from_str_unsafe(CONFIG_OPTION_NODE_ROLES),
                 self.role_group_config
@@ -284,8 +280,16 @@ impl NodeConfig {
                     // Node roles cannot contain commas, therefore creating a comma-separated list
                     // is safe.
                     .join(","),
-            )
-            .merge(self.role_group_config.env_overrides.clone())
+            );
+
+        if let Some(initial_cluster_manager_nodes) = self.initial_cluster_manager_nodes() {
+            env_vars = env_vars.with_value(
+                &EnvVarName::from_str_unsafe(CONFIG_OPTION_INITIAL_CLUSTER_MANAGER_NODES),
+                initial_cluster_manager_nodes,
+            );
+        }
+
+        env_vars.merge(self.role_group_config.env_overrides.clone())
     }
 
     fn to_yaml(kv: serde_json::Map<String, Value>) -> String {
@@ -311,33 +315,70 @@ impl NodeConfig {
         }
     }
 
-    /// Configuration for `cluster.initial_cluster_manager_nodes` which replaces
-    /// `cluster.initial_master_nodes`, see
-    /// <https://github.com/opensearch-project/OpenSearch/blob/3.0.0/server/src/main/java/org/opensearch/cluster/coordination/ClusterBootstrapService.java#L79-L93>.
+    /// Configuration for `cluster.initial_cluster_manager_nodes`
+    ///
+    /// Returns the node names of the initial cluster-manager nodes if
+    /// * this is a multi-node cluster and
+    /// * this node has the cluster-manager node role.
+    ///
+    /// Please read the following sections for an explanation of these restrictions.
+    ///
+    /// This configuration setting replaces the setting `cluster.initial_master_nodes`, see
+    /// <https://github.com/opensearch-project/OpenSearch/blob/3.1.0/server/src/main/java/org/opensearch/cluster/coordination/ClusterBootstrapService.java#L79-L93>.
     ///
-    /// According to
-    /// <https://docs.opensearch.org/docs/3.0/install-and-configure/configuring-opensearch/discovery-gateway-settings/>,
-    /// it contains "a list of cluster-manager-eligible nodes used to bootstrap the cluster."
+    /// This setting is required on nodes with the cluster-manager node role on a multi-node
+    /// cluster. Otherwise the bootstrapping of the cluster fails and all pods report:
+    /// > Wait for cluster to be available ...
     ///
-    /// However, the documentation for Elasticsearch is more detailed and contains the following
-    /// notes (see <https://www.elastic.co/guide/en/elasticsearch/reference/9.0/modules-discovery-settings.html>):
+    /// This setting must not be set on a single-node cluster, because otherwise the following
+    /// error is thrown:
+    ///  > setting [cluster.initial_cluster_manager_nodes] is not allowed when [discovery.type] is set to [single-node]
+    ///
+    /// see <https://github.com/opensearch-project/OpenSearch/blob/3.1.0/server/src/main/java/org/opensearch/cluster/coordination/ClusterBootstrapService.java#L126-L136>
+    ///
+    /// This setting does not seem to have an effect on nodes without the cluster-manager node
+    /// role. However, as it is recommended (see the Elasticsearch documentation below) to not set
+    /// it on master-ineligible nodes, it is not set.
+    ///
+    /// This setting seems to be ignored when the cluster has already formed. It is recommended in
+    /// the Elasticsearch documentation to remove it once the cluster has formed, but as it is hard
+    /// to determine if the bootstrapping was successfully completed, this setting is still set.
+    /// Adding a new cluster-manager node and updating this setting also seems to be okay.
+    ///
+    /// # OpenSearch documentation
+    ///
+    /// > This setting is required when bootstrapping a cluster for the first time and should
+    /// > contain the node names (as defined by `node.name`) of the initial cluster-manager-eligible
+    /// > nodes. This list should be empty for nodes joining an existing cluster.
+    ///
+    /// see <https://docs.opensearch.org/3.3/install-and-configure/configuring-opensearch/discovery-gateway-settings/#static-discovery-settings>
+    ///
+    /// # Elasticsearch documentation
+    ///
+    /// The documentation for Elasticsearch is more detailed and contains the following
+    /// notes:
     /// * Remove this setting once the cluster has formed, and never set it again for this cluster.
     /// * Do not configure this setting on master-ineligible nodes.
     /// * Do not configure this setting on nodes joining an existing cluster.
     /// * Do not configure this setting on nodes which are restarting.
     /// * Do not configure this setting when performing a full-cluster restart.
     ///
-    /// The OpenSearch Helm chart only sets master nodes but does not handle the other cases (see
-    /// <https://github.com/opensearch-project/helm-charts/blob/opensearch-3.0.0/charts/opensearch/templates/statefulset.yaml#L414-L415>),
-    /// so they are also ignored here for the moment.
-    fn initial_cluster_manager_nodes(&self) -> String {
-        if !self.cluster.is_single_node()
-            && self
+    /// see <https://www.elastic.co/docs/reference/elasticsearch/configuration-reference/discovery-cluster-formation-settings>
+    ///
+    /// # Implementation in the OpenSearch Helm chart
+    ///
+    /// The OpenSearch Helm chart sets this setting on master nodes on multi-node clusters, see
+    /// see </home/sigi/projects/stackable/workspace/opensearch-operator/target/doc/stackable_opensearch_operator/index.html>.
+    fn initial_cluster_manager_nodes(&self) -> Option<String> {
+        if self.cluster.is_single_node()
+            || !self
                 .role_group_config
                 .config
                 .node_roles
                 .contains(&v1alpha1::NodeRole::ClusterManager)
         {
+            None
+        } else {
             let cluster_manager_configs = self
                 .cluster
                 .role_group_configs_filtered_by_node_role(&v1alpha1::NodeRole::ClusterManager);
@@ -360,11 +401,7 @@ impl NodeConfig {
                 );
             }
             // Pod names cannot contain commas, therefore creating a comma-separated list is safe.
-            pod_names.join(",")
-        } else {
-            // This setting is not allowed on single node cluster, see
-            // <https://github.com/opensearch-project/OpenSearch/blob/3.0.0/server/src/main/java/org/opensearch/cluster/coordination/ClusterBootstrapService.java#L126-L136>
-            String::new()
+            Some(pod_names.join(","))
         }
     }
 
@@ -509,7 +546,7 @@ mod tests {
             cluster,
             role_group_name,
             role_group_config,
-            ServiceName::from_str_unsafe("my-opensearch-cluster-manager"),
+            ServiceName::from_str_unsafe("my-opensearch-cluster-discovery"),
         )
     }
 
@@ -615,7 +652,7 @@ mod tests {
                 )
                 .with_value(
                     &EnvVarName::from_str_unsafe("discovery.seed_hosts"),
-                    "my-opensearch-cluster-manager",
+                    "my-opensearch-cluster-discovery",
                 )
                 .with_field_path(
                     &EnvVarName::from_str_unsafe("node.name"),
@@ -664,11 +701,11 @@ mod tests {
         });
 
         assert_eq!(
-            "".to_owned(),
+            None,
             node_config_single_node.initial_cluster_manager_nodes()
         );
         assert_eq!(
-            "my-opensearch-cluster-nodes-default-0,my-opensearch-cluster-nodes-default-1,my-opensearch-cluster-nodes-default-2".to_owned(),
+            Some("my-opensearch-cluster-nodes-default-0,my-opensearch-cluster-nodes-default-1,my-opensearch-cluster-nodes-default-2".to_owned()),
             node_config_multiple_nodes.initial_cluster_manager_nodes()
         );
     }

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

@@ -232,7 +232,6 @@ impl<'a> RoleGroupBuilder<'a> {
     /// Builds the [`PodTemplateSpec`] for the role-group [`StatefulSet`]
     fn build_pod_template(&self) -> PodTemplateSpec {
         let mut node_role_labels = Labels::new();
-        let service_scopes = vec![self.node_config.discovery_service_name.clone()];
 
         for node_role in self.role_group_config.config.node_roles.iter() {
             node_role_labels.insert(Self::build_node_role_label(node_role));
@@ -323,6 +322,15 @@ impl<'a> RoleGroupBuilder<'a> {
         ];
 
         if let Some(tls_http_secret_class_name) = &self.cluster.tls_config.server_secret_class {
+            let mut service_scopes = vec![];
+            if self
+                .role_group_config
+                .config
+                .node_roles
+                .contains(&v1alpha1::NodeRole::ClusterManager)
+            {
+                service_scopes.push(self.node_config.discovery_service_name.clone());
+            }
             volumes.push(self.build_tls_volume(
                 &TLS_SERVER_VOLUME_NAME,
                 tls_http_secret_class_name,
@@ -1129,10 +1137,6 @@ mod tests {
                                         "-c"
                                     ],
                                     "env": [
-                                        {
-                                            "name": "cluster.initial_cluster_manager_nodes",
-                                            "value": ""
-                                        },
                                         {
                                             "name": "discovery.seed_hosts",
                                             "value": "my-opensearch-cluster"

rust/operator-binary/src/framework/macros/attributed_string_type.rs

@@ -3,7 +3,7 @@ use strum::{EnumDiscriminants, IntoStaticStr};
 
 /// Maximum length of label values
 ///
-/// Duplicates the private constant [`stackable-operator::kvp::label::value::LABEL_VALUE_MAX_LEN`]
+/// Duplicates the private constant [`stackable_operator::kvp::LABEL_VALUE_MAX_LEN`]
 pub const MAX_LABEL_VALUE_LENGTH: usize = 63;
 
 #[derive(Debug, EnumDiscriminants, Snafu)]

tests/templates/kuttl/smoke/10-assert.yaml.j2

@@ -446,7 +446,6 @@ spec:
           value: "true"
         - name: OPENSEARCH_HOME
           value: {{ test_scenario['values']['opensearch_home'] }}
-        - name: cluster.initial_cluster_manager_nodes
         - name: discovery.seed_hosts
           value: opensearch-discovery
         - name: node.name
@@ -614,7 +613,7 @@ spec:
                 secrets.stackable.tech/backend.autotls.cert.lifetime: 1d
                 secrets.stackable.tech/class: tls
                 secrets.stackable.tech/format: tls-pem
-                secrets.stackable.tech/scope: service=opensearch-discovery,listener-volume=listener,pod
+                secrets.stackable.tech/scope: listener-volume=listener,pod
             spec:
               accessModes:
               - ReadWriteOnce