Create the Node subcommands #6556

sharifelgamal · 2020-02-08T01:01:11Z

Fixes #6300

Also refactors minikube start and minikube stop

k8s-ci-robot · 2020-02-08T01:02:33Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sharifelgamal

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~~ [sharifelgamal]

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

tstromberg

Minor nits to note the obviously incomplete parts, but it looks great and safely mergeable.

tstromberg · 2020-02-12T17:47:16Z

cmd/minikube/cmd/logs.go

@@ -66,7 +67,7 @@ var logsCmd = &cobra.Command{
 		if err != nil {
 			exit.WithError("command runner", err)
 		}
-		bs, err := getClusterBootstrapper(api, viper.GetString(cmdcfg.Bootstrapper))
+		bs, err := node.Bootstrapper(api, viper.GetString(cmdcfg.Bootstrapper))


This placement feels a bit weird: the bootstrapper is a cluster-property rather than a node property.

tstromberg · 2020-02-12T17:51:13Z

cmd/minikube/cmd/stop.go

@@ -53,15 +53,15 @@ func runStop(cmd *cobra.Command, args []string) {

 	nonexistent := false
 	stop := func() (err error) {
-		err = cluster.StopHost(api)
+		err = cluster.StopHost(api, profile)


This should probably be node.Stop() or cluster.Stop()

tstromberg · 2020-02-12T17:55:32Z

pkg/minikube/node/start.go

+)
+
+// Start spins up a guest and starts the kubernetes node.
+func Start(mc *config.MachineConfig, n *config.Node, primary bool, existingAddons map[string]bool) (*kubeconfig.Settings, error) {


Unless this is altering the contents of mc & n, consider passing in values rather than pointers.

"Don't pass pointers as function arguments just to save a few bytes. If a function refers to its argument x only as *x throughout, then the argument shouldn't be a pointer."

tstromberg · 2020-02-12T17:58:23Z

pkg/minikube/node/start.go

+	showVersionInfo(k8sVersion, cr)
+	waitCacheRequiredImages(&cacheGroup)
+
+	// Must be written before bootstrap, otherwise health checks may flake due to stale IP


Feel free to do this later, but kubeconfig (and other bits of this function) is on a cluster level rather than a node level.

medyagh · 2020-02-13T18:45:39Z

/ok-to-test

minikube-pr-bot · 2020-02-13T18:50:32Z

Error: running mkcmp: exit status 1

medyagh · 2020-02-13T19:28:08Z

the integration tests look good !!!!

medyagh · 2020-02-13T19:31:16Z

only test that looks a bit suspectious is TestVersionUpgrade that fails across all of them

codecov-io · 2020-02-13T22:37:50Z

Codecov Report

❗ No coverage uploaded for pull request base (master@8c55c9c). Click here to learn what that means.
The diff coverage is 14.28%.

@@            Coverage Diff            @@
##             master    #6556   +/-   ##
=========================================
  Coverage          ?   38.52%           
=========================================
  Files             ?      142           
  Lines             ?     8603           
  Branches          ?        0           
=========================================
  Hits              ?     3314           
  Misses            ?     4875           
  Partials          ?      414

Impacted Files	Coverage Δ
pkg/minikube/driver/driver_linux.go	`0% <ø> (ø)`
pkg/minikube/command/kic_runner.go	`0% <0%> (ø)`
pkg/minikube/driver/driver.go	`57.5% <0%> (ø)`
cmd/minikube/cmd/kubectl.go	`0% <0%> (ø)`
pkg/minikube/machine/fix.go	`32.33% <0%> (ø)`
pkg/minikube/machine/start.go	`51.23% <0%> (ø)`
pkg/minikube/cluster/ip.go	`0% <0%> (ø)`
cmd/minikube/cmd/start.go	`32.87% <0%> (ø)`
pkg/minikube/machine/cache_images.go	`0% <0%> (ø)`
pkg/minikube/machine/client.go	`43.84% <0%> (ø)`
... and 4 more

minikube-pr-bot · 2020-02-13T22:42:07Z

All Times minikube: [ 94.593679 94.446244 92.649031]
All Times Minikube (PR 6556): [ 93.824419 90.321647 95.728519]

Average minikube: 93.896318
Average Minikube (PR 6556): 93.291529

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6556) |
+----------------------+-----------+--------------------+
| minikube v           |  0.196925 |           0.203300 |
| Creating kvm2        | 20.016630 |          20.187398 |
| Preparing Kubernetes | 50.884715 |          50.866796 |
| Pulling images       |           |                    |
| Launching Kubernetes | 21.084867 |          20.351941 |
| Waiting for cluster  |  0.042750 |           0.068516 |
+----------------------+-----------+--------------------+

tstromberg

Good god this is an awesome refactor. Ship it!

sharifelgamal added 13 commits January 24, 2020 11:40

Adding the initial node cmd and pkg files

ad03309

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

8db7f59

moving node package around

317dd41

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

ae8309f

moving packages around

593d3fc

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

a539efc

node add works

3cbe879

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

dc0be4a

shuffling stuff around for import purposes

8d365fc

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

eded9a1

fill in cmd code

f6f8b50

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

60c8cfb

let's rearrange some stuff

e1fc3ce

sharifelgamal changed the title ~~Create the Node subcommands~~ WIP: Create the Node subcommands Feb 8, 2020

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/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 8, 2020

k8s-ci-robot requested review from blueelvis and medyagh February 8, 2020 01:02

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

begin start refactor

69aa54f

k8s-ci-robot added needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2020

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

f2ebbb9

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 11, 2020

hey did you want more files? good

b6a429d

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 Feb 11, 2020

sharifelgamal added 3 commits February 11, 2020 17:06

REFACTOR COMPLETE?

744efaa

revert changelog change

8de1f8b

search and replace cleanup

adc1a56

sharifelgamal requested a review from tstromberg February 12, 2020 01:15

sharifelgamal added 2 commits February 11, 2020 18:39

linting nonsense

9830d91

fixed commands, they mostly work now

b427dc5

tstromberg suggested changes Feb 12, 2020

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 12, 2020

sharifelgamal added 2 commits February 12, 2020 18:34

holy crap cyclical import hell sucks

8c55c9c

Merge branch 'master' of github.com:kubernetes/minikube into node-pkg

2785d29

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 13, 2020

fix unit tests and fix merge conflicts

c1a3e43

sharifelgamal changed the title ~~WIP: Create the Node subcommands~~ Create the Node subcommands Feb 13, 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 Feb 13, 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 Feb 13, 2020

don\'t return that the machine doesn\'t exist if the k8s is stopped

6d8a112

tstromberg approved these changes Feb 13, 2020

View reviewed changes

sharifelgamal merged commit 744e389 into kubernetes:master Feb 14, 2020

fmotrifork mentioned this pull request Feb 24, 2020

minikube 1.7.3 fishworks/fish-food#585

Merged

ialidzhikov mentioned this pull request Mar 23, 2020

Unable to override node-name: lookup minikube on x:53: no such host #7161

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create the Node subcommands #6556

Create the Node subcommands #6556

sharifelgamal commented Feb 8, 2020 •

edited

Loading

k8s-ci-robot commented Feb 8, 2020

tstromberg left a comment

tstromberg Feb 12, 2020

tstromberg Feb 12, 2020

tstromberg Feb 12, 2020

tstromberg Feb 12, 2020

medyagh commented Feb 13, 2020

minikube-pr-bot commented Feb 13, 2020

medyagh commented Feb 13, 2020

medyagh commented Feb 13, 2020

codecov-io commented Feb 13, 2020

minikube-pr-bot commented Feb 13, 2020

tstromberg left a comment

Create the Node subcommands #6556

Create the Node subcommands #6556

Conversation

sharifelgamal commented Feb 8, 2020 • edited Loading

k8s-ci-robot commented Feb 8, 2020

tstromberg left a comment

Choose a reason for hiding this comment

tstromberg Feb 12, 2020

Choose a reason for hiding this comment

tstromberg Feb 12, 2020

Choose a reason for hiding this comment

tstromberg Feb 12, 2020

Choose a reason for hiding this comment

tstromberg Feb 12, 2020

Choose a reason for hiding this comment

medyagh commented Feb 13, 2020

minikube-pr-bot commented Feb 13, 2020

medyagh commented Feb 13, 2020

medyagh commented Feb 13, 2020

codecov-io commented Feb 13, 2020

Codecov Report

minikube-pr-bot commented Feb 13, 2020

tstromberg left a comment

Choose a reason for hiding this comment

sharifelgamal commented Feb 8, 2020 •

edited

Loading