From 99aeba77c79a2709b044b23b61ee69063a435c99 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 6 Mar 2025 15:10:06 +0100 Subject: [PATCH 1/3] fix: Add proper startUp probe --- CHANGELOG.md | 7 +- rust/operator-binary/src/controller.rs | 88 ++++++++++++++++++-------- 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 74afe5ee..fa2bbae9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,12 +12,17 @@ All notable changes to this project will be documented in this file. - Support configuring JVM arguments ([#677]). - Aggregate emitted Kubernetes events on the CustomResources ([#677]). -## Changed +### Changed - Increased the default temporary secret lifetime for coordinators from 1 day to 15 days. This is because Trino currently does not offer a HA setup for them, a restart kills all running queries ([#694]). - Default to OCI for image metadata and product image selection ([#695]). +### Fixed + +- Add a startupProbe, which checks via `/v1/info` that the coordinator/worker have finished starting. + Also migrate the other probes from `tcpSocket` to `httpGet` on `/v1/info` ([#XXX]). + [#676]: https://github.com/stackabletech/trino-operator/pull/676 [#677]: https://github.com/stackabletech/trino-operator/pull/677 [#687]: https://github.com/stackabletech/trino-operator/pull/687 diff --git a/rust/operator-binary/src/controller.rs b/rust/operator-binary/src/controller.rs index 27c08b36..8ac9d3a1 100644 --- a/rust/operator-binary/src/controller.rs +++ b/rust/operator-binary/src/controller.rs @@ -35,8 +35,8 @@ use stackable_operator::{ api::{ apps::v1::{StatefulSet, StatefulSetSpec}, core::v1::{ - ConfigMap, ConfigMapVolumeSource, ContainerPort, EnvVar, EnvVarSource, Probe, - Secret, SecretKeySelector, Service, ServicePort, ServiceSpec, TCPSocketAction, + ConfigMap, ConfigMapVolumeSource, ContainerPort, EnvVar, EnvVarSource, ExecAction, + HTTPGetAction, Probe, Secret, SecretKeySelector, Service, ServicePort, ServiceSpec, Volume, }, }, @@ -1061,6 +1061,8 @@ fn build_rolegroup_statefulset( .context(AddVolumeMountSnafu)? .add_container_ports(container_ports(trino)) .resources(merged_config.resources.clone().into()) + // The probes are set on coordinators and workers + .startup_probe(startup_probe(trino)) .readiness_probe(readiness_probe(trino)) .liveness_probe(liveness_probe(trino)) .build(); @@ -1484,40 +1486,74 @@ fn container_ports(trino: &v1alpha1::TrinoCluster) -> Vec { ports } -fn readiness_probe(trino: &v1alpha1::TrinoCluster) -> Probe { - let port_name = if trino.expose_https_port() { - HTTPS_PORT_NAME - } else { - HTTP_PORT_NAME - }; +fn startup_probe(trino: &v1alpha1::TrinoCluster) -> Probe { + Probe { + exec: Some(finished_starting_probe(trino)), + period_seconds: Some(5), + // Give the coordinator or worker 10 minutes to start up + failure_threshold: Some(120), + timeout_seconds: Some(3), + ..Default::default() + } +} +fn readiness_probe(trino: &v1alpha1::TrinoCluster) -> Probe { Probe { - initial_delay_seconds: Some(10), - period_seconds: Some(10), - failure_threshold: Some(5), - tcp_socket: Some(TCPSocketAction { - port: IntOrString::String(port_name.to_string()), - ..TCPSocketAction::default() - }), + http_get: Some(http_get_probe(trino)), + period_seconds: Some(5), + failure_threshold: Some(1), + timeout_seconds: Some(3), ..Probe::default() } } fn liveness_probe(trino: &v1alpha1::TrinoCluster) -> Probe { - let port_name = if trino.expose_https_port() { - HTTPS_PORT_NAME + Probe { + http_get: Some(http_get_probe(trino)), + period_seconds: Some(5), + // Coordinators are currently not highly available, so you always have a singe instance. + // Restarting it causes all queries to fail, so let's not restart it directly after the first + // probe failure, but wait for 3 failures + // NOTE: This also applies to workers + failure_threshold: Some(3), + timeout_seconds: Some(3), + ..Probe::default() + } +} + +/// Check that `/v1/info` returns `200`. +/// +/// This is the same probe as the [upstream helm-chart](https://github.com/trinodb/charts/blob/7cd0a7bff6c52e0ee6ca6d5394cd72c150ad4379/charts/trino/templates/deployment-coordinator.yaml#L214) +/// is using. +fn http_get_probe(trino: &v1alpha1::TrinoCluster) -> HTTPGetAction { + let (schema, port_name) = if trino.expose_https_port() { + ("HTTPS", HTTPS_PORT_NAME) } else { - HTTP_PORT_NAME + ("HTTP", HTTP_PORT_NAME) }; - Probe { - initial_delay_seconds: Some(30), - period_seconds: Some(10), - tcp_socket: Some(TCPSocketAction { - port: IntOrString::String(port_name.to_string()), - ..TCPSocketAction::default() - }), - ..Probe::default() + HTTPGetAction { + port: IntOrString::String(port_name.to_string()), + scheme: Some(schema.to_string()), + path: Some("/v1/info".to_string()), + ..Default::default() + } +} + +/// Wait until `/v1/info` returns `"starting":false`. +/// +/// This probe works on coordinators and workers. +fn finished_starting_probe(trino: &v1alpha1::TrinoCluster) -> ExecAction { + let port = trino.exposed_port(); + ExecAction { + command: Some(vec![ + "/bin/bash".to_string(), + "-x".to_string(), + "-euo".to_string(), + "pipefail".to_string(), + "-c".to_string(), + format!("curl --fail --insecure https://127.0.0.1:{port}/v1/info | grep --silent '\\\"starting\\\":false'"), + ]), } } From e3019a400760518e27b39d7e50172d322eed0306 Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 6 Mar 2025 15:32:20 +0100 Subject: [PATCH 2/3] changelog --- CHANGELOG.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa2bbae9..40bfc5f5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,13 +21,14 @@ All notable changes to this project will be documented in this file. ### Fixed - Add a startupProbe, which checks via `/v1/info` that the coordinator/worker have finished starting. - Also migrate the other probes from `tcpSocket` to `httpGet` on `/v1/info` ([#XXX]). + Also migrate the other probes from `tcpSocket` to `httpGet` on `/v1/info` ([#715]). [#676]: https://github.com/stackabletech/trino-operator/pull/676 [#677]: https://github.com/stackabletech/trino-operator/pull/677 [#687]: https://github.com/stackabletech/trino-operator/pull/687 [#694]: https://github.com/stackabletech/trino-operator/pull/694 [#695]: https://github.com/stackabletech/trino-operator/pull/695 +[#715]: https://github.com/stackabletech/trino-operator/pull/715 ## [24.11.1] - 2025-01-10 From 3f362adb9792f0b0726f62ced90f9c48f3da303a Mon Sep 17 00:00:00 2001 From: Sebastian Bernauer Date: Thu, 6 Mar 2025 15:33:42 +0100 Subject: [PATCH 3/3] clippy --- rust/operator-binary/src/crd/catalog/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/rust/operator-binary/src/crd/catalog/mod.rs b/rust/operator-binary/src/crd/catalog/mod.rs index ac26569b..254a409d 100644 --- a/rust/operator-binary/src/crd/catalog/mod.rs +++ b/rust/operator-binary/src/crd/catalog/mod.rs @@ -48,8 +48,8 @@ pub mod versioned { pub struct TrinoCatalogSpec { /// The `connector` defines which connector is used. pub connector: TrinoCatalogConnector, - #[serde(default)] + #[serde(default)] /// The `configOverrides` allow overriding arbitrary Trino settings. /// For example, for Hive you could add `hive.metastore.username: trino`. pub config_overrides: HashMap,