Skip to content

Commit

Permalink
Merge pull request #7032 from deefdragon/main
Browse files Browse the repository at this point in the history 
Add check for owner references in backup sync, removing if missing
  • Loading branch information
@blackpiglet
blackpiglet authored Nov 17, 2023
2 parents d42505d + 292aa34 commit c283edf
Show file tree
Hide file tree
Showing 3 changed files with 204 additions and 0 deletions.
1 change: 1 addition & 0 deletions changelogs/unreleased/7032-deefdragon
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Fix #6857. Added check for matching Owner References when synchronizing backups, removing references that are not found/have mismatched uid.
33 changes: 33 additions & 0 deletions pkg/controller/backup_sync_controller.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import (
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/labels"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/sets"
"sigs.k8s.io/controller-runtime/pkg/builder"

Expand Down Expand Up @@ -181,6 +182,9 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request)
}
backup.Labels[velerov1api.StorageLocationLabel] = label.GetValidName(backup.Spec.StorageLocation)

//check for the ownership references. If they do not exist, remove them.
backup.ObjectMeta.OwnerReferences = b.filterBackupOwnerReferences(ctx, backup, log)

// attempt to create backup custom resource via API
err = b.client.Create(ctx, backup, &client.CreateOptions{})
switch {
Expand Down Expand Up @@ -296,6 +300,35 @@ func (b *backupSyncReconciler) Reconcile(ctx context.Context, req ctrl.Request)
return ctrl.Result{}, nil
}

func (b *backupSyncReconciler) filterBackupOwnerReferences(ctx context.Context, backup *velerov1api.Backup, log logrus.FieldLogger) []metav1.OwnerReference {
listedReferences := backup.ObjectMeta.OwnerReferences
foundReferences := make([]metav1.OwnerReference, 0)
for _, v := range listedReferences {
switch v.Kind {
case "Schedule":
schedule := new(velerov1api.Schedule)
err := b.client.Get(ctx, types.NamespacedName{
Name: v.Name,
Namespace: backup.Namespace,
}, schedule)
switch {
case err != nil && apierrors.IsNotFound(err):
log.Warnf("Removing missing schedule ownership reference %s/%s from backup", backup.Namespace, v.Name)
continue
case schedule.UID != v.UID:
log.Warnf("Removing schedule ownership reference with mismatched UIDs. Expected %s, got %s", v.UID, schedule.UID)
continue
case err != nil && !apierrors.IsNotFound(err):
log.WithError(errors.WithStack(err)).Error("Error finding schedule ownership reference, keeping schedule on backup")
}
default:
log.Warnf("Unable to check ownership reference for unknown kind, %s", v.Kind)
}
foundReferences = append(foundReferences, v)
}
return foundReferences
}

// SetupWithManager is used to setup controller and its watching sources.
func (b *backupSyncReconciler) SetupWithManager(mgr ctrl.Manager) error {
backupSyncSource := kube.NewPeriodicalEnqueueSource(
Expand Down
170 changes: 170 additions & 0 deletions pkg/controller/backup_sync_controller_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -725,4 +725,174 @@ var _ = Describe("Backup Sync Reconciler", func() {
Expect(testObjList).To(BeEquivalentTo(locationList))

})

When("testing validateOwnerReferences", func() {

testCases := []struct {
name string
backup *velerov1api.Backup
toCreate []ctrlClient.Object
expectedReferences []metav1.OwnerReference
}{
{
name: "handles empty owner references",
backup: &velerov1api.Backup{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{},
},
},
expectedReferences: []metav1.OwnerReference{},
},
{
name: "handles missing schedule",
backup: &velerov1api.Backup{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "some name",
},
},
},
},
expectedReferences: []metav1.OwnerReference{},
},
{
name: "handles existing reference",
backup: &velerov1api.Backup{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "existing-schedule",
},
},
Namespace: "test-namespace",
},
},
toCreate: []ctrlClient.Object{
&velerov1api.Schedule{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-schedule",
Namespace: "test-namespace",
},
},
},
expectedReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "existing-schedule",
},
},
},
{
name: "handles existing mismatched UID",
backup: &velerov1api.Backup{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "existing-schedule",
UID: "backup-UID",
},
},
Namespace: "test-namespace",
},
},
toCreate: []ctrlClient.Object{
&velerov1api.Schedule{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-schedule",
Namespace: "test-namespace",
UID: "schedule-UID",
},
},
},
expectedReferences: []metav1.OwnerReference{},
},
{
name: "handles multiple references",
backup: &velerov1api.Backup{
ObjectMeta: metav1.ObjectMeta{
OwnerReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "existing-schedule",
UID: "1",
},
{
Kind: "Schedule",
Name: "missing-schedule",
UID: "2",
},
{
Kind: "Schedule",
Name: "mismatched-uid-schedule",
UID: "3",
},
{
Kind: "Schedule",
Name: "another-existing-schedule",
UID: "4",
},
},
Namespace: "test-namespace",
},
},
toCreate: []ctrlClient.Object{
&velerov1api.Schedule{
ObjectMeta: metav1.ObjectMeta{
Name: "existing-schedule",
Namespace: "test-namespace",
UID: "1",
},
},
&velerov1api.Schedule{
ObjectMeta: metav1.ObjectMeta{
Name: "mismatched-uid-schedule",
Namespace: "test-namespace",
UID: "not-3",
},
},
&velerov1api.Schedule{
ObjectMeta: metav1.ObjectMeta{
Name: "another-existing-schedule",
Namespace: "test-namespace",
UID: "4",
},
},
},
expectedReferences: []metav1.OwnerReference{
{
Kind: "Schedule",
Name: "existing-schedule",
UID: "1",
},
{
Kind: "Schedule",
Name: "another-existing-schedule",
UID: "4",
},
},
},
}
for _, test := range testCases {
test := test
It(test.name, func() {
logger := velerotest.NewLogger()
b := backupSyncReconciler{
client: ctrlfake.NewClientBuilder().Build(),
}

//create all required schedules as needed.
for _, creatable := range test.toCreate {
err := b.client.Create(context.Background(), creatable)
Expect(err).ShouldNot(HaveOccurred())
}

references := b.filterBackupOwnerReferences(context.Background(), test.backup, logger)
Expect(references).To(BeEquivalentTo(test.expectedReferences))
})
}
})
})

0 comments on commit c283edf

Please sign in to comment.