From 9e9d4cc17c85c0809165b4e6f4503cb31fbdf29e Mon Sep 17 00:00:00 2001 From: jlandowner Date: Wed, 21 Jun 2023 13:14:01 +0900 Subject: [PATCH 1/2] add gc for user addon --- .../__snapshots__/user_controller_test.snap | 39 +----- internal/controllers/user_controller.go | 120 ++++++++++------- internal/controllers/user_controller_test.go | 127 +++++++++++++----- 3 files changed, 168 insertions(+), 118 deletions(-) diff --git a/internal/controllers/__snapshots__/user_controller_test.snap b/internal/controllers/__snapshots__/user_controller_test.snap index 2b40c59a..3746ab7d 100644 --- a/internal/controllers/__snapshots__/user_controller_test.snap +++ b/internal/controllers/__snapshots__/user_controller_test.snap @@ -35,6 +35,7 @@ SnapShot = """ \"addons\": [ { \"kind\": \"Instance\", + \"namespace\": \"cosmo-user-ua\", \"name\": \"useraddon-namespaced-addon\", \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\" }, @@ -48,7 +49,7 @@ SnapShot = """ } """ -['User controller when updating user addon with invalid addon should try to create addon but status AddonFailed 1'] +['User controller when updating user addon should gc old addon and try to create new addon 1'] SnapShot = """ { \"metadata\": { @@ -64,13 +65,7 @@ SnapShot = """ \"name\": \"namespaced-addon\" }, \"vars\": { - \"IMAGE_TAG\": \"v0.71.0\" - } - }, - { - \"template\": { - \"name\": \"cluster-addon\", - \"clusterScoped\": true + \"KEY\": \"VAL\" } }, { @@ -90,6 +85,7 @@ SnapShot = """ \"addons\": [ { \"kind\": \"Instance\", + \"namespace\": \"cosmo-user-ua\", \"name\": \"useraddon-namespaced-addon\", \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\" }, @@ -103,7 +99,7 @@ SnapShot = """ } """ -['User controller when updating user addon with invalid addon should try to create addon but status AddonFailed 2'] +['User controller when updating user addon should gc old namespaced addon 1'] SnapShot = """ { \"metadata\": { @@ -114,20 +110,6 @@ SnapShot = """ \"displayName\": \"お名前\", \"authType\": \"password-secret\", \"addons\": [ - { - \"template\": { - \"name\": \"namespaced-addon\" - }, - \"vars\": { - \"IMAGE_TAG\": \"v0.71.0\" - } - }, - { - \"template\": { - \"name\": \"cluster-addon\", - \"clusterScoped\": true - } - }, { \"template\": { \"name\": \"empty-addon\" @@ -145,18 +127,9 @@ SnapShot = """ \"addons\": [ { \"kind\": \"Instance\", + \"namespace\": \"cosmo-user-ua\", \"name\": \"useraddon-empty-addon\", \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\" - }, - { - \"kind\": \"Instance\", - \"name\": \"useraddon-namespaced-addon\", - \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\" - }, - { - \"kind\": \"ClusterInstance\", - \"name\": \"useraddon-ua-cluster-addon\", - \"apiVersion\": \"cosmo-workspace.github.io/v1alpha1\" } ] } diff --git a/internal/controllers/user_controller.go b/internal/controllers/user_controller.go index 0448482c..8579b9f2 100644 --- a/internal/controllers/user_controller.go +++ b/internal/controllers/user_controller.go @@ -7,7 +7,10 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" + apierrs "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" "k8s.io/client-go/tools/record" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" @@ -74,15 +77,6 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. CreationTimestamp: &ns.CreationTimestamp, } - // update user status - if !equality.Semantic.DeepEqual(currentUser, &user) { - log.Debug().PrintObjectDiff(currentUser, &user) - if err := r.Status().Update(ctx, &user); err != nil { - return ctrl.Result{}, err - } - log.Info("status updated") - } - if user.Spec.AuthType == cosmov1alpha1.UserAuthTypePasswordSecert { // generate default password if password secret is not found if _, err := password.GetDefaultPassword(ctx, r.Client, user.Name); err != nil && apierrors.IsNotFound(err) { @@ -96,53 +90,59 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. } // reconcile user addon - if len(user.Spec.Addons) > 0 { - errs := make([]error, 0) + addonErrs := make([]error, 0) - addonStatusMap := sliceToObjectMap(user.Status.Addons) - for _, addon := range user.Spec.Addons { - log.Info("syncing user addon", "addon", addon) + lastAddons := make([]cosmov1alpha1.ObjectRef, len(user.Status.Addons)) + copy(lastAddons, user.Status.Addons) - inst := useraddon.EmptyInstanceObject(addon, user.GetName()) - if inst == nil { - log.Info("WARNING: addon has no Template or ClusterTemplate", "addon", addon) - continue - } + currAddonsMap := make(map[types.UID]cosmov1alpha1.ObjectRef) + for _, addon := range user.Spec.Addons { + log.Info("syncing user addon", "addon", addon) - _, err := kubeutil.CreateOrUpdate(ctx, r.Client, inst, func() error { - return useraddon.PatchUserAddonInstanceAsDesired(inst, addon, user, r.Scheme) - }) - if err != nil { - errs = append(errs, fmt.Errorf("failed to create or update addon %s :%w", inst.GetSpec().Template.Name, err)) - continue - } + inst := useraddon.EmptyInstanceObject(addon, user.GetName()) + if inst == nil { + log.Info("WARNING: addon has no Template or ClusterTemplate", "addon", addon) + continue + } - ct := inst.GetCreationTimestamp() - gvk, err := apiutil.GVKForObject(inst, r.Scheme) - if err != nil { - errs = append(errs, fmt.Errorf("failed to recognize addon instance GVK %s :%w", inst.GetSpec().Template.Name, err)) - continue - } - addonStatusMap[inst.GetUID()] = cosmov1alpha1.ObjectRef{ - ObjectReference: corev1.ObjectReference{ - APIVersion: gvk.GroupVersion().String(), - Kind: gvk.Kind, - Name: inst.GetName(), - UID: inst.GetUID(), - ResourceVersion: inst.GetResourceVersion(), - }, - CreationTimestamp: &ct, - } + op, err := kubeutil.CreateOrUpdate(ctx, r.Client, inst, func() error { + return useraddon.PatchUserAddonInstanceAsDesired(inst, addon, user, r.Scheme) + }) + if err != nil { + addonErrs = append(addonErrs, fmt.Errorf("failed to create or update addon %s :%w", inst.GetSpec().Template.Name, err)) + continue } - user.Status.Addons = objectRefMapToSlice(addonStatusMap) - if len(errs) > 0 { - for _, e := range errs { - r.Recorder.Eventf(&user, corev1.EventTypeWarning, "AddonFailed", "failed to create or update user addon: %v", e) - log.Error(e, "failed to create or update user addon") - } - user.Status.Phase = "AddonFailed" + if op != controllerutil.OperationResultNone { + r.Recorder.Eventf(&user, corev1.EventTypeNormal, "Addon Synced", fmt.Sprintf("addon %s is %s", addon.Template.Name, op)) + } + + ct := inst.GetCreationTimestamp() + gvk, err := apiutil.GVKForObject(inst, r.Scheme) + if err != nil { + addonErrs = append(addonErrs, fmt.Errorf("failed to recognize addon instance GVK %s :%w", inst.GetSpec().Template.Name, err)) + continue + } + currAddonsMap[inst.GetUID()] = cosmov1alpha1.ObjectRef{ + ObjectReference: corev1.ObjectReference{ + APIVersion: gvk.GroupVersion().String(), + Kind: gvk.Kind, + Name: inst.GetName(), + Namespace: inst.GetNamespace(), + UID: inst.GetUID(), + ResourceVersion: inst.GetResourceVersion(), + }, + CreationTimestamp: &ct, + } + } + user.Status.Addons = objectRefMapToSlice(currAddonsMap) + + if len(addonErrs) > 0 { + for _, e := range addonErrs { + r.Recorder.Eventf(&user, corev1.EventTypeWarning, "AddonFailed", "failed to create or update user addon: %v", e) + log.Error(e, "failed to create or update user addon") } + user.Status.Phase = "AddonFailed" } // update user status @@ -154,6 +154,28 @@ func (r *UserReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl. log.Info("status updated") } + // garbage collection + shouldDeletes := objectRefNotExistsInMap(lastAddons, currAddonsMap) + for _, d := range shouldDeletes { + log.Info("start garbage collection", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) + + var obj unstructured.Unstructured + obj.SetAPIVersion(d.APIVersion) + obj.SetKind(d.Kind) + err := r.Get(ctx, types.NamespacedName{Name: d.GetName(), Namespace: d.Namespace}, &obj) + if err != nil { + if !apierrs.IsNotFound(err) { + log.Error(err, "failed to get object to be deleted", "apiVersion", d.APIVersion, "kind", d.Kind, "name", d.Name) + } + continue + } + + if err := r.Delete(ctx, &obj); err != nil { + r.Recorder.Eventf(&user, corev1.EventTypeWarning, "GCFailed", "failed to delete unused addon: %s %s", obj.GetKind(), obj.GetName()) + } + r.Recorder.Eventf(&user, corev1.EventTypeNormal, "GC", "deleted unmanaged addon: %s %s", obj.GetKind(), obj.GetName()) + } + log.Debug().Info("finish reconcile") return ctrl.Result{}, err } diff --git a/internal/controllers/user_controller_test.go b/internal/controllers/user_controller_test.go index 59dc86f7..8689c1b2 100644 --- a/internal/controllers/user_controller_test.go +++ b/internal/controllers/user_controller_test.go @@ -13,6 +13,7 @@ import ( corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/scheme" "sigs.k8s.io/controller-runtime/pkg/client" @@ -108,31 +109,29 @@ spec: }, } - Context("when creating User resource", func() { - - BeforeEach(func() { - ctx := context.Background() + BeforeEach(func() { + ctx := context.Background() - By("creating template") - addon := namespacedUserAddon.DeepCopy() - err := k8sClient.Create(ctx, addon) - Expect(err).ShouldNot(HaveOccurred()) - - clusterAddon := clusterUserAddon.DeepCopy() - err = k8sClient.Create(ctx, clusterAddon) - Expect(err).ShouldNot(HaveOccurred()) - }) + By("creating template") + err := k8sClient.Create(ctx, namespacedUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + err = k8sClient.Create(ctx, clusterUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + err = k8sClient.Create(ctx, emptyUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + }) - AfterEach(func() { - By("delete template") - addon := namespacedUserAddon.DeepCopy() - err := k8sClient.Delete(ctx, addon) - Expect(err).ShouldNot(HaveOccurred()) - clusterAddon := clusterUserAddon.DeepCopy() - err = k8sClient.Delete(ctx, clusterAddon) - Expect(err).ShouldNot(HaveOccurred()) - }) + AfterEach(func() { + By("delete template") + err := k8sClient.Delete(ctx, namespacedUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + err = k8sClient.Delete(ctx, clusterUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + err = k8sClient.Delete(ctx, emptyUserAddon.DeepCopy()) + Expect(err).ShouldNot(HaveOccurred()) + }) + Context("when creating User resource", func() { It("should do create namespace, password and addons", func() { By("creating user") @@ -371,37 +370,93 @@ spec: }) }) - Context("when updating user addon with invalid addon", func() { - It("should try to create addon but status AddonFailed", func() { + Context("when updating user addon", func() { + It("should gc old addon and try to create new addon", func() { ctx := context.Background() - By("creating invalid template") + By("fetching and update user") + var user cosmov1alpha1.User + Eventually(func() error { + err := k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &user) + Expect(err).ShouldNot(HaveOccurred()) - err := k8sClient.Create(ctx, &emptyUserAddon) - Expect(err).ShouldNot(HaveOccurred()) + var ci cosmov1alpha1.ClusterInstance + err = k8sClient.Get(ctx, client.ObjectKey{Name: useraddon.InstanceName(clusterUserAddon.Name, user.GetName())}, &ci) + Expect(err).ShouldNot(HaveOccurred()) - By("fetching and update user") + user.Spec.Addons = []cosmov1alpha1.UserAddon{ + { + Template: cosmov1alpha1.UserAddonTemplateRef{ + Name: namespacedUserAddon.Name, + }, + Vars: map[string]string{ + "KEY": "VAL", + }, + }, + { + Template: cosmov1alpha1.UserAddonTemplateRef{ + Name: emptyUserAddon.Name, + }, + }, + } + return k8sClient.Update(ctx, &user) + }, time.Second*30).Should(Succeed()) + + var updatedUser cosmov1alpha1.User + Eventually(func() int { + err := k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &updatedUser) + Expect(err).ShouldNot(HaveOccurred()) + return len(updatedUser.Status.Addons) + }, time.Second*30).Should(Equal(2)) + Expect(UserSnapshot(&updatedUser)).Should(MatchSnapShot()) + + Eventually(func() bool { + var ci cosmov1alpha1.ClusterInstance + err := k8sClient.Get(ctx, client.ObjectKey{Name: useraddon.InstanceName(clusterUserAddon.Name, user.GetName())}, &ci) + return apierrors.IsNotFound(err) + }, time.Second*30).Should(BeTrue()) + }) + + It("should gc old namespaced addon", func() { + ctx := context.Background() + + By("fetching and updating user") var user cosmov1alpha1.User Eventually(func() error { - err = k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &user) + err := k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &user) Expect(err).ShouldNot(HaveOccurred()) - user.Spec.Addons = append(user.Spec.Addons, cosmov1alpha1.UserAddon{ - Template: cosmov1alpha1.UserAddonTemplateRef{ - Name: emptyUserAddon.Name, + + var i cosmov1alpha1.Instance + err = k8sClient.Get(ctx, client.ObjectKey{ + Name: useraddon.InstanceName(namespacedUserAddon.Name, ""), + Namespace: user.Status.Namespace.GetName()}, &i) + Expect(err).ShouldNot(HaveOccurred()) + + user.Spec.Addons = []cosmov1alpha1.UserAddon{ + { + Template: cosmov1alpha1.UserAddonTemplateRef{ + Name: emptyUserAddon.Name, + }, }, - }) + } return k8sClient.Update(ctx, &user) }, time.Second*30).Should(Succeed()) - Expect(UserSnapshot(&user)).Should(MatchSnapShot()) var updatedUser cosmov1alpha1.User Eventually(func() int { - err = k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &updatedUser) + err := k8sClient.Get(ctx, client.ObjectKey{Name: "ua"}, &updatedUser) Expect(err).ShouldNot(HaveOccurred()) return len(updatedUser.Status.Addons) - }, time.Second*30).Should(Equal(3)) + }, time.Second*30).Should(Equal(1)) Expect(UserSnapshot(&updatedUser)).Should(MatchSnapShot()) + Eventually(func() bool { + var i cosmov1alpha1.Instance + err := k8sClient.Get(ctx, client.ObjectKey{ + Name: useraddon.InstanceName(namespacedUserAddon.Name, user.GetName()), + Namespace: user.Status.Namespace.GetName()}, &i) + return apierrors.IsNotFound(err) + }, time.Second*30).Should(BeTrue()) }) }) From c3fad88cfdd42a4cc18fc7a436053f3bbc094558 Mon Sep 17 00:00:00 2001 From: jlandowner Date: Wed, 21 Jun 2023 13:19:29 +0900 Subject: [PATCH 2/2] improve test code that sometimes fail --- internal/controllers/instance_controller_test.go | 9 ++++++--- internal/controllers/workspace_controller_test.go | 8 +++++--- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/controllers/instance_controller_test.go b/internal/controllers/instance_controller_test.go index 03321054..4955b0ac 100644 --- a/internal/controllers/instance_controller_test.go +++ b/internal/controllers/instance_controller_test.go @@ -177,13 +177,16 @@ spec: By("fetching instance resource and checking if last applied resources added in instance status") var createdInst cosmov1alpha1.Instance - Eventually(func() error { + Eventually(func() int { key := client.ObjectKey{ Name: inst.Name, Namespace: inst.Namespace, } - return k8sClient.Get(ctx, key, &createdInst) - }, time.Second*10).Should(Succeed()) + err = k8sClient.Get(ctx, key, &createdInst) + Expect(err).ShouldNot(HaveOccurred()) + + return createdInst.Status.LastAppliedObjectsCount + }, time.Second*10).ShouldNot(BeZero()) Ω(InstanceSnapshot(&createdInst)).To(MatchSnapShot()) }) }) diff --git a/internal/controllers/workspace_controller_test.go b/internal/controllers/workspace_controller_test.go index f3d3d662..33a5b4c7 100644 --- a/internal/controllers/workspace_controller_test.go +++ b/internal/controllers/workspace_controller_test.go @@ -229,9 +229,11 @@ spec: Expect(InstanceSnapshot(&createdInst)).To(MatchSnapShot()) var createdIngRoute traefikv1.IngressRoute - Eventually(func() error { - return k8sClient.Get(ctx, client.ObjectKey{Name: wsName, Namespace: nsName}, &createdIngRoute) - }, time.Second*10).Should(Succeed()) + Eventually(func() int { + err := k8sClient.Get(ctx, client.ObjectKey{Name: wsName, Namespace: nsName}, &createdIngRoute) + Expect(err).ShouldNot(HaveOccurred()) + return len(createdIngRoute.Spec.Routes) + }, time.Second*10).ShouldNot(BeZero()) Expect(ObjectSnapshot(&createdIngRoute)).To(MatchSnapShot()) }) })