diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go index 513ab3dbda..b8f2afd537 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes.go @@ -119,13 +119,35 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. s.fillContainerDiskImagePullPolicies(kvvmInClusterCopy, kvvmiInCluster) s.fillContainerDiskImagePullPolicies(builtKVVM, kvvmiInCluster) + migrationRequested := builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy != nil && *builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration + kvvmiSynced := equality.Semantic.DeepEqual(kvvmInClusterCopy.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) if !kvvmiSynced { + // KVVM holds a dead migration set kubevirt will never sync (e.g. the target + // PVC was removed): revert to source. Only when the strategy is still set + // (plain divergence like a hotplug mid-attach must sync, not revert) and no + // round is wanted (a storage class round runs without a VMOP and must not + // be reverted at its start). + migrationStuck := kvvmInCluster.Spec.UpdateVolumesStrategy != nil && *kvvmInCluster.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration + if vmop == nil && migrationStuck && !migrationRequested && s.shouldPatchVolumes(kvvmInClusterCopy, builtKVVM) { + log.Info("No in-progress migration but kvvm/kvvmi diverged, force revert kvvm to source volumes.") + return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) + } // kubevirt does not sync volumes with kvvmi yet log.Info("kvvmi volumes are not synced yet, skip volume migration.") return reconcile.Result{}, nil } + // Clear a stale updateVolumesStrategy after a finished migration; kubevirt never + // clears it and keeps treating the VM as mid-migration. Volumes-equal guard skips + // the mid-completion window; the normalized copy is required for containerdisks. + if vmop == nil && + equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmInClusterCopy.Spec.Template.Spec.Volumes) && + !equality.Semantic.DeepEqual(builtKVVM.Spec.UpdateVolumesStrategy, kvvmInClusterCopy.Spec.UpdateVolumesStrategy) { + log.Info("clearing stale updateVolumesStrategy on kvvm after migration finished.") + return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) + } + readWriteOnceDisks, storageClassChangedDisks, err := s.getDisks(ctx, vmState) if err != nil { return reconcile.Result{}, err @@ -149,14 +171,9 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. } if !equality.Semantic.DeepEqual(builtKVVM.Spec.Template.Spec.Volumes, kvvmiInCluster.Spec.Volumes) { - // A difference here (ignoring migration target PVCs, which live in - // builtKVVMWithMigrationVolumes) means the desired volume set differs from - // the running one. Only a structural change (a disk added or removed) may - // require a restart, so it must not be propagated to KVVM while the VM - // awaits restart. A difference that is only a PVC swap on the same disks is - // a volume migration or a revert of one: it keeps the structure intact and - // must proceed regardless of restart, otherwise a KVVM left pointing at a - // dead migration target can never be reverted back to the source. + // Defer only structural changes (disk added/removed) under restart. A PVC swap + // on the same disks is a migration or its revert and must proceed regardless, + // else a KVVM pointing at a dead migration target can never be reverted. if restartRequired && isStructuralVolumeChange(builtKVVM, kvvmiInCluster) { log.Info("Virtualmachine is restart required, delay structural volume changes to KVVM.") return reconcile.Result{}, nil @@ -164,8 +181,6 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, s.patchVolumes(ctx, builtKVVM) } - migrationRequested := builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy != nil && *builtKVVMWithMigrationVolumes.Spec.UpdateVolumesStrategy == virtv1.UpdateVolumesStrategyMigration - // Check disks in generated KVVM before running kvvmSynced check: detect non-migratable disks and disks with changed storage class. if !readWriteOnceDisksSynced { log.Info("ReadWriteOnce disks are not synced yet, skip volume migration.") @@ -177,6 +192,22 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. } if migrationRequested { + // Completeness: patch the whole RWO set at once. A partial set mixes this + // round's targets with a previous round's PVCs, which KubeVirt rejects or, + // worse, drops an RWO volume and breaks the domain on the target node. + if !allDisksMigrating(readWriteOnceDisks) { + log.Info("not all ReadWriteOnce disks are migrating in this round yet, wait for a complete volume set.") + return reconcile.Result{RequeueAfter: 10 * time.Second}, nil + } + + // Serialization: while a migration is in flight, KubeVirt only accepts + // continuing to the same targets or reverting to the source (handled above); + // a new, different target set is rejected. Wait for the current one to finalize. + if isVolumeMigrating(kvvmiInCluster) && !destinationsMatch(kvvmiInCluster, builtKVVMWithMigrationVolumes) { + log.Info("a volume migration is already in progress with different targets, wait for it to finalize.") + return reconcile.Result{RequeueAfter: 5 * time.Second}, nil + } + // We should wait delayDuration seconds. This delay allows user to change storage class on other volumes if len(storageClassChangedDisks) > 0 { delay, exists := s.delay[vm.UID] @@ -224,10 +255,8 @@ func (s MigrationVolumesService) SyncVolumes(ctx context.Context, vmState state. return reconcile.Result{}, nil } -// isStructuralVolumeChange reports whether the desired and running volume sets -// differ structurally, i.e. a volume was added or removed. A difference that is -// only a PersistentVolumeClaim swap on the same set of volume names is a volume -// migration or its revert, not a structural change. +// isStructuralVolumeChange reports whether a volume was added or removed. A PVC +// swap on the same volume names is a migration or its revert, not structural. func isStructuralVolumeChange(builtKVVM *virtv1.VirtualMachine, kvvmi *virtv1.VirtualMachineInstance) bool { desired := make(map[string]struct{}, len(builtKVVM.Spec.Template.Spec.Volumes)) for _, v := range builtKVVM.Spec.Template.Spec.Volumes { @@ -283,13 +312,19 @@ func (s MigrationVolumesService) shouldRevert(kvvmi *virtv1.VirtualMachineInstan } func (s MigrationVolumesService) patchVolumes(ctx context.Context, kvvm *virtv1.VirtualMachine) error { - patchBytes, err := patch.NewJSONPatch( + ops := []patch.JSONPatchOperation{ patch.WithReplace("/spec/updateVolumesStrategy", kvvm.Spec.UpdateVolumesStrategy), patch.WithReplace("/spec/template/spec/volumes", kvvm.Spec.Template.Spec.Volumes), // Affinity is patched together with volumes because the migration target PVCs // can resolve to a different node than the source. patch.WithReplace("/spec/template/spec/affinity", kvvm.Spec.Template.Spec.Affinity), - ).Bytes() + } + // Optimistic lock: kubevirt persists hotplug (addvolume) volumes into the same + // array concurrently; replacing it from a stale read silently drops them. + if kvvm.ResourceVersion != "" { + ops = append([]patch.JSONPatchOperation{patch.WithTest("/metadata/resourceVersion", kvvm.ResourceVersion)}, ops...) + } + patchBytes, err := patch.NewJSONPatch(ops...).Bytes() if err != nil { return err } @@ -554,6 +589,68 @@ func (s MigrationVolumesService) makeKVVMFromVirtualMachineSpec(ctx context.Cont return kvvm, kvvmWithMigrationVolumes, nil } +// allDisksMigrating reports whether every disk is migrating in the current round. +func allDisksMigrating(disks map[string]*v1alpha2.VirtualDisk) bool { + for _, d := range disks { + if !commonvd.IsMigrating(d) { + return false + } + } + return true +} + +// isVolumeMigrating reports whether KubeVirt is currently running a volume +// migration for the VMI (condition VolumesChange=True). +func isVolumeMigrating(kvvmi *virtv1.VirtualMachineInstance) bool { + cond, _ := conditions.GetKVVMICondition(virtv1.VirtualMachineInstanceVolumesChange, kvvmi.Status.Conditions) + return cond.Status == corev1.ConditionTrue +} + +// destinationsMatch reports whether the patched set merely continues the in-flight +// migration: every volume keeps its running claim or goes to its recorded destination +// (kvvmi.status.migratedVolumes), and no in-flight volume is left out. Anything else +// KubeVirt rejects mid-migration. +func destinationsMatch(kvvmi *virtv1.VirtualMachineInstance, built *virtv1.VirtualMachine) bool { + current := make(map[string]string, len(kvvmi.Spec.Volumes)) + for _, v := range kvvmi.Spec.Volumes { + if v.PersistentVolumeClaim != nil { + current[v.Name] = v.PersistentVolumeClaim.ClaimName + } + } + + dest := make(map[string]string, len(kvvmi.Status.MigratedVolumes)) + for _, mv := range kvvmi.Status.MigratedVolumes { + if mv.DestinationPVCInfo != nil { + dest[mv.VolumeName] = mv.DestinationPVCInfo.ClaimName + } + } + + seen := make(map[string]struct{}, len(built.Spec.Template.Spec.Volumes)) + for _, v := range built.Spec.Template.Spec.Volumes { + if v.PersistentVolumeClaim == nil { + continue + } + seen[v.Name] = struct{}{} + if d, ok := dest[v.Name]; ok { + if v.PersistentVolumeClaim.ClaimName != d { + return false + } + continue + } + if v.PersistentVolumeClaim.ClaimName != current[v.Name] { + return false + } + } + + for name := range dest { + if _, ok := seen[name]; !ok { + return false + } + } + + return true +} + // areDisksSynced checks whether all disks are synchronized with their corresponding PVCs in kvvm // All TargetPVCs on disks must be present in kvvm func (s MigrationVolumesService) areDisksSynced(kvvm *virtv1.VirtualMachine, disks map[string]*v1alpha2.VirtualDisk) bool { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go index 496a52f724..542f835433 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/service/migration_volumes_test.go @@ -250,6 +250,202 @@ var _ = Describe("MigrationVolumesService", func() { Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC)) Expect(updatedKVVM.Spec.Template.Spec.Affinity).To(Equal(desiredKVVM.Spec.Template.Spec.Affinity)) }) + + It("clears a stale migration strategy left on kvvm when volumes already match and no migration is in progress", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + // A finished migration left updateVolumesStrategy=Migration on KVVM while KVVM + // and KVVMI already agree on the volumes. The stale strategy must be cleared. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmi := newKVVMIWithVolume(targetPVC) + desiredKVVM := newKVVMWithVolume(targetPVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) + + It("clears a stale migration strategy on a VM with a containerdisk volume despite the pull-policy drift", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + containerDisk := func(policy corev1.PullPolicy) virtv1.Volume { + return virtv1.Volume{ + Name: "cdrom", + VolumeSource: virtv1.VolumeSource{ + ContainerDisk: &virtv1.ContainerDiskSource{Image: "registry.example.com/image:tag", ImagePullPolicy: policy}, + }, + } + } + + vm := newVM() + // The pull policy is defaulted by kubevirt only on the VMI: KVVM and the + // desired spec carry an empty one. The stale strategy must still be cleared. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmInCluster.Spec.Template.Spec.Volumes = append(kvvmInCluster.Spec.Template.Spec.Volumes, containerDisk("")) + kvvmi := newKVVMIWithVolume(targetPVC) + kvvmi.Spec.Volumes = append(kvvmi.Spec.Volumes, containerDisk(corev1.PullIfNotPresent)) + desiredKVVM := newKVVMWithVolume(targetPVC, nil, "node") + desiredKVVM.Spec.Template.Spec.Volumes = append(desiredKVVM.Spec.Template.Spec.Volumes, containerDisk("")) + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + }) + + It("force-reverts kvvm to source when kvvm/kvvmi diverged and no migration is in progress", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + // KVVM is stuck on a dead migration target (with the migration strategy), + // while KVVMI never synced and still points at the source. With no in-progress + // migration this must be force-reverted instead of waiting on the kvvmiSynced + // barrier forever. + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "target-node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "source-node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(BeNil()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes).To(HaveLen(1)) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(sourcePVC)) + }) + + It("does not revert diverged volumes when no migration strategy is set (e.g. hotplug mid-attach)", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + + vm := newVM() + // KVVM and KVVMI diverge but there is no migration strategy: a benign + // transient such as a hotplug volume being attached. It must be left to sync, + // not force-reverted (that would tear down the in-flight attachment). + kvvmInCluster := newKVVMWithVolume(targetPVC, nil, "node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) + + It("rejects a volume patch built from a stale kvvm read", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + vm := newVM() + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "target-node") + kvvmi := newKVVMIWithVolume(sourcePVC) + // The desired spec carries the resourceVersion of the kvvm it was built from; + // if the kvvm changed since (e.g. kubevirt persisted a hotplug volume), the + // patch must fail instead of overwriting the volumes from the stale read. + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "source-node") + desiredKVVM.ResourceVersion = "stale" + vmState := setupState(vm, kvvmInCluster, kvvmi) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).To(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) + + It("does not force-revert kvvm while a migration round without a vmop is active", func() { + ctx := testutil.ContextBackgroundWithNoOpLogger() + migrationStrategy := virtv1.UpdateVolumesStrategyMigration + + // A storage class change round runs without any VMOP: the disk is migrating, + // KVVM is already patched with the round's target, KVVMI is not synced yet. + // The diverged KVVM is the round starting, not a leftover to revert. + vm := newVM() + vm.Status.BlockDeviceRefs = []v1alpha2.BlockDeviceStatusRef{{Kind: v1alpha2.DiskDevice, Name: "root"}} + vd := &v1alpha2.VirtualDisk{ + ObjectMeta: metav1.ObjectMeta{Name: "root", Namespace: namespace}, + } + vd.Status.MigrationState.StartTimestamp = metav1.Now() + vd.Status.MigrationState.TargetPVC = targetPVC + kvvmInCluster := newKVVMWithVolume(targetPVC, &migrationStrategy, "node") + kvvmi := newKVVMIWithVolume(sourcePVC) + desiredKVVM := newKVVMWithVolume(sourcePVC, nil, "node") + vmState := setupState(vm, kvvmInCluster, kvvmi, vd) + + service := NewMigrationVolumesService( + vmState.Client(), + func(context.Context, state.VirtualMachineState) (*virtv1.VirtualMachine, error) { + return desiredKVVM.DeepCopy(), nil + }, + 10*time.Second, + ) + + _, err := service.SyncVolumes(ctx, vmState, false) + Expect(err).NotTo(HaveOccurred()) + + updatedKVVM := &virtv1.VirtualMachine{} + Expect(vmState.Client().Get(ctx, types.NamespacedName{Name: vmName, Namespace: namespace}, updatedKVVM)).To(Succeed()) + Expect(updatedKVVM.Spec.UpdateVolumesStrategy).To(HaveValue(Equal(migrationStrategy))) + Expect(updatedKVVM.Spec.Template.Spec.Volumes[0].PersistentVolumeClaim.ClaimName).To(Equal(targetPVC)) + }) }) var _ = Describe("isStructuralVolumeChange", func() { @@ -302,3 +498,114 @@ var _ = Describe("isStructuralVolumeChange", func() { Entry("disk renamed (same count, different name)", map[string]string{"root": "a"}, map[string]string{"data": "a"}, true), ) }) + +var _ = Describe("allDisksMigrating", func() { + mk := func(started, ended bool) *v1alpha2.VirtualDisk { + vd := &v1alpha2.VirtualDisk{} + if started { + vd.Status.MigrationState.StartTimestamp = metav1.Now() + } + if ended { + vd.Status.MigrationState.EndTimestamp = metav1.Now() + } + return vd + } + + It("is true for an empty set", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{})).To(BeTrue()) + }) + It("is true when every disk is migrating this round", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(true, false)})).To(BeTrue()) + }) + It("is false when a disk has not started migrating", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(false, false)})).To(BeFalse()) + }) + It("is false when a disk already completed a previous round", func() { + Expect(allDisksMigrating(map[string]*v1alpha2.VirtualDisk{"a": mk(true, false), "b": mk(true, true)})).To(BeFalse()) + }) +}) + +var _ = Describe("isVolumeMigrating", func() { + withVolumesChange := func(status corev1.ConditionStatus, set bool) *virtv1.VirtualMachineInstance { + vmi := &virtv1.VirtualMachineInstance{} + if set { + vmi.Status.Conditions = []virtv1.VirtualMachineInstanceCondition{ + {Type: virtv1.VirtualMachineInstanceVolumesChange, Status: status}, + } + } + return vmi + } + + It("is true when VolumesChange condition is True", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionTrue, true))).To(BeTrue()) + }) + It("is false when VolumesChange condition is False", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionFalse, true))).To(BeFalse()) + }) + It("is false when VolumesChange condition is absent", func() { + Expect(isVolumeMigrating(withVolumesChange(corev1.ConditionTrue, false))).To(BeFalse()) + }) +}) + +var _ = Describe("destinationsMatch", func() { + pvcVolumes := func(nameToClaim map[string]string) []virtv1.Volume { + vols := make([]virtv1.Volume, 0, len(nameToClaim)) + for name, claim := range nameToClaim { + vols = append(vols, virtv1.Volume{ + Name: name, + VolumeSource: virtv1.VolumeSource{PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{PersistentVolumeClaimVolumeSource: corev1.PersistentVolumeClaimVolumeSource{ClaimName: claim}}}, + }) + } + return vols + } + built := func(nameToClaim map[string]string) *virtv1.VirtualMachine { + return &virtv1.VirtualMachine{Spec: virtv1.VirtualMachineSpec{Template: &virtv1.VirtualMachineInstanceTemplateSpec{Spec: virtv1.VirtualMachineInstanceSpec{Volumes: pvcVolumes(nameToClaim)}}}} + } + kvvmi := func(running, volToDest map[string]string) *virtv1.VirtualMachineInstance { + vmi := &virtv1.VirtualMachineInstance{} + vmi.Spec.Volumes = pvcVolumes(running) + for vol, dest := range volToDest { + vmi.Status.MigratedVolumes = append(vmi.Status.MigratedVolumes, virtv1.StorageMigratedVolumeInfo{ + VolumeName: vol, + DestinationPVCInfo: &virtv1.PersistentVolumeClaimInfo{ClaimName: dest}, + }) + } + return vmi + } + + It("is true when the set keeps the running claims and there is no recorded migration", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, nil), built(map[string]string{"root": "src"}))).To(BeTrue()) + }) + It("is false when a claim changes without a recorded migration for it", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, nil), built(map[string]string{"root": "new"}))).To(BeFalse()) + }) + It("is true when the recorded destination matches the target being patched", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, map[string]string{"root": "tgt"}), built(map[string]string{"root": "tgt"}))).To(BeTrue()) + }) + It("is false when the recorded destination differs from the new target", func() { + Expect(destinationsMatch(kvvmi(map[string]string{"root": "src"}, map[string]string{"root": "old-tgt"}), built(map[string]string{"root": "new-tgt"}))).To(BeFalse()) + }) + It("is false when the set migrates an extra volume on top of the in-flight round", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src", "data": "data-src"}, map[string]string{"root": "tgt"}), + built(map[string]string{"root": "tgt", "data": "data-tgt"}), + )).To(BeFalse()) + }) + It("is false when the set keeps a volume at source while it is migrating", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src", "data": "data-src"}, map[string]string{"root": "tgt", "data": "data-tgt"}), + built(map[string]string{"root": "tgt", "data": "data-src"}), + )).To(BeFalse()) + }) + It("is false when an in-flight volume is absent from the set being patched", func() { + Expect(destinationsMatch( + kvvmi(map[string]string{"root": "src"}, map[string]string{"hotplug": "hp-tgt"}), + built(map[string]string{"root": "src"}), + )).To(BeFalse()) + }) + It("ignores recorded entries without destination info", func() { + vmi := kvvmi(map[string]string{"root": "src"}, nil) + vmi.Status.MigratedVolumes = []virtv1.StorageMigratedVolumeInfo{{VolumeName: "root", DestinationPVCInfo: nil}} + Expect(destinationsMatch(vmi, built(map[string]string{"root": "src"}))).To(BeTrue()) + }) +}) diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go index b8a72e409d..0d9089fb6e 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -297,7 +297,12 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat cond, _ := conditions.GetKVVMCondition(virtv1.VirtualMachineRestartRequired, kvvm.Status.Conditions) if cond.Status == corev1.ConditionTrue && len(kvvm.Status.StateChangeRequests) == 0 { msg := "Please restart the virtual machine to synchronize its configuration." - log.Error(msg) + // Log only on transition: the condition is latched until restart, so + // logging every reconcile would spam. + prev, _ := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, current.Status.Conditions) + if prev.Status != metav1.ConditionTrue || prev.Reason != vmcondition.ReasonUnexpectedState.String() { + log.Info(msg, "kvvmRestartRequiredReason", cond.Reason, "kvvmRestartRequiredMessage", cond.Message) + } cbAwaitingRestart. Status(metav1.ConditionTrue). Reason(vmcondition.ReasonUnexpectedState). diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go index 0abae12f95..809bd9edd5 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle.go @@ -49,6 +49,10 @@ const lifecycleHandlerName = "LifecycleHandler" const timeElapsedUpdateInterval = 10 * time.Second +// waitForVMReadyToMigrateTimeout fails a migration whose disks never sync instead +// of waiting for ReadyToMigrate forever. Healthy migrations reach it within ~1m. +const waitForVMReadyToMigrateTimeout = 5 * time.Minute + const ( progressMigrationPending int32 = 0 progressTargetScheduling int32 = 2 @@ -259,6 +263,12 @@ func (h LifecycleHandler) Handle(ctx context.Context, vmop *v1alpha2.VirtualMach // 7. Check if the vm is migratable. if !h.canExecute(vmop, vm) { + // Drive the deadline in canExecute while the operation waits for the VM to + // become ready to migrate, so a wedged migration is failed even without + // further watch events. + if vmop.Status.Phase == v1alpha2.VMOPPhasePending { + return reconcile.Result{RequeueAfter: timeElapsedUpdateInterval}, nil + } return reconcile.Result{}, nil } // 7.1 The Operation is valid, and can be executed. @@ -440,6 +450,22 @@ func (h LifecycleHandler) canExecute(vmop *v1alpha2.VirtualMachineOperation, vm migratable, _ := conditions.GetCondition(vmcondition.TypeMigratable, vm.Status.Conditions) if migratable.Status == metav1.ConditionTrue { + completed, _ := conditions.GetCondition(vmopcondition.TypeCompleted, vmop.Status.Conditions) + waitingForReady := completed.Reason == vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate.String() + if waitingForReady && !completed.LastTransitionTime.IsZero() && + time.Since(completed.LastTransitionTime.Time) > waitForVMReadyToMigrateTimeout { + vmop.Status.Phase = v1alpha2.VMOPPhaseFailed + h.recorder.Event(vmop, corev1.EventTypeWarning, v1alpha2.ReasonErrVMOPFailed, "Timed out waiting for the VirtualMachine to become ready to migrate") + conditions.SetCondition( + conditions.NewConditionBuilder(vmopcondition.TypeCompleted). + Generation(vmop.GetGeneration()). + Reason(vmopcondition.ReasonOperationFailed). + Status(metav1.ConditionFalse). + Message("Timed out waiting for the VirtualMachine to become ready to migrate."), + &vmop.Status.Conditions) + return false + } + vmop.Status.Phase = v1alpha2.VMOPPhasePending conditions.SetCondition( conditions.NewConditionBuilder(vmopcondition.TypeCompleted). diff --git a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go index 23fbd0a170..6a37ff1f5c 100644 --- a/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go +++ b/images/virtualization-artifact/pkg/controller/vmop/migration/internal/handler/lifecycle_test.go @@ -247,6 +247,38 @@ var _ = Describe("LifecycleHandler", func() { Expect(completed.Reason).To(Equal(vmopcondition.ReasonTargetScheduling.String())) }) + waitingVMOP := func(age time.Duration) *v1alpha2.VirtualMachineOperation { + vmop := newVMOPMigrate() + vmop.Status.Phase = v1alpha2.VMOPPhasePending + vmop.Status.Conditions = []metav1.Condition{{ + Type: vmopcondition.TypeCompleted.String(), + Status: metav1.ConditionFalse, + Reason: vmopcondition.ReasonWaitingForVirtualMachineToBeReadyToMigrate.String(), + LastTransitionTime: metav1.NewTime(time.Now().Add(-age)), + }} + return vmop + } + + It("fails a migration that waited past the timeout for the VM to be ready", func() { + h := LifecycleHandler{recorder: recorderMock} + vm := newVM(v1alpha2.AlwaysSafeMigrationPolicy) + vmop := waitingVMOP(waitForVMReadyToMigrateTimeout + time.Minute) + + Expect(h.canExecute(vmop, vm)).To(BeFalse()) + Expect(vmop.Status.Phase).To(Equal(v1alpha2.VMOPPhaseFailed)) + completed, _ := conditions.GetCondition(vmopcondition.TypeCompleted, vmop.Status.Conditions) + Expect(completed.Reason).To(Equal(vmopcondition.ReasonOperationFailed.String())) + }) + + It("keeps a migration pending while within the ready-to-migrate timeout", func() { + h := LifecycleHandler{recorder: recorderMock} + vm := newVM(v1alpha2.AlwaysSafeMigrationPolicy) + vmop := waitingVMOP(time.Minute) + + Expect(h.canExecute(vmop, vm)).To(BeFalse()) + Expect(vmop.Status.Phase).To(Equal(v1alpha2.VMOPPhasePending)) + }) + DescribeTable("TargetMigration", func(vmPolicy v1alpha2.LiveMigrationPolicy, nodeSelector map[string]string, targetMigrationEnabled bool) { vm := newVM(vmPolicy) vm.Status.Conditions = []metav1.Condition{ diff --git a/test/e2e/internal/util/vm.go b/test/e2e/internal/util/vm.go index d1778bffc9..fe84a47fa7 100644 --- a/test/e2e/internal/util/vm.go +++ b/test/e2e/internal/util/vm.go @@ -41,8 +41,7 @@ import ( ) const ( - VmopE2ePrefix = "vmop-e2e" - knownVolumeUpdateFailureReason = "VolumesUpdateError" + VmopE2ePrefix = "vmop-e2e" ) var knownKubeVirtClientSocketClosedRe = regexp.MustCompile(`(?is)virError\(Code=1,.*internal error:\s*client\s+socket\s+is\s+closed`) @@ -77,69 +76,16 @@ func SkipIfKnownKubeVirtClientSocketClosedMigrationFailureWithContext(ctx contex } } -func IsKnownVolumesUpdateFailureReason(reason string) bool { - return reason == knownVolumeUpdateFailureReason -} - -// TODO: remove temporary migration skip logic when known issue "VolumesUpdateError" is fixed: -func SkipIfKnownVolumesUpdateMigrationFailure(vm *v1alpha2.VirtualMachine) { - SkipIfKnownVolumesUpdateMigrationFailureWithContext(context.Background(), vm) -} - -// TODO: remove temporary migration skip logic when known issue "VolumesUpdateError" is fixed: -func SkipIfKnownVolumesUpdateMigrationFailureWithContext(ctx context.Context, vm *v1alpha2.VirtualMachine) { - GinkgoHelper() - - if vm == nil { - return - } - - intvirtvmi, err := GetInternalVirtualMachineInstance(ctx, vm) - Expect(err).NotTo(HaveOccurred()) - if intvirtvmi == nil { - return - } - - // Prefer checking the concrete migratable condition, where volume update issues are expected. - migratableCondition, exists := conditions.GetKVVMICondition(virtv1.VirtualMachineInstanceIsMigratable, intvirtvmi.Status.Conditions) - if exists && IsKnownVolumesUpdateFailureReason(migratableCondition.Reason) { - Skip(fmt.Sprintf("skip due to known volume update migration issue for vm %s/%s: condition=%s, reason=%s, message=%s", - vm.Namespace, vm.Name, migratableCondition.Type, migratableCondition.Reason, migratableCondition.Message)) - } -} - -// TODO: remove temporary migration skip logic when both known issues are fixed: -// kubevirt "client socket is closed" and VolumesUpdateError. +// TODO: remove temporary migration skip logic when issue "client socket is closed" is fixed: func SkipIfKnownMigrationFailure(vm *v1alpha2.VirtualMachine) { SkipIfKnownMigrationFailureWithContext(context.Background(), vm) } -// TODO: remove temporary migration skip logic when both known issues are fixed: -// kubevirt "client socket is closed" and VolumesUpdateError. +// TODO: remove temporary migration skip logic when issue "client socket is closed" is fixed: func SkipIfKnownMigrationFailureWithContext(ctx context.Context, vm *v1alpha2.VirtualMachine) { GinkgoHelper() SkipIfKnownKubeVirtClientSocketClosedMigrationFailureWithContext(ctx, vm) - SkipIfKnownVolumesUpdateMigrationFailureWithContext(ctx, vm) -} - -// TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: -// controller may revert volume migration (VM not running, VM not migrating, etc.). -func SkipIfVDMigrationReverted(namespace string) { - GinkgoHelper() - - vds, err := framework.GetClients().VirtClient().VirtualDisks(namespace).List(context.Background(), metav1.ListOptions{}) - if err != nil { - GinkgoWriter.Printf("Failed to list VirtualDisks in namespace %q for revert check: %v\n", namespace, err) - return - } - - for _, vd := range vds.Items { - if vd.Status.MigrationState.Result == v1alpha2.VirtualDiskMigrationResultFailed && - vd.Status.MigrationState.Message == "Migration reverted." { - Skip(fmt.Sprintf("skip: VD %s/%s migration was reverted", namespace, vd.Name)) - } - } } func GetInternalVirtualMachineInstance(ctx context.Context, vm *v1alpha2.VirtualMachine) (*virtv1.VirtualMachineInstance, error) { @@ -274,10 +220,6 @@ func UntilVMMigrationSucceeded(key client.ObjectKey, timeout time.Duration) { // The VM object mirrors the migration state of the completed VMOP with a small lag; keep // asserting the same final state as before. Eventually(func() error { - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - SkipIfVDMigrationReverted(key.Namespace) - vm, err := framework.GetClients().VirtClient().VirtualMachines(key.Namespace).Get(context.Background(), key.Name, metav1.GetOptions{}) if err != nil { return err diff --git a/test/e2e/internal/util/vmop.go b/test/e2e/internal/util/vmop.go index ef68694a60..0802a08f6c 100644 --- a/test/e2e/internal/util/vmop.go +++ b/test/e2e/internal/util/vmop.go @@ -70,10 +70,6 @@ func failVMOPMigration(vmop *v1alpha2.VirtualMachineOperation, err error) { func skipIfKnownMigrationIssue(vmop *v1alpha2.VirtualMachineOperation) { GinkgoHelper() - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - SkipIfVDMigrationReverted(vmop.Namespace) - // The context is intentionally fresh: the caller's context may already be expired on the // timeout path, while the skip checks must still be able to inspect the cluster. ctx := context.Background() diff --git a/test/e2e/vm/util.go b/test/e2e/vm/util.go index ccd9613527..a908210093 100644 --- a/test/e2e/vm/util.go +++ b/test/e2e/vm/util.go @@ -105,6 +105,23 @@ func rootAndAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, r return vm, vds } +// rootAndManyAdditionalBuild builds a VM with a root disk plus count additional +// ReadWriteOnce disks on the given storage class. +func rootAndManyAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, root buildOption, storageClass *string, count int) (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { + refs := []v1alpha2.BlockDeviceSpecRef{{Kind: v1alpha2.VirtualDiskKind, Name: root.name}} + vds := []*v1alpha2.VirtualDisk{newRootVD(f, root, vi)} + for i := range count { + name := fmt.Sprintf("vd-alpine-additional-disk-%d", i) + refs = append(refs, v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.VirtualDiskKind, Name: name}) + vds = append(vds, newBlankVD(f, buildOption{name: name, storageClass: storageClass, rwo: true})) + } + vm := object.NewMinimalVM("volume-migration-many-disks-", f.Namespace().Name, + vmbuilder.WithBlockDeviceRefs(refs...), + vmbuilder.WithCPU(1, ptr.To("100%")), + ) + return vm, vds +} + func onlyAdditionalBuild(f *framework.Framework, vi *v1alpha2.VirtualImage, root, additional buildOption) (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { vm := object.NewMinimalVM( "volume-migration-only-additional-disk-", @@ -133,10 +150,6 @@ func untilVirtualDisksMigrationsSucceeded(f *framework.Framework) { By("Wait until VirtualDisks migrations succeeded") Eventually(func(g Gomega) { - // TODO: remove temporary migration skip logic when VD Migration Controller revert issue is fixed: - // controller may revert volume migration (VM not running, VM not migrating, etc.). - e2eutil.SkipIfVDMigrationReverted(f.Namespace().Name) - vms, err := f.VirtClient().VirtualMachines(f.Namespace().Name).List(context.Background(), metav1.ListOptions{}) g.Expect(err).NotTo(HaveOccurred()) for _, vm := range vms.Items { diff --git a/test/e2e/vm/volume_migration_local_disks.go b/test/e2e/vm/volume_migration_local_disks.go index fb931fec7a..4797743b23 100644 --- a/test/e2e/vm/volume_migration_local_disks.go +++ b/test/e2e/vm/volume_migration_local_disks.go @@ -104,6 +104,10 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab ) } + localMigrationManyDisksBuild := func() (*v1alpha2.VirtualMachine, []*v1alpha2.VirtualDisk) { + return rootAndManyAdditionalBuild(f, vi, buildOption{name: vdRootName, storageClass: &storageClass.Name, rwo: true}, &storageClass.Name, 3) + } + DescribeTable("should be successful", func(build func() (vm *v1alpha2.VirtualMachine, vds []*v1alpha2.VirtualDisk)) { ns := f.Namespace().Name @@ -213,6 +217,58 @@ var _ = Describe("RWOVirtualDiskMigration", decoratorsForVolumeMigrations(), Lab } }) + It("keeps a multi-disk volume set consistent when a restart is requested mid-migration", func() { + ns := f.Namespace().Name + + vm, vds := localMigrationManyDisksBuild() + + vm, err := f.VirtClient().VirtualMachines(ns).Create(ctx, vm, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(vm) + + for _, vd := range vds { + _, err := f.VirtClient().VirtualDisks(ns).Create(ctx, vd, metav1.CreateOptions{}) + Expect(err).NotTo(HaveOccurred()) + f.DeferDelete(vd) + } + + By("Wait until VM agent is ready") + util.UntilVMAgentReady(ctx, crclient.ObjectKeyFromObject(vm), framework.LongTimeout) + + By("Migrating the whole set once so the disks move off their base PVCs") + firstVMOP := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName("many-disks-migration-1")) + util.UntilVMOPMigrationSucceeded(ctx, firstVMOP, framework.MaxTimeout) + untilVirtualDisksMigrationsSucceeded(f) + + By("Starting a second migration of the whole volume set") + vmop := util.MigrateVirtualMachine(f, vm, vmopbuilder.WithName("many-disks-migration-2")) + + // Request a restart right after the migration starts, so the restart reconcile + // races with the still-unfinalized volume set. It must not issue a conflicting + // volume update over that set, otherwise KubeVirt rejects it ("the volume can only + // be reverted to the previous version during the update") and the set is left + // inconsistent. On copy-based storage the patch lands mid-migration; on instant + // (replicated) storage it lands right after — both must finalize cleanly. + By("Requesting a restart around the migration") + patchBytes, err := patch.NewJSONPatch(patch.WithAdd("/spec/terminationGracePeriodSeconds", int64(11))).Bytes() + Expect(err).NotTo(HaveOccurred()) + _, err = f.VirtClient().VirtualMachines(ns).Patch(ctx, vm.GetName(), types.JSONPatchType, patchBytes, metav1.PatchOptions{}) + Expect(err).NotTo(HaveOccurred()) + + By("The migration still finalizes cleanly") + util.UntilVMOPMigrationSucceeded(ctx, vmop, framework.MaxTimeout) + + vm, err = f.VirtClient().VirtualMachines(ns).Get(ctx, vm.GetName(), metav1.GetOptions{}) + Expect(err).NotTo(HaveOccurred()) + Expect(vm.Status.MigrationState).ShouldNot(BeNil()) + Expect(vm.Status.MigrationState.Result).To(Equal(v1alpha2.MigrationResultSucceeded)) + + untilVirtualDisksMigrationsSucceeded(f) + + By("Restart stays pending: the change was neither lost nor applied without a restart") + Expect(util.IsRestartRequired(vm, framework.ShortTimeout)).To(BeTrue()) + }) + It("should be successful when a restart is pending", func() { ns := f.Namespace().Name