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

Implement multi-node cluster capabilities #6787

Merged
merged 50 commits into from
Mar 21, 2020

Conversation

sharifelgamal
Copy link
Collaborator

@sharifelgamal sharifelgamal commented Feb 24, 2020

Fixes #6505
Begins to address #94

You can spin up a multinode cluster in 2 ways:

minikube start --nodes=2
minikube start
minikube node add

You can spin up and down individual nodes by name:

minikube node stop node2
minikube node start node2

Multi-node clusters are still experimental and won't be fully featured.

NOTE: This PR does not account for CNI, that will come in a subsequent PR. Currently, you will need to install CNI manually for multi-node to work properly.

@k8s-ci-robot 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 Feb 24, 2020
@k8s-ci-robot
Copy link
Contributor

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

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 the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 24, 2020
pkg/minikube/node/node.go Outdated Show resolved Hide resolved
@@ -47,16 +47,23 @@ var nodeAddCmd = &cobra.Command{
if nodeName == "" {
name = profile + strconv.Itoa(len(mc.Nodes)+1)
Copy link
Member

Choose a reason for hiding this comment

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

while we are at it, why not rename all mc to cc and make sure we call it cc everywhere in the code

@codecov-io
Copy link

codecov-io commented Feb 25, 2020

Codecov Report

Merging #6787 into master will not change coverage by %.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6787   +/-   ##
=======================================
  Coverage   36.98%   36.98%           
=======================================
  Files         145      145           
  Lines        9161     9161           
=======================================
  Hits         3388     3388           
  Misses       5355     5355           
  Partials      418      418           

@k8s-ci-robot 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 25, 2020
@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2020
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 20, 2020
n, _, err = node.Retrieve(cc, nodeName)
if err != nil {
out.FailureT("Node {{.nodeName}} does not exist.", out.V{"nodeName": nodeName})
exit.WithError("", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

Combine these two calls into one which generates the proper exit code:

exit.WithCodeT(exit.Unavailable, "node ... does not exist")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}
if numNodes > 1 {
if driver.BareMetal(driverName) {
out.T(out.Meh, "The none driver is not compatible with multi-node clusters.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Exit rather than ignoring the user:

exit.WithCodeT(exit.Config, "....")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

out.T(out.Meh, "The none driver is not compatible with multi-node clusters.")
} else {
for i := 1; i < numNodes; i++ {
nodeName := fmt.Sprintf("m%02d", i+1)
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed this formatting in two locations. Add a node.Name(int) function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, added.

return "", errors.Wrap(err, "generating bootstrap token")
}

/*cp, err := config.PrimaryControlPlane(&cc)
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove debugging comment

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done.

@sharifelgamal sharifelgamal changed the title WIP: Implement multi-node cluster capabilities Implement multi-node cluster capabilities Mar 20, 2020
@k8s-ci-robot 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 Mar 20, 2020
@tstromberg
Copy link
Contributor

/ok-to-test

@k8s-ci-robot k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Mar 20, 2020
@minikube-pr-bot
Copy link

All Times minikube: [ 65.822917 65.489519 64.712395]
All Times Minikube (PR 6787): [ 65.617288 66.311797 62.593569]

Average minikube: 65.341610
Average Minikube (PR 6787): 64.840885

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6787) |
+----------------------+-----------+--------------------+
| minikube v           |  0.228925 |           0.207446 |
| Creating kvm2        | 41.684456 |          42.867836 |
| Preparing Kubernetes |  0.754215 |          19.903093 |
| Pulling images       |           |                    |
| Launching Kubernetes | 20.794955 |                    |
| Waiting for cluster  |  0.252219 |           0.232201 |
+----------------------+-----------+--------------------+

Copy link
Contributor

@tstromberg tstromberg left a comment

Choose a reason for hiding this comment

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

I have a comment I would like to see fixed before release, but I'm OK with merging this PR to get miles on it.

Plumbing the name of the ContainerRuntime to configure into the provision library may require trickery, such as adding it to an unused field in say, SwarmOptions. I look forward to your creative solution.

return fmt.Sprintf("%s---%s", cc.Name, n.Name)
}

// ClusterNameFromMachine retrieves the cluster name embedded in the machine name
Copy link
Contributor

Choose a reason for hiding this comment

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

I see pain and suffering on this path. Instead of adding structure to an unstructured field & calling config.Load outside of main, we should be passing the config object around.

@minikube-pr-bot
Copy link

All Times minikube: [ 69.090527 65.157437 64.588555]
All Times Minikube (PR 6787): [ 65.998519 64.871788 65.604756]

Average minikube: 66.278840
Average Minikube (PR 6787): 65.491688

Averages Time Per Log

+----------------------+-----------+--------------------+
|         LOG          | MINIKUBE  | MINIKUBE (PR 6787) |
+----------------------+-----------+--------------------+
| minikube v           |  0.210650 |           0.217285 |
| Creating kvm2        | 42.059744 |          42.759002 |
| Preparing Kubernetes |  0.748763 |          20.362227 |
| Pulling images       |           |                    |
| Launching Kubernetes | 21.485618 |                    |
| Waiting for cluster  |  0.071921 |           0.229123 |
+----------------------+-----------+--------------------+

@tstromberg
Copy link
Contributor

TestFunctional/parallel/SSHCmd failure is interesting and unexpected:

--- FAIL: TestFunctional/parallel/SSHCmd (0.35s)
functional_test.go:702: (dbg) Run:  out/minikube-linux-amd64 -p functional-20200320T163208.831234864-7487 ssh "echo hello"
functional_test.go:707: [out/minikube-linux-amd64 -p functional-20200320T163208.831234864-7487 ssh echo hello] = "hello\n", want = "hello\r\n"

@sharifelgamal
Copy link
Collaborator Author

I have no idea why my change to ssh would cause a carriage return to get swallowed up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add JoinCluster function to bootstrapper
6 participants