Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Minimum CPUs check #5086

Merged
merged 4 commits into from
Sep 5, 2019
Merged

Minimum CPUs check #5086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5ec324e
Added check to exit app and show error message if number of cpus
MaxKam Aug 15, 2019
ac49c96
Updated cpu check to handle none driver cpu check
MaxKam Aug 21, 2019
68f8dca
Fixed counting of physical CPU cores
MaxKam Aug 22, 2019
4831b48
Fixed formatting issue
MaxKam Sep 2, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions cmd/minikube/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ import (
"github.com/google/go-containerregistry/pkg/authn"
"github.com/google/go-containerregistry/pkg/name"
"github.com/google/go-containerregistry/pkg/v1/remote"
"github.com/shirou/gopsutil/cpu"
gopshost "github.com/shirou/gopsutil/host"
"github.com/spf13/cobra"
"github.com/spf13/viper"
Expand Down Expand Up @@ -574,6 +575,22 @@ func validateConfig() {
out.V{"memory": memorySizeMB, "default_memorysize": pkgutil.CalculateSizeInMB(constants.DefaultMemorySize)})
}

var cpuCount int
if viper.GetString(vmDriver) == constants.DriverNone {
// Uses the gopsutil cpu package to count the number of physical cpu cores
ci, err := cpu.Counts(false)
if err != nil {
glog.Warningf("Unable to get CPU info: %v", err)
} else {
cpuCount = ci
}
} else {
cpuCount = viper.GetInt(cpus)
}
if cpuCount < constants.MinimumCPUS {
exit.UsageT("Requested cpu count {{.requested_cpus}} is less than the minimum allowed of {{.minimum_cpus}}", out.V{"requested_cpus": cpuCount, "minimum_cpus": constants.MinimumCPUS})
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check looks great, but to fully resolve the issue, there is an additional complication: The none driver doesn't respect the value of --cpus, as it is not possible to programatically add a CPU to a physical machine.

This check will need an extra step, something like:

import "github.com/shirou/gopsutil/cpu"

if viper.GetString(vmDriver) == constants.DriverNone {
  ci, err := cpu.Info()
  if err == nil {
    glog.Warningf("Unable to get CPU info: %v", err)
  } else {
    cpuCount = ci.Cores
  }

For what it's worth, the memory check suffers the same issue, but I'm not sure what the appropriate API is to solve it, so I won't ask you to do so =)

Copy link
Collaborator

@afbjorklund afbjorklund Aug 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

constants.MinimumCPUS

This should probably tie in with the Bootstrapper somehow ?

Since the requirement comes from kubeadm rather than k8s

https://github.com/kubernetes/kubernetes/blob/v1.15.2/cmd/kubeadm/app/constants/constants.go#L353

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tstromberg Hows this? 1217054

@afbjorklund sorry I'm not quite sure what you want me to change.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

afbjorklund was remarking that our minimum memory and CPU core requirements may change in the future depending on the bootstrapper used. For instance, k3s won't require two cores.

I'm OK with deferring per-bootstrapper requirements until we have two with different requirements.

}

// check that kubeadm extra args contain only whitelisted parameters
for param := range extraOptions.AsMap().Get(kubeadm.Kubeadm) {
if !cfg.ContainsParam(kubeadm.KubeadmExtraArgsWhitelist[kubeadm.KubeadmCmdParam], param) &&
Expand Down
2 changes: 2 additions & 0 deletions pkg/minikube/constants/constants.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,8 @@ const (
MinimumMemorySize = "1024mb"
// DefaultCPUS is the default number of cpus of a host
DefaultCPUS = 2
// MinimumCPUS is the minimum number of cpus of a host
MinimumCPUS = 2
// DefaultDiskSize is the default disk image size, in megabytes
DefaultDiskSize = "20000mb"
// MinimumDiskSize is the minimum disk image size, in megabytes
Expand Down