Skip to content

Commit

Permalink
Split needsUpdate into two separate variables and update unit tests
Browse files Browse the repository at this point in the history 
Signed-off-by: Rashmi Gottipati <chowdary.grashmi@gmail.com>
  • Loading branch information
@rashmigottipati
rashmigottipati committed Jun 7, 2021
1 parent ac093b0 commit 60ab6fd
Show file tree
Hide file tree
Showing 3 changed files with 62 additions and 42 deletions.
26 changes: 18 additions & 8 deletions pkg/finalizer/finalizer.go
View file
Expand Up @@ -24,6 +24,12 @@ import (

type finalizers map[string]Finalizer

// Result struct holds Updated and StatusUpdated fields
type Result struct {
Updated bool
StatusUpdated bool
}

// NewFinalizers returns the Finalizers interface
func NewFinalizers() Finalizers {
return finalizers{}
Expand All @@ -37,27 +43,31 @@ func (f finalizers) Register(key string, finalizer Finalizer) error {
return nil
}

func (f finalizers) Finalize(ctx context.Context, obj client.Object) (bool, error) {
var errList []error
needsUpdate := false
func (f finalizers) Finalize(ctx context.Context, obj client.Object) (Result, error) {
var (
res Result
errList []error
)
res.Updated = false
for key, finalizer := range f {
if dt := obj.GetDeletionTimestamp(); dt.IsZero() && !controllerutil.ContainsFinalizer(obj, key) {
controllerutil.AddFinalizer(obj, key)
needsUpdate = true
res.Updated = true
} else if !dt.IsZero() && controllerutil.ContainsFinalizer(obj, key) {
finalizerNeedsUpdate, err := finalizer.Finalize(ctx, obj)
finalizerRes, err := finalizer.Finalize(ctx, obj)
if err != nil {
// Even when the finalizer fails, it may need to signal to update the primary
// object (e.g. it may set a condition and need a status update).
needsUpdate = needsUpdate || finalizerNeedsUpdate
res.Updated = res.Updated || finalizerRes.Updated
res.StatusUpdated = res.StatusUpdated || finalizerRes.StatusUpdated
errList = append(errList, fmt.Errorf("finalizer %q failed: %v", key, err))
} else {
// If the finalizer succeeds, we remove the finalizer from the primary
// object's metadata, so we know it will need an update.
needsUpdate = true
res.Updated = true
controllerutil.RemoveFinalizer(obj, key)
}
}
}
return needsUpdate, utilerrors.NewAggregate(errList)
return res, utilerrors.NewAggregate(errList)
}
76 changes: 43 additions & 33 deletions pkg/finalizer/finalizer_test.go
View file
Expand Up @@ -14,12 +14,12 @@ import (
)

type mockFinalizer struct {
needsUpdate bool
err error
result Result
err error
}

func (f mockFinalizer) Finalize(context.Context, client.Object) (needsUpdate bool, err error) {
return f.needsUpdate, f.err
func (f mockFinalizer) Finalize(context.Context, client.Object) (Result, error) {
return f.result, f.err
}
func TestFinalizer(t *testing.T) {
RegisterFailHandler(Fail)
Expand Down Expand Up @@ -56,24 +56,25 @@ var _ = Describe("TestFinalizer", func() {

})
})

Describe("Finalize", func() {
It("successfully finalizes and returns true for needsUpdate when deletion timestamp is nil and finalizer does not exist", func() {
It("successfully finalizes and returns true for Updated when deletion timestamp is nil and finalizer does not exist", func() {
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
Expect(err).To(BeNil())

pod.DeletionTimestamp = nil
pod.Finalizers = []string{}

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(needsUpdate).To(BeTrue())
Expect(result.Updated).To(BeTrue())
// when deletion timestamp is nil and finalizer is not present, the registered finalizer would be added to the obj
Expect(len(pod.Finalizers)).To(Equal(1))
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer"))

})

It("successfully finalizes and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
It("successfully finalizes and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() {
now := metav1.Now()
pod.DeletionTimestamp = &now

Expand All @@ -82,37 +83,37 @@ var _ = Describe("TestFinalizer", func() {

pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"}

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(needsUpdate).To(BeTrue())
Expect(result.Updated).To(BeTrue())
// finalizer will be removed from the obj upon successful finalization
Expect(len(pod.Finalizers)).To(Equal(0))
})

It("should return no error and return false for needsUpdate when deletion timestamp is nil and finalizer doesn't exist", func() {
It("should return no error and return false for Updated when deletion timestamp is nil and finalizer doesn't exist", func() {
pod.DeletionTimestamp = nil
pod.Finalizers = []string{}

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(needsUpdate).To(BeFalse())
Expect(result.Updated).To(BeFalse())
Expect(len(pod.Finalizers)).To(Equal(0))

})

It("should return no error and return false for needsUpdate when deletion timestamp is not nil and the finalizer doesn't exist", func() {
It("should return no error and return false for Updated when deletion timestamp is not nil and the finalizer doesn't exist", func() {
now := metav1.Now()
pod.DeletionTimestamp = &now
pod.Finalizers = []string{}

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(needsUpdate).To(BeFalse())
Expect(result.Updated).To(BeFalse())
Expect(len(pod.Finalizers)).To(Equal(0))

})

It("successfully finalizes multiple finalizers and returns true for needsUpdate when deletion timestamp is not nil and the finalizer exists", func() {
It("successfully finalizes multiple finalizers and returns true for Updated when deletion timestamp is not nil and the finalizer exists", func() {
now := metav1.Now()
pod.DeletionTimestamp = &now

Expand All @@ -124,32 +125,35 @@ var _ = Describe("TestFinalizer", func() {

pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer", "finalizers.sigs.k8s.io/newtestfinalizer"}

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(needsUpdate).To(BeTrue())
Expect(result.Updated).To(BeTrue())
Expect(result.StatusUpdated).To(BeFalse())
Expect(len(pod.Finalizers)).To(Equal(0))
})

It("should return needsUpdate as false and a non-nil error", func() {
It("should return result as false and a non-nil error", func() {
now := metav1.Now()
pod.DeletionTimestamp = &now
pod.Finalizers = []string{"finalizers.sigs.k8s.io/testfinalizer"}

f.needsUpdate = false
f.result.Updated = false
f.result.StatusUpdated = false
f.err = fmt.Errorf("finalizer failed for %q", pod.Finalizers[0])

err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer", f)
Expect(err).To(BeNil())

needsUpdate, err := finalizers.Finalize(context.TODO(), pod)
result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
Expect(needsUpdate).To(BeFalse())
Expect(result.Updated).To(BeFalse())
Expect(result.StatusUpdated).To(BeFalse())
Expect(len(pod.Finalizers)).To(Equal(1))
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer"))
})

It("should return expected needsUpdate and error values when registering multiple finalizers", func() {
It("should return expected result values and error values when registering multiple finalizers", func() {
now := metav1.Now()
pod.DeletionTimestamp = &now
pod.Finalizers = []string{
Expand All @@ -159,45 +163,51 @@ var _ = Describe("TestFinalizer", func() {
}

// registering multiple finalizers with different return values
// test for needsUpdate as true, and nil error
f.needsUpdate = true
// test for Updated as true, and nil error
f.result.Updated = true
f.result.StatusUpdated = false
f.err = nil
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer1", f)
Expect(err).To(BeNil())

result, err := finalizers.Finalize(context.TODO(), pod)
Expect(err).To(BeNil())
Expect(result).To(BeTrue())
Expect(result.Updated).To(BeTrue())
Expect(result.StatusUpdated).To(BeFalse())
// `finalizers.sigs.k8s.io/testfinalizer1` will be removed from the list
// of finalizers, so length will be 2.
Expect(len(pod.Finalizers)).To(Equal(2))
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))

// test for needsUpdate as false, and non-nil error
f.needsUpdate = false
// test for Updated and StatusUpdated as false, and non-nil error
f.result.Updated = false
f.result.StatusUpdated = false
f.err = fmt.Errorf("finalizer failed")
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer2", f)
Expect(err).To(BeNil())

result, err = finalizers.Finalize(context.TODO(), pod)
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
Expect(result).To(BeFalse())
Expect(result.Updated).To(BeFalse())
Expect(result.StatusUpdated).To(BeFalse())
Expect(len(pod.Finalizers)).To(Equal(2))
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))

// test for needsUpdate as true, and non-nil error
f.needsUpdate = true
// test for result as true, and non-nil error
f.result.Updated = true
f.result.StatusUpdated = true
f.err = fmt.Errorf("finalizer failed")
err = finalizers.Register("finalizers.sigs.k8s.io/testfinalizer3", f)
Expect(err).To(BeNil())

result, err = finalizers.Finalize(context.TODO(), pod)
Expect(err).ToNot(BeNil())
Expect(err.Error()).To(ContainSubstring("finalizer failed"))
Expect(result).To(BeTrue())
Expect(result.Updated).To(BeTrue())
Expect(result.StatusUpdated).To(BeTrue())
Expect(len(pod.Finalizers)).To(Equal(2))
Expect(pod.Finalizers[0]).To(Equal("finalizers.sigs.k8s.io/testfinalizer2"))
Expect(pod.Finalizers[1]).To(Equal("finalizers.sigs.k8s.io/testfinalizer3"))
Expand Down
2 changes: 1 addition & 1 deletion pkg/finalizer/types.go
View file
Expand Up @@ -30,7 +30,7 @@ type Registerer interface {
// deletion timestamp being set and return an indication of whether the
// obj needs an update or not
type Finalizer interface {
Finalize(context.Context, client.Object) (needsUpdate bool, err error)
Finalize(context.Context, client.Object) (Result, error)
}

// Finalizers implements Registerer and Finalizer to finalize all registered
Expand Down

0 comments on commit 60ab6fd

Please sign in to comment.