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

Support for multiple Elasticsearch nodes and sniffing #21928

Open
wants to merge 40 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@jbudz
Contributor

jbudz commented Aug 13, 2018

This adds support for node sniffing. The elasticsearch-js client supports this natively, but we create proxies that bypass the client. Two routes are used for searching and these are rewritten to use the client.

The last is used by Console, and not covered in this pull request - the first host will be used.

Configuration changes, ref:

  • elasticsearch.url (string) -> elasticsearch.hosts [(string)]
  • elasticsearch.sniffOnStart
  • elasticsearch.sniffInterval
  • elasticsearch.sniffOnConnectionFault
@elasticmachine

This comment was marked as outdated.

elasticmachine commented Aug 13, 2018

@jbudz jbudz force-pushed the jbudz:node-sniffing branch 2 times, most recently from ecd890b to f2776a4 Oct 16, 2018

@jbudz

This comment was marked as resolved.

Contributor

jbudz commented Oct 16, 2018

retest

1 similar comment
@jbudz

This comment was marked as resolved.

Contributor

jbudz commented Oct 16, 2018

retest

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

jbudz added some commits Jul 16, 2018

@jbudz jbudz force-pushed the jbudz:node-sniffing branch from 853be76 to 55743a0 Oct 16, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

jbudz added some commits Oct 16, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Oct 16, 2018

jbudz added some commits Oct 16, 2018

jbudz added some commits Dec 3, 2018

@elasticmachine

This comment was marked as outdated.

elasticmachine commented Dec 4, 2018

@tylersmalley

This comment has been minimized.

Member

tylersmalley commented Dec 10, 2018

@jbudz we should be pretty close there - mind merging in master, resolving the conflict, and my last comment?

jbudz added some commits Dec 10, 2018

@elasticmachine

This comment was marked as resolved.

elasticmachine commented Dec 10, 2018

jbudz added some commits Dec 10, 2018

@elasticmachine

This comment was marked as resolved.

elasticmachine commented Dec 10, 2018

@elasticmachine

This comment was marked as resolved.

elasticmachine commented Dec 11, 2018

@elasticmachine

This comment has been minimized.

elasticmachine commented Dec 11, 2018

@tylersmalley

Question and small nit regarding a comment. After that we're good.

@@ -564,7 +563,8 @@ export default function SenseEditor($el) {
const esMethod = req.method;
const esData = req.data;
const elasticsearchBaseUrl = chrome.getInjected('elasticsearchUrl');
// TODO: set url from ui settings
const elasticsearchBaseUrl = 'http://localhost:9200';

This comment has been minimized.

@tylersmalley

tylersmalley Dec 12, 2018

Member

Can we remove the TODO and replace it with a description of why we are using localhost instead of exposing the underlying ES hostname?

This comment has been minimized.

@jbudz

This comment has been minimized.

@bmcconaghy

bmcconaghy Dec 12, 2018

Contributor

This is likely to never work in the real world as most people won't be running their ES on the local machine, so seems like a bad thing to return for that. This would basically break the feature for most people. Could we maybe scrub out basic auth credentials?

This comment has been minimized.

@jbudz

jbudz Dec 12, 2018

Contributor

👍 , reverted in 6c8f1f4

...await Promise.all(plugins.map(({ spec }) => getTransform(spec)))
];
const settings = transforms.reduce((a, c) => c(a), rawSettings);
// transform deprecated core settings

This comment has been minimized.

@tylersmalley

tylersmalley Dec 12, 2018

Member

What was the motivation behind these changes? Are the still necessary after the blocking PR was merged?

This comment has been minimized.

@jbudz

jbudz Dec 12, 2018

Contributor

The short version is I had a really difficult time digging through the code originally and couldn't tell if we were expected to set by reference or value. Turns out it does both, probably by accident.

We could deeply deprecate by reference (plugin.b.c), but not shallowly (plugin.b). The weird part is it's not exactly shallow because it's one level deep.

So this sets everything by value. I can put it in a new PR if we want, np.

@elasticmachine

This comment has been minimized.

elasticmachine commented Dec 12, 2018

@elasticmachine

This comment has been minimized.

elasticmachine commented Dec 12, 2018

@jbudz

This comment has been minimized.

Contributor

jbudz commented Dec 13, 2018

cc @elastic/stack-monitoring lf review too if anyone gets time :). It's roughly a find and replace on xpack.monitoring.elasticsearch.url to xpack.elasticsearch.monitoring.hosts. url will be deprecated in 6.x and removed in the future.

@pickypg

Just have a comment on the deprecation, but otherwise LGTM.

return (settings, log) => {
const deprecatedUrl = get(settings, 'url');
if (!deprecatedUrl) return;
set(settings, 'hosts', [deprecatedUrl]);

This comment has been minimized.

@pickypg

pickypg Dec 13, 2018

Member

This seems unsafe if they have set hosts and forgotten to unset url. We should ensure that hosts is unset before doing so, or fail more horribly otherwise.

This comment has been minimized.

@jbudz

jbudz Dec 14, 2018

Contributor

👍 146a5d1

This comment has been minimized.

@pickypg

pickypg Dec 14, 2018

Member

Can we also add a separate log message that because both are set, elasticsearch.url is being ignored? That should more forcefully discourage leaving it set.

@@ -81,7 +81,7 @@ export const ml = (kibana) => {
const config = server.config();
return {
kbnIndex: config.get('kibana.index'),
esServerUrl: config.get('elasticsearch.url'),
esServerUrl: config.get('elasticsearch.hosts')[0]

This comment has been minimized.

@pickypg

pickypg Dec 13, 2018

Member

@elastic/ml-ui may want to improve what they're doing here now that we can get a full array.

if (!deprecatedUrl) {
return;
}
set(settings, 'hosts', [deprecatedUrl]);

This comment has been minimized.

@pickypg

pickypg Dec 13, 2018

Member

Same issue as non-Monitoring version.

This comment has been minimized.

@jbudz

jbudz Dec 14, 2018

Contributor

👍 146a5d1

jbudz added some commits Dec 14, 2018

@elasticmachine

This comment has been minimized.

elasticmachine commented Dec 14, 2018

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