-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Fix multinode cluster creation for VM drivers #7700
Conversation
[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 |
Codecov Report
@@ Coverage Diff @@
## master #7700 +/- ##
==========================================
- Coverage 36.48% 36.22% -0.27%
==========================================
Files 147 148 +1
Lines 9162 9217 +55
==========================================
- Hits 3343 3339 -4
- Misses 5430 5488 +58
- Partials 389 390 +1
|
/ok-to-test |
kvm2 Driver |
TestFunctional/parallel/MultiNode
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
integeration test failure needs attention
Travis tests have failedHey @sharifelgamal, TravisBuddy Request Identifier: 950bc5f0-8005-11ea-ac79-d7a1e92f42eb |
kvm2 Driver Times for Minikube (PR 7700): [59.60069063499999 61.226016404000006 62.377339624] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [24.253607765 28.168776019 27.313193147000003] Averages Time Per Log
|
kvm2 Driver Times for Minikube (PR 7700): [64.635815507 65.946127612 63.456418284] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [25.9206135 23.975291520000003 25.579708308] Averages Time Per Log
|
Travis tests have failedHey @sharifelgamal, TravisBuddy Request Identifier: 76fe9800-8035-11ea-ac79-d7a1e92f42eb |
kvm2 Driver Times for Minikube (PR 7700): [73.21120695399999 63.648737697 63.56360985300001] Averages Time Per Log
docker Driver |
kvm2 Driver Times for Minikube (PR 7700): [64.173319022 62.811509994000005 66.60189722] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [24.282152896 27.837081859999994 25.952439893] Averages Time Per Log
|
kvm2 Driver Times for Minikube (PR 7700): [66.02202185900002 64.83706205899999 63.42499518099999] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [27.503617784 25.107442984 26.006683977999998] Averages Time Per Log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please name the PR in such a way that makes sense for release notes.
@@ -57,5 +57,5 @@ func UniqueProfileName(prefix string) string { | |||
return "minikube" | |||
} | |||
// example: prefix-20200413T162239-3215 | |||
return fmt.Sprintf("%s-%s-%d", prefix, time.Now().Format("20060102T150405"), os.Getpid()) | |||
return fmt.Sprintf("%s-%s-%d", prefix, time.Now().Format("20060102150405"), os.Getpid()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be part of another PR? I don't understand its relevance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the multinode test that I added fails without this change
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do u mind sharing the failed message in an issue and add in the PR description, that this fixes that ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's no real message, kubeadm join just fails with exit code 70
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sharifelgamal can u update the comment with the right example
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example containerd-20200417104139-6189
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed
kvm2 Driver Times for Minikube (PR 7700): [62.965772325 63.647177850000006 63.97799089200001] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [25.310121515 25.513365378 23.693296197] Averages Time Per Log
|
kvm2 Driver Times for Minikube (PR 7700): [63.66640616400001 60.078892640999996 65.763345634] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [35.234676338999996 24.921866190000003 25.937115455] Averages Time Per Log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, plz let wait for a integraion tests to pass.
linux virtualbox, kvm, docker and none and hyperkit
/retest |
kvm2 Driver Times for Minikube (PR 7700): [62.76367389400001 65.462877132 63.720044285] Averages Time Per Log
docker Driver Times for Minikube (PR 7700): [25.596129033 25.534473208 25.770170040999997] Averages Time Per Log
|
NOTE: this does not fix multinode clusters for the docker or podman drivers, that will be worked on in a subsequent PR.
Changing the timestamp format in UniqueProfileName fixes an issue with kubeadm join:
Before
After: