diff --git a/images/virtualization-artifact/pkg/common/vm/vm.go b/images/virtualization-artifact/pkg/common/vm/vm.go index d98b4877cf..648649f454 100644 --- a/images/virtualization-artifact/pkg/common/vm/vm.go +++ b/images/virtualization-artifact/pkg/common/vm/vm.go @@ -36,6 +36,14 @@ import ( // Container name is "d8v-compute", but previous versions may have "compute" container. const VMContainerNameSuffix = "compute" +const ( + MaxCoresPerSocketLow = 16 + MaxCoresPerSocketHigh = 31 + MaxSockets = 8 + + MaxCores = MaxCoresPerSocketHigh * MaxSockets +) + // CalculateCoresAndSockets calculates the number of sockets and cores per socket needed to achieve // the desired total number of CPU cores. // The function tries to minimize the number of sockets while ensuring the desired core count. @@ -43,30 +51,31 @@ const VMContainerNameSuffix = "compute" // https://bugzilla.redhat.com/show_bug.cgi?id=1653453 // The number of cores per socket and the growth of the number of sockets is chosen in such a way as // to have less impact on the performance of the virtual machine, as well as on compatibility with operating systems -func CalculateCoresAndSockets(desiredCores int) (sockets, coresPerSocket int) { - if desiredCores < 1 { - return 1, 1 - } - - if desiredCores <= 16 { - return 1, desiredCores - } +func CalculateCoresAndSockets(desiredCores int) (sockets, coresPerSocket, maxCoresPerSocket int) { + maxCoresPerSocket = MaxCoresPerSocketLow switch { - case desiredCores <= 32: + case desiredCores <= 0: + return 1, 1, MaxCoresPerSocketLow + case desiredCores <= MaxCoresPerSocketLow: + return 1, desiredCores, MaxCoresPerSocketLow + case desiredCores <= MaxCoresPerSocketLow*2: sockets = 2 - case desiredCores <= 64: + case desiredCores <= MaxCoresPerSocketLow*4: sockets = 4 default: - sockets = 8 + sockets = MaxSockets + // Increased cores per socket to have maximum of 248 cores. + maxCoresPerSocket = MaxCoresPerSocketHigh } + // Grow cores if desired count is not dividable by sockets count. coresPerSocket = desiredCores / sockets if desiredCores%sockets != 0 { coresPerSocket++ } - return sockets, coresPerSocket + return sockets, coresPerSocket, maxCoresPerSocket } func ApprovalMode(vm *v1alpha2.VirtualMachine) v1alpha2.RestartApprovalMode { diff --git a/images/virtualization-artifact/pkg/common/vm/vm_test.go b/images/virtualization-artifact/pkg/common/vm/vm_test.go index e7ba2156d3..0d262dd9cb 100644 --- a/images/virtualization-artifact/pkg/common/vm/vm_test.go +++ b/images/virtualization-artifact/pkg/common/vm/vm_test.go @@ -27,45 +27,50 @@ import ( func TestCalculateCoresAndSockets(t *testing.T) { tests := []struct { - desiredCores int - sockets int - cores int + desiredCores int + sockets int + cores int + coresPerSocket int }{ - {-1, 1, 1}, - {1, 1, 1}, - {2, 1, 2}, - {3, 1, 3}, - {4, 1, 4}, - {5, 1, 5}, - {15, 1, 15}, - {16, 1, 16}, - - {18, 2, 9}, - {19, 2, 10}, - {20, 2, 10}, - {31, 2, 16}, - {32, 2, 16}, - - {36, 4, 9}, - {37, 4, 10}, - {40, 4, 10}, - {60, 4, 15}, - {63, 4, 16}, - {64, 4, 16}, - - {72, 8, 9}, - {76, 8, 10}, - {80, 8, 10}, - {248, 8, 31}, - {256, 8, 32}, - {252, 8, 32}, + {-1, 1, 1, 16}, + {1, 1, 1, 16}, + {2, 1, 2, 16}, + {3, 1, 3, 16}, + {4, 1, 4, 16}, + {5, 1, 5, 16}, + {15, 1, 15, 16}, + {16, 1, 16, 16}, + + {18, 2, 9, 16}, + {19, 2, 10, 16}, + {20, 2, 10, 16}, + {31, 2, 16, 16}, + {32, 2, 16, 16}, + + {36, 4, 9, 16}, + {37, 4, 10, 16}, + {40, 4, 10, 16}, + {60, 4, 15, 16}, + {63, 4, 16, 16}, + {64, 4, 16, 16}, + + {72, 8, 9, 31}, + {76, 8, 10, 31}, + {80, 8, 10, 31}, + {248, 8, 31, 31}, + {252, 8, 32, 31}, + {256, 8, 32, 31}, } for _, test := range tests { t.Run("", func(t *testing.T) { - sockets, cores := CalculateCoresAndSockets(test.desiredCores) + sockets, cores, coresPerSocket := CalculateCoresAndSockets(test.desiredCores) if cores != test.cores && sockets != test.sockets { - t.Errorf("For %d cores, expected %d sockets and %d cores, got %d sockets and %d cores", test.desiredCores, test.sockets, test.cores, sockets, cores) + t.Errorf("For %d cores, expected topology %ds/%dc/%dmax, got %ds/%dc/%dmax", + test.desiredCores, + test.sockets, test.cores, test.coresPerSocket, + sockets, cores, coresPerSocket, + ) } }) } diff --git a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm.go b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm.go index 1a55e10b9b..6eeddc1ad9 100644 --- a/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm.go +++ b/images/virtualization-artifact/pkg/controller/kvbuilder/kvvm.go @@ -300,7 +300,7 @@ func (b *KVVM) setCPUNonHotpluggable(cores int, coreFraction string) error { domainSpec.Resources.Requests[corev1.ResourceCPU] = *cpuRequest domainSpec.Resources.Limits[corev1.ResourceCPU] = *cpuLimit - socketsNeeded, coresNeeded := vm.CalculateCoresAndSockets(cores) + socketsNeeded, coresNeeded, _ := vm.CalculateCoresAndSockets(cores) domainSpec.CPU.Cores = uint32(coresNeeded) domainSpec.CPU.Sockets = uint32(socketsNeeded) @@ -324,13 +324,13 @@ func (b *KVVM) setCPUHotpluggable(cores int, coreFraction string) error { } b.SetKVVMIAnnotation(CPUResourcesRequestsFractionAnnotation, strconv.Itoa(fraction)) - socketsNeeded, coresPerSocketNeeded := vm.CalculateCoresAndSockets(cores) + socketsNeeded, coresPerSocketNeeded, maxCoresPerSocketNeeded := vm.CalculateCoresAndSockets(cores) // Use "dynamic cores" hotplug strategy. // Workaround: swap cores and sockets in domainSpec to bypass vm-validator webhook. b.SetKVVMIAnnotation(VCPUTopologyDynamicCoresAnnotation, "") domainSpec.CPU.Cores = uint32(socketsNeeded) domainSpec.CPU.Sockets = uint32(coresPerSocketNeeded) - domainSpec.CPU.MaxSockets = CPUMaxCoresPerSocket + domainSpec.CPU.MaxSockets = uint32(maxCoresPerSocketNeeded) // Remove CPU limits and requests if set by previous implementation. res := &b.Resource.Spec.Template.Spec.Domain.Resources diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go b/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go index e77a10209e..4d37f8e43b 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/statistic.go @@ -99,7 +99,7 @@ func (h *StatisticHandler) syncResources(changed *v1alpha2.VirtualMachine, ) if kvvmi == nil { memorySize = changed.Spec.Memory.Size - sockets, coresPerSocket := vm.CalculateCoresAndSockets(changed.Spec.CPU.Cores) + sockets, coresPerSocket, _ := vm.CalculateCoresAndSockets(changed.Spec.CPU.Cores) topology = v1alpha2.Topology{CoresPerSocket: coresPerSocket, Sockets: sockets} coreFraction = changed.Spec.CPU.CoreFraction } else { @@ -282,7 +282,7 @@ func (h *StatisticHandler) getCurrentTopologyByKVVMI(kvvmi *virtv1.VirtualMachin } cores = h.getCoresByKVVMI(kvvmi) - sockets, coresPerSocket := vm.CalculateCoresAndSockets(cores) + sockets, coresPerSocket, _ := vm.CalculateCoresAndSockets(cores) return v1alpha2.Topology{CoresPerSocket: coresPerSocket, Sockets: sockets} } 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 56e7250a2d..dbbd4db4be 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/sync_kvvm.go @@ -21,6 +21,7 @@ import ( "errors" "fmt" "reflect" + "strings" "time" corev1 "k8s.io/api/core/v1" @@ -215,10 +216,15 @@ func (h *SyncKvvmHandler) Handle(ctx context.Context, s state.VirtualMachineStat Status(metav1.ConditionFalse). Reason(vmcondition.ReasonConfigurationNotApplied). Message("Waiting for the user to restart in order to apply the configuration changes.") + restartMessages := changes.GetRestartMessages() + additionalMessage := "" + if len(restartMessages) > 0 { + additionalMessage = strings.Join(restartMessages, " ") + } cbAwaitingRestart. Status(metav1.ConditionTrue). Reason(vmcondition.ReasonChangesPendingRestart). - Message("Waiting for the user to restart in order to apply the configuration changes.") + Message("Waiting for the user to restart in order to apply the configuration changes. " + additionalMessage) case classChanged: h.recorder.Event(current, corev1.EventTypeNormal, v1alpha2.ReasonErrRestartAwaitingChanges, "Restart required to propagate changes from the vmclass spec") cbConfApplied. diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator.go index 17c2de3639..ace4048e3a 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator.go @@ -43,7 +43,11 @@ func (v *CPUCountValidator) ValidateUpdate(_ context.Context, _, newVM *v1alpha2 func (v *CPUCountValidator) Validate(vm *v1alpha2.VirtualMachine) (admission.Warnings, error) { cores := vm.Spec.CPU.Cores - sockets, coresPerSocket := commonvm.CalculateCoresAndSockets(cores) + if cores > commonvm.MaxCores { + return nil, fmt.Errorf("number of cores should not exceed %d", commonvm.MaxCores) + } + + sockets, coresPerSocket, _ := commonvm.CalculateCoresAndSockets(cores) if cores == sockets*coresPerSocket { return nil, nil diff --git a/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator_test.go b/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator_test.go index 7d1da9b743..dae4cd8cce 100644 --- a/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator_test.go +++ b/images/virtualization-artifact/pkg/controller/vm/internal/validators/cpu_count_validator_test.go @@ -28,6 +28,7 @@ func TestCpuCountValidate(t *testing.T) { desiredCores int valid bool }{ + // 1 socket topology {1, true}, {2, true}, {3, true}, @@ -36,12 +37,14 @@ func TestCpuCountValidate(t *testing.T) { {15, true}, {16, true}, + // 2 sockets topology {18, true}, {19, false}, {20, true}, {31, false}, {32, true}, + // 4 sockets topology {36, true}, {37, false}, {40, true}, @@ -49,12 +52,16 @@ func TestCpuCountValidate(t *testing.T) { {63, false}, {64, true}, + // 8 sockets topology {72, true}, {76, false}, {80, true}, + // Maximum cores count. {248, true}, - {256, true}, + + // Beyond maximum. {252, false}, + {256, false}, } for i, test := range tests { diff --git a/images/virtualization-artifact/pkg/controller/vmchange/comparator_cpu.go b/images/virtualization-artifact/pkg/controller/vmchange/comparator_cpu.go index 945f14a48e..ef3c588985 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/comparator_cpu.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/comparator_cpu.go @@ -38,12 +38,15 @@ func NewComparatorCPU(featureGate featuregate.FeatureGate) VMSpecFieldComparator // // It supports CPU hotplug mechanism for cores changes. // // It requires reboot if cpu fraction is changed or if COU hotplug is disabled. func (c *comparatorCPU) Compare(current, desired *v1alpha2.VirtualMachineSpec) []FieldChange { + coresRestartMsg := "" + // Cores can be changed "on the fly" using CPU Hotplug ... coresChangedAction := ActionApplyImmediate // ... but sockets count change requires a reboot. - currentSockets, _ := vm.CalculateCoresAndSockets(current.CPU.Cores) - desiredSockets, _ := vm.CalculateCoresAndSockets(desired.CPU.Cores) + currentSockets, _, _ := vm.CalculateCoresAndSockets(current.CPU.Cores) + desiredSockets, _, _ := vm.CalculateCoresAndSockets(desired.CPU.Cores) if currentSockets != desiredSockets { + coresRestartMsg = "Changing the number of CPU cores requires changing the CPU topology (number of sockets)." coresChangedAction = ActionRestart } @@ -53,6 +56,9 @@ func (c *comparatorCPU) Compare(current, desired *v1alpha2.VirtualMachineSpec) [ } coresChanges := compareInts("cpu.cores", current.CPU.Cores, desired.CPU.Cores, 0, coresChangedAction) + if HasChanges(coresChanges) && coresRestartMsg != "" { + coresChanges[0].RestartMessage = coresRestartMsg + } fractionChanges := compareStrings("cpu.coreFraction", current.CPU.CoreFraction, desired.CPU.CoreFraction, DefaultCPUCoreFraction, ActionRestart) // Yield full replace if both fields changed. @@ -64,6 +70,7 @@ func (c *comparatorCPU) Compare(current, desired *v1alpha2.VirtualMachineSpec) [ CurrentValue: current.CPU, DesiredValue: desired.CPU, ActionRequired: ActionRestart, + RestartMessage: coresRestartMsg, }, } } diff --git a/images/virtualization-artifact/pkg/controller/vmchange/field_change.go b/images/virtualization-artifact/pkg/controller/vmchange/field_change.go index 60fa5602b0..c2ac2feea2 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/field_change.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/field_change.go @@ -40,6 +40,7 @@ type FieldChange struct { DesiredValue interface{} `json:"desiredValue,omitempty"` ActionRequired ActionType `json:"-"` + RestartMessage string `json:"-"` } func HasChanges(changes []FieldChange) bool { diff --git a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go b/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go index 857b1c1312..b9ed736b99 100644 --- a/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go +++ b/images/virtualization-artifact/pkg/controller/vmchange/spec_changes.go @@ -218,6 +218,16 @@ func (s *SpecChanges) GetAll() []FieldChange { return s.changes } +func (s *SpecChanges) GetRestartMessages() []string { + msgs := make([]string, 0) + for _, change := range s.changes { + if change.RestartMessage != "" { + msgs = append(msgs, change.RestartMessage) + } + } + return msgs +} + func (s *SpecChanges) ConvertPendingChanges() ([]apiextensionsv1.JSON, error) { res := make([]apiextensionsv1.JSON, 0, len(s.changes)) for i := range s.changes {