From 71ddeefcd63763ac3ec35f5c47cc58810746e738 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 29 Jan 2026 16:25:52 -0500 Subject: [PATCH] Fix VolumePolicy PVC phase condition filter for unbound PVCs Use typed error approach: Make GetPVForPVC return ErrPVNotFoundForPVC when PV is not expected to be found (unbound PVC), then use errors.Is to check for this error type. When a matching policy exists (e.g., pvcPhase: [Pending, Lost] with action: skip), apply the action without error. When no policy matches, return the original error to preserve default behavior. Changes: - Add ErrPVNotFoundForPVC sentinel error to pvc_pv.go - Update ShouldPerformSnapshot to handle unbound PVCs with policies - Update ShouldPerformFSBackup to handle unbound PVCs with policies - Update item_backupper.go to handle Lost PVCs in tracking functions - Remove checkPVCOnlySkip helper (no longer needed) - Update tests to reflect new behavior Signed-off-by: Tiger Kaovilai Co-Authored-By: Claude Opus 4.5 --- changelogs/unreleased/9508-kaovilai | 1 + internal/volumehelper/volume_policy_helper.go | 37 ++- .../volumehelper/volume_policy_helper_test.go | 311 +++++++++++++++++- pkg/backup/item_backupper.go | 31 +- pkg/backup/item_backupper_test.go | 225 +++++++++++++ pkg/podvolume/backupper.go | 8 +- pkg/podvolume/backupper_test.go | 188 ++++++++++- pkg/util/kube/pvc_pv.go | 12 +- 8 files changed, 780 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/9508-kaovilai diff --git a/changelogs/unreleased/9508-kaovilai b/changelogs/unreleased/9508-kaovilai new file mode 100644 index 000000000..3c224aee3 --- /dev/null +++ b/changelogs/unreleased/9508-kaovilai @@ -0,0 +1 @@ +Fix VolumePolicy PVC phase condition filter for unbound PVCs (#9507) diff --git a/internal/volumehelper/volume_policy_helper.go b/internal/volumehelper/volume_policy_helper.go index 160a2005e..a47f7be83 100644 --- a/internal/volumehelper/volume_policy_helper.go +++ b/internal/volumehelper/volume_policy_helper.go @@ -134,6 +134,7 @@ func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group pv := new(corev1api.PersistentVolume) var err error + var pvNotFoundErr error if groupResource == kuberesource.PersistentVolumeClaims { if err = runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), &pvc); err != nil { v.logger.WithError(err).Error("fail to convert unstructured into PVC") @@ -142,8 +143,10 @@ func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group pv, err = kubeutil.GetPVForPVC(pvc, v.client) if err != nil { - v.logger.WithError(err).Errorf("fail to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) - return false, err + // Any error means PV not available - save to return later if no policy matches + v.logger.Debugf("PV not found for PVC %s: %v", pvc.Namespace+"/"+pvc.Name, err) + pvNotFoundErr = err + pv = nil } } @@ -158,7 +161,7 @@ func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group vfd := resourcepolicies.NewVolumeFilterData(pv, nil, pvc) action, err := v.volumePolicy.GetMatchAction(vfd) if err != nil { - v.logger.WithError(err).Errorf("fail to get VolumePolicy match action for PV %s", pv.Name) + v.logger.WithError(err).Errorf("fail to get VolumePolicy match action for %+v", vfd) return false, err } @@ -167,15 +170,21 @@ func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group // If there is no match action, go on to the next check. if action != nil { if action.Type == resourcepolicies.Snapshot { - v.logger.Infof(fmt.Sprintf("performing snapshot action for pv %s", pv.Name)) + v.logger.Infof("performing snapshot action for %+v", vfd) return true, nil } else { - v.logger.Infof("Skip snapshot action for pv %s as the action type is %s", pv.Name, action.Type) + v.logger.Infof("Skip snapshot action for %+v as the action type is %s", vfd, action.Type) return false, nil } } } + // If resource is PVC, and PV is nil (e.g., Pending/Lost PVC with no matching policy), return the original error + if groupResource == kuberesource.PersistentVolumeClaims && pv == nil && pvNotFoundErr != nil { + v.logger.WithError(pvNotFoundErr).Errorf("fail to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) + return false, pvNotFoundErr + } + // If this PV is claimed, see if we've already taken a (pod volume backup) // snapshot of the contents of this PV. If so, don't take a snapshot. if pv.Spec.ClaimRef != nil { @@ -209,7 +218,7 @@ func (v *volumeHelperImpl) ShouldPerformSnapshot(obj runtime.Unstructured, group return true, nil } - v.logger.Infof(fmt.Sprintf("skipping snapshot action for pv %s possibly due to no volume policy setting or snapshotVolumes is false", pv.Name)) + v.logger.Infof("skipping snapshot action for pv %s possibly due to no volume policy setting or snapshotVolumes is false", pv.Name) return false, nil } @@ -219,6 +228,7 @@ func (v volumeHelperImpl) ShouldPerformFSBackup(volume corev1api.Volume, pod cor return false, nil } + var pvNotFoundErr error if v.volumePolicy != nil { var resource any var err error @@ -230,10 +240,13 @@ func (v volumeHelperImpl) ShouldPerformFSBackup(volume corev1api.Volume, pod cor v.logger.WithError(err).Errorf("fail to get PVC for pod %s", pod.Namespace+"/"+pod.Name) return false, err } - resource, err = kubeutil.GetPVForPVC(pvc, v.client) + pvResource, err := kubeutil.GetPVForPVC(pvc, v.client) if err != nil { - v.logger.WithError(err).Errorf("fail to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) - return false, err + // Any error means PV not available - save to return later if no policy matches + v.logger.Debugf("PV not found for PVC %s: %v", pvc.Namespace+"/"+pvc.Name, err) + pvNotFoundErr = err + } else { + resource = pvResource } } @@ -260,6 +273,12 @@ func (v volumeHelperImpl) ShouldPerformFSBackup(volume corev1api.Volume, pod cor return false, nil } } + + // If no policy matched and PV was not found, return the original error + if pvNotFoundErr != nil { + v.logger.WithError(pvNotFoundErr).Errorf("fail to get PV for PVC %s", pvc.Namespace+"/"+pvc.Name) + return false, pvNotFoundErr + } } if v.shouldPerformFSBackupLegacy(volume, pod) { diff --git a/internal/volumehelper/volume_policy_helper_test.go b/internal/volumehelper/volume_policy_helper_test.go index 8d6073c2b..5e52ae73b 100644 --- a/internal/volumehelper/volume_policy_helper_test.go +++ b/internal/volumehelper/volume_policy_helper_test.go @@ -286,7 +286,7 @@ func TestVolumeHelperImpl_ShouldPerformSnapshot(t *testing.T) { expectedErr: false, }, { - name: "PVC not having PV, return false and error case PV not found", + name: "PVC not having PV, return false and error when no matching policy", inputObj: builder.ForPersistentVolumeClaim("default", "example-pvc").StorageClass("gp2-csi").Result(), groupResource: kuberesource.PersistentVolumeClaims, resourcePolicies: &resourcepolicies.ResourcePolicies{ @@ -1234,3 +1234,312 @@ func TestNewVolumeHelperImplWithCache_UsesCache(t *testing.T) { require.NoError(t, err) require.False(t, shouldSnapshot, "Expected snapshot to be skipped due to fs-backup selection via cache") } + +// TestVolumeHelperImpl_ShouldPerformSnapshot_UnboundPVC tests that Pending and Lost PVCs with +// phase-based skip policies don't cause errors when GetPVForPVC would fail. +func TestVolumeHelperImpl_ShouldPerformSnapshot_UnboundPVC(t *testing.T) { + testCases := []struct { + name string + inputPVC *corev1api.PersistentVolumeClaim + resourcePolicies *resourcepolicies.ResourcePolicies + shouldSnapshot bool + expectedErr bool + }{ + { + name: "Pending PVC with phase-based skip policy should not error and return false", + inputPVC: builder.ForPersistentVolumeClaim("ns", "pvc-pending"). + StorageClass("non-existent-class"). + Phase(corev1api.ClaimPending). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldSnapshot: false, + expectedErr: false, + }, + { + name: "Pending PVC without matching skip policy should error (no PV)", + inputPVC: builder.ForPersistentVolumeClaim("ns", "pvc-pending-no-policy"). + StorageClass("non-existent-class"). + Phase(corev1api.ClaimPending). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "storageClass": []string{"gp2-csi"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldSnapshot: false, + expectedErr: true, + }, + { + name: "Lost PVC with phase-based skip policy should not error and return false", + inputPVC: builder.ForPersistentVolumeClaim("ns", "pvc-lost"). + StorageClass("some-class"). + Phase(corev1api.ClaimLost). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldSnapshot: false, + expectedErr: false, + }, + { + name: "Lost PVC with policy for Pending and Lost should not error and return false", + inputPVC: builder.ForPersistentVolumeClaim("ns", "pvc-lost"). + StorageClass("some-class"). + Phase(corev1api.ClaimLost). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending", "Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldSnapshot: false, + expectedErr: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClient(t) + + var p *resourcepolicies.Policies + if tc.resourcePolicies != nil { + p = &resourcepolicies.Policies{} + err := p.BuildPolicy(tc.resourcePolicies) + require.NoError(t, err) + } + + vh := NewVolumeHelperImpl( + p, + ptr.To(true), + logrus.StandardLogger(), + fakeClient, + false, + false, + ) + + obj, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.inputPVC) + require.NoError(t, err) + + actualShouldSnapshot, actualError := vh.ShouldPerformSnapshot(&unstructured.Unstructured{Object: obj}, kuberesource.PersistentVolumeClaims) + if tc.expectedErr { + require.Error(t, actualError, "Want error; Got nil error") + return + } + + require.NoError(t, actualError) + require.Equalf(t, tc.shouldSnapshot, actualShouldSnapshot, "Want shouldSnapshot as %t; Got shouldSnapshot as %t", tc.shouldSnapshot, actualShouldSnapshot) + }) + } +} + +// TestVolumeHelperImpl_ShouldPerformFSBackup_UnboundPVC tests that Pending and Lost PVCs with +// phase-based skip policies don't cause errors when GetPVForPVC would fail. +func TestVolumeHelperImpl_ShouldPerformFSBackup_UnboundPVC(t *testing.T) { + testCases := []struct { + name string + pod *corev1api.Pod + pvc *corev1api.PersistentVolumeClaim + resourcePolicies *resourcepolicies.ResourcePolicies + shouldFSBackup bool + expectedErr bool + }{ + { + name: "Pending PVC with phase-based skip policy should not error and return false", + pod: builder.ForPod("ns", "pod-1"). + Volumes( + &corev1api.Volume{ + Name: "vol-pending", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-pending", + }, + }, + }).Result(), + pvc: builder.ForPersistentVolumeClaim("ns", "pvc-pending"). + StorageClass("non-existent-class"). + Phase(corev1api.ClaimPending). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldFSBackup: false, + expectedErr: false, + }, + { + name: "Pending PVC without matching skip policy should error (no PV)", + pod: builder.ForPod("ns", "pod-1"). + Volumes( + &corev1api.Volume{ + Name: "vol-pending", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-pending-no-policy", + }, + }, + }).Result(), + pvc: builder.ForPersistentVolumeClaim("ns", "pvc-pending-no-policy"). + StorageClass("non-existent-class"). + Phase(corev1api.ClaimPending). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "storageClass": []string{"gp2-csi"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldFSBackup: false, + expectedErr: true, + }, + { + name: "Lost PVC with phase-based skip policy should not error and return false", + pod: builder.ForPod("ns", "pod-1"). + Volumes( + &corev1api.Volume{ + Name: "vol-lost", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-lost", + }, + }, + }).Result(), + pvc: builder.ForPersistentVolumeClaim("ns", "pvc-lost"). + StorageClass("some-class"). + Phase(corev1api.ClaimLost). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldFSBackup: false, + expectedErr: false, + }, + { + name: "Lost PVC with policy for Pending and Lost should not error and return false", + pod: builder.ForPod("ns", "pod-1"). + Volumes( + &corev1api.Volume{ + Name: "vol-lost", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pvc-lost", + }, + }, + }).Result(), + pvc: builder.ForPersistentVolumeClaim("ns", "pvc-lost"). + StorageClass("some-class"). + Phase(corev1api.ClaimLost). + Result(), + resourcePolicies: &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending", "Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + }, + shouldFSBackup: false, + expectedErr: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, tc.pvc) + require.NoError(t, fakeClient.Create(t.Context(), tc.pod)) + + var p *resourcepolicies.Policies + if tc.resourcePolicies != nil { + p = &resourcepolicies.Policies{} + err := p.BuildPolicy(tc.resourcePolicies) + require.NoError(t, err) + } + + vh := NewVolumeHelperImpl( + p, + ptr.To(true), + logrus.StandardLogger(), + fakeClient, + false, + false, + ) + + actualShouldFSBackup, actualError := vh.ShouldPerformFSBackup(tc.pod.Spec.Volumes[0], *tc.pod) + if tc.expectedErr { + require.Error(t, actualError, "Want error; Got nil error") + return + } + + require.NoError(t, actualError) + require.Equalf(t, tc.shouldFSBackup, actualShouldFSBackup, "Want shouldFSBackup as %t; Got shouldFSBackup as %t", tc.shouldFSBackup, actualShouldFSBackup) + }) + } +} diff --git a/pkg/backup/item_backupper.go b/pkg/backup/item_backupper.go index 7b1ea69bd..feae0e01c 100644 --- a/pkg/backup/item_backupper.go +++ b/pkg/backup/item_backupper.go @@ -687,15 +687,14 @@ func (ib *itemBackupper) getMatchAction(obj runtime.Unstructured, groupResource return nil, errors.WithStack(err) } - pvName := pvc.Spec.VolumeName - if pvName == "" { - return nil, errors.Errorf("PVC has no volume backing this claim") - } - - pv := &corev1api.PersistentVolume{} - if err := ib.kbClient.Get(context.Background(), kbClient.ObjectKey{Name: pvName}, pv); err != nil { - return nil, errors.WithStack(err) + var pv *corev1api.PersistentVolume + if pvName := pvc.Spec.VolumeName; pvName != "" { + pv = &corev1api.PersistentVolume{} + if err := ib.kbClient.Get(context.Background(), kbClient.ObjectKey{Name: pvName}, pv); err != nil { + return nil, errors.WithStack(err) + } } + // If pv is nil for unbound PVCs - policy matching will use PVC-only conditions vfd := resourcepolicies.NewVolumeFilterData(pv, nil, pvc) return ib.backupRequest.ResPolicies.GetMatchAction(vfd) } @@ -709,7 +708,10 @@ func (ib *itemBackupper) trackSkippedPV(obj runtime.Unstructured, groupResource if name, err := getPVName(obj, groupResource); len(name) > 0 && err == nil { ib.backupRequest.SkippedPVTracker.Track(name, approach, reason) } else if err != nil { - log.WithError(err).Warnf("unable to get PV name, skip tracking.") + // Log at info level for tracking purposes. This is not an error because + // it's expected for some resources (e.g., PVCs in Pending or Lost phase) + // to not have a PV name. This occurs when volume policy skips unbound PVCs. + log.WithError(err).Infof("unable to get PV name, skip tracking.") } } @@ -719,6 +721,17 @@ func (ib *itemBackupper) unTrackSkippedPV(obj runtime.Unstructured, groupResourc if name, err := getPVName(obj, groupResource); len(name) > 0 && err == nil { ib.backupRequest.SkippedPVTracker.Untrack(name) } else if err != nil { + // For PVCs in Pending or Lost phase, it's expected that there's no PV name. + // Log at debug level instead of warning to reduce noise. + if groupResource == kuberesource.PersistentVolumeClaims { + pvc := new(corev1api.PersistentVolumeClaim) + if convErr := runtime.DefaultUnstructuredConverter.FromUnstructured(obj.UnstructuredContent(), pvc); convErr == nil { + if pvc.Status.Phase == corev1api.ClaimPending || pvc.Status.Phase == corev1api.ClaimLost { + log.WithError(err).Debugf("unable to get PV name for %s PVC, skip untracking.", pvc.Status.Phase) + return + } + } + } log.WithError(err).Warnf("unable to get PV name, skip untracking.") } } diff --git a/pkg/backup/item_backupper_test.go b/pkg/backup/item_backupper_test.go index b76536baa..be91b6d34 100644 --- a/pkg/backup/item_backupper_test.go +++ b/pkg/backup/item_backupper_test.go @@ -17,12 +17,15 @@ limitations under the License. package backup import ( + "bytes" "testing" "github.com/sirupsen/logrus" "github.com/stretchr/testify/require" "k8s.io/apimachinery/pkg/runtime/schema" + ctrlfake "sigs.k8s.io/controller-runtime/pkg/client/fake" + "github.com/vmware-tanzu/velero/internal/resourcepolicies" "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/stretchr/testify/assert" @@ -269,3 +272,225 @@ func TestAddVolumeInfo(t *testing.T) { }) } } + +func TestGetMatchAction_PendingLostPVC(t *testing.T) { + scheme := runtime.NewScheme() + require.NoError(t, corev1api.AddToScheme(scheme)) + + // Create resource policies that skip Pending/Lost PVCs + resPolicies := &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending", "Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + } + policies := &resourcepolicies.Policies{} + err := policies.BuildPolicy(resPolicies) + require.NoError(t, err) + + testCases := []struct { + name string + pvc *corev1api.PersistentVolumeClaim + pv *corev1api.PersistentVolume + expectedAction *resourcepolicies.Action + expectError bool + }{ + { + name: "Pending PVC with no VolumeName should match pvcPhase policy", + pvc: builder.ForPersistentVolumeClaim("ns", "pending-pvc"). + StorageClass("test-sc"). + Phase(corev1api.ClaimPending). + Result(), + pv: nil, + expectedAction: &resourcepolicies.Action{Type: resourcepolicies.Skip}, + expectError: false, + }, + { + name: "Lost PVC with no VolumeName should match pvcPhase policy", + pvc: builder.ForPersistentVolumeClaim("ns", "lost-pvc"). + StorageClass("test-sc"). + Phase(corev1api.ClaimLost). + Result(), + pv: nil, + expectedAction: &resourcepolicies.Action{Type: resourcepolicies.Skip}, + expectError: false, + }, + { + name: "Bound PVC with VolumeName and matching PV should not match pvcPhase policy", + pvc: builder.ForPersistentVolumeClaim("ns", "bound-pvc"). + StorageClass("test-sc"). + VolumeName("test-pv"). + Phase(corev1api.ClaimBound). + Result(), + pv: builder.ForPersistentVolume("test-pv").StorageClass("test-sc").Result(), + expectedAction: nil, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Build fake client with PV if present + clientBuilder := ctrlfake.NewClientBuilder().WithScheme(scheme) + if tc.pv != nil { + clientBuilder = clientBuilder.WithObjects(tc.pv) + } + fakeClient := clientBuilder.Build() + + ib := &itemBackupper{ + kbClient: fakeClient, + backupRequest: &Request{ + ResPolicies: policies, + }, + } + + // Convert PVC to unstructured + pvcData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pvc) + require.NoError(t, err) + obj := &unstructured.Unstructured{Object: pvcData} + + action, err := ib.getMatchAction(obj, kuberesource.PersistentVolumeClaims, csiBIAPluginName) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + if tc.expectedAction == nil { + assert.Nil(t, action) + } else { + require.NotNil(t, action) + assert.Equal(t, tc.expectedAction.Type, action.Type) + } + }) + } +} + +func TestTrackSkippedPV_PendingLostPVC(t *testing.T) { + testCases := []struct { + name string + pvc *corev1api.PersistentVolumeClaim + }{ + { + name: "Pending PVC should log at info level", + pvc: builder.ForPersistentVolumeClaim("ns", "pending-pvc"). + Phase(corev1api.ClaimPending). + Result(), + }, + { + name: "Lost PVC should log at info level", + pvc: builder.ForPersistentVolumeClaim("ns", "lost-pvc"). + Phase(corev1api.ClaimLost). + Result(), + }, + { + name: "Bound PVC without VolumeName should log at info level", + pvc: builder.ForPersistentVolumeClaim("ns", "bound-pvc"). + Phase(corev1api.ClaimBound). + Result(), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ib := &itemBackupper{ + backupRequest: &Request{ + SkippedPVTracker: NewSkipPVTracker(), + }, + } + + // Set up log capture + logOutput := &bytes.Buffer{} + logger := logrus.New() + logger.SetOutput(logOutput) + logger.SetLevel(logrus.DebugLevel) + + // Convert PVC to unstructured + pvcData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pvc) + require.NoError(t, err) + obj := &unstructured.Unstructured{Object: pvcData} + + ib.trackSkippedPV(obj, kuberesource.PersistentVolumeClaims, "", "test reason", logger) + + logStr := logOutput.String() + assert.Contains(t, logStr, "level=info") + assert.Contains(t, logStr, "unable to get PV name, skip tracking.") + }) + } +} + +func TestUnTrackSkippedPV_PendingLostPVC(t *testing.T) { + testCases := []struct { + name string + pvc *corev1api.PersistentVolumeClaim + expectWarningLog bool + expectDebugMessage string + }{ + { + name: "Pending PVC should log at debug level, not warning", + pvc: builder.ForPersistentVolumeClaim("ns", "pending-pvc"). + Phase(corev1api.ClaimPending). + Result(), + expectWarningLog: false, + expectDebugMessage: "unable to get PV name for Pending PVC, skip untracking.", + }, + { + name: "Lost PVC should log at debug level, not warning", + pvc: builder.ForPersistentVolumeClaim("ns", "lost-pvc"). + Phase(corev1api.ClaimLost). + Result(), + expectWarningLog: false, + expectDebugMessage: "unable to get PV name for Lost PVC, skip untracking.", + }, + { + name: "Bound PVC without VolumeName should log warning", + pvc: builder.ForPersistentVolumeClaim("ns", "bound-pvc"). + Phase(corev1api.ClaimBound). + Result(), + expectWarningLog: true, + expectDebugMessage: "", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + ib := &itemBackupper{ + backupRequest: &Request{ + SkippedPVTracker: NewSkipPVTracker(), + }, + } + + // Set up log capture + logOutput := &bytes.Buffer{} + logger := logrus.New() + logger.SetOutput(logOutput) + logger.SetLevel(logrus.DebugLevel) + + // Convert PVC to unstructured + pvcData, err := runtime.DefaultUnstructuredConverter.ToUnstructured(tc.pvc) + require.NoError(t, err) + obj := &unstructured.Unstructured{Object: pvcData} + + ib.unTrackSkippedPV(obj, kuberesource.PersistentVolumeClaims, logger) + + logStr := logOutput.String() + if tc.expectWarningLog { + assert.Contains(t, logStr, "level=warning") + assert.Contains(t, logStr, "unable to get PV name, skip untracking.") + } else { + assert.NotContains(t, logStr, "level=warning") + if tc.expectDebugMessage != "" { + assert.Contains(t, logStr, "level=debug") + assert.Contains(t, logStr, tc.expectDebugMessage) + } + } + }) + } +} diff --git a/pkg/podvolume/backupper.go b/pkg/podvolume/backupper.go index fba3cb19a..1747f1b33 100644 --- a/pkg/podvolume/backupper.go +++ b/pkg/podvolume/backupper.go @@ -210,11 +210,9 @@ func resultsKey(ns, name string) string { func (b *backupper) getMatchAction(resPolicies *resourcepolicies.Policies, pvc *corev1api.PersistentVolumeClaim, volume *corev1api.Volume) (*resourcepolicies.Action, error) { if pvc != nil { - pv := new(corev1api.PersistentVolume) - err := b.crClient.Get(context.TODO(), ctrlclient.ObjectKey{Name: pvc.Spec.VolumeName}, pv) - if err != nil { - return nil, errors.Wrapf(err, "error getting pv for pvc %s", pvc.Spec.VolumeName) - } + // Ignore err, if the PV is not available (Pending/Lost PVC or PV fetch failed) - try matching with PVC only + // GetPVForPVC returns nil for all error cases + pv, _ := kube.GetPVForPVC(pvc, b.crClient) vfd := resourcepolicies.NewVolumeFilterData(pv, nil, pvc) return resPolicies.GetMatchAction(vfd) } diff --git a/pkg/podvolume/backupper_test.go b/pkg/podvolume/backupper_test.go index 6359df696..846f65796 100644 --- a/pkg/podvolume/backupper_test.go +++ b/pkg/podvolume/backupper_test.go @@ -309,8 +309,8 @@ func createNodeObj() *corev1api.Node { func TestBackupPodVolumes(t *testing.T) { scheme := runtime.NewScheme() - velerov1api.AddToScheme(scheme) - corev1api.AddToScheme(scheme) + require.NoError(t, velerov1api.AddToScheme(scheme)) + require.NoError(t, corev1api.AddToScheme(scheme)) log := logrus.New() tests := []struct { @@ -778,7 +778,7 @@ func TestWaitAllPodVolumesProcessed(t *testing.T) { backuper := newBackupper(c.ctx, log, nil, nil, informer, nil, "", &velerov1api.Backup{}) if c.pvb != nil { - backuper.pvbIndexer.Add(c.pvb) + require.NoError(t, backuper.pvbIndexer.Add(c.pvb)) backuper.wg.Add(1) } @@ -833,3 +833,185 @@ func TestPVCBackupSummary(t *testing.T) { assert.Empty(t, pbs.Skipped) assert.Len(t, pbs.Backedup, 2) } + +func TestGetMatchAction_PendingPVC(t *testing.T) { + // Create resource policies that skip Pending/Lost PVCs + resPolicies := &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending", "Lost"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + } + policies := &resourcepolicies.Policies{} + err := policies.BuildPolicy(resPolicies) + require.NoError(t, err) + + testCases := []struct { + name string + pvc *corev1api.PersistentVolumeClaim + volume *corev1api.Volume + pv *corev1api.PersistentVolume + expectedAction *resourcepolicies.Action + expectError bool + }{ + { + name: "Pending PVC with pvcPhase skip policy should return skip action", + pvc: builder.ForPersistentVolumeClaim("ns", "pending-pvc"). + StorageClass("test-sc"). + Phase(corev1api.ClaimPending). + Result(), + volume: &corev1api.Volume{ + Name: "test-volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pending-pvc", + }, + }, + }, + pv: nil, + expectedAction: &resourcepolicies.Action{Type: resourcepolicies.Skip}, + expectError: false, + }, + { + name: "Lost PVC with pvcPhase skip policy should return skip action", + pvc: builder.ForPersistentVolumeClaim("ns", "lost-pvc"). + StorageClass("test-sc"). + Phase(corev1api.ClaimLost). + Result(), + volume: &corev1api.Volume{ + Name: "test-volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "lost-pvc", + }, + }, + }, + pv: nil, + expectedAction: &resourcepolicies.Action{Type: resourcepolicies.Skip}, + expectError: false, + }, + { + name: "Bound PVC with matching PV should not match pvcPhase policy", + pvc: builder.ForPersistentVolumeClaim("ns", "bound-pvc"). + StorageClass("test-sc"). + VolumeName("test-pv"). + Phase(corev1api.ClaimBound). + Result(), + volume: &corev1api.Volume{ + Name: "test-volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "bound-pvc", + }, + }, + }, + pv: builder.ForPersistentVolume("test-pv").StorageClass("test-sc").Result(), + expectedAction: nil, + expectError: false, + }, + { + name: "Pending PVC with no matching policy should return nil action", + pvc: builder.ForPersistentVolumeClaim("ns", "pending-pvc-no-match"). + StorageClass("test-sc"). + Phase(corev1api.ClaimPending). + Result(), + volume: &corev1api.Volume{ + Name: "test-volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pending-pvc-no-match", + }, + }, + }, + pv: nil, + expectedAction: &resourcepolicies.Action{Type: resourcepolicies.Skip}, // Will match the pvcPhase policy + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + // Build fake client with PV if present + var objs []runtime.Object + if tc.pv != nil { + objs = append(objs, tc.pv) + } + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, objs...) + + b := &backupper{ + crClient: fakeClient, + } + + action, err := b.getMatchAction(policies, tc.pvc, tc.volume) + if tc.expectError { + require.Error(t, err) + } else { + require.NoError(t, err) + } + + if tc.expectedAction == nil { + assert.Nil(t, action) + } else { + require.NotNil(t, action) + assert.Equal(t, tc.expectedAction.Type, action.Type) + } + }) + } +} + +func TestGetMatchAction_PVCWithoutPVLookupError(t *testing.T) { + // Test that when a PVC has a VolumeName but the PV doesn't exist, + // the function ignores the error and tries to match with PVC only + resPolicies := &resourcepolicies.ResourcePolicies{ + Version: "v1", + VolumePolicies: []resourcepolicies.VolumePolicy{ + { + Conditions: map[string]any{ + "pvcPhase": []string{"Pending"}, + }, + Action: resourcepolicies.Action{ + Type: resourcepolicies.Skip, + }, + }, + }, + } + policies := &resourcepolicies.Policies{} + err := policies.BuildPolicy(resPolicies) + require.NoError(t, err) + + // Pending PVC without a matching PV in the cluster + pvc := builder.ForPersistentVolumeClaim("ns", "pending-pvc"). + StorageClass("test-sc"). + Phase(corev1api.ClaimPending). + Result() + + volume := &corev1api.Volume{ + Name: "test-volume", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: "pending-pvc", + }, + }, + } + + // Empty client - no PV exists + fakeClient := velerotest.NewFakeControllerRuntimeClient(t) + + b := &backupper{ + crClient: fakeClient, + } + + // Should succeed even though PV lookup would fail + // because the function ignores PV lookup errors and uses PVC-only matching + action, err := b.getMatchAction(policies, pvc, volume) + require.NoError(t, err) + require.NotNil(t, action) + assert.Equal(t, resourcepolicies.Skip, action.Type) +} diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index 786cef2a5..5209cf6ea 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -417,19 +417,19 @@ func MakePodPVCAttachment(volumeName string, volumeMode *corev1api.PersistentVol return volumeMounts, volumeDevices, volumePath } +// GetPVForPVC returns the PersistentVolume backing a PVC +// returns PV, error. +// PV will be nil on error func GetPVForPVC( pvc *corev1api.PersistentVolumeClaim, crClient crclient.Client, ) (*corev1api.PersistentVolume, error) { if pvc.Spec.VolumeName == "" { - return nil, errors.Errorf("PVC %s/%s has no volume backing this claim", - pvc.Namespace, pvc.Name) + return nil, errors.Errorf("PVC %s/%s has no volume backing this claim", pvc.Namespace, pvc.Name) } if pvc.Status.Phase != corev1api.ClaimBound { - // TODO: confirm if this PVC should be snapshotted if it has no PV bound - return nil, - errors.Errorf("PVC %s/%s is in phase %v and is not bound to a volume", - pvc.Namespace, pvc.Name, pvc.Status.Phase) + return nil, errors.Errorf("PVC %s/%s is in phase %v and is not bound to a volume", + pvc.Namespace, pvc.Name, pvc.Status.Phase) } pv := &corev1api.PersistentVolume{}