Move file-based discovery to core/server #33030
Comments
ywelsch
added
:Delivery/Build
|
Pinging @elastic/es-core-infra |
|
Pinging @elastic/es-distributed |
|
The issue here is that we should not simply drop the file-based discovery plugin, because this would be a breaking change to any scripts that provision ES nodes with this plugin installed. I propose:
This means that setting |
|
@DaveCTurner yes thats very manageable for us 👍 |
|
@ywelsch What about adding file based discovery as a dependency of the test framework? It would slightly complicate the file based discovery unit tests, but we didn't add it to server in the first place for a reason right? |
|
That’s not enough, it needs to be available as production code for external-to-us reasons. |
That was not in this issue description? I only see test uses. |
|
@rjernst I will reach out to discuss. |
|
I was not aware of a non-test-related reason for doing this. I am also not aware of the reasons why we kept it out of Are there other plugins that are available to integ and REST tests? I ask so I can look at the plumbing involved. |
There are non-test production use-cases, for example cloud providers and Docker. One of the big advantages of file-based discovery is that we reload the file when discovery is needed so that hosts can be dynamically added. This is unlike the unicast host list which is not dynamic. That it is a file is also convenient for deployment.
I am not either. And I do not see a compelling reason to keep it out of
We make the Let us go with the simple approach of moving to |
|
Makes sense, thanks @jasontedor. |
Today we support a static list of seed hosts in core Elasticsearch, and allow a dynamic list of seed hosts to be provided via a file using the `discovery-file` plugin. In fact the ability to provide a dynamic list of seed hosts is increasingly useful in containerized environments, so this change moves this functionality to core Elasticsearch to avoid the need for a plugin. For BWC purposes the plugin still exists, but does nothing more than issue a warning when it is used. Furthermore, in order to start up nodes in integration tests we currently assign a known port to each node before startup, which unfortunately sometimes fails if another process grabs the selected port in the meantime. By moving the `discovery-file` functionality into the core product we can use it to avoid this race. Relates elastic#29244 Closes elastic#33030
Today we support a static list of seed hosts in core Elasticsearch, and allow a dynamic list of seed hosts to be provided via a file using the `discovery-file` plugin. In fact the ability to provide a dynamic list of seed hosts is increasingly useful, so this change moves this functionality to core Elasticsearch to avoid the need for a plugin. Furthermore, in order to start up nodes in integration tests we currently assign a known port to each node before startup, which unfortunately sometimes fails if another process grabs the selected port in the meantime. By moving the `discovery-file` functionality into the core product we can use it to avoid this race. This change also moves the expected path to the file from `$ES_PATH_CONF/discovery-file/unicast_hosts.txt` to `$ES_PATH_CONF/unicast_hosts.txt`. An example of this file is not included in distributions. For BWC purposes the plugin still exists, but does nothing more than create the example file in the old location, and issue a warning when it is used. We also continue to support the old location for the file, but warn about its deprecation. Relates #29244 Closes #33030
Today we support a static list of seed hosts in core Elasticsearch, and allow a dynamic list of seed hosts to be provided via a file using the `discovery-file` plugin. In fact the ability to provide a dynamic list of seed hosts is increasingly useful, so this change moves this functionality to core Elasticsearch to avoid the need for a plugin. Furthermore, in order to start up nodes in integration tests we currently assign a known port to each node before startup, which unfortunately sometimes fails if another process grabs the selected port in the meantime. By moving the `discovery-file` functionality into the core product we can use it to avoid this race. This change also moves the expected path to the file from `$ES_PATH_CONF/discovery-file/unicast_hosts.txt` to `$ES_PATH_CONF/unicast_hosts.txt`. An example of this file is not included in distributions. For BWC purposes the plugin still exists, but does nothing more than create the example file in the old location, and issue a warning when it is used. We also continue to support the old location for the file, but warn about its deprecation. Relates #29244 Closes #33030
Some of our integration tests as well as the REST tests currently use the unicast hosts list setting for node discovery. This list is statically defined at node startup, which makes it very inflexible. For the
AbstractDisruptionTestCases, for example, we do port scanning to find free ports in order to fix the ports used by each node so that we can properly setup the unicast hosts list before starting the nodes. This port scanning does not always work reliably.. For our REST tests, we have to wait for the first node to be started up and bound to a port before we can start up the other nodes, as we need to put the address:port of the first node into the unicast hosts list of the other nodes. If we had something more flexible to achieve the same, this could really simplify our test infrastructure while also making it more resilient. Fortunately, such a more flexible node discovery already exists: it's the file-based discovery which is currently packaged as a plugin. The proposal here is to move the file-based discovery to the server project, so that it can be readily used in the testing framework for bothAbstractDisruptionTestCaseas well as all REST tests./cc: @franekrichardson
The text was updated successfully, but these errors were encountered: