feat: Support graceful shutdown (#740)

* feat: Support graceful shutdown * docs * changelog * Update docs/modules/zookeeper/pages/usage_guide/operations/graceful-shutdown.adoc Co-authored-by: Malte Sander <contact@maltesander.com> * Use formatdoc! * Update docs/modules/zookeeper/pages/usage_guide/operations/graceful-shutdown.adoc Co-authored-by: Malte Sander <contact@maltesander.com> --------- Co-authored-by: Malte Sander <contact@maltesander.com>
stackabletech · Nov 6, 2023 · c930bd2 · c930bd2
1 parent 5a472ee
commit c930bd2
Show file tree

Hide file tree

Showing 11 changed files with 113 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,11 +10,11 @@ All notable changes to this project will be documented in this file.
 - Configuration overrides for the JVM security properties, such as DNS caching ([#715]).
 - Support PodDisruptionBudgets ([#730], [#731]).
 - Support for ZooKeeper 3.8.3 added ([#732]).
+- Support graceful shutdown ([#740]).
 
 ### Changed
 
 - `vector` `0.26.0` -> `0.33.0` ([#709], [#732]).
-- `operator-rs` `0.44.0` -> `0.52.1` ([#711], [#730], [#731]).
 - Let secret-operator handle certificate conversion ([#695]).
 - [BREAKING]: removed the `logging` layer in the `clusterConfig` CRD to make logging config consistent with other operators ([#739]).
 
@@ -30,6 +30,7 @@ All notable changes to this project will be documented in this file.
 [#731]: https://github.com/stackabletech/zookeeper-operator/pull/731
 [#732]: https://github.com/stackabletech/zookeeper-operator/pull/732
 [#739]: https://github.com/stackabletech/zookeeper-operator/pull/739
+[#740]: https://github.com/stackabletech/zookeeper-operator/pull/740
 
 ## [23.7.0] - 2023-07-14
 

diff --git a/Cargo.lock b/Cargo.lock
diff --git a/Cargo.toml b/Cargo.toml
@@ -15,6 +15,7 @@ clap = "4.3"
 failure = "0.1"
 fnv = "1.0"
 futures = { version = "0.3", features = ["compat"] }
+indoc = "2.0"
 pin-project = "1.1"
 rstest = "0.18"
 semver = "1.0"

diff --git a/deploy/helm/zookeeper-operator/crds/crds.yaml b/deploy/helm/zookeeper-operator/crds/crds.yaml
@@ -628,6 +628,10 @@ spec:
                                   type: array
                               type: object
                           type: object
+                        gracefulShutdownTimeout:
+                          description: Time period Pods have to gracefully shut down, e.g. `30m`, `1h` or `2d`. Consult the operator documentation for details.
+                          nullable: true
+                          type: string
                         initLimit:
                           format: uint32
                           minimum: 0.0
@@ -4132,6 +4136,10 @@ spec:
                                         type: array
                                     type: object
                                 type: object
+                              gracefulShutdownTimeout:
+                                description: Time period Pods have to gracefully shut down, e.g. `30m`, `1h` or `2d`. Consult the operator documentation for details.
+                                nullable: true
+                                type: string
                               initLimit:
                                 format: uint32
                                 minimum: 0.0

diff --git a/docs/modules/zookeeper/pages/usage_guide/operations/graceful-shutdown.adoc b/docs/modules/zookeeper/pages/usage_guide/operations/graceful-shutdown.adoc
@@ -1,7 +1,14 @@
 = Graceful shutdown
 
-Graceful shutdown of Zookeeper nodes is either not supported by the product itself
-or we have not implemented it yet.
+You can configure the graceful shutdown as described in xref:concepts:operations/graceful_shutdown.adoc[].
 
-The efforts of implementing graceful shutdown for all products that have this functionality is tracked in
-https://github.com/stackabletech/issues/issues/357
+== Servers
+
+As a default, ZooKeeper servers have `2 minutes` to shut down gracefully.
+
+The ZooKeeper server process will receive a `SIGTERM` signal when Kubernetes wants to terminate the Pod.
+After the graceful shutdown timeout runs out, and the process still didn't exit, Kubernetes will issue a `SIGKILL` signal.
+
+This is equivalent to executing the `bin/zkServer.sh stop` command, which internally executes `kill <zookeeper-pid>` (https://github.com/apache/zookeeper/blob/74db005175a4ec545697012f9069cb9dcc8cdda7/bin/zkServer.sh#L219[code]).
+
+However, there is no acknowledge message in the log indicating a graceful shutdown.
diff --git a/rust/crd/src/lib.rs b/rust/crd/src/lib.rs
@@ -25,6 +25,7 @@ use stackable_operator::{
     role_utils::{GenericRoleConfig, Role, RoleGroup, RoleGroupRef},
     schemars::{self, JsonSchema},
     status::condition::{ClusterCondition, HasStatusCondition},
+    time::Duration,
 };
 use strum::{Display, EnumIter, EnumString, IntoEnumIterator};
 
@@ -67,6 +68,8 @@ const JVM_HEAP_FACTOR: f32 = 0.8;
 
 pub const DOCKER_IMAGE_BASE_NAME: &str = "zookeeper";
 
+const DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT: Duration = Duration::from_minutes_unchecked(2);
+
 mod built_info {
     pub const CARGO_PKG_VERSION: &str = env!("CARGO_PKG_VERSION");
 }
@@ -210,12 +213,19 @@ pub struct ZookeeperConfig {
     pub sync_limit: Option<u32>,
     pub tick_time: Option<u32>,
     pub myid_offset: u16,
+
     #[fragment_attrs(serde(default))]
     pub resources: Resources<ZookeeperStorageConfig, NoRuntimeLimits>,
+
     #[fragment_attrs(serde(default))]
     pub logging: Logging<Container>,
+
     #[fragment_attrs(serde(default))]
     pub affinity: StackableAffinity,
+
+    /// Time period Pods have to gracefully shut down, e.g. `30m`, `1h` or `2d`. Consult the operator documentation for details.
+    #[fragment_attrs(serde(default))]
+    pub graceful_shutdown_timeout: Option<Duration>,
 }
 
 #[derive(Clone, Debug, Default, JsonSchema, PartialEq, Fragment)]
@@ -292,6 +302,7 @@ impl ZookeeperConfig {
             },
             logging: product_logging::spec::default_logging(),
             affinity: get_affinity(cluster_name, role),
+            graceful_shutdown_timeout: Some(DEFAULT_SERVER_GRACEFUL_SHUTDOWN_TIMEOUT),
         }
     }
 }

diff --git a/rust/operator-binary/Cargo.toml b/rust/operator-binary/Cargo.toml
@@ -17,16 +17,17 @@ clap.workspace = true
 failure.workspace = true
 fnv.workspace = true
 futures.workspace = true
+indoc.workspace = true
+pin-project.workspace = true
+product-config.workspace = true
 semver.workspace = true
 serde.workspace = true
 snafu.workspace = true
+stackable-operator.workspace = true
 strum.workspace = true
-tokio.workspace = true
 tokio-zookeeper.workspace = true
+tokio.workspace = true
 tracing.workspace = true
-pin-project.workspace = true
-stackable-operator.workspace = true
-product-config.workspace = true
 
 [dev-dependencies]
 rstest.workspace = true

diff --git a/rust/operator-binary/src/operations/graceful_shutdown.rs b/rust/operator-binary/src/operations/graceful_shutdown.rs
@@ -0,0 +1,26 @@
+use snafu::{ResultExt, Snafu};
+use stackable_operator::builder::PodBuilder;
+use stackable_zookeeper_crd::ZookeeperConfig;
+
+#[derive(Debug, Snafu)]
+pub enum Error {
+    #[snafu(display("Failed to set terminationGracePeriod"))]
+    SetTerminationGracePeriod {
+        source: stackable_operator::builder::pod::Error,
+    },
+}
+
+pub fn add_graceful_shutdown_config(
+    merged_config: &ZookeeperConfig,
+    pod_builder: &mut PodBuilder,
+) -> Result<(), Error> {
+    // This must be always set by the merge mechanism, as we provide a default value,
+    // users can not disable graceful shutdown.
+    if let Some(graceful_shutdown_timeout) = merged_config.graceful_shutdown_timeout {
+        pod_builder
+            .termination_grace_period(&graceful_shutdown_timeout)
+            .context(SetTerminationGracePeriodSnafu)?;
+    }
+
+    Ok(())
+}
diff --git a/rust/operator-binary/src/operations/mod.rs b/rust/operator-binary/src/operations/mod.rs
@@ -1 +1,2 @@
+pub mod graceful_shutdown;
 pub mod pdb;
diff --git a/rust/operator-binary/src/zk_controller.rs b/rust/operator-binary/src/zk_controller.rs
@@ -8,6 +8,7 @@ use std::{
 };
 
 use fnv::FnvHasher;
+use indoc::formatdoc;
 use product_config::{
     types::PropertyNameKind,
     writer::{to_java_properties_string, PropertiesWriterError},
@@ -42,6 +43,7 @@ use stackable_operator::{
     product_config_utils::{transform_all_roles_to_config, validate_all_roles_and_groups_config},
     product_logging::{
         self,
+        framework::{create_vector_shutdown_file_command, remove_vector_shutdown_file_command},
         spec::{
             ConfigMapLogConfig, ContainerLogConfig, ContainerLogConfigChoice,
             CustomContainerLogConfig,
@@ -53,6 +55,7 @@ use stackable_operator::{
         statefulset::StatefulSetConditionBuilder,
     },
     time::Duration,
+    utils::COMMON_BASH_TRAP_FUNCTIONS,
 };
 use stackable_zookeeper_crd::{
     security::ZookeeperSecurity, Container, ZookeeperCluster, ZookeeperClusterStatus,
@@ -66,7 +69,7 @@ use strum::{EnumDiscriminants, IntoStaticStr};
 use crate::{
     command::create_init_container_command_args,
     discovery::{self, build_discovery_configmaps},
-    operations::pdb::add_pdbs,
+    operations::{graceful_shutdown::add_graceful_shutdown_config, pdb::add_pdbs},
     product_logging::{extend_role_group_config_map, resolve_vector_aggregator_address},
     utils::build_recommended_labels,
     ObjectRef, APP_NAME, OPERATOR_NAME,
@@ -223,6 +226,11 @@ pub enum Error {
     FailedToCreatePdb {
         source: crate::operations::pdb::Error,
     },
+
+    #[snafu(display("failed to configure graceful shutdown"))]
+    GracefulShutdown {
+        source: crate::operations::graceful_shutdown::Error,
+    },
 }
 
 type Result<T, E = Error> = std::result::Result<T, E>;
@@ -261,6 +269,7 @@ impl ReconcilerError for Error {
             Error::FailedToInitializeSecurityContext { .. } => None,
             Error::FailedToResolveConfig { .. } => None,
             Error::FailedToCreatePdb { .. } => None,
+            Error::GracefulShutdown { .. } => None,
         }
     }
 }
@@ -725,8 +734,14 @@ fn build_server_rolegroup_statefulset(
 
     let container_prepare = cb_prepare
         .image_from_product_image(resolved_product_image)
-        .command(vec!["bash".to_string(), "-c".to_string()])
-        .args(vec![args.join(" && ")])
+        .command(vec![
+            "/bin/bash".to_string(),
+            "-x".to_string(),
+            "-euo".to_string(),
+            "pipefail".to_string(),
+            "-c".to_string(),
+        ])
+        .args(vec![args.join("\n")])
         .add_env_vars(env_vars.clone())
         .add_env_vars(vec![EnvVar {
             name: "POD_NAME".to_string(),
@@ -755,11 +770,26 @@ fn build_server_rolegroup_statefulset(
 
     let container_zk = cb_zookeeper
         .image_from_product_image(resolved_product_image)
-        .args(vec![
-            "bin/zkServer.sh".to_string(),
-            "start-foreground".to_string(),
-            format!("{dir}/zoo.cfg", dir = STACKABLE_RW_CONFIG_DIR),
+        .command(vec![
+            "/bin/bash".to_string(),
+            "-x".to_string(),
+            "-euo".to_string(),
+            "pipefail".to_string(),
+            "-c".to_string(),
         ])
+        .args(vec![formatdoc! {"
+            {COMMON_BASH_TRAP_FUNCTIONS}
+            {remove_vector_shutdown_file_command}
+            prepare_signal_handlers
+            bin/zkServer.sh start-foreground {STACKABLE_RW_CONFIG_DIR}/zoo.cfg &
+            wait_for_termination $!
+            {create_vector_shutdown_file_command}
+            ",
+            remove_vector_shutdown_file_command =
+                remove_vector_shutdown_file_command(STACKABLE_LOG_DIR),
+            create_vector_shutdown_file_command =
+                create_vector_shutdown_file_command(STACKABLE_LOG_DIR),
+        }])
         .add_env_vars(env_vars)
         // Only allow the global load balancing service to send traffic to pods that are members of the quorum
         // This also acts as a hint to the StatefulSet controller to wait for each pod to enter quorum before taking down the next
@@ -876,6 +906,8 @@ fn build_server_rolegroup_statefulset(
         ));
     }
 
+    add_graceful_shutdown_config(config, &mut pod_builder).context(GracefulShutdownSnafu)?;
+
     let mut pod_template = pod_builder.build_template();
     pod_template.merge_from(role.config.pod_overrides.clone());
     pod_template.merge_from(rolegroup.config.pod_overrides.clone());

diff --git a/tests/templates/kuttl/smoke/10-assert.yaml.j2 b/tests/templates/kuttl/smoke/10-assert.yaml.j2
@@ -24,6 +24,7 @@ spec:
 {% if lookup('env', 'VECTOR_AGGREGATOR') %}
         - name: vector
 {% endif %}
+      terminationGracePeriodSeconds: 120
 status:
   readyReplicas: 2
   replicas: 2
@@ -47,6 +48,7 @@ spec:
 {% if lookup('env', 'VECTOR_AGGREGATOR') %}
         - name: vector
 {% endif %}
+      terminationGracePeriodSeconds: 120
 status:
   readyReplicas: 1
   replicas: 1