Skip to content

fix(controller): propagate context during backup request preparation #10621

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
abhayrajjais01 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(controller): propagate context during backup request preparation #10621

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelogs/unreleased/10621-abhayrajjais01
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Propagate caller context during backup request preparation and snapshot location validation in backup controller
16 changes: 8 additions & 8 deletions pkg/controller/backup_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel
// TODO(2.0) b.defaultBackupLocation will be deprecated
request.Spec.StorageLocation = b.defaultBackupLocation

locationList, err := storage.ListBackupStorageLocations(context.Background(), b.kbClient, request.Namespace)
locationList, err := storage.ListBackupStorageLocations(ctx, b.kbClient, request.Namespace)
if err == nil {
for _, location := range locationList.Items {
if location.Spec.Default {
Expand All @@ -476,7 +476,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel

// get the storage location, and store the BackupStorageLocation API obj on the request
storageLocation := &velerov1api.BackupStorageLocation{}
if err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{
if err := b.kbClient.Get(ctx, kbclient.ObjectKey{
Namespace: request.Namespace,
Name: request.Spec.StorageLocation,
}, storageLocation); err != nil {
Expand Down Expand Up @@ -514,7 +514,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel

// validate and get the backup's VolumeSnapshotLocations, and store the
// VolumeSnapshotLocation API objs on the request
if locs, errs := b.validateAndGetSnapshotLocations(request.Backup); len(errs) > 0 {
if locs, errs := b.validateAndGetSnapshotLocations(ctx, request.Backup); len(errs) > 0 {
request.Status.ValidationErrors = append(request.Status.ValidationErrors, errs...)
} else {
request.Spec.VolumeSnapshotLocations = nil
Expand All @@ -536,7 +536,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel
// Add namespaces with label velero.io/exclude-from-backup=true into request.Spec.ExcludedNamespaces
// Essentially, adding the label velero.io/exclude-from-backup=true to a namespace would be equivalent to setting spec.ExcludedNamespaces
namespaces := corev1api.NamespaceList{}
if err := b.kbClient.List(context.Background(), &namespaces, kbclient.MatchingLabels{velerov1api.ExcludeFromBackupLabel: "true"}); err == nil {
if err := b.kbClient.List(ctx, &namespaces, kbclient.MatchingLabels{velerov1api.ExcludeFromBackupLabel: "true"}); err == nil {
for _, ns := range namespaces.Items {
request.Spec.ExcludedNamespaces = append(request.Spec.ExcludedNamespaces, ns.Name)
}
Expand Down Expand Up @@ -778,14 +778,14 @@ func mergeNamespacesByLabel(
// it will automatically be used)
//
// if backup has snapshotVolume disabled then it returns empty VSL
func (b *backupReconciler) validateAndGetSnapshotLocations(backup *velerov1api.Backup) (map[string]*velerov1api.VolumeSnapshotLocation, []string) {
func (b *backupReconciler) validateAndGetSnapshotLocations(ctx context.Context, backup *velerov1api.Backup) (map[string]*velerov1api.VolumeSnapshotLocation, []string) {
errors := []string{}
providerLocations := make(map[string]*velerov1api.VolumeSnapshotLocation)

for _, locationName := range backup.Spec.VolumeSnapshotLocations {
// validate each locationName exists as a VolumeSnapshotLocation
location := &velerov1api.VolumeSnapshotLocation{}
if err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: backup.Namespace, Name: locationName}, location); err != nil {
if err := b.kbClient.Get(ctx, kbclient.ObjectKey{Namespace: backup.Namespace, Name: locationName}, location); err != nil {
if apierrors.IsNotFound(err) {
errors = append(errors, fmt.Sprintf("a VolumeSnapshotLocation CRD for the location %s with the name specified in the backup spec needs to be created before this snapshot can be executed. Error: %v", locationName, err))
} else {
Expand All @@ -811,7 +811,7 @@ func (b *backupReconciler) validateAndGetSnapshotLocations(backup *velerov1api.B
return nil, errors
}
volumeSnapshotLocations := &velerov1api.VolumeSnapshotLocationList{}
err := b.kbClient.List(context.Background(), volumeSnapshotLocations, &kbclient.ListOptions{Namespace: backup.Namespace, LabelSelector: labels.Everything()})
err := b.kbClient.List(ctx, volumeSnapshotLocations, &kbclient.ListOptions{Namespace: backup.Namespace, LabelSelector: labels.Everything()})
if err != nil {
errors = append(errors, fmt.Sprintf("error listing volume snapshot locations: %v", err))
return nil, errors
Expand Down Expand Up @@ -841,7 +841,7 @@ func (b *backupReconciler) validateAndGetSnapshotLocations(backup *velerov1api.B
continue
}
location := &velerov1api.VolumeSnapshotLocation{}
if err := b.kbClient.Get(context.Background(), kbclient.ObjectKey{Namespace: backup.Namespace, Name: defaultLocation}, location); err != nil {
if err := b.kbClient.Get(ctx, kbclient.ObjectKey{Namespace: backup.Namespace, Name: defaultLocation}, location); err != nil {
errors = append(errors, fmt.Sprintf("error getting volume snapshot location named %s: %v", defaultLocation, err))
continue
}
Expand Down
29 changes: 28 additions & 1 deletion pkg/controller/backup_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ package controller

import (
"bytes"
"context"
"fmt"
"io"
"reflect"
Expand Down Expand Up @@ -46,6 +47,7 @@ import (
ctrl "sigs.k8s.io/controller-runtime"
kbclient "sigs.k8s.io/controller-runtime/pkg/client"
fakeClient "sigs.k8s.io/controller-runtime/pkg/client/fake"
"sigs.k8s.io/controller-runtime/pkg/client/interceptor"

"github.com/vmware-tanzu/velero/internal/resourcepolicies"
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
Expand Down Expand Up @@ -2335,7 +2337,7 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
require.NoError(t, c.kbClient.Create(t.Context(), location))
}

providerLocations, errs := c.validateAndGetSnapshotLocations(backup)
providerLocations, errs := c.validateAndGetSnapshotLocations(t.Context(), backup)
if test.expectedSuccess {
for _, err := range errs {
require.NoError(t, errors.New(err), "validateAndGetSnapshotLocations unexpected error: %v", err)
Expand All @@ -2355,6 +2357,31 @@ func TestValidateAndGetSnapshotLocations(t *testing.T) {
}
})
}

t.Run("returns error when context is canceled", func(t *testing.T) {
backup := defaultBackup().Phase(velerov1api.BackupPhaseNew).VolumeSnapshotLocations("aws-us-west-1").Result()
ctx, cancel := context.WithCancel(t.Context())
cancel()

crClient := velerotest.NewFakeControllerRuntimeClientBuilder(t).
WithInterceptorFuncs(interceptor.Funcs{
Get: func(ctx context.Context, client kbclient.WithWatch, key kbclient.ObjectKey, obj kbclient.Object, opts ...kbclient.GetOption) error {
if err := ctx.Err(); err != nil {
return err
}
return client.Get(ctx, key, obj, opts...)
},
}).Build()

c := &backupReconciler{
logger: velerotest.NewLogger(),
kbClient: crClient,
}

_, errs := c.validateAndGetSnapshotLocations(ctx, backup)
require.NotEmpty(t, errs)
require.Contains(t, errs[0], "context canceled")
})
}

// Test_getLastSuccessBySchedule verifies that the getLastSuccessBySchedule helper function correctly returns
Expand Down
Loading