Add condition to check --cpus count with available cpu count #10388

BLasan · 2021-02-06T18:26:46Z

Added a condition to check the argument value(--cpus) is less than the available cpus in the system

Before PR
When passing a value for --cpus argument, even if the value is larger than the available cpu count, it was going to print an error message which was not appropriate. see the logs mentioned in the issue #10386

After PR
It'll print an appropriate error message according to the newly added condition. See changes in this PR

minikube start -p functional-20210207001733-2929731 --memory=4000 --apiserver-port=8441 --wait=true --cpus=10
😄  [functional-20210207001733-2929731] minikube v1.17.1 on Ubuntu 20.04
✨  Automatically selected the docker driver

⛔  Exiting due to RSRC_INSUFFICIENT_CORES: Requested cpu count 10 is greater than the available cpus of 4

k8s-ci-robot · 2021-02-06T18:26:54Z

Hi @BLasan. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

minikube-bot · 2021-02-06T18:30:05Z

Can one of the admins verify this patch?

BLasan · 2021-02-06T19:53:08Z

@afbjorklund @medyagh Please review

medyagh

@BLasan do u mind putting the out Before and After this PR in the PR Description ?

BLasan · 2021-02-07T03:17:58Z

@BLasan do u mind putting the out Before and After this PR in the PR Description ?

Added. Please take a look

medyagh

@BLasan
thank you for fixing this, lets improve the Exit message with advice for Docker Desktop on Mac and windows so they can allocate more CPUs

cmd/minikube/cmd/start.go

afbjorklund · 2021-02-08T07:48:33Z

thank you for fixing this, lets improve the Exit message with advice for Docker Desktop on Mac and windows so they can allocate more CPUs

But only if there are more CPUs available on the host, than what is actually allocated to the Docker VM (reported by it)

Typical scenario would be that the host has 4 CPU (hyperthreads), but that the Docker VM only has 2 vCPU (by default).

So if user is trying to limit docker to 4 CPU, there would be this instruction. You can increase the VM, to use 4 vCPU.

But if they are trying to limit it to 8 CPU, the error from this change would apply... There is no more hardware available.

medyagh · 2021-02-08T17:15:57Z

cmd/minikube/cmd/start.go

@@ -1029,6 +1029,18 @@ func validateCPUCount(drvName string) {

 	}

+	if si.CPUs < cpuCount {
+		out.Step(style.Empty, `- Ensure your {{.driver_name}} daemon has access to enough CPU/memory resources.`, out.V{"driver_name": drvName})


we want to show this advice only for Docker Desktop Drivers (docker on mac and windows) and not other drivers (kvm or docker on linux...)

so lets add this check

if driver.IsDockerDesktop(drvName) { ...

Added. Please review sir

BLasan · 2021-02-08T18:36:49Z

@medyagh @afbjorklund please review

medyagh

thank you @BLasan

medyagh · 2021-02-09T20:46:31Z

/ok-to-test

k8s-ci-robot · 2021-02-09T20:46:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BLasan, medyagh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [medyagh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

minikube-pr-bot · 2021-02-09T20:52:28Z

kvm2 Driver
error collecting results for kvm2 driver: timing run 0 with Minikube (PR 10388): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/10388/minikube start --driver=kvm2]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/10388/minikube: exec format error
docker Driver
error collecting results for docker driver: timing run 0 with Minikube (PR 10388): timing cmd: [/home/performance-monitor/.minikube/minikube-binaries/10388/minikube start --driver=docker]: starting cmd: fork/exec /home/performance-monitor/.minikube/minikube-binaries/10388/minikube: exec format error

BLasan · 2021-02-12T17:04:25Z

@medyagh Any remaining work to be done sir?

medyagh

thank you

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 6, 2021

k8s-ci-robot requested review from blueelvis and ilya-zuyev February 6, 2021 18:26

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Feb 6, 2021

BLasan force-pushed the issue-10386 branch 2 times, most recently from 399c23d to 8b84fa7 Compare February 6, 2021 19:36

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 6, 2021

Add condition to check --cpus count with available cpu count

cc2bf6a

BLasan force-pushed the issue-10386 branch from 8b84fa7 to cc2bf6a Compare February 6, 2021 19:38

medyagh reviewed Feb 7, 2021

View reviewed changes

medyagh requested changes Feb 7, 2021

View reviewed changes

cmd/minikube/cmd/start.go Show resolved Hide resolved

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Feb 8, 2021

BLasan force-pushed the issue-10386 branch from c42d1a9 to 56fc8a9 Compare February 8, 2021 08:45

medyagh requested changes Feb 8, 2021

View reviewed changes

BLasan force-pushed the issue-10386 branch from 56fc8a9 to f2d950a Compare February 8, 2021 17:52

Add advice message to increase cpus

9481787

BLasan force-pushed the issue-10386 branch from f2d950a to 9481787 Compare February 8, 2021 17:53

medyagh approved these changes Feb 9, 2021

View reviewed changes

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Feb 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2021

medyagh reviewed Feb 12, 2021

View reviewed changes

medyagh merged commit 27d86a4 into kubernetes:master Feb 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add condition to check --cpus count with available cpu count #10388

Add condition to check --cpus count with available cpu count #10388

BLasan commented Feb 6, 2021 •

edited

Loading

k8s-ci-robot commented Feb 6, 2021

minikube-bot commented Feb 6, 2021

BLasan commented Feb 6, 2021 •

edited

Loading

medyagh left a comment

BLasan commented Feb 7, 2021

medyagh left a comment

afbjorklund commented Feb 8, 2021

medyagh Feb 8, 2021

BLasan Feb 8, 2021

BLasan commented Feb 8, 2021 •

edited

Loading

medyagh left a comment

medyagh commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

minikube-pr-bot commented Feb 9, 2021

BLasan commented Feb 12, 2021

medyagh left a comment

Add condition to check --cpus count with available cpu count #10388

Add condition to check --cpus count with available cpu count #10388

Conversation

BLasan commented Feb 6, 2021 • edited Loading

k8s-ci-robot commented Feb 6, 2021

minikube-bot commented Feb 6, 2021

BLasan commented Feb 6, 2021 • edited Loading

medyagh left a comment

Choose a reason for hiding this comment

BLasan commented Feb 7, 2021

medyagh left a comment

Choose a reason for hiding this comment

afbjorklund commented Feb 8, 2021

medyagh Feb 8, 2021

Choose a reason for hiding this comment

BLasan Feb 8, 2021

Choose a reason for hiding this comment

BLasan commented Feb 8, 2021 • edited Loading

medyagh left a comment

Choose a reason for hiding this comment

medyagh commented Feb 9, 2021

k8s-ci-robot commented Feb 9, 2021

minikube-pr-bot commented Feb 9, 2021

BLasan commented Feb 12, 2021

medyagh left a comment

Choose a reason for hiding this comment

BLasan commented Feb 6, 2021 •

edited

Loading

BLasan commented Feb 6, 2021 •

edited

Loading

BLasan commented Feb 8, 2021 •

edited

Loading