From 2826b98190ec16cf7bfc7e153bb61e8dff23644b Mon Sep 17 00:00:00 2001 From: Joseph Antony Vaikath Date: Wed, 17 Jun 2026 08:30:33 -0400 Subject: [PATCH] Fix restore finalization overwriting dynamically provisioned PV labels (#9903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * Merge backup PV labels instead of wholesale replacement during restore finalization Change patchDynamicPVWithVolumeInfo to merge backup labels into the dynamically provisioned PV rather than overwriting its entire label map. Labels already present on the new PV (e.g. topology labels set by the provisioner) are preserved, and only missing labels from the backup are added. This prevents stale topology labels from the source cluster from overwriting correct values set by the target cluster's provisioner. Update needPatch to only trigger when backup labels are absent from the new PV, not when values differ — since differing values now intentionally favour the dynamically provisioned PV. Signed-off-by: Joseph Signed-off-by: Joseph * Add changelog Signed-off-by: Joseph * Update changelog to reflect new approach Signed-off-by: Joseph Signed-off-by: Joseph * Trigger CI rebuild Signed-off-by: Joseph Signed-off-by: Joseph --------- Signed-off-by: Joseph Signed-off-by: Joseph --- changelogs/unreleased/9903-Joeavaikath | 1 + .../restore_finalizer_controller.go | 16 ++-- .../restore_finalizer_controller_test.go | 81 +++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 changelogs/unreleased/9903-Joeavaikath diff --git a/changelogs/unreleased/9903-Joeavaikath b/changelogs/unreleased/9903-Joeavaikath new file mode 100644 index 000000000..d586acfaa --- /dev/null +++ b/changelogs/unreleased/9903-Joeavaikath @@ -0,0 +1 @@ +Fix restore finalization overwriting dynamically provisioned PV labels with stale backup values diff --git a/pkg/controller/restore_finalizer_controller.go b/pkg/controller/restore_finalizer_controller.go index 20e4bc849..4e02bb0ef 100644 --- a/pkg/controller/restore_finalizer_controller.go +++ b/pkg/controller/restore_finalizer_controller.go @@ -421,7 +421,16 @@ func (ctx *finalizerContext) patchDynamicPVWithVolumeInfo() (errs results.Result // patch PV's reclaim policy and label using the corresponding data stored in volume info if needPatch(pv, volInfo.PVInfo) { updatedPV := pv.DeepCopy() - updatedPV.Labels = volInfo.PVInfo.Labels + + if updatedPV.Labels == nil { + updatedPV.Labels = make(map[string]string) + } + for k, v := range volInfo.PVInfo.Labels { + if _, exists := updatedPV.Labels[k]; !exists { + updatedPV.Labels[k] = v + } + } + updatedPV.Spec.PersistentVolumeReclaimPolicy = corev1api.PersistentVolumeReclaimPolicy(volInfo.PVInfo.ReclaimPolicy) if err := kubeutil.PatchResource(pv, updatedPV, ctx.crClient); err != nil { return false, err @@ -553,13 +562,10 @@ func needPatch(newPV *corev1api.PersistentVolume, pvInfo *volume.PVInfo) bool { } newPVLabels, pvLabels := newPV.Labels, pvInfo.Labels - for k, v := range pvLabels { + for k := range pvLabels { if _, ok := newPVLabels[k]; !ok { return true } - if newPVLabels[k] != v { - return true - } } return false diff --git a/pkg/controller/restore_finalizer_controller_test.go b/pkg/controller/restore_finalizer_controller_test.go index f07d2576c..8f2618f9d 100644 --- a/pkg/controller/restore_finalizer_controller_test.go +++ b/pkg/controller/restore_finalizer_controller_test.go @@ -634,6 +634,87 @@ func Test_restoreFinalizerReconciler_finishProcessing(t *testing.T) { } } +func TestNeedPatch(t *testing.T) { + tests := []struct { + name string + newPV *corev1api.PersistentVolume + pvInfo *volume.PVInfo + expected bool + }{ + { + name: "reclaim policy differs", + newPV: builder.ForPersistentVolume("pv1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimRetain), + Labels: map[string]string{}, + }, + expected: true, + }, + { + name: "backup has label new PV does not", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("existing", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"existing": "val", "missing": "val"}, + }, + expected: true, + }, + { + name: "same labels same values", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("key", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"key": "val"}, + }, + expected: false, + }, + { + name: "same label key different values", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("topology.kubernetes.io/zone", "us-west-2a")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{"topology.kubernetes.io/zone": "us-east-1a"}, + }, + expected: false, + }, + { + name: "new PV has labels backup does not", + newPV: builder.ForPersistentVolume("pv1"). + ObjectMeta(builder.WithLabels("provisioner-label", "val")). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: map[string]string{}, + }, + expected: false, + }, + { + name: "both labels nil", + newPV: builder.ForPersistentVolume("pv1"). + ReclaimPolicy(corev1api.PersistentVolumeReclaimDelete).Result(), + pvInfo: &volume.PVInfo{ + ReclaimPolicy: string(corev1api.PersistentVolumeReclaimDelete), + Labels: nil, + }, + expected: false, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + result := needPatch(tc.newPV, tc.pvInfo) + assert.Equal(t, tc.expected, result) + }) + } +} + func TestRestoreOperationList(t *testing.T) { var empty []*itemoperation.RestoreOperation tests := []struct {