Skip to content

Commit

Permalink
move clone populator cleanup function to planner
Browse files Browse the repository at this point in the history 
other review comments

verifier pod should bount readonly

Signed-off-by: Michael Henriksen <mhenriks@redhat.com>
  • Loading branch information
@mhenriks
mhenriks committed May 18, 2023
1 parent a5ac4cb commit aa2dd2f
Show file tree
Hide file tree
Showing 8 changed files with 120 additions and 99 deletions.
1 change: 1 addition & 0 deletions pkg/controller/clone/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ go_library(
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/meta:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library",
"//vendor/k8s.io/client-go/tools/record:go_default_library",
Expand Down
29 changes: 29 additions & 0 deletions pkg/controller/clone/planner.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import (
corev1 "k8s.io/api/core/v1"
"k8s.io/apimachinery/pkg/api/meta"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/types"
"k8s.io/client-go/tools/record"
"sigs.k8s.io/controller-runtime/pkg/client"
Expand Down Expand Up @@ -58,6 +59,13 @@ var coreTypesCreated = []client.Object{
&corev1.Pod{},
}

// all types that may have been created
var listTypesToDelete = []client.ObjectList{
&corev1.PersistentVolumeClaimList{},
&corev1.PodList{},
&snapshotv1.VolumeSnapshotList{},
}

// AddCoreWatches watches "core" types
func (p *Planner) AddCoreWatches(log logr.Logger) error {
p.watchMutex.Lock()
Expand Down Expand Up @@ -129,6 +137,27 @@ func (p *Planner) Plan(ctx context.Context, args *PlanArgs) ([]Phase, error) {
return nil, fmt.Errorf("unknown strategy/source %s", string(args.Strategy))
}

// Cleanup cleans up after a clone op
func (p *Planner) Cleanup(ctx context.Context, log logr.Logger, owner client.Object) error {
log.V(3).Info("Cleaning up for obj", "obj", owner)

for _, lt := range listTypesToDelete {
ls, err := labels.Parse(fmt.Sprintf("%s=%s", p.OwnershipLabel, string(owner.GetUID())))
if err != nil {
return err
}

lo := &client.ListOptions{
LabelSelector: ls,
}
if err := cc.BulkDeleteResources(ctx, p.Client, lt, lo); err != nil {
return err
}
}

return nil
}

func (p *Planner) watchSnapshots(ctx context.Context, log logr.Logger) error {
p.watchMutex.Lock()
defer p.watchMutex.Unlock()
Expand Down
55 changes: 53 additions & 2 deletions pkg/controller/clone/planner_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ import (
snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand All @@ -47,6 +48,7 @@ var _ = Describe("Planner test", func() {
namespace = "ns"
sourceName = "source"
targetName = "target"
ownerLabel = "label"
)

var (
Expand Down Expand Up @@ -75,7 +77,7 @@ var _ = Describe("Planner test", func() {

return &Planner{
RootObjectType: &corev1.PersistentVolumeClaimList{},
OwnershipLabel: "label",
OwnershipLabel: ownerLabel,
UIDField: "uid",
Image: "image",
PullPolicy: corev1.PullIfNotPresent,
Expand Down Expand Up @@ -369,7 +371,7 @@ var _ = Describe("Planner test", func() {
})
})

Context("Paln tests", func() {
Context("Plan tests", func() {
cdiConfig := &cdiv1.CDIConfig{
ObjectMeta: metav1.ObjectMeta{
Name: "config",
Expand Down Expand Up @@ -510,4 +512,53 @@ var _ = Describe("Planner test", func() {
validateRebindPhase(planner, args, plan[2])
})
})

Context("Cleanup tests", func() {
tempResources := func() []runtime.Object {
target := createTargetClaim()
return []runtime.Object{
&corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpClaim",
Labels: map[string]string{
ownerLabel: string(target.UID),
},
},
},
&corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpPod",
Labels: map[string]string{
ownerLabel: string(target.UID),
},
},
},
&snapshotv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpSnapshot",
Labels: map[string]string{
ownerLabel: string(target.UID),
},
},
},
}
}

It("should cleanup tmp resources", func() {
tempObjs := tempResources()
target := createTargetClaim()
planner := createPlanner(tempObjs...)
err := planner.Cleanup(context.Background(), log, target)
Expect(err).ToNot(HaveOccurred())
for _, r := range tempResources() {
co := r.(client.Object)
err = planner.Client.Get(context.Background(), client.ObjectKeyFromObject(co), co)
Expect(err).To(HaveOccurred())
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
}
})
})
})
3 changes: 1 addition & 2 deletions pkg/controller/populators/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,14 +19,12 @@ go_library(
"//pkg/util:go_default_library",
"//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library",
"//vendor/github.com/go-logr/logr:go_default_library",
"//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
"//vendor/github.com/pkg/errors:go_default_library",
"//vendor/k8s.io/api/core/v1:go_default_library",
"//vendor/k8s.io/api/storage/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/equality:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/runtime/schema:go_default_library",
"//vendor/k8s.io/apimachinery/pkg/types:go_default_library",
Expand Down Expand Up @@ -56,6 +54,7 @@ go_test(
"//pkg/feature-gates:go_default_library",
"//staging/src/kubevirt.io/containerized-data-importer-api/pkg/apis/core/v1beta1:go_default_library",
"//tests/reporters:go_default_library",
"//vendor/github.com/go-logr/logr:go_default_library",
"//vendor/github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1:go_default_library",
"//vendor/github.com/onsi/ginkgo:go_default_library",
"//vendor/github.com/onsi/ginkgo/extensions/table:go_default_library",
Expand Down
30 changes: 2 additions & 28 deletions pkg/controller/populators/clone-populator.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,11 +22,9 @@ import (
"time"

"github.com/go-logr/logr"
snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
corev1 "k8s.io/api/core/v1"
apiequality "k8s.io/apimachinery/pkg/api/equality"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/labels"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/controller"
"sigs.k8s.io/controller-runtime/pkg/manager"
Expand Down Expand Up @@ -63,6 +61,7 @@ const (
type Planner interface {
ChooseStrategy(context.Context, *clone.ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error)
Plan(context.Context, *clone.PlanArgs) ([]clone.Phase, error)
Cleanup(context.Context, logr.Logger, client.Object) error
}

// ClonePopulatorReconciler reconciles PVCs with VolumeCloneSources
Expand Down Expand Up @@ -260,7 +259,7 @@ func (r *ClonePopulatorReconciler) reconcilePending(ctx context.Context, log log

func (r *ClonePopulatorReconciler) reconcileDone(ctx context.Context, log logr.Logger, pvc *corev1.PersistentVolumeClaim) (reconcile.Result, error) {
log.V(3).Info("executing cleanup")
if err := r.cleanup(ctx, log, pvc); err != nil {
if err := r.planner.Cleanup(ctx, log, pvc); err != nil {
return reconcile.Result{}, err
}

Expand All @@ -269,31 +268,6 @@ func (r *ClonePopulatorReconciler) reconcileDone(ctx context.Context, log logr.L
return reconcile.Result{}, r.client.Update(ctx, pvc)
}

func (r *ClonePopulatorReconciler) cleanup(ctx context.Context, log logr.Logger, owner client.Object) error {
log.V(3).Info("Cleaning up for obj", "obj", owner)
listTypes := []client.ObjectList{
&corev1.PersistentVolumeClaimList{},
&snapshotv1.VolumeSnapshotList{},
&corev1.PodList{},
}

for _, lt := range listTypes {
ls, err := labels.Parse(fmt.Sprintf("%s=%s", LabelOwnedByUID, string(owner.GetUID())))
if err != nil {
return err
}

lo := &client.ListOptions{
LabelSelector: ls,
}
if err := cc.BulkDeleteResources(ctx, r.client, lt, lo); err != nil {
return err
}
}

return nil
}

func (r *ClonePopulatorReconciler) initTargetClaim(ctx context.Context, pvc *corev1.PersistentVolumeClaim, vcs *cdiv1.VolumeCloneSource, cs cdiv1.CDICloneStrategy) (bool, error) {
claimCpy := pvc.DeepCopy()
clone.AddCommonClaimLabels(claimCpy)
Expand Down
72 changes: 12 additions & 60 deletions pkg/controller/populators/clone-populator_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ import (
"context"
"fmt"

"github.com/go-logr/logr"
. "github.com/onsi/ginkgo"
. "github.com/onsi/gomega"

snapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v6/apis/volumesnapshot/v1"
corev1 "k8s.io/api/core/v1"
storagev1 "k8s.io/api/storage/v1"
extv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -131,38 +131,6 @@ var _ = Describe("Clone populator tests", func() {
return target
}

tempResources := func() []runtime.Object {
return []runtime.Object{
&corev1.PersistentVolumeClaim{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpClaim",
Labels: map[string]string{
LabelOwnedByUID: "uid",
},
},
},
&corev1.Pod{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpPod",
Labels: map[string]string{
LabelOwnedByUID: "uid",
},
},
},
&snapshotv1.VolumeSnapshot{
ObjectMeta: metav1.ObjectMeta{
Namespace: namespace,
Name: "tmpSnapshot",
Labels: map[string]string{
LabelOwnedByUID: "uid",
},
},
},
}
}

getTarget := func(c client.Client) *corev1.PersistentVolumeClaim {
target, _ := targetAndDataSource()
result := &corev1.PersistentVolumeClaim{}
Expand Down Expand Up @@ -354,38 +322,16 @@ var _ = Describe("Clone populator tests", func() {
Expect(pvc.Annotations[AnnClonePhase]).To(Equal("Succeeded"))
})

It("should remove finalizer and cleanup when succeeded", func() {
tempObjs := tempResources()
It("should remove finalizer and call cleanup when succeeded", func() {
target := succeededTarget()
allResources := append(tempObjs, target)
reconciler := createClonePopulatorReconciler(allResources...)
reconciler := createClonePopulatorReconciler(target)
fp := &fakePlanner{}
reconciler.planner = fp
result, err := reconciler.Reconcile(context.Background(), nn)
isDefaultResult(result, err)
pvc := getTarget(reconciler.client)
Expect(pvc.Finalizers).ToNot(ContainElement(cloneFinalizer))
for _, r := range tempResources() {
co := r.(client.Object)
err = reconciler.client.Get(context.Background(), client.ObjectKeyFromObject(co), co)
Expect(err).To(HaveOccurred())
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
}
})

It("should remove finalizer and cleanup when deleted while in progress", func() {
tempObjs := tempResources()
target, _ := initinializedTargetAndDataSource()
ts := metav1.Now()
target.DeletionTimestamp = &ts
allResources := append(tempObjs, target)
reconciler := createClonePopulatorReconciler(allResources...)
result, err := reconciler.Reconcile(context.Background(), nn)
isDefaultResult(result, err)
for _, r := range tempResources() {
co := r.(client.Object)
err = reconciler.client.Get(context.Background(), client.ObjectKeyFromObject(co), co)
Expect(err).To(HaveOccurred())
Expect(k8serrors.IsNotFound(err)).To(BeTrue())
}
Expect(fp.cleanupCalled).To(BeTrue())
})
})

Expand All @@ -395,6 +341,7 @@ type fakePlanner struct {
chooseStrategyError error
planResult []clone.Phase
planError error
cleanupCalled bool
}

func (p *fakePlanner) ChooseStrategy(ctx context.Context, args *clone.ChooseStrategyArgs) (*cdiv1.CDICloneStrategy, error) {
Expand All @@ -405,6 +352,11 @@ func (p *fakePlanner) Plan(ctx context.Context, args *clone.PlanArgs) ([]clone.P
return p.planResult, p.planError
}

func (p *fakePlanner) Cleanup(ctx context.Context, log logr.Logger, owner client.Object) error {
p.cleanupCalled = true
return nil
}

type fakePhase struct {
name string
result *reconcile.Result
Expand Down

0 comments on commit aa2dd2f

Please sign in to comment.