Behavior change: start with no arguments uses existing cluster config #7449

medyagh · 2020-04-06T08:19:14Z

Change behavior of minikube start

if minkube already started with args
like minikube start --container-runtime=containerd
a minikube start (without args) on an already started minikube should NOT change the run-time back to the default config ('docker').
an attempt to fix soft "start" without args - will overwrite the container-runtime to docker. #7448
unfourntaely, it is not clear since when this is broken but currently the minikube does not respect the profile config.
also fixes the Port : 0 in our configs .

before this PR:

minikube start --driver=hyperkit --container-runtime=conrtainerd
Fine.

minikube start --driver=hyperkit --container-runtime=conrtainerd
Fine Also

 $ cat ~/.minikube/profiles/minikube/config.json | grep Container
        "ContainerRuntime": "containerd",

but !!!! if you do a soft start without args:

minikube start

and in the config, you will see, the run time has been changed to docker

 $ cat ~/.minikube/profiles/minikube/config.json | grep "ContainerRuntime"
                "ContainerRuntime": "docker",

and minikube disrepects the user and restarts the cluster with a hard start to a different container-runtime.

Integeration test:

added a SoftStart integeration tests to the functional tests
also in
#7435 there is additional tests for soft start that validates it doesn not do a reset.

I tested locally on mac hyperkit seems to be working.

I did this PR with a tired mind, I do feel like I might have missed some flags in the 'func updateExistingConfigFromFlags' . please anyone who reviews this, review with 80% chance of human error on my side on This PR.

k8s-ci-robot · 2020-04-06T08:19:51Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: 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

medyagh · 2020-04-06T10:46:34Z

/ok-to-test

minikube-pr-bot · 2020-04-06T11:00:42Z

All Times minikube: [ 65.975062 65.493687 64.066381]
All Times Minikube (PR 7449): [ 67.176053 67.778659 66.787898]

Average minikube: 65.178377
Average Minikube (PR 7449): 67.247537

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.246623 |           0.255467 |
| Creating kvm2        | 40.885454 |          41.180895 |
| Preparing Kubernetes | 21.924858 |          23.696690 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-06T11:17:44Z

All Times minikube: [ 68.677821 67.571879 66.023208]
All Times Minikube (PR 7449): [ 68.501772 70.629598 65.808743]

Average minikube: 67.424303
Average Minikube (PR 7449): 68.313371

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.243688 |           0.236887 |
| Creating kvm2        | 42.229437 |          43.099722 |
| Preparing Kubernetes | 22.732493 |          22.880553 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

tstromberg

Looks good, but also subtle enough that it needs an integration test.

minikube-pr-bot · 2020-04-07T04:36:29Z

All Times minikube: [ 66.572089 65.297555 65.810070]
All Times Minikube (PR 7449): [ 68.204604 65.443892 66.854200]

Average minikube: 65.893238
Average Minikube (PR 7449): 66.834232

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.237393 |           0.236684 |
| Creating kvm2        | 41.527594 |          40.896047 |
| Preparing Kubernetes | 21.848430 |          23.408922 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

codecov-io · 2020-04-07T04:37:59Z

Codecov Report

Merging #7449 into master will decrease coverage by 0.40%.
The diff coverage is 50.32%.

@@            Coverage Diff             @@
##           master    #7449      +/-   ##
==========================================
- Coverage   37.07%   36.67%   -0.41%     
==========================================
  Files         146      146              
  Lines        8871     8980     +109     
==========================================
+ Hits         3289     3293       +4     
- Misses       5193     5294     +101     
- Partials      389      393       +4

Impacted Files	Coverage Δ
pkg/minikube/config/config.go	`50.00% <ø> (ø)`
cmd/minikube/cmd/start_flags.go	`50.00% <50.00%> (ø)`
cmd/minikube/cmd/start.go	`17.20% <66.66%> (-16.47%)`	⬇️

minikube-pr-bot · 2020-04-07T06:04:06Z

All Times minikube: [ 71.772491 64.711845 66.667994]
All Times Minikube (PR 7449): [ 65.908929 67.328790 70.961439]

Average minikube: 67.717443
Average Minikube (PR 7449): 68.066386

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.284215 |           0.235688 |
| Creating kvm2        | 42.948542 |          42.507515 |
| Preparing Kubernetes | 21.966796 |          22.954312 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-07T08:42:17Z

All Times minikube: [ 72.477549 66.346935 64.828128]
All Times Minikube (PR 7449): [ 66.093214 68.171455 66.353393]

Average minikube: 67.884204
Average Minikube (PR 7449): 66.872688

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.263917 |           0.239449 |
| Creating kvm2        | 42.413636 |          40.933022 |
| Preparing Kubernetes | 23.018743 |          23.627250 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-07T09:09:17Z

All Times minikube: [ 65.846263 66.792026 66.708484]
All Times Minikube (PR 7449): [ 65.975812 65.304265 66.177839]

Average minikube: 66.448924
Average Minikube (PR 7449): 65.819305

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.240037 |           0.237166 |
| Creating kvm2        | 40.593749 |          40.806218 |
| Preparing Kubernetes | 23.480378 |          22.499964 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-07T10:16:41Z

All Times minikube: [ 68.268333 69.220528 65.387331]
All Times Minikube (PR 7449): [ 65.955928 69.214513 63.918516]

Average Minikube (PR 7449): 66.362986
Average minikube: 67.625397

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.241282 |           0.238251 |
| Creating kvm2        | 41.580603 |          40.919445 |
| Preparing Kubernetes | 23.534235 |          22.812227 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

tstromberg

One tricky part of this soft start PR, is that it changes the behavior of minikube: always respect the flags set to --start. We did admittedly start to creep in this direction however when changing it so that the kubernetes-version is sticky.

If a user omits start flags, minikube has always tried to start the cluster up with default values.

This terrible and confusing behavior is why we've spoken about splitting cluster creation commands from cluster startup commands.

Just to understand, is the behavior you are suggesting in this PR to:

If "minikube start" is run with no flags, use the previous configuration with no changes
If "minikube start" is run with flags, overwrite the previous configuration (no merge)

Thank you for cleaning this yuckier part of our UX up.

test/integration/functional_test.go

tstromberg · 2020-04-07T17:38:49Z

Also, since this is an explicit behavioral change, give the PR a really clear title.

sharifelgamal · 2020-04-07T19:44:06Z

Merging start.go and flags.go into start_flags.go makes this change hard to review, and I don't think it's especially necessary. Is there a reason for the change?

medyagh · 2020-04-07T20:20:07Z

Merging start.go and flags.go into start_flags.go makes this change hard to review, and I don't think it's especially necessary. Is there a reason for the change?

thank you thats a good point, I will do that on my next PRs.
the reason that I did this in this PR was I had to go back an forth many times in the code to create the new func updateExisting.... and start.go was extremely large, made the work extremely hard to keep looking for start flag variable names.

minikube-pr-bot · 2020-04-07T20:30:59Z

All Times minikube: [ 65.143763 66.057339 65.568464]
All Times Minikube (PR 7449): [ 67.588083 66.736478 69.879251]

Average minikube: 65.589855
Average Minikube (PR 7449): 68.067938

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.265042 |           0.235814 |
| Creating kvm2        | 40.572348 |          41.902111 |
| Preparing Kubernetes | 22.314500 |          23.628202 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

minikube-pr-bot · 2020-04-07T21:08:03Z

All Times minikube: [ 66.125782 64.535043 64.835762]
All Times Minikube (PR 7449): [ 62.816259 64.761445 66.830730]

Average minikube: 65.165529
Average Minikube (PR 7449): 64.802811

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 7449) |
+----------------------+-----------+--------------------+
| minikube v           |  0.149083 |           0.227639 |
| Creating kvm2        | 40.738728 |          40.343709 |
| Preparing Kubernetes | 22.348933 |          22.022296 |
| Pulling images       |           |                    |
| Launching Kubernetes |           |                    |
| Waiting for cluster  |           |                    |
+----------------------+-----------+--------------------+

tstromberg

LGTM pending integration tests.

If possible, please leave this PR open for at least 48 hours for visibility to others.

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 6, 2020

k8s-ci-robot requested review from blueelvis and RA489 April 6, 2020 08:19

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 6, 2020

medyagh changed the title ~~wip : Fix soft start nondocker~~ wip : fix not respecting profile config Apr 6, 2020

medyagh changed the title ~~wip : fix not respecting profile config~~ fix not respecting profile config Apr 6, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 6, 2020

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Apr 6, 2020

medyagh requested review from tstromberg and sharifelgamal April 6, 2020 10:46

tstromberg suggested changes Apr 6, 2020

View reviewed changes

medyagh added 10 commits April 6, 2020 18:58

respect the user existing config

792ca63

add new func updateExistingConfigFromFlags

a350065

save profile

bee6815

refactor more

b777d97

fix it

1d892a2

fix unit test

2c037b4

remove most of the viper calls from node start

9c0e5df

remove debugging statement

5517e2b

remove duplicate setDockerProxy

ee1f8ff

add wait flag to updateExistingConfigFromFlags

84693ea

medyagh force-pushed the fix_soft_start_nondocker branch from 644f36b to 84693ea Compare April 7, 2020 01:58

medyagh added 2 commits April 6, 2020 20:14

add soft start test

52fee50

add soft start integeration test

67f1276

lint

e4abe73

fix

c4f5f4d

fix select default cni

26c999a

warn if changed memory cpu of an exiting cluster

e683dd3

handle Port 0 in configs

dfd9f7a

tstromberg suggested changes Apr 7, 2020

View reviewed changes

test/integration/functional_test.go Outdated Show resolved Hide resolved

put apiPort in a variable

c11e642

medyagh changed the title ~~fix soft start overwriting cluster config, add tests for it~~ change behaviour of "start" to not to revert an already strated minikube with args. Apr 7, 2020

remove duplicate checking existing driver flag

65e1ff3

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Apr 7, 2020

medyagh changed the title ~~change behaviour of "start" to not to revert an already strated minikube with args.~~ change behaviour of "start without args" not to revert an already started minikube Apr 7, 2020

medyagh changed the title ~~change behaviour of "start without args" not to revert an already started minikube~~ change behaviour of "start without args" not to revert an already started cluster. Apr 7, 2020

medyagh changed the title ~~change behaviour of "start without args" not to revert an already started cluster.~~ change: "start without args" not to revert an already started cluster with args. Apr 7, 2020

tstromberg changed the title ~~change: "start without args" not to revert an already started cluster with args.~~ Behavior change: start with no arguments uses existing cluster config Apr 7, 2020

tstromberg approved these changes Apr 7, 2020

View reviewed changes

medyagh merged commit e098a3c into kubernetes:master Apr 8, 2020

sharifelgamal mentioned this pull request Apr 8, 2020

memory size reset if --memory command-line arg is not always supplied to start #6771

Closed

medyagh deleted the fix_soft_start_nondocker branch May 2, 2020 22:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Behavior change: start with no arguments uses existing cluster config #7449

Behavior change: start with no arguments uses existing cluster config #7449

medyagh commented Apr 6, 2020 •

edited

Loading

k8s-ci-robot commented Apr 6, 2020

medyagh commented Apr 6, 2020

minikube-pr-bot commented Apr 6, 2020

minikube-pr-bot commented Apr 6, 2020

tstromberg left a comment

minikube-pr-bot commented Apr 7, 2020

codecov-io commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

tstromberg left a comment

tstromberg commented Apr 7, 2020

sharifelgamal commented Apr 7, 2020

medyagh commented Apr 7, 2020 •

edited

Loading

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

tstromberg left a comment

Behavior change: start with no arguments uses existing cluster config #7449

Behavior change: start with no arguments uses existing cluster config #7449

Conversation

medyagh commented Apr 6, 2020 • edited Loading

Change behavior of minikube start

before this PR:

but !!!! if you do a soft start without args:

Integeration test:

k8s-ci-robot commented Apr 6, 2020

medyagh commented Apr 6, 2020

minikube-pr-bot commented Apr 6, 2020

minikube-pr-bot commented Apr 6, 2020

tstromberg left a comment

Choose a reason for hiding this comment

minikube-pr-bot commented Apr 7, 2020

codecov-io commented Apr 7, 2020

Codecov Report

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg commented Apr 7, 2020

sharifelgamal commented Apr 7, 2020

medyagh commented Apr 7, 2020 • edited Loading

minikube-pr-bot commented Apr 7, 2020

minikube-pr-bot commented Apr 7, 2020

tstromberg left a comment

Choose a reason for hiding this comment

medyagh commented Apr 6, 2020 •

edited

Loading

medyagh commented Apr 7, 2020 •

edited

Loading