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 a2de001ae5..07b6ef6a06 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -159,8 +159,11 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat changes = h.detectSpecChanges(ctx, kvvm, ¤t.Spec, lastAppliedSpec) if !changes.IsEmpty() { kvvmi, kvvmiErr := s.KVVMI(ctx) - if kvvmiErr == nil && hasNonHotpluggableVolumes(kvvmi) { - changes.UpgradeBlockDeviceChangesToRestart() + if kvvmiErr == nil { + nonHotpluggableVolumes := nonHotpluggableVolumeRefs(kvvmi) + changes.UpgradeBlockDeviceChangesToRestartIf(func(change vmchange.FieldChange) bool { + return blockDeviceChangeTouchesRefs(change, nonHotpluggableVolumes) + }) } // Require restart for CPU and memory changes if VM is non migratable. if h.isVMNonMigratable(current) { @@ -193,7 +196,7 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat if kvvm == nil || changes.IsEmpty() { changed.Status.RestartAwaitingChanges = nil } else { - changed.Status.RestartAwaitingChanges, err = changes.ConvertPendingChanges() + changed.Status.RestartAwaitingChanges, err = changes.ConvertPendingRestartChanges() if err != nil { err = fmt.Errorf("failed to generate pending configuration changes: %w", err) cbConfApplied. @@ -202,6 +205,9 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat Message(service.CapitalizeFirstLetter(err.Error()) + ".") return reconcile.Result{}, err } + if len(changed.Status.RestartAwaitingChanges) == 0 { + changed.Status.RestartAwaitingChanges = nil + } } // 2. Wait if dependent resources are not ready yet. @@ -1193,20 +1199,56 @@ func (h *SyncKvvmHandler) patchPodNetworkAnnotation(ctx context.Context, s state return desired, nil } -// isPlacementPolicyChanged returns true if any of the Affinity, NodePlacement, or Toleration rules have changed. -func hasNonHotpluggableVolumes(kvvmi *virtv1.VirtualMachineInstance) bool { +func nonHotpluggableVolumeRefs(kvvmi *virtv1.VirtualMachineInstance) map[nameKindKey]struct{} { + refs := make(map[nameKindKey]struct{}) if kvvmi == nil { - return false + return refs } + for _, v := range kvvmi.Spec.Volumes { if v.PersistentVolumeClaim != nil && !v.PersistentVolumeClaim.Hotpluggable || v.ContainerDisk != nil && !v.ContainerDisk.Hotpluggable { + name, kind := kvbuilder.GetOriginalDiskName(v.Name) + if kind == "" { + continue + } + refs[nameKindKey{kind: kind, name: name}] = struct{}{} + } + } + + return refs +} + +func blockDeviceChangeTouchesRefs(change vmchange.FieldChange, refs map[nameKindKey]struct{}) bool { + if len(refs) == 0 { + return false + } + + for _, ref := range blockDeviceRefsFromValue(change.CurrentValue) { + if _, ok := refs[nameKindKey{kind: ref.Kind, name: ref.Name}]; ok { return true } } + for _, ref := range blockDeviceRefsFromValue(change.DesiredValue) { + if _, ok := refs[nameKindKey{kind: ref.Kind, name: ref.Name}]; ok { + return true + } + } + return false } +func blockDeviceRefsFromValue(value interface{}) []v1alpha2.BlockDeviceSpecRef { + switch v := value.(type) { + case v1alpha2.BlockDeviceSpecRef: + return []v1alpha2.BlockDeviceSpecRef{v} + case []v1alpha2.BlockDeviceSpecRef: + return v + default: + return nil + } +} + func (h *SyncKvvmHandler) isPlacementPolicyChanged(allChanges vmchange.SpecChanges) bool { for _, c := range allChanges.GetAll() { switch c.Path { diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go index ea6c9693a5..33e432550b 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm_test.go @@ -542,6 +542,56 @@ var _ = Describe("SyncKvvmHandler", func() { ), ) + It("does not expose apply-immediate block device changes in RestartAwaitingChanges while dependencies are not ready", func() { + ip := makeVMIP() + vmClass := makeVMClass() + + rootRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "root"} + blankRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "blank-disk"} + + vm := makeVM(v1alpha2.MachineRunning) + vm.Spec.EnableParavirtualization = ptr.To(true) + vm.Spec.BlockDeviceRefs = []v1alpha2.BlockDeviceSpecRef{rootRef, blankRef} + vm.Status.Conditions = append(vm.Status.Conditions, metav1.Condition{ + Type: vmcondition.TypeBlockDevicesReady.String(), + Status: metav1.ConditionFalse, + Reason: "BlockDevicesNotReady", + }) + + kvvm := makeKVVM(vm) + Expect(kvbuilder.SetLastAppliedSpec(kvvm, &v1alpha2.VirtualMachine{ + Spec: v1alpha2.VirtualMachineSpec{ + CPU: v1alpha2.CPUSpec{ + Cores: vm.Spec.CPU.Cores, + }, + Memory: v1alpha2.MemorySpec{ + Size: vm.Spec.Memory.Size, + }, + VirtualMachineIPAddress: vm.Spec.VirtualMachineIPAddress, + RunPolicy: vm.Spec.RunPolicy, + OsType: vm.Spec.OsType, + VirtualMachineClassName: vm.Spec.VirtualMachineClassName, + EnableParavirtualization: ptr.To(true), + BlockDeviceRefs: []v1alpha2.BlockDeviceSpecRef{rootRef}, + TerminationGracePeriodSeconds: vm.Spec.TerminationGracePeriodSeconds, + Disruptions: &v1alpha2.Disruptions{ + RestartApprovalMode: vm.Spec.Disruptions.RestartApprovalMode, + }, + }, + })).To(Succeed()) + + kvvmi := makeKVVMI() + fakeClient, reconcileObj, vmState = setupEnvironment(vm, kvvm, kvvmi, ip, vmClass) + + reconcile() + + updatedVM := &v1alpha2.VirtualMachine{} + Expect(fakeClient.Get(ctx, client.ObjectKeyFromObject(vm), updatedVM)).To(Succeed()) + Expect(updatedVM.Status.RestartAwaitingChanges).To(BeNil()) + awaitCond, awaitExists := conditions.GetCondition(vmcondition.TypeAwaitingRestartToApplyConfiguration, updatedVM.Status.Conditions) + Expect(awaitExists).To(BeFalse(), "ApplyImmediate block device changes should not set %s, got %+v", vmcondition.TypeAwaitingRestartToApplyConfiguration, awaitCond) + }) + It("keeps ConfigurationApplied False and requeues while SDN is not ready", func() { ip := &v1alpha2.VirtualMachineIPAddress{ ObjectMeta: metav1.ObjectMeta{Name: "test-ip", Namespace: namespace}, @@ -578,6 +628,46 @@ var _ = Describe("SyncKvvmHandler", func() { Expect(cond.Reason).To(Equal(vmcondition.ReasonConfigurationNotApplied.String())) }) + It("upgrades only block device changes touching non-hotpluggable volumes", func() { + rootRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "root"} + dataRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "data"} + extraRef := v1alpha2.BlockDeviceSpecRef{Kind: v1alpha2.DiskDevice, Name: "extra"} + + kvvmi := newEmptyKVVMI(name, namespace) + kvvmi.Spec.Volumes = []virtv1.Volume{ + { + Name: kvbuilder.GenerateDiskName(rootRef.Kind, rootRef.Name), + VolumeSource: virtv1.VolumeSource{ + PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{Hotpluggable: false}, + }, + }, + { + Name: kvbuilder.GenerateDiskName(dataRef.Kind, dataRef.Name), + VolumeSource: virtv1.VolumeSource{ + PersistentVolumeClaim: &virtv1.PersistentVolumeClaimVolumeSource{Hotpluggable: true}, + }, + }, + } + + nonHotpluggableRefs := nonHotpluggableVolumeRefs(kvvmi) + changes := vmchange.SpecChanges{} + changes.Add( + vmchange.FieldChange{Path: "blockDeviceRefs.0", Operation: vmchange.ChangeRemove, CurrentValue: dataRef, ActionRequired: vmchange.ActionApplyImmediate}, + vmchange.FieldChange{Path: "blockDeviceRefs.1", Operation: vmchange.ChangeAdd, DesiredValue: extraRef, ActionRequired: vmchange.ActionApplyImmediate}, + vmchange.FieldChange{Path: "blockDeviceRefs.2", Operation: vmchange.ChangeReplace, CurrentValue: rootRef, DesiredValue: dataRef, ActionRequired: vmchange.ActionApplyImmediate}, + vmchange.FieldChange{Path: "cpu.cores", Operation: vmchange.ChangeReplace, ActionRequired: vmchange.ActionApplyImmediate}, + ) + + changes.UpgradeBlockDeviceChangesToRestartIf(func(change vmchange.FieldChange) bool { + return blockDeviceChangeTouchesRefs(change, nonHotpluggableRefs) + }) + + Expect(changes.GetAll()[0].ActionRequired).To(Equal(vmchange.ActionApplyImmediate)) + Expect(changes.GetAll()[1].ActionRequired).To(Equal(vmchange.ActionApplyImmediate)) + Expect(changes.GetAll()[2].ActionRequired).To(Equal(vmchange.ActionRestart)) + Expect(changes.GetAll()[3].ActionRequired).To(Equal(vmchange.ActionApplyImmediate)) + }) + DescribeTable("isPlacementPolicyChanged", func(path string, expected bool) { h := &SyncKvvmHandler{} diff --git a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go b/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go index e400f72a4a..0580338e9e 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go @@ -230,8 +230,22 @@ func (s *SpecChanges) GetRestartMessages() []string { } func (s *SpecChanges) ConvertPendingChanges() ([]apiextensionsv1.JSON, error) { + return s.convertPendingChanges(func(FieldChange) bool { return true }) +} + +func (s *SpecChanges) ConvertPendingRestartChanges() ([]apiextensionsv1.JSON, error) { + return s.convertPendingChanges(func(change FieldChange) bool { + return change.ActionRequired == ActionRestart + }) +} + +func (s *SpecChanges) convertPendingChanges(include func(FieldChange) bool) ([]apiextensionsv1.JSON, error) { res := make([]apiextensionsv1.JSON, 0, len(s.changes)) for i := range s.changes { + if !include(s.changes[i]) { + continue + } + b, err := json.Marshal(s.changes[i]) if err != nil { return nil, fmt.Errorf("change[%d]: %w", i, err) @@ -243,9 +257,13 @@ func (s *SpecChanges) ConvertPendingChanges() ([]apiextensionsv1.JSON, error) { } func (s *SpecChanges) UpgradeBlockDeviceChangesToRestart() { + s.UpgradeBlockDeviceChangesToRestartIf(nil) +} + +func (s *SpecChanges) UpgradeBlockDeviceChangesToRestartIf(shouldUpgrade func(FieldChange) bool) { for i := range s.changes { isBlockDeviceChange := s.changes[i].Path == blockDevicesPath || strings.HasPrefix(s.changes[i].Path, blockDevicesPath+".") - if isBlockDeviceChange && s.changes[i].ActionRequired == ActionApplyImmediate { + if isBlockDeviceChange && s.changes[i].ActionRequired == ActionApplyImmediate && (shouldUpgrade == nil || shouldUpgrade(s.changes[i])) { s.changes[i].ActionRequired = ActionRestart } }