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

Adds basic support for node IDs. #2661

Merged
merged 5 commits into from Jan 19, 2017
Merged

Adds basic support for node IDs. #2661

merged 5 commits into from Jan 19, 2017

Conversation

slackpad
Copy link
Contributor

This puts in some basic support for a node ID by having agents make on up at startup and include it in their Serf tags, which is the minimum required for Raft server management with the V2 library.

We had originally intended to add it to the catalog as part of this change, but it's not that useful there unless we also expose it via DNS, etc. so we will hold off on that for now.

@slackpad
Copy link
Contributor Author

Talked to some folks internally and we need it at least exposed /v1/catalog (not DNS) so I'll add that before committing this.

This fixes #2663 and fixes #1899. It's not super related to this PR,
but the startup time changes that this PR brings made this a lot worse
so I was able to track it down.
@slackpad
Copy link
Contributor Author

Fixes #2663 and #1899 along the way (see latest commit).

@slackpad slackpad added this to the 0.7.3 milestone Jan 18, 2017
Copy link
Contributor

@kyhavlov kyhavlov left a comment

Choose a reason for hiding this comment

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

Looks good 👍 , just one small thing

@@ -33,6 +33,8 @@ It returns a JSON body like this:
```javascript
[
{
"ID": "40e4a748-2192-161a-0510-9bf59fe950b5",
"Node": "foobar",
Copy link
Contributor

Choose a reason for hiding this comment

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

this seems like a duplicate/typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops - thanks!

@slackpad slackpad merged commit acbc228 into master Jan 19, 2017
@slackpad slackpad deleted the f-node-id branch January 19, 2017 00:02

// If we still don't have a valid node ID, make one.
if config.NodeID == "" {
id, err := uuid.GenerateUUID()
Copy link
Contributor

Choose a reason for hiding this comment

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

This would be the place where we could hook into github.com/shirou/gopsutil:

import "github.com/shirou/gopsutil"
info, err := host.Info()
if info != nil && info.HostID != "" {
  config.NodeID = types.NodeID(info.HostID)
}

If the ID came from gopsutil then I wouldn't necessarily want to persist it in the data directory, but there shouldn't be any harm in that either since the ID coming from gopsutil and the OS should outlive the contents of whatever is in the data directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah - it might not be a bad idea to save it even from gopsutil in case we improve the algorithm over there and people upgrade Consul in-place, but that's probably not a huge issue either way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants