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.

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

kubeadm: Let the --node-name flag flow down to --hostname-override for the kubelet #64706

Merged

Conversation

liztio
Copy link
Contributor

@liztio liztio commented Jun 4, 2018

What this PR does / why we need it:

Kubeadm-initialised kubelet uses provided hostname if present

If --node-name is passed in to kubeadm init, --hostname-override will be
passed to kubelet. This prevents timeout errors for kubeadm init.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes kubernetes/kubeadm#846

Special notes for your reviewer:
Depends on #64624 to work fully, but can safely merged before hand.

Release note:

[action required] The `--node-name` flag for kubeadm now dictates the Node API object name the
kubelet uses for registration, in all cases but where you might use an in-tree cloud provider.
If you're not using an in-tree cloud provider, `--node-name` will set the Node API object name.
If you're using an in-tree cloud provider, you MUST make `--node-name` match the name the
in-tree cloud provider decides to use.

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 4, 2018
@k8s-ci-robot k8s-ci-robot added area/kubeadm sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. labels Jun 4, 2018
@liztio
Copy link
Contributor Author

liztio commented Jun 4, 2018

/cc @kubernetes/sig-cluster-lifecycle-pr-reviews

If --node-name is passed in to `kubeadm init`, `--hostname-override` will be
passed to kubelet.
@liztio liztio force-pushed the kubeadm-hostname-override branch from e2e2f84 to b42564a Compare June 4, 2018 18:26
@liztio
Copy link
Contributor Author

liztio commented Jun 4, 2018

/test pull-kubernetes-e2e-gce

@luxas luxas changed the title Kubeadm hostname override kubeadm: Let the --node-name override option flow down to --hostname-override for the kubelet Jun 4, 2018
@luxas luxas changed the title kubeadm: Let the --node-name override option flow down to --hostname-override for the kubelet kubeadm: Let the --node-name flag flow down to --hostname-override for the kubelet Jun 4, 2018
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 4, 2018
Copy link
Member

@luxas luxas left a comment

Choose a reason for hiding this comment

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

Code-wise I'm happy with this, but we have to clearly document the change in behavior we have here.
Earlier --node-name let you change the SAN of the cert so it matches the name the kubelet decides to have by using autodetection.
Now we basically force the kubelet to have the node name you give to kubeadm.

Further, it seems --hostname-override has no effect at all when using at least in-tree cloud providers: #64661

/lgtm
/assign @timothysc
for the second approval

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 4, 2018
Copy link
Member

@timothysc timothysc left a comment

Choose a reason for hiding this comment

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

generally I approve, but I have a question about usage and how this overlaps with your other patch, b/c people can muck it up pretty easily.

@@ -78,7 +79,12 @@ func buildKubeletArgMap(nodeRegOpts *kubeadmapi.NodeRegistrationOptions, registe
kubeletFlags["resolv-conf"] = "/run/systemd/resolve/resolv.conf"
}

// TODO: Pass through --hostname-override if a custom name is used?
// Make sure the node name we're passed will work with Kubelet
if nodeRegOpts.Name != "" && nodeRegOpts.Name != nodeutil.GetHostname("") {
Copy link
Member

Choose a reason for hiding this comment

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

given that the most common thing folks do is use fully qualified domain names I'm almost wondering if we should force tolower on the names similar to your other patch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just tested this, and it appears this already happens automatically:

ubuntu@ip-172-31-84-104:~$ sudo /tmp/kubeadm init -v 1 --pod-network-cidr=192.168.0.0/16 --node-name ALLCAPS.horse

ubuntu@ip-172-31-84-104:~$ kubectl get nodes
NAME            STATUS    ROLES     AGE       VERSION
allcaps.horse   Ready     master    44s       v1.11.0-beta.0

I don't particularly know how, but I think this is a nonissue either way.

Copy link
Member

Choose a reason for hiding this comment

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

The nodeutil.GetHostname() func lowercases automatically:

// GetHostname returns OS's hostname if 'hostnameOverride' is empty; otherwise, return 'hostnameOverride'.
func GetHostname(hostnameOverride string) string {
	hostname := hostnameOverride
	if hostname == "" {
		nodename, err := os.Hostname()
		if err != nil {
			glog.Fatalf("Couldn't determine hostname: %v", err)
		}
		hostname = nodename
	}
	return strings.ToLower(strings.TrimSpace(hostname))
}

@luxas
Copy link
Member

luxas commented Jun 5, 2018

/milestone v1.11
/status approved-for-milestone
/kind feature
/priority critical-urgent

@k8s-ci-robot k8s-ci-robot added this to the v1.11 milestone Jun 5, 2018
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. and removed release-note-none Denotes a PR that doesn't merit a release note. labels Jun 5, 2018
Copy link
Member

@fabriziopandini fabriziopandini left a comment

Choose a reason for hiding this comment

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

thanks @liztio!
/lgtm
/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini, liztio, luxas

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-github-robot
Copy link

[MILESTONENOTIFIER] Milestone Pull Request: Up-to-date for process

@fabriziopandini @liztio @luxas @timothysc

Pull Request Labels
  • sig/cluster-lifecycle: Pull Request will be escalated to these SIGs if needed.
  • priority/critical-urgent: Never automatically move pull request out of a release milestone; continually escalate to contributor and SIG through all available channels.
  • kind/feature: New functionality.
Help

@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubeadm cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/critical-urgent Highest priority. Must be actively worked on as someone's top priority right now. release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. sig/cluster-lifecycle Categorizes an issue or PR as relevant to SIG Cluster Lifecycle. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Set the kubelet --hostname-override flag conditionally at kubeadm init time
6 participants