-
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
Use profile name as cluster/node name #6200
Conversation
Signed-off-by: Zhongcheng Lao <[email protected]>
Signed-off-by: Zhongcheng Lao <[email protected]>
Signed-off-by: Zhongcheng Lao <[email protected]>
Can one of the admins verify this patch? |
Hi @laozc. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: laozc The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Codecov Report
@@ Coverage Diff @@
## master #6200 +/- ##
==========================================
- Coverage 38.38% 37.97% -0.42%
==========================================
Files 124 128 +4
Lines 8378 8722 +344
==========================================
+ Hits 3216 3312 +96
- Misses 4739 4983 +244
- Partials 423 427 +4
|
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.
Thank you for this PR ! and this is a good idea !
two thigns
1- please use profile name as node name
2- please make sure the kubeadm config templates also receive the node name.
/ok-to-test |
Signed-off-by: Zhongcheng Lao <[email protected]>
All Times minikube: [ 124.025748 131.830177 125.899917] Average minikube: 127.251947 Averages Time Per Log
|
Signed-off-by: Zhongcheng Lao <[email protected]>
All Times minikube: [ 123.943295 125.486914 125.486131] Average minikube: 124.972113 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.
we also need to change the "clusterName: kubernetes" in the config template files
for example this one
https://github.com/kubernetes/minikube/blob/master/pkg/minikube/bootstrapper/bsutil/template/template.go#L80
make sure to change for all versions v1alpha, beta...
cmd/minikube/cmd/start.go
Outdated
@@ -907,6 +907,17 @@ func generateCfgFromFlags(cmd *cobra.Command, k8sVersion string, drvName string) | |||
out.T(out.SuccessType, "Using image repository {{.name}}", out.V{"name": repository}) | |||
} | |||
|
|||
var kubeNodeName string | |||
if drvName == driver.None { |
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.
Is there any reason that the none driver should not have the same node name as Profile name ?
I would rather we treat all drivers same way as much as possible.
so lets have the profile name for none driver unless there is a good reason for it that I dont know ?
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.
In the none driver the host is created by the user not minikube. If we're not setting the default host name, it will result a different node name with the host.
This issue would not exist for other drivers as those drivers create the VM and set the host name.
We may need to ask user to set the profile name to keep the same behavior as other drivers which may be confusing.
Let me work on it. |
Signed-off-by: Zhongcheng Lao <[email protected]>
Signed-off-by: Zhongcheng Lao <[email protected]>
All Times minikube: [ 122.312750 126.452668 128.741744] Average Minikube (PR 6200): 127.021444 Averages Time Per Log
|
All Times minikube: [ 230.471049 231.896780 255.411575] Average minikube: 239.259802 Averages Time Per Log
|
All Times minikube: [ 115.674457 114.736020 113.975103] Average minikube: 114.795194 Averages Time Per Log
|
/retest-this-please |
Signed-off-by: Zhongcheng Lao <[email protected]>
All Times minikube: [ 96.419035 97.156901 95.234201] Average minikube: 96.270046 Averages Time Per Log
|
All Times minikube: [ 95.112039 96.441117 99.449515] Average Minikube (PR 6200): 97.855276 Averages Time Per Log
|
The node name is always
minikube
nowadays and this PR allows user to pick up one with--node-name
.If not specified, the node will be named
minikube
for the default profile name (minikube
) or[PROFILE]-minikube
.For
none
driver the default node name will be set to the host name.fixes #2800 #4063