Add memory size sanity checking

[blueelvis]

Fixes #4437

1. Added Sanity Check for Memory
2. Refactored code to use similar format like Hard Disk size
3. Added Minimum and default for Memory

Edit -- Fixed the failing test.

[k8s-ci-robot]

Hi @blueelvis. 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]

Can one of the admins verify this patch?

[afbjorklund]

That first merge looks weird (PR#1?), can you squash and rebase perhaps ?

I think this needs to be backwards-compatible. That is, it should accept MB.
So if the user says "2048", it should still be read as 2G - rather than as 2K.

[blueelvis]

@afbjorklund -

1. Regarding first, will do that.
2. I am not sure what you mean by backwards compatible in this case. The parameter will accept the following values -
   1. 2000
   2. 2MB
   3. 2gb

Do you mean at the starting log like below?

Creating hyperv VM (CPUs=2, Memory=2048MB, Disk=20000MB) ...

[afbjorklund]

If it still accepts a number without a suffix as megabytes, then all is fine. I thought it interpreted it as bytes

EDIT: and it seems that I was right:

$ ./out/minikube start --memory=2000
😄  minikube v1.1.1 on linux (amd64)
💡  Requested memory allocation (0MB) is less than the minimum allowed of 1500MB

[blueelvis]

I am on mobile right now but yes, on a windows box, at 1000mb, the cluster setup was crashing at random points. At 1.5gb,it started without issues so that's why kept it default.

…

On Mon 10 Jun, 2019, 10:59 PM Medya Ghazizadeh, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In pkg/minikube/constants/constants.go <#4463 (comment)>: > @@ -121,8 +121,10 @@ const ( DefaultKeepContext = false // SHASuffix is the suffix of a SHA-256 checksum file SHASuffix = ".sha256" - // DefaultMemory is the default memory of a host, in megabytes - DefaultMemory = 2048 + // DefaultMemory is the default memory which will be allocated to minikube, parseable + DefaultMemory = "2048mb" + //MinimumMemorySizeMB is the minimum memory size, in megabytes + MinimumMemorySizeMB = 1500 Sure. @tstromberg <https://github.com/tstromberg> mentioned the lowest he tried was 1.2 gb though — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4463?email_source=notifications&email_token=ABVJQI42S5DA7YZGFCSMFTDPZ2FO7A5CNFSM4HWUNWR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3CDQTY#discussion_r292108829>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVJQI32RKTLXOBHXNPMTITPZ2FO7ANCNFSM4HWUNWRQ> .

[medyagh]

I am on mobile right now but yes, on a windows box, at 1000mb, the cluster setup was crashing at random points. At 1.5gb,it started without issues so that's why kept it default.
…

lets go with hard failing on less than 1024 and warning for less than 2048 mb

[afbjorklund]

I am on mobile right now but yes, on a windows box, at 1000mb, the cluster setup was crashing at random points. At 1.5gb,it started without issues so that's why kept it default.

Thanks for explaining, still would like to keep it to the lower limit since we are hard-coding it and since we might end up running more low-end deployments like containerd or minions that might get away with it...

[blueelvis]

If it still accepts a number without a suffix as megabytes, then all is fine. I thought it interpreted it as bytes

EDIT: and it seems that I was right:

$ ./out/minikube start --memory=2000
😄  minikube v1.1.1 on linux (amd64)
💡  Requested memory allocation (0MB) is less than the minimum allowed of 1500MB

@afbjorklund @medyagh @tstromberg
I am trying to think through what can be an elegant way to solve this but I am not clear on why we need this to be backward compatible. Because, this issue then exists with the --disk-size argument as well -

PS C:\utilities> .\minikube-windows-amd64.exe start --vm-driver "hyperv" --hyperv-virtual-switch "Default Switch" --disk-size 3000 -v 9999
* minikube v1.1.1 on windows (amd64)
X Requested disk size (0MB) is less than minimum of 2000MB
PS C:\utilities>

All I can think of is that it would be best to standardize both of them. The issue seems to be here where the function does not have a default multiplier and returns 0 in case mb,kb... are missing (as per the regex) - https://github.com/docker/go-units/blob/519db1ee28dcc9fd2474ae59fca29a810482bfb1/size.go#L91

I can think of the following options as of now -

1. If the string doesn't contain a string from the map mb,kb..., append the mb characters by default. This then should be applied to both the Disk Size and Memory size.
2. Do not allow the users to pass in a string without the keywords (Basically, no backwards compatibility) and we update the documents. As the function is returning 0 when only given integers, we error out if the result is 0.

Also, do you think a patch should be created to fix the error handling of the go-units library? Because this should error out if it is not able to parse out the string in human readable size.

Edit -- My bad on saying above that it will work with 2000.

[medyagh]

I would suggest keeping it backward compatible,
so if only provided an integer, it should be treated as mb
if they provided mb or M or 'gbor G` it should be treated as such.

[RA489]

/ok-to-test

[blueelvis]

Guys, i am travelling. Thanks for the feedback! I will work on this next week once I am back.

…

-Pranav

On Sat 15 Jun, 2019, 5:53 AM Medya Ghazizadeh, ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In pkg/minikube/constants/constants.go <#4463 (comment)>: > @@ -121,8 +121,10 @@ const ( DefaultKeepContext = false // SHASuffix is the suffix of a SHA-256 checksum file SHASuffix = ".sha256" - // DefaultMemory is the default memory of a host, in megabytes - DefaultMemory = 2048 + // DefaultMemory is the default memory which will be allocated to minikube, parseable + DefaultMemory = "2048mb" + //MinimumMemorySizeMB is the minimum memory size, in megabytes + MinimumMemorySizeMB = 1500 lets warn the user if they are less than DefaultMemory (2048mb) — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#4463?email_source=notifications&email_token=ABVJQIYH4ARAWLASLC6TU53P2QHMHA5CNFSM4HWUNWR2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB3UN52A#pullrequestreview-250142440>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABVJQI3GGDPCE5RM6TBYBMTP2QHMHANCNFSM4HWUNWRQ> .

[k8s-ci-robot]

@blueelvis: 1 invalid OWNERS file

In response to this:

Fixes #4437

1. Added Sanity Check for Memory
2. Refactored code to use similar format like Hard Disk size
3. Added Minimum and default for Memory

Edit -- Fixed the failing test.

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.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: blueelvis
To complete the pull request process, please assign ra489
You can assign the PR to them by writing /assign @ra489 in a comment when ready.

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

Needs approval from an approver in each of these files:

• OWNERS

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

[blueelvis]

@medyagh @afbjorklund - I rebased the PR and reverted that merge commit but this seems to be an issue because now the number of files which have been changed have increased quite a lot. Do you think it would make sense to just abandon this PR and I can send the changes from a fresh branch?

[medyagh]

@blueelvis it needs a rebase

[blueelvis]

@medyagh - I just rebased. Could you please check now?

[blueelvis]

I am abandoning this PR and sending out a fresh one. The history on my local is messed up as of now. Let me clean it up and send a fresh PR. Sorry for the inconvenience.