Skip to content

Define behavior of retention policy in multi-namespace plans. #45

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kubevirt-bot merged 3 commits into from
Apr 13, 2026
Merged

Define behavior of retention policy in multi-namespace plans. #45

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
cc19c30
Define behavior of retention policy in multi-namespace plans.
awels Mar 26, 2026
db80dbd
Added unit tests for retention policy
awels Apr 6, 2026
1522a7c
Fix linter
awels Apr 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion api/migrationcontroller/v1alpha1/multinamespacevmstoragemigrationplan_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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
Expand Down
3 changes: 2 additions & 1 deletion api/migrationcontroller/v1alpha1/virtualmachinestoragemigrationplan_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Copy Markdown
Contributor

@akalenyu akalenyu Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure self == oldSelf doesn't cover this case? logically I would assume it does

Copy link
Copy Markdown
Member Author

@awels awels Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is some weirdness that happens when going from nil to something or vice versa. Especially with a default defined. I believe this is to ensure that if we have nil and select the default, it actually 'works'.

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.
Expand Down
10 changes: 8 additions & 2 deletions ...g/crd/bases/migrations.kubevirt.io_multinamespacevirtualmachinestoragemigrationplans.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand Down
5 changes: 4 additions & 1 deletion config/crd/bases/migrations.kubevirt.io_virtualmachinestoragemigrationplans.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -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
Expand Down
19 changes: 16 additions & 3 deletions internal/controller/multinamespacestoragemigplan/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Comment on lines +198 to +204
Copy link
Copy Markdown
Contributor

@akalenyu akalenyu Apr 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we want to unit test the priority? doesn't have to involve etcd, could extract to small util and test that

Copy link
Copy Markdown
Member Author

@awels awels Apr 6, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure give me a bit to update this.

}
// 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),
Expand Down
Loading