WIP: neonvm: apply code review fixes, drop condition for dummy server…

…, always enable it Signed-off-by: Misha Sakhnov <[email protected]>
neondatabase · Oct 21, 2024 · 56fcb36 · 56fcb36
1 parent 2515797
commit 56fcb36
Show file tree

Hide file tree

Showing 2 changed files with 19 additions and 67 deletions.
diff --git a/neonvm-runner/cmd/main.go b/neonvm-runner/cmd/main.go
@@ -20,7 +20,6 @@ import (
 	"os/signal"
 	"path/filepath"
 	"regexp"
-	"strconv"
 	"strings"
 	"sync"
 	"sync/atomic"
@@ -612,7 +611,6 @@ type Config struct {
 	kernelPath           string
 	appendKernelCmdline  string
 	skipCgroupManagement bool
-	enableDummyCPUServer bool
 	diskCacheSettings    string
 	memoryProvider       vmv1.MemoryProvider
 	autoMovableRatio     string
@@ -626,7 +624,6 @@ func newConfig(logger *zap.Logger) *Config {
 		kernelPath:           defaultKernelPath,
 		appendKernelCmdline:  "",
 		skipCgroupManagement: false,
-		enableDummyCPUServer: false,
 		diskCacheSettings:    "cache=none",
 		memoryProvider:       "", // Require that this is explicitly set. We'll check later.
 		autoMovableRatio:     "", // Require that this is explicitly set IFF memoryProvider is VirtioMem. We'll check later.
@@ -642,9 +639,6 @@ func newConfig(logger *zap.Logger) *Config {
 	flag.BoolVar(&cfg.skipCgroupManagement, "skip-cgroup-management",
 		cfg.skipCgroupManagement,
 		"Don't try to manage CPU")
-	flag.BoolVar(&cfg.enableDummyCPUServer, "enable-dummy-cpu-server",
-		cfg.skipCgroupManagement,
-		"Use with -skip-cgroup-management. Provide a CPU server but don't actually do anything with it")
 	flag.StringVar(&cfg.diskCacheSettings, "qemu-disk-cache-settings",
 		cfg.diskCacheSettings, "Cache settings to add to -drive args for VM disks")
 	flag.Func("memory-provider", "Set provider for memory hotplug", cfg.memoryProvider.FlagFunc)
@@ -662,9 +656,6 @@ func newConfig(logger *zap.Logger) *Config {
 	if cfg.cpuScalingMode == "" {
 		logger.Fatal("missing required flag '-cpu-scaling-mode'")
 	}
-	if cfg.enableDummyCPUServer && !cfg.skipCgroupManagement {
-		logger.Fatal("flag -enable-dummy-cpu-server requires -skip-cgroup-management")
-	}
 
 	return cfg
 }
@@ -1058,39 +1049,27 @@ func runQEMU(
 
 	wg.Add(1)
 	go terminateQemuOnSigterm(ctx, logger, &wg)
-	if !cfg.skipCgroupManagement || cfg.enableDummyCPUServer || cfg.cpuScalingMode == vmv1.CpuScalingModeCpuSysfsState {
+	if !cfg.skipCgroupManagement || cfg.cpuScalingMode == vmv1.CpuScalingModeCpuSysfsState {
 		var callbacks cpuServerCallbacks
 
-		if cfg.enableDummyCPUServer {
-			lastValue := &atomic.Uint32{}
-			lastValue.Store(uint32(vmSpec.Guest.CPUs.Min))
-
-			callbacks = cpuServerCallbacks{
-				get: func(logger *zap.Logger) (*vmv1.MilliCPU, error) {
-					return lo.ToPtr(vmv1.MilliCPU(lastValue.Load())), nil
-				},
-				set: func(logger *zap.Logger, cpu vmv1.MilliCPU) error {
-					if cfg.cpuScalingMode == vmv1.CpuScalingModeCpuSysfsState {
-						err := setNeonvmDaemonCPU(cpu)
-						if err != nil {
-							logger.Error("setting CPU through NeonVM Daemon failed", zap.Any("cpu", cpu), zap.Error(err))
-							return err
-						}
+		lastValue := &atomic.Uint32{}
+		lastValue.Store(uint32(vmSpec.Guest.CPUs.Min))
+
+		callbacks = cpuServerCallbacks{
+			get: func(logger *zap.Logger) (*vmv1.MilliCPU, error) {
+				return lo.ToPtr(vmv1.MilliCPU(lastValue.Load())), nil
+			},
+			set: func(logger *zap.Logger, cpu vmv1.MilliCPU) error {
+				if cfg.cpuScalingMode == vmv1.CpuScalingModeCpuSysfsState {
+					err := setNeonvmDaemonCPU(cpu)
+					if err != nil {
+						logger.Error("setting CPU through NeonVM Daemon failed", zap.Any("cpu", cpu), zap.Error(err))
+						return err
 					}
-					lastValue.Store(uint32(cpu))
-					return nil
-				},
-			}
-		} else {
-			// Standard implementation -- actually set the cgroup
-			callbacks = cpuServerCallbacks{
-				get: func(logger *zap.Logger) (*vmv1.MilliCPU, error) {
-					return getCgroupQuota(cgroupPath)
-				},
-				set: func(logger *zap.Logger, cpu vmv1.MilliCPU) error {
-					return setCgroupLimit(logger, cpu, cgroupPath)
-				},
-			}
+				}
+				lastValue.Store(uint32(cpu))
+				return nil
+			},
 		}
 
 		wg.Add(1)
@@ -1498,32 +1477,6 @@ func setCgroupLimit(logger *zap.Logger, r vmv1.MilliCPU, cgroupPath string) erro
 	return nil
 }
 
-func getCgroupQuota(cgroupPath string) (*vmv1.MilliCPU, error) {
-	isV2 := cgroups.Mode() == cgroups.Unified
-	var path string
-	if isV2 {
-		path = filepath.Join(cgroupMountPoint, cgroupPath, "cpu.max")
-	} else {
-		path = filepath.Join(cgroupMountPoint, "cpu", cgroupPath, "cpu.cfs_quota_us")
-	}
-	data, err := os.ReadFile(path)
-	if err != nil {
-		return nil, err
-	}
-
-	arr := strings.Split(strings.Trim(string(data), "\n"), " ")
-	if len(arr) == 0 {
-		return nil, errors.New("unexpected cgroup data")
-	}
-	quota, err := strconv.ParseUint(arr[0], 10, 64)
-	if err != nil {
-		return nil, err
-	}
-	cpu := vmv1.MilliCPU(uint32(quota * 1000 / cgroupPeriod))
-	cpu /= cpuLimitOvercommitFactor
-	return &cpu, nil
-}
-
 func terminateQemuOnSigterm(ctx context.Context, logger *zap.Logger, wg *sync.WaitGroup) {
 	logger = logger.Named("terminate-qemu-on-sigterm")
 

diff --git a/pkg/neonvm/controllers/vm_controller.go b/pkg/neonvm/controllers/vm_controller.go
@@ -1415,10 +1415,9 @@ func podSpec(
 					Command: func() []string {
 						cmd := []string{"runner"}
 						if config.DisableRunnerCgroup {
-							cmd = append(cmd, "-skip-cgroup-management")
 							// cgroup management disabled, but we still need something to provide
 							// the server, so the runner will just provide a dummy implementation.
-							cmd = append(cmd, "-enable-dummy-cpu-server")
+							cmd = append(cmd, "-skip-cgroup-management")
 						}
 						cmd = append(
 							cmd,