From 4788b75fcf828d120485a40fc0ed08e78cc21735 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 10 Mar 2020 16:06:11 -0700 Subject: [PATCH 1/7] Round suggested memory alloc by 100MB for VM's --- cmd/minikube/cmd/start.go | 72 ++++++++++++++++++++++++++++------ cmd/minikube/cmd/start_test.go | 33 +++++++++++++++- 2 files changed, 91 insertions(+), 14 deletions(-) diff --git a/cmd/minikube/cmd/start.go b/cmd/minikube/cmd/start.go index 758deb59a2..3d682b3e8c 100644 --- a/cmd/minikube/cmd/start.go +++ b/cmd/minikube/cmd/start.go @@ -642,27 +642,68 @@ func validateUser(drvName string) { } } -// defaultMemorySize calculates the default memory footprint in MB -func defaultMemorySize(drvName string) int { - fallback := 2200 - maximum := 6000 - +// memoryLimits returns the amount of memory allocated to the system and hypervisor +func memoryLimits(drvName string) (int, int, error) { v, err := mem.VirtualMemory() if err != nil { - return fallback + return -1, -1, err } - available := v.Total / 1024 / 1024 + sysLimit := int(v.Total / 1024 / 1024) + containerLimit := 0 - // For KIC, do not allocate more memory than the container has available (+ some slack) if driver.IsKIC(drvName) { s, err := oci.DaemonInfo(drvName) if err != nil { - return fallback + return -1, -1, err } - maximum = int(s.TotalMemory/1024/1024) - 128 + containerLimit = int(s.TotalMemory / 1024 / 1024) + } + return sysLimit, containerLimit, nil +} + +// suggestMemoryAllocation calculates the default memory footprint in MB +func suggestMemoryAllocation(sysLimit int, containerLimit int) int { + fallback := 2200 + maximum := 6000 + + if sysLimit > 0 && fallback > sysLimit { + return sysLimit } - suggested := int(available / 4) + // If there are container limits, add tiny bit of slack for non-minikube components + if containerLimit > 0 { + if fallback > containerLimit { + return containerLimit + } + maximum = containerLimit - 48 + } + + // Suggest 25% of RAM, rounded to nearest 100MB. Hyper-V requires an even number! + suggested := int(sysLimit/400.0) * 100 + + if suggested > maximum { + return maximum + } + + if suggested < fallback { + return fallback + } + + return suggested +} + +// defaultMemorySize calculates the default memory footprint in MB +func suggest(drvName string, available int) int { + fallback := 2200 + maximum := 6000 + + // For KIC, do not allocate more memory than the container has available (+ some slack) + if driver.IsKIC(drvName) { + maximum = available - 100 + } + + // Suggest 25% of RAM, rounded to nearest 100MB + suggested := int(available/400.0) * 100 if suggested > maximum { suggested = maximum @@ -821,9 +862,16 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) kubeNodeName = "m01" } - mem := defaultMemorySize(drvName) + sysLimit, containerLimit, err := memoryLimits(drvName) + if err != nil { + glog.Warningf("Unable to query memory limits: %v", err) + } + + mem := suggestMemoryAllocation(sysLimit, containerLimit) if viper.GetString(memory) != "" { mem = pkgutil.CalculateSizeInMB(viper.GetString(memory)) + } else { + glog.Infof("Using suggested %dMB memory alloc based on sys=%dMB, container=%dMB", mem, sysLimit, containerLimit) } // Create the initial node, which will necessarily be a control plane diff --git a/cmd/minikube/cmd/start_test.go b/cmd/minikube/cmd/start_test.go index b664e3486b..43f22d87f6 100644 --- a/cmd/minikube/cmd/start_test.go +++ b/cmd/minikube/cmd/start_test.go @@ -71,8 +71,6 @@ func TestGetKuberneterVersion(t *testing.T) { } func TestGenerateCfgFromFlagsHTTPProxyHandling(t *testing.T) { - viper.SetDefault(memory, defaultMemorySize) - viper.SetDefault(humanReadableDiskSize, defaultDiskSize) originalEnv := os.Getenv("HTTP_PROXY") defer func() { err := os.Setenv("HTTP_PROXY", originalEnv) @@ -124,3 +122,34 @@ func TestGenerateCfgFromFlagsHTTPProxyHandling(t *testing.T) { }) } } + +func TestSuggestMemoryAllocation(t *testing.T) { + var tests = []struct { + description string + sysLimit int + containerLimit int + want int + }{ + {"128GB sys", 128000, 0, 6000}, + {"64GB sys", 64000, 0, 6000}, + {"16GB sys", 16384, 0, 4000}, + {"odd sys", 14567, 0, 3600}, + {"4GB sys", 4096, 0, 2200}, + {"2GB sys", 2048, 0, 2048}, + {"Unable to poll sys", 0, 0, 2200}, + {"128GB sys, 16GB container", 128000, 16384, 16336}, + {"64GB sys, 16GB container", 64000, 16384, 16000}, + {"16GB sys, 4GB container", 16384, 4096, 4000}, + {"4GB sys, 3.5GB container", 16384, 3500, 3452}, + {"2GB sys, 2GB container", 16384, 2048, 2048}, + {"2GB sys, unable to poll container", 16384, 0, 4000}, + } + for _, test := range tests { + t.Run(test.description, func(t *testing.T) { + got := suggestMemoryAllocation(test.sysLimit, test.containerLimit) + if got != test.want { + t.Errorf("defaultMemorySize(sys=%d, container=%d) = %d, want: %d", test.sysLimit, test.containerLimit, got, test.want) + } + }) + } +} From f1d2127498b696607f49a36a84186126a08fc1ad Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 10 Mar 2020 16:44:50 -0700 Subject: [PATCH 2/7] Only parse memory if set --- cmd/minikube/cmd/start.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/minikube/cmd/start.go b/cmd/minikube/cmd/start.go index 3d682b3e8c..505b98dd7e 100644 --- a/cmd/minikube/cmd/start.go +++ b/cmd/minikube/cmd/start.go @@ -868,7 +868,7 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) } mem := suggestMemoryAllocation(sysLimit, containerLimit) - if viper.GetString(memory) != "" { + if cmd.Flags().Changed(memory) { mem = pkgutil.CalculateSizeInMB(viper.GetString(memory)) } else { glog.Infof("Using suggested %dMB memory alloc based on sys=%dMB, container=%dMB", mem, sysLimit, containerLimit) From c6333b95aa842ed24bcb3f890e0439c12eca6e47 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 10 Mar 2020 17:43:56 -0700 Subject: [PATCH 3/7] Test fixes: Add viper.SetDefault back, and make CalculateSizeInMB return an error instead of exit early --- cmd/minikube/cmd/start.go | 57 ++++++++++++++-------------------- cmd/minikube/cmd/start_test.go | 2 ++ pkg/addons/addons.go | 5 ++- pkg/util/utils.go | 8 ++--- pkg/util/utils_test.go | 5 ++- 5 files changed, 37 insertions(+), 40 deletions(-) diff --git a/cmd/minikube/cmd/start.go b/cmd/minikube/cmd/start.go index 505b98dd7e..7591f2b55b 100644 --- a/cmd/minikube/cmd/start.go +++ b/cmd/minikube/cmd/start.go @@ -116,7 +116,7 @@ const ( minUsableMem = 1024 // Kubernetes will not start with less than 1GB minRecommendedMem = 2000 // Warn at no lower than existing configurations minimumCPUS = 2 - minimumDiskSize = "2000mb" + minimumDiskSize = 2000 autoUpdate = "auto-update-drivers" hostOnlyNicType = "host-only-nic-type" natNicType = "nat-nic-type" @@ -679,7 +679,7 @@ func suggestMemoryAllocation(sysLimit int, containerLimit int) int { } // Suggest 25% of RAM, rounded to nearest 100MB. Hyper-V requires an even number! - suggested := int(sysLimit/400.0) * 100 + suggested := int(float32(sysLimit)/400.0) * 100 if suggested > maximum { return maximum @@ -692,34 +692,12 @@ func suggestMemoryAllocation(sysLimit int, containerLimit int) int { return suggested } -// defaultMemorySize calculates the default memory footprint in MB -func suggest(drvName string, available int) int { - fallback := 2200 - maximum := 6000 - - // For KIC, do not allocate more memory than the container has available (+ some slack) - if driver.IsKIC(drvName) { - maximum = available - 100 - } - - // Suggest 25% of RAM, rounded to nearest 100MB - suggested := int(available/400.0) * 100 - - if suggested > maximum { - suggested = maximum - } - - if suggested < fallback { - suggested = fallback - } - - glog.Infof("Selecting memory default of %dMB, given %dMB available and %dMB maximum", suggested, available, maximum) - return suggested -} - // validateMemorySize validates the memory size matches the minimum recommended func validateMemorySize() { - req := pkgutil.CalculateSizeInMB(viper.GetString(memory)) + req, err := pkgutil.CalculateSizeInMB(viper.GetString(memory)) + if err != nil { + exit.WithCodeT(exit.Config, "Unable to parse memory '{{.memory}}': {{.error}}", out.V{"memory": viper.GetString(memory), "error": err}) + } if req < minUsableMem && !viper.GetBool(force) { exit.WithCodeT(exit.Config, "Requested memory allocation {{.requested}}MB is less than the usable minimum of {{.minimum}}MB", out.V{"requested": req, "mininum": minUsableMem}) @@ -752,9 +730,13 @@ func validateCPUCount(local bool) { // validateFlags validates the supplied flags against known bad combinations func validateFlags(cmd *cobra.Command, drvName string) { if cmd.Flags().Changed(humanReadableDiskSize) { - diskSizeMB := pkgutil.CalculateSizeInMB(viper.GetString(humanReadableDiskSize)) - if diskSizeMB < pkgutil.CalculateSizeInMB(minimumDiskSize) && !viper.GetBool(force) { - exit.WithCodeT(exit.Config, "Requested disk size {{.requested_size}} is less than minimum of {{.minimum_size}}", out.V{"requested_size": diskSizeMB, "minimum_size": pkgutil.CalculateSizeInMB(minimumDiskSize)}) + diskSizeMB, err := pkgutil.CalculateSizeInMB(viper.GetString(humanReadableDiskSize)) + if err != nil { + exit.WithCodeT(exit.Config, "Validation unable to parse disk size '{{.diskSize}}': {{.error}}", out.V{"diskSize": viper.GetString(humanReadableDiskSize), "error": err}) + } + + if diskSizeMB < minimumDiskSize && !viper.GetBool(force) { + exit.WithCodeT(exit.Config, "Requested disk size {{.requested_size}} is less than minimum of {{.minimum_size}}", out.V{"requested_size": diskSizeMB, "minimum_size": minimumDiskSize}) } } @@ -869,7 +851,11 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) mem := suggestMemoryAllocation(sysLimit, containerLimit) if cmd.Flags().Changed(memory) { - mem = pkgutil.CalculateSizeInMB(viper.GetString(memory)) + mem, err = pkgutil.CalculateSizeInMB(viper.GetString(memory)) + if err != nil { + exit.WithCodeT(exit.Config, "Generate unable to parse memory '{{.memory}}': {{.error}}", out.V{"memory": viper.GetString(memory), "error": err}) + } + } else { glog.Infof("Using suggested %dMB memory alloc based on sys=%dMB, container=%dMB", mem, sysLimit, containerLimit) } @@ -883,6 +869,11 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) Worker: true, } + diskSize, err := pkgutil.CalculateSizeInMB(viper.GetString(humanReadableDiskSize)) + if err != nil { + exit.WithCodeT(exit.Config, "Generate unable to parse disk size '{{.diskSize}}': {{.error}}", out.V{"diskSize": viper.GetString(humanReadableDiskSize), "error": err}) + } + cfg := config.ClusterConfig{ Name: viper.GetString(config.ProfileName), KeepContext: viper.GetBool(keepContext), @@ -890,7 +881,7 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) MinikubeISO: viper.GetString(isoURL), Memory: mem, CPUs: viper.GetInt(cpus), - DiskSize: pkgutil.CalculateSizeInMB(viper.GetString(humanReadableDiskSize)), + DiskSize: diskSize, Driver: drvName, HyperkitVpnKitSock: viper.GetString(vpnkitSock), HyperkitVSockPorts: viper.GetStringSlice(vsockPorts), diff --git a/cmd/minikube/cmd/start_test.go b/cmd/minikube/cmd/start_test.go index 43f22d87f6..cb81d2fc1a 100644 --- a/cmd/minikube/cmd/start_test.go +++ b/cmd/minikube/cmd/start_test.go @@ -104,6 +104,8 @@ func TestGenerateCfgFromFlagsHTTPProxyHandling(t *testing.T) { }, } for _, test := range tests { + // Set default disk size value in lieu of flag init + viper.SetDefault(humanReadableDiskSize, defaultDiskSize) t.Run(test.description, func(t *testing.T) { cmd := &cobra.Command{} if err := os.Setenv("HTTP_PROXY", test.proxy); err != nil { diff --git a/pkg/addons/addons.go b/pkg/addons/addons.go index e320af75ac..9c64a28989 100644 --- a/pkg/addons/addons.go +++ b/pkg/addons/addons.go @@ -127,7 +127,10 @@ func enableOrDisableAddon(name, val, profile string) error { if name == "istio" && enable { minMem := 8192 minCpus := 4 - memorySizeMB := pkgutil.CalculateSizeInMB(viper.GetString("memory")) + memorySizeMB, err := pkgutil.CalculateSizeInMB(viper.GetString("memory")) + if err != nil { + return errors.Wrap(err, "calculate memory") + } cpuCount := viper.GetInt("cpus") if memorySizeMB < minMem || cpuCount < minCpus { out.WarningT("Enable istio needs {{.minMem}} MB of memory and {{.minCpus}} CPUs.", out.V{"minMem": minMem, "minCpus": minCpus}) diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 9ab0a57bb3..f6e87259b6 100644 --- a/pkg/util/utils.go +++ b/pkg/util/utils.go @@ -25,8 +25,6 @@ import ( units "github.com/docker/go-units" "github.com/pkg/errors" - "k8s.io/minikube/pkg/minikube/exit" - "k8s.io/minikube/pkg/minikube/out" ) const ( @@ -34,17 +32,17 @@ const ( ) // CalculateSizeInMB returns the number of MB in the human readable string -func CalculateSizeInMB(humanReadableSize string) int { +func CalculateSizeInMB(humanReadableSize string) (int, error) { _, err := strconv.ParseInt(humanReadableSize, 10, 64) if err == nil { humanReadableSize += "mb" } size, err := units.FromHumanSize(humanReadableSize) if err != nil { - exit.WithCodeT(exit.Config, "Invalid size passed in argument: {{.error}}", out.V{"error": err}) + return 0, fmt.Errorf("FromHumanSize: %v", err) } - return int(size / units.MB) + return int(size / units.MB), nil } // GetBinaryDownloadURL returns a suitable URL for the platform diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index b8a3514d28..7521fa3c73 100644 --- a/pkg/util/utils_test.go +++ b/pkg/util/utils_test.go @@ -52,7 +52,10 @@ func TestCalculateSizeInMB(t *testing.T) { } for _, tt := range testData { - number := CalculateSizeInMB(tt.size) + number, err := CalculateSizeInMB(tt.size) + if err != nil { + t.Fatalf("unexpected err: %v", err) + } if number != tt.expectedNumber { t.Fatalf("Expected '%d'' but got '%d'", tt.expectedNumber, number) } From d98bddac152b8e81adb1967668e96cc565fdb6e4 Mon Sep 17 00:00:00 2001 From: Thomas Stromberg Date: Tue, 10 Mar 2020 17:45:43 -0700 Subject: [PATCH 4/7] Set viper default in a better place --- cmd/minikube/cmd/start_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/cmd/minikube/cmd/start_test.go b/cmd/minikube/cmd/start_test.go index cb81d2fc1a..23675528b6 100644 --- a/cmd/minikube/cmd/start_test.go +++ b/cmd/minikube/cmd/start_test.go @@ -71,6 +71,9 @@ func TestGetKuberneterVersion(t *testing.T) { } func TestGenerateCfgFromFlagsHTTPProxyHandling(t *testing.T) { + // Set default disk size value in lieu of flag init + viper.SetDefault(humanReadableDiskSize, defaultDiskSize) + originalEnv := os.Getenv("HTTP_PROXY") defer func() { err := os.Setenv("HTTP_PROXY", originalEnv) @@ -104,8 +107,6 @@ func TestGenerateCfgFromFlagsHTTPProxyHandling(t *testing.T) { }, } for _, test := range tests { - // Set default disk size value in lieu of flag init - viper.SetDefault(humanReadableDiskSize, defaultDiskSize) t.Run(test.description, func(t *testing.T) { cmd := &cobra.Command{} if err := os.Setenv("HTTP_PROXY", test.proxy); err != nil { From a522175211afc79cbff4c6436699df1f8517434c Mon Sep 17 00:00:00 2001 From: Medya Gh Date: Tue, 10 Mar 2020 17:55:35 -0700 Subject: [PATCH 5/7] fix test containerd test for docker driver --- test/integration/start_stop_delete_test.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go index a7b11c8f42..007ae4b568 100644 --- a/test/integration/start_stop_delete_test.go +++ b/test/integration/start_stop_delete_test.go @@ -62,8 +62,6 @@ func TestStartStop(t *testing.T) { }}, {"containerd", constants.DefaultKubernetesVersion, []string{ "--container-runtime=containerd", - "--docker-opt", - "containerd=/var/run/containerd/containerd.sock", "--apiserver-port=8444", }}, {"crio", "v1.15.7", []string{ From 9ff0dc0ebc7040e76f73595c500e2ad4e20606d4 Mon Sep 17 00:00:00 2001 From: Medya Gh Date: Tue, 10 Mar 2020 18:33:59 -0700 Subject: [PATCH 6/7] pass api server port to kic create container --- pkg/drivers/kic/kic.go | 4 ++-- test/integration/start_stop_delete_test.go | 2 ++ 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/pkg/drivers/kic/kic.go b/pkg/drivers/kic/kic.go index 74f1123299..bba3c17eb1 100644 --- a/pkg/drivers/kic/kic.go +++ b/pkg/drivers/kic/kic.go @@ -81,7 +81,7 @@ func (d *Driver) Create() error { // control plane specific options params.PortMappings = append(params.PortMappings, oci.PortMapping{ ListenAddress: oci.DefaultBindIPV4, - ContainerPort: constants.APIServerPort, + ContainerPort: int32(params.APIServerPort), }, oci.PortMapping{ ListenAddress: oci.DefaultBindIPV4, @@ -202,7 +202,7 @@ func (d *Driver) GetSSHKeyPath() string { // GetURL returns ip of the container running kic control-panel func (d *Driver) GetURL() (string, error) { - p, err := oci.HostPortBinding(d.NodeConfig.OCIBinary, d.MachineName, d.NodeConfig.APIServerPort) + p, err := oci.HostPortBinding(d.NodeConfig.OCIBinary, d.MachineName, int(d.NodeConfig.APIServerPort)) url := fmt.Sprintf("https://%s", net.JoinHostPort("127.0.0.1", fmt.Sprint(p))) if err != nil { return url, errors.Wrap(err, "api host port binding") diff --git a/test/integration/start_stop_delete_test.go b/test/integration/start_stop_delete_test.go index 007ae4b568..a7b11c8f42 100644 --- a/test/integration/start_stop_delete_test.go +++ b/test/integration/start_stop_delete_test.go @@ -62,6 +62,8 @@ func TestStartStop(t *testing.T) { }}, {"containerd", constants.DefaultKubernetesVersion, []string{ "--container-runtime=containerd", + "--docker-opt", + "containerd=/var/run/containerd/containerd.sock", "--apiserver-port=8444", }}, {"crio", "v1.15.7", []string{ From b5681d52c5837daa18ce0e59e7382bc0ce125106 Mon Sep 17 00:00:00 2001 From: Medya Gh Date: Tue, 10 Mar 2020 18:45:56 -0700 Subject: [PATCH 7/7] lint --- pkg/drivers/kic/kic.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/drivers/kic/kic.go b/pkg/drivers/kic/kic.go index bba3c17eb1..267f41dbc6 100644 --- a/pkg/drivers/kic/kic.go +++ b/pkg/drivers/kic/kic.go @@ -202,7 +202,7 @@ func (d *Driver) GetSSHKeyPath() string { // GetURL returns ip of the container running kic control-panel func (d *Driver) GetURL() (string, error) { - p, err := oci.HostPortBinding(d.NodeConfig.OCIBinary, d.MachineName, int(d.NodeConfig.APIServerPort)) + p, err := oci.HostPortBinding(d.NodeConfig.OCIBinary, d.MachineName, d.NodeConfig.APIServerPort) url := fmt.Sprintf("https://%s", net.JoinHostPort("127.0.0.1", fmt.Sprint(p))) if err != nil { return url, errors.Wrap(err, "api host port binding")