import-populator: update labels before annotations on succeeded PVC (#4174)

When the import populator copies PVC Prime metadata to the target PVC,
labels must land before annotations. The AnnPodPhase=Succeeded annotation
triggers downstream controllers (DV, DIC) that snapshot the PVC, so
instancetype labels need to already be present at that point.

Signed-off-by: Alex Kalenyuk <akalenyu@redhat.com>
Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>

Author: akalenyu

pkg/controller/populators/BUILD.bazel

@@ -85,6 +85,7 @@ go_test(
         "//vendor/k8s.io/utils/ptr:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/client:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/client/fake:go_default_library",
+        "//vendor/sigs.k8s.io/controller-runtime/pkg/client/interceptor:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/log:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/log/zap:go_default_library",
         "//vendor/sigs.k8s.io/controller-runtime/pkg/reconcile:go_default_library",

pkg/controller/populators/import-populator.go

@@ -195,11 +195,12 @@ func (r *ImportPopulatorReconciler) reconcileTargetPVC(pvc, pvcPrime *corev1.Per
 		}
 
 		if cc.IsPVCComplete(pvcPrime) && cc.IsUnbound(pvc) {
-			// Once the import is succeeded, we copy annotations and labels and rebind the PV from PVC to target PVC
-			if pvcCopy, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil {
+			// Labels must be updated before annotations so that instancetype labels
+			// are present on the PVC before AnnPodPhase=Succeeded triggers downstream controllers.
+			if pvcCopy, err = r.updatePVCWithPVCPrimeLabels(pvcCopy, pvcPrime.GetLabels()); err != nil {
 				return reconcile.Result{}, err
 			}
-			if pvcCopy, err = r.updatePVCWithPVCPrimeLabels(pvcCopy, pvcPrime.GetLabels()); err != nil {
+			if pvcCopy, err = r.updatePVCWithPVCPrimeAnnotations(pvcCopy, pvcPrime, r.updateImportAnnotations); err != nil {
 				return reconcile.Result{}, err
 			}
 			if err := cc.Rebind(context.TODO(), r.client, pvcPrime, pvcCopy); err != nil {

pkg/controller/populators/import-populator_test.go

@@ -22,6 +22,7 @@ import (
 	"net/http"
 	"net/http/httptest"
 	"net/url"
+	"reflect"
 	"strconv"
 	"strings"
 	"time"
@@ -42,7 +43,9 @@ import (
 	"k8s.io/client-go/tools/record"
 	"k8s.io/utils/ptr"
 
+	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
+	"sigs.k8s.io/controller-runtime/pkg/client/interceptor"
 	logf "sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 
@@ -459,6 +462,27 @@ var _ = Describe("Import populator tests", func() {
 
 			By("Reconcile")
 			reconciler = createImportPopulatorReconciler(targetPvc, pvcPrime, pv, volumeImportSource, sc)
+			var operations []string
+			reconciler.client = interceptor.NewClient(reconciler.client.(client.WithWatch), interceptor.Funcs{
+				Update: func(ctx context.Context, c client.WithWatch, obj client.Object, opts ...client.UpdateOption) error {
+					pvc, ok := obj.(*corev1.PersistentVolumeClaim)
+					if !ok {
+						return c.Update(ctx, obj, opts...)
+					}
+					old := &corev1.PersistentVolumeClaim{}
+					if err := c.Get(ctx, types.NamespacedName{Name: pvc.Name, Namespace: pvc.Namespace}, old); err == nil {
+						if !reflect.DeepEqual(old.Labels, pvc.Labels) {
+							operations = append(operations, "labels")
+						}
+						oldPhase, currentPhase := old.Annotations[AnnPodPhase], pvc.Annotations[AnnPodPhase]
+						if currentPhase == string(corev1.PodSucceeded) && oldPhase != currentPhase {
+							operations = append(operations, "phase")
+						}
+					}
+					return c.Update(ctx, obj, opts...)
+				},
+			})
+
 			result, err := reconciler.Reconcile(context.TODO(), reconcile.Request{NamespacedName: types.NamespacedName{Name: targetPvcName, Namespace: metav1.NamespaceDefault}})
 			Expect(err).To(Not(HaveOccurred()))
 			Expect(result).ToNot(BeNil())
@@ -472,6 +496,9 @@ var _ = Describe("Import populator tests", func() {
 			Expect(updatedPVC.Labels).To(HaveKeyWithValue(testInstancetypeKubevirtIoKey, testInstancetypeKubevirtIoValue))
 			Expect(updatedPVC.Labels).To(HaveKeyWithValue(testKubevirtIoKeyExisting, testKubevirtIoValueExisting))
 			Expect(updatedPVC.Labels).ToNot(HaveKey(testUndesiredKey))
+
+			By("Verify labels are updated before AnnPodPhase=Succeeded and phase is set once")
+			Expect(operations).To(Equal([]string{"labels", "phase"}))
 		})
 
 		It("Should set multistage migration annotations on PVC prime", func() {
@@ -707,7 +734,6 @@ func createImportPopulatorReconcilerWithoutConfig(objects ...runtime.Object) *Im
 	for _, ia := range getIndexArgs() {
 		builder = builder.WithIndex(ia.obj, ia.field, ia.extractValue)
 	}
-
 	cl := builder.Build()
 
 	rec := record.NewFakeRecorder(10)