Wait for all parent apps to be deleted before deleting children

When a project is deleted, we clean up all apps. The problem with this current implementation is that all apps will be deleted, regardless of whether it was a child app. This leads to child apps have issues deleting. This change will first ensure all "parent acorns" are deleted first, giving the child acorns time to delete. After all such acorns are deleted, then the remaining child acorns are also deleted. Signed-off-by: Donnie Adams <donnie@acorn.io>
acorn-io · Jul 25, 2023 · 1537619 · 1537619
1 parent 99fab6b
commit 1537619
Show file tree

Hide file tree

Showing 3 changed files with 164 additions and 10 deletions.
diff --git a/pkg/controller/routes.go b/pkg/controller/routes.go
@@ -87,7 +87,8 @@ func routes(router *router.Router, cfg *rest.Config, registryTransport http.Roun
 
 	projectRouter := router.Type(&v1.ProjectInstance{})
 	projectRouter.HandlerFunc(project.SetProjectSupportedRegions)
-	projectRouter.HandlerFunc(project.CreateNamespace)
+	// Don't delete the namespace until the project instance is deleted.
+	projectRouter.IncludeFinalizing().HandlerFunc(project.CreateNamespace)
 	projectRouter.FinalizeFunc(labels.Prefix+"project-app-delete", project.EnsureAllAppsRemoved)
 
 	router.Type(&v1.DevSessionInstance{}).HandlerFunc(devsession.ExpireDevSession)

diff --git a/pkg/project/handlers.go b/pkg/project/handlers.go
@@ -10,6 +10,8 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	klabels "k8s.io/apimachinery/pkg/labels"
+	"k8s.io/apimachinery/pkg/selection"
 	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -46,21 +48,35 @@ func CreateNamespace(req router.Request, resp router.Response) error {
 
 // EnsureAllAppsRemoved ensures that all apps are removed from the project before the namespace is deleted.
 func EnsureAllAppsRemoved(req router.Request, resp router.Response) error {
-	apps := new(v1.AppInstanceList)
-	if err := req.List(apps, &kclient.ListOptions{Namespace: req.Object.GetName()}); err != nil {
+	// A "child" app is one that has a parent label. Therefore, all "parent" apps won't have a parent label.
+	parentAppRequirement, err := klabels.NewRequirement(labels.AcornParentAcornName, selection.DoesNotExist, nil)
+	if err != nil {
 		return err
 	}
 
-	for _, app := range apps.Items {
-		if app.DeletionTimestamp.IsZero() {
-			if err := req.Client.Delete(req.Ctx, &app); err != nil && !apierrors.IsNotFound(err) {
-				return err
+	// First delete all "parent" apps. If no "parent" apps exist, then ensure that all apps are deleted.
+	for _, ls := range []klabels.Selector{klabels.NewSelector().Add(*parentAppRequirement), klabels.Everything()} {
+		apps := new(v1.AppInstanceList)
+		if err = req.List(apps, &kclient.ListOptions{
+			Namespace:     req.Object.GetName(),
+			LabelSelector: ls,
+		}); err != nil {
+			return err
+		}
+
+		for _, app := range apps.Items {
+			if app.DeletionTimestamp.IsZero() {
+				if err = req.Client.Delete(req.Ctx, &app); err != nil && !apierrors.IsNotFound(err) {
+					return err
+				}
 			}
 		}
-	}
 
-	if len(apps.Items) > 0 {
-		resp.RetryAfter(5 * time.Second)
+		if len(apps.Items) > 0 {
+			resp.RetryAfter(5 * time.Second)
+			return nil
+		}
 	}
+
 	return nil
 }
diff --git a/pkg/project/handlers_test.go b/pkg/project/handlers_test.go
@@ -1,10 +1,17 @@
 package project
 
 import (
+	"context"
 	"testing"
+	"time"
 
+	"github.com/acorn-io/baaah/pkg/router"
 	"github.com/acorn-io/baaah/pkg/router/tester"
+	v1 "github.com/acorn-io/runtime/pkg/apis/internal.acorn.io/v1"
 	"github.com/acorn-io/runtime/pkg/scheme"
+	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	kclient "sigs.k8s.io/controller-runtime/pkg/client"
 )
 
 func TestSetProjectSupportedRegions(t *testing.T) {
@@ -17,3 +24,133 @@ func TestCreateNamespace(t *testing.T) {
 	tester.DefaultTest(t, scheme.Scheme, "testdata/createnamespace/without-labels-anns", CreateNamespace)
 	tester.DefaultTest(t, scheme.Scheme, "testdata/createnamespace/with-labels-anns", CreateNamespace)
 }
+
+func TestEnsureAllAppsRemovedNoParents(t *testing.T) {
+	input := &v1.ProjectInstance{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-project",
+		},
+	}
+	existing := []kclient.Object{
+		&v1.AppInstance{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-app",
+				Namespace: "my-project",
+			},
+		},
+		&v1.AppInstance{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-other-app",
+				Namespace: "my-project",
+			},
+		},
+	}
+
+	c := &deleteClient{
+		Client: &tester.Client{
+			Objects:   append(existing, input.DeepCopyObject().(kclient.Object)),
+			SchemeObj: scheme.Scheme,
+		},
+	}
+	req := router.Request{
+		Client:      c,
+		Object:      input,
+		Ctx:         context.Background(),
+		GVK:         v1.SchemeGroupVersion.WithKind("ProjectInstance"),
+		Namespace:   input.GetNamespace(),
+		Name:        input.GetName(),
+		Key:         input.GetName(),
+		FromTrigger: false,
+	}
+
+	resp := new(tester.Response)
+	assert.NoError(t, EnsureAllAppsRemoved(req, resp))
+	assert.Len(t, c.deleted, 2)
+	assert.Equal(t, resp.Delay, 5*time.Second)
+}
+
+func TestEnsureAllParentAppsRemoved(t *testing.T) {
+	input := &v1.ProjectInstance{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-project",
+		},
+	}
+	existing := []kclient.Object{
+		&v1.AppInstance{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "my-app",
+				Namespace: "my-project",
+			},
+		},
+		&v1.AppInstance{
+			ObjectMeta: metav1.ObjectMeta{
+				Labels: map[string]string{
+					"acorn.io/parent-acorn-name": "my-app",
+				},
+				Name:      "my-other-app",
+				Namespace: "my-project",
+			},
+		},
+	}
+
+	c := &deleteClient{
+		Client: &tester.Client{
+			Objects:   append(existing, input.DeepCopyObject().(kclient.Object)),
+			SchemeObj: scheme.Scheme,
+		},
+	}
+	req := router.Request{
+		Client:      c,
+		Object:      input,
+		Ctx:         context.Background(),
+		GVK:         v1.SchemeGroupVersion.WithKind("ProjectInstance"),
+		Namespace:   input.GetNamespace(),
+		Name:        input.GetName(),
+		Key:         input.GetName(),
+		FromTrigger: false,
+	}
+
+	resp := new(tester.Response)
+	assert.NoError(t, EnsureAllAppsRemoved(req, resp))
+	assert.Len(t, c.deleted, 1)
+	assert.Equal(t, c.deleted[0].GetName(), "my-app")
+	assert.Equal(t, resp.Delay, 5*time.Second)
+}
+
+func TestEnsureAllAppsRemoved(t *testing.T) {
+	input := &v1.ProjectInstance{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "my-project",
+		},
+	}
+	c := &deleteClient{
+		Client: &tester.Client{
+			Objects:   []kclient.Object{input.DeepCopyObject().(kclient.Object)},
+			SchemeObj: scheme.Scheme,
+		},
+	}
+	req := router.Request{
+		Client:      c,
+		Object:      input,
+		Ctx:         context.Background(),
+		GVK:         v1.SchemeGroupVersion.WithKind("ProjectInstance"),
+		Namespace:   input.GetNamespace(),
+		Name:        input.GetName(),
+		Key:         input.GetName(),
+		FromTrigger: false,
+	}
+
+	resp := new(tester.Response)
+	assert.NoError(t, EnsureAllAppsRemoved(req, resp))
+	assert.Equal(t, resp.Delay, time.Duration(0))
+}
+
+type deleteClient struct {
+	*tester.Client
+	deleted []kclient.Object
+}
+
+func (c *deleteClient) Delete(_ context.Context, obj kclient.Object, _ ...kclient.DeleteOption) error {
+	c.deleted = append(c.deleted, obj)
+	return nil
+}