From 4338c93bc092f6b8f101232d1dcd427a08734938 Mon Sep 17 00:00:00 2001 From: Ekaterina Kazakova Date: Mon, 28 Oct 2024 17:13:11 +0400 Subject: [PATCH 1/3] Rework orphaned Templates removal Closes #570 Includes: * Remove managed-by-chain label from templates since one template can be managed by multiple chains * Trigger TemplateChain reconciliation on TemplateManagement updates * Use cached k8s client in testing for operations that require field indexers since field indexers are only work with cached client * Separate TemplateChain reconciliation to Update and Delete operations --- api/v1alpha1/templatechain_common.go | 2 + internal/controller/suite_test.go | 1 + .../controller/templatechain_controller.go | 237 +++++++++++++++--- .../templatechain_controller_test.go | 96 +++++-- .../templatemanagement_controller.go | 3 + 5 files changed, 276 insertions(+), 63 deletions(-) diff --git a/api/v1alpha1/templatechain_common.go b/api/v1alpha1/templatechain_common.go index 36a2d29c3..efffd1642 100644 --- a/api/v1alpha1/templatechain_common.go +++ b/api/v1alpha1/templatechain_common.go @@ -14,6 +14,8 @@ package v1alpha1 +const TemplateChainFinalizer = "hmc.mirantis.com/template-chain" + // TemplateChainSpec defines the observed state of TemplateChain type TemplateChainSpec struct { // SupportedTemplates is the list of supported Templates definitions and all available upgrade sequences for it. diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 1cc10a197..22602dbc5 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -144,6 +144,7 @@ var _ = BeforeSuite(func() { }) Expect(err).NotTo(HaveOccurred()) mgrClient = mgr.GetClient() + Expect(mgr).NotTo(BeNil()) err = hmcmirantiscomv1alpha1.SetupIndexers(ctx, mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/templatechain_controller.go b/internal/controller/templatechain_controller.go index 5c749d21c..dd4bf6039 100644 --- a/internal/controller/templatechain_controller.go +++ b/internal/controller/templatechain_controller.go @@ -18,17 +18,24 @@ import ( "context" "errors" "fmt" + "reflect" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/fields" + "k8s.io/apimachinery/pkg/labels" ctrl "sigs.k8s.io/controller-runtime" + "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" + "sigs.k8s.io/controller-runtime/pkg/event" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/log" + "sigs.k8s.io/controller-runtime/pkg/predicate" hmc "github.com/Mirantis/hmc/api/v1alpha1" ) -const HMCManagedByChainLabelKey = "hmc.mirantis.com/managed-by-chain" - // TemplateChainReconciler reconciles a TemplateChain object type TemplateChainReconciler struct { client.Client @@ -65,7 +72,12 @@ func (r *ClusterTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl l.Error(err, "Failed to get ClusterTemplateChain") return ctrl.Result{}, err } - return r.ReconcileTemplateChain(ctx, clusterTemplateChain) + + if !clusterTemplateChain.DeletionTimestamp.IsZero() { + l.Info("Deleting ClusterTemplateChain") + return r.Delete(ctx, clusterTemplateChain) + } + return r.Update(ctx, clusterTemplateChain) } func (r *ServiceTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -82,15 +94,28 @@ func (r *ServiceTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl l.Error(err, "Failed to get ServiceTemplateChain") return ctrl.Result{}, err } - return r.ReconcileTemplateChain(ctx, serviceTemplateChain) + + if !serviceTemplateChain.DeletionTimestamp.IsZero() { + l.Info("Deleting ServiceTemplateChain") + return r.Delete(ctx, serviceTemplateChain) + } + return r.Update(ctx, serviceTemplateChain) } -func (r *TemplateChainReconciler) ReconcileTemplateChain(ctx context.Context, templateChain templateChain) (ctrl.Result, error) { +func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain templateChain) (ctrl.Result, error) { l := ctrl.LoggerFrom(ctx) - systemTemplates, managedTemplates, err := getCurrentTemplates(ctx, r.Client, templateChain.TemplateKind(), r.SystemNamespace, templateChain.GetNamespace(), templateChain.GetName()) + if controllerutil.AddFinalizer(templateChain, hmc.TemplateChainFinalizer) { + if err := r.Client.Update(ctx, templateChain); err != nil { + l.Error(err, "Failed to update TemplateChain finalizers") + return ctrl.Result{}, err + } + return ctrl.Result{}, nil + } + + systemTemplates, err := r.getSystemTemplates(ctx, templateChain) if err != nil { - return ctrl.Result{}, fmt.Errorf("failed to get current templates: %w", err) + return ctrl.Result{}, fmt.Errorf("failed to get system templates: %w", err) } var ( @@ -102,8 +127,7 @@ func (r *TemplateChainReconciler) ReconcileTemplateChain(ctx context.Context, te Name: supportedTemplate.Name, Namespace: templateChain.GetNamespace(), Labels: map[string]string{ - hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue, - HMCManagedByChainLabelKey: templateChain.GetName(), + hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue, }, } keepTemplate[supportedTemplate.Name] = struct{}{} @@ -145,70 +169,144 @@ func (r *TemplateChainReconciler) ReconcileTemplateChain(ctx context.Context, te errs = errors.Join(errs, err) } } + if errs != nil { + return ctrl.Result{}, errs + } - for _, template := range managedTemplates { - templateName := template.GetName() - if _, keep := keepTemplate[templateName]; keep { - continue - } + err = r.removeOrphanedTemplates(ctx, templateChain) + if err != nil { + return ctrl.Result{}, err + } + return ctrl.Result{}, nil +} - ll := l.WithValues("template kind", templateChain.TemplateKind(), "template namespace", templateChain.GetNamespace(), "template name", templateName) - ll.Info("Deleting Template") +func (r *TemplateChainReconciler) Delete(ctx context.Context, chain templateChain) (ctrl.Result, error) { + l := log.FromContext(ctx) - if err := r.Delete(ctx, template); client.IgnoreNotFound(err) != nil { - errs = errors.Join(errs, err) - continue - } + err := r.removeOrphanedTemplates(ctx, chain) + if err != nil { + return ctrl.Result{}, err + } - ll.Info("Template has been deleted") + // Removing finalizer in the end of cleanup + l.Info("Removing TemplateChain finalizer") + if controllerutil.RemoveFinalizer(chain, hmc.TemplateChainFinalizer) { + return ctrl.Result{}, r.Client.Update(ctx, chain) + } + return ctrl.Result{}, nil +} + +func (r TemplateChainReconciler) getSystemTemplates(ctx context.Context, chain templateChain) (systemTemplates map[string]templateCommon, _ error) { + templates, err := r.getTemplates(ctx, chain.TemplateKind(), &client.ListOptions{Namespace: r.SystemNamespace}) + if err != nil { + return nil, err } - return ctrl.Result{}, errs + systemTemplates = make(map[string]templateCommon, len(templates)) + for _, template := range templates { + systemTemplates[template.GetName()] = template + } + return systemTemplates, nil } -func getCurrentTemplates(ctx context.Context, cl client.Client, templateKind, systemNamespace, targetNamespace, templateChainName string) (systemTemplates map[string]templateCommon, managedTemplates []templateCommon, _ error) { +func (r TemplateChainReconciler) getTemplates(ctx context.Context, templateKind string, opts *client.ListOptions) ([]templateCommon, error) { var templates []templateCommon switch templateKind { case hmc.ClusterTemplateKind: ctList := &hmc.ClusterTemplateList{} - err := cl.List(ctx, ctList) + err := r.Client.List(ctx, ctList, opts) if err != nil { - return nil, nil, err + return nil, err } for _, template := range ctList.Items { templates = append(templates, &template) } case hmc.ServiceTemplateKind: stList := &hmc.ServiceTemplateList{} - err := cl.List(ctx, stList) + err := r.Client.List(ctx, stList, opts) if err != nil { - return nil, nil, err + return nil, err } for _, template := range stList.Items { templates = append(templates, &template) } default: - return nil, nil, fmt.Errorf("invalid Template kind. Supported kinds are %s and %s", hmc.ClusterTemplateKind, hmc.ServiceTemplateKind) + return nil, fmt.Errorf("invalid Template kind. Supported kinds are %s and %s", hmc.ClusterTemplateKind, hmc.ServiceTemplateKind) } + return templates, nil +} - systemTemplates = make(map[string]templateCommon, len(templates)) - managedTemplates = make([]templateCommon, 0, len(templates)) - for _, template := range templates { - if template.GetNamespace() == systemNamespace { - systemTemplates[template.GetName()] = template +func (r TemplateChainReconciler) removeOrphanedTemplates(ctx context.Context, chain templateChain) error { + l := log.FromContext(ctx) + + managedTemplates, err := r.getTemplates(ctx, chain.TemplateKind(), &client.ListOptions{ + Namespace: chain.GetNamespace(), + LabelSelector: labels.SelectorFromSet(map[string]string{hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue}), + }) + if err != nil { + return err + } + + // Removing templates not managed by any chain + var errs error + for _, template := range managedTemplates { + isOrphaned, err := isTemplateOrphaned(ctx, r.Client, chain.TemplateKind(), template.GetNamespace(), template.GetName()) + if err != nil { + errs = errors.Join(errs, err) continue } - - labels := template.GetLabels() - if template.GetNamespace() == targetNamespace && - labels[hmc.HMCManagedLabelKey] == hmc.HMCManagedLabelValue && - labels[HMCManagedByChainLabelKey] == templateChainName { - managedTemplates = append(managedTemplates, template) + if isOrphaned { + ll := l.WithValues("template kind", chain.TemplateKind(), "template namespace", template.GetNamespace(), "template name", template.GetName()) + ll.Info("Deleting Template") + + if err := r.Client.Delete(ctx, template); client.IgnoreNotFound(err) != nil { + errs = errors.Join(errs, err) + continue + } + ll.Info("Template has been deleted") } } + if errs != nil { + return errs + } + return nil +} - return systemTemplates, managedTemplates, nil +func isTemplateOrphaned(ctx context.Context, cl client.Client, templateKind, namespace, templateName string) (bool, error) { + opts := &client.ListOptions{ + Namespace: namespace, + FieldSelector: fields.SelectorFromSet(fields.Set{hmc.SupportedTemplateKey: templateName}), + } + + switch templateKind { + case hmc.ClusterTemplateKind: + list := &hmc.ClusterTemplateChainList{} + err := cl.List(ctx, list, opts) + if err != nil { + return false, err + } + for _, chain := range list.Items { + if chain.DeletionTimestamp == nil { + return false, nil + } + } + return true, nil + case hmc.ServiceTemplateKind: + list := &hmc.ServiceTemplateChainList{} + err := cl.List(ctx, list, opts) + if err != nil { + return false, err + } + for _, chain := range list.Items { + if chain.DeletionTimestamp == nil { + return false, nil + } + } + return true, nil + default: + return false, fmt.Errorf("invalid Template kind. Supported kinds are %s and %s", hmc.ClusterTemplateKind, hmc.ServiceTemplateKind) + } } func getTemplateNamesManagedByChain(chain templateChain) []string { @@ -219,10 +317,33 @@ func getTemplateNamesManagedByChain(chain templateChain) []string { return result } +var tmEvents = predicate.Funcs{ + UpdateFunc: func(e event.UpdateEvent) bool { + // Only trigger if access rules were changed + oldObject, ok := e.ObjectOld.(*hmc.TemplateManagement) + if !ok { + return false + } + newObject, ok := e.ObjectNew.(*hmc.TemplateManagement) + if !ok { + return false + } + return !reflect.DeepEqual(oldObject.Spec.AccessRules, newObject.Spec.AccessRules) + }, + DeleteFunc: func(event.DeleteEvent) bool { return false }, + GenericFunc: func(event.GenericEvent) bool { return false }, +} + // SetupWithManager sets up the controller with the Manager. func (r *ClusterTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&hmc.ClusterTemplateChain{}). + Watches(&hmc.TemplateManagement{}, + handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []ctrl.Request { + return getTemplateChainRequests(ctx, r.Client, r.SystemNamespace, &hmc.ClusterTemplateChainList{}) + }), + builder.WithPredicates(tmEvents), + ). Complete(r) } @@ -230,5 +351,41 @@ func (r *ClusterTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) erro func (r *ServiceTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&hmc.ServiceTemplateChain{}). + Watches(&hmc.TemplateManagement{}, + handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []ctrl.Request { + return getTemplateChainRequests(ctx, r.Client, r.SystemNamespace, &hmc.ServiceTemplateChainList{}) + }), + builder.WithPredicates(tmEvents), + ). Complete(r) } + +func getTemplateChainRequests(ctx context.Context, cl client.Client, systemNamespace string, list client.ObjectList) []ctrl.Request { + err := cl.List(ctx, list, client.InNamespace(systemNamespace)) + if err != nil { + return nil + } + + var req []ctrl.Request + switch chainList := list.(type) { + case *hmc.ClusterTemplateChainList: + for _, chain := range chainList.Items { + req = append(req, ctrl.Request{ + NamespacedName: client.ObjectKey{ + Namespace: chain.Namespace, + Name: chain.Name, + }, + }) + } + case *hmc.ServiceTemplateChainList: + for _, chain := range chainList.Items { + req = append(req, ctrl.Request{ + NamespacedName: client.ObjectKey{ + Namespace: chain.Namespace, + Name: chain.Name, + }, + }) + } + } + return req +} diff --git a/internal/controller/templatechain_controller_test.go b/internal/controller/templatechain_controller_test.go index 68bfeaf7d..9abab7530 100644 --- a/internal/controller/templatechain_controller_test.go +++ b/internal/controller/templatechain_controller_test.go @@ -17,6 +17,7 @@ package controller import ( "context" "fmt" + "time" helmcontrollerv2 "github.com/fluxcd/helm-controller/api/v2" . "github.com/onsi/ginkgo/v2" @@ -76,7 +77,6 @@ var _ = Describe("Template Chain Controller", func() { template.WithName("ct1"), template.WithNamespace(namespace.Name), template.WithHelmSpec(templateHelmSpec), - template.WithLabels(map[string]string{HMCManagedByChainLabelKey: ctChain1Name}), template.ManagedByHMC(), ), // Should be unchanged (unmanaged) @@ -104,7 +104,6 @@ var _ = Describe("Template Chain Controller", func() { template.WithName("st1"), template.WithNamespace(namespace.Name), template.WithHelmSpec(templateHelmSpec), - template.WithLabels(map[string]string{HMCManagedByChainLabelKey: stChain1Name}), template.ManagedByHMC(), ), // Should be unchanged (unmanaged) @@ -210,25 +209,42 @@ var _ = Describe("Template Chain Controller", func() { }) AfterEach(func() { - for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ - ctTemplates["test"], ctTemplates["ct0"], ctTemplates["ct1"], ctTemplates["ct2"], - } { - err := k8sClient.Delete(ctx, template) - Expect(crclient.IgnoreNotFound(err)).To(Succeed()) - } - for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{ - stTemplates["test"], stTemplates["st0"], stTemplates["st1"], stTemplates["st2"], - } { - err := k8sClient.Delete(ctx, template) - Expect(crclient.IgnoreNotFound(err)).To(Succeed()) + templateChainReconciler := TemplateChainReconciler{ + Client: mgrClient, + SystemNamespace: utils.DefaultSystemNamespace, } for _, chain := range ctChainNames { clusterTemplateChainResource := &hmcmirantiscomv1alpha1.ClusterTemplateChain{} err := k8sClient.Get(ctx, chain, clusterTemplateChainResource) Expect(err).NotTo(HaveOccurred()) + By("Cleanup the specific resource instance ClusterTemplateChain") - Expect(k8sClient.Delete(ctx, clusterTemplateChainResource)).To(Succeed()) + Expect(mgrClient.Delete(ctx, clusterTemplateChainResource)).To(Succeed()) + + // Wait until the cached client reflects the deletion by checking the deletion timestamp + Eventually(func() bool { + err := mgrClient.Get(ctx, chain, clusterTemplateChainResource) + if err != nil { + return errors.IsNotFound(err) + } + return !clusterTemplateChainResource.DeletionTimestamp.IsZero() + }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ClusterTemplateChain should have a deletion timestamp") + + // Running reconcile to remove the finalizer and delete the ClusterTemplateChain + clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} + _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + Expect(err).NotTo(HaveOccurred()) + + Eventually(k8sClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, chain, clusterTemplateChainResource).Should(HaveOccurred()) + } + // These ClusterTemplates should be deleted once the ClusterTemplateChain is deleted + for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ctTemplates["test"], ctTemplates["ct0"]} { + verifyObjectDeleted(ctx, template.Namespace, template) + } + for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ctTemplates["ct1"], ctTemplates["ct2"]} { + err := k8sClient.Delete(ctx, template) + Expect(crclient.IgnoreNotFound(err)).To(Succeed()) } for _, chain := range stChainNames { @@ -238,6 +254,30 @@ var _ = Describe("Template Chain Controller", func() { By("Cleanup the specific resource instance ServiceTemplateChain") Expect(k8sClient.Delete(ctx, serviceTemplateChainResource)).To(Succeed()) + + // Wait until the cached client reflects the deletion by checking the deletion timestamp + Eventually(func() bool { + err := mgrClient.Get(ctx, chain, serviceTemplateChainResource) + if err != nil { + return errors.IsNotFound(err) + } + return !serviceTemplateChainResource.DeletionTimestamp.IsZero() + }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ServiceTemplateChain should have a deletion timestamp") + + // Running reconcile to remove the finalizer and delete the ServiceTemplateChain + serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} + _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + Expect(err).NotTo(HaveOccurred()) + + Eventually(mgrClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, chain, serviceTemplateChainResource).Should(HaveOccurred()) + } + // These ServiceTemplates should be deleted once the ServiceTemplateChain is deleted + for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{stTemplates["test"], stTemplates["st0"]} { + verifyObjectDeleted(ctx, template.Namespace, template) + } + for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{stTemplates["st1"], stTemplates["st2"]} { + err := k8sClient.Delete(ctx, template) + Expect(crclient.IgnoreNotFound(err)).To(Succeed()) } By("Cleanup the namespace") @@ -255,21 +295,39 @@ var _ = Describe("Template Chain Controller", func() { Expect(err).NotTo(HaveOccurred()) templateChainReconciler := TemplateChainReconciler{ - Client: k8sClient, + Client: mgrClient, SystemNamespace: utils.DefaultSystemNamespace, } By("Reconciling the ClusterTemplateChain resources") for _, chain := range ctChainNames { + // First Reconcile should only add finalizer clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) + + ctChain := &hmcmirantiscomv1alpha1.ClusterTemplateChain{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, ctChain)).NotTo(HaveOccurred()) + Expect(ctChain.Finalizers).Should(ContainElement(hmcmirantiscomv1alpha1.TemplateChainFinalizer)) + + // Second Reconcile should reconcile the TemplateChain + _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + Expect(err).NotTo(HaveOccurred()) } By("Reconciling the ServiceTemplateChain resources") for _, chain := range stChainNames { + // First Reconcile should only add finalizer serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) + + stChain := &hmcmirantiscomv1alpha1.ServiceTemplateChain{} + Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, stChain)).NotTo(HaveOccurred()) + Expect(stChain.Finalizers).Should(ContainElement(hmcmirantiscomv1alpha1.TemplateChainFinalizer)) + + // Second Reconcile should reconcile the TemplateChain + _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + Expect(err).NotTo(HaveOccurred()) } /* @@ -286,16 +344,12 @@ var _ = Describe("Template Chain Controller", func() { */ verifyObjectCreated(ctx, namespace.Name, ctTemplates["test"]) - checkHMCManagedByChainLabelExistence(ctTemplates["test"].GetLabels(), ctChain1Name) verifyObjectCreated(ctx, namespace.Name, ctTemplates["ct0"]) - checkHMCManagedByChainLabelExistence(ctTemplates["test"].GetLabels(), ctChain1Name) verifyObjectDeleted(ctx, namespace.Name, ctTemplates["ct1"]) verifyObjectUnchanged(ctx, namespace.Name, ctUnmanagedBefore, ctTemplates["ct2"]) verifyObjectCreated(ctx, namespace.Name, stTemplates["test"]) - checkHMCManagedByChainLabelExistence(stTemplates["test"].GetLabels(), stChain1Name) verifyObjectCreated(ctx, namespace.Name, stTemplates["st0"]) - checkHMCManagedByChainLabelExistence(stTemplates["st0"].GetLabels(), stChain1Name) verifyObjectDeleted(ctx, namespace.Name, stTemplates["st1"]) verifyObjectUnchanged(ctx, namespace.Name, stUnmanagedBefore, stTemplates["st2"]) }) @@ -333,7 +387,3 @@ func verifyObjectUnchanged(ctx context.Context, namespace string, oldObj, newObj func checkHMCManagedLabelExistence(labels map[string]string) { Expect(labels).To(HaveKeyWithValue(hmcmirantiscomv1alpha1.HMCManagedLabelKey, hmcmirantiscomv1alpha1.HMCManagedLabelValue)) } - -func checkHMCManagedByChainLabelExistence(labels map[string]string, chainName string) { - Expect(labels).To(HaveKeyWithValue(HMCManagedByChainLabelKey, chainName)) -} diff --git a/internal/controller/templatemanagement_controller.go b/internal/controller/templatemanagement_controller.go index 281a8061c..6b9c03131 100644 --- a/internal/controller/templatemanagement_controller.go +++ b/internal/controller/templatemanagement_controller.go @@ -231,6 +231,9 @@ func (r *TemplateManagementReconciler) createTemplateChain(ctx context.Context, Labels: map[string]string{ hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue, }, + Finalizers: []string{ + hmc.TemplateChainFinalizer, + }, } var target templateChain switch source.Kind() { From 6cde14bfacd6ba2402fbd10f7e2d785c0b36e76a Mon Sep 17 00:00:00 2001 From: Ekaterina Kazakova Date: Tue, 5 Nov 2024 20:28:10 +0400 Subject: [PATCH 2/3] Rework oprhaned Templates removal: addressed comments --- api/v1alpha1/clustertemplate_types.go | 16 ++ api/v1alpha1/providertemplate_types.go | 7 + api/v1alpha1/servicetemplate_types.go | 16 ++ internal/controller/template_controller.go | 1 + .../controller/templatechain_controller.go | 153 +++++++----------- .../templatechain_controller_test.go | 4 + .../templatemanagement_controller.go | 9 +- 7 files changed, 103 insertions(+), 103 deletions(-) diff --git a/api/v1alpha1/clustertemplate_types.go b/api/v1alpha1/clustertemplate_types.go index 9018a01e9..036021bc8 100644 --- a/api/v1alpha1/clustertemplate_types.go +++ b/api/v1alpha1/clustertemplate_types.go @@ -15,10 +15,12 @@ package v1alpha1 import ( + "context" "fmt" "github.com/Masterminds/semver/v3" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -106,6 +108,20 @@ func (t *ClusterTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } +// IsOrphaned checks whether the Template is orphaned or not. +func (t *ClusterTemplate) IsOrphaned(ctx context.Context, cl client.Client) (bool, error) { + list := new(ClusterTemplateChainList) + if err := cl.List(ctx, list, client.InNamespace(t.Namespace), client.MatchingFields{SupportedTemplateKey: t.Name}); err != nil { + return false, fmt.Errorf("failed to list %s: %w", list.GroupVersionKind(), err) + } + for _, chain := range list.Items { + if chain.DeletionTimestamp == nil { + return false, nil + } + } + return true, nil +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=clustertmpl diff --git a/api/v1alpha1/providertemplate_types.go b/api/v1alpha1/providertemplate_types.go index 66e82f1bb..a6c843fb1 100644 --- a/api/v1alpha1/providertemplate_types.go +++ b/api/v1alpha1/providertemplate_types.go @@ -15,9 +15,11 @@ package v1alpha1 import ( + "context" "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) // ProviderTemplateKind denotes the providertemplate resource Kind. @@ -66,6 +68,11 @@ func (t *ProviderTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } +// IsOrphaned checks whether the Template is orphaned or not. +func (*ProviderTemplate) IsOrphaned(_ context.Context, _ client.Client) (bool, error) { + return false, nil +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=providertmpl,scope=Cluster diff --git a/api/v1alpha1/servicetemplate_types.go b/api/v1alpha1/servicetemplate_types.go index 97d52fb3e..f7b084fae 100644 --- a/api/v1alpha1/servicetemplate_types.go +++ b/api/v1alpha1/servicetemplate_types.go @@ -15,10 +15,12 @@ package v1alpha1 import ( + "context" "fmt" "github.com/Masterminds/semver/v3" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -85,6 +87,20 @@ func (t *ServiceTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } +// IsOrphaned checks whether the Template is orphaned or not. +func (t *ServiceTemplate) IsOrphaned(ctx context.Context, cl client.Client) (bool, error) { + list := new(ServiceTemplateChainList) + if err := cl.List(ctx, list, client.InNamespace(t.Namespace), client.MatchingFields{SupportedTemplateKey: t.Name}); err != nil { + return false, fmt.Errorf("failed to list %s: %w", list.GroupVersionKind(), err) + } + for _, chain := range list.Items { + if chain.DeletionTimestamp == nil { + return false, nil + } + } + return true, nil +} + // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=svctmpl diff --git a/internal/controller/template_controller.go b/internal/controller/template_controller.go index 560f7c02f..90a997f29 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -168,6 +168,7 @@ type templateCommon interface { GetHelmSpec() *hmc.HelmSpec GetCommonStatus() *hmc.TemplateStatusCommon FillStatusWithProviders(map[string]string) error + IsOrphaned(context.Context, client.Client) (bool, error) } func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template templateCommon) (ctrl.Result, error) { diff --git a/internal/controller/templatechain_controller.go b/internal/controller/templatechain_controller.go index dd4bf6039..cb51af7ae 100644 --- a/internal/controller/templatechain_controller.go +++ b/internal/controller/templatechain_controller.go @@ -18,11 +18,10 @@ import ( "context" "errors" "fmt" - "reflect" + "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" @@ -40,6 +39,8 @@ import ( type TemplateChainReconciler struct { client.Client SystemNamespace string + + templateKind string } type ClusterTemplateChainReconciler struct { @@ -53,8 +54,6 @@ type ServiceTemplateChainReconciler struct { // templateChain is the interface defining a list of methods to interact with *templatechains type templateChain interface { client.Object - Kind() string - TemplateKind() string GetSpec() *hmc.TemplateChainSpec } @@ -113,7 +112,7 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp return ctrl.Result{}, nil } - systemTemplates, err := r.getSystemTemplates(ctx, templateChain) + systemTemplates, err := r.getSystemTemplates(ctx) if err != nil { return ctrl.Result{}, fmt.Errorf("failed to get system templates: %w", err) } @@ -134,11 +133,11 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp source, found := systemTemplates[supportedTemplate.Name] if !found { - errs = errors.Join(errs, fmt.Errorf("source %s %s/%s is not found", templateChain.TemplateKind(), r.SystemNamespace, supportedTemplate.Name)) + errs = errors.Join(errs, fmt.Errorf("source %s %s/%s is not found", r.templateKind, r.SystemNamespace, supportedTemplate.Name)) continue } if source.GetCommonStatus().ChartRef == nil { - errs = errors.Join(errs, fmt.Errorf("source %s %s/%s does not have chart reference yet", templateChain.TemplateKind(), r.SystemNamespace, supportedTemplate.Name)) + errs = errors.Join(errs, fmt.Errorf("source %s %s/%s does not have chart reference yet", r.templateKind, r.SystemNamespace, supportedTemplate.Name)) continue } @@ -147,12 +146,12 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp } var target client.Object - switch templateChain.Kind() { - case hmc.ClusterTemplateChainKind: + switch r.templateKind { + case hmc.ClusterTemplateKind: target = &hmc.ClusterTemplate{ObjectMeta: meta, Spec: hmc.ClusterTemplateSpec{ Helm: helmSpec, }} - case hmc.ServiceTemplateChainKind: + case hmc.ServiceTemplateKind: target = &hmc.ServiceTemplate{ObjectMeta: meta, Spec: hmc.ServiceTemplateSpec{ Helm: helmSpec, }} @@ -161,7 +160,7 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp } if err := r.Create(ctx, target); err == nil { - l.Info(templateChain.TemplateKind()+" was successfully created", "template namespace", templateChain.GetNamespace(), "template name", supportedTemplate.Name) + l.Info(r.templateKind+" was successfully created", "template namespace", templateChain.GetNamespace(), "template name", supportedTemplate.Name) continue } @@ -173,43 +172,36 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp return ctrl.Result{}, errs } - err = r.removeOrphanedTemplates(ctx, templateChain) - if err != nil { - return ctrl.Result{}, err - } - return ctrl.Result{}, nil + return ctrl.Result{}, r.removeOrphanedTemplates(ctx, templateChain) } func (r *TemplateChainReconciler) Delete(ctx context.Context, chain templateChain) (ctrl.Result, error) { - l := log.FromContext(ctx) - - err := r.removeOrphanedTemplates(ctx, chain) - if err != nil { + if err := r.removeOrphanedTemplates(ctx, chain); err != nil { return ctrl.Result{}, err } // Removing finalizer in the end of cleanup - l.Info("Removing TemplateChain finalizer") if controllerutil.RemoveFinalizer(chain, hmc.TemplateChainFinalizer) { + ctrl.LoggerFrom(ctx).Info("Removing TemplateChain finalizer") return ctrl.Result{}, r.Client.Update(ctx, chain) } return ctrl.Result{}, nil } -func (r TemplateChainReconciler) getSystemTemplates(ctx context.Context, chain templateChain) (systemTemplates map[string]templateCommon, _ error) { - templates, err := r.getTemplates(ctx, chain.TemplateKind(), &client.ListOptions{Namespace: r.SystemNamespace}) +func (r *TemplateChainReconciler) getSystemTemplates(ctx context.Context) (systemTemplates map[string]templateCommon, _ error) { + templates, err := r.getTemplates(ctx, r.templateKind, &client.ListOptions{Namespace: r.SystemNamespace}) if err != nil { return nil, err } systemTemplates = make(map[string]templateCommon, len(templates)) - for _, template := range templates { - systemTemplates[template.GetName()] = template + for _, tmpl := range templates { + systemTemplates[tmpl.GetName()] = tmpl } return systemTemplates, nil } -func (r TemplateChainReconciler) getTemplates(ctx context.Context, templateKind string, opts *client.ListOptions) ([]templateCommon, error) { +func (r *TemplateChainReconciler) getTemplates(ctx context.Context, templateKind string, opts *client.ListOptions) ([]templateCommon, error) { var templates []templateCommon switch templateKind { @@ -240,7 +232,7 @@ func (r TemplateChainReconciler) getTemplates(ctx context.Context, templateKind func (r TemplateChainReconciler) removeOrphanedTemplates(ctx context.Context, chain templateChain) error { l := log.FromContext(ctx) - managedTemplates, err := r.getTemplates(ctx, chain.TemplateKind(), &client.ListOptions{ + managedTemplates, err := r.getTemplates(ctx, r.templateKind, &client.ListOptions{ Namespace: chain.GetNamespace(), LabelSelector: labels.SelectorFromSet(map[string]string{hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue}), }) @@ -250,69 +242,30 @@ func (r TemplateChainReconciler) removeOrphanedTemplates(ctx context.Context, ch // Removing templates not managed by any chain var errs error - for _, template := range managedTemplates { - isOrphaned, err := isTemplateOrphaned(ctx, r.Client, chain.TemplateKind(), template.GetNamespace(), template.GetName()) + for _, tmpl := range managedTemplates { + orphaned, err := tmpl.IsOrphaned(ctx, r.Client) if err != nil { errs = errors.Join(errs, err) continue } - if isOrphaned { - ll := l.WithValues("template kind", chain.TemplateKind(), "template namespace", template.GetNamespace(), "template name", template.GetName()) + if orphaned { + ll := l.WithValues("template kind", r.templateKind, "template namespace", tmpl.GetNamespace(), "template name", tmpl.GetName()) ll.Info("Deleting Template") - if err := r.Client.Delete(ctx, template); client.IgnoreNotFound(err) != nil { + if err := r.Client.Delete(ctx, tmpl); client.IgnoreNotFound(err) != nil { errs = errors.Join(errs, err) continue } ll.Info("Template has been deleted") } } - if errs != nil { - return errs - } - return nil -} - -func isTemplateOrphaned(ctx context.Context, cl client.Client, templateKind, namespace, templateName string) (bool, error) { - opts := &client.ListOptions{ - Namespace: namespace, - FieldSelector: fields.SelectorFromSet(fields.Set{hmc.SupportedTemplateKey: templateName}), - } - - switch templateKind { - case hmc.ClusterTemplateKind: - list := &hmc.ClusterTemplateChainList{} - err := cl.List(ctx, list, opts) - if err != nil { - return false, err - } - for _, chain := range list.Items { - if chain.DeletionTimestamp == nil { - return false, nil - } - } - return true, nil - case hmc.ServiceTemplateKind: - list := &hmc.ServiceTemplateChainList{} - err := cl.List(ctx, list, opts) - if err != nil { - return false, err - } - for _, chain := range list.Items { - if chain.DeletionTimestamp == nil { - return false, nil - } - } - return true, nil - default: - return false, fmt.Errorf("invalid Template kind. Supported kinds are %s and %s", hmc.ClusterTemplateKind, hmc.ServiceTemplateKind) - } + return errs } func getTemplateNamesManagedByChain(chain templateChain) []string { result := make([]string, 0, len(chain.GetSpec().SupportedTemplates)) - for _, template := range chain.GetSpec().SupportedTemplates { - result = append(result, template.Name) + for _, tmpl := range chain.GetSpec().SupportedTemplates { + result = append(result, tmpl.Name) } return result } @@ -328,7 +281,7 @@ var tmEvents = predicate.Funcs{ if !ok { return false } - return !reflect.DeepEqual(oldObject.Spec.AccessRules, newObject.Spec.AccessRules) + return !equality.Semantic.DeepEqual(oldObject.Spec.AccessRules, newObject.Spec.AccessRules) }, DeleteFunc: func(event.DeleteEvent) bool { return false }, GenericFunc: func(event.GenericEvent) bool { return false }, @@ -336,11 +289,13 @@ var tmEvents = predicate.Funcs{ // SetupWithManager sets up the controller with the Manager. func (r *ClusterTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.templateKind = hmc.ClusterTemplateKind + return ctrl.NewControllerManagedBy(mgr). For(&hmc.ClusterTemplateChain{}). Watches(&hmc.TemplateManagement{}, - handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []ctrl.Request { - return getTemplateChainRequests(ctx, r.Client, r.SystemNamespace, &hmc.ClusterTemplateChainList{}) + handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []ctrl.Request { + return getTemplateChainRequests(o, r.SystemNamespace, hmc.ClusterTemplateKind) }), builder.WithPredicates(tmEvents), ). @@ -349,43 +304,43 @@ func (r *ClusterTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) erro // SetupWithManager sets up the controller with the Manager. func (r *ServiceTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) error { + r.templateKind = hmc.ServiceTemplateKind + return ctrl.NewControllerManagedBy(mgr). For(&hmc.ServiceTemplateChain{}). Watches(&hmc.TemplateManagement{}, - handler.EnqueueRequestsFromMapFunc(func(ctx context.Context, _ client.Object) []ctrl.Request { - return getTemplateChainRequests(ctx, r.Client, r.SystemNamespace, &hmc.ServiceTemplateChainList{}) + handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []ctrl.Request { + return getTemplateChainRequests(o, r.SystemNamespace, hmc.ServiceTemplateKind) }), builder.WithPredicates(tmEvents), ). Complete(r) } -func getTemplateChainRequests(ctx context.Context, cl client.Client, systemNamespace string, list client.ObjectList) []ctrl.Request { - err := cl.List(ctx, list, client.InNamespace(systemNamespace)) - if err != nil { +func getTemplateChainRequests(o client.Object, systemNamespace, kind string) (reqs []ctrl.Request) { + tm, ok := o.(*hmc.TemplateManagement) + if !ok { return nil } - - var req []ctrl.Request - switch chainList := list.(type) { - case *hmc.ClusterTemplateChainList: - for _, chain := range chainList.Items { - req = append(req, ctrl.Request{ + f := func(chains []string) { + for _, c := range chains { + reqs = append(reqs, ctrl.Request{ NamespacedName: client.ObjectKey{ - Namespace: chain.Namespace, - Name: chain.Name, + Namespace: systemNamespace, + Name: c, }, }) } - case *hmc.ServiceTemplateChainList: - for _, chain := range chainList.Items { - req = append(req, ctrl.Request{ - NamespacedName: client.ObjectKey{ - Namespace: chain.Namespace, - Name: chain.Name, - }, - }) + } + if kind == hmc.ClusterTemplateChainKind { + for _, ar := range tm.Spec.AccessRules { + f(ar.ClusterTemplateChains) + } + } + if kind == hmc.ServiceTemplateChainKind { + for _, ar := range tm.Spec.AccessRules { + f(ar.ServiceTemplateChains) } } - return req + return reqs } diff --git a/internal/controller/templatechain_controller_test.go b/internal/controller/templatechain_controller_test.go index 9abab7530..b8de60147 100644 --- a/internal/controller/templatechain_controller_test.go +++ b/internal/controller/templatechain_controller_test.go @@ -232,6 +232,7 @@ var _ = Describe("Template Chain Controller", func() { }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ClusterTemplateChain should have a deletion timestamp") // Running reconcile to remove the finalizer and delete the ClusterTemplateChain + templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ClusterTemplateKind clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) @@ -265,6 +266,7 @@ var _ = Describe("Template Chain Controller", func() { }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ServiceTemplateChain should have a deletion timestamp") // Running reconcile to remove the finalizer and delete the ServiceTemplateChain + templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ServiceTemplateKind serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) @@ -301,6 +303,7 @@ var _ = Describe("Template Chain Controller", func() { By("Reconciling the ClusterTemplateChain resources") for _, chain := range ctChainNames { // First Reconcile should only add finalizer + templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ClusterTemplateKind clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) @@ -317,6 +320,7 @@ var _ = Describe("Template Chain Controller", func() { By("Reconciling the ServiceTemplateChain resources") for _, chain := range stChainNames { // First Reconcile should only add finalizer + templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ServiceTemplateKind serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/templatemanagement_controller.go b/internal/controller/templatemanagement_controller.go index 6b9c03131..93d08fe5d 100644 --- a/internal/controller/templatemanagement_controller.go +++ b/internal/controller/templatemanagement_controller.go @@ -111,7 +111,7 @@ func (r *TemplateManagementReconciler) Reconcile(ctx context.Context, req ctrl.R for _, managedChain := range append(managedCtChains, managedStChains...) { keep := false templateNamespacedName := getNamespacedName(managedChain.GetNamespace(), managedChain.GetName()) - switch managedChain.Kind() { + switch managedChain.GetObjectKind().GroupVersionKind().Kind { case hmc.ClusterTemplateChainKind: keep = keepCtChains[templateNamespacedName] case hmc.ServiceTemplateChainKind: @@ -236,7 +236,8 @@ func (r *TemplateManagementReconciler) createTemplateChain(ctx context.Context, }, } var target templateChain - switch source.Kind() { + kind := source.GetObjectKind().GroupVersionKind().Kind + switch kind { case hmc.ClusterTemplateChainKind: target = &hmc.ClusterTemplateChain{ObjectMeta: meta, Spec: *source.GetSpec()} case hmc.ServiceTemplateChainKind: @@ -250,7 +251,7 @@ func (r *TemplateManagementReconciler) createTemplateChain(ctx context.Context, } return err } - l.Info(source.Kind()+" was successfully created", "target namespace", targetNamespace, "source name", source.GetName()) + l.Info(kind+" was successfully created", "target namespace", targetNamespace, "source name", source.GetName()) return nil } @@ -264,7 +265,7 @@ func (r *TemplateManagementReconciler) deleteTemplateChain(ctx context.Context, } return err } - l.Info(chain.Kind()+"%s was successfully deleted", "chain namespace", chain.GetNamespace(), "chain name", chain.GetName()) + l.Info(chain.GetObjectKind().GroupVersionKind().Kind+" was successfully deleted", "chain namespace", chain.GetNamespace(), "chain name", chain.GetName()) return nil } From 3566f959a0f412ae8ee0c00ce66ed211004073b6 Mon Sep 17 00:00:00 2001 From: Ekaterina Kazakova Date: Thu, 7 Nov 2024 19:53:15 +0400 Subject: [PATCH 3/3] Add ownerReference on managed Templates --- api/v1alpha1/clustertemplate_types.go | 16 -- api/v1alpha1/providertemplate_types.go | 7 - api/v1alpha1/servicetemplate_types.go | 16 -- api/v1alpha1/templatechain_common.go | 2 - internal/controller/suite_test.go | 2 +- internal/controller/template_controller.go | 1 - .../controller/templatechain_controller.go | 157 ++---------------- .../templatechain_controller_test.go | 148 +++++++++-------- .../templatemanagement_controller.go | 3 - test/objects/template/template.go | 12 ++ 10 files changed, 106 insertions(+), 258 deletions(-) diff --git a/api/v1alpha1/clustertemplate_types.go b/api/v1alpha1/clustertemplate_types.go index 036021bc8..9018a01e9 100644 --- a/api/v1alpha1/clustertemplate_types.go +++ b/api/v1alpha1/clustertemplate_types.go @@ -15,12 +15,10 @@ package v1alpha1 import ( - "context" "fmt" "github.com/Masterminds/semver/v3" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -108,20 +106,6 @@ func (t *ClusterTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } -// IsOrphaned checks whether the Template is orphaned or not. -func (t *ClusterTemplate) IsOrphaned(ctx context.Context, cl client.Client) (bool, error) { - list := new(ClusterTemplateChainList) - if err := cl.List(ctx, list, client.InNamespace(t.Namespace), client.MatchingFields{SupportedTemplateKey: t.Name}); err != nil { - return false, fmt.Errorf("failed to list %s: %w", list.GroupVersionKind(), err) - } - for _, chain := range list.Items { - if chain.DeletionTimestamp == nil { - return false, nil - } - } - return true, nil -} - // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=clustertmpl diff --git a/api/v1alpha1/providertemplate_types.go b/api/v1alpha1/providertemplate_types.go index a6c843fb1..66e82f1bb 100644 --- a/api/v1alpha1/providertemplate_types.go +++ b/api/v1alpha1/providertemplate_types.go @@ -15,11 +15,9 @@ package v1alpha1 import ( - "context" "fmt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) // ProviderTemplateKind denotes the providertemplate resource Kind. @@ -68,11 +66,6 @@ func (t *ProviderTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } -// IsOrphaned checks whether the Template is orphaned or not. -func (*ProviderTemplate) IsOrphaned(_ context.Context, _ client.Client) (bool, error) { - return false, nil -} - // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=providertmpl,scope=Cluster diff --git a/api/v1alpha1/servicetemplate_types.go b/api/v1alpha1/servicetemplate_types.go index f7b084fae..97d52fb3e 100644 --- a/api/v1alpha1/servicetemplate_types.go +++ b/api/v1alpha1/servicetemplate_types.go @@ -15,12 +15,10 @@ package v1alpha1 import ( - "context" "fmt" "github.com/Masterminds/semver/v3" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "sigs.k8s.io/controller-runtime/pkg/client" ) const ( @@ -87,20 +85,6 @@ func (t *ServiceTemplate) GetCommonStatus() *TemplateStatusCommon { return &t.Status.TemplateStatusCommon } -// IsOrphaned checks whether the Template is orphaned or not. -func (t *ServiceTemplate) IsOrphaned(ctx context.Context, cl client.Client) (bool, error) { - list := new(ServiceTemplateChainList) - if err := cl.List(ctx, list, client.InNamespace(t.Namespace), client.MatchingFields{SupportedTemplateKey: t.Name}); err != nil { - return false, fmt.Errorf("failed to list %s: %w", list.GroupVersionKind(), err) - } - for _, chain := range list.Items { - if chain.DeletionTimestamp == nil { - return false, nil - } - } - return true, nil -} - // +kubebuilder:object:root=true // +kubebuilder:subresource:status // +kubebuilder:resource:shortName=svctmpl diff --git a/api/v1alpha1/templatechain_common.go b/api/v1alpha1/templatechain_common.go index efffd1642..36a2d29c3 100644 --- a/api/v1alpha1/templatechain_common.go +++ b/api/v1alpha1/templatechain_common.go @@ -14,8 +14,6 @@ package v1alpha1 -const TemplateChainFinalizer = "hmc.mirantis.com/template-chain" - // TemplateChainSpec defines the observed state of TemplateChain type TemplateChainSpec struct { // SupportedTemplates is the list of supported Templates definitions and all available upgrade sequences for it. diff --git a/internal/controller/suite_test.go b/internal/controller/suite_test.go index 22602dbc5..e805f260c 100644 --- a/internal/controller/suite_test.go +++ b/internal/controller/suite_test.go @@ -144,7 +144,7 @@ var _ = BeforeSuite(func() { }) Expect(err).NotTo(HaveOccurred()) mgrClient = mgr.GetClient() - Expect(mgr).NotTo(BeNil()) + Expect(mgrClient).NotTo(BeNil()) err = hmcmirantiscomv1alpha1.SetupIndexers(ctx, mgr) Expect(err).NotTo(HaveOccurred()) diff --git a/internal/controller/template_controller.go b/internal/controller/template_controller.go index 90a997f29..560f7c02f 100644 --- a/internal/controller/template_controller.go +++ b/internal/controller/template_controller.go @@ -168,7 +168,6 @@ type templateCommon interface { GetHelmSpec() *hmc.HelmSpec GetCommonStatus() *hmc.TemplateStatusCommon FillStatusWithProviders(map[string]string) error - IsOrphaned(context.Context, client.Client) (bool, error) } func (r *TemplateReconciler) ReconcileTemplate(ctx context.Context, template templateCommon) (ctrl.Result, error) { diff --git a/internal/controller/templatechain_controller.go b/internal/controller/templatechain_controller.go index cb51af7ae..1eae1d58c 100644 --- a/internal/controller/templatechain_controller.go +++ b/internal/controller/templatechain_controller.go @@ -19,20 +19,14 @@ import ( "errors" "fmt" - "k8s.io/apimachinery/pkg/api/equality" apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/apimachinery/pkg/labels" ctrl "sigs.k8s.io/controller-runtime" - "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" - "sigs.k8s.io/controller-runtime/pkg/event" - "sigs.k8s.io/controller-runtime/pkg/handler" - "sigs.k8s.io/controller-runtime/pkg/log" - "sigs.k8s.io/controller-runtime/pkg/predicate" hmc "github.com/Mirantis/hmc/api/v1alpha1" + "github.com/Mirantis/hmc/internal/utils" ) // TemplateChainReconciler reconciles a TemplateChain object @@ -72,11 +66,7 @@ func (r *ClusterTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if !clusterTemplateChain.DeletionTimestamp.IsZero() { - l.Info("Deleting ClusterTemplateChain") - return r.Delete(ctx, clusterTemplateChain) - } - return r.Update(ctx, clusterTemplateChain) + return r.ReconcileTemplateChain(ctx, clusterTemplateChain) } func (r *ServiceTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { @@ -94,21 +84,13 @@ func (r *ServiceTemplateChainReconciler) Reconcile(ctx context.Context, req ctrl return ctrl.Result{}, err } - if !serviceTemplateChain.DeletionTimestamp.IsZero() { - l.Info("Deleting ServiceTemplateChain") - return r.Delete(ctx, serviceTemplateChain) - } - return r.Update(ctx, serviceTemplateChain) + return r.ReconcileTemplateChain(ctx, serviceTemplateChain) } -func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain templateChain) (ctrl.Result, error) { +func (r *TemplateChainReconciler) ReconcileTemplateChain(ctx context.Context, templateChain templateChain) (ctrl.Result, error) { l := ctrl.LoggerFrom(ctx) - if controllerutil.AddFinalizer(templateChain, hmc.TemplateChainFinalizer) { - if err := r.Client.Update(ctx, templateChain); err != nil { - l.Error(err, "Failed to update TemplateChain finalizers") - return ctrl.Result{}, err - } + if templateChain.GetNamespace() == r.SystemNamespace { return ctrl.Result{}, nil } @@ -117,10 +99,7 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp return ctrl.Result{}, fmt.Errorf("failed to get system templates: %w", err) } - var ( - errs error - keepTemplate = make(map[string]struct{}, len(templateChain.GetSpec().SupportedTemplates)) - ) + var errs error for _, supportedTemplate := range templateChain.GetSpec().SupportedTemplates { meta := metav1.ObjectMeta{ Name: supportedTemplate.Name, @@ -129,7 +108,6 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue, }, } - keepTemplate[supportedTemplate.Name] = struct{}{} source, found := systemTemplates[supportedTemplate.Name] if !found { @@ -159,33 +137,24 @@ func (r *TemplateChainReconciler) Update(ctx context.Context, templateChain temp return ctrl.Result{}, fmt.Errorf("invalid TemplateChain kind. Supported kinds are %s and %s", hmc.ClusterTemplateChainKind, hmc.ServiceTemplateChainKind) } - if err := r.Create(ctx, target); err == nil { - l.Info(r.templateKind+" was successfully created", "template namespace", templateChain.GetNamespace(), "template name", supportedTemplate.Name) + operation, err := ctrl.CreateOrUpdate(ctx, r.Client, target, func() error { + utils.AddOwnerReference(target, templateChain) + return nil + }) + if err != nil { + errs = errors.Join(errs, err) continue } - if !apierrors.IsAlreadyExists(err) { - errs = errors.Join(errs, err) + if operation == controllerutil.OperationResultCreated { + l.Info(r.templateKind+" was successfully created", "template namespace", templateChain.GetNamespace(), "template name", supportedTemplate.Name) + } + if operation == controllerutil.OperationResultUpdated { + l.Info("Successfully updated OwnerReference on "+r.templateKind, "template namespace", templateChain.GetNamespace(), "template name", supportedTemplate.Name) } - } - if errs != nil { - return ctrl.Result{}, errs - } - - return ctrl.Result{}, r.removeOrphanedTemplates(ctx, templateChain) -} - -func (r *TemplateChainReconciler) Delete(ctx context.Context, chain templateChain) (ctrl.Result, error) { - if err := r.removeOrphanedTemplates(ctx, chain); err != nil { - return ctrl.Result{}, err } - // Removing finalizer in the end of cleanup - if controllerutil.RemoveFinalizer(chain, hmc.TemplateChainFinalizer) { - ctrl.LoggerFrom(ctx).Info("Removing TemplateChain finalizer") - return ctrl.Result{}, r.Client.Update(ctx, chain) - } - return ctrl.Result{}, nil + return ctrl.Result{}, errs } func (r *TemplateChainReconciler) getSystemTemplates(ctx context.Context) (systemTemplates map[string]templateCommon, _ error) { @@ -229,39 +198,6 @@ func (r *TemplateChainReconciler) getTemplates(ctx context.Context, templateKind return templates, nil } -func (r TemplateChainReconciler) removeOrphanedTemplates(ctx context.Context, chain templateChain) error { - l := log.FromContext(ctx) - - managedTemplates, err := r.getTemplates(ctx, r.templateKind, &client.ListOptions{ - Namespace: chain.GetNamespace(), - LabelSelector: labels.SelectorFromSet(map[string]string{hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue}), - }) - if err != nil { - return err - } - - // Removing templates not managed by any chain - var errs error - for _, tmpl := range managedTemplates { - orphaned, err := tmpl.IsOrphaned(ctx, r.Client) - if err != nil { - errs = errors.Join(errs, err) - continue - } - if orphaned { - ll := l.WithValues("template kind", r.templateKind, "template namespace", tmpl.GetNamespace(), "template name", tmpl.GetName()) - ll.Info("Deleting Template") - - if err := r.Client.Delete(ctx, tmpl); client.IgnoreNotFound(err) != nil { - errs = errors.Join(errs, err) - continue - } - ll.Info("Template has been deleted") - } - } - return errs -} - func getTemplateNamesManagedByChain(chain templateChain) []string { result := make([]string, 0, len(chain.GetSpec().SupportedTemplates)) for _, tmpl := range chain.GetSpec().SupportedTemplates { @@ -270,35 +206,12 @@ func getTemplateNamesManagedByChain(chain templateChain) []string { return result } -var tmEvents = predicate.Funcs{ - UpdateFunc: func(e event.UpdateEvent) bool { - // Only trigger if access rules were changed - oldObject, ok := e.ObjectOld.(*hmc.TemplateManagement) - if !ok { - return false - } - newObject, ok := e.ObjectNew.(*hmc.TemplateManagement) - if !ok { - return false - } - return !equality.Semantic.DeepEqual(oldObject.Spec.AccessRules, newObject.Spec.AccessRules) - }, - DeleteFunc: func(event.DeleteEvent) bool { return false }, - GenericFunc: func(event.GenericEvent) bool { return false }, -} - // SetupWithManager sets up the controller with the Manager. func (r *ClusterTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) error { r.templateKind = hmc.ClusterTemplateKind return ctrl.NewControllerManagedBy(mgr). For(&hmc.ClusterTemplateChain{}). - Watches(&hmc.TemplateManagement{}, - handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []ctrl.Request { - return getTemplateChainRequests(o, r.SystemNamespace, hmc.ClusterTemplateKind) - }), - builder.WithPredicates(tmEvents), - ). Complete(r) } @@ -308,39 +221,5 @@ func (r *ServiceTemplateChainReconciler) SetupWithManager(mgr ctrl.Manager) erro return ctrl.NewControllerManagedBy(mgr). For(&hmc.ServiceTemplateChain{}). - Watches(&hmc.TemplateManagement{}, - handler.EnqueueRequestsFromMapFunc(func(_ context.Context, o client.Object) []ctrl.Request { - return getTemplateChainRequests(o, r.SystemNamespace, hmc.ServiceTemplateKind) - }), - builder.WithPredicates(tmEvents), - ). Complete(r) } - -func getTemplateChainRequests(o client.Object, systemNamespace, kind string) (reqs []ctrl.Request) { - tm, ok := o.(*hmc.TemplateManagement) - if !ok { - return nil - } - f := func(chains []string) { - for _, c := range chains { - reqs = append(reqs, ctrl.Request{ - NamespacedName: client.ObjectKey{ - Namespace: systemNamespace, - Name: c, - }, - }) - } - } - if kind == hmc.ClusterTemplateChainKind { - for _, ar := range tm.Spec.AccessRules { - f(ar.ClusterTemplateChains) - } - } - if kind == hmc.ServiceTemplateChainKind { - for _, ar := range tm.Spec.AccessRules { - f(ar.ServiceTemplateChains) - } - } - return reqs -} diff --git a/internal/controller/templatechain_controller_test.go b/internal/controller/templatechain_controller_test.go index b8de60147..4457644d4 100644 --- a/internal/controller/templatechain_controller_test.go +++ b/internal/controller/templatechain_controller_test.go @@ -72,12 +72,11 @@ var _ = Describe("Template Chain Controller", func() { template.WithNamespace(utils.DefaultSystemNamespace), template.WithHelmSpec(templateHelmSpec), ), - // Should be deleted (not in the list of supported templates) + // Should be created in target namespace. Template is managed by two chains. "ct1": template.NewClusterTemplate( template.WithName("ct1"), - template.WithNamespace(namespace.Name), + template.WithNamespace(utils.DefaultSystemNamespace), template.WithHelmSpec(templateHelmSpec), - template.ManagedByHMC(), ), // Should be unchanged (unmanaged) "ct2": template.NewClusterTemplate( @@ -99,12 +98,11 @@ var _ = Describe("Template Chain Controller", func() { template.WithNamespace(utils.DefaultSystemNamespace), template.WithHelmSpec(templateHelmSpec), ), - // Should be deleted (not in the list of supported templates) + // Should be created in target namespace. Template is managed by two chains. "st1": template.NewServiceTemplate( template.WithName("st1"), - template.WithNamespace(namespace.Name), + template.WithNamespace(utils.DefaultSystemNamespace), template.WithHelmSpec(templateHelmSpec), - template.ManagedByHMC(), ), // Should be unchanged (unmanaged) "st2": template.NewServiceTemplate( @@ -125,27 +123,28 @@ var _ = Describe("Template Chain Controller", func() { supportedClusterTemplates := map[string][]hmcmirantiscomv1alpha1.SupportedTemplate{ ctChain1Name: { - { - Name: "test", - }, - { - Name: "ct0", - }, + {Name: "test"}, + {Name: "ct0"}, + {Name: "ct1"}, + }, + ctChain2Name: { + {Name: "ct1"}, }, - ctChain2Name: {}, } supportedServiceTemplates := map[string][]hmcmirantiscomv1alpha1.SupportedTemplate{ stChain1Name: { - { - Name: "test", - }, - { - Name: "st0", - }, + {Name: "test"}, + {Name: "st0"}, + {Name: "st1"}, + }, + stChain2Name: { + {Name: "st1"}, }, - stChain2Name: {}, } + ctChainUIDs := make(map[string]types.UID) + stChainUIDs := make(map[string]types.UID) + BeforeEach(func() { By("creating the system and test namespaces") for _, ns := range []string{namespace.Name, utils.DefaultSystemNamespace} { @@ -173,6 +172,7 @@ var _ = Describe("Template Chain Controller", func() { Expect(k8sClient.Create(ctx, resource)).To(Succeed()) } } + By("creating the custom resources for the Kind ServiceTemplateChain") for _, chain := range stChainNames { serviceTemplateChain := &hmcmirantiscomv1alpha1.ServiceTemplateChain{} err := k8sClient.Get(ctx, chain, serviceTemplateChain) @@ -197,6 +197,7 @@ var _ = Describe("Template Chain Controller", func() { template.Status.ChartRef = chartRef Expect(k8sClient.Status().Update(ctx, template)).To(Succeed()) } + By("creating the custom resource for the Kind ServiceTemplate") for name, template := range stTemplates { st := &hmcmirantiscomv1alpha1.ServiceTemplate{} err := k8sClient.Get(ctx, types.NamespacedName{Name: name, Namespace: utils.DefaultSystemNamespace}, st) @@ -220,18 +221,9 @@ var _ = Describe("Template Chain Controller", func() { Expect(err).NotTo(HaveOccurred()) By("Cleanup the specific resource instance ClusterTemplateChain") - Expect(mgrClient.Delete(ctx, clusterTemplateChainResource)).To(Succeed()) + Expect(k8sClient.Delete(ctx, clusterTemplateChainResource)).To(Succeed()) - // Wait until the cached client reflects the deletion by checking the deletion timestamp - Eventually(func() bool { - err := mgrClient.Get(ctx, chain, clusterTemplateChainResource) - if err != nil { - return errors.IsNotFound(err) - } - return !clusterTemplateChainResource.DeletionTimestamp.IsZero() - }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ClusterTemplateChain should have a deletion timestamp") - - // Running reconcile to remove the finalizer and delete the ClusterTemplateChain + // Running reconcile to delete the ClusterTemplateChain templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ClusterTemplateKind clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) @@ -239,13 +231,8 @@ var _ = Describe("Template Chain Controller", func() { Eventually(k8sClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, chain, clusterTemplateChainResource).Should(HaveOccurred()) } - // These ClusterTemplates should be deleted once the ClusterTemplateChain is deleted - for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ctTemplates["test"], ctTemplates["ct0"]} { - verifyObjectDeleted(ctx, template.Namespace, template) - } - for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ctTemplates["ct1"], ctTemplates["ct2"]} { - err := k8sClient.Delete(ctx, template) - Expect(crclient.IgnoreNotFound(err)).To(Succeed()) + for _, template := range []*hmcmirantiscomv1alpha1.ClusterTemplate{ctTemplates["test"], ctTemplates["ct0"], ctTemplates["ct2"]} { + Expect(crclient.IgnoreNotFound(k8sClient.Delete(ctx, template))).To(Succeed()) } for _, chain := range stChainNames { @@ -256,30 +243,16 @@ var _ = Describe("Template Chain Controller", func() { By("Cleanup the specific resource instance ServiceTemplateChain") Expect(k8sClient.Delete(ctx, serviceTemplateChainResource)).To(Succeed()) - // Wait until the cached client reflects the deletion by checking the deletion timestamp - Eventually(func() bool { - err := mgrClient.Get(ctx, chain, serviceTemplateChainResource) - if err != nil { - return errors.IsNotFound(err) - } - return !serviceTemplateChainResource.DeletionTimestamp.IsZero() - }, 1*time.Minute, 5*time.Second).Should(BeTrue(), "ServiceTemplateChain should have a deletion timestamp") - - // Running reconcile to remove the finalizer and delete the ServiceTemplateChain + // Running reconcile to delete the ServiceTemplateChain templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ServiceTemplateKind serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) - Eventually(mgrClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, chain, serviceTemplateChainResource).Should(HaveOccurred()) - } - // These ServiceTemplates should be deleted once the ServiceTemplateChain is deleted - for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{stTemplates["test"], stTemplates["st0"]} { - verifyObjectDeleted(ctx, template.Namespace, template) + Eventually(k8sClient.Get, 1*time.Minute, 5*time.Second).WithArguments(ctx, chain, serviceTemplateChainResource).Should(HaveOccurred()) } - for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{stTemplates["st1"], stTemplates["st2"]} { - err := k8sClient.Delete(ctx, template) - Expect(crclient.IgnoreNotFound(err)).To(Succeed()) + for _, template := range []*hmcmirantiscomv1alpha1.ServiceTemplate{stTemplates["test"], stTemplates["st0"], stTemplates["st2"]} { + Expect(crclient.IgnoreNotFound(k8sClient.Delete(ctx, template))).To(Succeed()) } By("Cleanup the namespace") @@ -302,59 +275,83 @@ var _ = Describe("Template Chain Controller", func() { } By("Reconciling the ClusterTemplateChain resources") for _, chain := range ctChainNames { - // First Reconcile should only add finalizer templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ClusterTemplateKind clusterTemplateChainReconciler := &ClusterTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) ctChain := &hmcmirantiscomv1alpha1.ClusterTemplateChain{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, ctChain)).NotTo(HaveOccurred()) - Expect(ctChain.Finalizers).Should(ContainElement(hmcmirantiscomv1alpha1.TemplateChainFinalizer)) - - // Second Reconcile should reconcile the TemplateChain - _, err = clusterTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, ctChain) + ctChainUIDs[ctChain.Name] = ctChain.UID Expect(err).NotTo(HaveOccurred()) } By("Reconciling the ServiceTemplateChain resources") for _, chain := range stChainNames { - // First Reconcile should only add finalizer templateChainReconciler.templateKind = hmcmirantiscomv1alpha1.ServiceTemplateKind serviceTemplateChainReconciler := &ServiceTemplateChainReconciler{TemplateChainReconciler: templateChainReconciler} _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) Expect(err).NotTo(HaveOccurred()) stChain := &hmcmirantiscomv1alpha1.ServiceTemplateChain{} - Expect(k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, stChain)).NotTo(HaveOccurred()) - Expect(stChain.Finalizers).Should(ContainElement(hmcmirantiscomv1alpha1.TemplateChainFinalizer)) - - // Second Reconcile should reconcile the TemplateChain - _, err = serviceTemplateChainReconciler.Reconcile(ctx, reconcile.Request{NamespacedName: chain}) + err = k8sClient.Get(ctx, types.NamespacedName{Namespace: chain.Namespace, Name: chain.Name}, stChain) + stChainUIDs[stChain.Name] = stChain.UID Expect(err).NotTo(HaveOccurred()) } /* - Expected state: + Expected state after Reconcile: * test-chains/test - should be created * test-chains/ct0 - should be created - * test-chains/ct1 - should be deleted * test-chains/ct2 - should be unchanged (unmanaged by HMC) * test-chains/test - should be created * test-chains/st0 - should be created - * test-chains/st1 - should be deleted * test-chains/st2 - should be unchanged (unmanaged by HMC) */ + ct1OwnerRef := metav1.OwnerReference{ + APIVersion: hmcmirantiscomv1alpha1.GroupVersion.String(), + Kind: hmcmirantiscomv1alpha1.ClusterTemplateChainKind, + Name: ctChain1Name, + UID: ctChainUIDs[ctChain1Name], + } + ct2OwnerRef := metav1.OwnerReference{ + APIVersion: hmcmirantiscomv1alpha1.GroupVersion.String(), + Kind: hmcmirantiscomv1alpha1.ClusterTemplateChainKind, + Name: ctChain2Name, + UID: ctChainUIDs[ctChain2Name], + } + st1OwnerRef := metav1.OwnerReference{ + APIVersion: hmcmirantiscomv1alpha1.GroupVersion.String(), + Kind: hmcmirantiscomv1alpha1.ServiceTemplateChainKind, + Name: stChain1Name, + UID: stChainUIDs[stChain1Name], + } + st2OwnerRef := metav1.OwnerReference{ + APIVersion: hmcmirantiscomv1alpha1.GroupVersion.String(), + Kind: hmcmirantiscomv1alpha1.ServiceTemplateChainKind, + Name: stChain2Name, + UID: stChainUIDs[stChain2Name], + } verifyObjectCreated(ctx, namespace.Name, ctTemplates["test"]) + verifyOwnerReferenceExistence(ctTemplates["test"], ct1OwnerRef) verifyObjectCreated(ctx, namespace.Name, ctTemplates["ct0"]) - verifyObjectDeleted(ctx, namespace.Name, ctTemplates["ct1"]) + + verifyObjectCreated(ctx, namespace.Name, ctTemplates["ct1"]) + verifyOwnerReferenceExistence(ctTemplates["ct1"], ct1OwnerRef) + verifyOwnerReferenceExistence(ctTemplates["ct1"], ct2OwnerRef) + verifyObjectUnchanged(ctx, namespace.Name, ctUnmanagedBefore, ctTemplates["ct2"]) verifyObjectCreated(ctx, namespace.Name, stTemplates["test"]) + verifyOwnerReferenceExistence(stTemplates["test"], st1OwnerRef) verifyObjectCreated(ctx, namespace.Name, stTemplates["st0"]) - verifyObjectDeleted(ctx, namespace.Name, stTemplates["st1"]) + + verifyObjectCreated(ctx, namespace.Name, stTemplates["st1"]) + verifyOwnerReferenceExistence(stTemplates["st1"], st1OwnerRef) + verifyOwnerReferenceExistence(stTemplates["st1"], st2OwnerRef) + verifyObjectUnchanged(ctx, namespace.Name, stUnmanagedBefore, stTemplates["st2"]) }) }) @@ -388,6 +385,11 @@ func verifyObjectUnchanged(ctx context.Context, namespace string, oldObj, newObj Expect(newObj.GetLabels()).NotTo(HaveKeyWithValue(hmcmirantiscomv1alpha1.HMCManagedLabelKey, hmcmirantiscomv1alpha1.HMCManagedLabelValue)) } +func verifyOwnerReferenceExistence(obj crclient.Object, ownerRef metav1.OwnerReference) { + By(fmt.Sprintf("Verifying owner reference existence on %s/%s", obj.GetNamespace(), obj.GetName())) + Expect(obj.GetOwnerReferences()).To(ContainElement(ownerRef)) +} + func checkHMCManagedLabelExistence(labels map[string]string) { Expect(labels).To(HaveKeyWithValue(hmcmirantiscomv1alpha1.HMCManagedLabelKey, hmcmirantiscomv1alpha1.HMCManagedLabelValue)) } diff --git a/internal/controller/templatemanagement_controller.go b/internal/controller/templatemanagement_controller.go index 93d08fe5d..bb0d40620 100644 --- a/internal/controller/templatemanagement_controller.go +++ b/internal/controller/templatemanagement_controller.go @@ -231,9 +231,6 @@ func (r *TemplateManagementReconciler) createTemplateChain(ctx context.Context, Labels: map[string]string{ hmc.HMCManagedLabelKey: hmc.HMCManagedLabelValue, }, - Finalizers: []string{ - hmc.TemplateChainFinalizer, - }, } var target templateChain kind := source.GetObjectKind().GroupVersionKind().Kind diff --git a/test/objects/template/template.go b/test/objects/template/template.go index 8518c861d..c26a50668 100644 --- a/test/objects/template/template.go +++ b/test/objects/template/template.go @@ -41,6 +41,10 @@ type ( func NewClusterTemplate(opts ...Opt) *v1alpha1.ClusterTemplate { t := &v1alpha1.ClusterTemplate{ + TypeMeta: metav1.TypeMeta{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: v1alpha1.ClusterTemplateKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultName, Namespace: DefaultNamespace, @@ -56,6 +60,10 @@ func NewClusterTemplate(opts ...Opt) *v1alpha1.ClusterTemplate { func NewServiceTemplate(opts ...Opt) *v1alpha1.ServiceTemplate { t := &v1alpha1.ServiceTemplate{ + TypeMeta: metav1.TypeMeta{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: v1alpha1.ServiceTemplateKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultName, Namespace: DefaultNamespace, @@ -71,6 +79,10 @@ func NewServiceTemplate(opts ...Opt) *v1alpha1.ServiceTemplate { func NewProviderTemplate(opts ...Opt) *v1alpha1.ProviderTemplate { t := &v1alpha1.ProviderTemplate{ + TypeMeta: metav1.TypeMeta{ + APIVersion: v1alpha1.GroupVersion.String(), + Kind: v1alpha1.ProviderTemplateKind, + }, ObjectMeta: metav1.ObjectMeta{ Name: DefaultName, },