From 36ebe5ed60e4f2180f45ad03a94dda23812d90b3 Mon Sep 17 00:00:00 2001 From: xeniape Date: Wed, 4 Jun 2025 17:39:32 +0200 Subject: [PATCH 1/9] helm idempotent installs --- rust/helm-sys/go-helm-wrapper/main.go | 24 ++++ rust/helm-sys/src/lib.rs | 28 +++++ rust/stackable-cockpit/src/helm.rs | 112 ++++++++++++++++++ .../src/platform/manifests.rs | 2 +- 4 files changed, 165 insertions(+), 1 deletion(-) diff --git a/rust/helm-sys/go-helm-wrapper/main.go b/rust/helm-sys/go-helm-wrapper/main.go index b1fc05af..182d80e9 100644 --- a/rust/helm-sys/go-helm-wrapper/main.go +++ b/rust/helm-sys/go-helm-wrapper/main.go @@ -58,6 +58,30 @@ func go_install_helm_release(releaseName *C.char, chartName *C.char, chartVersio return C.CString("") } +//export go_upgrade_or_install_helm_release +func go_upgrade_or_install_helm_release(releaseName *C.char, chartName *C.char, chartVersion *C.char, valuesYaml *C.char, namespace *C.char, suppressOutput bool) *C.char { + helmClient := getHelmClient(namespace, suppressOutput) + + timeout, _ := time.ParseDuration("20m") + chartSpec := gohelm.ChartSpec{ + ReleaseName: C.GoString(releaseName), + ChartName: C.GoString(chartName), + Version: C.GoString(chartVersion), + ValuesYaml: C.GoString(valuesYaml), + Namespace: C.GoString(namespace), + UpgradeCRDs: true, + Wait: true, + Timeout: timeout, + Force: true, + } + + if _, err := helmClient.InstallOrUpgradeChart(context.Background(), &chartSpec, nil); err != nil { + return C.CString(fmt.Sprintf("%s%s", HELM_ERROR_PREFIX, err)) + } + + return C.CString("") +} + //export go_uninstall_helm_release func go_uninstall_helm_release(releaseName *C.char, namespace *C.char, suppressOutput bool) *C.char { helmClient := getHelmClient(namespace, suppressOutput) diff --git a/rust/helm-sys/src/lib.rs b/rust/helm-sys/src/lib.rs index 64f98965..b994ac6a 100644 --- a/rust/helm-sys/src/lib.rs +++ b/rust/helm-sys/src/lib.rs @@ -37,6 +37,34 @@ pub fn install_helm_release( } } +pub fn upgrade_or_install_helm_release( + release_name: &str, + chart_name: &str, + chart_version: &str, + values_yaml: &str, + namespace: &str, + suppress_output: bool, +) -> String { + let release_name = CString::new(release_name).unwrap(); + let chart_name = CString::new(chart_name).unwrap(); + let chart_version = CString::new(chart_version).unwrap(); + let values_yaml = CString::new(values_yaml).unwrap(); + let namespace = CString::new(namespace).unwrap(); + + unsafe { + let c = go_upgrade_or_install_helm_release( + release_name.as_ptr() as *mut c_char, + chart_name.as_ptr() as *mut c_char, + chart_version.as_ptr() as *mut c_char, + values_yaml.as_ptr() as *mut c_char, + namespace.as_ptr() as *mut c_char, + suppress_output as u8, + ); + + cstr_ptr_to_string(c) + } +} + pub fn uninstall_helm_release( release_name: &str, namespace: &str, diff --git a/rust/stackable-cockpit/src/helm.rs b/rust/stackable-cockpit/src/helm.rs index 4cca0bba..3048baa5 100644 --- a/rust/stackable-cockpit/src/helm.rs +++ b/rust/stackable-cockpit/src/helm.rs @@ -61,6 +61,9 @@ pub enum Error { #[snafu(display("failed to install Helm release"))] InstallRelease { source: InstallReleaseError }, + #[snafu(display("failed to upgrade/install Helm release"))] + UpgradeRelease { source: InstallReleaseError }, + #[snafu(display("failed to uninstall Helm release ({error})"))] UninstallRelease { error: String }, } @@ -248,6 +251,78 @@ pub fn install_release_from_repo_or_registry( }) } +/// Upgrades a Helm release from a repo or registry. +/// +/// This function expects the fully qualified Helm release name. In case of our +/// operators this is: `-operator`. +#[instrument(skip(values_yaml), fields(with_values = values_yaml.is_some(), indicatif.pb_show = true))] +pub fn upgrade_or_install_release_from_repo_or_registry( + release_name: &str, + ChartVersion { + chart_source, + chart_name, + chart_version, + }: ChartVersion, + values_yaml: Option<&str>, + namespace: &str, + suppress_output: bool, +) -> Result { + // Ideally, each Helm invocation would spawn_blocking instead in/around helm_sys, + // but that requires a larger refactoring + block_in_place(|| { + debug!("Install/Upgrade Helm release from repo"); + Span::current() + .pb_set_message(format!("Installing/Upgrading {chart_name} Helm chart").as_str()); + + if check_release_exists(release_name, namespace)? { + let release = get_release(release_name, namespace)?.ok_or(Error::InstallRelease { + source: InstallReleaseError::NoSuchRelease { + name: release_name.to_owned(), + }, + })?; + + let current_version = release.version; + + match chart_version { + Some(chart_version) => { + if chart_version == current_version { + return Ok(InstallReleaseStatus::ReleaseAlreadyInstalledWithVersion { + requested_version: chart_version.to_string(), + release_name: release_name.to_string(), + current_version, + }); + } + } + None => { + return Ok(InstallReleaseStatus::ReleaseAlreadyInstalledUnspecified { + release_name: release_name.to_string(), + current_version, + }); + } + } + } + + let full_chart_name = format!("{chart_source}/{chart_name}"); + let chart_version = chart_version.unwrap_or(HELM_DEFAULT_CHART_VERSION); + + debug!( + release_name, + chart_version, full_chart_name, "Installing Helm release" + ); + + upgrade_release( + release_name, + &full_chart_name, + chart_version, + values_yaml, + namespace, + suppress_output, + )?; + + Ok(InstallReleaseStatus::Installed(release_name.to_string())) + }) +} + /// Installs a Helm release. /// /// This function expects the fully qualified Helm release name. In case of our @@ -281,6 +356,43 @@ fn install_release( Ok(()) } +/// Upgrades a Helm release. +/// If a release with the specified `chart_name` does not already exist, +/// this function installs it instead. +/// +/// This function expects the fully qualified Helm release name. In case of our +/// operators this is: `-operator`. +#[instrument(fields(with_values = values_yaml.is_some()))] +fn upgrade_release( + release_name: &str, + chart_name: &str, + chart_version: &str, + values_yaml: Option<&str>, + namespace: &str, + suppress_output: bool, +) -> Result<(), Error> { + let result = helm_sys::upgrade_or_install_helm_release( + release_name, + chart_name, + chart_version, + values_yaml.unwrap_or(""), + namespace, + suppress_output, + ); + + if let Some(error) = helm_sys::to_helm_error(&result) { + error!( + "Go wrapper function go_upgrade_or_install_helm_release encountered an error: {error}" + ); + + return Err(Error::UpgradeRelease { + source: InstallReleaseError::HelmWrapper { error }, + }); + } + + Ok(()) +} + /// Uninstall a Helm release. /// /// This function expects the fully qualified Helm release name. In case of our diff --git a/rust/stackable-cockpit/src/platform/manifests.rs b/rust/stackable-cockpit/src/platform/manifests.rs index d9d23794..4d983317 100644 --- a/rust/stackable-cockpit/src/platform/manifests.rs +++ b/rust/stackable-cockpit/src/platform/manifests.rs @@ -116,7 +116,7 @@ pub trait InstallManifestsExt { .context(SerializeOptionsSnafu)?; // Install the Helm chart using the Helm wrapper - helm::install_release_from_repo_or_registry( + helm::upgrade_or_install_release_from_repo_or_registry( &helm_chart.release_name, helm::ChartVersion { chart_source: &helm_chart.repo.name, From efbb2097c8a00d52325036d5771e05809e3ccc73 Mon Sep 17 00:00:00 2001 From: xeniape Date: Wed, 4 Jun 2025 17:40:46 +0200 Subject: [PATCH 2/9] ignore jobs on reapply, merge instead of apply for existing objects --- .../stackable-cockpit/src/utils/k8s/client.rs | 58 ++++++++++++++++--- 1 file changed, 50 insertions(+), 8 deletions(-) diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index e678b71b..e7bda5c0 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -15,7 +15,7 @@ use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{commons::listener::Listener, kvp::Labels}; use tokio::sync::RwLock; use tracing::{Span, info, instrument}; -use tracing_indicatif::span_ext::IndicatifSpanExt as _; +use tracing_indicatif::{indicatif_println, span_ext::IndicatifSpanExt as _}; #[cfg(doc)] use crate::utils::k8s::ListParamsExt; @@ -114,6 +114,7 @@ impl Client { // TODO (Techassi): Impl IntoIterator for Labels let labels: BTreeMap = labels.into(); + let mut failed_manifests = Vec::new(); for manifest in serde_yaml::Deserializer::from_str(manifests) { let mut object = DynamicObject::deserialize(manifest).context(DeserializeYamlSnafu)?; @@ -144,13 +145,54 @@ impl Client { } }; - api.patch( - &object.name_any(), - &PatchParams::apply("stackablectl"), - &Patch::Apply(object), - ) - .await - .context(KubeClientPatchSnafu)?; + if let Some(existing_object) = api + .get_opt(&object.name_any()) + .await + .context(KubeClientFetchSnafu)? + { + object.metadata.resource_version = existing_object.resource_version(); + + match api + .patch( + &object.name_any(), + &PatchParams::apply("stackablectl"), + &Patch::Merge(object.clone()), + ) + .await + { + Ok(result) => result, + Err(e) => { + // If re-applying a Job fails due to immutability, print out the failed manifests instead of erroring out, + // so the user can decide if the existing Job needs a deletion and recreation + if resource.kind == *"Job" && e.to_string().contains("field is immutable") { + failed_manifests.push(format!( + "{kind} {object_name}", + kind = resource.kind, + object_name = object.name_any().clone() + )); + object + } else { + indicatif_println!( + "{object_name} {object:?}", + object_name = &object.name_any() + ); + return Err(e).context(KubeClientPatchSnafu); + } + } + }; + } else { + api.patch( + &object.name_any(), + &PatchParams::apply("stackablectl"), + &Patch::Apply(object.clone()), + ) + .await + .context(KubeClientPatchSnafu)?; + } + } + + if !failed_manifests.is_empty() { + indicatif_println!("Failed manifests due to immutability: {failed_manifests:?}"); } Ok(()) From 11930955c40cb1866a153eeb566fd50682dc1b52 Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 11:34:04 +0200 Subject: [PATCH 3/9] helm installs: uninstall and install chart instead of upgrade due to helmv3 force changes --- rust/helm-sys/go-helm-wrapper/main.go | 24 ----------------------- rust/helm-sys/src/lib.rs | 28 --------------------------- rust/stackable-cockpit/src/helm.rs | 4 +++- 3 files changed, 3 insertions(+), 53 deletions(-) diff --git a/rust/helm-sys/go-helm-wrapper/main.go b/rust/helm-sys/go-helm-wrapper/main.go index 182d80e9..b1fc05af 100644 --- a/rust/helm-sys/go-helm-wrapper/main.go +++ b/rust/helm-sys/go-helm-wrapper/main.go @@ -58,30 +58,6 @@ func go_install_helm_release(releaseName *C.char, chartName *C.char, chartVersio return C.CString("") } -//export go_upgrade_or_install_helm_release -func go_upgrade_or_install_helm_release(releaseName *C.char, chartName *C.char, chartVersion *C.char, valuesYaml *C.char, namespace *C.char, suppressOutput bool) *C.char { - helmClient := getHelmClient(namespace, suppressOutput) - - timeout, _ := time.ParseDuration("20m") - chartSpec := gohelm.ChartSpec{ - ReleaseName: C.GoString(releaseName), - ChartName: C.GoString(chartName), - Version: C.GoString(chartVersion), - ValuesYaml: C.GoString(valuesYaml), - Namespace: C.GoString(namespace), - UpgradeCRDs: true, - Wait: true, - Timeout: timeout, - Force: true, - } - - if _, err := helmClient.InstallOrUpgradeChart(context.Background(), &chartSpec, nil); err != nil { - return C.CString(fmt.Sprintf("%s%s", HELM_ERROR_PREFIX, err)) - } - - return C.CString("") -} - //export go_uninstall_helm_release func go_uninstall_helm_release(releaseName *C.char, namespace *C.char, suppressOutput bool) *C.char { helmClient := getHelmClient(namespace, suppressOutput) diff --git a/rust/helm-sys/src/lib.rs b/rust/helm-sys/src/lib.rs index b994ac6a..64f98965 100644 --- a/rust/helm-sys/src/lib.rs +++ b/rust/helm-sys/src/lib.rs @@ -37,34 +37,6 @@ pub fn install_helm_release( } } -pub fn upgrade_or_install_helm_release( - release_name: &str, - chart_name: &str, - chart_version: &str, - values_yaml: &str, - namespace: &str, - suppress_output: bool, -) -> String { - let release_name = CString::new(release_name).unwrap(); - let chart_name = CString::new(chart_name).unwrap(); - let chart_version = CString::new(chart_version).unwrap(); - let values_yaml = CString::new(values_yaml).unwrap(); - let namespace = CString::new(namespace).unwrap(); - - unsafe { - let c = go_upgrade_or_install_helm_release( - release_name.as_ptr() as *mut c_char, - chart_name.as_ptr() as *mut c_char, - chart_version.as_ptr() as *mut c_char, - values_yaml.as_ptr() as *mut c_char, - namespace.as_ptr() as *mut c_char, - suppress_output as u8, - ); - - cstr_ptr_to_string(c) - } -} - pub fn uninstall_helm_release( release_name: &str, namespace: &str, diff --git a/rust/stackable-cockpit/src/helm.rs b/rust/stackable-cockpit/src/helm.rs index 3048baa5..abc401d7 100644 --- a/rust/stackable-cockpit/src/helm.rs +++ b/rust/stackable-cockpit/src/helm.rs @@ -371,7 +371,9 @@ fn upgrade_release( namespace: &str, suppress_output: bool, ) -> Result<(), Error> { - let result = helm_sys::upgrade_or_install_helm_release( + uninstall_release(release_name, namespace, suppress_output)?; + + let result = helm_sys::install_helm_release( release_name, chart_name, chart_version, From d4691458099526018af8dfce8b3f2a1846f42dc7 Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 11:35:06 +0200 Subject: [PATCH 4/9] jobs: change failed job apply output --- rust/stackable-cockpit/src/utils/k8s/client.rs | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index e7bda5c0..16275d21 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -15,7 +15,7 @@ use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{commons::listener::Listener, kvp::Labels}; use tokio::sync::RwLock; use tracing::{Span, info, instrument}; -use tracing_indicatif::{indicatif_println, span_ext::IndicatifSpanExt as _}; +use tracing_indicatif::{indicatif_eprintln, indicatif_println, span_ext::IndicatifSpanExt as _}; #[cfg(doc)] use crate::utils::k8s::ListParamsExt; @@ -114,7 +114,6 @@ impl Client { // TODO (Techassi): Impl IntoIterator for Labels let labels: BTreeMap = labels.into(); - let mut failed_manifests = Vec::new(); for manifest in serde_yaml::Deserializer::from_str(manifests) { let mut object = DynamicObject::deserialize(manifest).context(DeserializeYamlSnafu)?; @@ -165,11 +164,11 @@ impl Client { // If re-applying a Job fails due to immutability, print out the failed manifests instead of erroring out, // so the user can decide if the existing Job needs a deletion and recreation if resource.kind == *"Job" && e.to_string().contains("field is immutable") { - failed_manifests.push(format!( - "{kind} {object_name}", + indicatif_eprintln!( + "Deploying {kind}/{object_name} manifest failed due to immutability", kind = resource.kind, object_name = object.name_any().clone() - )); + ); object } else { indicatif_println!( @@ -191,10 +190,6 @@ impl Client { } } - if !failed_manifests.is_empty() { - indicatif_println!("Failed manifests due to immutability: {failed_manifests:?}"); - } - Ok(()) } From 501d1124fd0ee5fc1d544f3c365138f2f96f690e Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 13:08:30 +0200 Subject: [PATCH 5/9] remove playaround printing --- rust/stackable-cockpit/src/utils/k8s/client.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index 16275d21..840d5b62 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -15,7 +15,7 @@ use snafu::{OptionExt, ResultExt, Snafu}; use stackable_operator::{commons::listener::Listener, kvp::Labels}; use tokio::sync::RwLock; use tracing::{Span, info, instrument}; -use tracing_indicatif::{indicatif_eprintln, indicatif_println, span_ext::IndicatifSpanExt as _}; +use tracing_indicatif::{indicatif_eprintln, span_ext::IndicatifSpanExt as _}; #[cfg(doc)] use crate::utils::k8s::ListParamsExt; @@ -171,10 +171,6 @@ impl Client { ); object } else { - indicatif_println!( - "{object_name} {object:?}", - object_name = &object.name_any() - ); return Err(e).context(KubeClientPatchSnafu); } } From f768989bf640b76eb527b317b7a7ab8429cfc242 Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 15:38:20 +0200 Subject: [PATCH 6/9] jobs: restructure code --- .../stackable-cockpit/src/utils/k8s/client.rs | 30 +++++++++---------- 1 file changed, 15 insertions(+), 15 deletions(-) diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index 840d5b62..2f3a266b 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -151,30 +151,30 @@ impl Client { { object.metadata.resource_version = existing_object.resource_version(); - match api + api .patch( &object.name_any(), &PatchParams::apply("stackablectl"), &Patch::Merge(object.clone()), ) .await - { - Ok(result) => result, - Err(e) => { + .or_else(|e| { // If re-applying a Job fails due to immutability, print out the failed manifests instead of erroring out, // so the user can decide if the existing Job needs a deletion and recreation - if resource.kind == *"Job" && e.to_string().contains("field is immutable") { - indicatif_eprintln!( - "Deploying {kind}/{object_name} manifest failed due to immutability", - kind = resource.kind, - object_name = object.name_any().clone() - ); - object - } else { - return Err(e).context(KubeClientPatchSnafu); + match (resource.kind.as_ref(), e) { + ("Job", kube::Error::Api(e)) if e.message.contains("field is immutable") => { + indicatif_eprintln!( + "Deploying {kind}/{object_name} manifest failed due to immutability", + kind = resource.kind, + object_name = object.name_any().clone() + ); + Ok(object) + } + (_, e) => { + Err(e).context(KubeClientPatchSnafu) + } } - } - }; + })?; } else { api.patch( &object.name_any(), From 81ba612fce0e99302db3be44bc949af6e5d30661 Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 15:58:37 +0200 Subject: [PATCH 7/9] helm installs: add comment about procedure, cleanup --- rust/stackable-cockpit/src/helm.rs | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/rust/stackable-cockpit/src/helm.rs b/rust/stackable-cockpit/src/helm.rs index abc401d7..76528584 100644 --- a/rust/stackable-cockpit/src/helm.rs +++ b/rust/stackable-cockpit/src/helm.rs @@ -371,6 +371,10 @@ fn upgrade_release( namespace: &str, suppress_output: bool, ) -> Result<(), Error> { + // In Helm 3 the behavior of the `--force` option has changed + // It no longer deletes and re-installs a resource https://github.com/helm/helm/issues/7082#issuecomment-559558318 + // Because of that, conflict errors might appear, which fail the upgrade, even if `helm upgrade --force` is used + // Therefore we uninstall the previous release (if present) and install the new one uninstall_release(release_name, namespace, suppress_output)?; let result = helm_sys::install_helm_release( @@ -383,9 +387,7 @@ fn upgrade_release( ); if let Some(error) = helm_sys::to_helm_error(&result) { - error!( - "Go wrapper function go_upgrade_or_install_helm_release encountered an error: {error}" - ); + error!("Go wrapper function go_install_helm_release encountered an error: {error}"); return Err(Error::UpgradeRelease { source: InstallReleaseError::HelmWrapper { error }, From f55d53f699539cee627f5f0f090bd61ec87c7dab Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 16:20:13 +0200 Subject: [PATCH 8/9] jobs: add comment for immutability error handling --- rust/stackable-cockpit/src/utils/k8s/client.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index 2f3a266b..c8c4863c 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -162,6 +162,8 @@ impl Client { // If re-applying a Job fails due to immutability, print out the failed manifests instead of erroring out, // so the user can decide if the existing Job needs a deletion and recreation match (resource.kind.as_ref(), e) { + // Errors for immutability in Kubernetes do not return meaningful `code`, `status`, or `reason` to filter on + // Currently we have to check the `message` for the actual error we are looking for ("Job", kube::Error::Api(e)) if e.message.contains("field is immutable") => { indicatif_eprintln!( "Deploying {kind}/{object_name} manifest failed due to immutability", From f63b77c4772d6edcfbe7e655201d5a85e18bf05f Mon Sep 17 00:00:00 2001 From: xeniape Date: Thu, 5 Jun 2025 16:27:46 +0200 Subject: [PATCH 9/9] add changelog entry --- rust/stackablectl/CHANGELOG.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/rust/stackablectl/CHANGELOG.md b/rust/stackablectl/CHANGELOG.md index aa4c6fce..f9f447c0 100644 --- a/rust/stackablectl/CHANGELOG.md +++ b/rust/stackablectl/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. ## [Unreleased] +### Fixed + +- Switch to idempotent Helm installations for demos and stacks ([#386]). +- Ignore failed re-application of Jobs due to immutability in demo and stack installations. + Display those manifests to the user, so they can decide if they need to delete and recreate it ([#386]). + +[#386]: https://github.com/stackabletech/stackable-cockpit/pull/386 + ## [1.0.0] - 2025-06-02 ### Added