From 80211d77e5fee1f75c805f4e4bfeb2caf3847951 Mon Sep 17 00:00:00 2001 From: "claude[bot]" <209825114+claude[bot]@users.noreply.github.com> Date: Thu, 7 Aug 2025 16:43:59 +0000 Subject: [PATCH] feat: Add VolumePolicy support for PVC Phase conditions to allow skipping Pending PVCs This commit implements VolumePolicy support for PVC Phase conditions, resolving vmware-tanzu/velero#7233 where backups fail with ''PVC has no volume backing this claim'' for Pending PVCs. Changes made: - Extended VolumePolicy API to support PVC phase conditions - Added pvcPhaseCondition struct with matching logic - Modified getMatchAction() to evaluate policies for unbound PVCs before returning errors - Added case to GetMatchAction() to handle PVC-only scenarios (nil PV) - Added comprehensive unit tests for PVC phase parsing and matching Users can now skip Pending PVCs through volume policy configuration: apiVersion: v1 kind: ConfigMap metadata: name: volume-policy namespace: velero data: policy.yaml: | version: v1 volumePolicies: - conditions: pvcPhase: [Pending] action: type: skip chore: rename changelog file to match PR #9166 Renamed changelogs/unreleased/7233-claude to changelogs/unreleased/9166-claude to match the opened PR at https://github.com/vmware-tanzu/velero/pull/9166 docs: Add PVC phase condition support to VolumePolicy documentation - Added pvcPhase field to YAML template example - Documented pvcPhase as a supported condition in the list - Added comprehensive examples for using PVC phase conditions - Included examples for Pending, Bound, and Lost phases - Demonstrated combining PVC phase with other conditions Co-Authored-By: Tiger Kaovilai --- changelogs/unreleased/9166-claude | 1 + .../resourcepolicies/resource_policies.go | 6 + .../resource_policies_test.go | 148 +++++++++++++++++- internal/resourcepolicies/volume_resources.go | 33 +++- .../volume_resources_validator.go | 1 + site/content/docs/main/resource-filtering.md | 61 +++++++- 6 files changed, 243 insertions(+), 7 deletions(-) create mode 100644 changelogs/unreleased/9166-claude diff --git a/changelogs/unreleased/9166-claude b/changelogs/unreleased/9166-claude new file mode 100644 index 000000000..8e3cb6610 --- /dev/null +++ b/changelogs/unreleased/9166-claude @@ -0,0 +1 @@ +Add VolumePolicy support for PVC Phase conditions to allow skipping Pending PVCs \ No newline at end of file diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index 0837e63e2..d484cabce 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/internal/resourcepolicies/resource_policies.go @@ -146,6 +146,9 @@ func (p *Policies) BuildPolicy(resPolicies *ResourcePolicies) error { if len(con.PVCLabels) > 0 { volP.conditions = append(volP.conditions, &pvcLabelsCondition{labels: con.PVCLabels}) } + if len(con.PVCPhase) > 0 { + volP.conditions = append(volP.conditions, &pvcPhaseCondition{phases: con.PVCPhase}) + } p.volumePolicies = append(p.volumePolicies, volP) } @@ -191,6 +194,9 @@ func (p *Policies) GetMatchAction(res any) (*Action, error) { if data.PVC != nil { volume.parsePVC(data.PVC) } + case data.PVC != nil: + // Handle PVC-only scenarios (e.g., unbound PVCs) + volume.parsePVC(data.PVC) default: return nil, errors.New("failed to convert object") } diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index b0d5f1fbd..06b1bea1c 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/internal/resourcepolicies/resource_policies_test.go @@ -983,6 +983,69 @@ volumePolicies: }, skip: false, }, + { + name: "PVC phase matching - Pending phase should skip", + yamlData: `version: v1 +volumePolicies: +- conditions: + pvcPhase: ["Pending"] + action: + type: skip`, + vol: nil, + podVol: nil, + pvc: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "pvc-pending", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimPending, + }, + }, + skip: true, + }, + { + name: "PVC phase matching - Bound phase should not skip", + yamlData: `version: v1 +volumePolicies: +- conditions: + pvcPhase: ["Pending"] + action: + type: skip`, + vol: nil, + podVol: nil, + pvc: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "pvc-bound", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, + skip: false, + }, + { + name: "PVC phase matching - Multiple phases (Pending, Lost)", + yamlData: `version: v1 +volumePolicies: +- conditions: + pvcPhase: ["Pending", "Lost"] + action: + type: skip`, + vol: nil, + podVol: nil, + pvc: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "default", + Name: "pvc-lost", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimLost, + }, + }, + skip: true, + }, } for _, tc := range testCases { t.Run(tc.name, func(t *testing.T) { @@ -1059,32 +1122,53 @@ func TestParsePVC(t *testing.T) { name string pvc *corev1api.PersistentVolumeClaim expectedLabels map[string]string + expectedPhase string expectErr bool }{ { - name: "valid PVC with labels", + name: "valid PVC with labels and Pending phase", pvc: &corev1api.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{"env": "prod"}, }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimPending, + }, }, expectedLabels: map[string]string{"env": "prod"}, + expectedPhase: "Pending", expectErr: false, }, { - name: "valid PVC with empty labels", + name: "valid PVC with Bound phase", pvc: &corev1api.PersistentVolumeClaim{ ObjectMeta: metav1.ObjectMeta{ Labels: map[string]string{}, }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, }, expectedLabels: nil, + expectedPhase: "Bound", + expectErr: false, + }, + { + name: "valid PVC with Lost phase", + pvc: &corev1api.PersistentVolumeClaim{ + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimLost, + }, + }, + expectedLabels: nil, + expectedPhase: "Lost", expectErr: false, }, { name: "nil PVC pointer", pvc: (*corev1api.PersistentVolumeClaim)(nil), expectedLabels: nil, + expectedPhase: "", expectErr: false, }, } @@ -1095,6 +1179,66 @@ func TestParsePVC(t *testing.T) { s.parsePVC(tc.pvc) assert.Equal(t, tc.expectedLabels, s.pvcLabels) + assert.Equal(t, tc.expectedPhase, s.pvcPhase) + }) + } +} + +func TestPVCPhaseMatch(t *testing.T) { + tests := []struct { + name string + condition *pvcPhaseCondition + volume *structuredVolume + expectedMatch bool + }{ + { + name: "match Pending phase", + condition: &pvcPhaseCondition{phases: []string{"Pending"}}, + volume: &structuredVolume{pvcPhase: "Pending"}, + expectedMatch: true, + }, + { + name: "match multiple phases - Pending matches", + condition: &pvcPhaseCondition{phases: []string{"Pending", "Bound"}}, + volume: &structuredVolume{pvcPhase: "Pending"}, + expectedMatch: true, + }, + { + name: "match multiple phases - Bound matches", + condition: &pvcPhaseCondition{phases: []string{"Pending", "Bound"}}, + volume: &structuredVolume{pvcPhase: "Bound"}, + expectedMatch: true, + }, + { + name: "no match for different phase", + condition: &pvcPhaseCondition{phases: []string{"Pending"}}, + volume: &structuredVolume{pvcPhase: "Bound"}, + expectedMatch: false, + }, + { + name: "no match for empty phase", + condition: &pvcPhaseCondition{phases: []string{"Pending"}}, + volume: &structuredVolume{pvcPhase: ""}, + expectedMatch: false, + }, + { + name: "match with empty phases list (always match)", + condition: &pvcPhaseCondition{phases: []string{}}, + volume: &structuredVolume{pvcPhase: "Pending"}, + expectedMatch: true, + }, + { + name: "match with nil phases list (always match)", + condition: &pvcPhaseCondition{phases: nil}, + volume: &structuredVolume{pvcPhase: "Pending"}, + expectedMatch: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := tc.condition.match(tc.volume) + assert.Equal(t, tc.expectedMatch, result) }) } } diff --git a/internal/resourcepolicies/volume_resources.go b/internal/resourcepolicies/volume_resources.go index 545c1f3f6..4ad34f484 100644 --- a/internal/resourcepolicies/volume_resources.go +++ b/internal/resourcepolicies/volume_resources.go @@ -51,6 +51,7 @@ type structuredVolume struct { csi *csiVolumeSource volumeType SupportedVolume pvcLabels map[string]string + pvcPhase string } func (s *structuredVolume) parsePV(pv *corev1api.PersistentVolume) { @@ -70,8 +71,11 @@ func (s *structuredVolume) parsePV(pv *corev1api.PersistentVolume) { } func (s *structuredVolume) parsePVC(pvc *corev1api.PersistentVolumeClaim) { - if pvc != nil && len(pvc.GetLabels()) > 0 { - s.pvcLabels = pvc.Labels + if pvc != nil { + if len(pvc.GetLabels()) > 0 { + s.pvcLabels = pvc.Labels + } + s.pvcPhase = string(pvc.Status.Phase) } } @@ -110,6 +114,31 @@ func (c *pvcLabelsCondition) validate() error { return nil } +// pvcPhaseCondition defines a condition that matches if the PVC's phase matches any of the provided phases. +type pvcPhaseCondition struct { + phases []string +} + +func (c *pvcPhaseCondition) match(v *structuredVolume) bool { + // No phases specified: always match. + if len(c.phases) == 0 { + return true + } + if v.pvcPhase == "" { + return false + } + for _, phase := range c.phases { + if v.pvcPhase == phase { + return true + } + } + return false +} + +func (c *pvcPhaseCondition) validate() error { + return nil +} + type capacityCondition struct { capacity capacity } diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index 0c719b09e..b6031eec2 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/internal/resourcepolicies/volume_resources_validator.go @@ -46,6 +46,7 @@ type volumeConditions struct { CSI *csiVolumeSource `yaml:"csi,omitempty"` VolumeTypes []SupportedVolume `yaml:"volumeTypes,omitempty"` PVCLabels map[string]string `yaml:"pvcLabels,omitempty"` + PVCPhase []string `yaml:"pvcPhase,omitempty"` } func (c *capacityCondition) validate() error { diff --git a/site/content/docs/main/resource-filtering.md b/site/content/docs/main/resource-filtering.md index 710564789..a9e65d157 100644 --- a/site/content/docs/main/resource-filtering.md +++ b/site/content/docs/main/resource-filtering.md @@ -278,6 +278,9 @@ The policies YAML config file would look like this: storageClass: - gp2 - standard + # pvc matches specific phase(s) + pvcPhase: + - Pending action: type: skip - conditions: @@ -370,6 +373,7 @@ Currently, Velero supports the volume attributes listed below: - "5Gi" which is not supported and will be failed in validating the configuration - storageClass: matching volumes those with specified `storageClass`, such as `gp2`, `ebs-sc` in eks - volume sources: matching volumes that used specified volume sources. Currently we support nfs or csi backend volume source +- pvcPhase: matching volumes based on the phase of their associated PVCs (Pending, Bound, Lost) Velero supported conditions and format listed below: - capacity @@ -462,9 +466,60 @@ Velero supported conditions and format listed below: type: skip ``` -#### VolumePolicies rules -- Velero already has lots of include or exclude filters. the volume policies are the final filters after others include or exclude filters in one backup processing workflow. So if use a defined similar filter like the opt-in approach to backup one pod volume but skip backup of the same pod volume in volume policies, as volume policies are the final filters that are applied, the volume will not be backed up. -- If volume policies conflict with themselves the first matched policy will be respected when many policies are defined. +- pvc Phase + + This condition filters volumes based on the phase of their associated PVCs. The condition is specified as a list of phases to match. The volume matches this condition if the PVC's phase matches any of the phases in the list. Supported phases are: `Pending`, `Bound`, and `Lost`. + ```yaml + pvcPhase: + - Pending + ``` + + Some examples: + - Skip Pending PVCs: Skip backup of volumes whose associated PVC is in `Pending` phase (useful for PVCs that haven't been bound to a PV yet). + ```yaml + volumePolicies: + - conditions: + pvcPhase: + - Pending + action: + type: skip + ``` + - Skip multiple phases: Skip backup of volumes whose associated PVC is either in `Pending` or `Lost` phase. + ```yaml + volumePolicies: + - conditions: + pvcPhase: + - Pending + - Lost + action: + type: skip + ``` + - Backup only Bound PVCs: Only backup volumes whose associated PVC is in `Bound` phase. + ```yaml + volumePolicies: + - conditions: + pvcPhase: + - Bound + action: + type: snapshot + ``` + - Combine with other conditions: You can combine PVC phase conditions with other conditions like storage class or labels. + ```yaml + volumePolicies: + - conditions: + pvcPhase: + - Pending + storageClass: + - gp2 + action: + type: skip + ``` + + + +### Resource policies rules +- Velero already has lots of include or exclude filters. the resource policies are the final filters after others include or exclude filters in one backup processing workflow. So if use a defined similar filter like the opt-in approach to backup one pod volume but skip backup of the same pod volume in resource policies, as resource policies are the final filters that are applied, the volume will not be backed up. +- If volume resource policies conflict with themselves the first matched policy will be respected when many policies are defined. #### VolumePolicy priority with existing filters * [Includes filters](#includes) and [Excludes filters](#excludes) have the highest priority. The filtered-out resources by them cannot reach to the VolumePolicy.