Move file-based discovery to core #33241

DaveCTurner · 2018-08-29T14:59:28Z

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 important 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. A followup PR will introduce the breaking change of
its removal.

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 #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 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

elasticmachine · 2018-08-29T14:59:39Z

Pinging @elastic/es-distributed

DaveCTurner · 2018-08-29T14:59:57Z

docs/reference/modules/discovery/zen.asciidoc

-
-If `discovery.zen.master_election.ignore_non_master_pings` is `true`, pings from nodes that are not master
-eligible (nodes where `node.master` is `false`) are ignored during master election; the default value is
+As part of the ping process a master of the cluster is either elected or joined


The rest of the changes in this file are just reformatting.

DaveCTurner · 2018-08-29T15:00:50Z

server/src/main/java/org/elasticsearch/discovery/zen/FileBasedUnicastHostsProvider.java

@@ -17,11 +17,12 @@
 * under the License.
 */

-package org.elasticsearch.discovery.file;
+package org.elasticsearch.discovery.zen;


I didn't think it worth introducing a new package just for this one class.

DaveCTurner · 2018-08-29T15:02:38Z

plugins/discovery-file/build.gradle

-        into 'config'
-    }
-}
-
 task setupSeedNodeAndUnicastHostsFile(type: DefaultTask) {


These integration tests remain in the plugin, to be removed at a later date. Since we plan on using file-based discovery for most of the cluster integration tests there seems little value in moving this.

DaveCTurner · 2018-08-29T15:04:42Z

distribution/src/config/discovery-file/unicast_hosts.txt

+# NOTE: all lines starting with a `#` are comments, and comments must exist on
+# lines of their own (i.e. comments cannot begin in the middle of a line)
+#
+# NOTE: Set `discovery.zen.hosts_provider: file` to enable file-based discovery


NB this file is now included in all distributions, rather than just in the plugin. The only change, apart from reformatting, is this NOTE.

Reverted this and gone back to having this file installed by the plugin. It cannot simply be removed because of BWC: for instance the following command would break if the directory were not there.

some-clever-stuff.sh > $ES_PATH_CONF/discovery-file/unicast_hosts.txt

… managers

The docs are clear enough.

jasontedor

I left a comment about deprecating the configuration path.

jasontedor · 2018-08-29T17:59:40Z

server/src/main/java/org/elasticsearch/discovery/zen/FileBasedUnicastHostsProvider.java

-        this.unicastHostsFilePath = environment.configFile().resolve("discovery-file").resolve(UNICAST_HOSTS_FILE);
+    public FileBasedUnicastHostsProvider(Settings settings, Path configFile) {
+        super(settings);
+        this.unicastHostsFilePath = configFile.resolve("discovery-file").resolve(UNICAST_HOSTS_FILE);


I think that we should deprecate the use of this location. So something like:

use configFile.resolve(UNICAST_HOSTS_FILE)

if that file does not exist, fall back to configFile.resolve("discovery-file").resolve(UNICAST_HOSTS_FILE) and deprecation warn

in 7.0.0 remove the ability to fallback to configFile.resolve("discovery-file").resolve(UNICAST_HOSTS_FILE)

Good plan. I pushed 8f9fcef.

Support both paths for BWC reasons, but emit a deprecation warning if the old one is used. Also, replace the unnecessary full stack trace emitted if the file isn't there with a single-line warning. A stack trace is still emitted if the file seems to be there but reading it fails.

jasontedor

LGTM.

jasontedor · 2018-08-30T04:28:46Z

Two comments:

I think we should add an entry to the migration guide on the backport (saying it is deprecated), and in master (saying it's removed); those can be separate
I think the label on this PR should be >breaking and not >breaking-java

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

In elastic#33241 we moved the file-based discovery functionality to core Elasticsearch, but preserved the `discovery-file` plugin, and support for the existing location of the `unicast_hosts.txt` file, for BWC reasons. This commit completes the removal of this plugin.

DaveCTurner · 2018-08-30T06:58:31Z

I opened #33259 to add this to the 6.5 release notes, and #33257 to remove it from master (including an update to the migration docs).

My intention was that #33257 is the >breaking change and this one is merely >breaking-java, since this one is backported to 6.x, but perhaps I misunderstood something.

Recently-merged PR #33241 broke the docs build, and this fixes it.

* 6.x: Mute test watcher usage stats output [Rollup] Fix FullClusterRestart test TEST: Disable soft-deletes in ParentChildTestCase TEST: Disable randomized soft-deletes settings Integrates soft-deletes into Elasticsearch (#33222) drop `index.shard.check_on_startup: fix` (#32279) Fix AwaitsFix issue number Mute SmokeTestWatcherWithSecurityIT testsi [DOCS] Moves ml folder from x-pack/docs to docs (#33248) TEST: mute more SmokeTestWatcherWithSecurityIT tests [DOCS] Move rollup APIs to docs (#31450) [DOCS] Rename X-Pack Commands section (#33005) Fixes SecurityIntegTestCase so it always adds at least one alias (#33296) TESTS: Fix Random Fail in MockTcpTransportTests (#33061) (#33307) MINOR: Remove Dead Code from PathTrie (#33280) (#33306) Fix pom for build-tools (#33300) Lazy evaluate java9home (#33301) SQL: test coverage for JdbcResultSet (#32813) Work around to be able to generate eclipse projects (#33295) Different handling for security specific errors in the CLI. Fix for #33230 (#33255) [ML] Refactor delimited file structure detection (#33233) SQL: Support multi-index format as table identifier (#33278) Enable forbiddenapis server java9 (#33245) [MUTE] SmokeTestWatcherWithSecurityIT flaky tests Add region ISO code to GeoIP Ingest plugin (#31669) (#33276) Don't be strict for 6.x Update serialization versions for custom IndexMetaData backport Replace IndexMetaData.Custom with Map-based custom metadata (#32749) Painless: Fix Bindings Bug (#33274) SQL: prevent duplicate generation for repeated aggs (#33252) TEST: Mute testMonitorClusterHealth Fix serialization of empty field capabilities response (#33263) Fix nested _source retrieval with includes/excludes (#33180) [DOCS] TLS file resources are reloadable (#33258) Watcher: Ensure TriggerEngine start replaces existing watches (#33157) Ignore module-info in jar hell checks (#33011) Fix docs build after #33241 [DOC] Repository GCS ADC not supported (#33238) Upgrade to latest Gradle 4.10 (#32801) Fix/30904 cluster formation part2 (#32877) Move file-based discovery to core (#33241) HLRC: add client side RefreshPolicy (#33209) [Kerberos] Add unsupported languages for tests (#33253) Watcher: Reload properly on remote shard change (#33167) Fix classpath security checks for external tests. (#33066) [Rollup] Only allow aggregating on multiples of configured interval (#32052) Added deprecation warning for rescore in scroll queries (#33070) Apply settings filter to get cluster settings API (#33247) [Rollup] Re-factor Rollup Indexer into a generic indexer for re-usability (#32743) HLRC: create base timed request class (#33216) HLRC: Use Optional in validation logic (#33104) Painless: Add Bindings (#33042)

Today we use a special unicast hosts provider, the `MockUncasedHostsProvider`, in many integration tests, to deal with the dynamic nature of the allocation of ports to nodes. However elastic#33241 allows us to use file-based discovery to achieve the same goal, so the special test-only `MockUncasedHostsProvider` is no longer required. This change removes `MockUncasedHostProvider` and replaces it with file-based discovery in tests based on `EsIntegTestCase`.

Today we use a special unicast hosts provider, the `MockUncasedHostsProvider`, in many integration tests, to deal with the dynamic nature of the allocation of ports to nodes. However #33241 allows us to use file-based discovery to achieve the same goal, so the special test-only `MockUncasedHostsProvider` is no longer required. This change removes `MockUncasedHostProvider` and replaces it with file-based discovery in tests based on `EsIntegTestCase`.

Today we use a special unicast hosts provider, the `MockUncasedHostsProvider`, in many integration tests, to deal with the dynamic nature of the allocation of ports to nodes. However elastic#33241 allows us to use file-based discovery to achieve the same goal, so the special test-only `MockUncasedHostsProvider` is no longer required. This change removes `MockUncasedHostProvider` and replaces it with file-based discovery in tests based on `EsIntegTestCase`.

In #33241 we moved the file-based discovery functionality to core Elasticsearch, but preserved the `discovery-file` plugin, and support for the existing location of the `unicast_hosts.txt` file, for BWC reasons. This commit completes the removal of this plugin.

Today we use a special unicast hosts provider, the `MockUncasedHostsProvider`, in many integration tests, to deal with the dynamic nature of the allocation of ports to nodes. However #33241 allows us to use file-based discovery to achieve the same goal, so the special test-only `MockUncasedHostsProvider` is no longer required. This change removes `MockUncasedHostProvider` and replaces it with file-based discovery in tests based on `EsIntegTestCase`. Backport of #33554 to 6.x.

DaveCTurner added >test Issues or PRs that are addressing/adding tests :Distributed Coordination/Discovery-Plugins Anything related to our integration plugins with EC2, GCP and Azure >breaking-java v7.0.0 v6.5.0 labels Aug 29, 2018

DaveCTurner commented Aug 29, 2018

View reviewed changes

DaveCTurner added 5 commits August 29, 2018 16:49

Pickiness

588c6c2

This is a config file. We need to say this so it's removed by package…

e2e59df

… managers

Fix name of config directory environment variable

13db1a7

Let's not bother shipping this extra config file

b581cb7

The docs are clear enough.

Reinstate unicast_hosts.txt

e2ef488

jasontedor reviewed Aug 29, 2018

View reviewed changes

DaveCTurner added 2 commits August 29, 2018 23:46

Fix path to file in docs

432a88c

jasontedor approved these changes Aug 30, 2018

View reviewed changes

DaveCTurner merged commit 47859e5 into elastic:master Aug 30, 2018

DaveCTurner deleted the 2018-08-29-file-based-hosts-provider branch August 30, 2018 05:43

This was referenced Aug 30, 2018

Remove discovery-file plugin #33257

Merged

Note discovery-file's deprecation in release notes #33259

Merged

DaveCTurner added a commit that referenced this pull request Aug 30, 2018

Fix docs build after #33241

51cbc61

Recently-merged PR #33241 broke the docs build, and this fixes it.

DaveCTurner added a commit that referenced this pull request Aug 30, 2018

Fix docs build after #33241

49399b3

Recently-merged PR #33241 broke the docs build, and this fixes it.

DaveCTurner mentioned this pull request Sep 10, 2018

Use file-based discovery not MockUncasedHostsProvider #33554

Merged

This was referenced Sep 13, 2018

Use file-based discovery not MockUncasedHostsProvider (backport of #33554) #33658

Merged

Fix port assignment and discovery in tests #33675

Closed

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move file-based discovery to core #33241

Move file-based discovery to core #33241

DaveCTurner commented Aug 29, 2018 •

edited

Loading

elasticmachine commented Aug 29, 2018

DaveCTurner Aug 29, 2018

DaveCTurner Aug 29, 2018

DaveCTurner Aug 29, 2018

DaveCTurner Aug 29, 2018

DaveCTurner Aug 29, 2018

jasontedor left a comment

jasontedor Aug 29, 2018

DaveCTurner Aug 29, 2018

jasontedor left a comment

jasontedor commented Aug 30, 2018

DaveCTurner commented Aug 30, 2018

Move file-based discovery to core #33241

Move file-based discovery to core #33241

Conversation

DaveCTurner commented Aug 29, 2018 • edited Loading

elasticmachine commented Aug 29, 2018

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor Aug 29, 2018

Choose a reason for hiding this comment

DaveCTurner Aug 29, 2018

Choose a reason for hiding this comment

jasontedor left a comment

Choose a reason for hiding this comment

jasontedor commented Aug 30, 2018

DaveCTurner commented Aug 30, 2018

DaveCTurner commented Aug 29, 2018 •

edited

Loading