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

Improved, unified, sniffing heuristics #24871

Closed
andrewvc opened this issue May 24, 2017 · 13 comments
Closed

Improved, unified, sniffing heuristics #24871

andrewvc opened this issue May 24, 2017 · 13 comments
Labels
:Core/Infra/REST API REST infrastructure and utilities discuss >enhancement

Comments

@andrewvc
Copy link
Contributor

andrewvc commented May 24, 2017

Currently, sniffing can be problematic due to its reliance of http endpoints meaning the node is sniffable. Part of this has bled through in #12792 . Its also complicated in that its hard or impossible for users to define arbitrary sniffing criteria.

Goals

I think a good set of sniffing heuristics would have the following heuristics:

  1. Simple: It should do the right thing for most people out of the box without them having to think too much about it
  2. Powerful: It should let users who want to customize it be able to
  3. Safe: It should make doing bad things (e.g. directing traffic toward master only nodes) difficult or impossible.

The Proposal

I propose that clients alter their sniffing logic to one of two modes:

  1. Match everything except dedicated masters
  2. Match everything, filtered by node metadata, except dedicated masters

So, we would use the same node metadata that is normally used for rack awareness to restrict sniffing based on a custom user defined property. A client, such as the logstash elasticsearch output, might expose the following config options:

elasticsearch {
  sniffing => true
  sniffing_filter => { my_key => myvalue }
}

A client would construct the list of sniffed nodes by:

  1. Querying the /_nodes/http API
  2. Including only nodes that have either:
    1. a roles array that includes data
    2. a roles array that does not include master
  3. Filter that list down further based on exact matches of key == value in the node metadata if present. If the user has not enabled this option then all results will be returned.

I think this algorithm meets all the goals discussed earlier.

@abeyad
Copy link

abeyad commented May 24, 2017

@javanna WDYT?

@andrewvc
Copy link
Contributor Author

It would also be nice to discuss which nodes should be queried for this data.

I would propose that clients:

  1. Be provided with a seed node list to connect to
  2. Hit the /_nodes/http endpoint on one of those nodes
  3. Perform subsequent sniffing requests against nodes on the sniffed list
  4. Be allowed to re-sniff the seed list if the sniffed nodes list fails

The idea here is that it would be advisable for users to seed their clients with stable nodes, which in many installations will be the master nodes. It is also advisable that clients try to keep load off the master nodes, hence making them a last resort.

There is a risk here, that in large installs, if the data nodes were to all disappear a thundering herd of /_nodes/http requests could overwhelm the master nodes, but I imagine that would only fill their request queues and would stabilize rather quickly.

@javanna javanna removed their assignment May 24, 2017
@javanna
Copy link
Member

javanna commented May 24, 2017

This seems like a general discussion on what Elasticsearch REST clients should do compared to what they do now in terms of sniffing. We should hear what @elastic/es-clients think of this proposal.

@javanna
Copy link
Member

javanna commented May 24, 2017

I thought that the official clients already allowed to plug in custom selectors so that users can choose which nodes get selected. The java low level REST client doesn't support this yet but there's #21888 for it. Probably the default behaviour when sniffing should be adjusted as I think all nodes are selected by the official clients while master only nodes should be skipped. Apart from that I am not sure what else should change provided that the notion of selector is exposed and pluggable in every client.

@andrewvc
Copy link
Contributor Author

@javanna I took a look at the python and ruby clients, I can't find any support for this. Maybe I'm missing something? I also don't believe beats supports this either. Maybe @honzakral @karmi and @andrewkroh can confirm?

Perhaps other clients support this? At any rate, the forced omission of master-only nodes would be a great thing to standardize.

@andrewkroh
Copy link
Member

I also don't believe beats supports this either.

Correct, Beats does not currently have sniffing support. @7AC is working on go-elasticsearch which Beats will eventually use. Having support for this in the client library would be nice.

@GlenRSmith
Copy link
Contributor

@andrewvc
Copy link
Contributor Author

Thanks @GlenRSmith , it looks like the python client rejects master nodes. That's great! It would still be great if we could standardize on node settings filtering as well.

I mean, maybe this has been tackled before, but ideally clients would all have identical sniffing algorithms, which is what I'm trying to get at here.

@GlenRSmith
Copy link
Contributor

@andrewvc of course, and that's very much the broad intent - uniformity and parity among the low-level clients. Striving for that on this particular item isn't objectionable.

@honzakral
Copy link
Contributor

Currently there is the default behavior (filtering out master-only nodes) and the ability to override the callback used to filter out the nodes. In python all you have to do is supply your own implementation of the get_host_info (0) function and introduce any logic they wish to have which is documented (1).

Since this is an advanced functionality I think that is sufficient. So far I haven't had a single request for anything more structured/systematic. This approach is common to all the clients.

0 - https://github.com/elastic/elasticsearch-py/blob/master/elasticsearch/transport.py#L11-L28
1 - https://github.com/elastic/elasticsearch-py/blob/master/elasticsearch/transport.py#L48-L50

@russcam
Copy link
Contributor

russcam commented May 25, 2017

There's documentation for the sniffing behaviour of the .NET clients, which includes the ability to specify a predicate to determine which nodes in the cluster API calls should be executed on.

@colings86 colings86 added :Core/Infra/REST API REST infrastructure and utilities >enhancement labels May 25, 2017
@karmi
Copy link
Contributor

karmi commented May 25, 2017

The Ruby client doesn't do any filtering right now, there's an old open issue: elastic/elasticsearch-ruby#251. So far nobody showed any interest in the feature, though, so I've put on backburner. I've realized that the Sniffer class is not injectable via the constructor, which is maybe an oversight on our consistent "plugability" of everything, I'd like to add it.

The Ruby client, as all the official clients, supports supplying a custom connection selector, which allows people to programatically select nodes based on arbitrary criteria, eg. the node attributes.

Regarding the feature itself, I'm not sure I grasp what is exactly being suggested -- it looks to me like a feature request to add a sniffing_filter option to the clients. I think along the same lines as @honzakral, that it's more or less an advanced functionality, which people can implement via custom component classes, and passing them to the client.

@andrewvc
Copy link
Contributor Author

Having spoken to a variety of client authors, the consensus is not to have this discussion on the ES repo since this isn't an ES issue, closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
:Core/Infra/REST API REST infrastructure and utilities discuss >enhancement
Projects
None yet
Development

No branches or pull requests

9 participants