Skip to content
This repository has been archived by the owner on Dec 1, 2018. It is now read-only.

remove host_id from influxdb #1662

Merged
merged 1 commit into from
Jun 1, 2017
Merged

remove host_id from influxdb #1662

merged 1 commit into from
Jun 1, 2017

Conversation

andyxning
Copy link
Contributor

@andyxning andyxning commented May 25, 2017 •
edited by DirectXMan12
Loading

According to doc about host_id, it is

Cloud-provider specified or user specified Identifier of a node

Since host_id is retrievable using the node name, host_id is redundant.

/cc @kubernetes/heapster-maintainers @piosz @DirectXMan12

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 25, 2017
@k8s-reviewable
Copy link

This change is Reviewable

@andyxning andyxning force-pushed the remove_host_id_from_influxdb branch from 83d3429 to ca34410 Compare May 25, 2017 05:39
@DirectXMan12
Copy link
Contributor

do we even need host_id at all? Is nodename not sufficient?

@andyxning
Copy link
Contributor Author

andyxning commented May 25, 2017 •
edited
Loading

I am not sure whether both host_id and nodename are all the same always not matter deployed on bare metal or GCE or AWS.

host_id is the value of node.Spec.ExternalID. nodename is the value of node.Name

@andyxning
Copy link
Contributor Author

Friendly ping @piosz @DirectXMan12 :)

@andyxning
Copy link
Contributor Author

@piosz @DirectXMan12 PTAL. :)

@DirectXMan12
Copy link
Contributor

sure, but as has been previously suggested, if you know one, you can probably find the other -- they're effectively two parallel axes, so we shouldn't store both and cause extra indices to be created in the TSDB.

@andyxning andyxning force-pushed the remove_host_id_from_influxdb branch from ca34410 to cc9fe8e Compare May 31, 2017 23:40
@andyxning
Copy link
Contributor Author

andyxning commented Jun 1, 2017 •
edited
Loading

@DirectXMan12 Comments addressed. PTAL.

After reading code about heapster and kubernetes. The value for host_id is the value of node.Spec.ExternalID. The value for ExternalID is specific to vendors. Its value maybe the same as node.Name(i.e., nodename in heapster) and may not.

So, if we can have an endpoint to query ExternalID from node.Name(i.e., nodename in heapster), we can remove host_id from InfluxDB.

DirectXMan12
DirectXMan12 approved these changes Jun 1, 2017
View reviewed changes
Copy link
Contributor

@DirectXMan12 DirectXMan12 left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@DirectXMan12 DirectXMan12 changed the title remove host_id from influxdb when duplicate with nodename remove host_id from influxdb Jun 1, 2017
@DirectXMan12
Copy link
Contributor

I've updated the PR description a bit to match the latest discussion. Merging.

@DirectXMan12 DirectXMan12 merged commit cdd47d9 into kubernetes-retired:master Jun 1, 2017
@andyxning andyxning deleted the remove_host_id_from_influxdb branch June 2, 2017 00:16
@andyxning
Copy link
Contributor Author

@DirectXMan12 Many thanks. 👏

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

@DirectXMan12 DirectXMan12 DirectXMan12 approved these changes

Assignees
No one assigned
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sink/influxdb
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@andyxning @k8s-reviewable @DirectXMan12 @googlebot @k8s-ci-robot