From 27299cb97b885885b5f333b93c2fd4b132773e0c Mon Sep 17 00:00:00 2001 From: Misha Sakhnov Date: Tue, 19 Nov 2024 10:22:18 +0400 Subject: [PATCH] neonvm: apply code review fixes Co-authored-by: Em Sharnoff Signed-off-by: Misha Sakhnov --- neonvm-daemon/cmd/main.go | 10 +++++-- pkg/neonvm/controllers/vm_controller.go | 9 +++--- .../controllers/vm_controller_cpu_scaling.go | 1 - pkg/neonvm/cpuscaling/sysfs.go | 28 ++++++++++++------- 4 files changed, 30 insertions(+), 18 deletions(-) diff --git a/neonvm-daemon/cmd/main.go b/neonvm-daemon/cmd/main.go index f36f27b97..4a675e688 100644 --- a/neonvm-daemon/cmd/main.go +++ b/neonvm-daemon/cmd/main.go @@ -51,6 +51,7 @@ type cpuServer struct { func (s *cpuServer) handleGetCPUStatus(w http.ResponseWriter) { s.cpuOperationsMutex.Lock() defer s.cpuOperationsMutex.Unlock() + activeCPUs, err := s.cpuScaler.ActiveCPUsCount() if err != nil { s.logger.Error("could not get active CPUs count", zap.Error(err)) @@ -58,6 +59,7 @@ func (s *cpuServer) handleGetCPUStatus(w http.ResponseWriter) { return } w.WriteHeader(http.StatusOK) + if _, err := w.Write([]byte(fmt.Sprintf("%d", activeCPUs*1000))); err != nil { s.logger.Error("could not write response", zap.Error(err)) } @@ -66,20 +68,24 @@ func (s *cpuServer) handleGetCPUStatus(w http.ResponseWriter) { func (s *cpuServer) handleSetCPUStatus(w http.ResponseWriter, r *http.Request) { s.cpuOperationsMutex.Lock() defer s.cpuOperationsMutex.Unlock() + body, err := io.ReadAll(r.Body) if err != nil { s.logger.Error("could not read request body", zap.Error(err)) w.WriteHeader(http.StatusBadRequest) return } + defer r.Body.Close() + updateInt, err := strconv.Atoi(string(body)) - update := vmv1.MilliCPU(updateInt) if err != nil { s.logger.Error("could not unmarshal request body", zap.Error(err)) w.WriteHeader(http.StatusBadRequest) return } - s.logger.Info("Setting CPU status", zap.String("body", string(body))) + + s.logger.Debug("Setting CPU status", zap.String("body", string(body))) + update := vmv1.MilliCPU(updateInt) if err := s.cpuScaler.ReconcileOnlineCPU(int(update.RoundedUp())); err != nil { s.logger.Error("could not ensure online CPUs", zap.Error(err)) w.WriteHeader(http.StatusInternalServerError) diff --git a/pkg/neonvm/controllers/vm_controller.go b/pkg/neonvm/controllers/vm_controller.go index 059116a95..12b2497f0 100644 --- a/pkg/neonvm/controllers/vm_controller.go +++ b/pkg/neonvm/controllers/vm_controller.go @@ -78,7 +78,8 @@ type VMReconciler struct { Scheme *runtime.Scheme Recorder record.EventRecorder Config *ReconcilerConfig - Metrics ReconcilerMetrics `exhaustruct:"optional"` + + Metrics ReconcilerMetrics `exhaustruct:"optional"` } // The following markers are used to generate the rules permissions (RBAC) on config/rbac using controller-gen @@ -162,9 +163,9 @@ func (r *VMReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Re } // examine cpuScalingMode and set it to the default value if it is not set - if vm.Spec.CpuScalingMode == nil || *vm.Spec.CpuScalingMode == "" { + if vm.Spec.CpuScalingMode == nil { log.Info("Setting default CPU scaling mode", "default", r.Config.DefaultCPUScalingMode) - vm.Spec.CpuScalingMode = lo.ToPtr(vmv1.CpuScalingMode(r.Config.DefaultCPUScalingMode)) + vm.Spec.CpuScalingMode = lo.ToPtr(r.Config.DefaultCPUScalingMode) if err := r.tryUpdateVM(ctx, &vm); err != nil { log.Error(err, "Failed to set default CPU scaling mode") return ctrl.Result{}, err @@ -576,10 +577,8 @@ func (r *VMReconciler) doReconcile(ctx context.Context, vm *vmv1.VirtualMachine) switch *vm.Spec.CpuScalingMode { case vmv1.CpuScalingModeSysfs: - log.Info("CPU usage check based on cgroups", "CpuScalingMode", *vm.Spec.CpuScalingMode) pluggedCPU = cgroupUsage.VCPUs.RoundedUp() case vmv1.CpuScalingModeQMP: - log.Info("CPU usage check based on QMP", "CpuScalingMode", *vm.Spec.CpuScalingMode) cpuSlotsPlugged, _, err := QmpGetCpus(QmpAddr(vm)) if err != nil { log.Error(err, "Failed to get CPU details from VirtualMachine", "VirtualMachine", vm.Name) diff --git a/pkg/neonvm/controllers/vm_controller_cpu_scaling.go b/pkg/neonvm/controllers/vm_controller_cpu_scaling.go index e4f3c3b4a..9266ae820 100644 --- a/pkg/neonvm/controllers/vm_controller_cpu_scaling.go +++ b/pkg/neonvm/controllers/vm_controller_cpu_scaling.go @@ -86,7 +86,6 @@ func (r *VMReconciler) handleCPUScalingQMP(ctx context.Context, vm *vmv1.Virtual return false, err } } else { - log.Info("No need to plug or unplug CPU") hotPlugCPUScaled = true } diff --git a/pkg/neonvm/cpuscaling/sysfs.go b/pkg/neonvm/cpuscaling/sysfs.go index 236f1a703..88813cbc3 100644 --- a/pkg/neonvm/cpuscaling/sysfs.go +++ b/pkg/neonvm/cpuscaling/sysfs.go @@ -32,21 +32,29 @@ func (cs *cpuSysfsState) SetState(cpuNum int, cpuState cpuState) error { } func (cs *cpuSysfsState) GetState(cpuNum int) (cpuState, error) { - data, err := os.ReadFile(filepath.Join(cpuPath, "online")) + onlineCPUs, err := cs.getAllOnlineCPUs() if err != nil { return cpuOffline, err } + if slices.Contains(onlineCPUs, cpuNum) { + return cpuOnline, nil + } - onlineCPUs, err := cs.parseMultipleCPURange(string(data)) + return cpuOffline, nil +} + +func (cs *cpuSysfsState) getAllOnlineCPUs() ([]int, error) { + data, err := os.ReadFile(filepath.Join(cpuPath, "online")) if err != nil { - return cpuOffline, err + return nil, err } - if slices.Contains(onlineCPUs, cpuNum) { - return cpuOnline, nil + onlineCPUs, err := cs.parseMultipleCPURange(string(data)) + if err != nil { + return onlineCPUs, err } - return cpuOffline, nil + return onlineCPUs, nil } // PossibleCPUs returns the start and end indexes of all possible CPUs. @@ -59,7 +67,7 @@ func (cs *cpuSysfsState) PossibleCPUs() (int, int, error) { return cs.parseCPURange(string(data)) } -// parseCPURange parses the CPU range string (e.g., "0-3") and returns a list of CPUs. +// parseCPURange parses the CPU range string (e.g., "0-3") and returns start and end indexes. func (cs *cpuSysfsState) parseCPURange(cpuRange string) (int, int, error) { cpuRange = strings.TrimSpace(cpuRange) parts := strings.Split(cpuRange, "-") @@ -86,9 +94,9 @@ func (cs *cpuSysfsState) parseCPURange(cpuRange string) (int, int, error) { } // parseMultipleCPURange parses the multiple CPU range string (e.g., "0-3,5-7") and returns a list of CPUs. -func (cs *cpuSysfsState) parseMultipleCPURange(cpuRange string) ([]int, error) { - cpuRange = strings.TrimSpace(cpuRange) - parts := strings.Split(cpuRange, ",") +func (cs *cpuSysfsState) parseMultipleCPURange(cpuRanges string) ([]int, error) { + cpuRanges = strings.TrimSpace(cpuRanges) + parts := strings.Split(cpuRanges, ",") var cpus []int for _, part := range parts {