Skip to content

Commit

Permalink
Partially replace cleanup with finalizers (#2431)
Browse files Browse the repository at this point in the history
* 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.
  • Loading branch information
weyfonk authored May 29, 2024
1 parent 2ff0128 commit 5bc51b0
Show file tree
Hide file tree
Showing 8 changed files with 477 additions and 113 deletions.
6 changes: 6 additions & 0 deletions dev/remove-fleet
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down
284 changes: 284 additions & 0 deletions e2e/single-cluster/finalizers_test.go
Original file line number Diff line number Diff line change
@@ -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())
})
})
})
56 changes: 33 additions & 23 deletions integrationtests/controller/gitrepo/gitrepo_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package gitrepo

import (
"encoding/hex"
"fmt"
"math/rand"

. "github.com/onsi/ginkgo/v2"
. "github.com/onsi/gomega"

Expand All @@ -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{
Expand All @@ -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
Expand All @@ -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)))
})
})
})
Loading

0 comments on commit 5bc51b0

Please sign in to comment.