Fix --memory flag parsing in minikube start #9033

vixus0 · 2020-08-19T12:48:01Z

Fixes #9017. Due to the way Go handles variable scope, mem was being redeclared inside the conditional. Outside, we were stuck with the value provided by suggestMemoryAllocation, therefore ignoring the value passed through the --memory flag.

Output of minikube start with the fix:

# minikube start --memory=2g --driver=kvm2
😄  minikube v1.12.3 on Ubuntu 18.04
✨  Using the kvm2 driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🔥  Creating kvm2 VM (CPUs=2, Memory=2048MB, Disk=20000MB) ...

# minikube start --memory=8g --driver=kvm2
😄  minikube v1.12.3 on Ubuntu 18.04
✨  Using the kvm2 driver based on user configuration
👍  Starting control plane node minikube in cluster minikube
🔥  Creating kvm2 VM (CPUs=2, Memory=8192MB, Disk=20000MB) ...

k8s-ci-robot · 2020-08-19T12:48:09Z

Hi @vixus0. 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 · 2020-08-19T12:48:16Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: vixus0
To complete the pull request process, please assign priyawadhwa
You can assign the PR to them by writing /assign @priyawadhwa 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

TravisBuddy · 2020-08-19T12:54:05Z

Travis tests have failed

Hey @vixus0,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test

which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.14.6.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.3/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.14.6.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.3/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
MINIKUBE_LDFLAGS="-X k8s.io/minikube/pkg/version.version=v1.12.3 -X k8s.io/minikube/pkg/version.isoVersion=v1.12.2 -X k8s.io/minikube/pkg/version.isoPath=minikube/iso -X k8s.io/minikube/pkg/version.gitCommitID="25a29054da4f96f92115d662c0872edff1691c15" -X k8s.io/minikube/pkg/version.storageProvisionerVersion=v2" ./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.29.0'
golangci/golangci-lint info found version: 1.29.0 for v1.29.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
cmd/minikube/cmd/start_flags.go:230: File is not `goimports`-ed (goimports)
      var err error
Makefile:408: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= go mod ================================================================
unused golang.org/x/exp
ok
= boilerplate ===========================================================
ok
Makefile:292: recipe for target 'test' failed
make: *** [test] Error 4

TravisBuddy Request Identifier: 0ed2c5b0-e21b-11ea-81c3-ef1e647c3fbd

TravisBuddy · 2020-08-19T12:55:26Z

Travis tests have failed

Hey @vixus0,
Please read the following log in order to understand the failure reason.
It'll be awesome if you fix what's wrong and commit the changes.

1st Build

View build log

make test

which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.14.6.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.3/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/assets/assets.go -pkg assets deploy/addons/...
gofmt -s -w pkg/minikube/assets/assets.go
which go-bindata || GO111MODULE=off GOBIN="/home/travis/gopath/bin" go get github.com/jteeuwen/go-bindata/...
/home/travis/gopath/bin/go-bindata
PATH="/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.14.6.linux.amd64/bin:/home/travis/bin:/home/travis/bin:/home/travis/.local/bin:/usr/local/lib/jvm/openjdk11/bin:/opt/pyenv/shims:/home/travis/.phpenv/shims:/home/travis/perl5/perlbrew/bin:/home/travis/.nvm/versions/node/v8.12.0/bin:/home/travis/.rvm/gems/ruby-2.5.3/bin:/home/travis/.rvm/gems/ruby-2.5.3@global/bin:/home/travis/.rvm/rubies/ruby-2.5.3/bin:/home/travis/gopath/bin:/home/travis/.gimme/versions/go1.11.1.linux.amd64/bin:/usr/local/maven-3.6.3/bin:/usr/local/cmake-3.12.4/bin:/usr/local/clang-7.0.0/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/snap/bin:/home/travis/.rvm/bin:/home/travis/.phpenv/bin:/opt/pyenv/bin:/home/travis/.yarn/bin:/home/travis/gopath/bin" go-bindata -nomemcopy -o pkg/minikube/translate/translations.go -pkg translate translations/...
gofmt -s -w pkg/minikube/translate/translations.go
MINIKUBE_LDFLAGS="-X k8s.io/minikube/pkg/version.version=v1.12.3 -X k8s.io/minikube/pkg/version.isoVersion=v1.12.2 -X k8s.io/minikube/pkg/version.isoPath=minikube/iso -X k8s.io/minikube/pkg/version.gitCommitID="bbcf0aa9b03e11f0ceed9203f171791ad6104e6d" -X k8s.io/minikube/pkg/version.storageProvisionerVersion=v2" ./test.sh
= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.29.0'
golangci/golangci-lint info found version: 1.29.0 for v1.29.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
cmd/minikube/cmd/start_flags.go:230: File is not `goimports`-ed (goimports)
      var err error
Makefile:408: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1
= go mod ================================================================
unused golang.org/x/exp
ok
= boilerplate ===========================================================
ok
Makefile:292: recipe for target 'test' failed
make: *** [test] Error 4

TravisBuddy Request Identifier: 3e821bd0-e21b-11ea-81c3-ef1e647c3fbd

minikube-bot · 2020-08-19T12:57:03Z

Can one of the admins verify this patch?

medyagh

thank you very much @vixus0 for fixing this bug, do you mind sharing the output of After this PR in the description?

also would u please run "make lint" and address the lint issue?

vixus0 · 2020-08-19T16:26:29Z

@medyagh I've updated the description, I hope that works. When I tried make lint I get the following:

./out/linters/golangci-lint-v1.29.0 run --timeout 7m --build-tags "integration container_image_ostree_stub containers_image_openpgp go_getter_nos3 go_getter_nogcs" --enable goimports,gocritic,golint,gocyclo,misspell,nakedret,stylecheck,unconvert,unparam,dogsled --exclude 'variable on range scope.*in function literal|ifElseChain' --skip-files "pkg/minikube/translate/translations.go|pkg/minikube/assets/assets.go" ./...
ERRO Running error: context loading failed: no go files to analyze
make: *** [Makefile:402: lint] Error 5

tstromberg · 2020-08-19T16:31:27Z

@vixus0 - You should be able to fix the lint error by running goimports -w cmd/minikube/cmd/start_flags.go

tstromberg · 2020-08-19T16:34:35Z

Odd that make lint fails on your machine. I cut & paste the same command you output on my own and it functioned properly.

We do have an alternative linter in the Makefile, but it does raise a few other issues that would not be related to your PR: make golint

medyagh · 2020-08-19T20:42:27Z

@medyagh I've updated the description, I hope that works. When I tried make lint I get the following:

= make lint =============================================================
golangci/golangci-lint info checking GitHub for tag 'v1.29.0'
golangci/golangci-lint info found version: 1.29.0 for v1.29.0/linux/amd64
golangci/golangci-lint info installed out/linters/golangci-lint
cmd/minikube/cmd/start_flags.go:230: File is not `goimports`-ed (goimports)
      var err error
Makefile:408: recipe for target 'lint-ci' failed
make[1]: *** [lint-ci] Error 1

you can fix that by

goimports -w cmd/minikube/cmd/start_flags.go

Due to the way Go handles variable scope, `mem` was being redeclared inside the conditional. Outside, we were stuck with the value provided by `suggestMemoryAllocation`, therefore ignoring the value passed through the `--memory` flag.

vixus0 · 2020-08-20T10:41:23Z

@medyagh lint issue should be fixed. Still couldn't get make lint to work, but the goimports -w sorted the indentation.

ThisGuyCodes · 2020-08-31T17:47:19Z

any timeline on another point release to get this fix out?

tstromberg · 2020-08-31T19:11:35Z

@ThisGuyCodes v1.13.0 will be released this week.

k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 19, 2020

k8s-ci-robot requested review from afbjorklund and priyawadhwa August 19, 2020 12:48

k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 19, 2020

medyagh requested changes Aug 19, 2020

View reviewed changes

Fix --memory flag parsing in minikube start

6e8f596

Due to the way Go handles variable scope, `mem` was being redeclared inside the conditional. Outside, we were stuck with the value provided by `suggestMemoryAllocation`, therefore ignoring the value passed through the `--memory` flag.

tstromberg merged commit 344a5be into kubernetes:master Aug 20, 2020

unixlab mentioned this pull request Aug 25, 2020

minikube 1.12.3 fails to interpret memory unit correctly #9057

Closed

aps831 mentioned this pull request Sep 12, 2020

Disk usage and memory inconsistency in minikube start #9115

Closed

tstromberg mentioned this pull request Sep 15, 2020

passing --memory=8g --driver virtualbox does not change memory for vm #9254

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix --memory flag parsing in minikube start #9033

Fix --memory flag parsing in minikube start #9033

vixus0 commented Aug 19, 2020 •

edited

Loading

k8s-ci-robot commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

TravisBuddy commented Aug 19, 2020

TravisBuddy commented Aug 19, 2020

minikube-bot commented Aug 19, 2020

medyagh left a comment •

edited

Loading

vixus0 commented Aug 19, 2020

tstromberg commented Aug 19, 2020

tstromberg commented Aug 19, 2020

medyagh commented Aug 19, 2020

vixus0 commented Aug 20, 2020

ThisGuyCodes commented Aug 31, 2020

tstromberg commented Aug 31, 2020 •

edited

Loading

Fix --memory flag parsing in minikube start #9033

Fix --memory flag parsing in minikube start #9033

Conversation

vixus0 commented Aug 19, 2020 • edited Loading

k8s-ci-robot commented Aug 19, 2020

k8s-ci-robot commented Aug 19, 2020

TravisBuddy commented Aug 19, 2020

Travis tests have failed

1st Build

TravisBuddy Request Identifier: 0ed2c5b0-e21b-11ea-81c3-ef1e647c3fbd

TravisBuddy commented Aug 19, 2020

Travis tests have failed

1st Build

TravisBuddy Request Identifier: 3e821bd0-e21b-11ea-81c3-ef1e647c3fbd

minikube-bot commented Aug 19, 2020

medyagh left a comment • edited Loading

Choose a reason for hiding this comment

vixus0 commented Aug 19, 2020

tstromberg commented Aug 19, 2020

tstromberg commented Aug 19, 2020

medyagh commented Aug 19, 2020

vixus0 commented Aug 20, 2020

ThisGuyCodes commented Aug 31, 2020

tstromberg commented Aug 31, 2020 • edited Loading

vixus0 commented Aug 19, 2020 •

edited

Loading

medyagh left a comment •

edited

Loading

tstromberg commented Aug 31, 2020 •

edited

Loading