mirror of
https://github.com/vmware-tanzu/velero.git
synced 2026-08-21 22:56:05 +00:00
Add snapshotClass parameter to volume policy snapshot action (#10070)
* Add SnapshotClassParameter constant and GetSnapshotClass getter Add a new snapshotClass action parameter to volume policies, allowing users to specify which VolumeSnapshotClass to use for CSI snapshots. This follows the existing dataMover parameter pattern with a typed constant and getter method on the Action struct. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add snapshotClass parameter validation Validate the snapshotClass parameter in Action.validate(): it must only appear on snapshot actions, must be a string, and must not be empty. Follows the same validation pattern as the dataMover parameter. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add volume policy tier to VolumeSnapshotClass selection Add GetVolumeSnapshotClassFromVolumePolicy helper and extend GetVolumeSnapshotClass with a policySnapshotClass parameter. The new tier sits between PVC annotation and backup annotation in the priority chain: PVC annotation > volume policy > backup annotation > VSC label. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Wire snapshotClass from volume policy through CSI plugin In pvcBackupItemAction.Execute, call GetActionParameters to extract the snapshotClass from the matched volume policy and pass it through getVolumeSnapshotReference and createVolumeSnapshot to GetVolumeSnapshotClass. This connects the volume policy parameter to the CSI snapshot creation path. Fixes #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add changelog for PR #10070 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Document snapshotClass volume policy parameter Add documentation for the new snapshotClass parameter in the volume policy snapshot action. Update the CSI docs to include volume policy as a tier in the VolumeSnapshotClass selection priority, and add Example 6 to resource-filtering.md showing multi-array usage. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Fix import ordering in pvc_action.go Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add end-to-end test for snapshotClass volume policy parameter Verify that when a volume policy specifies snapshotClass, the CSI plugin creates a VolumeSnapshot using that VolumeSnapshotClass. The test uses a VSC without the velero label to confirm selection comes from the volume policy parameter, not the label-based fallback. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Fix gofmt struct field alignment in pvc_action_test.go Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Add GetSnapshotClass to VolumeHelper interface Add a GetSnapshotClass method to VolumeHelper that encapsulates the extraction of the snapshotClass parameter from volume policy actions. This avoids requiring callers to parse raw parameters from GetActionParameters. Simplify the CSI plugin to use the new method. Ref: #8807 Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> * Fix gofmt formatting in resource_policies.go Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com> --------- Signed-off-by: Shubham Pampattiwar <spampatt@redhat.com>
This commit is contained in:
Shubham Pampattiwar
@@ -212,6 +212,7 @@ func (p *pvcBackupItemAction) validatePVCAndPV(
|
||||
func (p *pvcBackupItemAction) createVolumeSnapshot(
|
||||
pvc corev1api.PersistentVolumeClaim,
|
||||
backup *velerov1api.Backup,
|
||||
policySnapshotClass string,
|
||||
) (
|
||||
vs *snapshotv1api.VolumeSnapshot,
|
||||
err error,
|
||||
@@ -232,6 +233,7 @@ func (p *pvcBackupItemAction) createVolumeSnapshot(
|
||||
&pvc,
|
||||
p.log,
|
||||
p.crClient,
|
||||
policySnapshotClass,
|
||||
)
|
||||
if err != nil {
|
||||
return nil, errors.Wrapf(
|
||||
@@ -338,7 +340,14 @@ func (p *pvcBackupItemAction) Execute(
|
||||
return nil, nil, "", nil, err
|
||||
}
|
||||
|
||||
vs, err := p.getVolumeSnapshotReference(context.TODO(), pvc, backup)
|
||||
policySnapshotClass, scErr := vh.GetSnapshotClass(item, kuberesource.PersistentVolumeClaims)
|
||||
if scErr != nil {
|
||||
p.log.WithError(scErr).Warn("failed to get snapshotClass from volume policy, proceeding without it")
|
||||
} else if policySnapshotClass != "" {
|
||||
p.log.Infof("Volume policy specifies snapshotClass=%s for PVC %s/%s", policySnapshotClass, pvc.Namespace, pvc.Name)
|
||||
}
|
||||
|
||||
vs, err := p.getVolumeSnapshotReference(context.TODO(), pvc, backup, policySnapshotClass)
|
||||
if err != nil {
|
||||
return nil, nil, "", nil, err
|
||||
}
|
||||
@@ -678,6 +687,7 @@ func (p *pvcBackupItemAction) getVolumeSnapshotReference(
|
||||
ctx context.Context,
|
||||
pvc corev1api.PersistentVolumeClaim,
|
||||
backup *velerov1api.Backup,
|
||||
policySnapshotClass string,
|
||||
) (*snapshotv1api.VolumeSnapshot, error) {
|
||||
vgsLabelKey := backup.Spec.VolumeGroupSnapshotLabelKey
|
||||
group, hasLabel := pvc.Labels[vgsLabelKey]
|
||||
@@ -808,7 +818,7 @@ func (p *pvcBackupItemAction) getVolumeSnapshotReference(
|
||||
}
|
||||
|
||||
// Legacy fallback: create individual VS
|
||||
return p.createVolumeSnapshot(pvc, backup)
|
||||
return p.createVolumeSnapshot(pvc, backup, policySnapshotClass)
|
||||
}
|
||||
|
||||
func (p *pvcBackupItemAction) findExistingVSForBackup(
|
||||
|
||||
@@ -79,21 +79,22 @@ func (c *errorInjectingClient) Create(ctx context.Context, obj crclient.Object,
|
||||
func TestExecute(t *testing.T) {
|
||||
boolTrue := true
|
||||
tests := []struct {
|
||||
name string
|
||||
backup *velerov1api.Backup
|
||||
pvc *corev1api.PersistentVolumeClaim
|
||||
pv *corev1api.PersistentVolume
|
||||
sc *storagev1api.StorageClass
|
||||
vsClass *snapshotv1api.VolumeSnapshotClass
|
||||
operationID string
|
||||
expectedErr error
|
||||
expectErr bool // Use bool for cases where we just need to check for any error
|
||||
expectedBackup *velerov1api.Backup
|
||||
expectedDataUpload *velerov2alpha1.DataUpload
|
||||
expectedPVC *corev1api.PersistentVolumeClaim
|
||||
resourcePolicy *corev1api.ConfigMap
|
||||
failVSCreate bool
|
||||
skipVSReadyUpdate bool // New flag to control VS readiness
|
||||
name string
|
||||
backup *velerov1api.Backup
|
||||
pvc *corev1api.PersistentVolumeClaim
|
||||
pv *corev1api.PersistentVolume
|
||||
sc *storagev1api.StorageClass
|
||||
vsClass *snapshotv1api.VolumeSnapshotClass
|
||||
operationID string
|
||||
expectedErr error
|
||||
expectErr bool // Use bool for cases where we just need to check for any error
|
||||
expectedBackup *velerov1api.Backup
|
||||
expectedDataUpload *velerov2alpha1.DataUpload
|
||||
expectedPVC *corev1api.PersistentVolumeClaim
|
||||
resourcePolicy *corev1api.ConfigMap
|
||||
failVSCreate bool
|
||||
skipVSReadyUpdate bool // New flag to control VS readiness
|
||||
expectedVSClassName string
|
||||
}{
|
||||
{
|
||||
name: "Skip PVC BIA when backup is in finalizing phase",
|
||||
@@ -187,6 +188,16 @@ func TestExecute(t *testing.T) {
|
||||
sc: builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
|
||||
vsClass: builder.ForVolumeSnapshotClass("tescVSClass").Driver("hostpath").ObjectMeta(builder.WithLabels(velerov1api.VolumeSnapshotClassSelectorLabel, "")).Result(),
|
||||
},
|
||||
{
|
||||
name: "Volume policy with snapshotClass selects correct VolumeSnapshotClass",
|
||||
backup: builder.ForBackup("velero", "test").ResourcePolicies("resourcePolicy").CSISnapshotTimeout(time.Duration(3600) * time.Second).Result(),
|
||||
resourcePolicy: builder.ForConfigMap("velero", "resourcePolicy").Data("policy", `{"version":"v1","volumePolicies":[{"conditions":{"csi":{}},"action":{"type":"snapshot","parameters":{"snapshotClass":"policy-selected-vsclass"}}}]}`).Result(),
|
||||
pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").VolumeName("testPV").StorageClass("testSC").Phase(corev1api.ClaimBound).Result(),
|
||||
pv: builder.ForPersistentVolume("testPV").CSI("hostpath", "testVolume").Result(),
|
||||
sc: builder.ForStorageClass("testSC").Provisioner("hostpath").Result(),
|
||||
vsClass: builder.ForVolumeSnapshotClass("policy-selected-vsclass").Driver("hostpath").Result(),
|
||||
expectedVSClassName: "policy-selected-vsclass",
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range tests {
|
||||
@@ -300,6 +311,15 @@ func TestExecute(t *testing.T) {
|
||||
runtime.DefaultUnstructuredConverter.FromUnstructured(resultUnstructed.UnstructuredContent(), resultPVC)
|
||||
require.True(t, cmp.Equal(tc.expectedPVC, resultPVC, cmpopts.IgnoreFields(corev1api.PersistentVolumeClaim{}, "ResourceVersion", "Annotations", "Labels")))
|
||||
}
|
||||
|
||||
if tc.expectedVSClassName != "" {
|
||||
vsList := new(snapshotv1api.VolumeSnapshotList)
|
||||
require.NoError(t, crClient.List(t.Context(), vsList, &crclient.ListOptions{Namespace: tc.pvc.Namespace}))
|
||||
require.NotEmpty(t, vsList.Items, "expected VolumeSnapshot to be created")
|
||||
require.NotNil(t, vsList.Items[0].Spec.VolumeSnapshotClassName)
|
||||
assert.Equal(t, tc.expectedVSClassName, *vsList.Items[0].Spec.VolumeSnapshotClassName,
|
||||
"VolumeSnapshot should use the VolumeSnapshotClass specified by volume policy")
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
@@ -314,6 +314,7 @@ func GetVolumeSnapshotClass(
|
||||
pvc *corev1api.PersistentVolumeClaim,
|
||||
log logrus.FieldLogger,
|
||||
crClient crclient.Client,
|
||||
policySnapshotClass string,
|
||||
) (*snapshotv1api.VolumeSnapshotClass, error) {
|
||||
snapshotClasses := new(snapshotv1api.VolumeSnapshotClassList)
|
||||
err := crClient.List(context.TODO(), snapshotClasses)
|
||||
@@ -331,6 +332,16 @@ func GetVolumeSnapshotClass(
|
||||
return snapshotClass, nil
|
||||
}
|
||||
|
||||
// If a snapshot class is specified by volume policy, use that
|
||||
snapshotClass, err = GetVolumeSnapshotClassFromVolumePolicy(
|
||||
policySnapshotClass, provisioner, snapshotClasses)
|
||||
if err != nil {
|
||||
log.Debugf("Didn't find VolumeSnapshotClass from volume policy: %v", err)
|
||||
}
|
||||
if snapshotClass != nil {
|
||||
return snapshotClass, nil
|
||||
}
|
||||
|
||||
// If there is no annotation in PVC, attempt to fetch it from backup annotations
|
||||
snapshotClass, err = GetVolumeSnapshotClassFromBackupAnnotationsForDriver(
|
||||
backup, provisioner, snapshotClasses)
|
||||
@@ -412,6 +423,34 @@ func GetVolumeSnapshotClassFromBackupAnnotationsForDriver(
|
||||
)
|
||||
}
|
||||
|
||||
// GetVolumeSnapshotClassFromVolumePolicy returns a VolumeSnapshotClass
|
||||
// specified by a volume policy's snapshotClass parameter. If
|
||||
// policySnapshotClass is empty, it returns nil (no match).
|
||||
func GetVolumeSnapshotClassFromVolumePolicy(
|
||||
policySnapshotClass string,
|
||||
provisioner string,
|
||||
snapshotClasses *snapshotv1api.VolumeSnapshotClassList,
|
||||
) (*snapshotv1api.VolumeSnapshotClass, error) {
|
||||
if policySnapshotClass == "" {
|
||||
return nil, nil
|
||||
}
|
||||
for _, sc := range snapshotClasses.Items {
|
||||
if strings.EqualFold(policySnapshotClass, sc.ObjectMeta.Name) {
|
||||
if !strings.EqualFold(sc.Driver, provisioner) {
|
||||
return nil, errors.Errorf(
|
||||
"VolumeSnapshotClass %s specified by volume policy is not for driver %s",
|
||||
sc.ObjectMeta.Name, provisioner,
|
||||
)
|
||||
}
|
||||
return &sc, nil
|
||||
}
|
||||
}
|
||||
return nil, errors.Errorf(
|
||||
"No CSI VolumeSnapshotClass found with name %s specified by volume policy for driver %s",
|
||||
policySnapshotClass, provisioner,
|
||||
)
|
||||
}
|
||||
|
||||
// GetVolumeSnapshotClassForStorageClass returns a VolumeSnapshotClass
|
||||
// for the supplied volume provisioner/ driver name.
|
||||
func GetVolumeSnapshotClassForStorageClass(
|
||||
|
||||
@@ -1032,7 +1032,7 @@ func TestGetVolumeSnapshotClass(t *testing.T) {
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
actualSnapshotClass, actualError := GetVolumeSnapshotClass(
|
||||
tc.driverName, tc.backup, tc.pvc, logrus.New(), fakeClient)
|
||||
tc.driverName, tc.backup, tc.pvc, logrus.New(), fakeClient, "")
|
||||
if tc.expectError {
|
||||
require.Error(t, actualError)
|
||||
assert.Nil(t, actualSnapshotClass)
|
||||
@@ -1043,6 +1043,93 @@ func TestGetVolumeSnapshotClass(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetVolumeSnapshotClassFromVolumePolicy(t *testing.T) {
|
||||
vscArray1 := &snapshotv1api.VolumeSnapshotClass{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "vsc-array-1"},
|
||||
Driver: "infinibox-csi-driver",
|
||||
}
|
||||
vscArray2 := &snapshotv1api.VolumeSnapshotClass{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "vsc-array-2"},
|
||||
Driver: "infinibox-csi-driver",
|
||||
}
|
||||
vscOther := &snapshotv1api.VolumeSnapshotClass{
|
||||
ObjectMeta: metav1.ObjectMeta{Name: "vsc-other"},
|
||||
Driver: "other-csi-driver",
|
||||
}
|
||||
|
||||
snapshotClasses := &snapshotv1api.VolumeSnapshotClassList{
|
||||
Items: []snapshotv1api.VolumeSnapshotClass{*vscArray1, *vscArray2, *vscOther},
|
||||
}
|
||||
|
||||
testCases := []struct {
|
||||
name string
|
||||
policySnapshotClass string
|
||||
provisioner string
|
||||
expectedVSC *snapshotv1api.VolumeSnapshotClass
|
||||
expectError bool
|
||||
}{
|
||||
{
|
||||
name: "empty policy returns nil",
|
||||
policySnapshotClass: "",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectedVSC: nil,
|
||||
expectError: false,
|
||||
},
|
||||
{
|
||||
name: "matching VSC with correct driver",
|
||||
policySnapshotClass: "vsc-array-1",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectedVSC: vscArray1,
|
||||
expectError: false,
|
||||
},
|
||||
{
|
||||
name: "matching VSC with correct driver second array",
|
||||
policySnapshotClass: "vsc-array-2",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectedVSC: vscArray2,
|
||||
expectError: false,
|
||||
},
|
||||
{
|
||||
name: "VSC exists but wrong driver",
|
||||
policySnapshotClass: "vsc-other",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "VSC does not exist",
|
||||
policySnapshotClass: "non-existent",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectError: true,
|
||||
},
|
||||
{
|
||||
name: "case-insensitive name matching",
|
||||
policySnapshotClass: "VSC-ARRAY-1",
|
||||
provisioner: "infinibox-csi-driver",
|
||||
expectedVSC: vscArray1,
|
||||
expectError: false,
|
||||
},
|
||||
}
|
||||
|
||||
for _, tc := range testCases {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
actualVSC, actualError := GetVolumeSnapshotClassFromVolumePolicy(
|
||||
tc.policySnapshotClass, tc.provisioner, snapshotClasses)
|
||||
if tc.expectError {
|
||||
require.Error(t, actualError)
|
||||
assert.Nil(t, actualVSC)
|
||||
return
|
||||
}
|
||||
if tc.expectedVSC == nil {
|
||||
assert.Nil(t, actualVSC)
|
||||
} else {
|
||||
require.NotNil(t, actualVSC)
|
||||
assert.Equal(t, tc.expectedVSC.Name, actualVSC.Name)
|
||||
assert.Equal(t, tc.expectedVSC.Driver, actualVSC.Driver)
|
||||
}
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
func TestGetVolumeSnapshotClassForStorageClass(t *testing.T) {
|
||||
hostpathClass := &snapshotv1api.VolumeSnapshotClass{
|
||||
ObjectMeta: metav1.ObjectMeta{
|
||||
|
||||
@@ -27,4 +27,5 @@ type VolumeHelper interface {
|
||||
ShouldPerformFSBackup(volume corev1api.Volume, pod corev1api.Pod) (bool, error)
|
||||
ShouldPerformCustomAction(obj runtime.Unstructured, groupResource schema.GroupResource, matchParams map[string]any) (bool, error)
|
||||
GetActionParameters(obj runtime.Unstructured, groupResource schema.GroupResource) (bool, string, map[string]any, error)
|
||||
GetSnapshotClass(obj runtime.Unstructured, groupResource schema.GroupResource) (string, error)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user