refactor(cmd/cli): update uninstall cmd

[shalier]

Description:
Updates uninstall mesh command so that user is prompted on whether they want to uninstall an existing mesh. Also, adds a warning for delete-cluster-wide-resources if there are remaining meshes in the cluster, before deleting cluster resources. Resolves #4613

Signed-off-by: Shalier Xia [email protected]

Testing done:
Uninstall mesh now prompts with existing mesh:

Can also uninstall specific mesh with mesh-name flag:

Informs when mesh doesn't exist (including when trying to force uninstall):

delete-namespace flag:

delete-cluster-wide-resources flag:

warning when deleting cluster resources and there are still meshes in the cluster (updated):

force flag uninstalls existing mesh [only one mesh in cluster]:

force flag uninstalls the default mesh+namespace (osm+osm-system) unless mesh-name and osm-namespace are specified [multiple meshes]:

Affected area:

Functional Area
New Functionality	[ ]
CI System	[ ]
CLI Tool	[ x]

Please answer the following questions with yes/no.

1. Does this change contain code from or inspired by another project? no

   • Did you notify the maintainers and provide attribution?

2. Is this a breaking change? no

3. Has documentation corresponding to this change been updated in the [osm-docs] (https://github.com/openservicemesh/osm-docs) repo (if applicable)?

[codecov-commenter]

Codecov Report

Merging #4664 (a50e1bc) into main (a8330dc) will increase coverage by 0.00%.
The diff coverage is 72.80%.

@@           Coverage Diff           @@
##             main    #4664   +/-   ##
=======================================
  Coverage   69.34%   69.34%           
=======================================
  Files         219      219           
  Lines       15642    15711   +69     
=======================================
+ Hits        10847    10895   +48     
- Misses       4745     4766   +21     
  Partials       50       50           

Flag	Coverage Δ
unittests	69.34% <72.80%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/cli/uninstall_mesh.go	69.27% <72.80%> (+1.27%)	⬆️
pkg/messaging/workqueue.go	89.28% <0.00%> (-10.72%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a8330dc...a50e1bc. Read the comment docs.

[nojnhuh]

I think aligning logic more closely with how it was before is slightly better since then the error is still shown when users pass --delete-cluster-wide-resources or --delete-namespace.

Also, is there a reason why we've only been looking for "not found" errors here? Even with how it was before it looks like if uninstalling failed for any other reason, users would get zero feedback on what that reason was. e.g. I made the following changes to make the post-delete hook fail and print the error we get back from this:

diff --git a/charts/osm/templates/cleanup-hook.yaml b/charts/osm/templates/cleanup-hook.yaml
index 0760004f2..a4005b6e5 100644
--- a/charts/osm/templates/cleanup-hook.yaml
+++ b/charts/osm/templates/cleanup-hook.yaml
@@ -63,6 +63,7 @@ metadata:
     helm.sh/hook: post-delete
     helm.sh/hook-delete-policy: before-hook-creation,hook-succeeded
 spec:
+  backoffLimit: 0
   template:
     metadata:
       name: {{ .Release.Name }}-cleanup
@@ -81,6 +82,7 @@ spec:
             - sh
             - -c
             - >
+             exit 1;
              kubectl replace -f /osm-crds;
              kubectl delete --ignore-not-found meshconfig -n '{{ include "osm.namespace" . }}' osm-mesh-config;
              kubectl delete --ignore-not-found secret -n '{{ include "osm.namespace" . }}' {{ .Values.osm.caBundleSecretName }};
diff --git a/cmd/cli/uninstall_mesh.go b/cmd/cli/uninstall_mesh.go
index 2f42a4d1d..48f999131 100644
--- a/cmd/cli/uninstall_mesh.go
+++ b/cmd/cli/uninstall_mesh.go
@@ -171,6 +171,8 @@ func (d *uninstallMeshCmd) run() error {
 			}
 			if err == nil {
 				fmt.Fprintf(d.out, "OSM [mesh name: %s] in namespace [%s] uninstalled\n", m.name, m.namespace)
+			} else {
+				fmt.Fprintf(d.out, "swallowed error: %v\n", err)
 			}
 
 			if d.deleteNamespace {

The only output produced when the mesh is uninstalled is:

% bin/osm uninstall mesh -f
swallowed error: uninstallation completed with 1 error(s): job failed: BackoffLimitExceeded

which indicates there's no actual output at all currently even though it failed.

[shalier]

Changed it to:

if err != nil {
	if errors.Cause(err) == helmStorage.ErrReleaseNotFound {
		fmt.Fprintf(d.out, "No OSM control plane with mesh name [%s] found in namespace [%s]\n", m.name, m.namespace)
	}

	if !d.deleteClusterWideResources && !d.deleteNamespace {
		return err
	}

[jaellio]

Should the user be prompted to confirm deleting the cluster-wide resources if another mesh is present? Should we give them the option to abort unless they have set "force"?

[shalier]

@jaellio atm, we don't support multiple meshes in the cluster, but if in the future we do choose to support multiple meshes then I think we should

[nojnhuh]

Is there a reason we wouldn't also want to check that the mesh exists if the flags match the default mesh?

And should there be a difference between not passing --mesh-name at all and passing --mesh-name osm (the default)? This check deals with both the same way.

[nojnhuh]

It looks like this is still silently passing over errors other than "not found" when the --delete-cluster-wide-resources or --delete-namespace flags are passed.

[nojnhuh]

Let's stick with any new checks in these tests using testify instead. Then the RegistryFailHandler line above can be removed too.