diff --git a/changelogs/unreleased/10004-chlins b/changelogs/unreleased/10004-chlins new file mode 100644 index 000000000..142a83705 --- /dev/null +++ b/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 diff --git a/internal/resourcepolicies/resource_policies.go b/internal/resourcepolicies/resource_policies.go index 867efc74a..235f48ed5 100644 --- a/internal/resourcepolicies/resource_policies.go +++ b/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"` diff --git a/internal/resourcepolicies/resource_policies_test.go b/internal/resourcepolicies/resource_policies_test.go index 4b03b833c..445b479f0 100644 --- a/internal/resourcepolicies/resource_policies_test.go +++ b/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) + }) + } +} diff --git a/internal/resourcepolicies/volume_resources_validator.go b/internal/resourcepolicies/volume_resources_validator.go index 928e17df6..332f98d2e 100644 --- a/internal/resourcepolicies/volume_resources_validator.go +++ b/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 } diff --git a/internal/resourcepolicies/volume_resources_validator_test.go b/internal/resourcepolicies/volume_resources_validator_test.go index f2e6bf0e0..489e9c653 100644 --- a/internal/resourcepolicies/volume_resources_validator_test.go +++ b/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) { diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 06ce3479e..fc7cb1a53 100644 --- a/pkg/controller/data_download_controller.go +++ b/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" ) diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index 9b2d9a2e3..78e4d1ed3 100644 --- a/pkg/controller/data_upload_controller.go +++ b/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" ) diff --git a/pkg/datamover/dataupload_delete_action.go b/pkg/datamover/dataupload_delete_action.go index 681bb79de..a50d0fce2 100644 --- a/pkg/datamover/dataupload_delete_action.go +++ b/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{ diff --git a/pkg/datamover/util.go b/pkg/datamover/util.go index 7e37695b6..ed66d497a 100644 --- a/pkg/datamover/util.go +++ b/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) } diff --git a/pkg/datamover/util_test.go b/pkg/datamover/util_test.go index 80e2f4e16..d44f3c307 100644 --- a/pkg/datamover/util_test.go +++ b/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 diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index 6c92a6973..ed510c798 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/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" ) diff --git a/pkg/exposer/csi_snapshot_test.go b/pkg/exposer/csi_snapshot_test.go index e1512e633..e5a7aa9a7 100644 --- a/pkg/exposer/csi_snapshot_test.go +++ b/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" ) diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index 9a68b7157..0f4b9c5b4 100644 --- a/pkg/exposer/generic_restore.go +++ b/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" ) diff --git a/pkg/exposer/generic_restore_test.go b/pkg/exposer/generic_restore_test.go index 48526a5fd..b65863318 100644 --- a/pkg/exposer/generic_restore_test.go +++ b/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" ) diff --git a/pkg/util/datamover/datamover.go b/pkg/util/datamover/datamover.go new file mode 100644 index 000000000..59dd1499b --- /dev/null +++ b/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 +} diff --git a/pkg/util/datamover/datamover_test.go b/pkg/util/datamover/datamover_test.go new file mode 100644 index 000000000..8576aed0e --- /dev/null +++ b/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()) +}