Merge pull request #10004 from chlins/feat/data-mover-selection

feat(resourcepolicies): support dataMover parameter in snapshot volume

changelogs/unreleased/10004-chlins

@@ -0,0 +1 @@
+Support selecting the data mover type (velero-fs or velero-block) through the volume policy snapshot action's dataMover parameter

internal/resourcepolicies/resource_policies.go

@@ -30,6 +30,7 @@ import (
 	crclient "sigs.k8s.io/controller-runtime/pkg/client"
 
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
+	datamover "github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/wildcard"
 )
 
@@ -48,6 +49,20 @@ const (
 	Custom VolumeActionType = "custom"
 )
 
+const (
+	// DataMoverParameter is the key of the action parameter that selects the data
+	// mover to be used for the matched volumes when the action type is snapshot.
+	DataMoverParameter = "dataMover"
+)
+
+// validDataMovers is the set of data mover values accepted in the snapshot
+// action's dataMover parameter.
+var validDataMovers = map[string]struct{}{
+	datamover.DataMoverTypeVelero:      {},
+	datamover.DataMoverTypeVeleroFs:    {},
+	datamover.DataMoverTypeVeleroBlock: {},
+}
+
 // Action defined as one action for a specific way of backup
 type Action struct {
 	// Type defined specific type of action, currently only support 'skip'
@@ -56,6 +71,35 @@ type Action struct {
 	Parameters map[string]any `yaml:"parameters,omitempty"`
 }
 
+// GetDataMover returns the data mover configured in the snapshot action's
+// dataMover parameter. The dataMover parameter is only meaningful for the
+// snapshot action, so it returns an error when the action is nil or its type is
+// not snapshot. When the parameter is absent, it returns the default built-in
+// data mover. The empty string and "velero" both denote the default built-in
+// data mover and are returned unchanged; normalizing them to the concrete
+// default mover is the consuming workflow's responsibility (issue #9830).
+func (a *Action) GetDataMover() (string, error) {
+	if a == nil || a.Type != Snapshot {
+		return "", fmt.Errorf("the %q parameter is only supported for the %q action", DataMoverParameter, Snapshot)
+	}
+	if len(a.Parameters) == 0 {
+		return datamover.GetDefaultBuiltInDataMover(), nil
+	}
+	raw, ok := a.Parameters[DataMoverParameter]
+	if !ok {
+		return datamover.GetDefaultBuiltInDataMover(), nil
+	}
+	dataMover, ok := raw.(string)
+	if !ok {
+		return "", fmt.Errorf("parameter %q must be a string, got %T", DataMoverParameter, raw)
+	}
+	if _, ok := validDataMovers[dataMover]; !ok {
+		return "", fmt.Errorf("invalid %q value %q, valid values are %q, %q, %q",
+			DataMoverParameter, dataMover, datamover.DataMoverTypeVelero, datamover.DataMoverTypeVeleroFs, datamover.DataMoverTypeVeleroBlock)
+	}
+	return dataMover, nil
+}
+
 // ResourceFilter defines a filter for specific resource kinds.
 type ResourceFilter struct {
 	Kinds            []string            `yaml:"kinds"`

internal/resourcepolicies/resource_policies_test.go

@@ -2845,3 +2845,70 @@ namespacedFilterPolicies:
 	assert.Nil(t, p.GetIncludeExcludePolicy())
 	assert.Nil(t, p.GetClusterScopedFilterPolicy())
 }
+
+func TestActionGetDataMover(t *testing.T) {
+	testCases := []struct {
+		name         string
+		action       *Action
+		expectedMove string
+		expectErr    bool
+	}{
+		{
+			name:      "nil action",
+			action:    nil,
+			expectErr: true,
+		},
+		{
+			name:         "snapshot action without parameters returns default mover",
+			action:       &Action{Type: Snapshot},
+			expectedMove: "velero-fs",
+		},
+		{
+			name:         "snapshot action without dataMover parameter returns default mover",
+			action:       &Action{Type: Snapshot, Parameters: map[string]any{"other": "value"}},
+			expectedMove: "velero-fs",
+		},
+		{
+			name:         "snapshot action with velero dataMover",
+			action:       &Action{Type: Snapshot, Parameters: map[string]any{"dataMover": "velero"}},
+			expectedMove: "velero",
+		},
+		{
+			name:         "snapshot action with velero-fs dataMover",
+			action:       &Action{Type: Snapshot, Parameters: map[string]any{"dataMover": "velero-fs"}},
+			expectedMove: "velero-fs",
+		},
+		{
+			name:         "snapshot action with velero-block dataMover",
+			action:       &Action{Type: Snapshot, Parameters: map[string]any{"dataMover": "velero-block"}},
+			expectedMove: "velero-block",
+		},
+		{
+			name:      "non-snapshot action returns error",
+			action:    &Action{Type: FSBackup, Parameters: map[string]any{"dataMover": "velero-fs"}},
+			expectErr: true,
+		},
+		{
+			name:      "snapshot action with non-string dataMover returns error",
+			action:    &Action{Type: Snapshot, Parameters: map[string]any{"dataMover": 123}},
+			expectErr: true,
+		},
+		{
+			name:      "snapshot action with invalid dataMover returns error",
+			action:    &Action{Type: Snapshot, Parameters: map[string]any{"dataMover": "unknown"}},
+			expectErr: true,
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			dataMover, err := tc.action.GetDataMover()
+			if tc.expectErr {
+				require.Error(t, err)
+				return
+			}
+			require.NoError(t, err)
+			assert.Equal(t, tc.expectedMove, dataMover)
+		})
+	}
+}

internal/resourcepolicies/volume_resources_validator.go

@@ -21,6 +21,8 @@ import (
 
 	"github.com/cockroachdb/errors"
 	"go.yaml.in/yaml/v3"
+
+	datamover "github.com/vmware-tanzu/velero/pkg/util/datamover"
 )
 
 const currentSupportDataVersion = "v1"
@@ -99,6 +101,22 @@ func (a *Action) validate() error {
 		return fmt.Errorf("invalid action type %s", a.Type)
 	}
 
-	// TODO validate parameters
+	// validate parameters
+	if raw, ok := a.Parameters[DataMoverParameter]; ok {
+		// the dataMover parameter is only meaningful for the snapshot action
+		if a.Type != Snapshot {
+			return fmt.Errorf("parameter %q is only supported for the %q action, but the action type is %q",
+				DataMoverParameter, Snapshot, a.Type)
+		}
+		dataMover, ok := raw.(string)
+		if !ok {
+			return fmt.Errorf("parameter %q must be a string, got %T", DataMoverParameter, raw)
+		}
+		if _, ok := validDataMovers[dataMover]; !ok {
+			return fmt.Errorf("invalid %q value %q, valid values are %q, %q, %q",
+				DataMoverParameter, dataMover, datamover.DataMoverTypeVelero, datamover.DataMoverTypeVeleroFs, datamover.DataMoverTypeVeleroBlock)
+		}
+	}
+
 	return nil
 }

internal/resourcepolicies/volume_resources_validator_test.go

@@ -549,6 +549,115 @@ func TestValidate(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "snapshot action with valid dataMover velero-fs",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       Snapshot,
+							Parameters: map[string]any{"dataMover": "velero-fs"},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "snapshot action with valid dataMover velero-block",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       Snapshot,
+							Parameters: map[string]any{"dataMover": "velero-block"},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "snapshot action with valid dataMover velero",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       Snapshot,
+							Parameters: map[string]any{"dataMover": "velero"},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: false,
+		},
+		{
+			name: "snapshot action with invalid dataMover value",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       Snapshot,
+							Parameters: map[string]any{"dataMover": "unknown-mover"},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "snapshot action with non-string dataMover value",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       Snapshot,
+							Parameters: map[string]any{"dataMover": 123},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "dataMover parameter on non-snapshot action is rejected",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action: Action{
+							Type:       FSBackup,
+							Parameters: map[string]any{"dataMover": "velero-fs"},
+						},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: true,
+		},
+		{
+			name: "snapshot action without parameters still valid",
+			res: &ResourcePolicies{
+				Version: "v1",
+				VolumePolicies: []VolumePolicy{
+					{
+						Action:     Action{Type: Snapshot},
+						Conditions: map[string]any{"storageClass": []string{"gp2"}},
+					},
+				},
+			},
+			wantErr: false,
+		},
 	}
 	for _, tc := range testCases {
 		t.Run(tc.name, func(t *testing.T) {

pkg/controller/data_download_controller.go

@@ -44,7 +44,6 @@ import (
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1"
 	"github.com/vmware-tanzu/velero/pkg/constant"
-	datamover "github.com/vmware-tanzu/velero/pkg/datamover"
 	"github.com/vmware-tanzu/velero/pkg/datapath"
 	"github.com/vmware-tanzu/velero/pkg/exposer"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
@@ -53,6 +52,7 @@ import (
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/uploader"
 	"github.com/vmware-tanzu/velero/pkg/util"
+	datamover "github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/controller/data_upload_controller.go

@@ -45,7 +45,6 @@ import (
 	velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
 	velerov2alpha1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v2alpha1"
 	"github.com/vmware-tanzu/velero/pkg/constant"
-	"github.com/vmware-tanzu/velero/pkg/datamover"
 	"github.com/vmware-tanzu/velero/pkg/datapath"
 	"github.com/vmware-tanzu/velero/pkg/exposer"
 	"github.com/vmware-tanzu/velero/pkg/metrics"
@@ -53,6 +52,7 @@ import (
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/uploader"
 	"github.com/vmware-tanzu/velero/pkg/util"
+	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/datamover/dataupload_delete_action.go

@@ -17,6 +17,7 @@ import (
 	"github.com/vmware-tanzu/velero/pkg/label"
 	"github.com/vmware-tanzu/velero/pkg/plugin/velero"
 	repotypes "github.com/vmware-tanzu/velero/pkg/repository/types"
+	datamoverutil "github.com/vmware-tanzu/velero/pkg/util/datamover"
 )
 
 type DataUploadDeleteAction struct {
@@ -88,7 +89,7 @@ func (d *DataUploadDeleteAction) Execute(input *velero.DeleteItemActionExecuteIn
 
 // generate the configmap which is to be created and used as a way to communicate the snapshot info to the backup deletion controller
 func genConfigmap(bak *velerov1.Backup, du velerov2alpha1.DataUpload) *corev1api.ConfigMap {
-	if !IsBuiltInDataMover(du.Spec.DataMover) || du.Status.SnapshotID == "" {
+	if !datamoverutil.IsBuiltInDataMover(du.Spec.DataMover) || du.Status.SnapshotID == "" {
 		return nil
 	}
 	snapshot := repotypes.SnapshotIdentifier{

pkg/datamover/util.go

@@ -16,25 +16,20 @@ limitations under the License.
 
 package datamover
 
-import "fmt"
+import (
+	"fmt"
 
-const (
-	DataMoverTypeVeleroFs    string = "velero-fs"
-	DataMoverTypeVeleroBlock string = "velero-block"
+	datamoverutil "github.com/vmware-tanzu/velero/pkg/util/datamover"
 )
 
 func GetUploaderType(dataMover string) string {
-	if dataMover == "" || dataMover == "velero" {
+	if datamoverutil.IsBuiltInDataMover(dataMover) {
 		return "kopia"
 	} else {
 		return dataMover
 	}
 }
 
-func IsBuiltInDataMover(dataMover string) bool {
-	return dataMover == "" || dataMover == "velero"
-}
-
 func GetRealSource(sourceNamespace string, pvcName string) string {
 	return fmt.Sprintf("%s/%s", sourceNamespace, pvcName)
 }

pkg/datamover/util_test.go

@@ -6,35 +6,6 @@ import (
 	"github.com/stretchr/testify/assert"
 )
 
-func TestIsBuiltInUploader(t *testing.T) {
-	testcases := []struct {
-		name      string
-		dataMover string
-		want      bool
-	}{
-		{
-			name:      "empty dataMover is builtin",
-			dataMover: "",
-			want:      true,
-		},
-		{
-			name:      "velero dataMover is builtin",
-			dataMover: "velero",
-			want:      true,
-		},
-		{
-			name:      "kopia dataMover is not builtin",
-			dataMover: "kopia",
-			want:      false,
-		},
-	}
-	for _, tc := range testcases {
-		t.Run(tc.name, func(tt *testing.T) {
-			assert.Equal(tt, tc.want, IsBuiltInDataMover(tc.dataMover))
-		})
-	}
-}
-
 func TestGetUploaderType(t *testing.T) {
 	testcases := []struct {
 		name  string

pkg/exposer/csi_snapshot.go

@@ -35,12 +35,12 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"github.com/vmware-tanzu/velero/pkg/datamover"
 	"github.com/vmware-tanzu/velero/pkg/nodeagent"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/util"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
 	"github.com/vmware-tanzu/velero/pkg/util/csi"
+	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/exposer/csi_snapshot_test.go

@@ -43,11 +43,11 @@ import (
 	clientFake "sigs.k8s.io/controller-runtime/pkg/client/fake"
 
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	"github.com/vmware-tanzu/velero/pkg/datamover"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/util"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
+	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/exposer/generic_restore.go

@@ -31,10 +31,10 @@ import (
 	"k8s.io/client-go/kubernetes"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
-	"github.com/vmware-tanzu/velero/pkg/datamover"
 	"github.com/vmware-tanzu/velero/pkg/nodeagent"
 	velerotypes "github.com/vmware-tanzu/velero/pkg/types"
 	"github.com/vmware-tanzu/velero/pkg/util/boolptr"
+	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/exposer/generic_restore_test.go

@@ -33,8 +33,8 @@ import (
 	clientTesting "k8s.io/client-go/testing"
 
 	velerov1 "github.com/vmware-tanzu/velero/pkg/apis/velero/v1"
-	"github.com/vmware-tanzu/velero/pkg/datamover"
 	velerotest "github.com/vmware-tanzu/velero/pkg/test"
+	"github.com/vmware-tanzu/velero/pkg/util/datamover"
 	"github.com/vmware-tanzu/velero/pkg/util/kube"
 )
 

pkg/util/datamover/datamover.go

@@ -0,0 +1,43 @@
+/*
+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 datamover holds the shared data mover type identifiers and helpers.
+// It must remain a leaf package (stdlib-only imports) so it can be referenced
+// from anywhere in the codebase without introducing import cycles.
+package datamover
+
+const (
+	// DataMoverTypeVelero refers to the default built-in data mover. The default
+	// data mover may change among releases; see GetDefaultBuiltInDataMover.
+	DataMoverTypeVelero = "velero"
+	// DataMoverTypeVeleroFs refers to the Velero file system data mover.
+	DataMoverTypeVeleroFs = "velero-fs"
+	// DataMoverTypeVeleroBlock refers to the Velero block data mover.
+	DataMoverTypeVeleroBlock = "velero-block"
+)
+
+// IsBuiltInDataMover reports whether the given data mover value refers to a
+// Velero built-in data mover (an empty value or the default "velero" alias).
+func IsBuiltInDataMover(dataMover string) bool {
+	return dataMover == "" || dataMover == DataMoverTypeVelero
+}
+
+// GetDefaultBuiltInDataMover returns the data mover used when the default
+// built-in data mover ("velero"/empty) is selected. The default may change
+// between releases; currently it is the file system data mover.
+func GetDefaultBuiltInDataMover() string {
+	return DataMoverTypeVeleroFs
+}

pkg/util/datamover/datamover_test.go

@@ -0,0 +1,56 @@
+/*
+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 datamover
+
+import (
+	"testing"
+
+	"github.com/stretchr/testify/assert"
+)
+
+func TestIsBuiltInDataMover(t *testing.T) {
+	testcases := []struct {
+		name      string
+		dataMover string
+		want      bool
+	}{
+		{
+			name:      "empty dataMover is builtin",
+			dataMover: "",
+			want:      true,
+		},
+		{
+			name:      "velero dataMover is builtin",
+			dataMover: "velero",
+			want:      true,
+		},
+		{
+			name:      "kopia dataMover is not builtin",
+			dataMover: "kopia",
+			want:      false,
+		},
+	}
+	for _, tc := range testcases {
+		t.Run(tc.name, func(tt *testing.T) {
+			assert.Equal(tt, tc.want, IsBuiltInDataMover(tc.dataMover))
+		})
+	}
+}
+
+func TestGetDefaultBuiltInDataMover(t *testing.T) {
+	assert.Equal(t, DataMoverTypeVeleroFs, GetDefaultBuiltInDataMover())
+}