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 <tkaovila@redhat.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
This commit is contained in:
Tiger Kaovilai
2026-01-29 16:25:52 -05:00
parent e159992f48
commit 71ddeefcd6
8 changed files with 780 additions and 33 deletions

View File

@@ -0,0 +1 @@
Fix VolumePolicy PVC phase condition filter for unbound PVCs (#9507)

View File

@@ -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) {

View File

@@ -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)
})
}
}

View File

@@ -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.")
}
}

View File

@@ -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)
}
}
})
}
}

View File

@@ -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)
}

View File

@@ -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)
}

View File

@@ -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{}