Files
velero/pkg/controller/backup_finalizer_controller_test.go
T
Prasad JoshiandGitHub b60fb22929
Run the E2E test on kind / setup-test-matrix (push) Failing after 5s
e2e-test-kind.yaml / extract (push) Failing after 10s
Run the E2E test on kind / get-go-version (push) Failing after 12s
Run the E2E test on kind / build (push) Skipped
Run the E2E test on kind / run-e2e-test (push) Skipped
push.yml / extract (push) Failing after 7s
Main CI / get-go-version (push) Failing after 8s
Main CI / Build (push) Skipped
Scorecard supply-chain security / Scorecard analysis (push) Skipped
fix: defer VGS cleanup until backup finalization (#10612)
* fix: defer VGS cleanup until backup finalization

The CSI PVC backup action deleted the VolumeGroupSnapshot and bound
VolumeGroupSnapshotContent after creating and waiting for all group
member VolumeSnapshots. At that point, Velero had not necessarily
completed processing the member VolumeSnapshots, CSI async operations,
or archive finalization.

Deleting the VGS triggered external-snapshotter member cleanup and could
remove VolumeSnapshot API objects still referenced by the Backup
workflow.

Remove immediate VGS/VGSC deletion from getVolumeSnapshotReference and
retain the group until the Backup reaches a terminal phase. Continue
processing VGS member VolumeSnapshots through the existing normal CSI
finalization path.

Add the velero.io/volume-group-snapshot-cleanup Backup finalizer before
VGS creation. Label VGSC resources with the Backup name and UID, enforce
DeletionPolicy=Retain before cleanup, and delete VGS/VGSC resources from
terminal Backup reconciliation only.

Make cleanup idempotent and retryable, including recovery of labeled
orphan VGSC resources. Remove the Backup finalizer only after VGS/VGSC
cleanup verification succeeds. Block Backup deletion while cleanup is
pending.

Add unit and controller integration tests covering finalizer
registration, VGS creation, terminal cleanup, failed-Backup cleanup,
orphan VGSC cleanup, and finalizer removal.

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* add changelog

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Adress review feedback

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* remove stale VGS finalizer during snapshot restore

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Improve codecoverage

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* explain VGS member snapshot cleanup timing

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Second pass at review comment

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Third pass at review comment

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Dont swallow get errors

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

* Patch backup CR only when annotation is missing

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>

---------

Signed-off-by: Prasad Joshi <prajoshi@redhat.com>
2026-10-01 12:57:32 -04:00

502 lines
19 KiB
Go

/*
Copyright the Velero contributors.
Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
package controller
import (
"bytes"
"fmt"
"io"
"testing"
"time"
volumegroupsnapshotv1 "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumegroupsnapshot/v1"
snapshotv1api "github.com/kubernetes-csi/external-snapshotter/client/v8/apis/volumesnapshot/v1"
"github.com/sirupsen/logrus"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
corev1api "k8s.io/api/core/v1"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/util/wait"
testclocks "k8s.io/utils/clock/testing"
ctrl "sigs.k8s.io/controller-runtime"
kbclient "sigs.k8s.io/controller-runtime/pkg/client"
velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
"github.com/vmware-tanzu/velero/pkg/builder"
"github.com/vmware-tanzu/velero/pkg/features"
"github.com/vmware-tanzu/velero/pkg/itemoperation"
"github.com/vmware-tanzu/velero/pkg/kuberesource"
"github.com/vmware-tanzu/velero/pkg/metrics"
persistencemocks "github.com/vmware-tanzu/velero/pkg/persistence/mocks"
"github.com/vmware-tanzu/velero/pkg/plugin/clientmgmt"
pluginmocks "github.com/vmware-tanzu/velero/pkg/plugin/mocks"
"github.com/vmware-tanzu/velero/pkg/plugin/velero"
velerotest "github.com/vmware-tanzu/velero/pkg/test"
"github.com/vmware-tanzu/velero/pkg/util/csi"
)
func mockBackupFinalizerReconciler(fakeClient kbclient.Client, fakeGlobalClient kbclient.Client, fakeClock *testclocks.FakeClock) (*backupFinalizerReconciler, *fakeBackupper) {
backupper := new(fakeBackupper)
return NewBackupFinalizerReconciler(
fakeClient,
fakeGlobalClient,
fakeClock,
backupper,
func(logrus.FieldLogger) clientmgmt.Manager { return pluginManager },
NewBackupTracker(),
NewFakeSingleObjectBackupStoreGetter(backupStore),
logrus.StandardLogger(),
metrics.NewServerMetrics(),
10*time.Minute,
), backupper
}
func TestBackupFinalizerReconcileRunsTerminalVGSCleanup(t *testing.T) {
backup := builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").
ObjectMeta(builder.WithUID("backup-uid"), builder.WithAnnotations(velerov1api.VolumeGroupSnapshotBackupAnnotation, "true")).
Phase(velerov1api.BackupPhaseCompleted).Result()
client := velerotest.NewFakeControllerRuntimeClient(t, backup)
reconciler := &backupFinalizerReconciler{
client: client,
globalCRClient: velerotest.NewFakeControllerRuntimeClient(t, backup),
log: logrus.New(),
}
_, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{
Namespace: backup.Namespace,
Name: backup.Name,
}})
require.NoError(t, err)
updated := &velerov1api.Backup{}
require.NoError(t, client.Get(t.Context(), kbclient.ObjectKeyFromObject(backup), updated))
require.Empty(t, updated.Finalizers)
require.Equal(t, "true", updated.Annotations[velerov1api.VolumeGroupSnapshotCleanupCompletedAnnotation])
}
func TestBackupFinalizerReconcileCleansVGSAfterFailedBackup(t *testing.T) {
contentName := "group-content"
backup := builder.ForBackup(velerov1api.DefaultNamespace, "backup-failed").
ObjectMeta(builder.WithUID("backup-uid"), builder.WithAnnotations(velerov1api.VolumeGroupSnapshotBackupAnnotation, "true")).
Phase(velerov1api.BackupPhaseFailed).Result()
group := &volumegroupsnapshotv1.VolumeGroupSnapshot{
ObjectMeta: metav1.ObjectMeta{
Name: "group",
Namespace: "app",
UID: "group-uid",
Labels: map[string]string{
velerov1api.BackupNameLabel: "backup-failed",
velerov1api.BackupUIDLabel: "backup-uid",
},
},
Status: &volumegroupsnapshotv1.VolumeGroupSnapshotStatus{
BoundVolumeGroupSnapshotContentName: &contentName,
},
}
content := &volumegroupsnapshotv1.VolumeGroupSnapshotContent{
ObjectMeta: metav1.ObjectMeta{Name: contentName},
Spec: volumegroupsnapshotv1.VolumeGroupSnapshotContentSpec{
DeletionPolicy: snapshotv1api.VolumeSnapshotContentDelete,
VolumeGroupSnapshotRef: corev1api.ObjectReference{
Name: group.Name,
Namespace: group.Namespace,
UID: group.UID,
},
},
}
client := velerotest.NewFakeControllerRuntimeClient(t, backup)
globalClient := velerotest.NewFakeControllerRuntimeClientWithVGS(t, group, content)
reconciler := &backupFinalizerReconciler{
client: client,
globalCRClient: globalClient,
log: logrus.New(),
}
_, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{
Namespace: backup.Namespace,
Name: backup.Name,
}})
require.NoError(t, err)
updated := &velerov1api.Backup{}
require.NoError(t, client.Get(t.Context(), kbclient.ObjectKeyFromObject(backup), updated))
_, err = csi.GetVGS(t.Context(), globalClient, group.Namespace, group.Name)
require.Error(t, err)
_, err = csi.GetVGSC(t.Context(), globalClient, contentName)
require.Error(t, err)
}
func TestBackupFinalizerReconcile(t *testing.T) {
fakeClock := testclocks.NewFakeClock(time.Now())
metav1Now := metav1.NewTime(fakeClock.Now())
defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()
tests := []struct {
name string
backup *velerov1api.Backup
backupOperations []*itemoperation.BackupOperation
backupLocation *velerov1api.BackupStorageLocation
enableCSI bool
expectError bool
expectPhase velerov1api.BackupPhase
expectedCompletedVS int
}{
{
name: "Finalizing backup is completed",
backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-1").
StorageLocation("default").
ObjectMeta(builder.WithUID("foo")).
StartTimestamp(fakeClock.Now()).
Phase(velerov1api.BackupPhaseFinalizing).Result(),
backupLocation: defaultBackupLocation,
expectPhase: velerov1api.BackupPhaseCompleted,
backupOperations: []*itemoperation.BackupOperation{
{
Spec: itemoperation.BackupOperationSpec{
BackupName: "backup-1",
BackupUID: "foo",
BackupItemAction: "foo",
ResourceIdentifier: velero.ResourceIdentifier{
GroupResource: kuberesource.Pods,
Namespace: "ns-1",
Name: "pod-1",
},
PostOperationItems: []velero.ResourceIdentifier{
{
GroupResource: kuberesource.Secrets,
Namespace: "ns-1",
Name: "secret-1",
},
},
OperationID: "operation-1",
},
Status: itemoperation.OperationStatus{
Phase: itemoperation.OperationPhaseCompleted,
Created: &metav1Now,
},
},
},
},
{
name: "FinalizingPartiallyFailed backup is partially failed",
backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-2").
StorageLocation("default").
ObjectMeta(builder.WithUID("foo")).
StartTimestamp(fakeClock.Now()).
Phase(velerov1api.BackupPhaseFinalizingPartiallyFailed).Result(),
backupLocation: defaultBackupLocation,
expectPhase: velerov1api.BackupPhasePartiallyFailed,
backupOperations: []*itemoperation.BackupOperation{
{
Spec: itemoperation.BackupOperationSpec{
BackupName: "backup-2",
BackupUID: "foo",
BackupItemAction: "foo",
ResourceIdentifier: velero.ResourceIdentifier{
GroupResource: kuberesource.Pods,
Namespace: "ns-2",
Name: "pod-2",
},
PostOperationItems: []velero.ResourceIdentifier{
{
GroupResource: kuberesource.Secrets,
Namespace: "ns-2",
Name: "secret-2",
},
},
OperationID: "operation-2",
},
Status: itemoperation.OperationStatus{
Phase: itemoperation.OperationPhaseCompleted,
Created: &metav1Now,
},
},
},
},
{
name: "Test calculate backup.Status.BackupItemOperationsCompleted",
backup: builder.ForBackup(velerov1api.DefaultNamespace, "backup-3").
StorageLocation("default").
ObjectMeta(builder.WithUID("foo")).
StartTimestamp(fakeClock.Now()).
WithStatus(velerov1api.BackupStatus{
StartTimestamp: &metav1Now,
CompletionTimestamp: &metav1Now,
CSIVolumeSnapshotsAttempted: 1,
Phase: velerov1api.BackupPhaseFinalizing,
}).
Result(),
backupLocation: defaultBackupLocation,
enableCSI: true,
expectPhase: velerov1api.BackupPhaseCompleted,
expectedCompletedVS: 1,
backupOperations: []*itemoperation.BackupOperation{
{
Spec: itemoperation.BackupOperationSpec{
BackupName: "backup-3",
BackupUID: "foo",
BackupItemAction: "foo",
ResourceIdentifier: velero.ResourceIdentifier{
GroupResource: kuberesource.VolumeSnapshots,
Namespace: "ns-1",
Name: "vs-1",
},
PostOperationItems: []velero.ResourceIdentifier{
{
GroupResource: kuberesource.Secrets,
Namespace: "ns-1",
Name: "secret-1",
},
},
OperationID: "operation-3",
},
Status: itemoperation.OperationStatus{
Phase: itemoperation.OperationPhaseCompleted,
Created: &metav1Now,
},
},
},
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
if test.backup == nil {
return
}
initObjs := []runtime.Object{}
initObjs = append(initObjs, test.backup)
if test.backupLocation != nil {
initObjs = append(initObjs, test.backupLocation)
}
if test.enableCSI {
features.Enable(velerov1api.CSIFeatureFlag)
defer features.Enable()
}
fakeClient := velerotest.NewFakeControllerRuntimeClient(t, initObjs...)
fakeGlobalClient := velerotest.NewFakeControllerRuntimeClient(t, initObjs...)
reconciler, backupper := mockBackupFinalizerReconciler(fakeClient, fakeGlobalClient, fakeClock)
pluginManager.On("CleanupClients").Return(nil)
backupStore.On("GetBackupItemOperations", test.backup.Name).Return(test.backupOperations, nil)
backupStore.On("GetBackupContents", mock.Anything).Return(io.NopCloser(bytes.NewReader([]byte("hello world"))), nil)
backupStore.On("PutBackupContents", mock.Anything, mock.Anything).Return(nil)
backupStore.On("PutBackupMetadata", mock.Anything, mock.Anything).Return(nil)
backupStore.On("GetBackupVolumeInfos", mock.Anything).Return(nil, nil)
backupStore.On("PutBackupVolumeInfos", mock.Anything, mock.Anything).Return(nil)
pluginManager.On("GetBackupItemActionsV2").Return(nil, nil)
backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
_, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: test.backup.Namespace, Name: test.backup.Name}})
gotErr := err != nil
assert.Equal(t, test.expectError, gotErr)
backupAfter := velerov1api.Backup{}
err = fakeClient.Get(t.Context(), types.NamespacedName{
Namespace: test.backup.Namespace,
Name: test.backup.Name,
}, &backupAfter)
require.NoError(t, err)
assert.Equal(t, test.expectPhase, backupAfter.Status.Phase)
assert.Equal(t, test.expectedCompletedVS, backupAfter.Status.CSIVolumeSnapshotsCompleted)
})
}
}
func TestBackupFinalizerReconcile_PutBackupMetadataFail(t *testing.T) {
origBackoff := objectStorageBackoff
objectStorageBackoff = wait.Backoff{Duration: time.Millisecond, Steps: 2}
t.Cleanup(func() { objectStorageBackoff = origBackoff })
tests := []struct {
name string
initialPhase velerov1api.BackupPhase
}{
{
name: "Finalizing backup stays Finalizing when PutBackupMetadata fails",
initialPhase: velerov1api.BackupPhaseFinalizing,
},
{
name: "FinalizingPartiallyFailed backup stays FinalizingPartiallyFailed when PutBackupMetadata fails",
initialPhase: velerov1api.BackupPhaseFinalizingPartiallyFailed,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
fakeClock := testclocks.NewFakeClock(time.Now())
defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()
backup := builder.ForBackup(velerov1api.DefaultNamespace, "backup-meta-fail").
StorageLocation("default").
ObjectMeta(builder.WithUID("foo")).
StartTimestamp(fakeClock.Now()).
Phase(test.initialPhase).Result()
fakeClient := velerotest.NewFakeControllerRuntimeClient(t, backup, defaultBackupLocation)
fakeGlobalClient := velerotest.NewFakeControllerRuntimeClient(t, backup, defaultBackupLocation)
// Use local mocks to avoid interference with other tests
localPluginManager := &pluginmocks.Manager{}
localBackupStore := &persistencemocks.BackupStore{}
backupper := new(fakeBackupper)
reconciler := NewBackupFinalizerReconciler(
fakeClient,
fakeGlobalClient,
fakeClock,
backupper,
func(logrus.FieldLogger) clientmgmt.Manager { return localPluginManager },
NewBackupTracker(),
NewFakeSingleObjectBackupStoreGetter(localBackupStore),
logrus.StandardLogger(),
metrics.NewServerMetrics(),
10*time.Minute,
)
localPluginManager.On("CleanupClients").Return(nil)
localBackupStore.On("GetBackupItemOperations", backup.Name).Return(nil, nil)
// PutBackupMetadata fails — retry exhausts after DefaultBackoff (5 retries)
localBackupStore.On("PutBackupMetadata", mock.Anything, mock.Anything).Return(fmt.Errorf("object lock prevented upload"))
localBackupStore.On("GetBackupVolumeInfos", mock.Anything).Return(nil, nil)
localBackupStore.On("PutBackupVolumeInfos", mock.Anything, mock.Anything).Return(nil)
_, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}})
require.Error(t, err, "reconcile should return error when PutBackupMetadata fails")
backupAfter := velerov1api.Backup{}
err = fakeClient.Get(t.Context(), types.NamespacedName{
Namespace: backup.Namespace,
Name: backup.Name,
}, &backupAfter)
require.NoError(t, err)
assert.Equal(t, test.initialPhase, backupAfter.Status.Phase,
"backup phase should remain %s when PutBackupMetadata fails", test.initialPhase)
assert.Nil(t, backupAfter.Status.CompletionTimestamp,
"CompletionTimestamp should not be set when PutBackupMetadata fails")
})
}
}
func TestBackupFinalizerReconcile_PutBackupContentsFail(t *testing.T) {
origBackoff := objectStorageBackoff
objectStorageBackoff = wait.Backoff{Duration: time.Millisecond, Steps: 2}
t.Cleanup(func() { objectStorageBackoff = origBackoff })
tests := []struct {
name string
initialPhase velerov1api.BackupPhase
}{
{
name: "Finalizing backup stays Finalizing when PutBackupContents fails",
initialPhase: velerov1api.BackupPhaseFinalizing,
},
{
name: "FinalizingPartiallyFailed backup stays FinalizingPartiallyFailed when PutBackupContents fails",
initialPhase: velerov1api.BackupPhaseFinalizingPartiallyFailed,
},
}
for _, test := range tests {
t.Run(test.name, func(t *testing.T) {
fakeClock := testclocks.NewFakeClock(time.Now())
metav1Now := metav1.NewTime(fakeClock.Now())
defaultBackupLocation := builder.ForBackupStorageLocation(velerov1api.DefaultNamespace, "default").Result()
backup := builder.ForBackup(velerov1api.DefaultNamespace, "backup-contents-fail").
StorageLocation("default").
ObjectMeta(builder.WithUID("foo")).
StartTimestamp(fakeClock.Now()).
Phase(test.initialPhase).Result()
fakeClient := velerotest.NewFakeControllerRuntimeClient(t, backup, defaultBackupLocation)
fakeGlobalClient := velerotest.NewFakeControllerRuntimeClient(t, backup, defaultBackupLocation)
localPluginManager := &pluginmocks.Manager{}
localBackupStore := &persistencemocks.BackupStore{}
backupper := new(fakeBackupper)
reconciler := NewBackupFinalizerReconciler(
fakeClient,
fakeGlobalClient,
fakeClock,
backupper,
func(logrus.FieldLogger) clientmgmt.Manager { return localPluginManager },
NewBackupTracker(),
NewFakeSingleObjectBackupStoreGetter(localBackupStore),
logrus.StandardLogger(),
metrics.NewServerMetrics(),
10*time.Minute,
)
operations := []*itemoperation.BackupOperation{
{
Spec: itemoperation.BackupOperationSpec{
BackupName: "backup-contents-fail",
BackupUID: "foo",
BackupItemAction: "foo",
ResourceIdentifier: velero.ResourceIdentifier{
GroupResource: kuberesource.Pods,
Namespace: "ns-1",
Name: "pod-1",
},
OperationID: "operation-1",
},
Status: itemoperation.OperationStatus{
Phase: itemoperation.OperationPhaseCompleted,
Created: &metav1Now,
},
},
}
localPluginManager.On("CleanupClients").Return(nil)
localPluginManager.On("GetBackupItemActionsV2").Return(nil, nil)
localBackupStore.On("GetBackupItemOperations", backup.Name).Return(operations, nil)
localBackupStore.On("GetBackupContents", mock.Anything).Return(io.NopCloser(bytes.NewReader([]byte("hello world"))), nil)
localBackupStore.On("PutBackupMetadata", mock.Anything, mock.Anything).Return(nil)
// PutBackupContents fails
localBackupStore.On("PutBackupContents", mock.Anything, mock.Anything).Return(fmt.Errorf("object lock prevented upload"))
localBackupStore.On("GetBackupVolumeInfos", mock.Anything).Return(nil, nil)
localBackupStore.On("PutBackupVolumeInfos", mock.Anything, mock.Anything).Return(nil)
backupper.On("FinalizeBackup", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(nil)
_, err := reconciler.Reconcile(t.Context(), ctrl.Request{NamespacedName: types.NamespacedName{Namespace: backup.Namespace, Name: backup.Name}})
require.Error(t, err, "reconcile should return error when PutBackupContents fails")
backupAfter := velerov1api.Backup{}
err = fakeClient.Get(t.Context(), types.NamespacedName{
Namespace: backup.Namespace,
Name: backup.Name,
}, &backupAfter)
require.NoError(t, err)
assert.Equal(t, test.initialPhase, backupAfter.Status.Phase,
"backup phase should remain %s when PutBackupContents fails", test.initialPhase)
assert.Nil(t, backupAfter.Status.CompletionTimestamp,
"CompletionTimestamp should not be set when PutBackupContents fails")
})
}
}