From 12c1ef4ff2781482e996d88521090db3e00c5af7 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 5 Aug 2026 14:03:42 +0530 Subject: [PATCH 01/28] validate backup name format before contacting the API server Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 5e18f468f..f42a4dc70 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -26,6 +26,7 @@ import ( "github.com/spf13/pflag" kubeerrs "k8s.io/apimachinery/pkg/util/errors" "k8s.io/client-go/tools/cache" + "k8s.io/apimachinery/pkg/util/validation" kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -186,10 +187,12 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return err } - // Ensure that unless FromSchedule is set, args contains a backup name - if o.FromSchedule == "" && len(args) != 1 { - return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") - } + // Ensure the backup name is a valid Kubernetes resource name + if o.FromSchedule == "" { + if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) + } +} errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { From be6b4d38f2e828045801096ad74224744cb26e52 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 5 Aug 2026 20:24:35 +0530 Subject: [PATCH 02/28] gofmt pkg/cmd/cli/backup/create.go Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index f42a4dc70..fd46c50fa 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -25,8 +25,8 @@ import ( "github.com/spf13/cobra" "github.com/spf13/pflag" kubeerrs "k8s.io/apimachinery/pkg/util/errors" - "k8s.io/client-go/tools/cache" "k8s.io/apimachinery/pkg/util/validation" + "k8s.io/client-go/tools/cache" kbclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" @@ -189,10 +189,10 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto // Ensure the backup name is a valid Kubernetes resource name if o.FromSchedule == "" { - if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { - return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) - } -} + if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) + } + } errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { From e97d01405b49c936971bbdc764c0798a1cac1bdc Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 6 Aug 2026 10:52:03 +0530 Subject: [PATCH 03/28] add changelog entry Signed-off-by: samay43 --- changelogs/unreleased/10167-samay43 | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10167-samay43 diff --git a/changelogs/unreleased/10167-samay43 b/changelogs/unreleased/10167-samay43 new file mode 100644 index 000000000..0955fcc99 --- /dev/null +++ b/changelogs/unreleased/10167-samay43 @@ -0,0 +1 @@ +Validate backup name format before contacting the API server From 164d9343aad08b9a4091ab042487af2d608c1d50 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 12 Aug 2026 09:51:10 +0530 Subject: [PATCH 04/28] move backup name validation to Args to run before cluster contact, and validate regardless of --from-schedule Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 12 +++++++++++- pkg/cmd/cli/backup/create_test.go | 2 +- 2 files changed, 12 insertions(+), 2 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 7046f83e2..74cfe861d 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,7 +46,17 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: cobra.MaximumNArgs(1), + Args: func(c *cobra.Command, args []string) error { + if err := cobra.MaximumNArgs(1)(c, args); err != nil { + return err + } + if len(args) == 1 { + if errs := validation.IsDNS1123Subdomain(args[0]); len(errs) > 0 { + return fmt.Errorf("invalid backup name %q: %s", args[0], strings.Join(errs, "; ")) + } + } + return nil + }, Run: func(c *cobra.Command, args []string) { cmd.CheckError(o.Complete(args, f)) cmd.CheckError(o.Validate(c, args, f)) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 718ab0e96..4512ccd6c 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -233,7 +233,7 @@ func TestCreateOptions_OrderedResources(t *testing.T) { } func TestCreateCommand(t *testing.T) { - name := "nameToBeCreated" + name := "name-to-be-created" args := []string{name} t.Run("create a backup create command with full options except fromSchedule and wait, then run by create option", func(t *testing.T) { From 52f7c24084fef0062c10f76a251826cfdc0d7167 Mon Sep 17 00:00:00 2001 From: samay43 Date: Wed, 12 Aug 2026 16:39:47 +0530 Subject: [PATCH 05/28] address review feedback: validate backup name and require name unless from-schedule Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 74cfe861d..b5821a9b1 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,10 +46,14 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: func(c *cobra.Command, args []string) error { + Args: func(c *cobra.Command, args []string) error { if err := cobra.MaximumNArgs(1)(c, args); err != nil { return err } + fromSchedule, _ := c.Flags().GetString("from-schedule") + if fromSchedule == "" && len(args) == 0 { + return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") + } if len(args) == 1 { if errs := validation.IsDNS1123Subdomain(args[0]); len(errs) > 0 { return fmt.Errorf("invalid backup name %q: %s", args[0], strings.Join(errs, "; ")) @@ -202,13 +206,16 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return err } - // Ensure the backup name is a valid Kubernetes resource name - if o.FromSchedule == "" { + // Ensure that unless FromSchedule is set, a backup name is required + if o.FromSchedule == "" && o.Name == "" { + return fmt.Errorf("a backup name is required, unless you are creating based on a schedule") + } + // Validate the backup name format whenever a name is provided + if o.Name != "" { if errs := validation.IsDNS1123Subdomain(o.Name); len(errs) > 0 { return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) } } - errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { return kubeerrs.NewAggregate(errs) From c27a661d1d269eaea2f90251c22aba10fff8b959 Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 09:44:47 +0530 Subject: [PATCH 06/28] gofmt: fix indentation in create.go Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index b5821a9b1..3ef773b47 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -46,7 +46,7 @@ func NewCreateCommand(f client.Factory, use string) *cobra.Command { c := &cobra.Command{ Use: use + " NAME", Short: "Create a backup", - Args: func(c *cobra.Command, args []string) error { + Args: func(c *cobra.Command, args []string) error { if err := cobra.MaximumNArgs(1)(c, args); err != nil { return err } From fa3f5737c896c2836d7b922bca744309eed7e25e Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 10:57:54 +0530 Subject: [PATCH 07/28] add test coverage for Args validation and Validate method Signed-off-by: samay43 --- pkg/cmd/cli/backup/create_test.go | 110 ++++++++++++++++++++++++++++++ 1 file changed, 110 insertions(+) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index fe321f9c6..e955719eb 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -449,3 +449,113 @@ func TestCreateCommand(t *testing.T) { assert.NoError(t, e) }) } +func TestCreateCommand_Args(t *testing.T) { + testCases := []struct { + name string + args []string + fromSchedule string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + args: []string{}, + expectError: true, + }, + { + name: "should pass when a valid name is provided", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when the name is not a valid DNS1123 subdomain", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass with no name when from-schedule is set", + args: []string{}, + fromSchedule: "daily-backup", + expectError: false, + }, + { + name: "should error when more than one arg is given", + args: []string{"name1", "name2"}, + expectError: true, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + if tc.fromSchedule != "" { + err := cmd.Flags().Set("from-schedule", tc.fromSchedule) + assert.NoError(t, err) + } + + err := cmd.Args(cmd, tc.args) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} + +func TestCreateOptions_Validate(t *testing.T) { + testCases := []struct { + name string + optName string + fromSchedule string + args []string + expectError bool + }{ + { + name: "should error when no name and no from-schedule", + optName: "", + args: []string{}, + expectError: true, + }, + { + name: "should pass with a valid name and no from-schedule", + optName: "my-backup", + args: []string{"my-backup"}, + expectError: false, + }, + { + name: "should error when name is invalid, regardless of from-schedule", + optName: "Invalid_Name!", + fromSchedule: "daily-backup", + args: []string{"Invalid_Name!"}, + expectError: true, + }, + { + name: "should pass when from-schedule is set and no name given", + optName: "", + fromSchedule: "daily-backup", + args: []string{}, + expectError: false, + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + f := &factorymocks.Factory{} + cmd := NewCreateCommand(f, "") + + o := NewCreateOptions() + o.Name = tc.optName + o.FromSchedule = tc.fromSchedule + + err := o.Validate(cmd, tc.args, f) + + if tc.expectError { + assert.Error(t, err) + } else { + assert.NoError(t, err) + } + }) + } +} From e2c3b4b2699836b7df83787ba2293c2053729eb2 Mon Sep 17 00:00:00 2001 From: samay43 Date: Thu, 13 Aug 2026 11:08:21 +0530 Subject: [PATCH 08/28] fix lint: use require.NoError for error assertion Signed-off-by: samay43 --- pkg/cmd/cli/backup/create_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index e955719eb..5f9b9aac0 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -490,7 +490,7 @@ func TestCreateCommand_Args(t *testing.T) { cmd := NewCreateCommand(f, "") if tc.fromSchedule != "" { err := cmd.Flags().Set("from-schedule", tc.fromSchedule) - assert.NoError(t, err) + require.NoError(t, err) } err := cmd.Args(cmd, tc.args) From efe1279d49b945b437f1ed598a4eaef798c8816d Mon Sep 17 00:00:00 2001 From: samay43 Date: Fri, 14 Aug 2026 10:37:57 +0530 Subject: [PATCH 09/28] validate schedule-derived backup names don't exceed length limit Signed-off-by: samay43 --- pkg/cmd/cli/backup/create.go | 11 +++++++++++ pkg/cmd/cli/backup/create_test.go | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/pkg/cmd/cli/backup/create.go b/pkg/cmd/cli/backup/create.go index 7fe6b0e64..a8d988d72 100644 --- a/pkg/cmd/cli/backup/create.go +++ b/pkg/cmd/cli/backup/create.go @@ -216,6 +216,17 @@ func (o *CreateOptions) Validate(c *cobra.Command, args []string, f client.Facto return fmt.Errorf("invalid backup name %q: %s", o.Name, strings.Join(errs, "; ")) } } + // When a backup name will be generated from the schedule (i.e. FromSchedule + // is set and no explicit name was given), ensure the schedule name leaves + // enough room for the generated timestamp suffix ("-" + 14-digit timestamp, + // 15 characters total) within the DNS1123 subdomain length limit. + if o.FromSchedule != "" && o.Name == "" { + const timestampSuffixLen = 15 // "-" + "20060102150405" + maxScheduleNameLen := validation.DNS1123SubdomainMaxLength - timestampSuffixLen + if len(o.FromSchedule) > maxScheduleNameLen { + return fmt.Errorf("schedule name %q is too long: must be %d characters or fewer to leave room for the generated timestamp suffix", o.FromSchedule, maxScheduleNameLen) + } + } errs := collections.ValidateNamespaceIncludesExcludes(o.IncludeNamespaces, o.ExcludeNamespaces) if len(errs) > 0 { return kubeerrs.NewAggregate(errs) diff --git a/pkg/cmd/cli/backup/create_test.go b/pkg/cmd/cli/backup/create_test.go index 5f9b9aac0..528e76943 100644 --- a/pkg/cmd/cli/backup/create_test.go +++ b/pkg/cmd/cli/backup/create_test.go @@ -538,6 +538,20 @@ func TestCreateOptions_Validate(t *testing.T) { args: []string{}, expectError: false, }, + { + name: "should pass when schedule name leaves room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 238), // exactly at the 238-char limit + args: []string{}, + expectError: false, + }, + { + name: "should error when schedule name is too long to leave room for timestamp suffix", + optName: "", + fromSchedule: strings.Repeat("a", 239), // one over the 238-char limit + args: []string{}, + expectError: true, + }, } for _, tc := range testCases { From 1d9d45ee917b3a6ec5905a2aa5f4f26f1fbcf60f Mon Sep 17 00:00:00 2001 From: opbot_xd Date: Mon, 24 Aug 2026 14:04:26 +0530 Subject: [PATCH 10/28] Fix context propagation in GetDefaultBackupStorageLocations and add tests * Use correct context in GetDefaultBackupStorageLocations * Add TestGetDefaultBackupStorageLocations * Add TestNamespacedSecretStore * Add changelog for PR 10376 Signed-off-by: opbot_xd --- changelogs/unreleased/10376-opbot-xd | 1 + internal/credentials/secret_store_test.go | 97 +++++++++++++++++++++++ internal/storage/storagelocation.go | 2 +- internal/storage/storagelocation_test.go | 76 ++++++++++++++++++ 4 files changed, 175 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/10376-opbot-xd create mode 100644 internal/credentials/secret_store_test.go diff --git a/changelogs/unreleased/10376-opbot-xd b/changelogs/unreleased/10376-opbot-xd new file mode 100644 index 000000000..a7f254e66 --- /dev/null +++ b/changelogs/unreleased/10376-opbot-xd @@ -0,0 +1 @@ +Fix context propagation bug in GetDefaultBackupStorageLocations and add missing test coverage for core components diff --git a/internal/credentials/secret_store_test.go b/internal/credentials/secret_store_test.go new file mode 100644 index 000000000..f9acf6c27 --- /dev/null +++ b/internal/credentials/secret_store_test.go @@ -0,0 +1,97 @@ +/* +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 credentials + +import ( + "testing" + + . "github.com/onsi/gomega" + corev1api "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +func TestNamespacedSecretStore(t *testing.T) { + scheme := runtime.NewScheme() + g := NewWithT(t) + g.Expect(corev1api.AddToScheme(scheme)).To(Succeed()) + + secret := &corev1api.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-secret", + Namespace: "velero", + }, + Data: map[string][]byte{ + "creds-key": []byte("my-super-secret-value"), + }, + } + + client := fake.NewClientBuilder().WithScheme(scheme).WithRuntimeObjects(secret).Build() + + store, err := NewNamespacedSecretStore(client, "velero") + g.Expect(err).ToNot(HaveOccurred()) + + tests := []struct { + name string + selector *corev1api.SecretKeySelector + expectedVal string + expectErr bool + }{ + { + name: "existing secret and key returns the correct value", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "test-secret"}, + Key: "creds-key", + }, + expectedVal: "my-super-secret-value", + expectErr: false, + }, + { + name: "missing secret returns an error", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "missing-secret"}, + Key: "creds-key", + }, + expectedVal: "", + expectErr: true, + }, + { + name: "missing key in existing secret returns an error", + selector: &corev1api.SecretKeySelector{ + LocalObjectReference: corev1api.LocalObjectReference{Name: "test-secret"}, + Key: "missing-key", + }, + expectedVal: "", + expectErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + g := NewWithT(t) + + val, err := store.Get(tc.selector) + if tc.expectErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(val).To(Equal(tc.expectedVal)) + } + }) + } +} diff --git a/internal/storage/storagelocation.go b/internal/storage/storagelocation.go index 59afe6b79..5024553dc 100644 --- a/internal/storage/storagelocation.go +++ b/internal/storage/storagelocation.go @@ -96,7 +96,7 @@ func ListBackupStorageLocations(ctx context.Context, kbClient client.Client, nam func GetDefaultBackupStorageLocations(ctx context.Context, kbClient client.Client, namespace string) (*velerov1api.BackupStorageLocationList, error) { locations := new(velerov1api.BackupStorageLocationList) defaultLocations := new(velerov1api.BackupStorageLocationList) - if err := kbClient.List(context.Background(), locations, &client.ListOptions{Namespace: namespace}); err != nil { + if err := kbClient.List(ctx, locations, &client.ListOptions{Namespace: namespace}); err != nil { return defaultLocations, errors.Wrapf(err, "failed to list backup storage locations in namespace %s", namespace) } diff --git a/internal/storage/storagelocation_test.go b/internal/storage/storagelocation_test.go index 44eabff48..474369487 100644 --- a/internal/storage/storagelocation_test.go +++ b/internal/storage/storagelocation_test.go @@ -173,3 +173,79 @@ func TestListBackupStorageLocations(t *testing.T) { }) } } + +func TestGetDefaultBackupStorageLocations(t *testing.T) { + tests := []struct { + name string + locations *velerov1api.BackupStorageLocationList + expectedDefaults []string + expectedErr bool + }{ + { + name: "no default locations", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(false).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(false).Result(), + }, + }, + expectedDefaults: nil, + expectedErr: false, + }, + { + name: "one default location", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(false).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(true).Result(), + }, + }, + expectedDefaults: []string{"loc-2"}, + expectedErr: false, + }, + { + name: "multiple default locations", + locations: &velerov1api.BackupStorageLocationList{ + Items: []velerov1api.BackupStorageLocation{ + *builder.ForBackupStorageLocation("ns-1", "loc-1").Default(true).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-2").Default(true).Result(), + *builder.ForBackupStorageLocation("ns-1", "loc-3").Default(false).Result(), + }, + }, + expectedDefaults: []string{"loc-1", "loc-2"}, + expectedErr: false, + }, + { + name: "empty locations list", + locations: &velerov1api.BackupStorageLocationList{}, + expectedDefaults: nil, + expectedErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + + client := fake.NewClientBuilder().WithScheme(util.VeleroScheme).WithRuntimeObjects(tt.locations).Build() + + defaults, err := GetDefaultBackupStorageLocations(t.Context(), client, "ns-1") + if tt.expectedErr { + g.Expect(err).To(HaveOccurred()) + } else { + g.Expect(err).ToNot(HaveOccurred()) + + var defaultNames []string + for _, loc := range defaults.Items { + defaultNames = append(defaultNames, loc.Name) + } + + if tt.expectedDefaults == nil { + g.Expect(defaultNames).To(BeEmpty()) + } else { + g.Expect(defaultNames).To(ConsistOf(tt.expectedDefaults)) + } + } + }) + } +} From ef5375e3e39327e96308477addca92c6bc6f4121 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:19:55 +0000 Subject: [PATCH 11/28] Check both daemonsets before returning non-NotFound lookup error in IsReady Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- pkg/nodeagent/node_agent.go | 16 ++++++++--- pkg/nodeagent/node_agent_test.go | 46 ++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 3 deletions(-) diff --git a/pkg/nodeagent/node_agent.go b/pkg/nodeagent/node_agent.go index b449a91f4..755a56d85 100644 --- a/pkg/nodeagent/node_agent.go +++ b/pkg/nodeagent/node_agent.go @@ -82,13 +82,17 @@ func KbClientIsRunningInNode(ctx context.Context, namespace string, nodeName str } // IsReady checks whether the node-agent daemonset has at least one ready pod -// by inspecting the DaemonSet status. +// by inspecting the DaemonSet status. Both the linux and windows daemonsets +// are checked before returning any non-NotFound lookup error, so that a +// transient error fetching one daemonset does not mask the other daemonset +// being ready. func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client) error { dsLinux := new(appsv1api.DaemonSet) + var lookupErr error if err := crClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: daemonSet}, dsLinux); err != nil { dsLinux = nil if !apierrors.IsNotFound(err) { - return errors.Wrap(err, "failed to get linux node-agent daemonset") + lookupErr = errors.Wrap(err, "failed to get linux node-agent daemonset") } } @@ -96,7 +100,9 @@ func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client) if err := crClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: daemonsetWindows}, dsWindows); err != nil { dsWindows = nil if !apierrors.IsNotFound(err) { - return errors.Wrap(err, "failed to get windows node-agent daemonset") + if lookupErr == nil { + lookupErr = errors.Wrap(err, "failed to get windows node-agent daemonset") + } } } @@ -108,6 +114,10 @@ func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client) return nil } + if lookupErr != nil { + return lookupErr + } + return errors.New("node-agent is not ready: no ready pods found") } diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 9bba67ec4..24e1471d9 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/pkg/nodeagent/node_agent_test.go @@ -275,6 +275,52 @@ func TestIsReady(t *testing.T) { }, expectErr: "failed to get windows node-agent daemonset: fake-get-error", }, + { + name: "linux daemonset get error but windows ready", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + dsWindowsReady, + }, + interceptor: &interceptor.Funcs{ + Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error { + if key.Name == "node-agent" { + return errors.New("fake-get-error") + } + return c.Get(ctx, key, obj, opts...) + }, + }, + }, + { + name: "windows daemonset get error but linux ready", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + dsLinuxReady, + }, + interceptor: &interceptor.Funcs{ + Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error { + if key.Name == "node-agent-windows" { + return errors.New("fake-get-error") + } + return c.Get(ctx, key, obj, opts...) + }, + }, + }, + { + name: "linux daemonset get error and windows not ready", + namespace: "fake-ns", + kubeClientObj: []runtime.Object{ + dsWindowsNotReady, + }, + interceptor: &interceptor.Funcs{ + Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error { + if key.Name == "node-agent" { + return errors.New("fake-get-error") + } + return c.Get(ctx, key, obj, opts...) + }, + }, + expectErr: "failed to get linux node-agent daemonset: fake-get-error", + }, { name: "linux ds exist but no ready pods", namespace: "fake-ns", From da01c1ca95b1d388e699f6476d4c2c462b1717cc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:27:05 +0000 Subject: [PATCH 12/28] Combine both daemonset lookup errors (windows retained as secondary) Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- pkg/nodeagent/node_agent.go | 4 +--- pkg/nodeagent/node_agent_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 3 deletions(-) diff --git a/pkg/nodeagent/node_agent.go b/pkg/nodeagent/node_agent.go index 755a56d85..6dcdbf89b 100644 --- a/pkg/nodeagent/node_agent.go +++ b/pkg/nodeagent/node_agent.go @@ -100,9 +100,7 @@ func IsReady(ctx context.Context, namespace string, crClient ctrlclient.Client) if err := crClient.Get(ctx, ctrlclient.ObjectKey{Namespace: namespace, Name: daemonsetWindows}, dsWindows); err != nil { dsWindows = nil if !apierrors.IsNotFound(err) { - if lookupErr == nil { - lookupErr = errors.Wrap(err, "failed to get windows node-agent daemonset") - } + lookupErr = errors.CombineErrors(lookupErr, errors.Wrap(err, "failed to get windows node-agent daemonset")) } } diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 24e1471d9..40e0d3fd8 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/pkg/nodeagent/node_agent_test.go @@ -18,6 +18,7 @@ package nodeagent import ( "context" + "fmt" "testing" "github.com/cockroachdb/errors" @@ -408,6 +409,35 @@ func TestIsReady(t *testing.T) { } } +// TestIsReadyBothDaemonsetsGetError ensures that when both daemonset lookups +// return a non-NotFound error, the linux error is returned as the primary +// error while the windows error is retained as a secondary/attached error +// rather than being silently discarded. +func TestIsReadyBothDaemonsetsGetError(t *testing.T) { + scheme := runtime.NewScheme() + appsv1api.AddToScheme(scheme) + + fakeClient := clientFake.NewClientBuilder(). + WithScheme(scheme). + WithInterceptorFuncs(interceptor.Funcs{ + Get: func(ctx context.Context, c ctrlclient.WithWatch, key ctrlclient.ObjectKey, obj ctrlclient.Object, opts ...ctrlclient.GetOption) error { + if key.Name == "node-agent" { + return errors.New("fake-linux-get-error") + } + if key.Name == "node-agent-windows" { + return errors.New("fake-windows-get-error") + } + return c.Get(ctx, key, obj, opts...) + }, + }). + Build() + + err := IsReady(t.Context(), "fake-ns", fakeClient) + require.Error(t, err) + assert.EqualError(t, err, "failed to get linux node-agent daemonset: fake-linux-get-error") + assert.Contains(t, fmt.Sprintf("%+v", err), "failed to get windows node-agent daemonset: fake-windows-get-error") +} + func TestGetPodSpec(t *testing.T) { podSpec := corev1api.PodSpec{ NodeName: "fake-node", From b4bcea9236840020715b120a60b640134078c814 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Aug 2026 18:27:40 +0000 Subject: [PATCH 13/28] Add changelog entry for PR #10403 Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- changelogs/unreleased/10403-kaovilai | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10403-kaovilai diff --git a/changelogs/unreleased/10403-kaovilai b/changelogs/unreleased/10403-kaovilai new file mode 100644 index 000000000..e87620ce6 --- /dev/null +++ b/changelogs/unreleased/10403-kaovilai @@ -0,0 +1 @@ +Check both node-agent daemonsets before returning a non-NotFound lookup error in IsReady, so a transient error fetching one daemonset no longer masks the other daemonset being ready \ No newline at end of file From acefa77d89b9e92b71b52fda343911424057f4e0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 25 Aug 2026 23:45:53 +0000 Subject: [PATCH 14/28] Fix testifylint require-error lint failure in node_agent_test.go Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- pkg/nodeagent/node_agent_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/pkg/nodeagent/node_agent_test.go b/pkg/nodeagent/node_agent_test.go index 40e0d3fd8..4a406dd39 100644 --- a/pkg/nodeagent/node_agent_test.go +++ b/pkg/nodeagent/node_agent_test.go @@ -433,8 +433,7 @@ func TestIsReadyBothDaemonsetsGetError(t *testing.T) { Build() err := IsReady(t.Context(), "fake-ns", fakeClient) - require.Error(t, err) - assert.EqualError(t, err, "failed to get linux node-agent daemonset: fake-linux-get-error") + require.EqualError(t, err, "failed to get linux node-agent daemonset: fake-linux-get-error") assert.Contains(t, fmt.Sprintf("%+v", err), "failed to get windows node-agent daemonset: fake-windows-get-error") } From de35c2f8be2bd049b5867967906fa25fe9071d70 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 26 Aug 2026 15:51:25 +0000 Subject: [PATCH 15/28] Restrict e2e-test-kind PR trigger to Go/workflow changes Co-authored-by: kaovilai <11228024+kaovilai@users.noreply.github.com> --- .github/workflows/e2e-test-kind.yaml | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-) diff --git a/.github/workflows/e2e-test-kind.yaml b/.github/workflows/e2e-test-kind.yaml index fcf0d37c2..881da4901 100644 --- a/.github/workflows/e2e-test-kind.yaml +++ b/.github/workflows/e2e-test-kind.yaml @@ -12,11 +12,13 @@ env: on: push: pull_request: - # Do not run when the change only includes these directories. - paths-ignore: - - "site/**" - - "design/**" - - "**/*.md" + # Only run when the change affects Go code or a workflow this job depends on. + paths: + - "**/*.go" + - "go.mod" + - "go.sum" + - ".github/workflows/e2e-test-kind.yaml" + - ".github/workflows/get-go-version.yaml" jobs: get-go-version: uses: ./.github/workflows/get-go-version.yaml From e67e08ce2f88c35589cba34f5cfd43ad5d114fea Mon Sep 17 00:00:00 2001 From: opbot_xd Date: Thu, 27 Aug 2026 05:33:57 +0530 Subject: [PATCH 16/28] Testing: Add missing unit tests for pkg/itemblock Add comprehensive unit tests for pkg/itemblock which previously had zero test coverage. The new tests cover AddUnstructured and FindItem methods with 14 test cases across 3 test functions, including preferred GVR ordering, nil item handling, namespace/name filtering, unparseable apiVersion handling, and cluster-scoped resource matching. Fixes #10418 Signed-off-by: opbot_xd --- changelogs/unreleased/10418-opbot-xd | 1 + pkg/itemblock/itemblock_test.go | 357 +++++++++++++++++++++++++++ 2 files changed, 358 insertions(+) create mode 100644 changelogs/unreleased/10418-opbot-xd create mode 100644 pkg/itemblock/itemblock_test.go diff --git a/changelogs/unreleased/10418-opbot-xd b/changelogs/unreleased/10418-opbot-xd new file mode 100644 index 000000000..72f91c6b0 --- /dev/null +++ b/changelogs/unreleased/10418-opbot-xd @@ -0,0 +1 @@ +Add unit tests for pkg/itemblock diff --git a/pkg/itemblock/itemblock_test.go b/pkg/itemblock/itemblock_test.go new file mode 100644 index 000000000..930332330 --- /dev/null +++ b/pkg/itemblock/itemblock_test.go @@ -0,0 +1,357 @@ +/* +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 itemblock + +import ( + "testing" + + "github.com/sirupsen/logrus" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/runtime/schema" +) + +// newUnstructuredItem creates an unstructured object with the given apiVersion, +// kind, namespace, and name for use in tests. +func newUnstructuredItem(apiVersion, kind, namespace, name string) *unstructured.Unstructured { + return &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": apiVersion, + "kind": kind, + "metadata": map[string]any{ + "namespace": namespace, + "name": name, + }, + }, + } +} + +func TestAddUnstructured(t *testing.T) { + tests := []struct { + name string + items []ItemBlockItem + expectedLen int + }{ + { + name: "add single item to empty block", + items: []ItemBlockItem{ + { + Gr: schema.GroupResource{Group: "apps", Resource: "deployments"}, + Item: newUnstructuredItem("apps/v1", "Deployment", "default", "nginx"), + PreferredGVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + }, + }, + expectedLen: 1, + }, + { + name: "add multiple items", + items: []ItemBlockItem{ + { + Gr: schema.GroupResource{Group: "", Resource: "pods"}, + Item: newUnstructuredItem("v1", "Pod", "default", "pod-1"), + PreferredGVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"}, + }, + { + Gr: schema.GroupResource{Group: "", Resource: "services"}, + Item: newUnstructuredItem("v1", "Service", "kube-system", "kube-dns"), + PreferredGVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "services"}, + }, + { + Gr: schema.GroupResource{Group: "apps", Resource: "deployments"}, + Item: newUnstructuredItem("apps/v1", "Deployment", "default", "web"), + PreferredGVR: schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"}, + }, + }, + expectedLen: 3, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ib := &ItemBlock{ + Log: logrus.New(), + } + + for _, item := range tc.items { + ib.AddUnstructured(item.Gr, item.Item, item.PreferredGVR) + } + + require.Len(t, ib.Items, tc.expectedLen) + + // Verify each added item matches what was provided + for i, item := range tc.items { + assert.Equal(t, item.Gr, ib.Items[i].Gr) + assert.Equal(t, item.Item, ib.Items[i].Item) + assert.Equal(t, item.PreferredGVR, ib.Items[i].PreferredGVR) + } + }) + } +} + +func TestFindItem(t *testing.T) { + podsGR := schema.GroupResource{Group: "", Resource: "pods"} + deploymentsGR := schema.GroupResource{Group: "apps", Resource: "deployments"} + servicesGR := schema.GroupResource{Group: "", Resource: "services"} + + podsGVR := schema.GroupVersionResource{Group: "", Version: "v1", Resource: "pods"} + deploymentsGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + + tests := []struct { + name string + existingItems []ItemBlockItem + searchGR schema.GroupResource + searchNamespace string + searchName string + expectedCount int + // If set, verifies the names in the returned items are in this exact order + expectedNames []string + }{ + { + name: "find item in empty block returns nil", + existingItems: nil, + searchGR: podsGR, + searchNamespace: "default", + searchName: "pod-1", + expectedCount: 0, + }, + { + name: "find matching item by GR, namespace, and name", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: newUnstructuredItem("v1", "Pod", "default", "pod-1"), + PreferredGVR: podsGVR, + }, + }, + searchGR: podsGR, + searchNamespace: "default", + searchName: "pod-1", + expectedCount: 1, + expectedNames: []string{"pod-1"}, + }, + { + name: "no match when GR differs", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: newUnstructuredItem("v1", "Pod", "default", "pod-1"), + PreferredGVR: podsGVR, + }, + }, + searchGR: deploymentsGR, + searchNamespace: "default", + searchName: "pod-1", + expectedCount: 0, + }, + { + name: "no match when namespace differs", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: newUnstructuredItem("v1", "Pod", "default", "pod-1"), + PreferredGVR: podsGVR, + }, + }, + searchGR: podsGR, + searchNamespace: "kube-system", + searchName: "pod-1", + expectedCount: 0, + }, + { + name: "no match when name differs", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: newUnstructuredItem("v1", "Pod", "default", "pod-1"), + PreferredGVR: podsGVR, + }, + }, + searchGR: podsGR, + searchNamespace: "default", + searchName: "pod-2", + expectedCount: 0, + }, + { + name: "nil item is skipped", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: nil, + PreferredGVR: podsGVR, + }, + }, + searchGR: podsGR, + searchNamespace: "default", + searchName: "pod-1", + expectedCount: 0, + }, + { + name: "preferred GVR match is returned first", + existingItems: []ItemBlockItem{ + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1beta1", "Deployment", "default", "web"), + PreferredGVR: deploymentsGVR, // preferred is v1, item is v1beta1 → non-preferred + }, + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1", "Deployment", "default", "web"), + PreferredGVR: deploymentsGVR, // preferred is v1, item is v1 → preferred match + }, + }, + searchGR: deploymentsGR, + searchNamespace: "default", + searchName: "web", + expectedCount: 2, + expectedNames: []string{"web", "web"}, + }, + { + name: "multiple non-preferred items returned when no preferred match", + existingItems: []ItemBlockItem{ + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1beta1", "Deployment", "default", "web"), + PreferredGVR: deploymentsGVR, + }, + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1beta2", "Deployment", "default", "web"), + PreferredGVR: deploymentsGVR, + }, + }, + searchGR: deploymentsGR, + searchNamespace: "default", + searchName: "web", + expectedCount: 2, + }, + { + name: "item with unparsable apiVersion is treated as non-preferred", + existingItems: []ItemBlockItem{ + { + Gr: deploymentsGR, + Item: newUnstructuredItem("not/a/valid/version", "Deployment", "default", "web"), + PreferredGVR: deploymentsGVR, + }, + }, + searchGR: deploymentsGR, + searchNamespace: "default", + searchName: "web", + expectedCount: 1, + }, + { + name: "only matching GR items are returned from mixed block", + existingItems: []ItemBlockItem{ + { + Gr: podsGR, + Item: newUnstructuredItem("v1", "Pod", "default", "app"), + PreferredGVR: podsGVR, + }, + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1", "Deployment", "default", "app"), + PreferredGVR: deploymentsGVR, + }, + { + Gr: servicesGR, + Item: newUnstructuredItem("v1", "Service", "default", "app"), + PreferredGVR: schema.GroupVersionResource{Group: "", Version: "v1", Resource: "services"}, + }, + }, + searchGR: deploymentsGR, + searchNamespace: "default", + searchName: "app", + expectedCount: 1, + expectedNames: []string{"app"}, + }, + { + name: "cluster-scoped item found with empty namespace", + existingItems: []ItemBlockItem{ + { + Gr: schema.GroupResource{Group: "", Resource: "namespaces"}, + Item: newUnstructuredItem("v1", "Namespace", "", "production"), + PreferredGVR: schema.GroupVersionResource{ + Group: "", Version: "v1", Resource: "namespaces", + }, + }, + }, + searchGR: schema.GroupResource{Group: "", Resource: "namespaces"}, + searchNamespace: "", + searchName: "production", + expectedCount: 1, + expectedNames: []string{"production"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + ib := &ItemBlock{ + Log: logrus.New(), + Items: tc.existingItems, + } + + result := ib.FindItem(tc.searchGR, tc.searchNamespace, tc.searchName) + + require.Len(t, result, tc.expectedCount) + + if tc.expectedNames != nil { + for i, expectedName := range tc.expectedNames { + assert.Equal(t, expectedName, result[i].Item.GetName()) + } + } + }) + } +} + +func TestFindItemPreferredOrdering(t *testing.T) { + deploymentsGR := schema.GroupResource{Group: "apps", Resource: "deployments"} + deploymentsGVR := schema.GroupVersionResource{Group: "apps", Version: "v1", Resource: "deployments"} + + // Insert non-preferred first, preferred second to verify ordering + ib := &ItemBlock{ + Log: logrus.New(), + Items: []ItemBlockItem{ + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1beta1", "Deployment", "ns", "deploy"), + PreferredGVR: deploymentsGVR, + }, + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1", "Deployment", "ns", "deploy"), + PreferredGVR: deploymentsGVR, + }, + { + Gr: deploymentsGR, + Item: newUnstructuredItem("apps/v1beta2", "Deployment", "ns", "deploy"), + PreferredGVR: deploymentsGVR, + }, + }, + } + + result := ib.FindItem(deploymentsGR, "ns", "deploy") + + require.Len(t, result, 3) + + // The preferred match (apps/v1) should be first, regardless of insertion order + assert.Equal(t, "apps/v1", result[0].Item.GetAPIVersion(), + "preferred GVR match should be returned first") + + // Non-preferred items follow in insertion order + assert.Equal(t, "apps/v1beta1", result[1].Item.GetAPIVersion()) + assert.Equal(t, "apps/v1beta2", result[2].Item.GetAPIVersion()) +} From 53eb2b7b70457a6a73b396ee0cfa4a1d3a26541e Mon Sep 17 00:00:00 2001 From: opbot_xd Date: Thu, 27 Aug 2026 05:59:18 +0530 Subject: [PATCH 17/28] ci: retry e2e tests Signed-off-by: opbot_xd From f4d9bef51aded321c2113de625a6d4edc08a07be Mon Sep 17 00:00:00 2001 From: Lyndon-Li Date: Fri, 28 Aug 2026 12:49:01 +0800 Subject: [PATCH 18/28] enhance the doc for backup deletion Signed-off-by: Lyndon-Li --- changelogs/unreleased/10430-Lyndon-Li | 1 + site/content/docs/main/backup-reference.md | 11 ++++++++--- 2 files changed, 9 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/10430-Lyndon-Li diff --git a/changelogs/unreleased/10430-Lyndon-Li b/changelogs/unreleased/10430-Lyndon-Li new file mode 100644 index 000000000..924b23a87 --- /dev/null +++ b/changelogs/unreleased/10430-Lyndon-Li @@ -0,0 +1 @@ +Enhance the doc for backup deletion \ No newline at end of file diff --git a/site/content/docs/main/backup-reference.md b/site/content/docs/main/backup-reference.md index f2045211f..46e0b0f5e 100644 --- a/site/content/docs/main/backup-reference.md +++ b/site/content/docs/main/backup-reference.md @@ -161,7 +161,12 @@ Pagination can be entirely disabled by setting `--client-page-size` to `0`. This ## Deleting Backups -Use the following commands to delete Velero backups and data: +Use the following commands to delete Velero backups: +`velero backup delete `: successful run of this command will: +- Immediately delete the resource backup data from the backup storage +- Immediately delete the volume snapshots associates to the backup if any (e.g., volumes are backed up with CSI snapshot backup or native snapshot method) +- Trigger the deletion of volume backup data if any data is persisted to the backup repository (e.g., volumes are backed up with CSI snapshot data movement or fs-backup method). You will not see the backup storage space is released immediately, the backup repository maintenance jobs will GC the data and finally release the storage space -* `kubectl delete backup -n ` will delete the backup custom resource only and will not delete any associated data from object/block storage -* `velero backup delete ` will delete the backup resource including all data in object/block storage +Velero backup deletion needs extra spaces in the backup storage, so make sure the backup storage is not full during the backup deletion, otherwise, the backup deletion or the following backup repository maintenance jobs may fail. + +`kubectl delete backup -n `: this command will delete the backup custom resource only and will not delete any associated data from the backup storage. So it is used for limited purposes only, e.g., all the backup data has been deleted in the backup storage, you just need to clear the orphaned CRs in the cluster. From cf2ad280d7970247c04113b2e4c96f60abf4e51a Mon Sep 17 00:00:00 2001 From: Adeet Phanse Date: Fri, 28 Aug 2026 16:02:01 -0400 Subject: [PATCH 19/28] chore(deps): bump klauspost/compress to v1.19.2 Signed-off-by: Adeet Phanse --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index c38ab050a..e47ac9190 100644 --- a/go.mod +++ b/go.mod @@ -148,7 +148,7 @@ require ( github.com/kcp-dev/kcp/cli v0.27.1 // indirect github.com/kcp-dev/kcp/sdk v0.27.1 // indirect github.com/kcp-dev/logicalcluster/v3 v3.0.5 // indirect - github.com/klauspost/compress v1.18.6 // indirect + github.com/klauspost/compress v1.19.2 // indirect github.com/klauspost/cpuid/v2 v2.3.0 // indirect github.com/klauspost/crc32 v1.3.0 // indirect github.com/klauspost/pgzip v1.2.6 // indirect diff --git a/go.sum b/go.sum index f744bb2e4..0d195bfba 100644 --- a/go.sum +++ b/go.sum @@ -297,8 +297,8 @@ github.com/keybase/go-keychain v0.0.1 h1:way+bWYa6lDppZoZcgMbYsvC7GxljxrskdNInRt github.com/keybase/go-keychain v0.0.1/go.mod h1:PdEILRW3i9D8JcdM+FmY6RwkHGnhHxXwkPPMeUgOK1k= github.com/kisielk/errcheck v1.5.0/go.mod h1:pFxgyoBC7bSaBwPgfKdkLd5X25qrDl4LWUI2bnpBCr8= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= -github.com/klauspost/compress v1.18.6 h1:2jupLlAwFm95+YDR+NwD2MEfFO9d4z4Prjl1XXDjuao= -github.com/klauspost/compress v1.18.6/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= +github.com/klauspost/compress v1.19.2 h1:hMRETovs/pu/dVWN7zIT1PGG8t509MwT6bO7XSi26R8= +github.com/klauspost/compress v1.19.2/go.mod h1:cwPg85FWrGar70rWktvGQj8/hthj3wpl0PGDogxkrSQ= github.com/klauspost/cpuid/v2 v2.0.1/go.mod h1:FInQzS24/EEf25PyTYn52gqo7WaD8xa0213Md/qVLRg= github.com/klauspost/cpuid/v2 v2.3.0 h1:S4CRMLnYUhGeDFDqkGriYKdfoFlDnMtqTiI/sFzhA9Y= github.com/klauspost/cpuid/v2 v2.3.0/go.mod h1:hqwkgyIinND0mEev00jJYCxPNVRVXFQeu1XKlok6oO0= From 023d5ad471cce15a5047c5fe31a0a97cd8069c2c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:15:03 +0000 Subject: [PATCH 20/28] Bump the github-actions group with 2 updates Bumps the github-actions group with 2 updates: [helm/kind-action](https://github.com/helm/kind-action) and [github/codeql-action](https://github.com/github/codeql-action). Updates `helm/kind-action` from 7a97ed793754775518f9db3a8151ee7461dc9c31 to c72b4750145dbfb1c71734c3782a4db35a1c65c0 - [Release notes](https://github.com/helm/kind-action/releases) - [Commits](https://github.com/helm/kind-action/compare/7a97ed793754775518f9db3a8151ee7461dc9c31...c72b4750145dbfb1c71734c3782a4db35a1c65c0) Updates `github/codeql-action` from 4.37.7 to 4.37.9 - [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.7...v4.37.9) --- updated-dependencies: - dependency-name: helm/kind-action dependency-version: c72b4750145dbfb1c71734c3782a4db35a1c65c0 dependency-type: direct:production dependency-group: github-actions - dependency-name: github/codeql-action dependency-version: 4.37.9 dependency-type: direct:production update-type: version-update:semver-patch dependency-group: github-actions ... Signed-off-by: dependabot[bot] --- .github/workflows/e2e-test-kind.yaml | 2 +- .github/workflows/nightly-trivy-scan.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/e2e-test-kind.yaml b/.github/workflows/e2e-test-kind.yaml index fcf0d37c2..e3f220353 100644 --- a/.github/workflows/e2e-test-kind.yaml +++ b/.github/workflows/e2e-test-kind.yaml @@ -192,7 +192,7 @@ jobs: - name: Install MinIO run: | docker run -d --rm -p 9000:9000 -e "MINIO_ROOT_USER=minio" -e "MINIO_ROOT_PASSWORD=minio123" -e "MINIO_DEFAULT_BUCKETS=bucket,additional-bucket" bitnami/minio:local - - uses: helm/kind-action@7a97ed793754775518f9db3a8151ee7461dc9c31 # v1 + fix: add curl retry flags (https://github.com/helm/kind-action/pull/165) + - uses: helm/kind-action@c72b4750145dbfb1c71734c3782a4db35a1c65c0 # v1 + fix: add curl retry flags (https://github.com/helm/kind-action/pull/165) with: cluster_name: "kind" version: "v0.32.0" diff --git a/.github/workflows/nightly-trivy-scan.yml b/.github/workflows/nightly-trivy-scan.yml index d3f2e6062..09dcdbf4f 100644 --- a/.github/workflows/nightly-trivy-scan.yml +++ b/.github/workflows/nightly-trivy-scan.yml @@ -31,6 +31,6 @@ jobs: output: 'trivy-results.sarif' - name: Upload Trivy scan results to GitHub Security tab - uses: github/codeql-action/upload-sarif@v4.37.7 + uses: github/codeql-action/upload-sarif@v4.37.9 with: sarif_file: 'trivy-results.sarif' \ No newline at end of file From 7707681783f5812a6dd77bdbca6ae8fd78f1911b Mon Sep 17 00:00:00 2001 From: R4mbo Date: Mon, 31 Aug 2026 07:10:25 +0530 Subject: [PATCH 21/28] add test coverage for CleanupVolumeSnapshot (#10198) Signed-off-by: samay43 --- changelogs/unreleased/10198-samay43 | 1 + pkg/util/csi/volume_snapshot_test.go | 104 +++++++++++++++++++++++++++ 2 files changed, 105 insertions(+) create mode 100644 changelogs/unreleased/10198-samay43 diff --git a/changelogs/unreleased/10198-samay43 b/changelogs/unreleased/10198-samay43 new file mode 100644 index 000000000..ab4c0c7f9 --- /dev/null +++ b/changelogs/unreleased/10198-samay43 @@ -0,0 +1 @@ +add test coverage for CleanupVolumeSnapshot diff --git a/pkg/util/csi/volume_snapshot_test.go b/pkg/util/csi/volume_snapshot_test.go index 895935b4b..a8ab5ed4e 100644 --- a/pkg/util/csi/volume_snapshot_test.go +++ b/pkg/util/csi/volume_snapshot_test.go @@ -2179,3 +2179,107 @@ func TestGetVSCForVS(t *testing.T) { }) } } + +func TestCleanupVolumeSnapshot(t *testing.T) { + retainVSCName := "retain-vsc" + + testCases := []struct { + name string + volSnap *snapshotv1api.VolumeSnapshot + objs []runtime.Object + expectDeleted bool + // name of the VolumeSnapshotContent expected to have been patched to + // DeletionPolicy=Delete; empty when no VSC should be touched. + expectedVSC string + }{ + { + name: "should be a no-op if the VolumeSnapshot no longer exists", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "does-not-exist", + Namespace: "velero", + }, + }, + objs: []runtime.Object{}, + expectDeleted: false, + }, + { + name: "should delete a VolumeSnapshot with no bound VolumeSnapshotContent", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-no-vsc", + Namespace: "velero", + }, + }, + objs: []runtime.Object{ + &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-no-vsc", + Namespace: "velero", + }, + }, + }, + expectDeleted: true, + }, + { + name: "should patch bound VSC DeletionPolicy to Delete and delete the VolumeSnapshot", + volSnap: &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-with-vsc", + Namespace: "velero", + }, + }, + objs: []runtime.Object{ + &snapshotv1api.VolumeSnapshot{ + ObjectMeta: metav1.ObjectMeta{ + Name: "vs-with-vsc", + Namespace: "velero", + }, + Status: &snapshotv1api.VolumeSnapshotStatus{ + BoundVolumeSnapshotContentName: &retainVSCName, + }, + }, + &snapshotv1api.VolumeSnapshotContent{ + ObjectMeta: metav1.ObjectMeta{ + Name: "retain-vsc", + }, + Spec: snapshotv1api.VolumeSnapshotContentSpec{ + DeletionPolicy: snapshotv1api.VolumeSnapshotContentRetain, + }, + }, + }, + expectDeleted: true, + expectedVSC: "retain-vsc", + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + fakeClient := velerotest.NewFakeControllerRuntimeClient(t, tc.objs...) + + CleanupVolumeSnapshot(t.Context(), tc.volSnap, fakeClient, velerotest.NewLogger()) + + actual := new(snapshotv1api.VolumeSnapshot) + err := fakeClient.Get( + t.Context(), + crclient.ObjectKey{Name: tc.volSnap.Name, Namespace: tc.volSnap.Namespace}, + actual, + ) + + if tc.expectDeleted { + assert.True(t, apierrors.IsNotFound(err), "expected VolumeSnapshot to be deleted") + } + + if tc.expectedVSC != "" { + actualVSC := new(snapshotv1api.VolumeSnapshotContent) + err := fakeClient.Get( + t.Context(), + crclient.ObjectKey{Name: tc.expectedVSC}, + actualVSC, + ) + require.NoError(t, err) + assert.Equal(t, snapshotv1api.VolumeSnapshotContentDelete, actualVSC.Spec.DeletionPolicy) + } + }) + } +} From a607892eb0cc67a3829e64e3faa3db2630b72e1a Mon Sep 17 00:00:00 2001 From: opbot_xd Date: Tue, 1 Sep 2026 06:35:16 +0530 Subject: [PATCH 22/28] test: resolve remaining Ginkgo V2 and Gomega anti-patterns (#10440) - Replaced Expect().Should() and Expect().ShouldNot() with .To() and .ToNot() across 12 files (Task 1). - Replaced synchronously evaluated Eventually() with Expect() in server_status_request_controller_test.go (Task 2B). - Extracted Skip() calls inside lazy callbacks into conditional checks using slices.Contains() in enable_api_group_extentions.go (Task 3). Signed-off-by: opbot_xd --- pkg/controller/backup_sync_controller_test.go | 28 +++++++++---------- .../server_status_request_controller_test.go | 2 +- .../api-group/enable_api_group_extentions.go | 27 ++++++++---------- .../backup-volume-info/csi_data_mover.go | 2 +- .../basic/backup-volume-info/csi_snapshot.go | 2 +- .../backup-volume-info/filesystem_upload.go | 2 +- .../backup-volume-info/native_snapshot.go | 2 +- .../backup-volume-info/skipped_volumes.go | 2 +- test/e2e/bsl-mgmt/deletion.go | 4 +-- test/e2e/resource-filtering/exclude_label.go | 10 +++---- test/e2e/schedule/in_progress.go | 2 +- test/e2e/schedule/periodical.go | 2 +- 12 files changed, 41 insertions(+), 44 deletions(-) diff --git a/pkg/controller/backup_sync_controller_test.go b/pkg/controller/backup_sync_controller_test.go index 75f9c5205..5d03907f0 100644 --- a/pkg/controller/backup_sync_controller_test.go +++ b/pkg/controller/backup_sync_controller_test.go @@ -439,7 +439,7 @@ var _ = Describe("Backup Sync Reconciler", func() { } if test.location != nil { - Expect(r.client.Create(ctx, test.location)).ShouldNot(HaveOccurred()) + Expect(r.client.Create(ctx, test.location)).ToNot(HaveOccurred()) backupStores[test.location.Name] = &persistencemocks.BackupStore{} backupStore, ok := backupStores[test.location.Name] @@ -457,12 +457,12 @@ var _ = Describe("Backup Sync Reconciler", func() { for _, existingBackup := range test.existingBackups { err := client.Create(context.TODO(), existingBackup, &ctrlClient.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) } for _, existingPodVolumeBackup := range test.existingPodVolumeBackups { err := client.Create(context.TODO(), existingPodVolumeBackup, &ctrlClient.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) } actualResult, err := r.Reconcile(ctx, ctrl.Request{ @@ -534,7 +534,7 @@ var _ = Describe("Backup Sync Reconciler", func() { cloudBackupData.backup.Status.Expiration.After(fakeClock.Now())) { Expect(apierrors.IsNotFound(err)).To(BeTrue()) } else { - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) // did this cloud pod volume backup already exist in the cluster? var existingPodVolumeBackup *velerov1api.PodVolumeBackup @@ -673,7 +673,7 @@ var _ = Describe("Backup Sync Reconciler", func() { } queueScheme := runtime.NewScheme() - Expect(velerov1api.AddToScheme(queueScheme)).ShouldNot(HaveOccurred()) + Expect(velerov1api.AddToScheme(queueScheme)).ToNot(HaveOccurred()) for _, test := range tests { var ( @@ -693,7 +693,7 @@ var _ = Describe("Backup Sync Reconciler", func() { logger: velerotest.NewLogger(), } - Expect(client.Create(ctx, location)).ShouldNot(HaveOccurred(), test.name) + Expect(client.Create(ctx, location)).ToNot(HaveOccurred(), test.name) backupStore := &persistencemocks.BackupStore{} backupStores[location.Name] = backupStore backupStore.On("ListBackups").Return([]string{test.cloudBackup.Name}, nil) @@ -704,7 +704,7 @@ var _ = Describe("Backup Sync Reconciler", func() { _, err := syncReconciler.Reconcile(ctx, ctrl.Request{ NamespacedName: types.NamespacedName{Namespace: location.Namespace, Name: location.Name}, }) - Expect(err).ShouldNot(HaveOccurred(), test.name) + Expect(err).ToNot(HaveOccurred(), test.name) backupKey := types.NamespacedName{Namespace: "ns-1", Name: test.cloudBackup.Name} synced := &velerov1api.Backup{} @@ -714,7 +714,7 @@ var _ = Describe("Backup Sync Reconciler", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue(), test.name) continue } - Expect(err).ShouldNot(HaveOccurred(), test.name) + Expect(err).ToNot(HaveOccurred(), test.name) // Reconcile the synced backup with the queue controller twice: the first // reconcile would move a New/empty-phase backup to Queued, the second one @@ -723,11 +723,11 @@ var _ = Describe("Backup Sync Reconciler", func() { queueReconciler := NewBackupQueueReconciler(client, queueScheme, velerotest.NewLogger(), 1, NewBackupTracker()) for range 2 { _, err = queueReconciler.Reconcile(ctx, ctrl.Request{NamespacedName: backupKey}) - Expect(err).ShouldNot(HaveOccurred(), test.name) + Expect(err).ToNot(HaveOccurred(), test.name) } after := &velerov1api.Backup{} - Expect(client.Get(ctx, backupKey, after)).ShouldNot(HaveOccurred(), test.name) + Expect(client.Get(ctx, backupKey, after)).ToNot(HaveOccurred(), test.name) Expect(after.Status.Phase).To(BeEquivalentTo(test.expectPhase), test.name) // Hooks are dropped on sync, so the stored metadata cannot carry a payload // that a later code path could execute. @@ -880,7 +880,7 @@ var _ = Describe("Backup Sync Reconciler", func() { for _, backup := range test.k8sBackups { // add test backup to client err := client.Create(context.TODO(), backup, &ctrlClient.CreateOptions{}) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) } bslName := "default" @@ -890,7 +890,7 @@ var _ = Describe("Backup Sync Reconciler", func() { r.deleteOrphanedBackups(ctx, bslName, test.cloudBackups, velerotest.NewLogger()) numBackups, err := numBackups(client) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) fmt.Println("") @@ -913,7 +913,7 @@ var _ = Describe("Backup Sync Reconciler", func() { testObjList := backupSyncSourceOrderFunc(locationList) testObjArray, err := meta.ExtractList(testObjList) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) expectLocation := testObjArray[0].(*velerov1api.BackupStorageLocation) Expect(expectLocation.Spec.Default).To(BeEquivalentTo(true)) @@ -1085,7 +1085,7 @@ var _ = Describe("Backup Sync Reconciler", func() { //create all required schedules as needed. for _, creatable := range test.toCreate { err := b.client.Create(context.Background(), creatable) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) } references := b.filterBackupOwnerReferences(context.Background(), test.backup, logger) diff --git a/pkg/controller/server_status_request_controller_test.go b/pkg/controller/server_status_request_controller_test.go index eb95c7e87..b114642b3 100644 --- a/pkg/controller/server_status_request_controller_test.go +++ b/pkg/controller/server_status_request_controller_test.go @@ -92,7 +92,7 @@ var _ = Describe("Server Status Request Reconciler", func() { Expect(apierrors.IsNotFound(err)).To(BeTrue()) } else { Expect(err).ToNot(HaveOccurred()) - Eventually(instance.Status.Phase == test.expected.Status.Phase, timeout).Should(BeTrue()) + Expect(instance.Status.Phase).To(Equal(test.expected.Status.Phase)) } }, Entry("with phase=empty will be processed and phased successfully patched", request{ diff --git a/test/e2e/basic/api-group/enable_api_group_extentions.go b/test/e2e/basic/api-group/enable_api_group_extentions.go index 546d2f721..66fcef36d 100644 --- a/test/e2e/basic/api-group/enable_api_group_extentions.go +++ b/test/e2e/basic/api-group/enable_api_group_extentions.go @@ -19,6 +19,7 @@ package basic import ( "context" "fmt" + "slices" "time" . "github.com/onsi/ginkgo/v2" @@ -45,26 +46,22 @@ func APIExtensionsVersionsTest() { veleroCfg = VeleroCfg Expect(KubectlConfigUseContext(context.Background(), veleroCfg.DefaultClusterContext)).To(Succeed()) srcVersions, err := GetAPIVersions(veleroCfg.DefaultClient, resourceName) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) dstVersions, err := GetAPIVersions(veleroCfg.StandbyClient, resourceName) - Expect(err).ShouldNot(HaveOccurred()) + Expect(err).ToNot(HaveOccurred()) - Expect(srcVersions).Should(ContainElement("v1"), func() string { + if !slices.Contains(srcVersions, "v1") { Skip("CRD with apiextension versions srcVersions should have v1") - return "" - }) - Expect(srcVersions).Should(ContainElement("v1beta1"), func() string { - Skip("CRD with apiextension versions srcVersions should have v1") - return "" - }) - Expect(dstVersions).Should(ContainElement("v1"), func() string { + } + if !slices.Contains(srcVersions, "v1beta1") { + Skip("CRD with apiextension versions srcVersions should have v1beta1") + } + if !slices.Contains(dstVersions, "v1") { Skip("CRD with apiextension versions dstVersions should have v1") - return "" - }) - Expect(len(srcVersions) > 1 && len(dstVersions) == 1).Should(BeTrue(), func() string { + } + if !(len(srcVersions) > 1 && len(dstVersions) == 1) { Skip("Source cluster should support apiextension v1 and v1beta1, destination cluster should only support apiextension v1") - return "" - }) + } }) AfterEach(func() { By(fmt.Sprintf("Switch to default kubeconfig context %s", veleroCfg.DefaultClusterContext), func() { diff --git a/test/e2e/basic/backup-volume-info/csi_data_mover.go b/test/e2e/basic/backup-volume-info/csi_data_mover.go index 43edb0ddb..b29cafcab 100644 --- a/test/e2e/basic/backup-volume-info/csi_data_mover.go +++ b/test/e2e/basic/backup-volume-info/csi_data_mover.go @@ -55,7 +55,7 @@ func (c *CSIDataMoverVolumeInfo) Verify() error { BackupObjectsPrefix+"/"+c.BackupName, ) - Expect(err).ShouldNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") + Expect(err).ToNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") fmt.Printf("The VolumeInfo metadata content: %+v\n", *volumeInfo[0]) Expect(volumeInfo).ToNot(BeEmpty()) diff --git a/test/e2e/basic/backup-volume-info/csi_snapshot.go b/test/e2e/basic/backup-volume-info/csi_snapshot.go index 5eddb4ea2..4e3e89579 100644 --- a/test/e2e/basic/backup-volume-info/csi_snapshot.go +++ b/test/e2e/basic/backup-volume-info/csi_snapshot.go @@ -54,7 +54,7 @@ func (c *CSISnapshotVolumeInfo) Verify() error { BackupObjectsPrefix+"/"+c.BackupName, ) - Expect(err).ShouldNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") + Expect(err).ToNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") fmt.Printf("The VolumeInfo metadata content: %+v\n", *volumeInfo[0]) Expect(volumeInfo).ToNot(BeEmpty()) diff --git a/test/e2e/basic/backup-volume-info/filesystem_upload.go b/test/e2e/basic/backup-volume-info/filesystem_upload.go index e6266c0c6..4ac8f4240 100644 --- a/test/e2e/basic/backup-volume-info/filesystem_upload.go +++ b/test/e2e/basic/backup-volume-info/filesystem_upload.go @@ -54,7 +54,7 @@ func (f *FilesystemUploadVolumeInfo) Verify() error { BackupObjectsPrefix+"/"+f.BackupName, ) - Expect(err).ShouldNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") + Expect(err).ToNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") fmt.Printf("The VolumeInfo metadata content: %+v\n", *volumeInfo[0]) Expect(volumeInfo).ToNot(BeEmpty()) diff --git a/test/e2e/basic/backup-volume-info/native_snapshot.go b/test/e2e/basic/backup-volume-info/native_snapshot.go index 13a75d2e6..e0a7a08df 100644 --- a/test/e2e/basic/backup-volume-info/native_snapshot.go +++ b/test/e2e/basic/backup-volume-info/native_snapshot.go @@ -55,7 +55,7 @@ func (n *NativeSnapshotVolumeInfo) Verify() error { BackupObjectsPrefix+"/"+n.BackupName, ) - Expect(err).ShouldNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") + Expect(err).ToNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") fmt.Printf("The VolumeInfo metadata content: %+v\n", *volumeInfo[0]) Expect(volumeInfo).ToNot(BeEmpty()) diff --git a/test/e2e/basic/backup-volume-info/skipped_volumes.go b/test/e2e/basic/backup-volume-info/skipped_volumes.go index 2fc801a5f..294d87947 100644 --- a/test/e2e/basic/backup-volume-info/skipped_volumes.go +++ b/test/e2e/basic/backup-volume-info/skipped_volumes.go @@ -54,7 +54,7 @@ func (s *SkippedVolumeInfo) Verify() error { BackupObjectsPrefix+"/"+s.BackupName, ) - Expect(err).ShouldNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") + Expect(err).ToNot(HaveOccurred(), "Fail to get VolumeInfo metadata in the Backup Repository.") fmt.Printf("The VolumeInfo metadata content: %+v\n", *volumeInfo[0]) Expect(volumeInfo).ToNot(BeEmpty()) diff --git a/test/e2e/bsl-mgmt/deletion.go b/test/e2e/bsl-mgmt/deletion.go index 6423b203c..12da25df5 100644 --- a/test/e2e/bsl-mgmt/deletion.go +++ b/test/e2e/bsl-mgmt/deletion.go @@ -308,7 +308,7 @@ func BslDeletionTest(useVolumeSnapshots bool) { By(fmt.Sprintf("Get all backups from 2 BSLs %s before deleting one of them", backupLocation1), func() { backupsBeforeDel, err := GetAllBackups(context.Background(), veleroCfg.VeleroCLI) Expect(err).To(Succeed()) - Expect(cmp.Diff(backupsInBsl1AndBsl2, backupsBeforeDel, cmpopts.SortSlices(less))).Should(BeEmpty()) + Expect(cmp.Diff(backupsInBsl1AndBsl2, backupsBeforeDel, cmpopts.SortSlices(less))).To(BeEmpty()) By(fmt.Sprintf("Backup1 %s should exist in cloud object store before bsl deletion", backupName1), func() { Expect(ObjectsShouldBeInBucket(veleroCfg.ObjectStoreProvider, veleroCfg.CloudCredentialsFile, @@ -325,7 +325,7 @@ func BslDeletionTest(useVolumeSnapshots bool) { backupsAfterDel, err := GetAllBackups(context.Background(), veleroCfg.VeleroCLI) Expect(err).To(Succeed()) // Default BSL is deleted, so backups in additional BSL should be left only - Expect(cmp.Diff(backupsInBSL2, backupsAfterDel, cmpopts.SortSlices(less))).Should(BeEmpty()) + Expect(cmp.Diff(backupsInBSL2, backupsAfterDel, cmpopts.SortSlices(less))).To(BeEmpty()) }) }) diff --git a/test/e2e/resource-filtering/exclude_label.go b/test/e2e/resource-filtering/exclude_label.go index 6cfd2d030..7d0d06e42 100644 --- a/test/e2e/resource-filtering/exclude_label.go +++ b/test/e2e/resource-filtering/exclude_label.go @@ -127,7 +127,7 @@ func (e *ExcludeFromBackup) CreateResources() error { } By(fmt.Sprintf("Checking secret %s should exists in namespaces ...%s\n", secretName, namespace), func() { _, err = GetSecret(e.Client.ClientGo, namespace, e.CaseBaseName) - Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) }) //Create Configmap: to be included configmaptName := e.CaseBaseName @@ -148,21 +148,21 @@ func (e *ExcludeFromBackup) Verify() error { By(fmt.Sprintf("Checking resources in namespaces ...%s\n", namespace), func() { //Check namespace checkNS, err := GetNamespace(e.Ctx, e.Client, namespace) - Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("Could not retrieve test namespace %s", namespace)) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("Could not retrieve test namespace %s", namespace)) Expect(checkNS.Name).To(Equal(namespace), fmt.Sprintf("Retrieved namespace for %s has name %s instead", namespace, checkNS.Name)) //Check deployment: should be included _, err = GetDeployment(e.Client.ClientGo, namespace, e.CaseBaseName) - Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) //Check secrets: secrets should not be included _, err = GetSecret(e.Client.ClientGo, namespace, e.CaseBaseName) - Expect(err).Should(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) + Expect(err).To(HaveOccurred(), fmt.Sprintf("failed to list deployment in namespace: %q", namespace)) Expect(apierrors.IsNotFound(err)).To(BeTrue()) //Check configmap: should be included _, err = GetConfigMap(e.Client.ClientGo, namespace, e.CaseBaseName) - Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("failed to list configmap in namespace: %q", namespace)) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to list configmap in namespace: %q", namespace)) }) return nil } diff --git a/test/e2e/schedule/in_progress.go b/test/e2e/schedule/in_progress.go index 060740c3f..9bf8c3e1b 100644 --- a/test/e2e/schedule/in_progress.go +++ b/test/e2e/schedule/in_progress.go @@ -170,7 +170,7 @@ func (s *InProgressCase) Backup() error { } // There should be at most one in-progress backup per schedule. - Expect(inProgressBackupCount).Should(BeNumerically("<=", 1)) + Expect(inProgressBackupCount).To(BeNumerically("<=", 1)) // Already ensured at most one in-progress backup when schedule triggered 2 backups. // Succeed. diff --git a/test/e2e/schedule/periodical.go b/test/e2e/schedule/periodical.go index 330356e99..fafffa0fd 100644 --- a/test/e2e/schedule/periodical.go +++ b/test/e2e/schedule/periodical.go @@ -184,7 +184,7 @@ func (n *PeriodicalCase) Verify() error { By("Namespaces were restored", func() { for _, ns := range *n.NSIncluded { _, err := k8sutil.GetConfigMap(n.Client.ClientGo, ns, n.CaseBaseName) - Expect(err).ShouldNot(HaveOccurred(), fmt.Sprintf("failed to list CM in namespace: %s\n", ns)) + Expect(err).ToNot(HaveOccurred(), fmt.Sprintf("failed to list CM in namespace: %s\n", ns)) } }) return nil From 66fd079abbf1207a54f48cc8fdf19837d0fc8048 Mon Sep 17 00:00:00 2001 From: opbot_xd Date: Tue, 1 Sep 2026 06:46:48 +0530 Subject: [PATCH 23/28] changelog: add changelog for PR 10453 Signed-off-by: opbot_xd --- changelogs/unreleased/10453-opbot-xd | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelogs/unreleased/10453-opbot-xd diff --git a/changelogs/unreleased/10453-opbot-xd b/changelogs/unreleased/10453-opbot-xd new file mode 100644 index 000000000..753878cc6 --- /dev/null +++ b/changelogs/unreleased/10453-opbot-xd @@ -0,0 +1 @@ +test: resolve remaining Ginkgo V2 and Gomega anti-patterns (#10440) From 07768e7b336d6b5b4cceae1a1a68e09acefd788d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wenkai=20Yin=28=E5=B0=B9=E6=96=87=E5=BC=80=29?= Date: Tue, 1 Sep 2026 15:43:53 +0800 Subject: [PATCH 24/28] Add "IncrementalBytes" field to status of DataDownload and PVR to indicate data transferred by the incremental restore (#10421) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add "IncrementalBytes" field to status of DataDownload and PVR to indicate data transferred by the incremental restore Signed-off-by: Wenkai Yin(尹文开) --- changelogs/unreleased/10421-ywk253100 | 1 + .../v1/bases/velero.io_podvolumerestores.yaml | 15 +++++++++++ .../bases/velero.io_datadownloads.yaml | 16 +++++++++++ pkg/apis/velero/v1/pod_volume_restore_type.go | 8 ++++++ pkg/apis/velero/v1/zz_generated.deepcopy.go | 5 ++++ .../velero/v2alpha1/data_download_types.go | 8 ++++++ .../velero/v2alpha1/zz_generated.deepcopy.go | 5 ++++ pkg/controller/data_download_controller.go | 2 ++ .../pod_volume_restore_controller.go | 2 ++ pkg/datamover/restore_micro_service_test.go | 2 +- pkg/datapath/data_path.go | 4 +-- pkg/datapath/data_path_test.go | 2 +- pkg/datapath/types.go | 5 ++-- pkg/podvolume/restore_micro_service_test.go | 2 +- pkg/uploader/block/snapshot.go | 18 ++++++------- pkg/uploader/block/snapshot_test.go | 2 +- pkg/uploader/provider/block.go | 18 ++++++------- pkg/uploader/provider/block_test.go | 12 ++++----- pkg/uploader/provider/kopia.go | 9 ++++--- pkg/uploader/provider/kopia_test.go | 6 +++-- pkg/uploader/provider/mocks/Provider.go | 27 ++++++++++++------- pkg/uploader/provider/provider.go | 2 +- 22 files changed, 123 insertions(+), 48 deletions(-) create mode 100644 changelogs/unreleased/10421-ywk253100 diff --git a/changelogs/unreleased/10421-ywk253100 b/changelogs/unreleased/10421-ywk253100 new file mode 100644 index 000000000..20e5525a6 --- /dev/null +++ b/changelogs/unreleased/10421-ywk253100 @@ -0,0 +1 @@ +Add "IncrementalBytes" field to status of DataDownload and PVR to indicate data transferred by the incremental restore \ No newline at end of file diff --git a/config/crd/v1/bases/velero.io_podvolumerestores.yaml b/config/crd/v1/bases/velero.io_podvolumerestores.yaml index 2eea696c2..c8ddb9c87 100644 --- a/config/crd/v1/bases/velero.io_podvolumerestores.yaml +++ b/config/crd/v1/bases/velero.io_podvolumerestores.yaml @@ -21,6 +21,11 @@ spec: jsonPath: .status.phase name: Status type: string + - description: Restore type such as Full/Incremental + jsonPath: .spec.restoreType + name: Restore Type + priority: 10 + type: string - description: Time duration since this PodVolumeRestore was started jsonPath: .status.startTimestamp name: Started @@ -35,6 +40,12 @@ spec: jsonPath: .status.progress.totalBytes name: Total Bytes type: integer + - description: Incremental bytes + format: int64 + jsonPath: .status.incrementalBytes + name: Incremental Bytes + priority: 10 + type: integer - description: Name of the Backup Storage Location where the backup data is stored jsonPath: .spec.backupStorageLocation name: Storage Location @@ -193,6 +204,10 @@ spec: format: date-time nullable: true type: string + incrementalBytes: + description: IncrementalBytes holds the number of bytes restored incrementally + format: int64 + type: integer message: description: Message is a message about the pod volume restore's status. type: string diff --git a/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml b/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml index 71e662fe8..88fe710e4 100644 --- a/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml +++ b/config/crd/v2alpha1/bases/velero.io_datadownloads.yaml @@ -21,6 +21,11 @@ spec: jsonPath: .status.phase name: Status type: string + - description: Restore type such as Full/Incremental + jsonPath: .spec.restoreType + name: Restore Type + priority: 10 + type: string - description: Time duration since this DataDownload was started jsonPath: .status.startTimestamp name: Started @@ -35,6 +40,12 @@ spec: jsonPath: .status.progress.totalBytes name: Total Bytes type: integer + - description: Incremental bytes + format: int64 + jsonPath: .status.incrementalBytes + name: Incremental Bytes + priority: 10 + type: integer - description: Name of the Backup Storage Location where the backup data is stored jsonPath: .spec.backupStorageLocation name: Storage Location @@ -202,6 +213,11 @@ spec: format: date-time nullable: true type: string + incrementalBytes: + description: IncrementalBytes holds the number of bytes restored incrementally + since the last snapshot + format: int64 + type: integer message: description: Message is a message about the DataDownload's status. type: string diff --git a/pkg/apis/velero/v1/pod_volume_restore_type.go b/pkg/apis/velero/v1/pod_volume_restore_type.go index 5ded78175..725630a83 100644 --- a/pkg/apis/velero/v1/pod_volume_restore_type.go +++ b/pkg/apis/velero/v1/pod_volume_restore_type.go @@ -111,6 +111,10 @@ type PodVolumeRestoreStatus struct { // +optional Progress shared.DataMoveOperationProgress `json:"progress,omitempty"` + // IncrementalBytes holds the number of bytes restored incrementally + // +optional + IncrementalBytes *int64 `json:"incrementalBytes,omitempty"` + // AcceptedTimestamp records the time the pod volume restore is to be prepared. // The server's time is used for AcceptedTimestamp // +optional @@ -129,9 +133,13 @@ type PodVolumeRestoreStatus struct { // +kubebuilder:object:root=true // +kubebuilder:storageversion // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="PodVolumeRestore status such as New/InProgress" +// The "Restore Type" column is hidden by default to align with PVB. +// +kubebuilder:printcolumn:name="Restore Type",type="string",JSONPath=".spec.restoreType",description="Restore type such as Full/Incremental",priority=10 // +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startTimestamp",description="Time duration since this PodVolumeRestore was started" // +kubebuilder:printcolumn:name="Bytes Done",type="integer",format="int64",JSONPath=".status.progress.bytesDone",description="Completed bytes" // +kubebuilder:printcolumn:name="Total Bytes",type="integer",format="int64",JSONPath=".status.progress.totalBytes",description="Total bytes" +// The "Incremental Bytes" column is hidden by default to align with PVB. +// +kubebuilder:printcolumn:name="Incremental Bytes",type="integer",format="int64",JSONPath=".status.incrementalBytes",description="Incremental bytes",priority=10 // +kubebuilder:printcolumn:name="Storage Location",type="string",JSONPath=".spec.backupStorageLocation",description="Name of the Backup Storage Location where the backup data is stored" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since this PodVolumeRestore was created" // +kubebuilder:printcolumn:name="Node",type="string",JSONPath=".status.node",description="Name of the node where the PodVolumeRestore is processed" diff --git a/pkg/apis/velero/v1/zz_generated.deepcopy.go b/pkg/apis/velero/v1/zz_generated.deepcopy.go index f4dc8a79a..9c6c84dda 100644 --- a/pkg/apis/velero/v1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v1/zz_generated.deepcopy.go @@ -1170,6 +1170,11 @@ func (in *PodVolumeRestoreStatus) DeepCopyInto(out *PodVolumeRestoreStatus) { *out = (*in).DeepCopy() } out.Progress = in.Progress + if in.IncrementalBytes != nil { + in, out := &in.IncrementalBytes, &out.IncrementalBytes + *out = new(int64) + **out = **in + } if in.AcceptedTimestamp != nil { in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp *out = (*in).DeepCopy() diff --git a/pkg/apis/velero/v2alpha1/data_download_types.go b/pkg/apis/velero/v2alpha1/data_download_types.go index 57827b97d..caaeceba7 100644 --- a/pkg/apis/velero/v2alpha1/data_download_types.go +++ b/pkg/apis/velero/v2alpha1/data_download_types.go @@ -132,6 +132,10 @@ type DataDownloadStatus struct { // +optional Progress shared.DataMoveOperationProgress `json:"progress,omitempty"` + // IncrementalBytes holds the number of bytes restored incrementally since the last snapshot + // +optional + IncrementalBytes *int64 `json:"incrementalBytes,omitempty"` + // Node is name of the node where the DataDownload is processed. // +optional Node string `json:"node,omitempty"` @@ -154,9 +158,13 @@ type DataDownloadStatus struct { // +kubebuilder:object:root=true // +kubebuilder:storageversion // +kubebuilder:printcolumn:name="Status",type="string",JSONPath=".status.phase",description="DataDownload status such as New/InProgress" +// The "Restore Type" column is hidden by default to align with DataUpload. +// +kubebuilder:printcolumn:name="Restore Type",type="string",JSONPath=".spec.restoreType",description="Restore type such as Full/Incremental",priority=10 // +kubebuilder:printcolumn:name="Started",type="date",JSONPath=".status.startTimestamp",description="Time duration since this DataDownload was started" // +kubebuilder:printcolumn:name="Bytes Done",type="integer",format="int64",JSONPath=".status.progress.bytesDone",description="Completed bytes" // +kubebuilder:printcolumn:name="Total Bytes",type="integer",format="int64",JSONPath=".status.progress.totalBytes",description="Total bytes" +// The "Incremental Bytes" column is hidden by default to align with DataUpload. +// +kubebuilder:printcolumn:name="Incremental Bytes",type="integer",format="int64",JSONPath=".status.incrementalBytes",description="Incremental bytes",priority=10 // +kubebuilder:printcolumn:name="Storage Location",type="string",JSONPath=".spec.backupStorageLocation",description="Name of the Backup Storage Location where the backup data is stored" // +kubebuilder:printcolumn:name="Age",type="date",JSONPath=".metadata.creationTimestamp",description="Time duration since this DataDownload was created" // +kubebuilder:printcolumn:name="Node",type="string",JSONPath=".status.node",description="Name of the node where the DataDownload is processed" diff --git a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go index 927dc531c..d8186605f 100644 --- a/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go +++ b/pkg/apis/velero/v2alpha1/zz_generated.deepcopy.go @@ -123,6 +123,11 @@ func (in *DataDownloadStatus) DeepCopyInto(out *DataDownloadStatus) { *out = (*in).DeepCopy() } out.Progress = in.Progress + if in.IncrementalBytes != nil { + in, out := &in.IncrementalBytes, &out.IncrementalBytes + *out = new(int64) + **out = **in + } if in.AcceptedTimestamp != nil { in, out := &in.AcceptedTimestamp, &out.AcceptedTimestamp *out = (*in).DeepCopy() diff --git a/pkg/controller/data_download_controller.go b/pkg/controller/data_download_controller.go index d8062bc72..e2a62830e 100644 --- a/pkg/controller/data_download_controller.go +++ b/pkg/controller/data_download_controller.go @@ -32,6 +32,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" "k8s.io/utils/clock" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -501,6 +502,7 @@ func (r *DataDownloadReconciler) OnDataDownloadCompleted(ctx context.Context, na } dd.Status.Phase = velerov2alpha1api.DataDownloadPhaseCompleted + dd.Status.IncrementalBytes = ptr.To(result.Restore.IncrementalBytes) dd.Status.CompletionTimestamp = &metav1.Time{Time: r.Clock.Now()} delete(dd.Labels, exposer.ExposeOnGoingLabel) diff --git a/pkg/controller/pod_volume_restore_controller.go b/pkg/controller/pod_volume_restore_controller.go index b6d4985fa..4fe9bdaa3 100644 --- a/pkg/controller/pod_volume_restore_controller.go +++ b/pkg/controller/pod_volume_restore_controller.go @@ -33,6 +33,7 @@ import ( "k8s.io/apimachinery/pkg/util/wait" "k8s.io/client-go/kubernetes" clocks "k8s.io/utils/clock" + "k8s.io/utils/ptr" ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/builder" "sigs.k8s.io/controller-runtime/pkg/client" @@ -837,6 +838,7 @@ func (r *PodVolumeRestoreReconciler) OnDataPathCompleted(ctx context.Context, na pvr.Status.Phase = velerov1api.PodVolumeRestorePhaseCompleted pvr.Status.CompletionTimestamp = &metav1.Time{Time: r.clock.Now()} + pvr.Status.IncrementalBytes = ptr.To(result.Restore.IncrementalBytes) delete(pvr.Labels, exposer.ExposeOnGoingLabel) diff --git a/pkg/datamover/restore_micro_service_test.go b/pkg/datamover/restore_micro_service_test.go index 311c015a7..63a3e6a5e 100644 --- a/pkg/datamover/restore_micro_service_test.go +++ b/pkg/datamover/restore_micro_service_test.go @@ -102,7 +102,7 @@ func TestOnDataDownloadCompleted(t *testing.T) { { name: "marshal fail", marshalErr: errors.New("fake-marshal-error"), - expectedErr: "Failed to marshal restore result {{ } 0}: fake-marshal-error", + expectedErr: "Failed to marshal restore result {{ } 0 0}: fake-marshal-error", }, { name: "succeed", diff --git a/pkg/datapath/data_path.go b/pkg/datapath/data_path.go index 1e7ae948e..0513619be 100644 --- a/pkg/datapath/data_path.go +++ b/pkg/datapath/data_path.go @@ -250,7 +250,7 @@ func (dp *generalDataPath) StartRestore(snapshotID string, target AccessPoint, u dp.wgDataPath.Done() }() - totalBytes, err := dp.uploaderProv.RunRestore(dp.ctx, snapshotID, target.ByPath, restoreParam.Incremental, + incrementalBytes, totalBytes, err := dp.uploaderProv.RunRestore(dp.ctx, snapshotID, target.ByPath, restoreParam.Incremental, provider.CBTParam{ Source: cbtservice.SourceInfo{ Snapshot: restoreParam.VolumeSnapshotName, @@ -268,7 +268,7 @@ func (dp *generalDataPath) StartRestore(snapshotID string, target AccessPoint, u } dp.callbacks.OnFailed(context.Background(), dp.namespace, dp.jobName, dataPathErr) } else { - dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Restore: RestoreResult{Target: target, TotalBytes: totalBytes}}) + dp.callbacks.OnCompleted(context.Background(), dp.namespace, dp.jobName, Result{Restore: RestoreResult{Target: target, TotalBytes: totalBytes, IncrementalBytes: incrementalBytes}}) } }() diff --git a/pkg/datapath/data_path_test.go b/pkg/datapath/data_path_test.go index 34f989517..495c949f9 100644 --- a/pkg/datapath/data_path_test.go +++ b/pkg/datapath/data_path_test.go @@ -190,7 +190,7 @@ func TestAsyncRestore(t *testing.T) { t.Run(test.name, func(t *testing.T) { dp := newGeneralDataPath("job-1", "test", nil, "velero", Callbacks{}, velerotest.NewLogger()).(*generalDataPath) mockProvider := providerMock.NewProvider(t) - mockProvider.On("RunRestore", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Restore.TotalBytes, test.err) + mockProvider.On("RunRestore", mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything, mock.Anything).Return(test.result.Restore.IncrementalBytes, test.result.Restore.TotalBytes, test.err) mockProvider.On("Close", mock.Anything).Return(nil) dp.uploaderProv = mockProvider dp.initialized = true diff --git a/pkg/datapath/types.go b/pkg/datapath/types.go index 339aa6ca4..bc7d4a9a9 100644 --- a/pkg/datapath/types.go +++ b/pkg/datapath/types.go @@ -43,8 +43,9 @@ type BackupResult struct { // RestoreResult represents the result of a restore type RestoreResult struct { - Target AccessPoint `json:"target,omitempty"` - TotalBytes int64 `json:"totalBytes,omitempty"` + Target AccessPoint `json:"target,omitempty"` + TotalBytes int64 `json:"totalBytes,omitempty"` + IncrementalBytes int64 `json:"incrementalBytes,omitempty"` } // Callbacks defines the collection of callbacks during backup/restore diff --git a/pkg/podvolume/restore_micro_service_test.go b/pkg/podvolume/restore_micro_service_test.go index 1964d5035..46c8813b6 100644 --- a/pkg/podvolume/restore_micro_service_test.go +++ b/pkg/podvolume/restore_micro_service_test.go @@ -165,7 +165,7 @@ func TestOnPvrCompleted(t *testing.T) { { name: "marshal fail", marshalErr: errors.New("fake-marshal-error"), - expectedErr: "error marshaling restore result {{ } 0}: fake-marshal-error", + expectedErr: "error marshaling restore result {{ } 0 0}: fake-marshal-error", }, { name: "succeed", diff --git a/pkg/uploader/block/snapshot.go b/pkg/uploader/block/snapshot.go index ff844d197..595a80fd5 100644 --- a/pkg/uploader/block/snapshot.go +++ b/pkg/uploader/block/snapshot.go @@ -205,12 +205,12 @@ func getParentBackupInfo(ctx context.Context, rep udmrepo.BackupRepo, forceFull } // Restore restore specific sourcePath with given snapshotID and update progress -func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapshotID, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, error) { +func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapshotID, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, int64, error) { log.Info("Start to restore...") snapshot, err := rep.GetSnapshot(ctx, udmrepo.ID(snapshotID)) if err != nil { - return 0, errors.Wrapf(err, "Unable to load snapshot %v", snapshotID) + return 0, 0, errors.Wrapf(err, "Unable to load snapshot %v", snapshotID) } log.Infof("Restore from snapshot %s, incremental %v, cbt source %v, description %s, created time %v, tags %v", snapshotID, incremental, cbtSource, snapshot.Description, snapshot.EndTime, snapshot.Tags) @@ -246,34 +246,34 @@ func Restore(ctx context.Context, blkUp Uploader, rep udmrepo.BackupRepo, snapsh destPath, err := filepath.Abs(dest) if err != nil { - return 0, errors.Wrapf(err, "invalid dest path '%s'", dest) + return 0, 0, errors.Wrapf(err, "invalid dest path '%s'", dest) } destPath = filepath.Clean(destPath) destDev, err := openBlockDeviceFunc(destPath, false) if err != nil { - return 0, errors.Wrapf(err, "error opening block device '%s'", destPath) + return 0, 0, errors.Wrapf(err, "error opening block device '%s'", destPath) } defer destDev.Close() destSize, err := destDev.Seek(0, io.SeekEnd) if err != nil { - return 0, errors.Wrapf(err, "error getting length of block device %s", dest) + return 0, 0, errors.Wrapf(err, "error getting length of block device %s", dest) } _, err = destDev.Seek(0, io.SeekStart) if err != nil { - return 0, errors.Wrapf(err, "error reset pos of block device %s", dest) + return 0, 0, errors.Wrapf(err, "error reset pos of block device %s", dest) } - _, totalSize, err := blkUp.Restore(snapshot, destInfo{dev: destDev, path: destPath, size: destSize}, bitmap.Iterator(), uploaderCfg) + incrementalBytes, totalSize, err := blkUp.Restore(snapshot, destInfo{dev: destDev, path: destPath, size: destSize}, bitmap.Iterator(), uploaderCfg) if err != nil { - return 0, errors.Wrapf(err, "error restoring to block dev %s", destPath) + return 0, 0, errors.Wrapf(err, "error restoring to block dev %s", destPath) } - return totalSize, nil + return incrementalBytes, totalSize, nil } func findPreviousSnapshot(ctx context.Context, rep udmrepo.BackupRepo, path string, snapshotTags map[string]string, noLaterThan *time.Time, log logrus.FieldLogger) (udmrepo.Snapshot, error) { diff --git a/pkg/uploader/block/snapshot_test.go b/pkg/uploader/block/snapshot_test.go index d7e7d2ee2..5a18c376e 100644 --- a/pkg/uploader/block/snapshot_test.go +++ b/pkg/uploader/block/snapshot_test.go @@ -842,7 +842,7 @@ func TestRestore(t *testing.T) { cbtSvc = tc.cbtService(t) } - size, err := Restore(ctx, mockBlkup, mockRepo, "snap-001", "/dev/sdb", tc.incremental, tc.cbtSource, cbtSvc, map[string]string{}, testLog()) + _, size, err := Restore(ctx, mockBlkup, mockRepo, "snap-001", "/dev/sdb", tc.incremental, tc.cbtSource, cbtSvc, map[string]string{}, testLog()) if tc.expectedErrStr != "" { require.Error(t, err) diff --git a/pkg/uploader/provider/block.go b/pkg/uploader/provider/block.go index 2b5ad275f..3d51e275b 100644 --- a/pkg/uploader/provider/block.go +++ b/pkg/uploader/provider/block.go @@ -167,9 +167,9 @@ func (bp *blockProvider) RunRestore( cbtParam CBTParam, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, - updater uploader.ProgressUpdater) (int64, error) { + updater uploader.ProgressUpdater) (int64, int64, error) { if updater == nil { - return 0, errors.New("restore progress updater is invalid") + return 0, 0, errors.New("restore progress updater is invalid") } log := bp.log.WithFields(logrus.Fields{ @@ -180,24 +180,24 @@ func (bp *blockProvider) RunRestore( blkUploader := block.NewUploader(ctx, bp.bkRepo, updater, log) - size, err := blockRestoreFunc(ctx, blkUploader, bp.bkRepo, snapshotID, volumePath, incremental, cbtParam.Source, cbtParam.Service, uploaderCfg, log) + incrementalBytes, totalBytes, err := blockRestoreFunc(ctx, blkUploader, bp.bkRepo, snapshotID, volumePath, incremental, cbtParam.Source, cbtParam.Service, uploaderCfg, log) // errors.Is, not ==: see the equivalent comment on the backup path above. if errors.Is(err, block.ErrCanceled) { log.Warn("Block restore is canceled") - return 0, ErrorCanceled + return 0, 0, ErrorCanceled } if err != nil { - return 0, errors.Wrapf(err, "Failed to run block restore") + return 0, 0, errors.Wrapf(err, "Failed to run block restore") } updater.UpdateProgress(&uploader.Progress{ - TotalBytes: size, - BytesDone: size, + TotalBytes: totalBytes, + BytesDone: totalBytes, }) - log.Infof("Block restore finished, restore size %v", size) + log.Infof("Block restore finished, restore incremental size %v, total size %v", incrementalBytes, totalBytes) - return size, nil + return incrementalBytes, totalBytes, nil } diff --git a/pkg/uploader/provider/block_test.go b/pkg/uploader/provider/block_test.go index 970fc7cf6..e38de4985 100644 --- a/pkg/uploader/provider/block_test.go +++ b/pkg/uploader/provider/block_test.go @@ -412,8 +412,8 @@ func TestBlockProviderCancelThroughWrappedError(t *testing.T) { t.Run("restore", func(t *testing.T) { orig := blockRestoreFunc defer func() { blockRestoreFunc = orig }() - blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ bool, _ cbtservice.SourceInfo, _ cbtservice.Service, _ map[string]string, _ logrus.FieldLogger) (int64, error) { - return 0, errors.Wrap(block.ErrCanceled, "error restoring bdev") + blockRestoreFunc = func(_ context.Context, _ block.Uploader, _ udmrepo.BackupRepo, _ string, _ string, _ bool, _ cbtservice.SourceInfo, _ cbtservice.Service, _ map[string]string, _ logrus.FieldLogger) (int64, int64, error) { + return 0, 0, errors.Wrap(block.ErrCanceled, "error restoring bdev") } bp := &blockProvider{ @@ -422,7 +422,7 @@ func TestBlockProviderCancelThroughWrappedError(t *testing.T) { log: logrus.New(), } - _, err := bp.RunRestore(t.Context(), "snap-1", "/dev/sda", false, CBTParam{}, + _, _, err := bp.RunRestore(t.Context(), "snap-1", "/dev/sda", false, CBTParam{}, uploader.PersistentVolumeBlock, map[string]string{}, &blockMockProgressUpdater{}) require.ErrorIs(t, err, ErrorCanceled) @@ -496,10 +496,10 @@ func TestBlockProviderRunRestore(t *testing.T) { var capturedSnapshotID string var capturedVolumePath string - blockRestoreFunc = func(ctx context.Context, blkUp block.Uploader, rep udmrepo.BackupRepo, snapshotID string, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, error) { + blockRestoreFunc = func(ctx context.Context, blkUp block.Uploader, rep udmrepo.BackupRepo, snapshotID string, dest string, incremental bool, cbtSource cbtservice.SourceInfo, cbtService cbtservice.Service, uploaderCfg map[string]string, log logrus.FieldLogger) (int64, int64, error) { capturedSnapshotID = snapshotID capturedVolumePath = dest - return tc.mockRestoreSize, tc.mockRestoreErr + return tc.mockRestoreSize, tc.mockRestoreSize, tc.mockRestoreErr } bp := &blockProvider{ @@ -507,7 +507,7 @@ func TestBlockProviderRunRestore(t *testing.T) { log: logrus.New(), } - size, err := bp.RunRestore( + _, size, err := bp.RunRestore( t.Context(), tc.snapshotID, tc.volumePath, diff --git a/pkg/uploader/provider/kopia.go b/pkg/uploader/provider/kopia.go index c9d9948bf..1cd1ecd45 100644 --- a/pkg/uploader/provider/kopia.go +++ b/pkg/uploader/provider/kopia.go @@ -215,7 +215,7 @@ func (kp *kopiaProvider) RunRestore( _ CBTParam, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, - updater uploader.ProgressUpdater) (int64, error) { + updater uploader.ProgressUpdater) (int64, int64, error) { log := kp.log.WithFields(logrus.Fields{ "snapshotID": snapshotID, "volumePath": volumePath, @@ -239,12 +239,12 @@ func (kp *kopiaProvider) RunRestore( size, fileCount, err := kopiaRestoreFunc(context.Background(), repoWriter, progress, snapshotID, volumePath, incremental, volMode, uploaderCfg, log, restoreCancel) if err != nil { - return 0, errors.Wrapf(err, "Failed to run kopia restore") + return 0, 0, errors.Wrapf(err, "Failed to run kopia restore") } if atomic.LoadInt32(&kp.canceling) == 1 { log.Error("Kopia restore is canceled") - return 0, ErrorCanceled + return 0, 0, ErrorCanceled } // which ensure that the statistic data of TotalBytes equal to BytesDone when finished @@ -257,5 +257,6 @@ func (kp *kopiaProvider) RunRestore( log.Info(output) - return size, nil + // the incremental bytes is the same as the total bytes because total bytes is the size of actual data Kopia writes + return size, size, nil } diff --git a/pkg/uploader/provider/kopia_test.go b/pkg/uploader/provider/kopia_test.go index a29a3c424..ca1cf8f5a 100644 --- a/pkg/uploader/provider/kopia_test.go +++ b/pkg/uploader/provider/kopia_test.go @@ -157,8 +157,10 @@ func TestRunRestore(t *testing.T) { if tc.volMode == "" { tc.volMode = uploader.PersistentVolumeFilesystem } - kopiaRestoreFunc = tc.hookRestoreFunc - _, err := kp.RunRestore(t.Context(), "", "/var", tc.incremental, CBTParam{}, tc.volMode, map[string]string{}, &updater) + kopiaRestoreFunc = func(ctx context.Context, rep repo.RepositoryWriter, progress *kopia.Progress, snapshotID, dest string, incremental bool, volMode uploader.PersistentVolumeMode, uploaderCfg map[string]string, log logrus.FieldLogger, cancleCh chan struct{}) (int64, int32, error) { + return tc.hookRestoreFunc(ctx, rep, progress, snapshotID, dest, incremental, volMode, uploaderCfg, log, cancleCh) + } + _, _, err := kp.RunRestore(t.Context(), "", "/var", tc.incremental, CBTParam{}, tc.volMode, map[string]string{}, &updater) if tc.notError { assert.NoError(t, err) } else { diff --git a/pkg/uploader/provider/mocks/Provider.go b/pkg/uploader/provider/mocks/Provider.go index 5bd3dda54..7fad73e59 100644 --- a/pkg/uploader/provider/mocks/Provider.go +++ b/pkg/uploader/provider/mocks/Provider.go @@ -223,7 +223,7 @@ func (_c *Provider_RunBackup_Call) RunAndReturn(run func(ctx context.Context, pa } // RunRestore provides a mock function for the type Provider -func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error) { +func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, int64, error) { ret := _mock.Called(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) if len(ret) == 0 { @@ -231,8 +231,9 @@ func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volume } var r0 int64 - var r1 error - if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) (int64, error)); ok { + var r1 int64 + var r2 error + if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) (int64, int64, error)); ok { return returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) } if returnFunc, ok := ret.Get(0).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) int64); ok { @@ -240,12 +241,20 @@ func (_mock *Provider) RunRestore(ctx context.Context, snapshotID string, volume } else { r0 = ret.Get(0).(int64) } - if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) error); ok { + + if returnFunc, ok := ret.Get(1).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) int64); ok { r1 = returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) } else { - r1 = ret.Error(1) + r1 = ret.Get(1).(int64) } - return r0, r1 + + if returnFunc, ok := ret.Get(2).(func(context.Context, string, string, bool, provider.CBTParam, uploader.PersistentVolumeMode, map[string]string, uploader.ProgressUpdater) error); ok { + r2 = returnFunc(ctx, snapshotID, volumePath, incremental, cbtParam, volMode, uploaderConfig, updater) + } else { + r2 = ret.Error(2) + } + + return r0, r1, r2 } // Provider_RunRestore_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'RunRestore' @@ -314,12 +323,12 @@ func (_c *Provider_RunRestore_Call) Run(run func(ctx context.Context, snapshotID return _c } -func (_c *Provider_RunRestore_Call) Return(n int64, err error) *Provider_RunRestore_Call { - _c.Call.Return(n, err) +func (_c *Provider_RunRestore_Call) Return(_a0 int64, _a1 int64, _a2 error) *Provider_RunRestore_Call { + _c.Call.Return(_a0, _a1, _a2) return _c } -func (_c *Provider_RunRestore_Call) RunAndReturn(run func(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, error)) *Provider_RunRestore_Call { +func (_c *Provider_RunRestore_Call) RunAndReturn(run func(ctx context.Context, snapshotID string, volumePath string, incremental bool, cbtParam provider.CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, updater uploader.ProgressUpdater) (int64, int64, error)) *Provider_RunRestore_Call { _c.Call.Return(run) return _c } diff --git a/pkg/uploader/provider/provider.go b/pkg/uploader/provider/provider.go index 9d06578d8..7f003989d 100644 --- a/pkg/uploader/provider/provider.go +++ b/pkg/uploader/provider/provider.go @@ -68,7 +68,7 @@ type Provider interface { cbtParam CBTParam, volMode uploader.PersistentVolumeMode, uploaderConfig map[string]string, - updater uploader.ProgressUpdater) (int64, error) + updater uploader.ProgressUpdater) (int64, int64, error) // Close which will close related repository Close(ctx context.Context) error } From 25b21f3c5c0ae85283f9ef0be409ebcdb3ef481f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Wenkai=20Yin=28=E5=B0=B9=E6=96=87=E5=BC=80=29?= Date: Tue, 1 Sep 2026 15:45:22 +0800 Subject: [PATCH 25/28] Get the volume ID before creating the restore PVC, otherwise the existing PV may be deleted during the creation of restore PVC (#10435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Get the volume ID before creating the restore PVC, otherwise the existing PV may be deleted during the creation of restore PVC Signed-off-by: Wenkai Yin(尹文开) --- pkg/exposer/generic_restore.go | 50 ++++++++++++++++++---------------- 1 file changed, 26 insertions(+), 24 deletions(-) diff --git a/pkg/exposer/generic_restore.go b/pkg/exposer/generic_restore.go index b19720389..0ecf06e83 100644 --- a/pkg/exposer/generic_restore.go +++ b/pkg/exposer/generic_restore.go @@ -252,6 +252,31 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap } }() + // Get volumeID before creating the restore pod because the existingPV may be deleted when creating the PVC if the volume policy is different + var volumeID string + if param.CSI != nil && param.CSI.Snapshot != nil { + vs := &snapshotv1api.VolumeSnapshot{} + if err := e.ctrlClient.Get(ctx, client.ObjectKey{ + Namespace: param.CSI.Snapshot.VolumeSnapshotNamespace, + Name: param.CSI.Snapshot.VolumeSnapshot, + }, vs); err != nil { + return errors.Wrapf(err, "error to get volume snapshot %s/%s", param.CSI.Snapshot.VolumeSnapshotNamespace, param.CSI.Snapshot.VolumeSnapshot) + } + + vsc, err := csi.GetVSCForVS(ctx, vs, e.ctrlClient) + if err != nil { + return errors.Wrapf(err, "error to get volume snapshot content for volume snapshot %s/%s", vs.Namespace, vs.Name) + } + + var cbtInfo csi.CBTInfo + cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, vs, vsc, param.TargetPVName) + if err != nil { + return errors.Wrap(err, "error to get CBT info") + } + curLog.Debugf("CBT info: %+v", cbtInfo) + volumeID = cbtInfo.VolumeID + } + curLog.Info("Creating restore PVC") var targetPV *corev1api.PersistentVolume @@ -281,29 +306,6 @@ func (e *genericRestoreExposer) Expose(ctx context.Context, ownerObject corev1ap }() curLog.Info("Creating restore pod") - var volumeID string - if param.CSI != nil && param.CSI.Snapshot != nil { - vs := &snapshotv1api.VolumeSnapshot{} - if err := e.ctrlClient.Get(ctx, client.ObjectKey{ - Namespace: param.CSI.Snapshot.VolumeSnapshotNamespace, - Name: param.CSI.Snapshot.VolumeSnapshot, - }, vs); err != nil { - return errors.Wrapf(err, "error to get volume snapshot %s/%s", param.CSI.Snapshot.VolumeSnapshotNamespace, param.CSI.Snapshot.VolumeSnapshot) - } - - vsc, err := csi.GetVSCForVS(ctx, vs, e.ctrlClient) - if err != nil { - return errors.Wrapf(err, "error to get volume snapshot content for volume snapshot %s/%s", vs.Namespace, vs.Name) - } - - var cbtInfo csi.CBTInfo - cbtInfo, err = csi.GetCBTInfo(ctx, e.kubeClient, e.log, vs, vsc, param.TargetPVName) - if err != nil { - return errors.Wrap(err, "error to get CBT info") - } - curLog.Debugf("CBT info: %+v", cbtInfo) - volumeID = cbtInfo.VolumeID - } var csiSnapshotMetadataServiceConfigs *velerotypes.CSISnapshotMetadataService if param.CSI != nil { csiSnapshotMetadataServiceConfigs = param.CSI.SnapshotMetadataServiceConfigs @@ -1004,7 +1006,7 @@ func (e *genericRestoreExposer) createRestorePVC(ctx context.Context, ownerObjec Spec: *targetPV.Spec.DeepCopy(), } tmpPV.Spec.VolumeMode = restorePVC.Spec.VolumeMode - e.log.Infof("the volume mode is different, creating temporary PV %s with volume mode %s", tmpPV.Name, tmpPV.Spec.VolumeMode) + e.log.Infof("the volume mode is different, creating temporary PV %s with volume mode %v", tmpPV.Name, tmpPV.Spec.VolumeMode) tmpPV, err = e.kubeClient.CoreV1().PersistentVolumes().Create(ctx, tmpPV, metav1.CreateOptions{}) if err != nil { return nil, errors.Wrapf(err, "fail to create the temporary PV %s", volumeName) From efc69c61aacfa18b3dca82a35e31c1515a37cca1 Mon Sep 17 00:00:00 2001 From: Chlins Zhang Date: Tue, 1 Sep 2026 17:08:25 +0800 Subject: [PATCH 26/28] Add in-place restore pre-flight check: target PVC must not be in use (#10419) Check the target PVC is not used by any active pod before any side effect, on both the CSI data mover path and the file system path. The in-use semantics align with the pvc-protection controller: terminal-phase pods don't block, terminating pods block with a wait hint. On the file system path, pods gated by this restore's restore-wait init container (identified by the restore UID in its args, and not yet terminated) are exempted: they must mount the PVC for the node-agent to restore the data and cannot write to the volume until the PodVolumeRestores complete. Leftover pods, controller-recreated pods, and pods gated by a different restore still block. Signed-off-by: chlins --- changelogs/unreleased/10419-chlins | 1 + .../volume-data-inplace-restore.md | 8 +- pkg/podvolume/restorer.go | 13 ++ pkg/podvolume/restorer_test.go | 104 ++++++++- pkg/restore/actions/csi/pvc_action.go | 7 + pkg/restore/actions/csi/pvc_action_test.go | 99 +++++++++ pkg/restore/inplace/preflight.go | 131 ++++++++++++ pkg/restore/inplace/preflight_test.go | 202 ++++++++++++++++++ 8 files changed, 563 insertions(+), 2 deletions(-) create mode 100644 changelogs/unreleased/10419-chlins create mode 100644 pkg/restore/inplace/preflight.go create mode 100644 pkg/restore/inplace/preflight_test.go diff --git a/changelogs/unreleased/10419-chlins b/changelogs/unreleased/10419-chlins new file mode 100644 index 000000000..62d3a1382 --- /dev/null +++ b/changelogs/unreleased/10419-chlins @@ -0,0 +1 @@ +Add in-place restore pre-flight check: target PVC must not be in use diff --git a/design/volume-data-inplace-restore/volume-data-inplace-restore.md b/design/volume-data-inplace-restore/volume-data-inplace-restore.md index b178b9314..4977c09d1 100644 --- a/design/volume-data-inplace-restore/volume-data-inplace-restore.md +++ b/design/volume-data-inplace-restore/volume-data-inplace-restore.md @@ -236,7 +236,13 @@ The key requirements for this approach are: Before initiating an in-place restore for a volume, Velero performs the following pre-flight checks to ensure the operation is safe and valid: #### 1. PVC is Not Actively Used by a Running Pod -Velero verifies that the target PVC is not currently mounted or consumed by any running Pods in the cluster. If the PVC is in use, Velero will skip the in-place restore for that volume and log an error. This enforces the prerequisite that users must completely delete consuming workloads prior to the restore, which prevents data corruption and avoids deadlocks caused by the Kubernetes `pvc-protection` finalizer during PVC recreation. +Velero verifies that the target PVC is not currently mounted or consumed by any active Pods in the cluster. If the PVC is in use, Velero will skip the in-place restore for that volume and log an error. This enforces the prerequisite that users must completely delete consuming workloads prior to the restore, which prevents data corruption and avoids deadlocks caused by the Kubernetes `pvc-protection` finalizer during PVC recreation. + +The "in use" semantics align with the Kubernetes `pvc-protection` controller: Pods in a terminal phase (`Succeeded`/`Failed`) do not block the restore, all other phases do, and terminating Pods are flagged in the error message so users know to simply wait and retry. + +The check runs on both restore paths before any side effect on the existing PVC/PV: in the PVC CSI RIA before deleting the existing PVC, and before creating the `PodVolumeRestore` on the file system path. On the file system path, Pods gated by this restore's `restore-wait` init container (identified by the restore UID in its args, and not yet terminated) are exempted: they must mount the PVC for the node-agent to restore the data, and they cannot write to the volume until this restore's `PodVolumeRestore`s complete. Leftover Pods, controller-recreated Pods, and Pods gated by a different restore still block. + +This check is a fail-fast validation, not an atomic guarantee; the `pvc-protection` finalizer remains the actual safety gate for PVC deletion. A residual `VolumeAttachment` check (e.g. a `Failed` Pod imposed by the control plane after a non-graceful node shutdown, where the node never unmounted the volume) may be added as a future enhancement. #### 2. PVC is Bound to the Original PV Velero checks whether the existing PVC in the cluster is still bound to the same PersistentVolume (PV) it was bound to at the time of the backup. If the PVC is bound to a different PV, performing an in-place restore (especially an incremental one that relies on Changed Block Tracking) may be unsafe or result in unpredictable behavior. If this check fails, Velero will log an error and skip the in-place restore for that volume. diff --git a/pkg/podvolume/restorer.go b/pkg/podvolume/restorer.go index 53d35215c..e135ba860 100644 --- a/pkg/podvolume/restorer.go +++ b/pkg/podvolume/restorer.go @@ -38,6 +38,7 @@ import ( "github.com/vmware-tanzu/velero/pkg/label" "github.com/vmware-tanzu/velero/pkg/nodeagent" "github.com/vmware-tanzu/velero/pkg/repository" + "github.com/vmware-tanzu/velero/pkg/restore/inplace" uploaderutil "github.com/vmware-tanzu/velero/pkg/uploader/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" "github.com/vmware-tanzu/velero/pkg/util/kube" @@ -179,6 +180,18 @@ func (r *restorer) RestorePodVolumes(data RestoreData, tracker *volume.RestoreVo } } + // Pre-flight checks for in-place restore. Pods gated by this + // restore's restore-wait init container are excluded: they must mount + // the PVC so the volume gets mounted on the node for the node-agent + // 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.CheckPVCNotInUse(r.ctx, r.crClient, pvc, data.Restore.UID); err != nil { + errs = append(errs, err) + continue + } + } + volumeRestore := newPodVolumeRestore(data.Restore, data.Pod, data.BackupLocation, volume, backupInfo.snapshotID, backupInfo.snapshotSize, "", backupInfo.uploaderType, data.SourceNamespace, pvc) if err := veleroclient.CreateRetryGenerateName(r.crClient, r.ctx, volumeRestore); err != nil { errs = append(errs, errors.WithStack(err)) diff --git a/pkg/podvolume/restorer_test.go b/pkg/podvolume/restorer_test.go index bd67e8480..e75f2f42b 100644 --- a/pkg/podvolume/restorer_test.go +++ b/pkg/podvolume/restorer_test.go @@ -37,6 +37,7 @@ import ( velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/repository" + "github.com/vmware-tanzu/velero/pkg/restorehelper" velerotest "github.com/vmware-tanzu/velero/pkg/test" ) @@ -119,6 +120,21 @@ func TestGetVolumesRepositoryType(t *testing.T) { } } +// createGatedPodObj returns the restored pod as it exists in the cluster: +// running with the restore-wait init container injected by the given restore. +func createGatedPodObj(restoreUID string, volumeNum int) *corev1api.Pod { + pod := createPodObj(true, true, true, volumeNum) + pod.Spec.InitContainers = append([]corev1api.Container{{ + Name: restorehelper.WaitInitContainer, + Args: []string{restoreUID}, + }}, pod.Spec.InitContainers...) + pod.Status.InitContainerStatuses = []corev1api.ContainerStatus{{ + Name: restorehelper.WaitInitContainer, + State: corev1api.ContainerState{Running: &corev1api.ContainerStateRunning{}}, + }} + return pod +} + func createNodeAgentDaemonset() *appsv1api.DaemonSet { ds := &appsv1api.DaemonSet{ ObjectMeta: metav1.ObjectMeta{ @@ -181,6 +197,7 @@ func TestRestorePodVolumes(t *testing.T) { pvbs []*velerov1api.PodVolumeBackup restoredPod *corev1api.Pod sourceNamespace string + inplace bool errs []expectError }{ { @@ -340,6 +357,86 @@ func TestRestorePodVolumes(t *testing.T) { completedPVR, }, }, + { + name: "in-place restore blocked when the PVC is used by another running pod", + pvbs: []*velerov1api.PodVolumeBackup{ + createPVBObj(true, true, 1, "kopia"), + }, + inplace: true, + kubeClientObj: []runtime.Object{ + createNodeAgentDaemonset(), + createPVCObj(1), + func() *corev1api.Pod { + pod := builder.ForPod("fake-ns", "other-pod"). + Volumes(builder.ForVolume("fake-volume-1").PersistentVolumeClaimSource("fake-pvc-1").Result()). + Result() + pod.Status.Phase = corev1api.PodRunning + return pod + }(), + }, + ctlClientObj: []runtime.Object{ + createBackupRepoObj(), + }, + restoredPod: createPodObj(true, true, true, 1), + sourceNamespace: "fake-ns", + bsl: "fake-bsl", + runtimeScheme: scheme, + errs: []expectError{ + { + err: "in-place restore pre-flight check failed", + prefixOnly: true, + }, + }, + }, + { + name: "in-place restore blocked when the pod is gated by a different restore", + pvbs: []*velerov1api.PodVolumeBackup{ + createPVBObj(true, true, 1, "kopia"), + }, + inplace: true, + kubeClientObj: []runtime.Object{ + createNodeAgentDaemonset(), + createPVCObj(1), + createGatedPodObj("old-restore-uid", 1), + }, + ctlClientObj: []runtime.Object{ + createBackupRepoObj(), + }, + restoredPod: createPodObj(true, true, true, 1), + sourceNamespace: "fake-ns", + bsl: "fake-bsl", + runtimeScheme: scheme, + errs: []expectError{ + { + err: "in-place restore pre-flight check failed", + prefixOnly: true, + }, + }, + }, + { + name: "in-place restore proceeds when the PVC is only used by the gated restored pod", + pvbs: []*velerov1api.PodVolumeBackup{ + createPVBObj(true, true, 1, "kopia"), + }, + inplace: true, + kubeClientObj: []runtime.Object{ + createNodeAgentDaemonset(), + createNodeObj(), + createPVCObj(1), + createGatedPodObj("fake-restore-uid", 1), + createNodeAgentPodObj(true), + }, + ctlClientObj: []runtime.Object{ + createBackupRepoObj(), + }, + restoredPod: createPodObj(true, true, true, 1), + sourceNamespace: "fake-ns", + bsl: "fake-bsl", + runtimeScheme: scheme, + retPVRs: []*velerov1api.PodVolumeRestore{ + completedPVR, + }, + }, } for _, test := range tests { @@ -362,7 +459,12 @@ func TestRestorePodVolumes(t *testing.T) { ensurer := repository.NewEnsurer(fakeCRClient, velerotest.NewLogger(), time.Millisecond) - restoreObj := builder.ForRestore(velerov1api.DefaultNamespace, "fake-restore").Result() + restoreBuilder := builder.ForRestore(velerov1api.DefaultNamespace, "fake-restore"). + ObjectMeta(builder.WithUID("fake-restore-uid")) + if test.inplace { + restoreBuilder = restoreBuilder.ExistingVolumeDataPolicy(string(velerov1api.VolumeDataPolicyTypeFull)) + } + restoreObj := restoreBuilder.Result() rs := newRestorer(ctx, repository.NewRepoLocker(), ensurer, pvrInformer, kubeClient, fakeCRClient, restoreObj, velerotest.NewLogger()) diff --git a/pkg/restore/actions/csi/pvc_action.go b/pkg/restore/actions/csi/pvc_action.go index a14b985a7..5bf8f45d1 100644 --- a/pkg/restore/actions/csi/pvc_action.go +++ b/pkg/restore/actions/csi/pvc_action.go @@ -44,6 +44,7 @@ import ( plugincommon "github.com/vmware-tanzu/velero/pkg/plugin/framework/common" "github.com/vmware-tanzu/velero/pkg/plugin/velero" riav2 "github.com/vmware-tanzu/velero/pkg/plugin/velero/restoreitemaction/v2" + "github.com/vmware-tanzu/velero/pkg/restore/inplace" uploaderUtil "github.com/vmware-tanzu/velero/pkg/uploader/util" "github.com/vmware-tanzu/velero/pkg/util" "github.com/vmware-tanzu/velero/pkg/util/boolptr" @@ -236,6 +237,12 @@ func (p *pvcRestoreItemAction) executeWithDataMove(logger *logrus.Entry, input * if existingPVC.Status.Phase != corev1api.ClaimBound { return nil, errors.New("ExistingVolumeDataPolicy is in-place restore, but the existing PVC is not bound.") } + + // Pre-flight checks must pass before any side effect on the existing PVC/PV. + if err := inplace.CheckPVCNotInUse(ctx, p.crClient, existingPVC, input.Restore.UID); err != nil { + return nil, errors.WithStack(err) + } + // take a CSI snapshot of the existing PVC as the baseline of CBT if input.Restore.IsVolumeDataInplaceIncrementalRestore() && datamover.IsVeleroBlockDataMover(dataUploadResult.DataMover) { logger.Info("ExistingVolumeDataPolicy is in-place incremental restore and data mover is velero-block. Taking a CSI snapshot of the existing PVC as the baseline of CBT...") diff --git a/pkg/restore/actions/csi/pvc_action_test.go b/pkg/restore/actions/csi/pvc_action_test.go index 47e8937a1..15cc4ca4d 100644 --- a/pkg/restore/actions/csi/pvc_action_test.go +++ b/pkg/restore/actions/csi/pvc_action_test.go @@ -741,6 +741,105 @@ func TestExecuteInplaceRestore(t *testing.T) { require.Equal(t, "testPV", dataDownloadList.Items[0].Spec.TargetVolume.PV) } +// TestExecuteInplaceRestorePreflight verifies the RIA fails the item without +// side effects when the pre-flight check fails. The in-use semantics are +// covered by the pkg/restore/inplace unit tests. +func TestExecuteInplaceRestorePreflight(t *testing.T) { + newPodUsingPVC := func(phase corev1api.PodPhase) *corev1api.Pod { + pod := builder.ForPod("velero", "consumer-pod"). + Volumes(builder.ForVolume("data").PersistentVolumeClaimSource("testPVC").Result()). + Result() + pod.Status.Phase = phase + return pod + } + + tests := []struct { + name string + pod *corev1api.Pod + expectBlock bool + }{ + { + name: "no pod, restore proceeds", + }, + { + name: "active pod blocks the restore", + pod: newPodUsingPVC(corev1api.PodRunning), + expectBlock: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + existingPVC := builder.ForPersistentVolumeClaim("velero", "testPVC"). + VolumeName("testPV"). + Phase(corev1api.ClaimBound).Result() + existingPV := builder.ForPersistentVolume("testPV").Result() + backup := builder.ForBackup("velero", "testBackup").SnapshotMoveData(true).Result() + restore := builder.ForRestore("velero", "testRestore").Backup("testBackup"). + ObjectMeta(builder.WithUID("uid")).ExistingVolumeDataPolicy("full").Result() + pvcFromBackup := builder.ForPersistentVolumeClaim("velero", "testPVC"). + ObjectMeta(builder.WithAnnotations( + velerov1api.VolumeSnapshotLabel, "vsName", + velerov1api.DataUploadNameAnnotation, "velero/testDU", + )).Result() + dataUploadResult := builder.ForConfigMap("velero", "testCM").Data("uid", "{}"). + ObjectMeta(builder.WithLabels( + velerov1api.RestoreUIDLabel, "uid", + velerov1api.PVCNamespaceNameLabel, "velero.testPVC", + velerov1api.ResourceUsageLabel, label.GetValidName(string(velerov1api.VeleroResourceUsageDataUploadResult)), + )).Result() + + crObjects := []runtime.Object{existingPVC, existingPV, backup, dataUploadResult} + kubeObjects := []runtime.Object{existingPVC, existingPV} + if tc.pod != nil { + crObjects = append(crObjects, tc.pod) + kubeObjects = append(kubeObjects, tc.pod) + } + + pvcRIA := pvcRestoreItemAction{ + log: logrus.New(), + crClient: velerotest.NewFakeControllerRuntimeClient(t, crObjects...), + kubeClient: fake.NewSimpleClientset(kubeObjects...), + } + + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup.DeepCopy()) + require.NoError(t, err) + pvcFromBackupMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(pvcFromBackup) + require.NoError(t, err) + + _, err = pvcRIA.Execute(&velero.RestoreItemActionExecuteInput{ + Item: &unstructured.Unstructured{Object: pvcMap}, + ItemFromBackup: &unstructured.Unstructured{Object: pvcFromBackupMap}, + Restore: restore, + }) + + gotPVC, getErr := pvcRIA.kubeClient.CoreV1().PersistentVolumeClaims("velero").Get(t.Context(), "testPVC", metav1.GetOptions{}) + dataDownloadList := new(velerov2alpha1.DataDownloadList) + require.NoError(t, pvcRIA.crClient.List(t.Context(), dataDownloadList, &crclient.ListOptions{})) + + if tc.expectBlock { + require.Error(t, err) + require.Contains(t, err.Error(), "pre-flight check failed") + require.Contains(t, err.Error(), "consumer-pod") + // No side effects: PVC untouched with the original volumeName, + // PV reclaim policy not patched, no DataDownload created. + require.NoError(t, getErr) + require.Equal(t, "testPV", gotPVC.Spec.VolumeName) + gotPV, pvErr := pvcRIA.kubeClient.CoreV1().PersistentVolumes().Get(t.Context(), "testPV", metav1.GetOptions{}) + require.NoError(t, pvErr) + require.Equal(t, existingPV.Spec.PersistentVolumeReclaimPolicy, gotPV.Spec.PersistentVolumeReclaimPolicy) + require.Empty(t, dataDownloadList.Items) + } else { + require.NoError(t, err) + // The in-place restore proceeded: the existing PVC is deleted + // and a DataDownload is created. + require.True(t, apierrors.IsNotFound(getErr)) + require.Len(t, dataDownloadList.Items, 1) + } + }) + } +} + func TestPVCAppliesTo(t *testing.T) { p := pvcRestoreItemAction{ log: logrus.StandardLogger(), diff --git a/pkg/restore/inplace/preflight.go b/pkg/restore/inplace/preflight.go new file mode 100644 index 000000000..c1a4fa3a4 --- /dev/null +++ b/pkg/restore/inplace/preflight.go @@ -0,0 +1,131 @@ +/* +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 inplace holds the pre-flight checks for in-place volume data +// restores. The checks must pass before Velero performs any side effect on +// the existing PVC/PV. +package inplace + +import ( + "context" + "fmt" + "strings" + + "github.com/cockroachdb/errors" + corev1api "k8s.io/api/core/v1" + "k8s.io/apimachinery/pkg/types" + crclient "sigs.k8s.io/controller-runtime/pkg/client" + + "github.com/vmware-tanzu/velero/pkg/restorehelper" +) + +// CheckPVCNotInUse verifies the target PVC is not used by any active pod, +// aligned with the pvc-protection controller semantics: terminal-phase pods +// (Succeeded/Failed) don't block; all other phases do, and terminating pods +// are flagged so the message can hint the user to wait. +// +// Pods gated by this restore's restore-wait init container are exempted: on +// the file system restore path the restored pods must mount the PVC for the +// node-agent to restore the data, and an RWX PVC may be mounted by several of +// them. Such a pod cannot write to the volume since its workload containers +// are blocked until this restore's PodVolumeRestores complete (see +// gatedByThisRestore). Any other pod, including one gated by a different +// restore whose release timing is out of our control, still blocks. +func CheckPVCNotInUse( + ctx context.Context, + cli crclient.Client, + pvc *corev1api.PersistentVolumeClaim, + restoreUID types.UID, +) error { + 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) + } + + podsInUse := []string{} + terminatingOnly := true + for i := range podList.Items { + pod := &podList.Items[i] + if !podUsesPVC(pod, pvc.Name) || + pod.Status.Phase == corev1api.PodSucceeded || pod.Status.Phase == corev1api.PodFailed || + gatedByThisRestore(pod, restoreUID) { + continue + } + state := string(pod.Status.Phase) + if pod.DeletionTimestamp != nil { + state += ", terminating" + } else { + terminatingOnly = false + } + podsInUse = append(podsInUse, fmt.Sprintf("%s (%s)", pod.Name, state)) + } + if len(podsInUse) == 0 { + return nil + } + + hint := "delete the workloads consuming the PVC and retry" + if terminatingOnly { + hint = "the pod(s) are terminating; retry after they are fully removed" + } + return errors.Errorf("in-place restore pre-flight check failed, skipping volume data restore: PVC %s/%s is still in use by pod(s) [%s]: %s", + pvc.Namespace, pvc.Name, strings.Join(podsInUse, ", "), hint) +} + +func podUsesPVC(pod *corev1api.Pod, pvcName string) bool { + for _, vol := range pod.Spec.Volumes { + if vol.PersistentVolumeClaim != nil && vol.PersistentVolumeClaim.ClaimName == pvcName { + return true + } + } + return false +} + +// gatedByThisRestore reports whether the pod is blocked by the restore-wait +// init container injected by this restore, identified by the restore UID in +// the init container's args. Such a pod cannot write to the volume: its +// workload containers won't start until this restore's PodVolumeRestores +// complete and write the done signal. A terminated init container means the +// gate is already open, so the pod is no longer exempted. Pods gated by a +// different restore are not exempted either, since their release timing is +// unrelated to this restore. +func gatedByThisRestore(pod *corev1api.Pod, restoreUID types.UID) bool { + if restoreUID == "" { + return false + } + + idx := -1 + for i, c := range pod.Spec.InitContainers { + if c.Name == restorehelper.WaitInitContainer { + if len(c.Args) == 0 || c.Args[0] != string(restoreUID) { + return false + } + idx = i + break + } + } + if idx < 0 { + return false + } + + for _, cs := range pod.Status.InitContainerStatuses { + if cs.Name == restorehelper.WaitInitContainer { + return cs.State.Terminated == nil + } + } + // Statuses not populated yet: the init container hasn't run, so the gate + // is still closed. + return true +} diff --git a/pkg/restore/inplace/preflight_test.go b/pkg/restore/inplace/preflight_test.go new file mode 100644 index 000000000..916b786f0 --- /dev/null +++ b/pkg/restore/inplace/preflight_test.go @@ -0,0 +1,202 @@ +/* +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 inplace + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1api "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/types" + + "github.com/vmware-tanzu/velero/pkg/restorehelper" + velerotest "github.com/vmware-tanzu/velero/pkg/test" +) + +func podUsingPVC(name, pvcName string, phase corev1api.PodPhase, terminating bool) *corev1api.Pod { + pod := &corev1api.Pod{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: "default", + UID: types.UID(name + "-uid"), + }, + Spec: corev1api.PodSpec{ + Volumes: []corev1api.Volume{ + { + Name: "data", + VolumeSource: corev1api.VolumeSource{ + PersistentVolumeClaim: &corev1api.PersistentVolumeClaimVolumeSource{ + ClaimName: pvcName, + }, + }, + }, + }, + }, + Status: corev1api.PodStatus{Phase: phase}, + } + if terminating { + now := metav1.Now() + pod.DeletionTimestamp = &now + pod.Finalizers = []string{"fake-finalizer"} + } + return pod +} + +// gatedPod adds the restore-wait init container (as injected by the +// PodVolumeRestoreAction RIA, carrying the restore UID in args) to the pod. +// terminated simulates the gate having already been released. +func gatedPod(pod *corev1api.Pod, restoreUID string, terminated bool) *corev1api.Pod { + pod.Spec.InitContainers = append([]corev1api.Container{{ + Name: restorehelper.WaitInitContainer, + Args: []string{restoreUID}, + }}, pod.Spec.InitContainers...) + status := corev1api.ContainerStatus{ + Name: restorehelper.WaitInitContainer, + State: corev1api.ContainerState{Running: &corev1api.ContainerStateRunning{}}, + } + if terminated { + status.State = corev1api.ContainerState{Terminated: &corev1api.ContainerStateTerminated{}} + } + pod.Status.InitContainerStatuses = []corev1api.ContainerStatus{status} + return pod +} + +func TestCheckPVCNotInUse(t *testing.T) { + tests := []struct { + name string + pods []*corev1api.Pod + restoreUID types.UID + expectPass bool + expectMessage []string + }{ + { + name: "no pods, check passes", + expectPass: true, + }, + { + name: "active pod blocks with the delete hint", + pods: []*corev1api.Pod{podUsingPVC("pod-1", "pvc-1", corev1api.PodRunning, false)}, + expectMessage: []string{"pod-1 (Running)", "delete the workloads"}, + }, + { + name: "unknown-phase pod blocks (node may be unreachable)", + pods: []*corev1api.Pod{podUsingPVC("pod-1", "pvc-1", corev1api.PodUnknown, false)}, + expectMessage: []string{"pod-1 (Unknown)"}, + }, + { + name: "terminating pod blocks with the wait hint", + pods: []*corev1api.Pod{podUsingPVC("pod-1", "pvc-1", corev1api.PodRunning, true)}, + expectMessage: []string{"pod-1 (Running, terminating)", "retry after they are fully removed"}, + }, + { + name: "terminal-phase pods do not block", + pods: []*corev1api.Pod{ + podUsingPVC("pod-1", "pvc-1", corev1api.PodSucceeded, false), + podUsingPVC("pod-2", "pvc-1", corev1api.PodFailed, false), + }, + expectPass: true, + }, + { + name: "pod using another PVC does not block", + pods: []*corev1api.Pod{podUsingPVC("pod-1", "other-pvc", corev1api.PodRunning, false)}, + expectPass: true, + }, + { + name: "pods gated by this restore do not block, other pods still do", + pods: []*corev1api.Pod{ + gatedPod(podUsingPVC("restored-pod-1", "pvc-1", corev1api.PodPending, false), "restore-uid", false), + gatedPod(podUsingPVC("restored-pod-2", "pvc-1", corev1api.PodPending, false), "restore-uid", false), + podUsingPVC("other-pod", "pvc-1", corev1api.PodRunning, false), + }, + restoreUID: "restore-uid", + expectMessage: []string{"[other-pod (Running)]"}, + }, + { + name: "multiple pods gated by this restore pass the check", + pods: []*corev1api.Pod{ + gatedPod(podUsingPVC("restored-pod-1", "pvc-1", corev1api.PodPending, false), "restore-uid", false), + gatedPod(podUsingPVC("restored-pod-2", "pvc-1", corev1api.PodPending, false), "restore-uid", false), + }, + restoreUID: "restore-uid", + expectPass: true, + }, + { + name: "pod gated by a different restore still blocks", + pods: []*corev1api.Pod{ + gatedPod(podUsingPVC("old-restored-pod", "pvc-1", corev1api.PodPending, false), "old-restore-uid", false), + }, + restoreUID: "restore-uid", + expectMessage: []string{"[old-restored-pod (Pending)]"}, + }, + { + name: "pod whose restore-wait already terminated still blocks", + pods: []*corev1api.Pod{ + gatedPod(podUsingPVC("released-pod", "pvc-1", corev1api.PodRunning, false), "restore-uid", true), + }, + restoreUID: "restore-uid", + expectMessage: []string{"[released-pod (Running)]"}, + }, + { + name: "gated pod without init container status yet passes the check", + pods: []*corev1api.Pod{ + func() *corev1api.Pod { + pod := gatedPod(podUsingPVC("new-pod", "pvc-1", corev1api.PodPending, false), "restore-uid", false) + pod.Status.InitContainerStatuses = nil + return pod + }(), + }, + restoreUID: "restore-uid", + expectPass: true, + }, + { + name: "empty restore UID exempts nothing", + pods: []*corev1api.Pod{ + gatedPod(podUsingPVC("restored-pod", "pvc-1", corev1api.PodPending, false), "", false), + }, + restoreUID: "", + expectMessage: []string{"[restored-pod (Pending)]"}, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + objs := []runtime.Object{} + for _, pod := range tc.pods { + objs = append(objs, pod) + } + cli := velerotest.NewFakeControllerRuntimeClient(t, objs...) + + pvc := &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{Name: "pvc-1", Namespace: "default"}, + } + + err := CheckPVCNotInUse(t.Context(), cli, pvc, tc.restoreUID) + if tc.expectPass { + require.NoError(t, err) + return + } + require.Error(t, err) + assert.Contains(t, err.Error(), "pre-flight check failed") + for _, fragment := range tc.expectMessage { + assert.Contains(t, err.Error(), fragment) + } + }) + } +} From e03ff894ffb8179f8e0e95041800a333b97ec420 Mon Sep 17 00:00:00 2001 From: chlins Date: Wed, 2 Sep 2026 15:24:51 +0800 Subject: [PATCH 27/28] Add operation context to user-facing error messages Prefix raw err.Error() strings surfaced in CR statuses and CLI stderr with the failed operation. Signed-off-by: chlins --- changelogs/unreleased/10464-chlins | 1 + pkg/cmd/errors.go | 2 +- pkg/controller/backup_controller.go | 4 ++-- pkg/controller/backup_controller_test.go | 4 ++-- pkg/controller/backup_deletion_controller.go | 14 +++++++------- .../backup_deletion_controller_test.go | 16 ++++++++-------- pkg/controller/pod_volume_backup_controller.go | 5 ++--- pkg/controller/restore_controller.go | 8 ++++---- 8 files changed, 27 insertions(+), 27 deletions(-) create mode 100644 changelogs/unreleased/10464-chlins diff --git a/changelogs/unreleased/10464-chlins b/changelogs/unreleased/10464-chlins new file mode 100644 index 000000000..0a89da583 --- /dev/null +++ b/changelogs/unreleased/10464-chlins @@ -0,0 +1 @@ +Add operation context to user-facing error messages in CR statuses and CLI output diff --git a/pkg/cmd/errors.go b/pkg/cmd/errors.go index 4f374c8af..50c0a8e35 100644 --- a/pkg/cmd/errors.go +++ b/pkg/cmd/errors.go @@ -27,7 +27,7 @@ import ( func CheckError(err error) { if err != nil { if err != context.Canceled { - fmt.Fprintf(os.Stderr, "An error occurred: %v\n", err) + fmt.Fprintf(os.Stderr, "velero: %v\n", err) } os.Exit(1) } diff --git a/pkg/controller/backup_controller.go b/pkg/controller/backup_controller.go index 569ff18d1..f3abcc6f9 100644 --- a/pkg/controller/backup_controller.go +++ b/pkg/controller/backup_controller.go @@ -362,7 +362,7 @@ func (b *backupReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr // result in the backup being Failed. log.WithError(err).Error("backup failed") request.Status.Phase = velerov1api.BackupPhaseFailed - request.Status.FailureReason = err.Error() + request.Status.FailureReason = fmt.Sprintf("backup execution failed: %v", err) } switch request.Status.Phase { @@ -619,7 +619,7 @@ func (b *backupReconciler) prepareBackupRequest(ctx context.Context, backup *vel resourcePolicies, err := resourcepolicies.GetResourcePoliciesFromBackupWithGlobal( *request.Backup, b.kbClient, b.globalVolumePoliciesConfigMap, request.Namespace, logger) if err != nil { - request.Status.ValidationErrors = append(request.Status.ValidationErrors, err.Error()) + request.Status.ValidationErrors = append(request.Status.ValidationErrors, fmt.Sprintf("invalid resource policies: %v", err)) } else if b.globalVolumePoliciesConfigMap != "" { // Record the contributing global volume policies ConfigMap so `velero backup describe` can surface it. request.Annotations[velerov1api.GlobalBackupVolumePolicyConfigMapAnnotation] = b.globalVolumePoliciesConfigMap diff --git a/pkg/controller/backup_controller_test.go b/pkg/controller/backup_controller_test.go index 13bac2e4c..eb7786f63 100644 --- a/pkg/controller/backup_controller_test.go +++ b/pkg/controller/backup_controller_test.go @@ -1205,7 +1205,7 @@ func TestProcessBackupCompletions(t *testing.T) { }, Status: velerov1api.BackupStatus{ Phase: velerov1api.BackupPhaseFailed, - FailureReason: "backup already exists in object storage", + FailureReason: "backup execution failed: backup already exists in object storage", Version: 1, FormatVersion: "1.1.0", StartTimestamp: ×tamp, @@ -1250,7 +1250,7 @@ func TestProcessBackupCompletions(t *testing.T) { }, Status: velerov1api.BackupStatus{ Phase: velerov1api.BackupPhaseFailed, - FailureReason: "error checking if backup already exists in object storage: Backup already exists in object storage", + FailureReason: "backup execution failed: error checking if backup already exists in object storage: Backup already exists in object storage", Version: 1, FormatVersion: "1.1.0", StartTimestamp: ×tamp, diff --git a/pkg/controller/backup_deletion_controller.go b/pkg/controller/backup_deletion_controller.go index 416c28cd4..18a545979 100644 --- a/pkg/controller/backup_deletion_controller.go +++ b/pkg/controller/backup_deletion_controller.go @@ -315,7 +315,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque volumeSnapshotter, ok := volumeSnapshotters[snapshot.Spec.Location] if !ok { if volumeSnapshotter, err = r.volumeSnapshottersForVSL(ctx, backup.Namespace, snapshot.Spec.Location, pluginManager); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrapf(err, "error getting volume snapshotter for location %q", snapshot.Spec.Location).Error()) continue } volumeSnapshotters[snapshot.Spec.Location] = volumeSnapshotter @@ -334,7 +334,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque log.Info("Removing pod volume snapshots") if deleteErrs := r.deletePodVolumeSnapshots(ctx, backup); len(deleteErrs) > 0 { for _, err := range deleteErrs { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error deleting pod volume snapshots").Error()) } } @@ -342,7 +342,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque log.Info("Removing snapshot data by data mover") if deleteErrs := r.deleteMovedSnapshots(ctx, backup); len(deleteErrs) > 0 { for _, err := range deleteErrs { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error deleting moved snapshot data").Error()) } } duList := &velerov2alpha1.DataUploadList{} @@ -354,12 +354,12 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque }), }); err != nil { log.WithError(err).Error("Error listing datauploads") - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error listing datauploads for backup").Error()) } else { for i := range duList.Items { du := duList.Items[i] if err := r.Delete(ctx, &du); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrapf(err, "error deleting dataupload %q", du.Name).Error()) } } } @@ -368,7 +368,7 @@ func (r *backupDeletionReconciler) Reconcile(ctx context.Context, req ctrl.Reque if backupStore != nil && len(errs) == 0 { log.Info("Removing backup from backup storage") if err := backupStore.DeleteBackup(backup.Name); err != nil { - errs = append(errs, err.Error()) + errs = append(errs, errors.Wrap(err, "error removing backup from backup storage").Error()) } } else if len(errs) > 0 { log.Info("Skipping removal of backup from backup storage due to previous errors") @@ -631,7 +631,7 @@ func (r *backupDeletionReconciler) patchDeleteBackupRequest(ctx context.Context, func (r *backupDeletionReconciler) patchDeleteBackupRequestWithError(ctx context.Context, req *velerov1api.DeleteBackupRequest, err error) error { _, err = r.patchDeleteBackupRequest(ctx, req, func(r *velerov1api.DeleteBackupRequest) { r.Status.Phase = velerov1api.DeleteBackupRequestPhaseProcessed - r.Status.Errors = []string{err.Error()} + r.Status.Errors = []string{errors.WithMessage(err, "backup deletion failed").Error()} }) return err } diff --git a/pkg/controller/backup_deletion_controller_test.go b/pkg/controller/backup_deletion_controller_test.go index d358fbe5e..3a0bc2fe5 100644 --- a/pkg/controller/backup_deletion_controller_test.go +++ b/pkg/controller/backup_deletion_controller_test.go @@ -137,7 +137,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { td.fakeClient.Get(ctx, td.req.NamespacedName, res) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.True(t, strings.HasPrefix(res.Status.Errors[0], "error getting the backup store")) + assert.True(t, strings.HasPrefix(res.Status.Errors[0], "backup deletion failed: error getting the backup store")) }) t.Run("missing spec.backupName", func(t *testing.T) { @@ -153,7 +153,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "spec.backupName is required", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: spec.backupName is required", res.Status.Errors[0]) }) t.Run("existing deletion requests for the backup are deleted", func(t *testing.T) { @@ -221,7 +221,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup is still in progress", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup is still in progress", res.Status.Errors[0]) }) t.Run("unable to find backup", func(t *testing.T) { @@ -235,7 +235,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup not found", res.Status.Errors[0]) }) t.Run("unable to find backup storage location", func(t *testing.T) { backup := builder.ForBackup(velerov1api.DefaultNamespace, "foo").StorageLocation("default").Result() @@ -250,7 +250,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup storage location default not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup storage location default not found", res.Status.Errors[0]) }) t.Run("backup storage location is in read-only mode", func(t *testing.T) { @@ -267,7 +267,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "cannot delete backup because backup storage location default is currently in read-only mode", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: cannot delete backup because backup storage location default is currently in read-only mode", res.Status.Errors[0]) }) t.Run("backup storage location is in unavailable state", func(t *testing.T) { @@ -284,7 +284,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "cannot delete backup because backup storage location default is currently in Unavailable state", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: cannot delete backup because backup storage location default is currently in Unavailable state", res.Status.Errors[0]) }) t.Run("full delete, no errors", func(t *testing.T) { @@ -877,7 +877,7 @@ func TestBackupDeletionControllerReconcile(t *testing.T) { require.NoError(t, err) assert.Equal(t, "Processed", string(res.Status.Phase)) assert.Len(t, res.Status.Errors, 1) - assert.Equal(t, "backup not found", res.Status.Errors[0]) + assert.Equal(t, "backup deletion failed: backup not found", res.Status.Errors[0]) }) } diff --git a/pkg/controller/pod_volume_backup_controller.go b/pkg/controller/pod_volume_backup_controller.go index 13dbd5d79..c4e68ce33 100644 --- a/pkg/controller/pod_volume_backup_controller.go +++ b/pkg/controller/pod_volume_backup_controller.go @@ -784,10 +784,9 @@ func UpdatePVBStatusToFailed(ctx context.Context, c client.Client, pvb *velerov1 pvb.Status.SnapshotID = dataPathError.GetSnapshotID() } if len(strings.TrimSpace(msg)) == 0 { - pvb.Status.Message = errOut.Error() - } else { - pvb.Status.Message = errors.WithMessage(errOut, msg).Error() + msg = "pod volume backup failed" } + pvb.Status.Message = errors.WithMessage(errOut, msg).Error() if pvb.Status.StartTimestamp.IsZero() { pvb.Status.StartTimestamp = &metav1.Time{Time: time} } diff --git a/pkg/controller/restore_controller.go b/pkg/controller/restore_controller.go index 69f8636b7..8248ee538 100644 --- a/pkg/controller/restore_controller.go +++ b/pkg/controller/restore_controller.go @@ -275,7 +275,7 @@ func (r *restoreReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ct if err := r.runValidatedRestore(restore, info, resourceModifiers, restoreResPolicies); err != nil { log.WithError(err).Debug("Restore failed") restore.Status.Phase = api.RestorePhaseFailed - restore.Status.FailureReason = err.Error() + restore.Status.FailureReason = fmt.Sprintf("restore execution failed: %v", err) r.metrics.RegisterRestoreFailed(backupScheduleName) } @@ -349,7 +349,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap // validate Restore Init Hook's InitContainers restoreHooks, err := hook.GetRestoreHooksFromSpec(&restore.Spec.Hooks) if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, err.Error()) + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("invalid restore hooks: %v", err)) } for _, resource := range restoreHooks { for _, h := range resource.RestoreHooks { @@ -357,7 +357,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap for _, container := range h.Init.InitContainers { err = hook.ValidateContainer(container.Raw) if err != nil { - restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, err.Error()) + restore.Status.ValidationErrors = append(restore.Status.ValidationErrors, fmt.Sprintf("invalid init container in restore hook %q: %v", resource.Name, err)) } } } @@ -433,7 +433,7 @@ func (r *restoreReconciler) validateAndComplete(ctx context.Context, restore *ap ) if err != nil { restore.Status.ValidationErrors = append( - restore.Status.ValidationErrors, err.Error(), + restore.Status.ValidationErrors, fmt.Sprintf("invalid restore resource policies: %v", err), ) return backupInfo{}, nil, nil } From 85c660612b532b70693f43bce3412802338ee169 Mon Sep 17 00:00:00 2001 From: Adam Zhang Date: Thu, 3 Sep 2026 00:03:06 +0800 Subject: [PATCH 28/28] Enforce resource filters on cluster-wide items (#10455) * Enforce resource filters on cluster-wide items When backups query all namespaces (wildcard or omitted includes), the item collector retrieved resources in bulk, bypassing per-namespace resource filter policies in Stage 1 collection. This caused resources not listed in the policy to be backed up. To preserve cluster-wide query performance while enforcing policy rules, evaluate namespace exclusions, resource kind allowlists, and label selectors in memory for each collected item. Signed-off-by: Adam Zhang * Optimize in-memory resource filter checks Optimize per-item filter evaluation in the item collector: - Precalculate GroupResource string once per resource type - Skip filter policy evaluation when no namespaced policies exist - Restrict in-memory filtering to cluster-wide queries - Cache consecutive namespace lookups across collected items - Lazily extract resource labels only when selectors are present Signed-off-by: Adam Zhang --------- Signed-off-by: Adam Zhang --- changelogs/unreleased/10455-adam-jian-zhang | 1 + pkg/backup/item_collector.go | 62 ++++++- pkg/backup/item_collector_test.go | 187 ++++++++++++++++++++ 3 files changed, 249 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/10455-adam-jian-zhang diff --git a/changelogs/unreleased/10455-adam-jian-zhang b/changelogs/unreleased/10455-adam-jian-zhang new file mode 100644 index 000000000..8c94a2518 --- /dev/null +++ b/changelogs/unreleased/10455-adam-jian-zhang @@ -0,0 +1 @@ +Fix issue 10454, enforce resource filter policies in memory in stage 1 to fix wildcard namespace bypassing issue diff --git a/pkg/backup/item_collector.go b/pkg/backup/item_collector.go index 1733e9ac9..3828fc3a2 100644 --- a/pkg/backup/item_collector.go +++ b/pkg/backup/item_collector.go @@ -478,12 +478,15 @@ func (r *itemCollector) getResourceItems( namespacesToList = []string{""} } + grString := gr.String() + hasNamespacedPolicies := len(r.backupRequest.NamespacedFilterMap) > 0 + var items []*kubernetesResource for _, namespace := range namespacesToList { // Check per-namespace resource type filter from ResourcePolicy if nsFilter := r.backupRequest.GetNamespaceFilter(namespace); nsFilter != nil { - _, hasSpecific := nsFilter.ResourceFilterMap[gr.String()] + _, hasSpecific := nsFilter.ResourceFilterMap[grString] if !hasSpecific && nsFilter.CatchAllFilter == nil { log.Debugf("Skipping resource %s in namespace %s: not in resourceFilters", gr, namespace) @@ -497,9 +500,66 @@ func (r *itemCollector) getResourceItems( continue } + var lastNS string + var lastNSFilter *ResolvedNamespaceFilter + // Collect items in included Namespaces for i := range unstructuredItems { item := &unstructuredItems[i] + itemNS := item.GetNamespace() + + // Apply namespace inclusion/exclusion and fine-grained filter policies in-memory for cluster-wide queries. + if itemNS != "" && namespace == "" { + if r.backupRequest.NamespaceIncludesExcludes != nil && + !r.backupRequest.NamespaceIncludesExcludes.ShouldInclude(itemNS) { + log.Debugf("Skipping resource %s in namespace %s: namespace excluded", + gr, itemNS) + continue + } + + if hasNamespacedPolicies { + if itemNS != lastNS { + lastNS = itemNS + lastNSFilter = r.backupRequest.GetNamespaceFilter(itemNS) + } + + if lastNSFilter != nil { + rf := lastNSFilter.ResourceFilterMap[grString] + if rf == nil { + rf = lastNSFilter.CatchAllFilter + } + if rf == nil { + log.Debugf("Skipping resource %s in namespace %s: not in resourceFilters", + gr, itemNS) + continue + } + + // In-memory label selector checks for fine-grained filters + if rf.LabelSelector != nil || len(rf.OrLabelSelectors) > 0 { + itemLabels := labels.Set(item.GetLabels()) + if rf.LabelSelector != nil && !rf.LabelSelector.Matches(itemLabels) { + log.Debugf("Skipping resource %s in namespace %s: does not match labelSelector", + gr, itemNS) + continue + } + if len(rf.OrLabelSelectors) > 0 { + matched := false + for _, s := range rf.OrLabelSelectors { + if s.Matches(itemLabels) { + matched = true + break + } + } + if !matched { + log.Debugf("Skipping resource %s in namespace %s: does not match orLabelSelectors", + gr, itemNS) + continue + } + } + } + } + } + } path, err := r.writeToFile(item) if err != nil { diff --git a/pkg/backup/item_collector_test.go b/pkg/backup/item_collector_test.go index 47a1d7be5..39e57621a 100644 --- a/pkg/backup/item_collector_test.go +++ b/pkg/backup/item_collector_test.go @@ -466,3 +466,190 @@ func TestGetOrderedResourcesForTypeTrimsSpaces(t *testing.T) { {namespace: "ns1", name: "pod3"}, }, sorted) } + +func TestGetResourceItems_NamespacedFilterPolicies_ClusterWideListing(t *testing.T) { + // Simulate cluster-wide API calls where client is called with namespace="" + prodSA := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]any{ + "name": "default", + "namespace": "production", + }, + }, + } + defaultSA := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]any{ + "name": "default", + "namespace": "default", + }, + }, + } + kubeSystemSA := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]any{ + "name": "default", + "namespace": "kube-system", + }, + }, + } + + prodCM1 := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "app-config", + "namespace": "production", + "labels": map[string]any{ + "app": "frontend", + }, + }, + }, + } + prodCM2 := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "other-config", + "namespace": "production", + "labels": map[string]any{ + "app": "backend", + }, + }, + }, + } + defaultCM := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ConfigMap", + "metadata": map[string]any{ + "name": "my-config", + "namespace": "default", + }, + }, + } + + dcClusterWideSA := &test.FakeDynamicClient{} + dcClusterWideSA.On("List", mock.Anything).Return(&unstructured.UnstructuredList{Items: []unstructured.Unstructured{prodSA, defaultSA, kubeSystemSA}}, nil) + + dcClusterWideCM := &test.FakeDynamicClient{} + dcClusterWideCM.On("List", mock.Anything).Return(&unstructured.UnstructuredList{Items: []unstructured.Unstructured{prodCM1, prodCM2, defaultCM}}, nil) + + factory := &test.FakeDynamicFactory{} + factory.On("ClientForGroupVersionResource", schema.GroupVersion{Version: "v1"}, metav1.APIResource{Name: "serviceaccounts", Namespaced: true, Kind: "ServiceAccount"}, "").Return(dcClusterWideSA, nil) + factory.On("ClientForGroupVersionResource", schema.GroupVersion{Version: "v1"}, metav1.APIResource{Name: "configmaps", Namespaced: true, Kind: "ConfigMap"}, "").Return(dcClusterWideCM, nil) + + frontendSelector, err := metav1.LabelSelectorAsSelector(&metav1.LabelSelector{ + MatchLabels: map[string]string{"app": "frontend"}, + }) + require.NoError(t, err) + + req := &Request{ + Backup: builder.ForBackup("velero", "backup").Result(), + NamespaceIncludesExcludes: collections.NewNamespaceIncludesExcludes(). + Includes("*"). + Excludes("kube-system"), + NamespacedFilterMap: map[string]*ResolvedNamespaceFilter{ + "production": { + ResourceFilterMap: map[string]*ResolvedResourceFilter{ + "configmaps": { + LabelSelector: frontendSelector, + }, + }, + }, + }, + ResourceIncludesExcludes: includeAllIE{}, + } + + tempDir := t.TempDir() + r := &itemCollector{ + backupRequest: req, + dynamicFactory: factory, + discoveryHelper: test.NewFakeDiscoveryHelper(true, nil), + log: test.NewLogger(), + dir: tempDir, + } + + // 1. ServiceAccounts: + // - production should be skipped because ServiceAccount is not in production's resourceFilters + // - kube-system should be skipped because kube-system is in excludedNamespaces + // - default should be included + saResource := metav1.APIResource{Name: "serviceaccounts", Namespaced: true, Kind: "ServiceAccount"} + saItems, err := r.getResourceItems(test.NewLogger(), schema.GroupVersion{Version: "v1"}, saResource, nil) + require.NoError(t, err) + require.Len(t, saItems, 1) + assert.Equal(t, "default", saItems[0].namespace) + assert.Equal(t, "default", saItems[0].name) + + // 2. ConfigMaps: + // - production/app-config matches label app=frontend and should be included + // - production/other-config has label app=backend and should be skipped by labelSelector + // - default/my-config has no filter policy and should be included + cmResource := metav1.APIResource{Name: "configmaps", Namespaced: true, Kind: "ConfigMap"} + cmItems, err := r.getResourceItems(test.NewLogger(), schema.GroupVersion{Version: "v1"}, cmResource, nil) + require.NoError(t, err) + require.Len(t, cmItems, 2) + var cmNames []string + for _, it := range cmItems { + cmNames = append(cmNames, it.namespace+"/"+it.name) + } + assert.ElementsMatch(t, []string{"production/app-config", "default/my-config"}, cmNames) +} + +func TestGetResourceItems_NamespacedFilterPolicies_SpecificNamespaces(t *testing.T) { + defaultSA := unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": "v1", + "kind": "ServiceAccount", + "metadata": map[string]any{ + "name": "default", + "namespace": "default", + }, + }, + } + + dcDefaultSA := &test.FakeDynamicClient{} + dcDefaultSA.On("List", mock.Anything).Return(&unstructured.UnstructuredList{Items: []unstructured.Unstructured{defaultSA}}, nil) + + factory := &test.FakeDynamicFactory{} + // Note: production client is never even requested because production skips ServiceAccount at the loop top! + factory.On("ClientForGroupVersionResource", schema.GroupVersion{Version: "v1"}, metav1.APIResource{Name: "serviceaccounts", Namespaced: true, Kind: "ServiceAccount"}, "default").Return(dcDefaultSA, nil) + + req := &Request{ + Backup: builder.ForBackup("velero", "backup").Result(), + NamespaceIncludesExcludes: collections.NewNamespaceIncludesExcludes(). + Includes("production", "default"), + NamespacedFilterMap: map[string]*ResolvedNamespaceFilter{ + "production": { + ResourceFilterMap: map[string]*ResolvedResourceFilter{ + "configmaps": {}, + }, + }, + }, + ResourceIncludesExcludes: includeAllIE{}, + } + + tempDir := t.TempDir() + r := &itemCollector{ + backupRequest: req, + dynamicFactory: factory, + discoveryHelper: test.NewFakeDiscoveryHelper(true, nil), + log: test.NewLogger(), + dir: tempDir, + } + + saResource := metav1.APIResource{Name: "serviceaccounts", Namespaced: true, Kind: "ServiceAccount"} + saItems, err := r.getResourceItems(test.NewLogger(), schema.GroupVersion{Version: "v1"}, saResource, nil) + require.NoError(t, err) + require.Len(t, saItems, 1) + assert.Equal(t, "default", saItems[0].namespace) + assert.Equal(t, "default", saItems[0].name) +}