From a71bc4befc62d540aa278ef5c92735b5c26359ee Mon Sep 17 00:00:00 2001 From: Xun Jiang Date: Wed, 16 Sep 2026 18:04:22 +0800 Subject: [PATCH] Reset the PVC binding information in pvc_action.go when it has referenced PVB. Signed-off-by: Xun Jiang --- changelogs/unreleased/10538-blackpiglet | 1 + pkg/cmd/server/plugin/plugin.go | 5 +- pkg/restore/actions/pvc_action.go | 76 +++++++--- pkg/restore/actions/pvc_action_test.go | 180 ++++++++++++++++++++++-- pkg/restore/restore_test.go | 5 +- 5 files changed, 234 insertions(+), 33 deletions(-) create mode 100644 changelogs/unreleased/10538-blackpiglet diff --git a/changelogs/unreleased/10538-blackpiglet b/changelogs/unreleased/10538-blackpiglet new file mode 100644 index 000000000..5c189b629 --- /dev/null +++ b/changelogs/unreleased/10538-blackpiglet @@ -0,0 +1 @@ +Reset the PVC binding information in pvc_action.go when it references PVB. \ No newline at end of file diff --git a/pkg/cmd/server/plugin/plugin.go b/pkg/cmd/server/plugin/plugin.go index 071f0bbe5..e5d5f9b99 100644 --- a/pkg/cmd/server/plugin/plugin.go +++ b/pkg/cmd/server/plugin/plugin.go @@ -346,15 +346,14 @@ func newClusterRoleBindingItemAction(logger logrus.FieldLogger) (any, error) { func newPVCRestoreItemAction(f client.Factory) plugincommon.HandlerInitializer { return func(logger logrus.FieldLogger) (any, error) { - client, err := f.KubeClient() + crClient, err := f.KubebuilderClient() if err != nil { return nil, err } return ria.NewPVCAction( logger, - client.CoreV1().ConfigMaps(f.Namespace()), - client.CoreV1().Nodes(), + crClient, ), nil } } diff --git a/pkg/restore/actions/pvc_action.go b/pkg/restore/actions/pvc_action.go index b9422d20f..c1d5fb005 100644 --- a/pkg/restore/actions/pvc_action.go +++ b/pkg/restore/actions/pvc_action.go @@ -17,16 +17,21 @@ limitations under the License. package actions import ( + "context" + "github.com/cockroachdb/errors" "github.com/sirupsen/logrus" corev1api "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "k8s.io/apimachinery/pkg/labels" "k8s.io/apimachinery/pkg/runtime" - corev1client "k8s.io/client-go/kubernetes/typed/core/v1" + crclient "sigs.k8s.io/controller-runtime/pkg/client" velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/kuberesource" + "github.com/vmware-tanzu/velero/pkg/label" "github.com/vmware-tanzu/velero/pkg/plugin/velero" + "github.com/vmware-tanzu/velero/pkg/podvolume/configs" "github.com/vmware-tanzu/velero/pkg/util" ) @@ -38,24 +43,21 @@ const ( AnnSelectedNode = "volume.kubernetes.io/selected-node" ) -// PVCAction updates/reset PVC's node selector -// if a mapping is found in the plugin's config map. +// PVCAction removes the Velero-backup related annotations and auto generated binding annotations. +// It also resets the PVC's bound info if it has a referenced PodVolumeBackup. type PVCAction struct { - logger logrus.FieldLogger - configMapClient corev1client.ConfigMapInterface - nodeClient corev1client.NodeInterface + logger logrus.FieldLogger + crClient crclient.Client } // NewPVCAction is the constructor for PVCAction. func NewPVCAction( logger logrus.FieldLogger, - configMapClient corev1client.ConfigMapInterface, - nodeClient corev1client.NodeInterface, + crClient crclient.Client, ) *PVCAction { return &PVCAction{ - logger: logger, - configMapClient: configMapClient, - nodeClient: nodeClient, + logger: logger, + crClient: crClient, } } @@ -67,11 +69,9 @@ func (p *PVCAction) AppliesTo() (velero.ResourceSelector, error) { } // PVC actions for restore: -// 1. updates the pvc's selected-node annotation: -// a) if node mapping found in the config map for the plugin -// b) if node mentioned in annotation doesn't exist -// 2. removes some additional annotations -// 3. returns bound PV as an additional item +// 1. removes some additional annotations +// 2. returns bound PV as an additional item +// 3. resets bound if PVC references a PVB func (p *PVCAction) Execute(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { p.logger.Info("Executing PVCAction") defer p.logger.Info("Done executing PVCAction") @@ -106,6 +106,18 @@ func (p *PVCAction) Execute(input *velero.RestoreItemActionExecuteInput) (*veler }, ) + hasPVB, err := p.hasPodVolumeBackup(context.Background(), input.Restore, &pvcFromBackup) + if err != nil { + return nil, errors.WithStack(err) + } + + if hasPVB { + log.Info("PVC has a matching PodVolumeBackup, resetting its volume name") + pvc.Spec.VolumeName = "" + pvc.Spec.DataSource = nil + pvc.Spec.DataSourceRef = nil + } + pvcMap, err := runtime.DefaultUnstructuredConverter.ToUnstructured(&pvc) if err != nil { return nil, errors.WithStack(err) @@ -118,6 +130,8 @@ func (p *PVCAction) Execute(input *velero.RestoreItemActionExecuteInput) (*veler // use pvcFromBackup because we need to look at status fields, which have been removed from pvc if pvcFromBackup.Status.Phase != corev1api.ClaimBound || pvcFromBackup.Spec.VolumeName == "" { log.Info("PVC is not bound or its volume name is empty") + } else if hasPVB { + log.Info("PVC has a matching PodVolumeBackup, skipping PV inclusion") } else { log.Infof("Adding PV %s as an additional item to restore", pvcFromBackup.Spec.VolumeName) output.AdditionalItems = []velero.ResourceIdentifier{ @@ -130,6 +144,36 @@ func (p *PVCAction) Execute(input *velero.RestoreItemActionExecuteInput) (*veler return output, nil } +func (p *PVCAction) hasPodVolumeBackup(ctx context.Context, restore *velerov1api.Restore, pvc *corev1api.PersistentVolumeClaim) (bool, error) { + if p.crClient == nil || restore == nil || pvc == nil { + return false, nil + } + + opts := &crclient.ListOptions{ + LabelSelector: labels.SelectorFromSet(map[string]string{ + velerov1api.BackupNameLabel: label.GetValidName(restore.Spec.BackupName), + velerov1api.PVCUIDLabel: string(pvc.UID), + }), + Namespace: restore.Namespace, + } + podVolumeBackupList := new(velerov1api.PodVolumeBackupList) + if err := p.crClient.List(ctx, podVolumeBackupList, opts); err != nil { + return false, errors.WithStack(err) + } + + var found bool + for _, pvb := range podVolumeBackupList.Items { + if pvb.Status.Phase != velerov1api.PodVolumeBackupPhaseCompleted || pvb.Status.SnapshotID == "" { + continue + } + if pvb.Spec.Pod.Namespace == pvc.Namespace && pvb.GetAnnotations()[configs.PVCNameAnnotation] == pvc.Name { + found = true + break + } + } + return found, nil +} + func removePVCAnnotations(pvc *corev1api.PersistentVolumeClaim, remove []string) { for k := range pvc.Annotations { if util.Contains(remove, k) { diff --git a/pkg/restore/actions/pvc_action_test.go b/pkg/restore/actions/pvc_action_test.go index aa2ddeeef..3363b110c 100644 --- a/pkg/restore/actions/pvc_action_test.go +++ b/pkg/restore/actions/pvc_action_test.go @@ -26,8 +26,8 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" - "k8s.io/client-go/kubernetes/fake" + velerov1api "github.com/vmware-tanzu/velero/pkg/apis/velero/v1" "github.com/vmware-tanzu/velero/pkg/builder" "github.com/vmware-tanzu/velero/pkg/kuberesource" "github.com/vmware-tanzu/velero/pkg/plugin/velero" @@ -82,12 +82,9 @@ func TestPVCActionExecute(t *testing.T) { for _, tc := range tests { t.Run(tc.name, func(t *testing.T) { - clientset := fake.NewSimpleClientset() - a := NewPVCAction( velerotest.NewLogger(), - clientset.CoreV1().ConfigMaps("velero"), - clientset.CoreV1().Nodes(), + nil, ) // set up test data @@ -129,10 +126,17 @@ func TestAddPVFromPVCActionExecute(t *testing.T) { name string itemFromBackup *corev1api.PersistentVolumeClaim want []velero.ResourceIdentifier + pvbs []runtime.Object + wantVolumeName string }{ { name: "bound PVC with volume name returns associated PV", itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, Spec: corev1api.PersistentVolumeClaimSpec{ VolumeName: "bound-pv", }, @@ -146,10 +150,16 @@ func TestAddPVFromPVCActionExecute(t *testing.T) { Name: "bound-pv", }, }, + wantVolumeName: "bound-pv", }, { name: "unbound PVC with volume name does not return any additional items", itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, Spec: corev1api.PersistentVolumeClaimSpec{ VolumeName: "pending-pv", }, @@ -157,17 +167,154 @@ func TestAddPVFromPVCActionExecute(t *testing.T) { Phase: corev1api.ClaimPending, }, }, - want: nil, + want: nil, + wantVolumeName: "pending-pv", }, { name: "bound PVC without volume name does not return any additional items", itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, Spec: corev1api.PersistentVolumeClaimSpec{}, Status: corev1api.PersistentVolumeClaimStatus{ Phase: corev1api.ClaimBound, }, }, - want: nil, + want: nil, + wantVolumeName: "", + }, + { + name: "bound PVC with volume name and matching PVB resets volume name and does not return additional items", + itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "bound-pv", + DataSource: &corev1api.TypedLocalObjectReference{ + Name: "some-ds", + }, + DataSourceRef: &corev1api.TypedObjectReference{ + Name: "some-ds", + }, + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, + pvbs: []runtime.Object{ + builder.ForPodVolumeBackup("ns-1", "pvb-1"). + PodNamespace("ns-1"). + Phase(velerov1api.PodVolumeBackupPhaseCompleted). + SnapshotID("snap-1"). + ObjectMeta(builder.WithLabels( + velerov1api.BackupNameLabel, "backup-1", + velerov1api.PVCUIDLabel, "uid-1", + ), builder.WithAnnotations("velero.io/pvc-name", "pvc-1")).Result(), + }, + want: nil, + wantVolumeName: "", + }, + { + name: "bound PVC with volume name and PVB missing SnapshotID does not reset volume name and returns additional items", + itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "bound-pv", + DataSource: &corev1api.TypedLocalObjectReference{ + Name: "some-ds", + }, + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, + pvbs: []runtime.Object{ + builder.ForPodVolumeBackup("ns-1", "pvb-1"). + PodNamespace("ns-1"). + Phase(velerov1api.PodVolumeBackupPhaseCompleted). + SnapshotID(""). + ObjectMeta(builder.WithLabels( + velerov1api.BackupNameLabel, "backup-1", + velerov1api.PVCUIDLabel, "uid-1", + ), builder.WithAnnotations("velero.io/pvc-name", "pvc-1")).Result(), + }, + want: []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.PersistentVolumes, + Name: "bound-pv", + }, + }, + wantVolumeName: "bound-pv", + }, + { + name: "bound PVC with volume name and PVB with phase Failed does not reset volume name and returns additional items", + itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "bound-pv", + DataSource: &corev1api.TypedLocalObjectReference{ + Name: "some-ds", + }, + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, + pvbs: []runtime.Object{ + builder.ForPodVolumeBackup("ns-1", "pvb-1"). + PodNamespace("ns-1"). + Phase(velerov1api.PodVolumeBackupPhaseFailed). + SnapshotID("snap-1"). + ObjectMeta(builder.WithLabels( + velerov1api.BackupNameLabel, "backup-1", + velerov1api.PVCUIDLabel, "uid-1", + ), builder.WithAnnotations("velero.io/pvc-name", "pvc-1")).Result(), + }, + want: []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.PersistentVolumes, + Name: "bound-pv", + }, + }, + wantVolumeName: "bound-pv", + }, + { + name: "defensive nil input for restore does not panic when crClient != nil", + itemFromBackup: &corev1api.PersistentVolumeClaim{ + ObjectMeta: metav1.ObjectMeta{ + Name: "pvc-1", + Namespace: "ns-1", + UID: "uid-1", + }, + Spec: corev1api.PersistentVolumeClaimSpec{ + VolumeName: "bound-pv", + }, + Status: corev1api.PersistentVolumeClaimStatus{ + Phase: corev1api.ClaimBound, + }, + }, + pvbs: []runtime.Object{}, + want: []velero.ResourceIdentifier{ + { + GroupResource: kuberesource.PersistentVolumes, + Name: "bound-pv", + }, + }, + wantVolumeName: "bound-pv", }, } @@ -181,22 +328,35 @@ func TestAddPVFromPVCActionExecute(t *testing.T) { // item should have no status delete(itemData, "status") - clientset := fake.NewSimpleClientset() + crClient := velerotest.NewFakeControllerRuntimeClient(t, test.pvbs...) action := NewPVCAction( velerotest.NewLogger(), - clientset.CoreV1().ConfigMaps("velero"), - clientset.CoreV1().Nodes(), + crClient, ) + restoreObj := builder.ForRestore("ns-1", "restore-1").Backup("backup-1").Result() + if test.name == "defensive nil input for restore does not panic when crClient != nil" { + restoreObj = nil + } input := &velero.RestoreItemActionExecuteInput{ Item: &unstructured.Unstructured{Object: itemData}, ItemFromBackup: &unstructured.Unstructured{Object: itemFromBackupData}, + Restore: restoreObj, } res, err := action.Execute(input) require.NoError(t, err) assert.Equal(t, test.want, res.AdditionalItems) + + var updatedPVC corev1api.PersistentVolumeClaim + err = runtime.DefaultUnstructuredConverter.FromUnstructured(res.UpdatedItem.UnstructuredContent(), &updatedPVC) + require.NoError(t, err) + assert.Equal(t, test.wantVolumeName, updatedPVC.Spec.VolumeName) + if test.wantVolumeName == "" && len(test.pvbs) > 0 { + assert.Nil(t, updatedPVC.Spec.DataSource) + assert.Nil(t, updatedPVC.Spec.DataSourceRef) + } }) } } diff --git a/pkg/restore/restore_test.go b/pkg/restore/restore_test.go index e582330ed..46436ce78 100644 --- a/pkg/restore/restore_test.go +++ b/pkg/restore/restore_test.go @@ -44,7 +44,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/client-go/dynamic" - k8sfake "k8s.io/client-go/kubernetes/fake" kubetesting "k8s.io/client-go/testing" "github.com/vmware-tanzu/velero/internal/volume" @@ -2984,11 +2983,9 @@ func TestRestoreInplaceSelectedNodeCarrierAnnotation(t *testing.T) { // action proves the carrier survives the real strip regardless of action order. &pluggableAction{ executeFunc: func(input *velero.RestoreItemActionExecuteInput) (*velero.RestoreItemActionExecuteOutput, error) { - clientset := k8sfake.NewSimpleClientset() return riav1.NewPVCAction( h.log, - clientset.CoreV1().ConfigMaps("velero"), - clientset.CoreV1().Nodes(), + nil, ).Execute(input) }, },