Define behavior of retention policy in multi-namespace plans.

api/migrationcontroller/v1alpha1/multinamespacevmstoragemigrationplan_types.go

@@ -28,6 +28,7 @@ const (
 )
 
 // MultiNamespaceVirtualMachineStorageMigrationPlanSpec defines the desired state of MultiNamespaceVirtualMachineStorageMigrationPlan
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)", message="retentionPolicy cannot be removed once set"
 type MultiNamespaceVirtualMachineStorageMigrationPlanSpec struct {
 	// +patchStrategy=merge
 	// +patchMergeKey=name
@@ -37,7 +38,7 @@ type MultiNamespaceVirtualMachineStorageMigrationPlanSpec struct {
 	Namespaces []VirtualMachineStorageMigrationPlanNamespaceSpec `json:"namespaces"`
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=keepSource
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="retentionPolicy is immutable"
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="retentionPolicy value cannot be changed once set"
 	// RetentionPolicy indicates whether to keep or delete the source DataVolume/PVC after each VM migration completes
 	// in each created namespace plan. When set to "deleteSource", every created VirtualMachineStorageMigrationPlan
 	// will have retentionPolicy set to deleteSource. When "keepSource" or unset, child plans keep their per-namespace

api/migrationcontroller/v1alpha1/virtualmachinestoragemigrationplan_types.go

@@ -37,12 +37,13 @@ const (
 )
 
 // VirtualMachineStorageMigrationPlanSpec defines the desired state of VirtualMachineStorageMigrationPlan
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)", message="retentionPolicy cannot be removed once set"
 type VirtualMachineStorageMigrationPlanSpec struct {
 	// The virtual machines to migrate.
 	VirtualMachines []VirtualMachineStorageMigrationPlanVirtualMachine `json:"virtualMachines"`
 	// +kubebuilder:validation:Optional
 	// +kubebuilder:default=keepSource
-	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="retentionPolicy is immutable"
+	// +kubebuilder:validation:XValidation:rule="self == oldSelf", message="retentionPolicy value cannot be changed once set"
 	// RetentionPolicy indicates whether to keep or delete the source DataVolume/PVC after each VM migration completes.
 	// When "keepSource" (default), the source is preserved. When "deleteSource", the source DataVolume is deleted
 	// if it exists, otherwise the source PVC is deleted.

config/crd/bases/migrations.kubevirt.io_multinamespacevirtualmachinestoragemigrationplans.yaml

@@ -63,7 +63,7 @@ spec:
                       - deleteSource
                       type: string
                       x-kubernetes-validations:
-                      - message: retentionPolicy is immutable
+                      - message: retentionPolicy value cannot be changed once set
                         rule: self == oldSelf
                     virtualMachines:
                       description: The virtual machines to migrate.
@@ -146,6 +146,9 @@ spec:
                   - name
                   - virtualMachines
                   type: object
+                  x-kubernetes-validations:
+                  - message: retentionPolicy cannot be removed once set
+                    rule: '!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)'
                 type: array
                 x-kubernetes-list-map-keys:
                 - name
@@ -162,11 +165,14 @@ spec:
                 - deleteSource
                 type: string
                 x-kubernetes-validations:
-                - message: retentionPolicy is immutable
+                - message: retentionPolicy value cannot be changed once set
                   rule: self == oldSelf
             required:
             - namespaces
             type: object
+            x-kubernetes-validations:
+            - message: retentionPolicy cannot be removed once set
+              rule: '!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)'
           status:
             description: MultiNamespaceVirtualMachineStorageMigrationPlanStatus defines
               the observed state of MultiNamespaceVirtualMachineStorageMigrationPlan

config/crd/bases/migrations.kubevirt.io_virtualmachinestoragemigrationplans.yaml

@@ -65,7 +65,7 @@ spec:
                 - deleteSource
                 type: string
                 x-kubernetes-validations:
-                - message: retentionPolicy is immutable
+                - message: retentionPolicy value cannot be changed once set
                   rule: self == oldSelf
               virtualMachines:
                 description: The virtual machines to migrate.
@@ -145,6 +145,9 @@ spec:
             required:
             - virtualMachines
             type: object
+            x-kubernetes-validations:
+            - message: retentionPolicy cannot be removed once set
+              rule: '!has(oldSelf.retentionPolicy) || has(self.retentionPolicy)'
           status:
             description: VirtualMachineStorageMigrationPlanStatus defines the observed
               state of VirtualMachineStorageMigrationPlan

internal/controller/multinamespacestoragemigplan/controller.go

@@ -187,10 +187,23 @@ func (r *MultiNamespaceStorageMigPlanReconciler) validateNamespace(ctx context.C
 
 func (r *MultiNamespaceStorageMigPlanReconciler) createNamespacePlan(ctx context.Context, plan *migrations.MultiNamespaceVirtualMachineStorageMigrationPlan, namespace *migrations.VirtualMachineStorageMigrationPlanNamespaceSpec) error {
 	spec := namespace.VirtualMachineStorageMigrationPlanSpec.DeepCopy()
-	// When the multinamespace plan has RetentionPolicy set (e.g. deleteSource), apply it to the child plan
-	if plan.Spec.RetentionPolicy != nil {
-		spec.RetentionPolicy = plan.Spec.RetentionPolicy
+
+	// Retention policy priority (with CRD defaults in place):
+	// 1. Namespace-specific retentionPolicy (if explicitly set to non-default value)
+	// 2. Multi-namespace plan retentionPolicy (if set to non-default value)
+	// 3. CRD default (keepSource)
+	//
+	// Due to CRD defaulting, both spec.RetentionPolicy and plan.Spec.RetentionPolicy will be non-nil
+	// (defaulted to keepSource). We treat keepSource as "not explicitly set" when determining priority.
+	if plan.Spec.RetentionPolicy != nil && *plan.Spec.RetentionPolicy != migrations.RetentionPolicyKeepSource {
+		// Parent has non-default value
+		if spec.RetentionPolicy == nil || *spec.RetentionPolicy == migrations.RetentionPolicyKeepSource {
+			// Namespace has default or nil - inherit from parent
+			spec.RetentionPolicy = plan.Spec.RetentionPolicy
+		}
+		// else: namespace has explicit non-default value, use it (priority 1)
 	}
+	// else: parent has default (keepSource), namespace keeps its value (default or explicit)
 	namespacePlan := &migrations.VirtualMachineStorageMigrationPlan{
 		ObjectMeta: metav1.ObjectMeta{
 			Name:      migrations.GetNamespacedPlanName(plan.Name, namespace.Name),

internal/controller/multinamespacestoragemigplan/controller_test.go

@@ -184,7 +184,7 @@ var _ = Describe("MultiNamespaceStorageMigPlan Controller", func() {
 			Expect(*childPlan.Spec.RetentionPolicy).To(Equal(migrations.RetentionPolicyDeleteSource))
 		})
 
-		It("Should allow setting the field for the first time, but not update/delete it", func() {
+		It("should enforce retentionPolicy immutability with type-level and field-level validation", func() {
 			key := types.NamespacedName{Name: "test-resource", Namespace: "default"}
 			created := &migrations.MultiNamespaceVirtualMachineStorageMigrationPlan{
 				ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace},
@@ -204,22 +204,32 @@ var _ = Describe("MultiNamespaceStorageMigPlan Controller", func() {
 			existing := &migrations.MultiNamespaceVirtualMachineStorageMigrationPlan{}
 			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
 
-			// Attempt to change the value
+			By("Attempting to change value from deleteSource to keepSource - field-level validation should fail")
 			existing.Spec.RetentionPolicy = ptr.To(migrations.RetentionPolicyKeepSource)
 			err := k8sClient.Update(ctx, existing)
 
 			Expect(err).To(HaveOccurred())
-			// Verify the specific CEL error message from your marker
-			Expect(err.Error()).To(ContainSubstring("retentionPolicy is immutable"))
+			Expect(err.Error()).To(ContainSubstring("retentionPolicy"))
 			existing = &migrations.MultiNamespaceVirtualMachineStorageMigrationPlan{}
 			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
+			Expect(*existing.Spec.RetentionPolicy).To(Equal(migrations.RetentionPolicyDeleteSource),
+				"Value should not have changed")
 
-			// Attempt to delete (set to nil)
+			By("Attempting to remove retentionPolicy (set to nil)")
 			existing.Spec.RetentionPolicy = nil
 			err = k8sClient.Update(ctx, existing)
 
+			// With CRD defaulting, nil becomes keepSource, so this triggers field-level validation
+			// The error will be about value change (deleteSource -> keepSource) not removal
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("retentionPolicy is immutable"))
+			Expect(err.Error()).To(Or(
+				ContainSubstring("retentionPolicy value cannot be changed"),
+				ContainSubstring("retentionPolicy cannot be removed"),
+			), "Either field-level or type-level validation should prevent the change")
+			existing = &migrations.MultiNamespaceVirtualMachineStorageMigrationPlan{}
+			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
+			Expect(*existing.Spec.RetentionPolicy).To(Equal(migrations.RetentionPolicyDeleteSource),
+				"Value should still be deleteSource")
 		})
 	})
 })

internal/controller/storagemig/controller_test.go

@@ -104,7 +104,7 @@ var _ = Describe("StorageMigration Controller", func() {
 	})
 
 	Context("When reconciling a migration that is completed", func() {
-		It("Should allow setting the field for the first time, but not update/delete it", func() {
+		It("should enforce retentionPolicy immutability with type-level and field-level validation", func() {
 			key := types.NamespacedName{Name: "test-resource", Namespace: "default"}
 			created := &migrations.VirtualMachineStorageMigrationPlan{
 				ObjectMeta: metav1.ObjectMeta{Name: key.Name, Namespace: key.Namespace},
@@ -126,22 +126,32 @@ var _ = Describe("StorageMigration Controller", func() {
 			existing := &migrations.VirtualMachineStorageMigrationPlan{}
 			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
 
-			// Attempt to change the value
+			By("Attempting to change value from deleteSource to keepSource - field-level validation should fail")
 			existing.Spec.RetentionPolicy = ptr.To(migrations.RetentionPolicyKeepSource)
 			err := k8sClient.Update(ctx, existing)
 
 			Expect(err).To(HaveOccurred())
-			// Verify the specific CEL error message from your marker
-			Expect(err.Error()).To(ContainSubstring("retentionPolicy is immutable"))
+			Expect(err.Error()).To(ContainSubstring("retentionPolicy"))
 			existing = &migrations.VirtualMachineStorageMigrationPlan{}
 			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
+			Expect(*existing.Spec.RetentionPolicy).To(Equal(migrations.RetentionPolicyDeleteSource),
+				"Value should not have changed")
 
-			// Attempt to delete (set to nil)
+			By("Attempting to remove retentionPolicy (set to nil)")
 			existing.Spec.RetentionPolicy = nil
 			err = k8sClient.Update(ctx, existing)
 
+			// With CRD defaulting, nil becomes keepSource, so this triggers field-level validation
+			// The error will be about value change (deleteSource -> keepSource) not removal
 			Expect(err).To(HaveOccurred())
-			Expect(err.Error()).To(ContainSubstring("retentionPolicy is immutable"))
+			Expect(err.Error()).To(Or(
+				ContainSubstring("retentionPolicy value cannot be changed"),
+				ContainSubstring("retentionPolicy cannot be removed"),
+			), "Either field-level or type-level validation should prevent the change")
+			existing = &migrations.VirtualMachineStorageMigrationPlan{}
+			Expect(k8sClient.Get(ctx, key, existing)).To(Succeed())
+			Expect(*existing.Spec.RetentionPolicy).To(Equal(migrations.RetentionPolicyDeleteSource),
+				"Value should still be deleteSource")
 		})
 	})
 })