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.

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

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

Techassi · 2026-03-25T08:50:15Z

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

-        parameters.insert("STACK".to_owned(), stack_name.into());
-        if let Some(demo_name) = demo_name {
-            parameters.insert("DEMO".to_owned(), demo_name.into());
-        }


question: Just that we are on the same track: These are now added earlier and are part of the passed parameters, right?

question: Out of curiosity, what was the reason you did that? I assume the demo and stack name are needed earlier in the call chain now?

I didn't add them in install_manifests because it seemed like this function is "outside of the scope of demos and stacks" and might be also used for something else. Along with the comment above that templating shouldn't be a concern here in the future. Since @sbernauer pulled that parameter addition from this PR while I was gone, I don't know what his thoughts were, we didn't coordinate. But I'm fine with also moving it back into install_manifests.

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.

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!

Techassi · 2026-03-27T13:40:09Z

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

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


note: We could potentially use deref here to save an allocation.

Suggested change

if value.eq(&label.value().to_string()) {

if value.eq(label.value().deref()) {

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).

Techassi · 2026-03-27T14:33:05Z

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

+                        },
+                    )?;
+                }
+                ManifestSpec::PlainYaml(_) => {}


Ah yeah, didn't even remember this ADR. I think the current approach is fine, but slightly harder to wrap your head around, because we delete different things in different places.

If we would handle deletions of manifests here, the "source of deletion" is more focused (at least that is my current impression). We could then finalize the deletion by deleting the namespace.

If we decide to keep it as is, a comment should definitely be added here.

Techassi · 2026-03-27T14:35:56Z

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

+                capability.supports_operation(discovery::verbs::LIST)
+                    && match namespace {
+                        Some(_) => capability.scope == kube::discovery::Scope::Namespaced,
+                        None => capability.scope == kube::discovery::Scope::Cluster,
+                    }


What do you think about renaming scope_matching to capability_scope_matches?

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

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

Open

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

	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(())
	}

	if value.eq(&label.value().to_string()) {
	if value.eq(label.value().deref()) {

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!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Uh oh!

Techassi left a comment

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 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!

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 27, 2026 •

edited

Loading