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.

Sign up for GitHub

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 experimental warning for multinode clusters #7934

Merged
merged 5 commits into from
May 1, 2020
Merged

add experimental warning for multinode clusters #7934

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d09ae65
add experimental warning for multinode clusters
sharifelgamal Apr 29, 2020
a2aaea8
swap to WarningT
sharifelgamal Apr 29, 2020
965e8da
fix multinode check in node_add
sharifelgamal Apr 29, 2020
5914fb2
extra newline
sharifelgamal May 1, 2020
fa039b1
Merge branch 'master' of github.com:kubernetes/minikube into warn-exp
sharifelgamal May 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion cmd/minikube/cmd/completion.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -288,7 +288,7 @@ __minikube_bash_source <(__minikube_convert_bash_to_zsh)
return nil
}

// GenerateBashCompletion generates the completion for the bash shell
// GenerateFishCompletion generates the completion for the bash shell
func GenerateFishCompletion(w io.Writer, cmd *cobra.Command) error {
_, err := w.Write([]byte(boilerPlate))
if err != nil {
Expand Down
7 changes: 5 additions & 2 deletions cmd/minikube/cmd/node_add.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,8 +56,11 @@ var nodeAddCmd = &cobra.Command{
}

// Make sure to decrease the default amount of memory we use per VM if this is the first worker node
if len(cc.Nodes) == 1 && viper.GetString(memory) == "" {
cc.Memory = 2200
if len(cc.Nodes) == 1 {
warnAboutMultiNode()
Copy link
Member

Choose a reason for hiding this comment

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

is this the only part of the code we could detect it is multi node?
I think I saw somewhere else too

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the only time we care about warning the user is when they are starting a multinode cluster, so just minikube start or the first minikube node add which is what this is detecting

Copy link
Member

Choose a reason for hiding this comment

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

so this condition " len(cc.Nodes) == 1 && viper.GetString(memory) == ""

would detect both
minikube node add
and also
minikube start -n=3?

wouldn;t the len(cc.Nodes) be 3 in that case and not trigger this if?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you're right that we shouldn't put the check inside this if because of the memory check. there'a separate call to warnAboutMultiNode() that will detect the minikube start -n 3 case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

fixed this check. the other check is in start.go

if viper.GetString(memory) == "" {
cc.Memory = 2200
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems unrelated - can it be moved to a second PR for release notes-sake?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the memory check already existed, I just moved a little of the logic around to make it more clear

}
}

if err := node.Add(cc, n); err != nil {
Expand Down
5 changes: 5 additions & 0 deletions cmd/minikube/cmd/start.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,7 @@ func startWithDriver(starter node.Starter, existing *config.ClusterConfig) (*kub
if driver.BareMetal(starter.Cfg.Driver) {
exit.WithCodeT(exit.Config, "The none driver is not compatible with multi-node clusters.")
} else {
warnAboutMultiNode()
for i := 1; i < numNodes; i++ {
nodeName := node.Name(i + 1)
n := config.Node{
Expand All @@ -305,6 +306,10 @@ func startWithDriver(starter node.Starter, existing *config.ClusterConfig) (*kub
return kubeconfig, nil
}

func warnAboutMultiNode() {
out.WarningT("Multi-node clusters are currently experimental and might exhibit unintended behavior.\nTo track progress on multi-node clusters, see https://github.com/kubernetes/minikube/issues/7538.")
}

func updateDriver(driverName string) {
v, err := version.GetSemverVersion()
if err != nil {
Expand Down