452: handle delete strategy for inoperable resources #468

kumaritanushree · 2022-03-25T07:21:56Z

Why this PR and what it is doing
Issue was: Inoperable resources like default namespace are not allowed to delete which was throwing error on kapp delete.
Solution: For all inoperable resources kapp will orphaned the requested resource instead of trying to delete.

TODO:

Manual testing for the changes
Add e2e test case

NOTE: If you feel there should be more resources in the list please suggest.

pkg/kapp/clusterapply/delete_change.go

…InoperableResource

vrabbi · 2022-04-03T12:05:12Z

i think this list should also include the namespaces:
kube-system
kube-public
kube-node-lease

these are all default namespaces in a cluster and as such should be treated i believe the same as the default namespace in terms of deletion strategy behavior

…, kube-public, kube-system

kumaritanushree · 2022-04-04T08:56:24Z

i think this list should also include the namespaces: kube-system kube-public kube-node-lease

these are all default namespaces in a cluster and as such should be treated i believe the same as the default namespace in terms of deletion strategy behavior

Updated the list as suggested. Thanks @vrabbi

renuy · 2022-04-04T09:01:03Z

pkg/kapp/clusterapply/delete_change.go

+	res := c.change.ExistingResource()
+
+	for _, r := range inoperableResourceList {
+		strings.EqualFold(r.Name, res.Name())


EqualFold do the non case-sensitive comparison of two strings. Which I found better than converting strings in one case and then compare.

If we have decide3d to go ahead with matching Group and Kind. We can change the values to have capital case (i.e. "Namespace") as that is always the case and go ahead with simply checking for equality

100mik

Just some thoughts 🤔
will take a closer look at the test as it is slightly diferent for what we have for the orphaning test.

100mik · 2022-04-04T09:28:37Z

pkg/kapp/clusterapply/delete_change.go

+	res := c.change.ExistingResource()
+
+	for _, r := range inoperableResourceList {
+		strings.EqualFold(r.Name, res.Name())


If we have decide3d to go ahead with matching Group and Kind. We can change the values to have capital case (i.e. "Namespace") as that is always the case and go ahead with simply checking for equality

100mik · 2022-04-04T09:35:33Z

pkg/kapp/clusterapply/delete_change.go

+	Name string
+}
+
+// if any new reource will encountered which kapp can not delete and required to orphaned then resource info will be added here.


nit: (just trying to make it a bit clearer)

Suggested change

// if any new reource will encountered which kapp can not delete and required to orphaned then resource info will be added here.

// List of resources use the "orphan" delete strategy implicitly as they cannot be deleted by end users

100mik · 2022-04-04T09:35:39Z

pkg/kapp/clusterapply/delete_change.go

@@ -32,10 +33,40 @@ type DeleteChange struct {
 	identifiedResources ctlres.IdentifiedResources
 }

+type uniqueResourceRef struct {


How about we change this to "inoperableResourceRef" as we are loosely matching it without being strict about versions now? It is also not being used anywhere else.
(Others might have other ideas as well)

Agree with the suggestion and updated the same. thanks

pkg/kapp/clusterapply/delete_change.go

100mik · 2022-04-04T09:37:36Z

pkg/kapp/clusterapply/delete_change.go

+
+	for _, r := range inoperableResourceList {
+		strings.EqualFold(r.Name, res.Name())
+		if strings.EqualFold(r.Name, res.Name()) && strings.EqualFold(r.Kind, res.GroupKind().Kind) && strings.EqualFold(r.Group, res.GroupKind().Group) {


As renu pointed out, I do not think we should do a match which ignores cases here if we have decided on using Group-Kinds to match as they are always the same.

100mik · 2022-04-04T09:38:08Z

test/e2e/delete_inoperable_test.go

+kind: Namespace
+metadata: 
+  name: default
+


nit: we do not need this empty line

100mik · 2022-04-04T09:44:26Z

The failing lint should be fixed by #470

rohitagg2020 · 2022-04-07T05:17:31Z

pkg/kapp/clusterapply/delete_change.go

@@ -122,3 +157,14 @@ func descMessage(res ctlres.Resource) []string {
 	}
 	return []string{}
 }
+
+func (c DeleteChange) isInoperableResource() bool {
+


nit: No need of new line

100mik

LGTM!

I see that there is an extra case (one more the orphaned test)
But I believe it addresses two cases, one where the GVR of the resource being deleted is available and the other when it is not.

I am fine with having this 🚀

pkg/kapp/clusterapply/delete_change.go

cppforlife · 2022-04-07T18:25:52Z

pkg/kapp/clusterapply/delete_change.go

+
+	res := c.change.ExistingResource()
+	for _, r := range inoperableResourceList {
+		if r.Name == res.Name() && r.Kind == res.GroupKind().Kind && r.Group == res.GroupKind().Group {


here is a trick:

if r == inoperableResourceRef{Name: res.Name(), GroupKind: res.GroupKind()} {

rohitagg2020 · 2022-04-11T04:54:26Z

LGTM

452: handle delete strategy for inoperable resources

43e303e

vmwclabot added the cla-not-required label Mar 25, 2022

kumaritanushree marked this pull request as draft March 25, 2022 07:22

kumaritanushree requested review from sethiyash, renuy, rohitagg2020, praveenrewar and 100mik March 25, 2022 07:27

100mik reviewed Mar 25, 2022

View reviewed changes

pkg/kapp/clusterapply/delete_change.go Outdated Show resolved Hide resolved

452: updated apply wait check for inoperable resource and scope of is…

f9913ac

…InoperableResource

praveenrewar linked an issue Mar 28, 2022 that may be closed by this pull request

Allow kapp delete even if default namespace was part of the application configuration #452

Closed

452: Added e2e test file, updated changes and comments

03309a3

452: updated inoperable resource list with namespaces kube-node-lease…

33c5f73

…, kube-public, kube-system

kumaritanushree marked this pull request as ready for review April 4, 2022 08:55

renuy reviewed Apr 4, 2022

View reviewed changes

100mik reviewed Apr 4, 2022

View reviewed changes

452: updated string comparasion and comments, removed empty lines

b1def8d

rohitagg2020 reviewed Apr 7, 2022

View reviewed changes

100mik approved these changes Apr 7, 2022

View reviewed changes

sethiyash reviewed Apr 7, 2022

View reviewed changes

pkg/kapp/clusterapply/delete_change.go Outdated Show resolved Hide resolved

cppforlife reviewed Apr 7, 2022

View reviewed changes

452: removed unwanted lines and condition check

8587e27

cppforlife merged commit 229c1cf into develop Apr 11, 2022

adduarte mentioned this pull request May 19, 2022

Metrics-server should not create or addopt kube-system namespace. vmware-tanzu/community-edition#4421

Closed

praveenrewar deleted the task/452-default-ns-delete branch March 28, 2024 10:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

452: handle delete strategy for inoperable resources #468

452: handle delete strategy for inoperable resources #468

kumaritanushree commented Mar 25, 2022 •

edited

Loading

vrabbi commented Apr 3, 2022

kumaritanushree commented Apr 4, 2022

renuy Apr 4, 2022

kumaritanushree Apr 4, 2022

100mik Apr 4, 2022

100mik left a comment

100mik Apr 4, 2022

100mik Apr 4, 2022

kumaritanushree Apr 6, 2022

100mik Apr 4, 2022

kumaritanushree Apr 6, 2022

100mik Apr 4, 2022

100mik Apr 4, 2022 •

edited

Loading

kumaritanushree Apr 6, 2022

100mik commented Apr 4, 2022

rohitagg2020 Apr 7, 2022

kumaritanushree Apr 9, 2022

100mik left a comment

cppforlife Apr 7, 2022

kumaritanushree Apr 9, 2022

rohitagg2020 commented Apr 11, 2022

	// if any new reource will encountered which kapp can not delete and required to orphaned then resource info will be added here.
	// List of resources use the "orphan" delete strategy implicitly as they cannot be deleted by end users

452: handle delete strategy for inoperable resources #468

452: handle delete strategy for inoperable resources #468

Conversation

kumaritanushree commented Mar 25, 2022 • edited Loading

vrabbi commented Apr 3, 2022

kumaritanushree commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100mik Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100mik commented Apr 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

100mik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rohitagg2020 commented Apr 11, 2022

kumaritanushree commented Mar 25, 2022 •

edited

Loading

100mik Apr 4, 2022 •

edited

Loading