From b7ffd5508505d79ac93744c12d6831e71dabb859 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Thu, 16 Jul 2026 18:37:03 -0400 Subject: [PATCH 01/15] Add set-based label selector test and docs coverage for restore Restore label selector filtering had only equality-based coverage; set-based selectors were exercised nowhere in the repo, relying entirely on apimachinery behavior. Add notin, in, and doesnotexist (!key) cases to TestRestoreResourceFiltering and set-based parse cases for the --selector CLI flag. Also document restore usage of --selector in resource-filtering.md: the notin phased-restore scenario (noting notin also matches resources without the label key), restoring only unlabeled resources via '!', and the caveat that restore item action dependencies bypass label selectors. Co-Authored-By: Claude Fable 5 Signed-off-by: Tiger Kaovilai --- pkg/cmd/util/flag/label_selector_test.go | 22 +++++ pkg/restore/restore_test.go | 93 ++++++++++++++++++++ site/content/docs/main/resource-filtering.md | 24 +++++ 3 files changed, 139 insertions(+) diff --git a/pkg/cmd/util/flag/label_selector_test.go b/pkg/cmd/util/flag/label_selector_test.go index 9d69a2718..ad2b4c711 100644 --- a/pkg/cmd/util/flag/label_selector_test.go +++ b/pkg/cmd/util/flag/label_selector_test.go @@ -24,6 +24,28 @@ func TestSetOfLabelSelector(t *testing.T) { assert.True(t, str == "k1=v1,k2=v2" || str == "k2=v2,k2=v2") } +func TestSetOfSetBasedLabelSelector(t *testing.T) { + selector := &LabelSelector{} + require.NoError(t, selector.Set("pr-label notin (1)")) + require.NotNil(t, selector.LabelSelector) + require.Len(t, selector.LabelSelector.MatchExpressions, 1) + req := selector.LabelSelector.MatchExpressions[0] + assert.Equal(t, "pr-label", req.Key) + assert.Equal(t, metav1.LabelSelectorOpNotIn, req.Operator) + assert.Equal(t, []string{"1"}, req.Values) +} + +func TestSetOfDoesNotExistLabelSelector(t *testing.T) { + selector := &LabelSelector{} + require.NoError(t, selector.Set("!pr-label")) + require.NotNil(t, selector.LabelSelector) + require.Len(t, selector.LabelSelector.MatchExpressions, 1) + req := selector.LabelSelector.MatchExpressions[0] + assert.Equal(t, "pr-label", req.Key) + assert.Equal(t, metav1.LabelSelectorOpDoesNotExist, req.Operator) + assert.Empty(t, req.Values) +} + func TestTypeOfLabelSelector(t *testing.T) { selector := &LabelSelector{} assert.Equal(t, "labelSelector", selector.Type()) diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index 074053444..942010e14 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -453,6 +453,99 @@ func TestRestoreResourceFiltering(t *testing.T) { test.PVs(): {"/pv-1"}, }, }, + { + name: "notin label selector excludes matching resources", + restore: defaultRestore().LabelSelector(&metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "pr-label", Operator: metav1.LabelSelectorOpNotIn, Values: []string{"1"}}, + }}).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + builder.ForPod("ns-2", "pod-2").Result(), + ). + AddItems("deployments.apps", + builder.ForDeployment("ns-1", "deploy-1").Result(), + builder.ForDeployment("ns-2", "deploy-2").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + ). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + builder.ForPersistentVolume("pv-2").ObjectMeta(builder.WithLabels("pr-label", "2")).Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + test.Deployments(), + test.PVs(), + }, + want: map[*test.APIResource][]string{ + test.Pods(): {"ns-2/pod-2"}, + test.Deployments(): {"ns-1/deploy-1"}, + test.PVs(): {"/pv-2"}, + }, + }, + { + name: "in label selector only restores matching resources", + restore: defaultRestore().LabelSelector(&metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "pr-label", Operator: metav1.LabelSelectorOpIn, Values: []string{"1", "2"}}, + }}).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + builder.ForPod("ns-2", "pod-2").ObjectMeta(builder.WithLabels("pr-label", "3")).Result(), + ). + AddItems("deployments.apps", + builder.ForDeployment("ns-1", "deploy-1").Result(), + builder.ForDeployment("ns-2", "deploy-2").ObjectMeta(builder.WithLabels("pr-label", "2")).Result(), + ). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1").ObjectMeta(builder.WithLabels("pr-label", "2")).Result(), + builder.ForPersistentVolume("pv-2").Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + test.Deployments(), + test.PVs(), + }, + want: map[*test.APIResource][]string{ + test.Pods(): {"ns-1/pod-1"}, + test.Deployments(): {"ns-2/deploy-2"}, + test.PVs(): {"/pv-1"}, + }, + }, + { + name: "doesnotexist label selector only restores resources without the label key", + restore: defaultRestore().LabelSelector(&metav1.LabelSelector{MatchExpressions: []metav1.LabelSelectorRequirement{ + {Key: "pr-label", Operator: metav1.LabelSelectorOpDoesNotExist}, + }}).Result(), + backup: defaultBackup().Result(), + tarball: test.NewTarWriter(t). + AddItems("pods", + builder.ForPod("ns-1", "pod-1").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + builder.ForPod("ns-2", "pod-2").Result(), + ). + AddItems("deployments.apps", + builder.ForDeployment("ns-1", "deploy-1").Result(), + builder.ForDeployment("ns-2", "deploy-2").ObjectMeta(builder.WithLabels("pr-label", "2")).Result(), + ). + AddItems("persistentvolumes", + builder.ForPersistentVolume("pv-1").ObjectMeta(builder.WithLabels("other-label", "x")).Result(), + builder.ForPersistentVolume("pv-2").ObjectMeta(builder.WithLabels("pr-label", "1")).Result(), + ). + Done(), + apiResources: []*test.APIResource{ + test.Pods(), + test.Deployments(), + test.PVs(), + }, + want: map[*test.APIResource][]string{ + test.Pods(): {"ns-2/pod-2"}, + test.Deployments(): {"ns-1/deploy-1"}, + test.PVs(): {"/pv-1"}, + }, + }, { name: "OrLabelSelectors only restores matching resources", restore: defaultRestore().OrLabelSelector([]*metav1.LabelSelector{{MatchLabels: map[string]string{"a1": "b1"}}, {MatchLabels: map[string]string{"a2": "b2"}}, diff --git a/site/content/docs/main/resource-filtering.md b/site/content/docs/main/resource-filtering.md index 9f57a7f4e..94838fe99 100644 --- a/site/content/docs/main/resource-filtering.md +++ b/site/content/docs/main/resource-filtering.md @@ -103,6 +103,30 @@ Includes cluster-scoped resources. Cannot work with `--include-cluster-scoped-re velero backup create --selector " notin ()" ``` +The same selector syntax works on restore. Set-based selectors are useful for phased restores: restore labeled resources first, then everything else. + +* Restore only resources matching the label selector. + + ```bash + velero restore create --from-backup --selector = + ``` + +* Restore everything in the backup except resources matching the selector. + + ```bash + velero restore create --from-backup --selector " notin ()" + ``` + + `notin` also matches resources that don't have the `` label at all: this restores resources whose `` label has any other value, as well as resources without the `` label. + +* Restore only resources that do not have a particular label key. + + ```bash + velero restore create --from-backup --selector '!' + ``` + +Note: resources pulled in as dependencies of selected items by restore item actions (for example, a restored pod's service account or persistent volume claims) are restored even if the label selector would exclude them. + For more information read the [Kubernetes label selector documentation](https://kubernetes.io/docs/concepts/overview/working-with-objects/labels/#label-selectors) ### --or-selector From 226fefbc29a7c823f50a9f0417f3b1a9405f0ba2 Mon Sep 17 00:00:00 2001 From: PratikMane0112 Date: Sun, 30 Aug 2026 11:00:07 +0530 Subject: [PATCH 02/15] Fix snapshot-location get --selector to filter VSLs by label Signed-off-by: PratikMane0112 --- changelogs/unreleased/XXXX-PratikMane0112 | 1 + pkg/cmd/cli/snapshotlocation/get.go | 7 ++- pkg/cmd/cli/snapshotlocation/get_test.go | 62 +++++++++++++++++++++++ 3 files changed, 68 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/XXXX-PratikMane0112 create mode 100644 pkg/cmd/cli/snapshotlocation/get_test.go diff --git a/changelogs/unreleased/XXXX-PratikMane0112 b/changelogs/unreleased/XXXX-PratikMane0112 new file mode 100644 index 000000000..8174c2b65 --- /dev/null +++ b/changelogs/unreleased/XXXX-PratikMane0112 @@ -0,0 +1 @@ +Fix snapshot-location get --selector flag to actually filter VolumeSnapshotLocations by label diff --git a/pkg/cmd/cli/snapshotlocation/get.go b/pkg/cmd/cli/snapshotlocation/get.go index 79da478bf..ff5b2af9e 100644 --- a/pkg/cmd/cli/snapshotlocation/get.go +++ b/pkg/cmd/cli/snapshotlocation/get.go @@ -1,5 +1,5 @@ /* -Copyright 2018 the Velero contributors. +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. @@ -50,7 +50,10 @@ func NewGetCommand(f client.Factory, use string) *cobra.Command { locations.Items = append(locations.Items, *location) } } else { - err = client.List(context.TODO(), locations, &kbclient.ListOptions{Namespace: f.Namespace()}) + err = client.List(context.TODO(), locations, &kbclient.ListOptions{ + Namespace: f.Namespace(), + Raw: &listOptions, + }) cmd.CheckError(err) } _, err = output.PrintWithFormat(c, locations) diff --git a/pkg/cmd/cli/snapshotlocation/get_test.go b/pkg/cmd/cli/snapshotlocation/get_test.go new file mode 100644 index 000000000..9a5ce6ef7 --- /dev/null +++ b/pkg/cmd/cli/snapshotlocation/get_test.go @@ -0,0 +1,62 @@ +/* +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 snapshotlocation + +import ( + "fmt" + "os" + "os/exec" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + + factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks" + cmdtest "github.com/vmware-tanzu/velero/pkg/cmd/test" + velerotest "github.com/vmware-tanzu/velero/pkg/test" + veleroexec "github.com/vmware-tanzu/velero/pkg/util/exec" +) + +func TestNewGetCommand(t *testing.T) { + vslList := []string{"vsl1", "vsl2"} + + f := &factorymocks.Factory{} + kbclient := velerotest.NewFakeControllerRuntimeClient(t) + f.On("Namespace").Return(mock.Anything) + f.On("KubebuilderClient").Return(kbclient, nil) + + // get command + c := NewGetCommand(f, "velero snapshot-location get") + assert.Equal(t, "Get snapshot locations", c.Short) + + c.Execute() + + if os.Getenv(cmdtest.CaptureFlag) == "1" { + c.SetArgs([]string{"vsl1", "vsl2"}) + c.Execute() + return + } + cmd := exec.CommandContext(t.Context(), os.Args[0], []string{"-test.run=TestNewGetCommand"}...) + cmd.Env = append(os.Environ(), fmt.Sprintf("%s=1", cmdtest.CaptureFlag)) + _, stderr, err := veleroexec.RunCommand(cmd) + + if err != nil { + assert.Contains(t, stderr, fmt.Sprintf("volumesnapshotlocations.velero.io \"%s\" not found", vslList[0])) + return + } + t.Fatalf("process ran with err %v, want snapshot location get to fail for non-existent VSL", err) +} From 1155635e20295e7c2351b8e9c0981d78e432f58c Mon Sep 17 00:00:00 2001 From: PratikMane0112 Date: Sun, 30 Aug 2026 11:14:19 +0530 Subject: [PATCH 03/15] Update changelog with PR no Signed-off-by: PratikMane0112 --- changelogs/unreleased/10449-PratikMane0112 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10449-PratikMane0112 diff --git a/changelogs/unreleased/10449-PratikMane0112 b/changelogs/unreleased/10449-PratikMane0112 new file mode 100644 index 000000000..8174c2b65 --- /dev/null +++ b/changelogs/unreleased/10449-PratikMane0112 @@ -0,0 +1 @@ +Fix snapshot-location get --selector flag to actually filter VolumeSnapshotLocations by label From 375ae2a1c1294ddc7ae822c47ce82140fc1d13d9 Mon Sep 17 00:00:00 2001 From: PratikMane0112 Date: Sun, 30 Aug 2026 11:15:56 +0530 Subject: [PATCH 04/15] Remove old changelog Signed-off-by: PratikMane0112 --- changelogs/unreleased/XXXX-PratikMane0112 | 1 - 1 file changed, 1 deletion(-) delete mode 100644 changelogs/unreleased/XXXX-PratikMane0112 diff --git a/changelogs/unreleased/XXXX-PratikMane0112 b/changelogs/unreleased/XXXX-PratikMane0112 deleted file mode 100644 index 8174c2b65..000000000 --- a/changelogs/unreleased/XXXX-PratikMane0112 +++ /dev/null @@ -1 +0,0 @@ -Fix snapshot-location get --selector flag to actually filter VolumeSnapshotLocations by label From 3c47a2c3269f896d60cdff2f112f2e81949a2190 Mon Sep 17 00:00:00 2001 From: PratikMane0112 Date: Mon, 31 Aug 2026 14:59:41 +0530 Subject: [PATCH 05/15] Update label selector pattern and add a test case Signed-off-by: PratikMane0112 --- pkg/cmd/cli/snapshotlocation/get.go | 9 +++-- pkg/cmd/cli/snapshotlocation/get_test.go | 42 ++++++++++++++++++++++++ 2 files changed, 48 insertions(+), 3 deletions(-) diff --git a/pkg/cmd/cli/snapshotlocation/get.go b/pkg/cmd/cli/snapshotlocation/get.go index ff5b2af9e..46c4897ca 100644 --- a/pkg/cmd/cli/snapshotlocation/get.go +++ b/pkg/cmd/cli/snapshotlocation/get.go @@ -1,5 +1,5 @@ /* -Copyright the Velero contributors. +Copyright 2018 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. @@ -21,6 +21,7 @@ import ( "github.com/spf13/cobra" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/labels" kbclient "sigs.k8s.io/controller-runtime/pkg/client" api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -50,9 +51,11 @@ func NewGetCommand(f client.Factory, use string) *cobra.Command { locations.Items = append(locations.Items, *location) } } else { + parsedSelector, err := labels.Parse(listOptions.LabelSelector) + cmd.CheckError(err) err = client.List(context.TODO(), locations, &kbclient.ListOptions{ - Namespace: f.Namespace(), - Raw: &listOptions, + LabelSelector: parsedSelector, + Namespace: f.Namespace(), }) cmd.CheckError(err) } diff --git a/pkg/cmd/cli/snapshotlocation/get_test.go b/pkg/cmd/cli/snapshotlocation/get_test.go index 9a5ce6ef7..e6ed31001 100644 --- a/pkg/cmd/cli/snapshotlocation/get_test.go +++ b/pkg/cmd/cli/snapshotlocation/get_test.go @@ -24,7 +24,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + kbclient "sigs.k8s.io/controller-runtime/pkg/client" + "github.com/vmware-tanzu/velero/pkg/builder" factorymocks "github.com/vmware-tanzu/velero/pkg/client/mocks" cmdtest "github.com/vmware-tanzu/velero/pkg/cmd/test" velerotest "github.com/vmware-tanzu/velero/pkg/test" @@ -60,3 +63,42 @@ func TestNewGetCommand(t *testing.T) { } t.Fatalf("process ran with err %v, want snapshot location get to fail for non-existent VSL", err) } + +func TestNewGetCommand_SelectorFiltersVSLs(t *testing.T) { + f := &factorymocks.Factory{} + client := velerotest.NewFakeControllerRuntimeClient(t) + + vslLabeled := builder.ForVolumeSnapshotLocation(cmdtest.VeleroNameSpace, "vsl-labeled"). + ObjectMeta(builder.WithLabels("env", "test")). + Result() + err := client.Create(t.Context(), vslLabeled, &kbclient.CreateOptions{}) + require.NoError(t, err) + + vslUnlabeled := builder.ForVolumeSnapshotLocation(cmdtest.VeleroNameSpace, "vsl-unlabeled"). + Result() + err = client.Create(t.Context(), vslUnlabeled, &kbclient.CreateOptions{}) + require.NoError(t, err) + + f.On("KubebuilderClient").Return(client, nil) + f.On("Namespace").Return(cmdtest.VeleroNameSpace) + + // get command with selector + c := NewGetCommand(f, "velero snapshot-location get") + c.SetArgs([]string{"--selector", "env=test"}) + err = c.Execute() + require.NoError(t, err) + + if os.Getenv(cmdtest.CaptureFlag) == "1" { + return + } + + cmd := exec.CommandContext(t.Context(), os.Args[0], []string{"-test.run=TestNewGetCommand_SelectorFiltersVSLs"}...) + cmd.Env = append(os.Environ(), fmt.Sprintf("%s=1", cmdtest.CaptureFlag)) + stdout, _, err := veleroexec.RunCommand(cmd) + require.NoError(t, err) + + // assert that the labeled VSL is returned + assert.Contains(t, stdout, "vsl-labeled") + // assert that the unlabeled VSL is not returned + assert.NotContains(t, stdout, "vsl-unlabeled") +} From b51a2b20d310c3ad86687bc919fc0f4eb140f529 Mon Sep 17 00:00:00 2001 From: Abhayraj Jaiswal Date: Sat, 5 Sep 2026 18:13:57 +0000 Subject: [PATCH 06/15] fix(restore): guard against nil PVC in in-place restore preflight checks Signed-off-by: Abhayraj Jaiswal --- pkg/restore/inplace/preflight.go | 7 +++++++ pkg/restore/inplace/preflight_test.go | 24 ++++++++++++++++++++++-- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/pkg/restore/inplace/preflight.go b/pkg/restore/inplace/preflight.go index 2ad76931d..cc384862b 100644 --- a/pkg/restore/inplace/preflight.go +++ b/pkg/restore/inplace/preflight.go @@ -50,6 +50,10 @@ func CheckPVCNotInUse( pvc *corev1api.PersistentVolumeClaim, restoreUID types.UID, ) error { + if pvc == nil { + return errors.New("pvc cannot be nil") + } + podList := new(corev1api.PodList) if err := cli.List(ctx, podList, &crclient.ListOptions{Namespace: pvc.Namespace}); err != nil { return errors.Wrapf(err, "failed to check whether PVC %s/%s is in use: failed to list pods in namespace %s", pvc.Namespace, pvc.Name, pvc.Namespace) @@ -139,6 +143,9 @@ func gatedByThisRestore(pod *corev1api.Pod, restoreUID types.UID) bool { // bound to a different PV (the documented cross-namespace clone-and-restore // workflow), and when the backed-up PV name is unknown. func CheckPVCBoundToBackedUpPV(existingPVC *corev1api.PersistentVolumeClaim, backedUpPVName, sourceNamespace string) error { + if existingPVC == nil { + return errors.New("existing PVC cannot be nil") + } if existingPVC.Status.Phase != corev1api.ClaimBound { return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s is not bound (phase %s)", existingPVC.Namespace, existingPVC.Name, existingPVC.Status.Phase) diff --git a/pkg/restore/inplace/preflight_test.go b/pkg/restore/inplace/preflight_test.go index 094a70273..8588951f9 100644 --- a/pkg/restore/inplace/preflight_test.go +++ b/pkg/restore/inplace/preflight_test.go @@ -82,10 +82,16 @@ func TestCheckPVCNotInUse(t *testing.T) { tests := []struct { name string pods []*corev1api.Pod + pvc *corev1api.PersistentVolumeClaim restoreUID types.UID expectPass bool expectMessage []string + expectError string }{ + { + name: "nil PVC returns error", + expectError: "pvc cannot be nil", + }, { name: "no pods, check passes", expectPass: true, @@ -183,11 +189,19 @@ func TestCheckPVCNotInUse(t *testing.T) { } cli := velerotest.NewFakeControllerRuntimeClient(t, objs...) - pvc := &corev1api.PersistentVolumeClaim{ - ObjectMeta: metav1.ObjectMeta{Name: "pvc-1", Namespace: "default"}, + pvc := tc.pvc + if pvc == nil && tc.name != "nil PVC returns error" { + pvc = &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-1", Namespace: "default"}, + } } err := CheckPVCNotInUse(t.Context(), cli, pvc, tc.restoreUID) + if tc.expectError != "" { + require.Error(t, err) + assert.EqualError(t, err, tc.expectError) + return + } if tc.expectPass { require.NoError(t, err) return @@ -216,6 +230,12 @@ func TestCheckPVCBoundToBackedUpPV(t *testing.T) { backedUpPVName string expectError string }{ + { + name: "nil existing PVC returns error", + existingPVC: nil, + backedUpPVName: "pv-1", + expectError: "existing PVC cannot be nil", + }, { name: "bound to the backed-up PV, check passes", existingPVC: pvc("default", "pv-1", corev1api.ClaimBound), From 770a251ed5710762c3074b3179efa8aa1af9c83c Mon Sep 17 00:00:00 2001 From: Abhayraj Jaiswal Date: Mon, 7 Sep 2026 08:07:24 +0000 Subject: [PATCH 07/15] fix(restore): propagate context and standardize errors in CSI PVC restore action Signed-off-by: Abhayraj Jaiswal --- pkg/restore/actions/csi/pvc_action.go | 18 +++++++++--------- pkg/restore/actions/csi/pvc_action_test.go | 21 ++++++++++++++++++++- 2 files changed, 29 insertions(+), 10 deletions(-) diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index f71d0965a..363507709 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -228,7 +228,7 @@ func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input * var dataUploadResult *velerov2alpha1.DataUploadResult dataUploadResult, err = getDataUploadResult(ctx, input.Restore, pvc, p.crClient) if err != nil { - return nil, errors.Wrapf(err, "fail get DataUploadResult for restore: %s", input.Restore.Name) + return nil, errors.Wrapf(err, "failed to get DataUploadResult for restore: %s", input.Restore.Name) } var volumeSnapshot *snapshotv1api.VolumeSnapshot @@ -279,10 +279,10 @@ func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input * var dataDownload *velerov2alpha1.DataDownload dataDownload, err = restoreFromDataUploadResult( - context.Background(), dataUploadResult, input.Restore, backup, pvc, existingPV, newNamespace, + ctx, dataUploadResult, input.Restore, backup, pvc, existingPV, newNamespace, operationID, string(restoreType), volumeSnapshot, p.crClient) if err != nil { - logger.Errorf("Fail to restore from DataUploadResult: %s", err.Error()) + logger.Errorf("Failed to restore from DataUploadResult: %s", err.Error()) return nil, errors.WithStack(err) } logger.Infof("DataDownload %s/%s is created successfully.", @@ -339,7 +339,7 @@ func (p *pvcRestoreItemAction) Progress( p.crClient, ) if err != nil { - logger.Errorf("fail to get DataDownload: %s", err.Error()) + logger.Errorf("Failed to get DataDownload: %s", err.Error()) return progress, err } if dataDownload.Status.Phase == velerov2alpha1.DataDownloadPhaseNew || @@ -392,13 +392,13 @@ func (p *pvcRestoreItemAction) Cancel( p.crClient, ) if err != nil { - logger.Errorf("fail to get DataDownload: %s", err.Error()) + logger.Errorf("Failed to get DataDownload: %s", err.Error()) return err } err = cancelDataDownload(context.Background(), p.crClient, dataDownload) if err != nil { - logger.Errorf("fail to cancel DataDownload %s: %s", dataDownload.Name, err.Error()) + logger.Errorf("Failed to cancel DataDownload %s: %s", dataDownload.Name, err.Error()) } return err } @@ -601,7 +601,7 @@ func restoreFromDataUploadResult( ) err := crClient.Create(ctx, dataDownload) if err != nil { - return nil, errors.Wrapf(err, "fail to create DataDownload") + return nil, errors.Wrapf(err, "failed to create DataDownload") } return dataDownload, nil @@ -654,8 +654,8 @@ func (p *pvcRestoreItemAction) deleteExistingPVC(ctx context.Context, logger *lo var err error logger.Info("ExistingVolumeDataPolicy is in-place restore. Deleting the existing PVC but keep the PV...") pv := &corev1api.PersistentVolume{} - if err = p.crClient.Get(context.Background(), crclient.ObjectKey{Name: existingPVC.Spec.VolumeName}, pv); err != nil { - return nil, errors.Errorf("Fail to get PV %s: %s", existingPVC.Spec.VolumeName, err.Error()) + if err = p.crClient.Get(ctx, crclient.ObjectKey{Name: existingPVC.Spec.VolumeName}, pv); err != nil { + return nil, errors.Wrapf(err, "failed to get PV %s", existingPVC.Spec.VolumeName) } // set reclaim policy to retain diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index b8e4912f6..acdfa5938 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -450,7 +450,7 @@ func TestExecute(t *testing.T) { restore: builder.ForRestore("velero", "testRestore").Backup("testBackup").Result(), pvc: builder.ForPersistentVolumeClaim("velero", "testPVC").ObjectMeta(builder.WithAnnotations(velerov1api.VolumeSnapshotLabel, "vsName", velerov1api.VolumeSnapshotRestoreSize, "10Gi", velerov1api.DataUploadNameAnnotation, "velero/")).Result(), expectedPVC: builder.ForPersistentVolumeClaim("velero", "testPVC").Result(), - expectedErr: "fail get DataUploadResult for restore: testRestore: no DataUpload result cm found with labels velero.io/pvc-namespace-name=velero.testPVC,velero.io/restore-uid=,velero.io/resource-usage=DataUpload", + expectedErr: "failed to get DataUploadResult for restore: testRestore: no DataUpload result cm found with labels velero.io/pvc-namespace-name=velero.testPVC,velero.io/restore-uid=,velero.io/resource-usage=DataUpload", }, { name: "Restore from DataUploadResult", @@ -884,3 +884,22 @@ func TestNewPvcRestoreItemAction(t *testing.T) { _, err1 := plugin1(logger) require.NoError(t, err1) } + +func TestDeleteExistingPVCFailure(t *testing.T) { + pvcRIA := pvcRestoreItemAction{ + log: logrus.New(), + crClient: velerotest.NewFakeControllerRuntimeClient(t), + kubeClient: fake.NewSimpleClientset(), + } + existingPVC := builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + VolumeName("non-existent-pv"). + Phase(corev1api.ClaimBound).Result() + targetPVC := builder.ForPersistentVolumeClaim("ns-1", "pvc-1").Result() + + returnedPV, err := pvcRIA.deleteExistingPVC( + t.Context(), logrus.New().WithField("test", "fail-to-get-pv"), + targetPVC, existingPVC, time.Minute) + require.Error(t, err) + assert.Nil(t, returnedPV) + assert.Contains(t, err.Error(), "failed to get PV non-existent-pv") +} From 98e51ae8e6f243383945c7bab3618eeaadb7139d Mon Sep 17 00:00:00 2001 From: krishhna24 Date: Thu, 10 Sep 2026 18:38:15 +0530 Subject: [PATCH 08/15] docs: fix VolumeGroupSnapshotClass example The example VolumeGroupSnapshotClass cannot be applied as written. It uses apiVersion v1alpha1, which external-snapshotter v8.2.0+ does not serve, and nests driver and deletionPolicy under spec. Both fields are top level and required on the CRD, which has no spec field at all. Applying the documented example fails with: no matches for kind "VolumeGroupSnapshotClass" in version "groupsnapshot.storage.k8s.io/v1alpha1" The same page already states that Velero 1.18.1+ uses the v1beta2 API and requires external-snapshotter v8.2.0 or later, so the example also contradicts its own prerequisites section. Verified against a kind cluster running external-snapshotter v8.6.0: the corrected example passes kubectl apply --dry-run=server, and the CRD reports driver and deletionPolicy as top level and required across v1, v1beta1 and v1beta2. Signed-off-by: krishhna24 --- site/content/docs/main/volume-group-snapshots.md | 7 +++---- site/content/docs/v1.18/volume-group-snapshots.md | 7 +++---- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/site/content/docs/main/volume-group-snapshots.md b/site/content/docs/main/volume-group-snapshots.md index e62a75624..9b77979ce 100644 --- a/site/content/docs/main/volume-group-snapshots.md +++ b/site/content/docs/main/volume-group-snapshots.md @@ -160,15 +160,14 @@ kubectl get volumegroupsnapshotclass -o wide **Important:** The VolumeGroupSnapshotClass must have the label `velero.io/csi-volumegroupsnapshot-class: "true"` for Velero to automatically discover and use it: ```yaml -apiVersion: groupsnapshot.storage.k8s.io/v1alpha1 +apiVersion: groupsnapshot.storage.k8s.io/v1beta2 kind: VolumeGroupSnapshotClass metadata: name: csi-vgs-class labels: velero.io/csi-volumegroupsnapshot-class: "true" -spec: - driver: ebs.csi.aws.com - deletionPolicy: Delete +driver: ebs.csi.aws.com +deletionPolicy: Delete ``` Verify your VolumeGroupSnapshotClass has the correct label: diff --git a/site/content/docs/v1.18/volume-group-snapshots.md b/site/content/docs/v1.18/volume-group-snapshots.md index e62a75624..9b77979ce 100644 --- a/site/content/docs/v1.18/volume-group-snapshots.md +++ b/site/content/docs/v1.18/volume-group-snapshots.md @@ -160,15 +160,14 @@ kubectl get volumegroupsnapshotclass -o wide **Important:** The VolumeGroupSnapshotClass must have the label `velero.io/csi-volumegroupsnapshot-class: "true"` for Velero to automatically discover and use it: ```yaml -apiVersion: groupsnapshot.storage.k8s.io/v1alpha1 +apiVersion: groupsnapshot.storage.k8s.io/v1beta2 kind: VolumeGroupSnapshotClass metadata: name: csi-vgs-class labels: velero.io/csi-volumegroupsnapshot-class: "true" -spec: - driver: ebs.csi.aws.com - deletionPolicy: Delete +driver: ebs.csi.aws.com +deletionPolicy: Delete ``` Verify your VolumeGroupSnapshotClass has the correct label: From d4f6eee8997db7e6639c5139a1764310e1aa7e3c Mon Sep 17 00:00:00 2001 From: krishhna24 Date: Thu, 10 Sep 2026 18:39:04 +0530 Subject: [PATCH 09/15] Add changelog for #10520 Signed-off-by: krishhna24 --- changelogs/unreleased/10520-krishhna24 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10520-krishhna24 diff --git a/changelogs/unreleased/10520-krishhna24 b/changelogs/unreleased/10520-krishhna24 new file mode 100644 index 000000000..6f6f184de --- /dev/null +++ b/changelogs/unreleased/10520-krishhna24 @@ -0,0 +1 @@ +Fix the VolumeGroupSnapshotClass example in the VolumeGroupSnapshot docs From bb01691e6b9b717d44284396f44b18a93a59adf1 Mon Sep 17 00:00:00 2001 From: chlins Date: Thu, 10 Sep 2026 13:48:44 +0800 Subject: [PATCH 10/15] Add in-place restore pre-flight check: PVC must be large enough for the source volume Compare the existing PVC's capacity against the source volume size recorded in the backup volume info (#10506) before any side effect and skip the volume when it is too small, so the restore fails early instead of running out of space midway. For the block data mover the source size is the device size; for the file system data movers it is the logical size of the backed-up files, a lower bound since file system metadata is not accounted for. The file system path reads the size from the volume info already carried in RestoreData. The PVC CSI RIA has no access to the volume info, so the restore engine carries the size on the PVC item through a Velero-internal annotation, the same mechanism as the selected-node carrier; both carrier annotations are stripped before the item is created in the cluster. The check is skipped when the source size is unknown (backups taken before it was recorded) or the PVC's capacity is not reported. Signed-off-by: chlins --- changelogs/unreleased/10512-chlins | 1 + internal/volume/volumes_information.go | 11 +++ pkg/apis/velero/v1/labels_annotations.go | 7 ++ pkg/podvolume/restorer.go | 7 +- pkg/podvolume/restorer_test.go | 32 +++++++++ pkg/restore/actions/csi/pvc_action.go | 11 +++ pkg/restore/actions/csi/pvc_action_test.go | 22 +++++- pkg/restore/inplace/preflight.go | 24 +++++++ pkg/restore/inplace/preflight_test.go | 65 +++++++++++++++++ pkg/restore/restore.go | 53 ++++++++++---- pkg/restore/restore_test.go | 81 ++++++++++++++++++++++ 11 files changed, 297 insertions(+), 17 deletions(-) create mode 100644 changelogs/unreleased/10512-chlins diff --git a/changelogs/unreleased/10512-chlins b/changelogs/unreleased/10512-chlins new file mode 100644 index 000000000..b22d0c86f --- /dev/null +++ b/changelogs/unreleased/10512-chlins @@ -0,0 +1 @@ +Add in-place restore pre-flight check: PVC must be large enough for the backed-up data diff --git a/internal/volume/volumes_information.go b/internal/volume/volumes_information.go index c5af900cb..4030eee63 100644 --- a/internal/volume/volumes_information.go +++ b/internal/volume/volumes_information.go @@ -368,6 +368,17 @@ func newPodVolumeInfoFromPVR(pvr *velerov1api.PodVolumeRestore) *PodVolumeRestor } } +// SourceSize returns the size of the source volume recorded at backup time, or 0 if unknown. +func (v BackupVolumeInfo) SourceSize() int64 { + switch { + case v.SnapshotDataMovementInfo != nil: + return v.SnapshotDataMovementInfo.SourceSize + case v.PVBInfo != nil: + return v.PVBInfo.SourceSize + } + return 0 +} + // PVInfo is used to store some PV information modified after creation. // Those information are lost after PV recreation. type PVInfo struct { diff --git a/pkg/apis/velero/v1/labels_annotations.go b/pkg/apis/velero/v1/labels_annotations.go index 5636ecd36..0a401cd45 100644 --- a/pkg/apis/velero/v1/labels_annotations.go +++ b/pkg/apis/velero/v1/labels_annotations.go @@ -185,6 +185,13 @@ const ( // on the cluster. Using a carrier annotation avoids any dependency on the execution order // of RestoreItemActions. InplaceRestoreSelectedNodeAnnotation = "restore.velero.io/inplace-restore-selected-node" + + // InplaceRestoreSourceSizeAnnotation is a Velero-internal carrier annotation set by the + // restore engine on a PVC item before RestoreItemActions run. It carries the size of the + // source volume recorded in the backup volume info, so the PVC CSI RestoreItemAction can + // run the in-place restore capacity pre-flight check without access to the volume info. + // The annotation is always stripped by the restore engine; it never lands on the cluster. + InplaceRestoreSourceSizeAnnotation = "restore.velero.io/inplace-restore-source-size" // SkippedNoCSIPVAnnotation - Velero checks this annotation on processed PVC to // find out if the snapshot was skipped b/c the PV is not provisioned via CSI SkippedNoCSIPVAnnotation = "backup.velero.io/skipped-no-csi-pv" diff --git a/pkg/podvolume/restorer.go b/pkg/podvolume/restorer.go index 488ce7331..232b6b518 100644 --- a/pkg/podvolume/restorer.go +++ b/pkg/podvolume/restorer.go @@ -189,7 +189,12 @@ func (r *restorer) RestorePodVolumes(data RestoreData, tracker *volume.RestoreVo // to write into, and they cannot write to it themselves until this // restore's PodVolumeRestores complete. if data.Restore.IsVolumeDataInplaceRestore() && pvc != nil { - if err := inplace.CheckPVCBoundToBackedUpPV(pvc, backedUpPVName(data.BackupVolumeInfos, data.SourceNamespace, pvc.Name), data.SourceNamespace); err != nil { + pvName := backedUpPVName(data.BackupVolumeInfos, data.SourceNamespace, pvc.Name) + if err := inplace.CheckPVCBoundToBackedUpPV(pvc, pvName, data.SourceNamespace); err != nil { + errs = append(errs, err) + continue + } + if err := inplace.CheckPVCCapacity(pvc, data.BackupVolumeInfos[pvName].SourceSize()); err != nil { errs = append(errs, err) continue } diff --git a/pkg/podvolume/restorer_test.go b/pkg/podvolume/restorer_test.go index 3d39d0a05..78e59fd1c 100644 --- a/pkg/podvolume/restorer_test.go +++ b/pkg/podvolume/restorer_test.go @@ -27,6 +27,7 @@ import ( "github.com/stretchr/testify/assert" appsv1api "k8s.io/api/apps/v1" corev1api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/client-go/kubernetes" @@ -439,6 +440,37 @@ func TestRestorePodVolumes(t *testing.T) { }, }, }, + { + name: "in-place restore blocked when the PVC is too small for the source volume", + pvbs: []*velerov1api.PodVolumeBackup{ + createPVBObj(true, true, 1, "kopia"), + }, + inplace: true, + kubeClientObj: []runtime.Object{ + createNodeAgentDaemonset(), + func() *corev1api.PersistentVolumeClaim { + pvc := createPVCObj(1) + pvc.Status.Capacity = corev1api.ResourceList{corev1api.ResourceStorage: resource.MustParse("100Mi")} + return pvc + }(), + }, + ctlClientObj: []runtime.Object{ + createBackupRepoObj(), + }, + restoredPod: createPodObj(true, true, true, 1), + sourceNamespace: "fake-ns", + bsl: "fake-bsl", + volumeInfos: map[string]volume.BackupVolumeInfo{ + "fake-pv-1": {PVCNamespace: "fake-ns", PVCName: "fake-pvc-1", PVBInfo: &volume.PodVolumeBackupInfo{SourceSize: 200 << 20}}, + }, + runtimeScheme: scheme, + errs: []expectError{ + { + err: "in-place restore pre-flight check failed, skipping volume data restore: PVC fake-ns/fake-pvc-1 capacity 100Mi is smaller than the backed-up volume size 209715200 bytes", + prefixOnly: true, + }, + }, + }, { name: "in-place restore proceeds when the PVC is only used by the gated restored pod", pvbs: []*velerov1api.PodVolumeBackup{ diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index f71d0965a..e4ab17534 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "strconv" "time" "github.com/cockroachdb/errors" @@ -238,6 +239,9 @@ func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input * if err := inplace.CheckPVCBoundToBackedUpPV(existingPVC, pvcFromBackup.Spec.VolumeName, pvcFromBackup.Namespace); err != nil { return nil, errors.WithStack(err) } + if err := inplace.CheckPVCCapacity(existingPVC, sourceSizeFromCarrier(pvc)); err != nil { + return nil, errors.WithStack(err) + } if err := inplace.CheckPVCNotInUse(ctx, p.crClient, existingPVC, input.Restore.UID); err != nil { return nil, errors.WithStack(err) } @@ -729,6 +733,13 @@ func (p *pvcRestoreItemAction) createVolumeSnapshot(ctx context.Context, logger return vs, nil } +// sourceSizeFromCarrier reads the source volume size the restore engine carries on the PVC +// item from the backup volume info, or 0 if absent or malformed. +func sourceSizeFromCarrier(pvc *corev1api.PersistentVolumeClaim) int64 { + size, _ := strconv.ParseInt(pvc.Annotations[velerov1api.InplaceRestoreSourceSizeAnnotation], 10, 64) + return size +} + func NewPvcRestoreItemAction(f client.Factory) plugincommon.HandlerInitializer { return func(logger logrus.FieldLogger) (any, error) { crClient, err := f.KubebuilderClient() diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index b8e4912f6..6e5cdb89a 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -757,6 +757,8 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { name string pod *corev1api.Pod backedUpPVName string + sourceSize string // carried on the PVC item by the restore engine + pvcCapacity string expectBlock string }{ { @@ -774,6 +776,17 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { backedUpPVName: "backupPV", expectBlock: "was bound to PV backupPV at backup time", }, + { + // Backed-up PV unknown so the same-volume skip does not apply. + name: "PVC smaller than the source volume blocks the restore", + sourceSize: "209715200", + pvcCapacity: "100Mi", + expectBlock: "capacity 100Mi is smaller than the backed-up volume size 209715200 bytes", + }, + { + name: "source size not carried skips the capacity check", + pvcCapacity: "100Mi", + }, } for _, tc := range tests { @@ -781,6 +794,9 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { existingPVC := builder.ForPersistentVolumeClaim("velero", "testPVC"). VolumeName("testPV"). Phase(corev1api.ClaimBound).Result() + if tc.pvcCapacity != "" { + existingPVC.Status.Capacity = corev1api.ResourceList{corev1api.ResourceStorage: resource.MustParse(tc.pvcCapacity)} + } existingPV := builder.ForPersistentVolume("testPV").Result() backup := builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result() restore := builder.ForRestore("velero", "testRestore").Backup("testBackup"). @@ -811,7 +827,11 @@ func TestExecuteInplaceRestorePreflight(t *testing.T) { kubeClient: fake.NewSimpleClientset(kubeObjects...), } - pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup.DeepCopy()) + item := pvcFromBackup.DeepCopy() + if tc.sourceSize != "" { + item.Annotations[velerov1api.InplaceRestoreSourceSizeAnnotation] = tc.sourceSize + } + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(item) require.NoError(t, err) pvcFromBackupMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup) require.NoError(t, err) diff --git a/pkg/restore/inplace/preflight.go b/pkg/restore/inplace/preflight.go index 2ad76931d..df2296290 100644 --- a/pkg/restore/inplace/preflight.go +++ b/pkg/restore/inplace/preflight.go @@ -26,6 +26,7 @@ import ( "github.com/cockroachdb/errors" corev1api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" "k8s.io/apimachinery/pkg/types" crclient "sigs.k8s.io/controller-runtime/pkg/client" @@ -149,3 +150,26 @@ func CheckPVCBoundToBackedUpPV(existingPVC *corev1api.PersistentVolumeClaim, bac return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s is bound to PV %s, but was bound to PV %s at backup time", existingPVC.Namespace, existingPVC.Name, existingPVC.Spec.VolumeName, backedUpPVName) } + +// CheckPVCCapacity verifies the existing PVC is large enough to hold the +// backed-up volume, failing early instead of letting the restore run out of +// space midway. sourceSize is the size of the source volume recorded at +// backup time: the device size for the block data mover, the logical size of +// the backed-up files for the file system data movers (a lower bound, since +// file system metadata is not accounted for). The check is skipped when the +// size is unknown (backups taken before it was recorded) or when the PVC's +// capacity is not reported. +func CheckPVCCapacity(existingPVC *corev1api.PersistentVolumeClaim, sourceSize int64) error { + if sourceSize <= 0 { + return nil + } + capacity, ok := existingPVC.Status.Capacity[corev1api.ResourceStorage] + if !ok || capacity.IsZero() { + return nil + } + if capacity.Cmp(*resource.NewQuantity(sourceSize, resource.BinarySI)) < 0 { + return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s capacity %s is smaller than the backed-up volume size %d bytes", + existingPVC.Namespace, existingPVC.Name, capacity.String(), sourceSize) + } + return nil +} diff --git a/pkg/restore/inplace/preflight_test.go b/pkg/restore/inplace/preflight_test.go index 094a70273..96e0051ee 100644 --- a/pkg/restore/inplace/preflight_test.go +++ b/pkg/restore/inplace/preflight_test.go @@ -22,6 +22,7 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/api/resource" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/types" @@ -263,3 +264,67 @@ func TestCheckPVCBoundToBackedUpPV(t *testing.T) { }) } } + +func TestCheckPVCCapacity(t *testing.T) { + pvc := func(capacity string) *corev1api.PersistentVolumeClaim { + p := &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-1", Namespace: "default"}, + } + if capacity != "" { + p.Status.Capacity = corev1api.ResourceList{corev1api.ResourceStorage: resource.MustParse(capacity)} + } + return p + } + const mi = int64(1 << 20) + + tests := []struct { + name string + existingPVC *corev1api.PersistentVolumeClaim + sourceSize int64 + expectError string + }{ + { + name: "capacity larger than source volume, check passes", + existingPVC: pvc("100Mi"), + sourceSize: 50 * mi, + }, + { + name: "capacity equal to source volume, check passes", + existingPVC: pvc("100Mi"), + sourceSize: 100 * mi, + }, + { + name: "capacity smaller than source volume, check fails", + existingPVC: pvc("50Mi"), + sourceSize: 100 * mi, + expectError: "capacity 50Mi is smaller than the backed-up volume size 104857600 bytes", + }, + { + name: "unknown source size is skipped", + existingPVC: pvc("50Mi"), + sourceSize: 0, + }, + { + name: "missing capacity is skipped", + existingPVC: pvc(""), + sourceSize: 100 * mi, + }, + { + name: "capacity in decimal units compares by value", + existingPVC: pvc("104857600"), + sourceSize: 100 * mi, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + err := CheckPVCCapacity(tc.existingPVC, tc.sourceSize) + if tc.expectError == "" { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), tc.expectError) + }) + } +} diff --git a/pkg/restore/restore.go b/pkg/restore/restore.go index 247c8abb3..b2ef9edf4 100644 --- a/pkg/restore/restore.go +++ b/pkg/restore/restore.go @@ -27,6 +27,7 @@ import ( "reflect" "slices" "sort" + "strconv" "strings" "sync" "time" @@ -1636,15 +1637,23 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso return warnings, errs, itemExists } - // Strip any pre-existing Velero-internal in-place restore carrier annotation coming from - // the backup metadata before RestoreItemActions run. The carrier is only trusted when it - // is set by a RestoreItemAction (the PVC CSI RIA) during this restore; a stale carrier - // baked into the backup must not be translated into the Kubernetes "selected-node" - // annotation, which could pin a newly provisioned PVC to a stale node. - if annotations := obj.GetAnnotations(); annotations != nil { - if _, present := annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]; present { - restoreLogger.Infof("Removing pre-existing %q annotation from backup metadata", velerov1api.InplaceRestoreSelectedNodeAnnotation) - delete(annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + // Strip any pre-existing Velero-internal in-place restore carrier annotations coming from + // the backup metadata before RestoreItemActions run. A carrier is only trusted when it is + // set during this restore (by the engine below or by the PVC CSI RIA); a stale carrier + // baked into the backup must not be acted on, e.g. a stale "selected-node" could pin a + // newly provisioned PVC to a stale node. + stripInplaceRestoreCarrierAnnotations(obj) + + // Carry the source volume size from the backup volume info to the PVC CSI RIA, which has no + // access to the volume info, so it can run the in-place restore capacity pre-flight check. + if groupResource == kuberesource.PersistentVolumeClaims { + pvName, _, _ := unstructured.NestedString(obj.Object, "spec", "volumeName") + if sourceSize := ctx.backupVolumeInfoMap[pvName].SourceSize(); sourceSize > 0 { + annotations := obj.GetAnnotations() + if annotations == nil { + annotations = map[string]string{} + } + annotations[velerov1api.InplaceRestoreSourceSizeAnnotation] = strconv.FormatInt(sourceSize, 10) obj.SetAnnotations(annotations) } } @@ -1788,15 +1797,13 @@ func (ctx *restoreContext) restoreItem(obj *unstructured.Unstructured, groupReso // while the carrier annotation passes through untouched. The carrier itself is always // stripped so it never lands on the cluster. if annotations := obj.GetAnnotations(); annotations != nil { - if selectedNode, present := annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]; present { - if selectedNode != "" { - restoreLogger.Infof("Restoring %q annotation with value %q from in-place restore carrier annotation", kube.KubeAnnSelectedNode, selectedNode) - annotations[kube.KubeAnnSelectedNode] = selectedNode - } - delete(annotations, velerov1api.InplaceRestoreSelectedNodeAnnotation) + if selectedNode := annotations[velerov1api.InplaceRestoreSelectedNodeAnnotation]; selectedNode != "" { + restoreLogger.Infof("Restoring %q annotation with value %q from in-place restore carrier annotation", kube.KubeAnnSelectedNode, selectedNode) + annotations[kube.KubeAnnSelectedNode] = selectedNode obj.SetAnnotations(annotations) } } + stripInplaceRestoreCarrierAnnotations(obj) // This comes after running item actions because we have built-in actions that restore // a PVC's associated PV (if applicable). As part of the PV being restored, the 'pvsToProvision' @@ -2513,6 +2520,22 @@ func resetMetadataAndStatus(obj *unstructured.Unstructured) (*unstructured.Unstr return obj, nil } +// inplaceRestoreCarrierAnnotations are the Velero-internal annotations used to pass data +// between the restore engine and the in-place restore RestoreItemActions. They never land on +// the cluster. +var inplaceRestoreCarrierAnnotations = []string{ + velerov1api.InplaceRestoreSelectedNodeAnnotation, + velerov1api.InplaceRestoreSourceSizeAnnotation, +} + +func stripInplaceRestoreCarrierAnnotations(obj metav1.Object) { + annotations := obj.GetAnnotations() + for _, k := range inplaceRestoreCarrierAnnotations { + delete(annotations, k) + } + obj.SetAnnotations(annotations) +} + // addRestoreLabels labels the provided object with the restore name and the // restored backup's name. func addRestoreLabels(obj metav1.Object, restoreName, backupName string) { diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index b1c1475b6..974b45fbf 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -5160,3 +5160,84 @@ func TestHasPodVolumeBackup(t *testing.T) { }) } } + +func TestRestoreInplaceSourceSizeCarrierAnnotation(t *testing.T) { + newRequest := func(t *testing.T, h *harness, volumeInfos map[string]volume.BackupVolumeInfo) *Request { + t.Helper() + return &Request{ + Log: h.log, + Restore: defaultRestore().Result(), + Backup: defaultBackup().Result(), + BackupReader: test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1").VolumeName("pv-1").Result()). + Done(), + BackupVolumeInfoMap: volumeInfos, + } + } + + // captureCarrier records the source-size carrier the RIA sees on the item. + captureCarrier := func(seen *string) riav2.RestoreItemAction { + return &pluggableAction{ + executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { + item := input.Item.(*unstructured.Unstructured) + *seen = item.GetAnnotations()[velerov1api.InplaceRestoreSourceSizeAnnotation] + return &velero.RestoreItemActionExecuteOutput{UpdatedItem: item}, nil + }, + } + } + + t.Run("source size from volume info is carried to RIAs and stripped from the cluster object", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + var seen string + + warnings, errs := h.restorer.Restore( + newRequest(t, h, map[string]volume.BackupVolumeInfo{ + "pv-1": {PVCNamespace: "ns-1", PVCName: "pvc-1", PVBInfo: &volume.PodVolumeBackupInfo{SourceSize: 31457288}}, + }), + []riav2.RestoreItemAction{captureCarrier(&seen)}, + nil, + ) + assertEmptyResults(t, warnings, errs) + assert.Equal(t, "31457288", seen) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + assert.NotContains(t, got.GetAnnotations(), velerov1api.InplaceRestoreSourceSizeAnnotation) + }) + + t.Run("no carrier when the volume info has no source size", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + var seen string + + warnings, errs := h.restorer.Restore( + newRequest(t, h, map[string]volume.BackupVolumeInfo{ + "pv-1": {PVCNamespace: "ns-1", PVCName: "pvc-1", PVBInfo: &volume.PodVolumeBackupInfo{}}, + }), + []riav2.RestoreItemAction{captureCarrier(&seen)}, + nil, + ) + assertEmptyResults(t, warnings, errs) + assert.Empty(t, seen) + }) + + t.Run("stale carrier from the backup metadata is not trusted", func(t *testing.T) { + h := newHarness(t) + h.AddItems(t, test.PVCs()) + var seen string + + req := newRequest(t, h, nil) + req.BackupReader = test.NewTarWriter(t). + AddItems("persistentvolumeclaims", builder.ForPersistentVolumeClaim("ns-1", "pvc-1"). + ObjectMeta(builder.WithAnnotations(velerov1api.InplaceRestoreSourceSizeAnnotation, "999")).Result()). + Done() + warnings, errs := h.restorer.Restore(req, []riav2.RestoreItemAction{captureCarrier(&seen)}, nil) + assertEmptyResults(t, warnings, errs) + assert.Empty(t, seen) + + got, err := h.DynamicClient.Resource(test.PVCs().GVR()).Namespace("ns-1").Get(t.Context(), "pvc-1", metav1.GetOptions{}) + require.NoError(t, err) + assert.NotContains(t, got.GetAnnotations(), velerov1api.InplaceRestoreSourceSizeAnnotation) + }) +} From 7d31c708e82faf4a0f769829d1775565cbb3ec8d Mon Sep 17 00:00:00 2001 From: Daniel Mungai Date: Fri, 11 Sep 2026 14:25:45 +0300 Subject: [PATCH 11/15] Fix schedule create dropping backup type Signed-off-by: Daniel Mungai --- pkg/cmd/cli/schedule/create.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/cmd/cli/schedule/create.go b/pkg/cmd/cli/schedule/create.go index 03f5626fd..cba121f5d 100644 --- a/pkg/cmd/cli/schedule/create.go +++ b/pkg/cmd/cli/schedule/create.go @@ -163,6 +163,7 @@ func (o *CreateOptions) Run(c *cobra.Command, f client.Factory) error { ItemOperationTimeout: metav1.Duration{Duration: o.BackupOptions.ItemOperationTimeout}, DataMover: o.BackupOptions.DataMover, SnapshotMoveData: o.BackupOptions.SnapshotMoveData.Value, + BackupType: api.BackupType(o.BackupOptions.BackupType), }, Schedule: o.Schedule, UseOwnerReferencesInBackup: &o.UseOwnerReferencesInBackup, From b1c1c145c2edad4f9d8d6e89b53d75a36316bcc5 Mon Sep 17 00:00:00 2001 From: Daniel Mungai Date: Sat, 12 Sep 2026 11:41:04 +0300 Subject: [PATCH 12/15] Add changelog for #10526 Signed-off-by: Daniel Mungai --- changelogs/unreleased/10526-Daniel-1600 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10526-Daniel-1600 diff --git a/changelogs/unreleased/10526-Daniel-1600 b/changelogs/unreleased/10526-Daniel-1600 new file mode 100644 index 000000000..0900b4d79 --- /dev/null +++ b/changelogs/unreleased/10526-Daniel-1600 @@ -0,0 +1 @@ +Fix schedule create dropping backup type \ No newline at end of file From 3db6638511fd2f1f367c2fef93447b27b9dc1e41 Mon Sep 17 00:00:00 2001 From: mrchatam Date: Sun, 13 Sep 2026 04:33:06 +0000 Subject: [PATCH 13/15] docs: fix 'doesn't allow to set' grammar in CSI snapshot data movement Signed-off-by: mrchatam --- site/content/docs/main/csi-snapshot-data-movement.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/site/content/docs/main/csi-snapshot-data-movement.md b/site/content/docs/main/csi-snapshot-data-movement.md index 9c9bc6184..1e789434d 100644 --- a/site/content/docs/main/csi-snapshot-data-movement.md +++ b/site/content/docs/main/csi-snapshot-data-movement.md @@ -373,7 +373,7 @@ spec: ...... ``` -At present, Velero doesn't allow to set `ReadOnlyRootFileSystem` parameter to data mover pods, so the root filesystem for the data mover pods are always writable. +At present, Velero doesn't allow setting the `ReadOnlyRootFileSystem` parameter on data mover pods, so the root filesystem for the data mover pods is always writable. ### Resource Consumption From 2c411fac9837c66e77f058ebde3bd32ad4c050a4 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 14 Sep 2026 13:50:37 -0400 Subject: [PATCH 14/15] Bump github/codeql-action in the github-actions group (#10527) Bumps the github-actions group with 1 update: [github/codeql-action](https://github.com/github/codeql-action). Updates `github/codeql-action` from 4.37.9 to 4.38.0 - [Release notes](https://github.com/github/codeql-action/releases) - [Changelog](https://github.com/github/codeql-action/blob/main/CHANGELOG.md) - [Commits](https://github.com/github/codeql-action/compare/v4.37.9...v4.38.0) --- updated-dependencies: - dependency-name: github/codeql-action dependency-version: 4.38.0 dependency-type: direct:production update-type: version-update:semver-minor dependency-group: github-actions ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/nightly-trivy-scan.yml | 2 +- .github/workflows/scorecard.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/nightly-trivy-scan.yml b/.github/workflows/nightly-trivy-scan.yml index 0999b1c0f..34f3eaf0c 100644 --- a/.github/workflows/nightly-trivy-scan.yml +++ b/.github/workflows/nightly-trivy-scan.yml @@ -35,6 +35,6 @@ jobs: output: 'trivy-results.sarif' - name: Upload Trivy scan results to GitHub Security tab - uses: github/codeql-action/upload-sarif@v4.37.9 + uses: github/codeql-action/upload-sarif@v4.38.0 with: sarif_file: 'trivy-results.sarif' \ No newline at end of file diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 9ba1b8b46..a69532faf 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -51,6 +51,6 @@ jobs: # Upload the results to GitHub's code scanning dashboard. - name: "Upload to code-scanning" - uses: github/codeql-action/upload-sarif@v4.37.9 + uses: github/codeql-action/upload-sarif@v4.38.0 with: sarif_file: results.sarif From 872f903091fe60e3b1f270bea3bd62286912c759 Mon Sep 17 00:00:00 2001 From: Tiger Kaovilai Date: Mon, 14 Sep 2026 18:05:01 -0400 Subject: [PATCH 15/15] Add configurable tolerations for PodVolumeBackup and data mover pods (#9575) * Remove toleration whitelist for PodVolumeBackup and data mover pods Instead of filtering tolerations through a hardcoded allowlist (ThirdPartyTolerations), inherit all tolerations from the node-agent daemonset for PodVolumeBackup/Restore and DataUpload/Download pods, and from the Velero deployment for maintenance jobs. This enables backups and restores on nodes with custom NoExecute taints, which was previously impossible since only two specific toleration keys were whitelisted. Fixes #9476 Signed-off-by: Tiger Kaovilai Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy * Fix codespell: replace 'whitelist' with 'allowlist' in changelog Signed-off-by: Tiger Kaovilai Generated with [Claude Code](https://claude.ai/code) via [Happy](https://happy.engineering) Co-Authored-By: Claude Co-Authored-By: Happy * Implement deduplication of tolerations and add unit tests for the new function Signed-off-by: Tiger Kaovilai * Merge node-agent-configmap tolerations with third-party allowlist Add a `tolerations` field to the node-agent-configmap so operators can declare hosting-pod tolerations explicitly, per blackpiglet's review feedback that tolerations shouldn't be read from the DaemonSet alone. These are merged with (and deduplicated against) DaemonSet tolerations matching the existing third-party allowlist (kubernetes.azure.com/scalesetpriority, CriticalAddonsOnly), restoring that allowlist per the follow-up suggestion to keep inheriting it alongside the new config option. The toleration dedup helper is moved from pkg/exposer to pkg/util/kube (exported as DeduplicateTolerations) so it can be shared with pkg/nodeagent without an import cycle. Signed-off-by: Tiger Kaovilai * Fix testifylint finding in TestGetTolerations golangci-lint v2.12.0 (pinned in pr-linter-check.yml) flagged the shared assert.Equal after the if/else as require-error: use require for the error assertion so each branch is self-contained, matching the pattern used elsewhere in this file. Signed-off-by: Tiger Kaovilai * Document toleration merge priority in GetTolerations Per blackpiglet's review feedback: clarify that configured tolerations take priority over allowlisted daemonset tolerations because they're appended first and DeduplicateTolerations keeps only the first occurrence of each exact (Key, Operator, Value, Effect) combination. Co-Authored-By: Claude Sonnet 5 Signed-off-by: Tiger Kaovilai --------- Signed-off-by: Tiger Kaovilai Co-authored-by: Claude Co-authored-by: Happy --- changelogs/unreleased/9575-kaovilai | 1 + pkg/cmd/cli/nodeagent/server.go | 10 ++ pkg/controller/data_download_controller.go | 15 +- .../data_download_controller_test.go | 4 +- pkg/controller/data_upload_controller.go | 15 +- pkg/controller/data_upload_controller_test.go | 6 +- .../pod_volume_backup_controller.go | 15 +- .../pod_volume_backup_controller_test.go | 2 + .../pod_volume_restore_controller.go | 15 +- .../pod_volume_restore_controller_test.go | 2 + pkg/exposer/csi_snapshot.go | 4 +- pkg/exposer/generic_restore.go | 4 +- pkg/exposer/pod_volume.go | 4 +- pkg/nodeagent/node_agent.go | 33 ++++- pkg/nodeagent/node_agent_test.go | 138 +++++++++++------- pkg/repository/maintenance/maintenance.go | 17 +-- .../maintenance/maintenance_test.go | 136 ++++------------- pkg/types/node_agent.go | 6 + pkg/util/kube/toleration.go | 37 +++++ pkg/util/kube/toleration_test.go | 86 +++++++++++ .../node-agent-configmap.md | 42 +++++- 21 files changed, 368 insertions(+), 224 deletions(-) create mode 100644 changelogs/unreleased/9575-kaovilai create mode 100644 pkg/util/kube/toleration.go create mode 100644 pkg/util/kube/toleration_test.go diff --git a/changelogs/unreleased/9575-kaovilai b/changelogs/unreleased/9575-kaovilai new file mode 100644 index 000000000..7c7c1817c --- /dev/null +++ b/changelogs/unreleased/9575-kaovilai @@ -0,0 +1 @@ +Add configurable tolerations to the node-agent-configmap for PodVolumeBackup and data mover pods; DaemonSet tolerations matching the existing third-party allowlist (kubernetes.azure.com/scalesetpriority, CriticalAddonsOnly) continue to be inherited automatically and are merged with the configmap value diff --git a/pkg/cmd/cli/nodeagent/server.go b/pkg/cmd/cli/nodeagent/server.go index c1442aab0..973e993dd 100644 --- a/pkg/cmd/cli/nodeagent/server.go +++ b/pkg/cmd/cli/nodeagent/server.go @@ -384,6 +384,12 @@ func (s *nodeAgentServer) run() { s.logger.Infof("Using customized pod annotations %+v", podAnnotations) } + var tolerations []corev1api.Toleration + if s.dataPathConfigs != nil && len(s.dataPathConfigs.Tolerations) > 0 { + tolerations = s.dataPathConfigs.Tolerations + s.logger.Infof("Using customized tolerations %+v", tolerations) + } + if s.backupRepoConfigs != nil { s.logger.Infof("Using backup repo config %v", s.backupRepoConfigs) } else if cachePVCConfig != nil { @@ -412,6 +418,7 @@ func (s *nodeAgentServer) run() { privilegedFsBackup, podLabels, podAnnotations, + tolerations, ) if err := pvbReconciler.SetupWithManager(s.mgr); err != nil { s.logger.Fatal(err, "unable to create controller", "controller", constant.ControllerPodVolumeBackup) @@ -435,6 +442,7 @@ func (s *nodeAgentServer) run() { s.repoConfigMgr, podLabels, podAnnotations, + tolerations, ) if err := pvrReconciler.SetupWithManager(s.mgr); err != nil { s.logger.WithError(err).Fatal("Unable to create the pod volume restore controller") @@ -459,6 +467,7 @@ func (s *nodeAgentServer) run() { podLabels, podAnnotations, csiSnapshotMetadataServiceConfigs, + tolerations, ) if err := dataUploadReconciler.SetupWithManager(s.mgr); err != nil { s.logger.WithError(err).Fatal("Unable to create the data upload controller") @@ -490,6 +499,7 @@ func (s *nodeAgentServer) run() { podLabels, podAnnotations, csiSnapshotMetadataServiceConfigs, + tolerations, ) if err := dataDownloadReconciler.SetupWithManager(s.mgr); err != nil { diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index 3507edfe5..513a3eefc 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -81,6 +81,7 @@ type DataDownloadReconciler struct { podLabels map[string]string podAnnotations map[string]string snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService + tolerations []corev1api.Toleration } func NewDataDownloadReconciler( @@ -103,6 +104,7 @@ func NewDataDownloadReconciler( podLabels map[string]string, podAnnotations map[string]string, snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService, + tolerations []corev1api.Toleration, ) *DataDownloadReconciler { return &DataDownloadReconciler{ client: client, @@ -126,6 +128,7 @@ func NewDataDownloadReconciler( podLabels: podLabels, podAnnotations: podAnnotations, snapshotMetadataServiceConfigs: snapshotMetadataServiceConfigs, + tolerations: tolerations, } } @@ -940,15 +943,9 @@ func (r *DataDownloadReconciler) setupExposeParam(dd *velerov2alpha1api.DataDown } } - hostingPodTolerations := []corev1api.Toleration{} - for _, k := range util.ThirdPartyTolerations { - if v, err := nodeagent.GetToleration(context.Background(), r.kubeClient, dd.Namespace, k, nodeOS); err != nil { - if err != nodeagent.ErrNodeAgentTolerationNotFound { - log.WithError(err).Warnf("Failed to check node-agent toleration, skip adding host pod toleration %s", k) - } - } else { - hostingPodTolerations = append(hostingPodTolerations, *v) - } + hostingPodTolerations, err := nodeagent.GetTolerations(context.Background(), r.kubeClient, dd.Namespace, nodeOS, r.tolerations) + if err != nil { + log.WithError(err).Warn("Failed to get node-agent daemonset tolerations, hosting pod will only get configured tolerations") } var cacheVolume *exposer.CacheConfigs diff --git a/pkg/controller/data_download_controller_test.go b/pkg/controller/data_download_controller_test.go index c9026eafa..7fcbb8978 100644 --- a/pkg/controller/data_download_controller_test.go +++ b/pkg/controller/data_download_controller_test.go @@ -151,6 +151,7 @@ func initDataDownloadReconcilerWithError(t *testing.T, objects []any, needError nil, // podLabels nil, // podAnnotations nil, // snapshotMetadataServiceConfigs + nil, // tolerations ), nil } @@ -1464,7 +1465,8 @@ func TestDataDownloadSetupExposeParam(t *testing.T) { nil, // repoConfigMgr (unused when cacheVolumeConfigs is nil) tt.args.customLabels, tt.args.customAnnotations, - nil, + nil, // snapshotMetadataServiceConfigs + nil, // tolerations ) // Act diff --git a/pkg/controller/data_upload_controller.go b/pkg/controller/data_upload_controller.go index 84504153f..5752eb0a4 100644 --- a/pkg/controller/data_upload_controller.go +++ b/pkg/controller/data_upload_controller.go @@ -86,6 +86,7 @@ type DataUploadReconciler struct { podLabels map[string]string podAnnotations map[string]string snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService + tolerations []corev1api.Toleration } func NewDataUploadReconciler( @@ -107,6 +108,7 @@ func NewDataUploadReconciler( podLabels map[string]string, podAnnotations map[string]string, snapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService, + tolerations []corev1api.Toleration, ) *DataUploadReconciler { return &DataUploadReconciler{ client: client, @@ -134,6 +136,7 @@ func NewDataUploadReconciler( podLabels: podLabels, podAnnotations: podAnnotations, snapshotMetadataServiceConfigs: snapshotMetadataServiceConfigs, + tolerations: tolerations, } } @@ -1010,15 +1013,9 @@ func (r *DataUploadReconciler) setupExposeParam(du *velerov2alpha1api.DataUpload } } - hostingPodTolerations := []corev1api.Toleration{} - for _, k := range util.ThirdPartyTolerations { - if v, err := nodeagent.GetToleration(context.Background(), r.kubeClient, du.Namespace, k, nodeOS); err != nil { - if err != nodeagent.ErrNodeAgentTolerationNotFound { - log.WithError(err).Warnf("Failed to check node-agent toleration, skip adding host pod toleration %s", k) - } - } else { - hostingPodTolerations = append(hostingPodTolerations, *v) - } + hostingPodTolerations, err := nodeagent.GetTolerations(context.Background(), r.kubeClient, du.Namespace, nodeOS, r.tolerations) + if err != nil { + log.WithError(err).Warn("Failed to get node-agent daemonset tolerations, hosting pod will only get configured tolerations") } return &exposer.CSISnapshotExposeParam{ diff --git a/pkg/controller/data_upload_controller_test.go b/pkg/controller/data_upload_controller_test.go index d50ceb749..5b4fc7780 100644 --- a/pkg/controller/data_upload_controller_test.go +++ b/pkg/controller/data_upload_controller_test.go @@ -270,7 +270,8 @@ func initDataUploaderReconcilerWithError(needError ...error) (*DataUploadReconci "", // dataMovePriorityClass nil, // podLabels nil, // podAnnotations - nil, + nil, // snapshotMetadataServiceConfigs + nil, // tolerations ), nil } @@ -1565,7 +1566,8 @@ func TestDataUploadSetupExposeParam(t *testing.T) { "upload-priority", tt.args.customLabels, tt.args.customAnnotations, - nil, + nil, // snapshotMetadataServiceConfigs + nil, // tolerations ) // Act diff --git a/pkg/controller/pod_volume_backup_controller.go b/pkg/controller/pod_volume_backup_controller.go index 399f4a3b8..2bf0b1fd2 100644 --- a/pkg/controller/pod_volume_backup_controller.go +++ b/pkg/controller/pod_volume_backup_controller.go @@ -74,6 +74,7 @@ func NewPodVolumeBackupReconciler( privileged bool, podLabels map[string]string, podAnnotations map[string]string, + tolerations []corev1api.Toleration, ) *PodVolumeBackupReconciler { return &PodVolumeBackupReconciler{ client: client, @@ -93,6 +94,7 @@ func NewPodVolumeBackupReconciler( privileged: privileged, podLabels: podLabels, podAnnotations: podAnnotations, + tolerations: tolerations, } } @@ -116,6 +118,7 @@ type PodVolumeBackupReconciler struct { privileged bool podLabels map[string]string podAnnotations map[string]string + tolerations []corev1api.Toleration } // +kubebuilder:rbac:groups=velero.io,resources=podvolumebackups,verbs=get;list;watch;create;update;patch;delete @@ -867,15 +870,9 @@ func (r *PodVolumeBackupReconciler) setupExposeParam(pvb *velerov1api.PodVolumeB } } - hostingPodTolerations := []corev1api.Toleration{} - for _, k := range util.ThirdPartyTolerations { - if v, err := nodeagent.GetToleration(context.Background(), r.kubeClient, pvb.Namespace, k, nodeOS); err != nil { - if err != nodeagent.ErrNodeAgentTolerationNotFound { - log.WithError(err).Warnf("Failed to check node-agent toleration, skip adding host pod toleration %s", k) - } - } else { - hostingPodTolerations = append(hostingPodTolerations, *v) - } + hostingPodTolerations, err := nodeagent.GetTolerations(context.Background(), r.kubeClient, pvb.Namespace, nodeOS, r.tolerations) + if err != nil { + log.WithError(err).Warn("Failed to get node-agent daemonset tolerations, hosting pod will only get configured tolerations") } return exposer.PodVolumeExposeParam{ diff --git a/pkg/controller/pod_volume_backup_controller_test.go b/pkg/controller/pod_volume_backup_controller_test.go index e74a1c269..9c8240951 100644 --- a/pkg/controller/pod_volume_backup_controller_test.go +++ b/pkg/controller/pod_volume_backup_controller_test.go @@ -157,6 +157,7 @@ func initPVBReconcilerWithError(needError ...error) (*PodVolumeBackupReconciler, false, // privileged nil, // podLabels nil, // podAnnotations + nil, // tolerations ), nil } @@ -1317,6 +1318,7 @@ func TestPodVolumeBackupSetupExposeParam(t *testing.T) { true, tt.args.customLabels, tt.args.customAnnotations, + nil, ) // Act diff --git a/pkg/controller/pod_volume_restore_controller.go b/pkg/controller/pod_volume_restore_controller.go index 81540aa15..7da739d2f 100644 --- a/pkg/controller/pod_volume_restore_controller.go +++ b/pkg/controller/pod_volume_restore_controller.go @@ -75,6 +75,7 @@ func NewPodVolumeRestoreReconciler( repoConfigMgr repository.ConfigManager, podLabels map[string]string, podAnnotations map[string]string, + tolerations []corev1api.Toleration, ) *PodVolumeRestoreReconciler { return &PodVolumeRestoreReconciler{ client: client, @@ -96,6 +97,7 @@ func NewPodVolumeRestoreReconciler( repoConfigMgr: repoConfigMgr, podLabels: podLabels, podAnnotations: podAnnotations, + tolerations: tolerations, } } @@ -120,6 +122,7 @@ type PodVolumeRestoreReconciler struct { repoConfigMgr repository.ConfigManager podLabels map[string]string podAnnotations map[string]string + tolerations []corev1api.Toleration } // +kubebuilder:rbac:groups=velero.io,resources=podvolumerestores,verbs=get;list;watch;create;update;patch;delete @@ -968,15 +971,9 @@ func (r *PodVolumeRestoreReconciler) setupExposeParam(pvr *velerov1api.PodVolume } } - hostingPodTolerations := []corev1api.Toleration{} - for _, k := range util.ThirdPartyTolerations { - if v, err := nodeagent.GetToleration(context.Background(), r.kubeClient, pvr.Namespace, k, nodeOS); err != nil { - if err != nodeagent.ErrNodeAgentTolerationNotFound { - log.WithError(err).Warnf("Failed to check node-agent toleration, skip adding host pod toleration %s", k) - } - } else { - hostingPodTolerations = append(hostingPodTolerations, *v) - } + hostingPodTolerations, err := nodeagent.GetTolerations(context.Background(), r.kubeClient, pvr.Namespace, nodeOS, r.tolerations) + if err != nil { + log.WithError(err).Warn("Failed to get node-agent daemonset tolerations, hosting pod will only get configured tolerations") } var cacheVolume *exposer.CacheConfigs diff --git a/pkg/controller/pod_volume_restore_controller_test.go b/pkg/controller/pod_volume_restore_controller_test.go index 6bd68e7e1..03b2841d1 100644 --- a/pkg/controller/pod_volume_restore_controller_test.go +++ b/pkg/controller/pod_volume_restore_controller_test.go @@ -751,6 +751,7 @@ func initPodVolumeRestoreReconcilerWithError(objects []runtime.Object, cliObj [] nil, nil, // podLabels nil, // podAnnotations + nil, // tolerations ), nil } @@ -1335,6 +1336,7 @@ func TestPodVolumeRestoreSetupExposeParam(t *testing.T) { nil, // repoConfigMgr (unused when cacheVolumeConfigs is nil) tt.args.customLabels, tt.args.customAnnotations, + nil, ) // Act diff --git a/pkg/exposer/csi_snapshot.go b/pkg/exposer/csi_snapshot.go index ea67d192d..5e51d0e99 100644 --- a/pkg/exposer/csi_snapshot.go +++ b/pkg/exposer/csi_snapshot.go @@ -785,7 +785,7 @@ func (e *csiSnapshotExposer) createBackupPod( Operator: metav1.LabelSelectorOpIn, }) - toleration = append(toleration, []corev1api.Toleration{ + toleration = kube.DeduplicateTolerations(append(toleration, []corev1api.Toleration{ { Key: "os", Operator: "Equal", @@ -798,7 +798,7 @@ func (e *csiSnapshotExposer) createBackupPod( Effect: "NoExecute", Value: "windows", }, - }...) + }...)) } else { userID := int64(0) securityCtx = &corev1api.PodSecurityContext{ diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index f144a03ae..bc22b1849 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -842,7 +842,7 @@ func (e *genericRestoreExposer) createRestorePod( Operator: metav1.LabelSelectorOpIn, }) - toleration = append(toleration, []corev1api.Toleration{ + toleration = kube.DeduplicateTolerations(append(toleration, []corev1api.Toleration{ { Key: "os", Operator: "Equal", @@ -855,7 +855,7 @@ func (e *genericRestoreExposer) createRestorePod( Effect: "NoExecute", Value: "windows", }, - }...) + }...)) } else { userID := int64(0) securityCtx = &corev1api.PodSecurityContext{ diff --git a/pkg/exposer/pod_volume.go b/pkg/exposer/pod_volume.go index 5d6de1831..4402396cb 100644 --- a/pkg/exposer/pod_volume.go +++ b/pkg/exposer/pod_volume.go @@ -456,7 +456,7 @@ func (e *podVolumeExposer) createHostingPod( Operator: metav1.LabelSelectorOpIn, }) - toleration = append(toleration, []corev1api.Toleration{ + toleration = kube.DeduplicateTolerations(append(toleration, []corev1api.Toleration{ { Key: "os", Operator: "Equal", @@ -469,7 +469,7 @@ func (e *podVolumeExposer) createHostingPod( Effect: "NoExecute", Value: "windows", }, - }...) + }...)) } else { userID := int64(0) securityCtx = &corev1api.PodSecurityContext{ diff --git a/pkg/nodeagent/node_agent.go b/pkg/nodeagent/node_agent.go index 6dcdbf89b..4d5040db7 100644 --- a/pkg/nodeagent/node_agent.go +++ b/pkg/nodeagent/node_agent.go @@ -31,6 +31,7 @@ import ( ctrlclient "sigs.k8s.io/controller-runtime/pkg/client" velerotypes "github.com/vmware-tanzu/velero/pkg/types" + "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/kube" ) @@ -55,7 +56,6 @@ var ( ErrDaemonSetNotFound = errors.New("daemonset not found") ErrNodeAgentLabelNotFound = errors.New("node-agent label not found") ErrNodeAgentAnnotationNotFound = errors.New("node-agent annotation not found") - ErrNodeAgentTolerationNotFound = errors.New("node-agent toleration not found") ) func IsRunningOnLinux(ctx context.Context, kubeClient kubernetes.Interface, namespace string) error { @@ -249,7 +249,14 @@ func GetAnnotationValue(ctx context.Context, kubeClient kubernetes.Interface, na return val, nil } -func GetToleration(ctx context.Context, kubeClient kubernetes.Interface, namespace string, key string, osType string) (*corev1api.Toleration, error) { +// GetTolerations returns the tolerations that should be applied to a node-agent-driven +// hosting pod: the explicitly configured tolerations (typically sourced from the +// node-agent-configmap), plus any toleration on the node-agent daemonset (linux or +// windows, based on osType) whose key is in util.ThirdPartyTolerations. The combined +// list is deduplicated by kube.DeduplicateTolerations. On a daemonset lookup error, +// configuredTolerations is still returned alongside the error so callers don't lose +// explicitly configured tolerations to a transient lookup failure. +func GetTolerations(ctx context.Context, kubeClient kubernetes.Interface, namespace string, osType string, configuredTolerations []corev1api.Toleration) ([]corev1api.Toleration, error) { dsName := daemonSet if osType == kube.NodeOSWindows { dsName = daemonsetWindows @@ -257,16 +264,28 @@ func GetToleration(ctx context.Context, kubeClient kubernetes.Interface, namespa ds, err := kubeClient.AppsV1().DaemonSets(namespace).Get(ctx, dsName, metav1.GetOptions{}) if err != nil { - return nil, errors.Wrapf(err, "error getting %s daemonset", dsName) + return configuredTolerations, errors.Wrapf(err, "error getting %s daemonset", dsName) } - for i, t := range ds.Spec.Template.Spec.Tolerations { - if t.Key == key { - return &ds.Spec.Template.Spec.Tolerations[i], nil + // configuredTolerations is appended first so it wins: DeduplicateTolerations + // keeps only the first occurrence of each exact (Key, Operator, Value, + // Effect) combination, so an allowlisted daemonset toleration identical to + // one already set in the configmap is dropped as a duplicate rather than + // overriding it. A daemonset toleration that only shares a Key (but + // differs in Operator/Value/Effect) isn't a duplicate and is kept + // alongside the configured one, not replaced by it. + merged := make([]corev1api.Toleration, 0, len(configuredTolerations)+len(ds.Spec.Template.Spec.Tolerations)) + merged = append(merged, configuredTolerations...) + for _, t := range ds.Spec.Template.Spec.Tolerations { + for _, allowed := range util.ThirdPartyTolerations { + if t.Key == allowed { + merged = append(merged, t) + break + } } } - return nil, ErrNodeAgentTolerationNotFound + return kube.DeduplicateTolerations(merged), nil } func GetHostPodPath(ctx context.Context, kubeClient kubernetes.Interface, namespace string, osType string) (string, error) { diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 4a406dd39..61efaac90 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/pkg/nodeagent/node_agent_test.go @@ -884,7 +884,7 @@ func TestGetAnnotationValue(t *testing.T) { } } -func TestGetToleration(t *testing.T) { +func TestGetTolerations(t *testing.T) { daemonSet := &appsv1api.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ Namespace: "fake-ns", @@ -895,7 +895,7 @@ func TestGetToleration(t *testing.T) { }, } - daemonSetWithOtherToleration := &appsv1api.DaemonSet{ + daemonSetWithTolerations := &appsv1api.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ Namespace: "fake-ns", Name: "node-agent", @@ -908,7 +908,14 @@ func TestGetToleration(t *testing.T) { Spec: corev1api.PodSpec{ Tolerations: []corev1api.Toleration{ { - Key: "other-toleration-key", + Key: "custom-taint", + Value: "true", + }, + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Value: "spot", + Effect: "NoSchedule", }, }, }, @@ -916,79 +923,110 @@ func TestGetToleration(t *testing.T) { }, } - daemonSetWithToleration := &appsv1api.DaemonSet{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "fake-ns", - Name: "node-agent", - }, - TypeMeta: metav1.TypeMeta{ - Kind: "DaemonSet", - }, - Spec: appsv1api.DaemonSetSpec{ - Template: corev1api.PodTemplateSpec{ - Spec: corev1api.PodSpec{ - Tolerations: []corev1api.Toleration{ - { - Key: "fake-toleration", - Value: "true", - }, - }, - }, - }, - }, + configuredToleration := corev1api.Toleration{ + Key: "dedicated", + Operator: "Equal", + Value: "backup", + Effect: "NoSchedule", } tests := []struct { - name string - kubeClientObj []runtime.Object - namespace string - expectedValue corev1api.Toleration - expectErr string + name string + kubeClientObj []runtime.Object + namespace string + configuredTolerations []corev1api.Toleration + expectedValues []corev1api.Toleration + expectErr string }{ - // { - // name: "ds get error", - // namespace: "fake-ns", - // expectErr: "error getting node-agent daemonset: daemonsets.apps \"node-agent\" not found", - // }, { - name: "no toleration", - namespace: "fake-ns", - kubeClientObj: []runtime.Object{ - daemonSet, - }, - expectErr: ErrNodeAgentTolerationNotFound.Error(), + name: "no tolerations", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{daemonSet}, + expectedValues: []corev1api.Toleration{}, }, { - name: "no expecting toleration", + name: "only non-allowlisted daemonset tolerations are dropped", namespace: "fake-ns", kubeClientObj: []runtime.Object{ - daemonSetWithOtherToleration, + daemonSetWithTolerations, + }, + expectedValues: []corev1api.Toleration{ + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Value: "spot", + Effect: "NoSchedule", + }, }, - expectErr: ErrNodeAgentTolerationNotFound.Error(), }, { - name: "expecting toleration", + name: "configured tolerations only", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{daemonSet}, + configuredTolerations: []corev1api.Toleration{configuredToleration}, + expectedValues: []corev1api.Toleration{configuredToleration}, + }, + { + name: "configured and allowlisted daemonset tolerations are merged", namespace: "fake-ns", kubeClientObj: []runtime.Object{ - daemonSetWithToleration, + daemonSetWithTolerations, }, - expectedValue: corev1api.Toleration{ - Key: "fake-toleration", - Value: "true", + configuredTolerations: []corev1api.Toleration{configuredToleration}, + expectedValues: []corev1api.Toleration{ + configuredToleration, + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Value: "spot", + Effect: "NoSchedule", + }, }, }, + { + name: "duplicate between configured and daemonset tolerations is deduplicated", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + daemonSetWithTolerations, + }, + configuredTolerations: []corev1api.Toleration{ + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Value: "spot", + Effect: "NoSchedule", + }, + }, + expectedValues: []corev1api.Toleration{ + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Value: "spot", + Effect: "NoSchedule", + }, + }, + }, + { + name: "daemonset get error still returns configured tolerations", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{}, + configuredTolerations: []corev1api.Toleration{configuredToleration}, + expectedValues: []corev1api.Toleration{configuredToleration}, + expectErr: "error getting node-agent daemonset: daemonsets.apps \"node-agent\" not found", + }, } for _, test := range tests { t.Run(test.name, func(t *testing.T) { fakeKubeClient := fake.NewSimpleClientset(test.kubeClientObj...) - value, err := GetToleration(t.Context(), fakeKubeClient, test.namespace, "fake-toleration", kube.NodeOSLinux) + values, err := GetTolerations(t.Context(), fakeKubeClient, test.namespace, kube.NodeOSLinux, test.configuredTolerations) if test.expectErr == "" { require.NoError(t, err) - assert.Equal(t, test.expectedValue, *value) + assert.Equal(t, test.expectedValues, values) } else { - assert.EqualError(t, err, test.expectErr) + require.EqualError(t, err, test.expectErr) + assert.Equal(t, test.expectedValues, values) } }) } diff --git a/pkg/repository/maintenance/maintenance.go b/pkg/repository/maintenance/maintenance.go index 86525d54f..9fa8ba054 100644 --- a/pkg/repository/maintenance/maintenance.go +++ b/pkg/repository/maintenance/maintenance.go @@ -482,9 +482,8 @@ func StartNewJob( } // buildTolerationsForMaintenanceJob builds the tolerations for maintenance jobs. -// It includes the required Windows toleration for backward compatibility and filters -// tolerations from the Velero deployment to only include those with keys that are -// in the ThirdPartyTolerations allowlist, following the same pattern as labels and annotations. +// It includes the required Windows toleration for backward compatibility and +// inherits all tolerations from the Velero deployment. func buildTolerationsForMaintenanceJob(deployment *appsv1api.Deployment) []corev1api.Toleration { // Start with the Windows toleration for backward compatibility windowsToleration := corev1api.Toleration{ @@ -495,17 +494,9 @@ func buildTolerationsForMaintenanceJob(deployment *appsv1api.Deployment) []corev } result := []corev1api.Toleration{windowsToleration} - // Filter tolerations from the Velero deployment to only include allowed ones - // Only tolerations that exist on the deployment AND have keys in the allowlist are inherited + // Inherit all tolerations from the Velero deployment deploymentTolerations := veleroutil.GetTolerationsFromVeleroServer(deployment) - for _, k := range util.ThirdPartyTolerations { - for _, toleration := range deploymentTolerations { - if toleration.Key == k { - result = append(result, toleration) - break // Only add the first matching toleration for each allowed key - } - } - } + result = append(result, deploymentTolerations...) return result } diff --git a/pkg/repository/maintenance/maintenance_test.go b/pkg/repository/maintenance/maintenance_test.go index ee34241ce..e69dadb33 100644 --- a/pkg/repository/maintenance/maintenance_test.go +++ b/pkg/repository/maintenance/maintenance_test.go @@ -1954,7 +1954,7 @@ func TestBuildTolerationsForMaintenanceJob(t *testing.T) { }, }, { - name: "non-allowed toleration should not be inherited", + name: "all tolerations should be inherited", deploymentTolerations: []corev1api.Toleration{ { Key: "vng-ondemand", @@ -1962,88 +1962,36 @@ func TestBuildTolerationsForMaintenanceJob(t *testing.T) { Effect: "NoSchedule", Value: "amd64", }, - }, - expectedTolerations: []corev1api.Toleration{ - windowsToleration, - }, - }, - { - name: "allowed toleration should be inherited", - deploymentTolerations: []corev1api.Toleration{ { - Key: "kubernetes.azure.com/scalesetpriority", - Operator: "Equal", - Effect: "NoSchedule", - Value: "spot", - }, - }, - expectedTolerations: []corev1api.Toleration{ - windowsToleration, - { - Key: "kubernetes.azure.com/scalesetpriority", - Operator: "Equal", - Effect: "NoSchedule", - Value: "spot", - }, - }, - }, - { - name: "mixed allowed and non-allowed tolerations should only inherit allowed", - deploymentTolerations: []corev1api.Toleration{ - { - Key: "vng-ondemand", // not in allowlist - Operator: "Equal", - Effect: "NoSchedule", - Value: "amd64", - }, - { - Key: "CriticalAddonsOnly", // in allowlist + Key: "CriticalAddonsOnly", Operator: "Exists", Effect: "NoSchedule", }, { - Key: "custom-key", // not in allowlist + Key: "custom-key", Operator: "Equal", - Effect: "NoSchedule", + Effect: "NoExecute", Value: "custom-value", }, }, expectedTolerations: []corev1api.Toleration{ windowsToleration, { - Key: "CriticalAddonsOnly", - Operator: "Exists", - Effect: "NoSchedule", - }, - }, - }, - { - name: "multiple allowed tolerations should all be inherited", - deploymentTolerations: []corev1api.Toleration{ - { - Key: "kubernetes.azure.com/scalesetpriority", + Key: "vng-ondemand", Operator: "Equal", Effect: "NoSchedule", - Value: "spot", + Value: "amd64", }, { Key: "CriticalAddonsOnly", Operator: "Exists", Effect: "NoSchedule", }, - }, - expectedTolerations: []corev1api.Toleration{ - windowsToleration, { - Key: "kubernetes.azure.com/scalesetpriority", + Key: "custom-key", Operator: "Equal", - Effect: "NoSchedule", - Value: "spot", - }, - { - Key: "CriticalAddonsOnly", - Operator: "Exists", - Effect: "NoSchedule", + Effect: "NoExecute", + Value: "custom-value", }, }, }, @@ -2069,36 +2017,6 @@ func TestBuildTolerationsForMaintenanceJob(t *testing.T) { } func TestBuildJobWithTolerationsInheritance(t *testing.T) { - // Define allowed tolerations that would be set on Velero deployment - allowedTolerations := []corev1api.Toleration{ - { - Key: "kubernetes.azure.com/scalesetpriority", - Operator: "Equal", - Effect: "NoSchedule", - Value: "spot", - }, - { - Key: "CriticalAddonsOnly", - Operator: "Exists", - Effect: "NoSchedule", - }, - } - - // Mixed tolerations (allowed and non-allowed) - mixedTolerations := []corev1api.Toleration{ - { - Key: "vng-ondemand", // not in allowlist - Operator: "Equal", - Effect: "NoSchedule", - Value: "amd64", - }, - { - Key: "CriticalAddonsOnly", // in allowlist - Operator: "Exists", - Effect: "NoSchedule", - }, - } - // Windows toleration that should always be present windowsToleration := corev1api.Toleration{ Key: "os", @@ -2120,8 +2038,21 @@ func TestBuildJobWithTolerationsInheritance(t *testing.T) { }, }, { - name: "allowed tolerations should be inherited along with Windows toleration", - deploymentTolerations: allowedTolerations, + name: "all tolerations should be inherited along with Windows toleration", + deploymentTolerations: []corev1api.Toleration{ + { + Key: "kubernetes.azure.com/scalesetpriority", + Operator: "Equal", + Effect: "NoSchedule", + Value: "spot", + }, + { + Key: "custom-taint", + Operator: "Equal", + Effect: "NoExecute", + Value: "dedicated", + }, + }, expectedTolerations: []corev1api.Toleration{ windowsToleration, { @@ -2131,21 +2062,10 @@ func TestBuildJobWithTolerationsInheritance(t *testing.T) { Value: "spot", }, { - Key: "CriticalAddonsOnly", - Operator: "Exists", - Effect: "NoSchedule", - }, - }, - }, - { - name: "mixed tolerations should only inherit allowed ones", - deploymentTolerations: mixedTolerations, - expectedTolerations: []corev1api.Toleration{ - windowsToleration, - { - Key: "CriticalAddonsOnly", - Operator: "Exists", - Effect: "NoSchedule", + Key: "custom-taint", + Operator: "Equal", + Effect: "NoExecute", + Value: "dedicated", }, }, }, diff --git a/pkg/types/node_agent.go b/pkg/types/node_agent.go index 16e50b283..a372d17e9 100644 --- a/pkg/types/node_agent.go +++ b/pkg/types/node_agent.go @@ -17,6 +17,7 @@ limitations under the License. package types import ( + corev1api "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -142,4 +143,9 @@ type NodeAgentConfigs struct { // CSISnapshotMetadataServiceConfigs is the config for CSI snapshot metadata service CSISnapshotMetadataServiceConfigs *CSISnapshotMetadataService `json:"csiSnapshotMetadataServiceConfigs,omitempty"` + + // Tolerations are tolerations to be added to pods created by node-agent, i.e., data mover pods. + // These are merged with (and deduplicated against) any node-agent DaemonSet tolerations + // whose key is in util.ThirdPartyTolerations. + Tolerations []corev1api.Toleration `json:"tolerations,omitempty"` } diff --git a/pkg/util/kube/toleration.go b/pkg/util/kube/toleration.go new file mode 100644 index 000000000..97b43e8d8 --- /dev/null +++ b/pkg/util/kube/toleration.go @@ -0,0 +1,37 @@ +/* +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 kube + +import ( + corev1api "k8s.io/api/core/v1" +) + +// DeduplicateTolerations removes duplicate tolerations from the slice. +// A toleration is considered a duplicate if another toleration with the same +// Key, Operator, Value, and Effect already exists in the slice. +func DeduplicateTolerations(tolerations []corev1api.Toleration) []corev1api.Toleration { + seen := make(map[string]struct{}) + result := make([]corev1api.Toleration, 0, len(tolerations)) + for _, t := range tolerations { + key := t.Key + "|" + string(t.Operator) + "|" + t.Value + "|" + string(t.Effect) + if _, exists := seen[key]; !exists { + seen[key] = struct{}{} + result = append(result, t) + } + } + return result +} diff --git a/pkg/util/kube/toleration_test.go b/pkg/util/kube/toleration_test.go new file mode 100644 index 000000000..1370af00c --- /dev/null +++ b/pkg/util/kube/toleration_test.go @@ -0,0 +1,86 @@ +/* +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 kube + +import ( + "testing" + + "github.com/stretchr/testify/assert" + corev1api "k8s.io/api/core/v1" +) + +func TestDeduplicateTolerations(t *testing.T) { + tests := []struct { + name string + input []corev1api.Toleration + expected []corev1api.Toleration + }{ + { + name: "nil input", + input: nil, + expected: []corev1api.Toleration{}, + }, + { + name: "empty input", + input: []corev1api.Toleration{}, + expected: []corev1api.Toleration{}, + }, + { + name: "no duplicates", + input: []corev1api.Toleration{ + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoExecute"}, + }, + expected: []corev1api.Toleration{ + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoExecute"}, + }, + }, + { + name: "duplicates removed", + input: []corev1api.Toleration{ + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoExecute"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoExecute"}, + }, + expected: []corev1api.Toleration{ + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoExecute"}, + }, + }, + { + name: "preserves order of first occurrence", + input: []corev1api.Toleration{ + {Key: "custom-taint", Operator: "Equal", Value: "true", Effect: "NoExecute"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + }, + expected: []corev1api.Toleration{ + {Key: "custom-taint", Operator: "Equal", Value: "true", Effect: "NoExecute"}, + {Key: "os", Operator: "Equal", Value: "windows", Effect: "NoSchedule"}, + }, + }, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + result := DeduplicateTolerations(test.input) + assert.Equal(t, test.expected, result) + }) + } +} diff --git a/site/content/docs/main/supported-configmaps/node-agent-configmap.md b/site/content/docs/main/supported-configmaps/node-agent-configmap.md index f8f8f2c5c..bea0e0a71 100644 --- a/site/content/docs/main/supported-configmaps/node-agent-configmap.md +++ b/site/content/docs/main/supported-configmaps/node-agent-configmap.md @@ -498,6 +498,37 @@ The configurations work for DataUpload, DataDownload, PodVolumeBackup, and PodVo - **Explicit Configuration Required**: If you need both custom annotations and in-tree third-party annotations, explicitly include the in-tree annotations in the `podAnnotations` configuration - **In-tree Annotations**: The default in-tree annotations include support for AWS IAM roles +### Tolerations Configuration (`tolerations`) + +Add customized tolerations for data mover pods to allow scheduling on nodes with custom taints. + +Unlike `podLabels`/`podAnnotations`, `tolerations` does **not** replace Velero's [in-tree third-party toleration allowlist](https://github.com/vmware-tanzu/velero/blob/main/pkg/util/third_party.go). Any toleration on the node-agent DaemonSet whose key is in that allowlist (currently `kubernetes.azure.com/scalesetpriority` and `CriticalAddonsOnly`) is always merged in alongside the tolerations configured here, with duplicates removed. + +The configurations work for DataUpload, DataDownload, PodVolumeBackup, and PodVolumeRestore pods. This does not affect repository maintenance jobs, which inherit tolerations from the Velero Deployment directly. + +#### Configuration Example +```json +{ + "tolerations": [ + { + "key": "dedicated", + "operator": "Equal", + "value": "backup", + "effect": "NoSchedule" + } + ] +} +``` + +#### Use Cases +- **Dedicated Backup Node Pools**: Nodes tainted to only run backup/restore workloads +- **Spot/Preemptible Node Pools**: Additional spot-instance taints beyond the built-in Azure allowlist +- **Custom Maintenance Taints**: Nodes with `NoExecute` taints applied during maintenance windows + +#### Important Notes +- **Merge, Not Replace**: `tolerations` is merged with (not a replacement for) the in-tree third-party allowlisted tolerations inherited from the node-agent DaemonSet +- **Deduplication**: Identical tolerations (same key, operator, value, and effect) from either source are only applied once + ## Complete Configuration Example Here's a comprehensive example showing how all configuration sections work together: @@ -579,7 +610,15 @@ Here's a comprehensive example showing how all configuration sections work toget "vault.hashicorp.com/agent-inject": "true", "prometheus.io/scrape": "true", "custom.company.com/environment": "production" - } + }, + "tolerations": [ + { + "key": "dedicated", + "operator": "Equal", + "value": "backup", + "effect": "NoSchedule" + } + ] } ``` @@ -596,6 +635,7 @@ This configuration: - Enable cache PVC for file system restore - The cache threshold is 1GB and use dedicated StorageClass - Use customized labels and annotations data mover pods +- Tolerate the `dedicated=backup:NoSchedule` taint, merged with any allowlisted DaemonSet tolerations ## Troubleshooting