Add node-name flag to `init` phase #48594

GheRivero · 2017-07-07T09:58:13Z

What this PR does / why we need it: Allow to specify a node-name instead of relaying in os.Hostname()
This is useful where kubelet use the name given by the cloud-provider to
register the node.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): Partially fix: kubernetes/kubeadm#64

Release note:

Added new flag to `kubeadm init`: --node-name, that lets you specify the name of the Node object that will be created

k8s-ci-robot · 2017-07-07T09:58:21Z

Hi @GheRivero. 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 /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

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. I understand the commands that are listed here.

luxas

There is more than this needed to be done.
Default cfg.NodeName in cmd/kubeadm/app/cmd/defaults.go to the hostname.
Then only use cfg.NodeName for that purpose.

Replace all os.Hostname() and node.GetHostname("") instances with cfg.NodeName.

luxas · 2017-07-07T13:14:25Z

/ok-to-test
/release-note

luxas

Thanks @GheRivero!

/lgtm

cc @caesarxuchao @wojtek-t for cherrypick-candidate

luxas · 2017-07-07T17:51:58Z

/retest

dmmcquay · 2017-07-07T18:04:21Z

/lgtm

luxas · 2017-07-07T20:29:11Z

Please update bazel etc. to fix the tests

…

On 07 Jul 2017, at 21:32, k8s-ci-robot ***@***.***> wrote: @GheRivero: The following tests failed, say /retest to rerun them all: Test name Commit Details Rerun command pull-kubernetes-unit 6ea7a91 link /test pull-kubernetes-unit pull-kubernetes-federation-e2e-gce 6ea7a91 link /test pull-kubernetes-federation-e2e-gce pull-kubernetes-verify 6ea7a91 link /test pull-kubernetes-verify Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. — You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or mute the thread.

luxas · 2017-07-08T09:45:44Z

/retest

wojtek-t · 2017-07-10T13:47:10Z

@luxas - thanks a lot for providing the rationale. This sounds reasonable to me.

@luxas @GheRivero - please add a release note to the PR (explaining what user problem it is fixing) and I will approve the cherrypick.

luxas · 2017-07-10T20:02:51Z

/retest

/lgtm

luxas · 2017-07-10T20:03:12Z

@wojtek-t Release note added, should be good to go

dmmcquay · 2017-07-11T01:28:12Z

/test pull-kubernetes-e2e-kops-aws

dmmcquay · 2017-07-11T01:28:26Z

/lgtm

luxas · 2017-07-11T07:53:35Z

/retest

luxas

Minor nits from latest push -- thanks for adding a validation function!
The presubmits are broken anyway, so it makes sense to fix the nits while waiting.

luxas · 2017-07-11T13:41:18Z

cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go

+	}{
+		{"", nil, false},                        // ok if not provided
+		{"1234", nil, true},                     // supported
+		{hostname, nil, true},                   // supported


I think hard coding the value here makes sense instead of getting something from os.Hostname as that might depend on the env. What about if you run the unit test on a node with an upper-case hostname?

luxas · 2017-07-11T13:41:43Z

cmd/kubeadm/app/apis/kubeadm/validation/validation_test.go

@@ -177,6 +203,7 @@ func TestValidateIPNetFromString(t *testing.T) {
 }

 func TestValidateMasterConfiguration(t *testing.T) {
+	nodename, _ := os.Hostname()


Just have something static here

luxas · 2017-07-11T13:44:24Z

cmd/kubeadm/app/cmd/defaults.go

@@ -71,5 +72,13 @@ func setInitDynamicDefaults(cfg *kubeadmapi.MasterConfiguration) error {
 		}
 	}

+	if cfg.NodeName == "" {


This can actually be trimmed down to cfg.NodeName = node.GetHostname(cfg.NodeName)

Please do this same thing in join.go

luxas · 2017-07-11T13:48:43Z

cmd/kubeadm/app/apis/kubeadm/validation/validation.go

+func ValidateNodeName(nodename string, fldPath *field.Path) field.ErrorList {
+	allErrs := field.ErrorList{}
+	if node.GetHostname(nodename) != nodename {
+		allErrs = append(allErrs, field.Invalid(fldPath, nodename, "nodename is not valid"))


nit: nodename is not valid, must be lower case

Allow to specify a node-name instead of relaying in `os.Hostname()` This is useful where kubelet use the name given by the cloud-provider to register the node. Partially fix: kubernetes/kubeadm#64

GheRivero · 2017-07-12T16:28:45Z

/retest

luxas

Very well. Thanks for rebasing @GheRivero

/lgtm

luxas · 2017-07-12T19:14:18Z

/retest

k8s-github-robot · 2017-07-12T19:16:22Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: GheRivero, dmmcquay, luxas

Associated issue: 64

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~cmd/kubeadm/OWNERS~~ [luxas]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-07-12T20:52:37Z

Automatic merge from submit-queue

…48538-upstream-release-1.7 Automatic merge from submit-queue Automated cherry pick of #48594 #48538 Cherry pick of #48594 #48538 on release-1.7. #48594: Add node-name flag to `init` phase #48538: Add node-name flag to `join` phase

k8s-cherrypick-bot · 2017-07-13T07:58:29Z

Commit found in the "release-1.7" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked.

Automatic merge from submit-queue kubeadm: Implementing the kubeconfig phase fully **What this PR does / why we need it:** This contains implementation of kubeconfig phases in kubeadm, which is part of the wider effort of implementing phases in kubeadm, previously in alpha stage. The original proposal for this activity can be found [here](https://github.com/kubernetes/kubeadm/pull/156/files) and related comments. Kubeadm phase implementation checklist is defined [here](kubernetes/kubeadm#267) Common implementation guidelines and principles for all phases are defined [here](https://docs.google.com/document/d/1VQMyFIVMfRGQPP3oCUpfjiWtOr3pLxp4g7cP-hXQFXc/edit?usp=sharing) This PR implements: - [x] kubeadm phase kubeconfig - [x] kubeadm phase kubeconfig all - [x] kubeadm phase kubeconfig admin - [x] kubeadm phase kubeconfig kubelet - [x] kubeadm phase kubeconfig scheduler - [x] kubeadm phase kubeconfig controller-manager - [x] kubeadm phase kubeconfig user **Which issue this PR fixes:** kubernetes/kubeadm#350 **Special notes for your reviewer:** This PR implements the second phases of kubeadm init; implementation of this PR follow the same approach already used for #48196 (cert phases). Please note that: - the API - phase\kubeconfig.go - is now totally free by any UX concerns, and implements only the core logic for kubeconfig generation. - the UX - cmd\phase\kubeconfig.go - now takes charge of UX commands and kubeadm own's rules for kubeconfig files in /etc/kubernetes folder (e.g. create only if not already exists) - The PR includes also a fix for a regression on a unit test for phase certs introduced by #48594 and few minor code changes in phase certs introduced to avoid code duplication between the two phases.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 7, 2017

k8s-github-robot assigned errordeveloper and dmmcquay Jul 7, 2017

k8s-github-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. release-note-label-needed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jul 7, 2017

GheRivero mentioned this pull request Jul 7, 2017

kubeadm on AWS cloud provider deadlocks because of insufficent permissions kubernetes/kubeadm#330

Closed

luxas reviewed Jul 7, 2017

View reviewed changes

k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. release-note-label-needed labels Jul 7, 2017

timothysc added this to the v1.7 milestone Jul 7, 2017

timothysc added cherrypick-candidate sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jul 7, 2017

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 7, 2017

k8s-ci-robot assigned luxas Jul 7, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 7, 2017

luxas approved these changes Jul 7, 2017

View reviewed changes

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 7, 2017

dmmcquay approved these changes Jul 7, 2017

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 10, 2017

dmmcquay approved these changes Jul 11, 2017

View reviewed changes

luxas mentioned this pull request Jul 11, 2017

kubeadm's os.Hostname() isn't getting the FQDN the expected way? (causes a fail to join a node in 1.7.0 when hostnameOverride is used) #48617

Closed

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 11, 2017

luxas reviewed Jul 11, 2017

View reviewed changes

This was referenced Jul 12, 2017

kubeadm join doesn't work for 1.7. #48687

Closed

Automated cherry pick of #48594 #48538 #48791

Merged

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2017

Add node-name flag to init phase

8181fd2

Allow to specify a node-name instead of relaying in `os.Hostname()` This is useful where kubelet use the name given by the cloud-provider to register the node. Partially fix: kubernetes/kubeadm#64

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 12, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 12, 2017

luxas approved these changes Jul 12, 2017

View reviewed changes

k8s-github-robot merged commit 80531cc into kubernetes:master Jul 12, 2017

k8s-cherrypick-bot removed the cherrypick-candidate label Jul 13, 2017

fabriziopandini mentioned this pull request Jul 21, 2017

kubeadm: Implementing the kubeconfig phase fully #49419

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add node-name flag to `init` phase #48594

Add node-name flag to `init` phase #48594

GheRivero commented Jul 7, 2017 •

edited by luxas

Loading

k8s-ci-robot commented Jul 7, 2017

luxas left a comment

luxas commented Jul 7, 2017

luxas left a comment

luxas commented Jul 7, 2017

dmmcquay commented Jul 7, 2017

luxas commented Jul 7, 2017 via email

luxas commented Jul 8, 2017

wojtek-t commented Jul 10, 2017

luxas commented Jul 10, 2017

luxas commented Jul 10, 2017

dmmcquay commented Jul 11, 2017

dmmcquay commented Jul 11, 2017

luxas commented Jul 11, 2017

luxas left a comment

luxas Jul 11, 2017

GheRivero Jul 12, 2017

luxas Jul 11, 2017

GheRivero Jul 12, 2017

luxas Jul 11, 2017

luxas Jul 11, 2017

GheRivero Jul 12, 2017

luxas Jul 11, 2017

GheRivero Jul 12, 2017

GheRivero commented Jul 12, 2017

luxas left a comment

luxas commented Jul 12, 2017

k8s-github-robot commented Jul 12, 2017

k8s-github-robot commented Jul 12, 2017

k8s-cherrypick-bot commented Jul 13, 2017

Add node-name flag to init phase #48594

Add node-name flag to init phase #48594

Conversation

GheRivero commented Jul 7, 2017 • edited by luxas Loading

k8s-ci-robot commented Jul 7, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 7, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 7, 2017

dmmcquay commented Jul 7, 2017

luxas commented Jul 7, 2017 via email

luxas commented Jul 8, 2017

wojtek-t commented Jul 10, 2017

luxas commented Jul 10, 2017

luxas commented Jul 10, 2017

dmmcquay commented Jul 11, 2017

dmmcquay commented Jul 11, 2017

luxas commented Jul 11, 2017

luxas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GheRivero commented Jul 12, 2017

luxas left a comment

Choose a reason for hiding this comment

luxas commented Jul 12, 2017

k8s-github-robot commented Jul 12, 2017

k8s-github-robot commented Jul 12, 2017

k8s-cherrypick-bot commented Jul 13, 2017

Add node-name flag to `init` phase #48594

Add node-name flag to `init` phase #48594

GheRivero commented Jul 7, 2017 •

edited by luxas

Loading