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

OpenStack cloud provider should register with instance id instead of instance name #57765

Closed
bsnchan opened this Issue Jan 2, 2018 · 8 comments

Comments

Projects
None yet
6 participants
@bsnchan
Contributor

bsnchan commented Jan 2, 2018

Is this a BUG REPORT or FEATURE REQUEST?:

/kind bug

What happened:

The OpenStack cloud provider currently uses the name of the kubelet node to register with the API server. However the name is not guaranteed to conform to the DNS-1123 subdomain spec.

Given a node name of worker/925f11f3-80af-4844-a00e-f0af521779d1, the following message appears in the kubelet logs:

Unable to register node "worker/925f11f3-80af-4844-a00e-f0af521779d1" with API server: Node "worker/925f11f3-80af-4844-a00e-f0af521779d1" is invalid: metadata.name: Invalid value: "worker/925f11f3-80af-4844-a00e-f0af521779d1": a DNS-1123 subdomain must consist of lower case alphanumeric characters, '-' or '.', and must start and end with an alphanumeric character (e.g. 'example.com', regex used for validation is 'a-z0-9?(.a-z0-9?)*')

What you expected to happen:

The OpenStack Cloud provider uses the instance id instead of the instance name. This will guarantee that the id conforms the DNS-1123 spec without making any assumptions on what the instance name may be.

How to reproduce it (as minimally and precisely as possible):

In the OpenStack console, rename a kubelet node to kubelet/1234 and attempt to register the kubelet node to the api server.

Anything else we need to know?:

Environment: OpenStack Newton

  • Kubernetes version (use kubectl version):
Client Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.4", GitCommit:"9befc2b8928a9426501d3bf62f72849d5cbcd5a3", GitTreeState:"clean", BuildDate:"2017-11-20T05:28:34Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"8", GitVersion:"v1.8.4", GitCommit:"9befc2b8928a9426501d3bf62f72849d5cbcd5a3", GitTreeState:"clean", BuildDate:"2017-11-20T05:17:43Z", GoVersion:"go1.8.3", Compiler:"gc", Platform:"linux/amd64"}
  • Cloud provider or hardware configuration: OpenStack
  • OS (e.g. from /etc/os-release): Ubuntu 14.04.5
  • Kernel (e.g. uname -a): Linux b4fdd4bc-cab9-44ff-9c99-44e91ff5b414 4.4.0-103-generic #126~14.04.1-Ubuntu SMP Mon Dec 4 19:33:04 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux
  • Install tools:
  • Others:
@bsnchan

This comment has been minimized.

Contributor

bsnchan commented Jan 2, 2018

/sig openstack

@k8s-ci-robot k8s-ci-robot added sig/openstack and removed needs-sig labels Jan 2, 2018

bsnchan added a commit to cloudfoundry-incubator/kubo-deployment that referenced this issue Jan 2, 2018

Adds openstack as a cloud-provider
- the BOSH director needs to have `human_readable_vm_names` set to false
in order for the kubelet to register with the apiserver successfully

See k8s issue: kubernetes/kubernetes#57765

[#153816342]
@dixudx

This comment has been minimized.

Member

dixudx commented Jan 4, 2018

/assign

I prefer to conform hostname to DNS-1123 subdomain spec. Already submitted a PR #57816 on this.

@bsnchan

This comment has been minimized.

Contributor

bsnchan commented Jan 4, 2018

@dixudx

I think the issue may be more than just the hostname. The hostname on my vm is set to 49ff9bab-a7de-490d-a428-c5282a5cf075

worker/3d977989-9e0f-4044-a29e-a367169145c7:/var/vcap/data/sys/log/kubelet$ hostname
49ff9bab-a7de-490d-a428-c5282a5cf075

Since the OpenStack cloud provider uses the instance name to register with the api server, I also get error messages like the following:

E0104 22:14:06.529695   10101 event.go:200] Server rejected event '&v1.Event{TypeMeta:v1.TypeMeta{Kind:"", APIVersion:""}, Ob
jectMeta:v1.ObjectMeta{Name:"worker/3d977989-9e0f-4044-a29e-a367169145c7.1506bb1a5a566fc9", GenerateName:"", Namespace:"defau
lt", SelfLink:"", UID:"", ResourceVersion:"", Generation:0, CreationTimestamp:v1.Time{Time:time.Time{sec:0, nsec:0, loc:(*tim
e.Location)(nil)}}, DeletionTimestamp:(*v1.Time)(nil), DeletionGracePeriodSeconds:(*int64)(nil), Labels:map[string]string(nil
), Annotations:map[string]string(nil), OwnerReferences:[]v1.OwnerReference(nil), Initializers:(*v1.Initializers)(nil), Finali
zers:[]string(nil), ClusterName:""}, InvolvedObject:v1.ObjectReference{Kind:"Node", Namespace:"", Name:"worker/3d977989-9e0f-
4044-a29e-a367169145c7", UID:"worker/3d977989-9e0f-4044-a29e-a367169145c7", APIVersion:"", ResourceVersion:"", FieldPath:""},
 Reason:"NodeAllocatableEnforced", Message:"Updated Node Allocatable limit across pods", Source:v1.EventSource{Component:"kub
elet", Host:"worker/3d977989-9e0f-4044-a29e-a367169145c7"}, FirstTimestamp:v1.Time{Time:time.Time{sec:63650700846, nsec:51590
8553, loc:(*time.Location)(0x5329a40)}}, LastTimestamp:v1.Time{Time:time.Time{sec:63650700846, nsec:515908553, loc:(*time.Loc
ation)(0x5329a40)}}, Count:1, Type:"Normal"}': 'Event "worker/3d977989-9e0f-4044-a29e-a367169145c7.1506bb1a5a566fc9" is inval
id: metadata.name: Invalid value: "worker/3d977989-9e0f-4044-a29e-a367169145c7.1506bb1a5a566fc9": may not contain '/'' (will
not retry!)

It seems like for OpenStack specifically, it sets it's node name to the name returned from the OpenStack metadata server (https://github.com/kubernetes/kubernetes/blob/master/pkg/cloudprovider/providers/openstack/openstack_instances.go#L61) which is not restricted to being DNS-1123 compliant.

@FengyunPan

This comment has been minimized.

FengyunPan commented Jan 8, 2018

@bsnchan The node name must match ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9], not contain '/'.

@dixudx

This comment has been minimized.

Member

dixudx commented Jan 8, 2018

I can confirm that Openstack can boot up instances with the name like xyz/abc. But this will break kubelet currently as stated above. For such cloud provider based nodes, the nodeName registered is retrieved from the provider, which should not be allowed when registering nodes for kubelet.

@bsnchan The node name must match ([A-Za-z0-9][-A-Za-z0-9_.]*)?[A-Za-z0-9], not contain '/'.

Agree. +1.

@FengyunPan @bsnchan I've added an extra check in PR #57816. PTAL. Thanks.

@voelzmo

This comment has been minimized.

voelzmo commented Jan 10, 2018

I think the question here is: Why did you choose to use metadata.name as the nodename? This is the display name of the server, a property made for humans by humans. Essentially it is a field that everybody should be able to fill in whatever they think their server should be called. This doesn't need to be unique and has no guarantees about format or length or character format. We've had a number of people putting in unicode characters in the display name, for example. Furthermore, it can be changed during runtime of the server, without rebooting.

Other possibilities for the nodename are:

  • instance UUID, which is a string in a defined format, guaranteed to be unique and is only separated by dashes. Found in metadata.uuid
  • If you would like to have some way to associate your nodename with whatever is shown in the Horizon dashboard as server display name, there is metadata.hostname. OpenStack already takes care to sanitize any weird display name. E.g. some weird display name like lol/what?This is weird. Ümlauts rule! gets a reasonable hostname:

screen shot 2018-01-10 at 08 52 49

What do you think about changing the source for selecting the nodename?

@FengyunPan

This comment has been minimized.

FengyunPan commented Jan 11, 2018

cc @dims

k8s-merge-robot added a commit that referenced this issue Jan 23, 2018

Merge pull request #58502 from dixudx/register_openstack_hostname
Automatic merge from submit-queue (batch tested with PRs 57867, 58490, 58502, 58134). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Openstack: register metadata.hostname as node name

**What this PR does / why we need it**:
Currently Openstack can boot up instances with the name like `xyz/abc`, which is not a valid kubelet node name. While `hostname` retrieved from `meta_data.json` has already been sanitized 
 by Openstack to valid DNS-1123 format string. It's safe to register this `metadata.hostname` as valid kubelet node name.

/kind bug
/sig openstack

**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 #57765

**Special notes for your reviewer**:
/assign @dims @FengyunPan 

**Release note**:

```release-note
Openstack: register metadata.hostname as node name
```

dims pushed a commit to dims/kubernetes that referenced this issue Feb 8, 2018

Merge pull request kubernetes#58502 from dixudx/register_openstack_ho…
…stname

Automatic merge from submit-queue (batch tested with PRs 57867, 58490, 58502, 58134). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Openstack: register metadata.hostname as node name

**What this PR does / why we need it**:
Currently Openstack can boot up instances with the name like `xyz/abc`, which is not a valid kubelet node name. While `hostname` retrieved from `meta_data.json` has already been sanitized 
 by Openstack to valid DNS-1123 format string. It's safe to register this `metadata.hostname` as valid kubelet node name.

/kind bug
/sig openstack

**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#57765

**Special notes for your reviewer**:
/assign @dims @FengyunPan 

**Release note**:

```release-note
Openstack: register metadata.hostname as node name
```
@anguslees

This comment has been minimized.

Member

anguslees commented May 16, 2018

Why did you choose to use metadata.name as the nodename?

Once upon a time, the cloud provider's (only!) role was to discover a dynamic list of nodes by regex. In that world, using the nodename made perfect sense because the admin could then configure it to be something like cluster17-node46, with the obvious use of regex patterns to group the relevant nodes.

Fast forward a few years, and this style of node registration is long gone, and there is no need to make the node name human readable, and in fact numerous reasons to choose other unique identifiers. There is still resistance to removing "human-friendly" node names, however.

In the end, I didn't see a good alternative that would please everyone (and meet the required technical character set/length constraints). I stuck with metadata.name because at least it was under the control of the cluster admin, and so it could be obviously contrived to match our imposed k8s node name == openstack instance name constraint (once that constraint was communicated).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment