feat: Add uninstall demo/stack feature by xeniape · Pull Request #429 · stackabletech/stackable-cockpit

xeniape · 2026-03-12T13:46:33Z

Description

Part of #187

This PR adds the uninstall subcommands to demo and stack commands. It implements the MVP features mentioned here #187 (comment)

The flag of skipping operators and CRD deletion was optional and I wasn't sure about the naming or if needs to be one command or two. Can also be just removed, added it because it was not much additional work.

Tested with all current demos/stacks. Some demos have edge cases in the deletion, will document that in the parent issue.

~~This PR also contains the DEMO/STACK parameter addition for stackabletech/demos#374 (comment)~~ stripped out into #432

Definition of Done Checklist

Not all of these items are applicable to all PRs, the author should update this template to only leave the boxes in that are relevant
Please make sure all these things are done and tick the boxes

Author

Changes are OpenShift compatible
CRD changes approved
Helm chart can be installed and deployed operator works
Integration tests passed (for non trivial changes)

Reviewer

Code contains useful comments
(Integration-)Test cases added
Documentation added or updated
Changelog updated
Cargo.toml only contains references to git tags (not specific commits or branches)

Acceptance

Feature Tracker has been updated
Proper release label has been added

NickLarsenNZ · 2026-03-25T08:36:04Z

docs/modules/stackablectl/pages/commands/demo.adoc

+
+[NOTE]
+====
+The uninstall command deletes the namespace the demo was installed in. Therefore it is not possible to uninstall demos in the `default` namespace.


I feel like this should be opt-in behaviour. Or rather, if we created the namespace, it gets deleted.

There could be existing namespaces that shouldn't be deleted.

One such example is if we have the monitoring stack installed, and want to add and remove demos.

According to #187 (comment) "Update after a discussion on 2025-11-26:" it was decided the first version provides no option to be less destructive. Not sure which meeting that was or if it makes sense to change the requirements now.

There were also problems with deleting everything without deleting the namespace, see stackabletech/hdfs-operator#761 (comment). So maybe for the first version / MVP it is fine for now like this? But feel free to disagree

I left an idea below in how we could do it: #429 (comment)

But of course, if this implements what we agreed on, that's fine.
It does feel a little dangerous though.

Yes, I agree it feels dangerous, tried to avoid it at first but ran into the problems above, which unfortunately won't be solved by your idea I think. The manifests we would delete with that approach were also deleted with the help of the labels, so would be the same situation, unless I misunderstood something 😞

Another thing might be adding a confirmation dialogue like the one I added to the installation for the namespace. That it asks if the user really wants to uninstall since that would delete the namespace, and cancel the deletion otherwise.

0f3cf7f (this PR) Maybe this addresses your concern a bit, let me know what you think :)

NickLarsenNZ · 2026-03-25T08:37:55Z

docs/modules/stackablectl/pages/commands/stack.adoc

+
+[NOTE]
+====
+The uninstall command deletes the namespace the stack was installed in. Therefore it is not possible to uninstall stacks in the `default` namespace.


Same as my comment about demos.

NickLarsenNZ

Left a couple of comments about namespace deletion.

NickLarsenNZ · 2026-03-25T08:42:19Z

rust/stackable-cockpit/src/platform/demo/spec.rs

+        // Delete demo namespace
+        client
+            .delete_object(
+                &uninstall_parameters.demo_namespace,
+                &ApiResource::from_gvk(&GroupVersionKind {
+                    group: "".to_owned(),
+                    version: "v1".to_owned(),
+                    kind: "Namespace".to_owned(),
+                }),
+                None,
+            )
+            .await
+            .context(DeleteObjectSnafu)?;


Here's where I think we should have a check to see if the namespace was "owned" by the demo (via labels/annotations).

One thing we could consider is reading in the demo, get the GVKs and Helm charts, and uninstall in reverse order before uninstalling the namespace (if it was created by the demo).

Same would apply for stacks.

That could be a follow-up PR, but I think at least we should be careful with namespace deletion.

NickLarsenNZ · 2026-03-25T08:45:58Z

rust/stackable-cockpit/src/platform/stack/spec.rs

+        // Delete stack namespace
+        client
+            .delete_object(
+                &uninstall_parameters.stack_namespace,
+                &ApiResource::from_gvk(&GroupVersionKind {
+                    group: "".to_owned(),
+                    version: "v1".to_owned(),
+                    kind: "Namespace".to_owned(),
+                }),
+                None,
+            )
+            .await
+            .context(DeleteObjectSnafu)?;


Techassi

Left a few comments and questions.

rust/stackable-cockpit/src/platform/demo/spec.rs

Techassi · 2026-03-25T08:39:56Z

rust/stackable-cockpit/src/platform/demo/spec.rs

+        // Delete demo namespace
+        client
+            .delete_object(
+                &uninstall_parameters.demo_namespace,
+                &ApiResource::from_gvk(&GroupVersionKind {
+                    group: "".to_owned(),
+                    version: "v1".to_owned(),
+                    kind: "Namespace".to_owned(),
+                }),
+                None,
+            )
+            .await
+            .context(DeleteObjectSnafu)?;


note: I think we should abstract that away in an associated function called delete_namespace similar to what we have to create a namespace. See:

stackable-cockpit/rust/stackable-cockpit/src/utils/k8s/client.rs

Lines 443 to 460 in 32cf847

pub async fn create_namespace(&self, name: String) -> Result<()> {

let namespace_api: Api<Namespace> = Api::all(self.client.clone());

namespace_api

.create(

&PostParams::default(),

&Namespace {

metadata: ObjectMeta {

name: Some(name),

..Default::default()

},

..Default::default()

},

)

.await

.context(KubeClientPatchSnafu)?;

Ok(())

}

Sounds good, thanks! e71437a (this PR)

Any particular reason why we are not using Api<Namespace> instead? I guess one upside to to doing it like currently implemented is that we don't need to clone the client for a single function call.

The client is cloned anyway in the delete_object function called by delete_namespace. I think I didn't use Api<Namespace> was because I wanted to reuse the delete_object function (before everything was moved into delete_namespace). Could possibly also rewrite that one to use Api<DynamicObject> instead of ApiResource. I don't remember exactly anymore, but I think the resulting code just looked better using ApiResource and that's what the discovery already provided. But no strong opinions here.

While writing, tried implementing it since then the Api creation could be moved from delete_object to delete_all_object_with_label, so one client clone per object list, instead of object function call. This doc for DynamicObject:

/// A dynamic representation of a kubernetes object
///
/// This will work with any non-list type object.

made me think, that if a function accepts Api<DynamicObject>, I could pass it a Api<Namespace> (or any other Object type). But seems to be not compatible. So I think I'm abandoning that implementation again.

Could also not use delete_object instead, but then I would just re-implement what's in that function anyway.

Just noting down thoughts here while trying it out, not trying to argue against it. Here the non-working diff because of Api<DynamicObject> <-> Api<Namespace> incompatibility.

diff --git a/rust/stackable-cockpit/src/utils/k8s/client.rs b/rust/stackable-cockpit/src/utils/k8s/client.rs index 79f5338..779ec7f 100644 --- a/rust/stackable-cockpit/src/utils/k8s/client.rs +++ b/rust/stackable-cockpit/src/utils/k8s/client.rs @@ -358,13 +358,20 @@ impl Client { continue; }; + let object_api = match namespace { + Some(namespace) => { + Api::<DynamicObject>::namespaced_with(self.client.clone(), namespace, &api_resource) + } + None => Api::<DynamicObject>::all_with(self.client.clone(), &api_resource), + }; + for object in object_list { if let Some(value) = object.labels().get(&label.key().to_string()) { if value.eq(&label.value().to_string()) { self.delete_object( &object.metadata.name.unwrap(), - &api_resource, - object.metadata.namespace.as_deref(), + &api_resource.kind, + object_api.clone(), ) .await?; } @@ -379,17 +386,10 @@ impl Client { pub async fn delete_object( &self, object_name: &str, - api_resource: &ApiResource, - namespace: Option<&str>, + object_kind: &str, + object_api: Api<DynamicObject>, ) -> Result<(), Error> { - Span::current().pb_set_message(&format!("Deleting {} {}", api_resource.kind, object_name)); - - let object_api = match namespace { - Some(namespace) => { - Api::<DynamicObject>::namespaced_with(self.client.clone(), namespace, api_resource) - } - None => Api::<DynamicObject>::all_with(self.client.clone(), api_resource), - }; + Span::current().pb_set_message(&format!("Deleting {} {}", object_kind, object_name)); kube::runtime::wait::delete::delete_and_finalize( object_api, @@ -574,14 +574,11 @@ impl Client { /// Deletes a [`Namespace`] with `name` in the cluster. pub async fn delete_namespace(&self, name: String) -> Result<()> { + let namespace_api: Api<Namespace> = Api::all(self.client.clone()); self.delete_object( &name, - &ApiResource::from_gvk(&GroupVersionKind { - group: "".to_owned(), - version: "v1".to_owned(), - kind: "Namespace".to_owned(), - }), - None, + "Namespace", + namespace_api, ) .await }

The client is cloned anyway in the delete_object function called by delete_namespace.

Ah I see. Then I think we should use the same style as in create_namespace via Api.

rust/stackable-cockpit/src/platform/demo/spec.rs

rust/stackable-cockpit/src/platform/stack/spec.rs

rust/stackable-cockpit/src/platform/manifests.rs

rust/stackable-cockpit/src/utils/k8s/client.rs

Techassi · 2026-03-25T09:08:28Z

rust/stackablectl/src/cmds/demo.rs

+    let demo_namespace = tracing_indicatif::suspend_tracing_indicatif(
+        || -> Result<String, CmdError> {
+            if args.namespaces.namespace == DEFAULT_NAMESPACE {
+                if Confirm::new()
+                .with_prompt(
+                    format!(
+                        "Demos installed in the '{DEFAULT_NAMESPACE}' namespace cannot be uninstalled with stackablectl. Install the demo in the '{demo_namespace}' namespace instead?",
+                    demo_namespace = args.demo_name.clone())
+                )
+                .default(true)
+                .interact()
+                .context(ConfirmDialogSnafu)? {
+                    // User selected to install in suggested namespace
+                    Ok(args.demo_name.clone())
+                } else {
+                    // User selected to install in default namespace
+                    Ok(args.namespaces.namespace.clone())
+                }
+            } else {
+                Ok(args.namespaces.namespace.clone())
+            }
+        },
+    )?;


note: Well that's a mouthful to read. Can we somehow make this easier to grasp?

Maybe I looked at the code too long at this point and unable to see it, but could you give me more information on what is making it hard to grasp. At least I currently don't know how to make it shorter. Or are there explanations missing somewhere? Or just the whole "formatting" of it?

0f3cf7f (this PR) Tried rearranging it a bit, let me know if know if that makes it any better

I don't feel like it does make it better, sorry 🙈

What might help on the other hand is to define the closure outside of the tracing_indicatif::suspend_tracing_indicatif call.

If that also doesn't help, we can leave it as is.

18e7f70 (this PR)

I already like this way better. I think we can now also switch to the previous style of assigning demo_namespace which should make it even cleaner.

rust/stackablectl/src/cmds/demo.rs

This reverts commit 7dfe50e.

Co-authored-by: Techassi <git@techassi.dev>

Techassi

Only a few small comments left, mostly looks nice!

rust/stackable-cockpit/src/utils/k8s/client.rs

Techassi · 2026-03-27T14:27:36Z

rust/stackable-cockpit/src/utils/k8s/client.rs

+            };
+
+            for object in object_list {
+                if let Some(value) = object.labels().get(&label.key().to_string()) {


note: This is kinda cumbersome to use, but could be improved by stackabletech/operator-rs#1182 (if we decide to move forward with it).

The linked PR was merged btw, but not yet released.

rust/stackable-cockpit/src/platform/manifests.rs

rust/stackable-cockpit/src/utils/k8s/client.rs

Co-authored-by: Techassi <git@techassi.dev>

feat: Add uninstall demo/stack feature

c9aec77

xeniape self-assigned this Mar 12, 2026

xeniape added 2 commits March 12, 2026 14:55

Merge branch 'main' into feat/add-demo-stack-deletion

d8476f3

changelog, some fixes after merge, cargo clippy

5b366b4

This was referenced Mar 12, 2026

fix: Enable Demo/Stack installations in non-default namespaces stackabletech/demos#374

Merged

Implement option to uninstall Stacks and Demos #187

Open

xeniape added 3 commits March 12, 2026 15:33

pre-commit fixes

d6dc693

pre-commit fix

f80624a

regenerate-nix

1066138

xeniape added this to Stackable Engineering Mar 12, 2026

xeniape moved this to Development: Waiting for Review in Stackable Engineering Mar 12, 2026

sbernauer mentioned this pull request Mar 17, 2026

feat: Add STACK and DEMO templating parameters #432

Merged

11 tasks

xeniape added 3 commits March 23, 2026 15:31

Merge branch 'main' into feat/add-demo-stack-deletion

73515e2

adjustments after main merge

0e0658f

further uninstall adjustments

9f9b923

Techassi self-requested a review March 23, 2026 15:32

xeniape added 3 commits March 23, 2026 19:30

small word fix

7da2228

cargo update

70fd483

add documentation

c187cff

NickLarsenNZ moved this from Development: Waiting for Review to Development: In Review in Stackable Engineering Mar 25, 2026

NickLarsenNZ reviewed Mar 25, 2026

View reviewed changes

Techassi reviewed Mar 25, 2026

View reviewed changes

xeniape added 6 commits March 25, 2026 14:47

delete namespace function

e71437a

use Label struct for label

4d1ff73

rename function

6d0edef

code readability

2367a88

reduce nesting

1a00526

delete object refactoring

043f0f1

xeniape and others added 5 commits March 26, 2026 11:38

small docs adjustment

7dfe50e

Revert "small docs adjustment"

d794c7e

This reverts commit 7dfe50e.

Update rust/stackablectl/src/cmds/demo.rs

5574319

Co-authored-by: Techassi <git@techassi.dev>

stackspec in uninstall refactoring

1bc08a2

confirm dialog for canceling delete, refactor install confim dialog

0f3cf7f

xeniape requested review from NickLarsenNZ and Techassi March 26, 2026 13:06

Techassi reviewed Mar 27, 2026

View reviewed changes

xeniape added 4 commits March 30, 2026 14:13

function comment for helm uninstall

506fbcf

rename variable

a6e2feb

refactor namespace confirmation

18e7f70

add stack parameter on demo uninstall

ad7dd15

xeniape requested a review from Techassi March 30, 2026 13:31

xeniape and others added 2 commits March 30, 2026 16:34

Update rust/stackable-cockpit/src/utils/k8s/client.rs

ba64478

Co-authored-by: Techassi <git@techassi.dev>

import deref

a2fbb1c

	pub async fn create_namespace(&self, name: String) -> Result<()> {
	let namespace_api: Api<Namespace> = Api::all(self.client.clone());
	namespace_api
	.create(
	&PostParams::default(),
	&Namespace {
	metadata: ObjectMeta {
	name: Some(name),
	..Default::default()
	},
	..Default::default()
	},
	)
	.await
	.context(KubeClientPatchSnafu)?;

	Ok(())
	}

Uh oh!

Conversation

xeniape commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Definition of Done Checklist

Author

Reviewer

Acceptance

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

xeniape Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickLarsenNZ left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

NickLarsenNZ Mar 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Techassi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Techassi Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Techassi Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Techassi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

xeniape commented Mar 12, 2026 •

edited

Loading

xeniape Mar 25, 2026 •

edited

Loading

NickLarsenNZ Mar 25, 2026 •

edited

Loading

Techassi Mar 27, 2026 •

edited

Loading

Techassi Mar 31, 2026 •

edited

Loading