From 5bc51b0fab630bb54de72764f21832c53bdb77b9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Corentin=20N=C3=A9au?= Date: Wed, 29 May 2024 09:07:07 +0200 Subject: [PATCH] Partially replace cleanup with finalizers (#2431) * Add finalizers to bundle deployments, bundles and gitrepos This prevents those resources from being deleted when no Fleet controller is running. The bundle reconciler creates bundle deployments with finalizers, which are removed when bundle deployments are purged by either of the bundle or `GitRepo` reconcilers. * Patch Fleet agent bundle and bundle deployment in deletion script This enables Fleet to be uninstalled. Without patching those resources manually, deleting namespace `fleet-local` would hang because of finalizers still being present on them. * Remove cleanup for bundles and bundle deployments Cleanup is now handled via finalizers for those resources. * Remove cleanup for imagescan When the GitRepo reconciler processes a GitRepo deletion, imagescans are deleted. Finalizers prevent a GitRepo from being deleted outside of that reconciler, therefore orphan imagescans should no longer exist. --- dev/remove-fleet | 6 + e2e/single-cluster/finalizers_test.go | 284 ++++++++++++++++++ .../controller/gitrepo/gitrepo_test.go | 56 ++-- .../cleanup/controllers/cleanup/controller.go | 62 +--- .../cleanup/controllers/controllers.go | 5 - .../reconciler/bundle_controller.go | 60 +++- .../reconciler/bundledeployment_controller.go | 27 ++ .../reconciler/gitrepo_controller.go | 90 +++++- 8 files changed, 477 insertions(+), 113 deletions(-) create mode 100644 e2e/single-cluster/finalizers_test.go diff --git a/dev/remove-fleet b/dev/remove-fleet index f4e86fe0e2..b585387a36 100755 --- a/dev/remove-fleet +++ b/dev/remove-fleet @@ -13,6 +13,12 @@ helm uninstall -n cattle-fleet-system fleet-crd kubectl delete ns cattle-fleet-system --now kubectl delete ns cattle-fleet-clusters-system --now + +# This will prevent deletion of namespace fleet-local from hanging +bd_ns=$(kubectl get ns -l fleet.cattle.io/managed=true --no-headers -o=jsonpath={.items[0].metadata.name}) +kubectl patch bundledeployment fleet-agent-local -n $bd_ns -p '{"metadata":{"finalizers":[]}}' --type=merge +kubectl patch bundle fleet-agent-local -n fleet-local -p '{"metadata":{"finalizers":[]}}' --type=merge + kubectl delete ns fleet-local --now kubectl delete ns -l "fleet.cattle.io/managed=true" diff --git a/e2e/single-cluster/finalizers_test.go b/e2e/single-cluster/finalizers_test.go new file mode 100644 index 0000000000..815933eaa4 --- /dev/null +++ b/e2e/single-cluster/finalizers_test.go @@ -0,0 +1,284 @@ +package singlecluster_test + +import ( + "errors" + "fmt" + "math/rand" + "strings" + "time" + + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" + + "github.com/rancher/fleet/e2e/testenv" + "github.com/rancher/fleet/e2e/testenv/kubectl" +) + +var _ = Describe("Deleting a resource with finalizers", func() { + var ( + k kubectl.Command + gitrepoName string + path string + r = rand.New(rand.NewSource(GinkgoRandomSeed())) + targetNamespace string + ) + + BeforeEach(func() { + k = env.Kubectl.Namespace(env.Namespace) + targetNamespace = testenv.NewNamespaceName("target", r) + }) + + JustBeforeEach(func() { + gitrepoName = testenv.RandomFilename("finalizers-test", r) + path = "simple-chart" + }) + + AfterEach(func() { + _, err := k.Namespace("cattle-fleet-system").Run( + "scale", + "deployment", + "fleet-controller", + "--replicas=1", + "--timeout=5s", + ) + Expect(err).ToNot(HaveOccurred()) + + _, _ = k.Delete("gitrepo", gitrepoName) + _, _ = k.Delete("bundle", fmt.Sprintf("%s-%s", gitrepoName, path)) + }) + + When("deleting an existing GitRepo", func() { + JustBeforeEach(func() { + By("creating a GitRepo") + err := testenv.CreateGitRepo(k, targetNamespace, gitrepoName, "master", "", path) + Expect(err).ToNot(HaveOccurred()) + }) + + It("updates the deployment", func() { + By("checking the bundle and bundle deployment exist") + Eventually(func() string { + out, _ := k.Get("bundles") + return out + }).Should(ContainSubstring(gitrepoName)) + + Eventually(func() string { + out, _ := k.Get("bundledeployments", "-A") + return out + }).Should(ContainSubstring(gitrepoName)) + + By("scaling down the Fleet controller to 0 replicas") + _, err := k.Namespace("cattle-fleet-system").Run( + "scale", + "deployment", + "fleet-controller", + "--replicas=0", + "--timeout=5s", + ) + Expect(err).ToNot(HaveOccurred()) + + By("deleting the GitRepo") + out, err := k.Delete("gitrepo", gitrepoName, "--timeout=2s") + Expect(err).To(HaveOccurred()) + Expect(out).To(ContainSubstring("timed out")) + + By("checking that the gitrepo still exists and has a deletion timestamp") + out, err = k.Get( + "gitrepo", + gitrepoName, + "-o=jsonpath={range .items[*]}{.metadata.deletionTimestamp}", + ) + Expect(err).ToNot(HaveOccurred()) + Expect(out).ToNot(BeZero()) + + By("checking that the bundle and bundle deployment still exist") + out, err = k.Get("bundles") + Expect(err).ToNot(HaveOccurred()) + Expect(out).To(ContainSubstring(gitrepoName)) + + out, err = k.Get("bundledeployments", "-A") + Expect(err).ToNot(HaveOccurred()) + Expect(out).To(ContainSubstring(gitrepoName)) + + By("checking that the auxiliary resources still exist") + serviceAccountName := fmt.Sprintf("git-%s", gitrepoName) + Consistently(func() error { + out, _ := k.Get("configmaps") + if !strings.Contains(out, fmt.Sprintf("%s-config", gitrepoName)) { + return errors.New("configmap not found") + } + + out, _ = k.Get("serviceaccounts") + if !strings.Contains(out, serviceAccountName) { + return errors.New("serviceaccount not found") + } + + out, _ = k.Get("roles") + if !strings.Contains(out, serviceAccountName) { + return errors.New("role not found") + } + + out, _ = k.Get("rolebindings") + if !strings.Contains(out, serviceAccountName) { + return errors.New("rolebinding not found") + } + + return nil + }, 2*time.Second, 100*time.Millisecond).ShouldNot(HaveOccurred()) + + By("deleting the GitRepo once the controller runs again") + _, err = k.Namespace("cattle-fleet-system").Run( + "scale", + "deployment", + "fleet-controller", + "--replicas=1", + "--timeout=5s", + ) + Expect(err).ToNot(HaveOccurred()) + + _, err = k.Delete("gitrepo", gitrepoName) + Expect(err).NotTo(HaveOccurred()) + + // These resources should be deleted when the GitRepo is deleted. + By("checking that the auxiliary resources don't exist anymore") + Eventually(func() error { + out, _ := k.Get("configmaps") + if strings.Contains(out, fmt.Sprintf("%s-config", gitrepoName)) { + return errors.New("configmap not expected") + } + + out, _ = k.Get("serviceaccounts") + if strings.Contains(out, serviceAccountName) { + return errors.New("serviceaccount not expected") + } + + out, _ = k.Get("roles") + if strings.Contains(out, serviceAccountName) { + return errors.New("role not expected") + } + + out, _ = k.Get("rolebindings") + if strings.Contains(out, serviceAccountName) { + return errors.New("rolebinding not expected") + } + + return nil + }).ShouldNot(HaveOccurred()) + }) + }) + + When("deleting an existing bundle", func() { + JustBeforeEach(func() { + By("creating a GitRepo") + err := testenv.CreateGitRepo(k, targetNamespace, gitrepoName, "master", "", path) + Expect(err).ToNot(HaveOccurred()) + }) + + It("updates the deployment", func() { + By("checking the bundle and bundle deployment exist") + Eventually(func() string { + out, _ := k.Get("bundles") + return out + }).Should(ContainSubstring(gitrepoName)) + + Eventually(func() string { + out, _ := k.Get("bundledeployments", "-A") + return out + }).Should(ContainSubstring(gitrepoName)) + + By("scaling down the Fleet controller to 0 replicas") + _, err := k.Namespace("cattle-fleet-system").Run( + "scale", + "deployment", + "fleet-controller", + "--replicas=0", + "--timeout=5s", + ) + Expect(err).ToNot(HaveOccurred()) + + By("deleting the bundle") + out, err := k.Delete( + "bundle", + fmt.Sprintf("%s-%s", gitrepoName, path), + "--timeout=2s", + ) + Expect(err).To(HaveOccurred()) + Expect(out).To(ContainSubstring("timed out")) + + By("checking that the bundle still exists and has a deletion timestamp") + out, err = k.Get( + "bundle", + fmt.Sprintf("%s-%s", gitrepoName, path), + "-o=jsonpath={range .items[*]}{.metadata.deletionTimestamp}", + ) + Expect(err).ToNot(HaveOccurred()) + Expect(out).ToNot(BeZero()) + + By("checking that the bundle deployment still exists") + out, err = k.Get("bundledeployments", "-A") + Expect(err).ToNot(HaveOccurred()) + Expect(out).To(ContainSubstring(gitrepoName)) + }) + }) + + When("deleting an existing bundledeployment", func() { + JustBeforeEach(func() { + By("creating a GitRepo") + err := testenv.CreateGitRepo(k, targetNamespace, gitrepoName, "master", "", path) + Expect(err).ToNot(HaveOccurred()) + }) + + It("updates the deployment", func() { + By("checking the bundle and bundle deployment exist") + Eventually(func() string { + out, _ := k.Get("bundles") + return out + }).Should(ContainSubstring(gitrepoName)) + + Eventually(func() string { + out, _ := k.Get("bundledeployments", "-A") + return out + }).Should(ContainSubstring(gitrepoName)) + + By("scaling down the Fleet controller to 0 replicas") + _, err := k.Namespace("cattle-fleet-system").Run( + "scale", + "deployment", + "fleet-controller", + "--replicas=0", + "--timeout=5s", + ) + Expect(err).ToNot(HaveOccurred()) + + By("deleting the bundledeployment") + bdNamespace, err := k.Get( + "ns", + "-o=jsonpath={.items[?(@."+ + `metadata.annotations.fleet\.cattle\.io/cluster-namespace=="fleet-local"`+ + ")].metadata.name}", + ) + Expect(err).ToNot(HaveOccurred()) + + // Deleting a bundle deployment should hang for as long as it has a finalizer + out, err := env.Kubectl.Namespace(bdNamespace).Delete( + "bundledeployment", + fmt.Sprintf("%s-%s", gitrepoName, path), + "--timeout=2s", + ) + Expect(err).To(HaveOccurred()) + Expect(out).To(ContainSubstring("timed out")) + + By("checking that the bundledeployment still exists and has a deletion timestamp") + out, err = env.Kubectl.Namespace(bdNamespace).Get( + "bundledeployment", + fmt.Sprintf("%s-%s", gitrepoName, path), + "-o=jsonpath={range .items[*]}{.metadata.deletionTimestamp}", + ) + Expect(err).ToNot(HaveOccurred()) + Expect(out).ToNot(BeZero()) + + By("checking that the configmap created by the bundle deployment still exists") + _, err = k.Namespace(targetNamespace).Get("configmap", "test-simple-chart-config") + Expect(err).ToNot(HaveOccurred()) + }) + }) +}) diff --git a/integrationtests/controller/gitrepo/gitrepo_test.go b/integrationtests/controller/gitrepo/gitrepo_test.go index 707ca00179..3e4d6c1da2 100644 --- a/integrationtests/controller/gitrepo/gitrepo_test.go +++ b/integrationtests/controller/gitrepo/gitrepo_test.go @@ -1,6 +1,10 @@ package gitrepo import ( + "encoding/hex" + "fmt" + "math/rand" + . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" @@ -13,25 +17,29 @@ import ( "k8s.io/apimachinery/pkg/types" ) -var _ = Describe("GitRepo", Ordered, func() { - BeforeAll(func() { +var _ = Describe("GitRepo", func() { + var ( + gitrepo *v1alpha1.GitRepo + gitrepoName string + ) + + BeforeEach(func() { var err error namespace, err = utils.NewNamespaceName() Expect(err).ToNot(HaveOccurred()) ns := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespace}} Expect(k8sClient.Create(ctx, ns)).ToNot(HaveOccurred()) - DeferCleanup(func() { - Expect(k8sClient.Delete(ctx, ns)).ToNot(HaveOccurred()) - }) - }) - - var gitrepo *v1alpha1.GitRepo + p := make([]byte, 12) + s := rand.New(rand.NewSource(GinkgoRandomSeed())) // nolint:gosec // non-crypto usage + if _, err := s.Read(p); err != nil { + panic(err) + } + gitrepoName = fmt.Sprintf("test-gitrepo-%.12s", hex.EncodeToString(p)) - BeforeEach(func() { gitrepo = &v1alpha1.GitRepo{ ObjectMeta: metav1.ObjectMeta{ - Name: "test-gitrepo", + Name: gitrepoName, Namespace: namespace, }, Spec: v1alpha1.GitRepoSpec{ @@ -54,7 +62,10 @@ var _ = Describe("GitRepo", Ordered, func() { Expect(gitrepo.Spec.PollingInterval).To(BeNil()) Eventually(func() bool { - ns := types.NamespacedName{Name: "git-test-gitrepo", Namespace: namespace} + ns := types.NamespacedName{ + Name: fmt.Sprintf("git-%s", gitrepoName), + Namespace: namespace, + } if err := k8sClient.Get(ctx, ns, &corev1.ServiceAccount{}); err != nil { return false @@ -73,19 +84,18 @@ var _ = Describe("GitRepo", Ordered, func() { It("updates the gitrepo status", func() { org := gitrepo.ResourceVersion Eventually(func() bool { - _ = k8sClient.Get(ctx, types.NamespacedName{Name: "test-gitrepo", Namespace: namespace}, gitrepo) - return gitrepo.ResourceVersion > org + _ = k8sClient.Get(ctx, types.NamespacedName{Name: gitrepoName, Namespace: namespace}, gitrepo) + return gitrepo.ResourceVersion > org && + gitrepo.Status.Display.ReadyBundleDeployments == "0/0" && + gitrepo.Status.Display.State == "GitUpdating" && + !gitrepo.Status.Display.Error && + len(gitrepo.Status.Conditions) == 2 && + gitrepo.Status.Conditions[0].Type == "Ready" && + string(gitrepo.Status.Conditions[0].Status) == "True" && + gitrepo.Status.Conditions[1].Type == "Accepted" && + string(gitrepo.Status.Conditions[1].Status) == "True" && + gitrepo.Status.DeepCopy().ObservedGeneration == int64(1) }).Should(BeTrue()) - - Expect(gitrepo.Status.Display.ReadyBundleDeployments).To(Equal("0/0")) - Expect(gitrepo.Status.Display.State).To(Equal("GitUpdating")) - Expect(gitrepo.Status.Display.Error).To(BeFalse()) - Expect(gitrepo.Status.Conditions).To(HaveLen(2)) - Expect(gitrepo.Status.Conditions[0].Type).To(Equal("Ready")) - Expect(string(gitrepo.Status.Conditions[0].Status)).To(Equal("True")) - Expect(gitrepo.Status.Conditions[1].Type).To(Equal("Accepted")) - Expect(string(gitrepo.Status.Conditions[1].Status)).To(Equal("True")) - Expect(gitrepo.Status.DeepCopy().ObservedGeneration).To(Equal(int64(1))) }) }) }) diff --git a/internal/cmd/controller/cleanup/controllers/cleanup/controller.go b/internal/cmd/controller/cleanup/controllers/cleanup/controller.go index 6a6d891f72..4085cf12ff 100644 --- a/internal/cmd/controller/cleanup/controllers/cleanup/controller.go +++ b/internal/cmd/controller/cleanup/controllers/cleanup/controller.go @@ -23,40 +23,23 @@ type handler struct { apply apply.Apply clusters fleetcontrollers.ClusterCache namespaces corecontrollers.NamespaceClient - bundles fleetcontrollers.BundleController - images fleetcontrollers.ImageScanController - gitRepo fleetcontrollers.GitRepoCache } func Register(ctx context.Context, apply apply.Apply, secrets corecontrollers.SecretController, serviceAccount corecontrollers.ServiceAccountController, - bundledeployment fleetcontrollers.BundleDeploymentController, role rbaccontrollers.RoleController, roleBinding rbaccontrollers.RoleBindingController, clusterRole rbaccontrollers.ClusterRoleController, clusterRoleBinding rbaccontrollers.ClusterRoleBindingController, namespaces corecontrollers.NamespaceController, - clusterCache fleetcontrollers.ClusterCache, - bundles fleetcontrollers.BundleController, - images fleetcontrollers.ImageScanController, - gitRepo fleetcontrollers.GitRepoCache) { + clusterCache fleetcontrollers.ClusterCache) { h := &handler{ apply: apply, clusters: clusterCache, namespaces: namespaces, - bundles: bundles, - images: images, - gitRepo: gitRepo, } - bundledeployment.OnChange(ctx, "managed-cleanup", func(_ string, obj *fleet.BundleDeployment) (*fleet.BundleDeployment, error) { - if obj == nil { - return nil, nil - } - return obj, h.cleanup(obj) - }) - clusterRole.OnChange(ctx, "managed-cleanup", func(_ string, obj *rbacv1.ClusterRole) (*rbacv1.ClusterRole, error) { if obj == nil { return nil, nil @@ -100,8 +83,6 @@ func Register(ctx context.Context, apply apply.Apply, }) namespaces.OnChange(ctx, "managed-namespace-cleanup", h.cleanupNamespace) - bundles.OnChange(ctx, "bundle-orphan", h.OnPurgeOrphaned) - images.OnChange(ctx, "imagescan-orphan", h.OnPurgeOrphanedImageScan) } func (h *handler) cleanupNamespace(key string, obj *corev1.Namespace) (*corev1.Namespace, error) { @@ -136,44 +117,3 @@ func (h *handler) cleanup(obj runtime.Object) error { } return err } - -func (h *handler) OnPurgeOrphaned(key string, bundle *fleet.Bundle) (*fleet.Bundle, error) { - if bundle == nil { - return bundle, nil - } - - repo := bundle.Labels[fleet.RepoLabel] - if repo == "" { - return nil, nil - } - logrus.Debugf("OnPurgeOrphaned for bundle '%s' change, checking if gitrepo still exists", bundle.Name) - - _, err := h.gitRepo.Get(bundle.Namespace, repo) - if apierrors.IsNotFound(err) { - logrus.Infof("OnPurgeOrphaned for bundle '%s', gitrepo not found, delete bundle", bundle.Name) - return nil, h.bundles.Delete(bundle.Namespace, bundle.Name, nil) - } else if err != nil { - return nil, err - } - - return bundle, nil -} - -func (h *handler) OnPurgeOrphanedImageScan(key string, image *fleet.ImageScan) (*fleet.ImageScan, error) { - if image == nil || image.DeletionTimestamp != nil { - return image, nil - } - logrus.Debugf("OnPurgeOrphanedImageScan for image '%s' change, checking if gitrepo still exists", image.Name) - - repo := image.Spec.GitRepoName - - _, err := h.gitRepo.Get(image.Namespace, repo) - if apierrors.IsNotFound(err) { - logrus.Infof("OnPurgeOrphaned for imagescan '%s', gitrepo not found, delete imagescan", image.Name) - return nil, h.images.Delete(image.Namespace, image.Name, nil) - } else if err != nil { - return nil, err - } - - return image, nil -} diff --git a/internal/cmd/controller/cleanup/controllers/controllers.go b/internal/cmd/controller/cleanup/controllers/controllers.go index ad69f1852b..0a81cf4299 100644 --- a/internal/cmd/controller/cleanup/controllers/controllers.go +++ b/internal/cmd/controller/cleanup/controllers/controllers.go @@ -55,7 +55,6 @@ func Register(ctx context.Context, appCtx *AppContext) error { appCtx.RBAC.RoleBinding(), appCtx.RBAC.ClusterRole(), appCtx.RBAC.ClusterRoleBinding(), - appCtx.Bundle(), appCtx.ClusterRegistrationToken(), appCtx.ClusterRegistration(), appCtx.ClusterGroup(), @@ -63,16 +62,12 @@ func Register(ctx context.Context, appCtx *AppContext) error { appCtx.Core.Namespace()), appCtx.Core.Secret(), appCtx.Core.ServiceAccount(), - appCtx.BundleDeployment(), appCtx.RBAC.Role(), appCtx.RBAC.RoleBinding(), appCtx.RBAC.ClusterRole(), appCtx.RBAC.ClusterRoleBinding(), appCtx.Core.Namespace(), appCtx.Cluster().Cache(), - appCtx.Bundle(), - appCtx.ImageScan(), - appCtx.GitRepo().Cache(), ) if err := appCtx.Start(ctx); err != nil { diff --git a/internal/cmd/controller/reconciler/bundle_controller.go b/internal/cmd/controller/reconciler/bundle_controller.go index 2ef71cfe98..0633df1daf 100644 --- a/internal/cmd/controller/reconciler/bundle_controller.go +++ b/internal/cmd/controller/reconciler/bundle_controller.go @@ -28,6 +28,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) +const bundleFinalizer = "fleet.cattle.io/bundle-finalizer" + type BundleQuery interface { // BundlesForCluster is used to map from a cluster to bundles BundlesForCluster(context.Context, *fleet.Cluster) ([]*fleet.Bundle, []*fleet.Bundle, error) @@ -64,18 +66,55 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr ctx = log.IntoContext(ctx, logger) bundle := &fleet.Bundle{} - err := r.Get(ctx, req.NamespacedName, bundle) - if apierrors.IsNotFound(err) { - metrics.BundleCollector.Delete(req.Name, req.Namespace) + if err := r.Get(ctx, req.NamespacedName, bundle); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + if bundle.DeletionTimestamp.IsZero() { + if !controllerutil.ContainsFinalizer(bundle, bundleFinalizer) { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, req.NamespacedName, bundle); err != nil { + return err + } + + controllerutil.AddFinalizer(bundle, bundleFinalizer) - logger.V(1).Info("Bundle not found, purging bundle deployments") - if err := purgeBundleDeployments(ctx, r.Client, req.NamespacedName); err != nil { - return ctrl.Result{}, client.IgnoreNotFound(err) + return r.Update(ctx, bundle) + }) + + if client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } } + } else { + if controllerutil.ContainsFinalizer(bundle, bundleFinalizer) { + metrics.BundleCollector.Delete(req.Name, req.Namespace) + + logger.V(1).Info("Bundle not found, purging bundle deployments") + if err := purgeBundleDeployments(ctx, r.Client, req.NamespacedName); err != nil { + // A bundle deployment may have been purged by the GitRepo reconciler, hence we ignore + // not-found errors here. + return ctrl.Result{}, client.IgnoreNotFound(err) + } + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, req.NamespacedName, bundle); err != nil { + return err + } + + controllerutil.RemoveFinalizer(bundle, bundleFinalizer) + + return r.Update(ctx, bundle) + }) + + if client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } + } + return ctrl.Result{}, nil - } else if err != nil { - return ctrl.Result{}, err } + logger.V(1).Info("Reconciling bundle, checking targets, calculating changes, building objects", "generation", bundle.Generation, "observedGeneration", bundle.Status.ObservedGeneration) manifest := manifest.FromBundle(bundle) @@ -150,6 +189,11 @@ func (r *BundleReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // NOTE we don't use the existing BundleDeployment, we discard annotations, status, etc // copy labels from Bundle as they might have changed bd := target.BundleDeployment() + + // No need to check the deletion timestamp here before adding a finalizer, since the bundle has just + // been created. + controllerutil.AddFinalizer(bd, bundleDeploymentFinalizer) + updated := bd.DeepCopy() op, err := controllerutil.CreateOrUpdate(ctx, r.Client, bd, func() error { bd.Spec = updated.Spec diff --git a/internal/cmd/controller/reconciler/bundledeployment_controller.go b/internal/cmd/controller/reconciler/bundledeployment_controller.go index 76ebbfc673..af4adbc6f2 100644 --- a/internal/cmd/controller/reconciler/bundledeployment_controller.go +++ b/internal/cmd/controller/reconciler/bundledeployment_controller.go @@ -18,11 +18,14 @@ import ( "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/log" "sigs.k8s.io/controller-runtime/pkg/predicate" ) +const bundleDeploymentFinalizer = "fleet.cattle.io/bundle-deployment-finalizer" + // BundleDeploymentReconciler reconciles a BundleDeployment object type BundleDeploymentReconciler struct { client.Client @@ -43,10 +46,34 @@ func (r *BundleDeploymentReconciler) Reconcile(ctx context.Context, req ctrl.Req bd := &fleet.BundleDeployment{} err := r.Get(ctx, req.NamespacedName, bd) + if err != nil { metrics.BundleDeploymentCollector.Delete(req.Name, req.Namespace) return ctrl.Result{}, client.IgnoreNotFound(err) } + + // The bundle reconciler takes care of adding the finalizer when creating a bundle deployment + if !bd.DeletionTimestamp.IsZero() { + if controllerutil.ContainsFinalizer(bd, bundleDeploymentFinalizer) { + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + t := &fleet.BundleDeployment{} + err := r.Get(ctx, req.NamespacedName, t) + if err != nil { + return err + } + + controllerutil.RemoveFinalizer(t, bundleDeploymentFinalizer) + + return r.Update(ctx, bd) + }) + if err != nil { + return ctrl.Result{}, err + } + } + + return ctrl.Result{}, nil + } + // increased log level, this triggers a lot logger.V(4).Info("Reconciling bundledeployment, updating display status field", "oldDisplay", bd.Status.Display) diff --git a/internal/cmd/controller/reconciler/gitrepo_controller.go b/internal/cmd/controller/reconciler/gitrepo_controller.go index 9923a45872..f7136fe868 100644 --- a/internal/cmd/controller/reconciler/gitrepo_controller.go +++ b/internal/cmd/controller/reconciler/gitrepo_controller.go @@ -40,6 +40,8 @@ import ( "sigs.k8s.io/controller-runtime/pkg/predicate" ) +const gitRepoFinalizer = "fleet.cattle.io/gitrepo-finalizer" + // GitRepoReconciler reconciles a GitRepo object type GitRepoReconciler struct { client.Client @@ -61,27 +63,59 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct logger := log.FromContext(ctx).WithName("gitrepo") gitrepo := &fleet.GitRepo{} - err := r.Get(ctx, req.NamespacedName, gitrepo) - if client.IgnoreNotFound(err) != nil { - return ctrl.Result{}, err + if err := r.Get(ctx, req.NamespacedName, gitrepo); err != nil { + return ctrl.Result{}, client.IgnoreNotFound(err) } - // Clean up - if apierrors.IsNotFound(err) { - logger.V(1).Info("Gitrepo deleted, deleting bundle, image scans") + if gitrepo.DeletionTimestamp.IsZero() { + if !controllerutil.ContainsFinalizer(gitrepo, gitRepoFinalizer) { + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, req.NamespacedName, gitrepo); err != nil { + return err + } + + controllerutil.AddFinalizer(gitrepo, gitRepoFinalizer) - metrics.GitRepoCollector.Delete(req.NamespacedName.Name, req.NamespacedName.Namespace) + return r.Update(ctx, gitrepo) + }) - if err := purgeBundles(ctx, r.Client, req.NamespacedName); err != nil { - return ctrl.Result{}, err + if client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } } + } else { + if controllerutil.ContainsFinalizer(gitrepo, gitRepoFinalizer) { + // Clean up + logger.V(1).Info("Gitrepo deleted, deleting bundle, image scans") + + metrics.GitRepoCollector.Delete(req.NamespacedName.Name, req.NamespacedName.Namespace) + + if err := purgeBundles(ctx, r.Client, req.NamespacedName); err != nil { + return ctrl.Result{}, err + } + + // remove the job scheduled by imagescan, if any + _ = r.Scheduler.DeleteJob(imagescan.GitCommitKey(req.Namespace, req.Name)) + + if err := purgeImageScans(ctx, r.Client, req.NamespacedName); err != nil { + return ctrl.Result{}, err + } + + err := retry.RetryOnConflict(retry.DefaultRetry, func() error { + if err := r.Get(ctx, req.NamespacedName, gitrepo); err != nil { + return err + } - // remove the job scheduled by imagescan, if any - _ = r.Scheduler.DeleteJob(imagescan.GitCommitKey(req.Namespace, req.Name)) + controllerutil.RemoveFinalizer(gitrepo, gitRepoFinalizer) - if err := purgeImageScans(ctx, r.Client, req.NamespacedName); err != nil { - return ctrl.Result{}, err + return r.Update(ctx, gitrepo) + }) + + if client.IgnoreNotFound(err) != nil { + return ctrl.Result{}, err + } } + return ctrl.Result{}, nil } @@ -97,7 +131,7 @@ func (r *GitRepoReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct // Restrictions / Overrides // AuthorizeAndAssignDefaults mutates GitRepo and it returns nil on error oldStatus := gitrepo.Status.DeepCopy() - gitrepo, err = grutil.AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo) + gitrepo, err := grutil.AuthorizeAndAssignDefaults(ctx, r.Client, gitrepo) if err != nil { return ctrl.Result{}, r.updateErrorStatus(ctx, req.NamespacedName, *oldStatus, err) } @@ -325,7 +359,7 @@ func purgeBundles(ctx context.Context, c client.Client, gitrepo types.Namespaced for _, bundle := range bundles.Items { err := c.Delete(ctx, &bundle) - if err != nil { + if client.IgnoreNotFound(err) != nil { return err } @@ -340,11 +374,35 @@ func purgeBundles(ctx context.Context, c client.Client, gitrepo types.Namespaced func purgeBundleDeployments(ctx context.Context, c client.Client, bundle types.NamespacedName) error { list := &fleet.BundleDeploymentList{} - err := c.List(ctx, list, client.MatchingLabels{fleet.BundleLabel: bundle.Name, fleet.BundleNamespaceLabel: bundle.Namespace}) + err := c.List( + ctx, + list, + client.MatchingLabels{ + fleet.BundleLabel: bundle.Name, + fleet.BundleNamespaceLabel: bundle.Namespace, + }, + ) if err != nil { return err } for _, bd := range list.Items { + if controllerutil.ContainsFinalizer(&bd, bundleDeploymentFinalizer) { // nolint: gosec // does not store pointer + nn := types.NamespacedName{Namespace: bd.Namespace, Name: bd.Name} + err = retry.RetryOnConflict(retry.DefaultRetry, func() error { + t := &fleet.BundleDeployment{} + if err := c.Get(ctx, nn, t); err != nil { + return err + } + + controllerutil.RemoveFinalizer(t, bundleDeploymentFinalizer) + + return c.Update(ctx, t) + }) + if err != nil { + return err + } + } + err := c.Delete(ctx, &bd) if err != nil { return err