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.

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

Add memory size sanity checking #4463

Conversation

blueelvis
Copy link
Contributor

@blueelvis blueelvis commented Jun 10, 2019

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 k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jun 10, 2019
@k8s-ci-robot
Copy link
Contributor

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.

@k8s-ci-robot k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 10, 2019
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

Copy link
Collaborator

@afbjorklund afbjorklund left a comment

Choose a reason for hiding this comment

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

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
Copy link
Contributor Author

@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
Copy link
Collaborator

afbjorklund commented Jun 10, 2019

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

pkg/minikube/constants/constants.go Outdated Show resolved Hide resolved
@blueelvis
Copy link
Contributor Author

blueelvis commented Jun 10, 2019 via email

@medyagh
Copy link
Member

medyagh commented Jun 10, 2019

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
Copy link
Collaborator

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
Copy link
Contributor Author

blueelvis commented Jun 11, 2019

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
Copy link
Member

medyagh commented Jun 12, 2019

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
Copy link

RA489 commented Jun 13, 2019

/ok-to-test

@k8s-ci-robot k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jun 13, 2019
pkg/minikube/constants/constants.go Outdated Show resolved Hide resolved
pkg/minikube/constants/constants.go Outdated Show resolved Hide resolved
@blueelvis
Copy link
Contributor Author

blueelvis commented Jun 14, 2019 via email

@medyagh medyagh added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 14, 2019
@blueelvis blueelvis force-pushed the bug/4437-Add-memory-size-sanity-checking branch from 290b1b2 to 7d44ae4 Compare June 24, 2019 13:21
@k8s-ci-robot k8s-ci-robot added the do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. label Jun 24, 2019
Copy link
Contributor

@k8s-ci-robot k8s-ci-robot left a comment

Choose a reason for hiding this comment

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

@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.

OWNERS Outdated Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2019
@k8s-ci-robot
Copy link
Contributor

[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:

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

@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. do-not-merge/invalid-owners-file Indicates that a PR should not merge because it has an invalid OWNERS file in it. labels Jun 24, 2019
@blueelvis
Copy link
Contributor Author

@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?

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 24, 2019
@blueelvis blueelvis force-pushed the bug/4437-Add-memory-size-sanity-checking branch from fe8d893 to 491159a Compare June 25, 2019 07:48
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@medyagh
Copy link
Member

medyagh commented Jun 25, 2019

@blueelvis it needs a rebase

@blueelvis blueelvis force-pushed the bug/4437-Add-memory-size-sanity-checking branch from 491159a to 33602cb Compare June 25, 2019 17:32
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@blueelvis blueelvis force-pushed the bug/4437-Add-memory-size-sanity-checking branch from 33602cb to 879d10a Compare June 25, 2019 17:59
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 25, 2019
@blueelvis blueelvis force-pushed the bug/4437-Add-memory-size-sanity-checking branch from 879d10a to 7cf7b54 Compare June 25, 2019 18:07
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jun 25, 2019
@blueelvis
Copy link
Contributor Author

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

@blueelvis
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add memory size sanity checking
8 participants