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: better default NodeName based on machine-id #52625

Closed
wants to merge 1 commit into from

Conversation

anguslees
Copy link
Member

@anguslees anguslees commented Sep 18, 2017

NodeName != hostname.

  • NodeName needs to be unique (cluster-wide)
  • System hostname (aka utsname.nodename) may not be unique,
    particularly on cloud providers that don't provide internal DNS (eg:
    openstack).
  • System hostname can change in response to DHCP/DNS replies
    (eg Node names change after reboot from short hostname to fqdn kubeadm#144)
  • NodeName does not need to resolve (historically this was required however)

This patch alters the --node-name default to "node-%m" if
/etc/machine-id is available, otherwise falls back to the existing
hostname logic.

Also: Rename the pre-flight HostnameCheck to NodenameCheck to
reduce confusion, and remove the unnecessary warning when the NodeName
fails to resolve.

What this PR does / why we need it:

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

Special notes for your reviewer:

Release note:

@k8s-ci-robot k8s-ci-robot added 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 Sep 18, 2017
@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: anguslees
We suggest the following additional approver: mikedanese

Assign the PR to them by writing /assign @mikedanese in a comment when ready.

Associated issue: 144

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

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

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
Copy link

Adding do-not-merge/release-note-label-needed because the release note process has not been followed.
One of the following labels is required "release-note", "release-note-action-required", or "release-note-none".
Please see: https://github.com/kubernetes/community/blob/master/contributors/devel/pull-requests.md#write-release-notes-if-needed.

@k8s-github-robot k8s-github-robot added the do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. label Sep 18, 2017
@anguslees
Copy link
Member Author

Discuss. I appreciate that I'm walking into a minefield here.

@anguslees
Copy link
Member Author

Other "better" defaults might be something based on POSIX gethostid() (usually an interface IPv4 address), ethernet MAC addresses, freshly-generated large random numbers (UUIDs), etc.

The general idea is that a) we have lots of actually-unique identifiers around us to choose from, and b) it's only the default - the value will be "frozen" in the etcd node data, and presumably on the kubelet as the increasingly-inaccurately-named --hostname-override flag.

NodeName != hostname.

- NodeName needs to be unique (cluster-wide)
- System hostname (aka utsname.nodename) may not be unique,
  particularly on cloud providers that don't provide internal DNS (eg:
  openstack).
- System hostname can change in response to DHCP/DNS replies
  (eg kubernetes/kubeadm#144)
- NodeName does not need to resolve (historically this was true however)

This patch alters the `--node-name` default to "node-%m" if
`/etc/machine-id` is available, otherwise falls back to the existing
hostname logic.

Also: Rename the pre-flight `HostnameCheck` to `NodenameCheck` to
reduce confusion, and remove the unnecessary warning when the NodeName
fails to resolve.
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Sep 18, 2017

@anguslees: The following tests failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-node-e2e f515e4e link /test pull-kubernetes-node-e2e
pull-kubernetes-e2e-gce-etcd3 f515e4e link /test pull-kubernetes-e2e-gce-etcd3

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.

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.

@k8s-github-robot
Copy link

@anguslees PR needs rebase

@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2017
@luxas luxas assigned timothysc and luxas and unassigned lukemarsden and dmmcquay Oct 20, 2017
@luxas
Copy link
Member

luxas commented Oct 20, 2017

@anguslees This would be a breaking change. Why would you like your machine id to be the node name?
Most people do want to have the hostname as the node name, although they don't necessarily have to equal.

@timothysc
Copy link
Member

I agree with @luxas here.

@anguslees
Copy link
Member Author

anguslees commented Nov 1, 2017

This would be a breaking change.

Hrm. I agree, but it shouldn't be. On the kubeadm side, this only changes the initial default used when joining a new node, and will have no effect on existing nodes. Nodes created under the old scheme can coexist with the new scheme just fine, since k8s just treats the nodename as an opaque unique identifier.

The value we use here does have to be coordinated with the kubelet's value for --hostname-override (even without this PR). This suggests that kubeadm should generate some kubelet config that sets this value (or the reverse - kubeadm should remove it's --node-name flag and fetch the value from wherever kubelet declares it). Currently that means command line flags in a systemd drop-in (or similar), which I note we don't have an existing mechanism for modifying in kubeadm.

Do we have an existing plan around kubelet bootstrap flags (required before dynamic kubelet conf can take over)? Shall I expand this PR to move this kubelet option into a distro-agnostic /etc/kubernetes/kubelet-conf.yaml file? (I'm not familiar with the details of the dynamic kubelet conf proposal)

Why would you like your machine id to be the node name?

For the reasons cited in the original comment: the machine id is unique and fixed for the lifetime of the root filesystem, unlike the system hostname. The system hostname is not a great choice for a unique ID.

@timothysc
Copy link
Member

The use case here flummoxes me, and I'd like to close.

@anguslees could you describe in more detail how the current configuration fails under a certain environment.

@anguslees
Copy link
Member Author

anguslees commented Nov 22, 2017

@timothysc My apologies, I thought the use case was more broadly understood, and the original comment would be sufficient context - let me explain further:

My personal example is a baremetal cluster composed of mixed coreos and an in-house minimal-image distro. The nodes are all PXE-installed and get addresses via DHCP. Nothing allocates unique hostnames for them in DNS, so they all have the default system hostname (os.Hostname returns "localhost" in the case of coreos, "bananapi" in the case of my in-house arm distro). In this (quite normal!) environment, hostnames are obviously a terrible source of unique machine IDs.

Another use case I have experience with is OpenStack clusters. There is no internal-DNS in typical openstack setups (unlike GCE, for example), and so configuring unique hostnames requires additional processes to ensure that openstack instance names are never reused (openstack instance names are not usually unique IDs).

In addition, there are some (eg: DHCP, dynamic DNS) environments where the hostname is assigned by the network, and may change over the lifetime of the machine.

For all these reasons, "hostname" is simply not a good source for a unique/static machine ID.

This PR uses /etc/machine-id, which is unique and fixed for the lifetime of the local harddisk (exactly what we want). See #52625 (comment) above for a bunch of other alternatives, all of which are more appropriate to use as sources for unique node names than the result of os.Hostname.

@timothysc Is that clearer?
I feel we're using os.Hostname here because of our GCE legacy, where hostnames are unique (and resolvable, etc), and os.Hostname is the natural interface into GCE's notion of unique VM ID. Sadly, these properties are absolutely not true of os.Hostname in other environments. We've finally removed the assumption that node names must be resolvable; I feel it's now time to tackle the assumption that hostnames are unique. The easy place to do this is for new node installs, where there is no migration challenge and the fix is as easy as picking some other default.

If this needs to be discussed in some other forum first, I would welcome a guide through the required bureaucracy.

@timothysc
Copy link
Member

@anguslees Thanks for the details, but this more the exception than the norm.
I think having an --opt-in use case makes sense, but do endorse changing the defaults.

@anguslees
Copy link
Member Author

anguslees commented Nov 30, 2017

@timothysc I feel you haven't engaged with the central issue here that os.Hostname value is not unique (or fixed) except by artificial construction, and I can't form a response without understanding why you think it is the most preferred source for a node identifier :/

Would a combined solution be more palatable? We could append a unique value to os.Hostname, for example (which addresses the "unique" but not "fixed" concerns - but at least its better than the present situation, and keeps the "human friendly" property of hostnames).

@timothysc
Copy link
Member

The defaults work for 80% of standard use cases, and I understand that they don't work for a smaller subset. I'm ok with adding an option to allow an override, but not currently changing the default for now seems like a reasonable approach to get what you want. The reason I don't want to change the default is legacy operators are used to their setup and environment and don't have your specific issue b/c their hostnames are unique.

I'm more concerned about standard UX, then strict correctness.

@dims
Copy link
Member

dims commented Apr 1, 2018

time to close this? please reopen if needed

/close

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. 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.

8 participants