Rebuild remote connections on profile changes #37678

Tim-Brooks · 2019-01-22T01:18:44Z

Currently remote compression and ping schedule settings are dynamic.
However, we do not listen for changes. This commit adds listeners for
changes to those two settings. Additionally, when those settings change
we now close existing connections and open new ones with the settings
applied.

Fixes #37201.

…tion_updates

elasticmachine · 2019-01-22T01:18:45Z

Pinging @elastic/es-distributed

Tim-Brooks · 2019-01-22T01:20:05Z

This is a POC for the change. I had to add a method to handle listening for more than two related settings. So please take a look at that as it is an area of the code I have not worked with much.

Additionally I need to add a test for setting changes in the RemoteClusterServiceTests. I will get to that in the next day or two.

jasontedor · 2019-01-22T01:33:16Z

server/src/main/java/org/elasticsearch/common/settings/AbstractScopedSettings.java

+     * Note: Only settings registered in {@link SettingsModule} can be changed dynamically.
+     * </p>
+     */
+    public synchronized void addAffixGroupUpdateConsumer(List<Setting.AffixSetting<?>> settings, BiConsumer<String, Settings> consumer) {


Can you pull this out into a separate change, leaving the usage here for reference? Then we can integrate this back after that has a separate review from the core/infra team.

I opened #37679

s1monw

left a question

s1monw · 2019-01-23T13:22:35Z

server/src/main/java/org/elasticsearch/transport/RemoteClusterService.java

+                        getNodePredicate(settings), proxyAddress);
+                    remoteClusters.put(clusterAlias, remote);
+                } else if (connectionProfileChanged(remote.getConnectionManager().getConnectionProfile(), connectionProfile)) {
+                    // New ConnectionProfile. Must tear down existing connection


if I understand this correctly we will cause existing connections to fail and further, if concurrently somebody is using is it will not be able to retry since it's close and there will be intermediate failures in such a case. I think in this case we have to somehow reconfigure instead of closing? I know I would like this to be simple but the sideeffects of a close of a connection while some consumer is "in the middle of something" might cause a lot of headaches since it's a rare event, hard to debug and to test. WDYT?

What you're describing is possible but will require changes at multiple levels of the networking stack. The ConnectionProfile is not currently mutable. So compression and ping intervals will need to be mutable. It also raises the question of whether connection timeout and handshake timeout would be mutable. I guess not since they are not dynamic settings currently. I think it would be very complicated to change the number of channels and their types, so that should stay static.

The compression change would be straightforward. For every request it we could just read from the volatile variable in the connection profile. Ping interval would be trickier because we would need to reschedule it to a different ping interval (probably at the completion of a ping-run).

It can be done, but it is uncertain to make the next release. It also would require reworking a lot with ConnectionProfile. Let me know if this is your preference and I will pursue updating this PR with those changes at some point in the future.

…tion_updates

Tim-Brooks · 2019-01-31T16:38:59Z

I just want to send a ping that this is still outstanding. Maybe we should have a discussion about what behavior we want here?

…tion_updates

Tim-Brooks · 2019-02-13T22:38:38Z

@jasontedor @ywelsch @s1monw - I know that we spoke about updating settings and accepting occasional failures earlier today.

However, when working on implementing a test for this scenario (that ccr following will recover) I encountered an issue. Both REMOTE_CLUSTER_PING_SCHEDULE and REMOTE_CLUSTER_COMPRESS mark REMOTE_CLUSTERS_SEEDS as a dependency. So you cannot currently submit a settings update without including that setting. Even if the seeds already exist, there must be seeds included in the settings update request.

java.lang.IllegalArgumentException: missing required setting [cluster.remote.leader_cluster.seeds] for setting [cluster.remote.leader_cluster.transport.compress]

	at __randomizedtesting.SeedInfo.seed([57EB59A912962E51:91509C043368EEF9]:0)
	at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:545)
	at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:476)
	at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:447)
	at org.elasticsearch.common.settings.AbstractScopedSettings.validate(AbstractScopedSettings.java:418)
	at org.elasticsearch.action.admin.cluster.settings.SettingsUpdater.updateSettings(SettingsUpdater.java:95)
	at org.elasticsearch.action.admin.cluster.settings.TransportClusterUpdateSettingsAction$1.execute(TransportClusterUpdateSettingsAction.java:182)
	at org.elasticsearch.cluster.ClusterStateUpdateTask.execute(ClusterStateUpdateTask.java:47)
	at org.elasticsearch.cluster.service.MasterService.executeTasks(MasterService.java:687)
	at org.elasticsearch.cluster.service.MasterService.calculateTaskOutputs(MasterService.java:310)
	at org.elasticsearch.cluster.service.MasterService.runTasks(MasterService.java:210)
	at org.elasticsearch.cluster.service.MasterService$Batcher.run(MasterService.java:142)
	at org.elasticsearch.cluster.service.TaskBatcher.runIfNotProcessed(TaskBatcher.java:150)
	at org.elasticsearch.cluster.service.TaskBatcher$BatchedTask.run(TaskBatcher.java:188)
	at org.elasticsearch.common.util.concurrent.ThreadContext$ContextPreservingRunnable.run(ThreadContext.java:681)
	at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.runAndClean(PrioritizedEsThreadPoolExecutor.java:252)
	at org.elasticsearch.common.util.concurrent.PrioritizedEsThreadPoolExecutor$TieBreakingPrioritizedRunnable.run(PrioritizedEsThreadPoolExecutor.java:215)
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
	at java.lang.Thread.run(Thread.java:834)

I assume this is fine? They need to re-add the seeds in order to add compression and ping schedule.

Tim-Brooks · 2019-02-13T22:41:50Z

I also updated the documentation in remote-clusters.asciidoc to describe the behavior. @lcawl do you mind taking a look?

ywelsch · 2019-02-15T09:37:42Z

However, when working on implementing a test for this scenario (that ccr following will recover) I encountered an issue. Both REMOTE_CLUSTER_PING_SCHEDULE and REMOTE_CLUSTER_COMPRESS mark REMOTE_CLUSTERS_SEEDS as a dependency. So you cannot currently submit a settings update without including that setting. Even if the seeds already exist, there must be seeds included in the settings update request.
I assume this is fine? They need to re-add the seeds in order to add compression and ping schedule.

I think this is fine as long as it's documented.

lcawl

I made a few additional edits in the documentation. LGTM and builds successfully

s1monw · 2019-02-19T12:41:22Z

I assume this is fine? They need to re-add the seeds in order to add compression and ping schedule.

the point here was that this should only be set when the cluster is added. I think we should add this as a recommendation.

Currently remote compression and ping schedule settings are dynamic. However, we do not listen for changes. This commit adds listeners for changes to those two settings. Additionally, when those settings change we now close existing connections and open new ones with the settings applied. Fixes elastic#37201.

…follow * elastic/master: (37 commits) Enable test logging for TransformIntegrationTests#testSearchTransform. stronger wording for ilm+rollover in docs (elastic#39159) Mute SingleNodeTests (elastic#39156) AwaitsFix XPackUsageIT#testXPackCcrUsage. Resolve concurrency with watcher trigger service (elastic#39092) Fix median calculation in MedianAbsoluteDeviationAggregatorTests (elastic#38979) [DOCS] Edits the remote clusters documentation (elastic#38996) add version 6.6.2 Revert "Mute failing test 20_mix_typless_typefull (elastic#38781)" (elastic#38912) Rebuild remote connections on profile changes (elastic#37678) Document 'max_size' parameter as shard size for rollover (elastic#38750) Add some missing toString() implementations (elastic#39124) Migrate Streamable to Writeable for cluster block package (elastic#37391) fix RethrottleTests retry (elastic#38978) Disable date parsing test in non english locale (elastic#39052) Remove BCryptTests (elastic#39098) [ML] Stop the ML memory tracker before closing node (elastic#39111) Allow retention lease operations under blocks (elastic#39089) ML refactor DatafeedsConfig(Update) so defaults are not populated in queries or aggs (elastic#38822) Fix retention leases sync on recovery test ...

Tim-Brooks added 7 commits January 9, 2019 17:47

WIP

d989ba2

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

e361f42

…tion_updates

WIP

000ee4e

WIP

1eb6150

WIP

c34ab4e

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

dbe3120

…tion_updates

WIP

c635cc1

Tim-Brooks added >bug :Distributed Coordination/Network Http and internode communication implementations v7.0.0 v6.7.0 labels Jan 22, 2019

Tim-Brooks requested review from javanna, s1monw and jasontedor January 22, 2019 01:18

jasontedor reviewed Jan 22, 2019

View reviewed changes

Tim-Brooks mentioned this pull request Jan 22, 2019

Add ability to listen to group of affix settings #37679

Merged

s1monw suggested changes Jan 23, 2019

View reviewed changes

Tim-Brooks added 6 commits January 23, 2019 18:55

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

50ab278

…tion_updates

Fix test

1eef1bc

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

d384ee8

…tion_updates

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

4f30f06

…tion_updates

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

38d740d

…tion_updates

Testing

131169b

Tim-Brooks requested a review from s1monw January 31, 2019 16:37

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

f1d87f2

…tion_updates

jasontedor added v8.0.0 and removed v7.0.0 labels Feb 6, 2019

danielmitterdorfer added v7.2.0 and removed v6.7.0 labels Feb 7, 2019

Tim-Brooks added 2 commits February 13, 2019 14:37

Merge remote-tracking branch 'upstream/master' into fix_remote_connec…

1afb3a4

…tion_updates

Changes

30688d4

Tim-Brooks requested a review from lcawl February 13, 2019 22:41

Fix settings

b31d2af

Tim-Brooks added v6.7.0 v7.0.0-beta1 labels Feb 13, 2019

Tim-Brooks requested a review from jasontedor February 14, 2019 17:57

[DOCS] Edits remote cluster configuration

8085001

lcawl approved these changes Feb 16, 2019

View reviewed changes

lcawl mentioned this pull request Feb 16, 2019

[DOCS] Edits the remote clusters documentation #38996

Merged

s1monw approved these changes Feb 19, 2019

View reviewed changes

Tim-Brooks merged commit a5cbef9 into elastic:master Feb 19, 2019

Tim-Brooks added the backport pending label Feb 19, 2019

Tim-Brooks removed the backport pending label Feb 21, 2019

Tim-Brooks deleted the fix_remote_connection_updates branch April 30, 2020 18:24

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rebuild remote connections on profile changes #37678

Rebuild remote connections on profile changes #37678

Tim-Brooks commented Jan 22, 2019

elasticmachine commented Jan 22, 2019

Tim-Brooks commented Jan 22, 2019

jasontedor Jan 22, 2019

Tim-Brooks Jan 22, 2019

s1monw left a comment

s1monw Jan 23, 2019

Tim-Brooks Jan 24, 2019

Tim-Brooks commented Jan 31, 2019

Tim-Brooks commented Feb 13, 2019

Tim-Brooks commented Feb 13, 2019

ywelsch commented Feb 15, 2019

lcawl left a comment

s1monw commented Feb 19, 2019

Rebuild remote connections on profile changes #37678

Rebuild remote connections on profile changes #37678

Conversation

Tim-Brooks commented Jan 22, 2019

elasticmachine commented Jan 22, 2019

Tim-Brooks commented Jan 22, 2019

jasontedor Jan 22, 2019

Choose a reason for hiding this comment

Tim-Brooks Jan 22, 2019

Choose a reason for hiding this comment

s1monw left a comment

Choose a reason for hiding this comment

s1monw Jan 23, 2019

Choose a reason for hiding this comment

Tim-Brooks Jan 24, 2019

Choose a reason for hiding this comment

Tim-Brooks commented Jan 31, 2019

Tim-Brooks commented Feb 13, 2019

Tim-Brooks commented Feb 13, 2019

ywelsch commented Feb 15, 2019

lcawl left a comment

Choose a reason for hiding this comment

s1monw commented Feb 19, 2019