add experimental warning for multinode clusters #7934
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[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 |
k8s-ci-robot
added
the
approved
cmd/minikube/cmd/start.go
Outdated
| @@ -305,6 +306,10 @@ func startWithDriver(starter node.Starter, existing *config.ClusterConfig) (*kub | |||
| return kubeconfig, nil | |||
| } | |||
|
|
|||
| func warnAboutMultiNode() { | |||
| out.T(out.Tip, "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.") | |||
There was a problem hiding this comment.
I wasn't sure if I wanted to pollute std err with this message, but it makes sense. Fixed.
|
/ok-to-test |
k8s-ci-robot
added
the
ok-to-test
|
kvm2 Driver |
| @@ -57,6 +57,7 @@ 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) == "" { | |||
| warnAboutMultiNode() | |||
There was a problem hiding this comment.
is this the only part of the code we could detect it is multi node?
I think I saw somewhere else too
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
fixed this check. the other check is in start.go
k8s-ci-robot
added
size/S
|
kvm2 Driver Times for Minikube (PR 7934): [66.431633393 65.43663727 64.067846902] Averages Time Per Log docker Driver Times for Minikube (PR 7934): [28.232376244999998 28.905191753999997 28.897069663999993] Averages Time Per Log |
There was a problem hiding this comment.
approved but needs to do manual check that this has not affected the single cluster.
need to check the following:
- start single cluster with no args (should be given the default auto select)
- start single cluster with memory = 3000mb and should see that minikube respected it in reality.
| if len(cc.Nodes) == 1 { | ||
| warnAboutMultiNode() | ||
| if viper.GetString(memory) == "" { | ||
| cc.Memory = 2200 |
There was a problem hiding this comment.
This seems unrelated - can it be moved to a second PR for release notes-sake?
There was a problem hiding this comment.
the memory check already existed, I just moved a little of the logic around to make it more clear
|
Codecov Report
@@ Coverage Diff @@
## master #7934 +/- ##
==========================================
+ Coverage 35.71% 36.04% +0.33%
==========================================
Files 149 144 -5
Lines 9408 9196 -212
==========================================
- Hits 3360 3315 -45
+ Misses 5644 5480 -164
+ Partials 404 401 -3
|
|
kvm2 Driver Times for Minikube (PR 7934): [68.91616142000001 68.69140936200002 65.899790825] Averages Time Per Log docker Driver Times for Minikube (PR 7934): [27.493045504999998 27.911607212 27.851864929000005] Averages Time Per Log |
Before:
After: