[CCR] Remote clusters settings sources #26067

jen-huang · 2018-11-22T05:50:56Z

@elastic/kibana-management
Please feel free to review, change, and merge this PR while I am away! This is not a set of changes that were anticipated from our initial roadmapping, so please read this novel of a PR summary in which I attempt to explain the reasoning 😄 I also ran out of time to adjust the tests, so I've disabled them for now.

Summary

This PR add support for transient settings (in addition to persistent settings) to the remote clusters UI. The UI will also attempt to differentiate configuration file settings (elasticsearch.yml).

This is needed because we use Remote Cluster Info API to retrieve the list of clusters & their state. This data is the aggregate of all cluster settings, which includes dynamic transient/persistent settings which are configured via Cluster Update Settings API, as well as static settings from elasticsearch.yml config file.

As a result, we have need to differentiate between clusters which were defined only through static config file settings (which cannot be removed/disconnected), from clusters that have dynamic definitions.

List view

A new Source column was added to the table:

Detail panel will list both transient and persistent settings if the cluster has them. The Status section is the aggregate settings from Remote Cluster Info API. This screenshot shows a cluster with both transient & persistent settings. There is no transient skip unavailable setting, so it falls back to the persistent setting:

Or config file settings, if there are no transient/persistent settings. Note that Disconnect is disabled for config file-only clusters in the detail panel, and their checkbox in the table is disabled too:

Add/Edit

The cluster form was updated to have two tabs to edit transient and persistent settings separately. When adding a cluster, we default to the Persistent tab:

When editing a cluster that has transient settings, we default to the Transient tab:

It is possible to "edit" a config file only cluster, but the effect is essentially adding transient/persistent settings under that cluster name, which will override the config file settings.

Delete (disconnect)

The copy on Disconnect was updated to explain that this will remove transient/persistent settings, but will not remove config file settings if the cluster has them. So it is possible to "disconnect" a cluster, but still have it be shown on the list as a config file cluster:

Known bugs

These are relatively minor UI bugs, but would be awesome if someone has the time to fix!

Disconnecting a cluster that has fallback config file settings - the cluster stays in the table correctly, but remains checked. Should be unchecked once the checkbox is disabled:

Seed node input stays in place when switching back and forth between Transient and Persistent tabs, until it is fully entered (pressing ENTER in combo box):

…, attempt to differentiate from config file settings

elasticmachine · 2018-11-22T08:05:05Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

sebelga

Loos good overall. Just a few questions we should address with Elasticsearch, a bit of code styling and maybe a small bug in the validation of settings.

sebelga · 2018-11-26T08:34:13Z

x-pack/plugins/remote_clusters/public/sections/remote_cluster_list/detail_panel/detail_panel.js

+  }
+
+  renderSettings(settings) {
+    const {


Code style: this could be deconstructed in the method argument.

sebelga · 2018-11-26T08:38:39Z

x-pack/plugins/remote_clusters/public/sections/remote_cluster_list/detail_panel/detail_panel.js

+            </EuiDescriptionListDescription>
+          </EuiFlexItem>
+          <EuiFlexItem>
+            <EuiDescriptionListTitle>


I think the code would be easier to read if the whole setting block would be rendered (not just the value). At first sight, renderSkipUnavailableValue() does not mean much, but something like renderSkipUnavailableSetting() containing both the title + description would be more meaningful.

sebelga · 2018-11-26T08:40:06Z

x-pack/plugins/remote_clusters/public/sections/remote_cluster_list/detail_panel/detail_panel.js

+      return (
+        <FormattedMessage
+          id="xpack.remoteClusters.detailPanel.skipUnavailableTrueValue"
+          defaultMessage="Yes"


I must say I am surprised we don't have global translation keys for "yes" and "no" 😊

One possible explanation is that context can change the meaning of an affirmative in other languages. For example, "hai" in Japanese is required for formal responses, but in other situations a more casual "ee" is appropriate (https://www.thoughtco.com/can-i-use-ee-instead-of-hai-4037928).

I see, thanks for the link @cjcenizal !

sebelga · 2018-11-26T08:48:57Z

x-pack/plugins/remote_clusters/public/sections/remote_cluster_list/detail_panel/detail_panel.js

@@ -115,6 +259,8 @@ export class DetailPanelUi extends Component {
      clusterName,
    } = this.props;

+    const isDisconnectDisabled = !cluster || !(cluster.isTransient || cluster.isPersistent);
+


We might also want to disable the button if the cluster is not connected, what do you think?

sebelga · 2018-11-26T08:56:47Z

x-pack/plugins/remote_clusters/server/lib/are_settings_valid.js

+  const hasPersistentSeeds = persistentSeeds && persistentSeeds.length;
+
+  // Check that at least one type of setting exists
+  if(!transientSettings && !persistentSettings) {


This will never be false as we provide a default value in the arguments. We might want to check if Object.keys length is == 0.

if (!Object.keys(transientSettings).length || !Object.keys(persistentSettings).length) { ... }

sebelga · 2018-11-26T09:02:18Z

x-pack/plugins/remote_clusters/server/lib/are_settings_valid.js

+    (!hasTransientSeeds && typeof transientSkipUnavailable === 'boolean')
+    || (!hasPersistentSeeds && typeof persistentSkipUnavailable === 'boolean')
+  ) {
+    return false;


Shouldn't be Elasticsearch that validate this and should not allow saving the skipUnavailable setting if there are no seeds?

sebelga · 2018-11-26T09:41:59Z

x-pack/plugins/remote_clusters/server/lib/cluster_serialization.js

-    seeds,
-    skip_unavailable: skipUnavailable,
-  };
+  if(transientSettings) {


It'd be nice to have a method to avoid the duplicated logic for transient and persistent settings

sebelga · 2018-11-26T09:47:50Z

x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_add_route.js

-        if(acknowledged && cluster) {
-          return deserializeCluster(name, cluster);
+        if(acknowledged) {
+          return {};


What is the reason we don't want to return the response from Elasticsearch?

sebelga · 2018-11-26T09:55:44Z

x-pack/plugins/remote_clusters/server/routes/api/remote_clusters/register_delete_route.js


-        if(acknowledged && !cluster) {
+        if(acknowledged && !transientCluster && !persistentCluster) {


Do we have an issue opened in Elasticsearch for this? It seems to me that if it acknowledges it should never return a transient or persistent cluster with the same name.

@cjcenizal This is what I mentioned yesterday? Do you think it Is this an ES bug?

It's difficult to say. I think we'd need to ask @jen-huang to explain the scenario in which acknowledged is true, but the settings still exist. It's also possible this is a non-issue given the way things are changing on the ES side, and given our current approach to hide transient settings from the user.

cjcenizal · 2018-11-27T00:08:46Z

We had a discussion with the ES team today and their general recommendation was to act under the assumption that all remote clusters are persistent (not transient or defined in the config file).

Ignoring transient settings

It sounded like the ES team is going to discourage/deprecate the use of transient settings for defining remote clusters. We can move ahead as if the user won't have any problems with transient settings overriding their persistent settings.

Ignoring the config file

The ES team is also going to discourage/deprecate the ability to specify remote clusters within the config file, since it's not clear how to resolve discrepancies between the settings files of different nodes within the same cluster.

This means we can consider the presence of a remote cluster defined via the config file as an anomaly. I think the simplest way to handle this anomaly would be to disable both editing and disconnection of this type of cluster and surface some information along the lines of, "This file is defined in one of your nodes' elasticsearch.yml files and its use has been deprecated." We could also add some functionality to make it easier to convert it into a persistent remote cluster.

What's left from this PR

After we've removed the functionality proscribed by these decisions, I think we may just be left with the ability to edit skip_unavailable and the logic for disabling editing config-based remote clusters.

bleskes · 2018-11-27T08:23:37Z

@cjcenizal, @martijnvg promised to research the interaction between the persistent settings and the node settings. I can see quite a bit of edge cases. I hope that we end up in a place where persistent settings always trumps local settings with the exceptions with deletes. Based on that we can decide what's wise to do.

@martijnvg can you make sure to look at two nodes clusters and the potential difference between settings on the master node and settings on the non-master node?

martijnvg · 2018-11-27T09:22:12Z

Persistent settings always overrule settings defined in elasticsearch.yml file on a node (node settings). Node settings are a kind of default in a setting has not been defined as persistent / transient setting in the cluster state. The problem with node settings is that it is a local default. Different nodes may have a different value for a setting or not specified a setting at all in the elasticsearch.yml file. This can happen because of a misconfiguration by the user. So for example in the case of the remote info api, it may return different connection info depending on which node answered the remote info api request.

The get cluster settings api only returns settings from the cluster state. So if a setting is only defined as a node setting then it is not visible by via the get cluster settings api. The node info api can be used to see whether the remote cluster settings have been defined as node settings. This api returns a settings response for each node in the cluster. So from that response it is possible to show UI warnings / errors if a remote cluster has been defined as a node setting.

If something is unclear or if there are more questions then feel free to ping me.

bleskes · 2018-11-27T14:24:45Z

@martijnvg thanks. A couple of concrete questions:

What happens when someone use the remote cluster info API? is it only cluster state based? does it include whatever is in the elasticsearch.yml of the node you happened to send the API request to?
If you do have a cluster defined in the elasticsearch.yml file and you update it via persistent settings, what happens? is it picked up by the remote cluster infra?

martijnvg · 2018-11-27T15:26:15Z

What happens when someone use the remote cluster info API? is it only cluster state based? does it include whatever is in the elasticsearch.yml of the node you happened to send the API request to?

It is based on both on what is the cluster state and what in the elasticsearch.yml of the node the request was sent to. The settings in cluster state overrule with what is defined in elasticsearch.yml

If you do have a cluster defined in the elasticsearch.yml file and you update it via persistent settings, what happens? is it picked up by the remote cluster infra?

Yes, updates via cluster update settings api are picked up by the remote cluster infra. Dynamic settings specified via cluster update api overwrite with what is defined in local elasticsearch.yml

cjcenizal · 2018-11-27T17:44:52Z

x-pack/plugins/remote_clusters/public/sections/remote_cluster_form/remote_cluster_form.js


-    // Show errors if there is a general form error or local errors.


I think this change broke the UX slightly. These conditions ensured that hitting "Save" when no seeds have been entered will show the validation message that seeds are required. With these changes, this validation doesn't appear.

cjcenizal · 2018-11-27T20:28:38Z

The get cluster settings api only returns settings from the cluster state. So if a setting is only defined as a node setting then it is not visible by via the get cluster settings api. The node info api can be used to see whether the remote cluster settings have been defined as node settings. This api returns a settings response for each node in the cluster. So from that response it is possible to show UI warnings / errors if a remote cluster has been defined as a node setting.

This sounds like what Jen implemented in this PR. We'll continue to use this tactic to warn the user when a remote cluster has been defined as a node setting and advise them to store it in cluster state instead.

cjcenizal · 2018-11-28T04:35:50Z

Many of the changes in this PR are still applicable but I ended up having to change enough of the UX to merit a separate PR. I'm going to close this in favor of #26318, but I made sure the commit history still includes you as an author @jen-huang so you get git cred. 😄

jen-huang added 3 commits November 21, 2018 20:57

Add support for transient settings in addition to persistent settings…

2e21d12

…, attempt to differentiate from config file settings

PR feedback fixes

dfbe820

Skip tests

d7f7fc3

jen-huang added review es-management labels Nov 22, 2018

jen-huang requested review from cjcenizal, bmcconaghy and sebelga November 22, 2018 05:50

sebelga requested changes Nov 26, 2018

View reviewed changes

cjcenizal reviewed Nov 27, 2018

View reviewed changes

cjcenizal closed this Nov 28, 2018

cjcenizal mentioned this pull request Nov 28, 2018

[CCR] Add skipUnavailable field to remote cluster and identify remote clusters which have been defined in elasticsearch.yml #26318

Merged

jen-huang deleted the feature/ccr-rc_setting_sources branch June 5, 2019 19:48

alisonelizabeth mentioned this pull request Mar 13, 2020

[Remote clusters] Add support for proxy mode #59221

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CCR] Remote clusters settings sources #26067

[CCR] Remote clusters settings sources #26067

jen-huang commented Nov 22, 2018 •

edited

Loading

elasticmachine commented Nov 22, 2018

sebelga left a comment

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

cjcenizal Nov 26, 2018

sebelga Nov 27, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 26, 2018

sebelga Nov 28, 2018

cjcenizal Nov 28, 2018 •

edited

Loading

sebelga Nov 29, 2018

cjcenizal commented Nov 27, 2018 •

edited

Loading

bleskes commented Nov 27, 2018

martijnvg commented Nov 27, 2018

bleskes commented Nov 27, 2018

martijnvg commented Nov 27, 2018

cjcenizal Nov 27, 2018

cjcenizal commented Nov 27, 2018 •

edited

Loading

cjcenizal commented Nov 28, 2018


		if(acknowledged && !cluster) {
		if(acknowledged && !transientCluster && !persistentCluster) {


		// Show errors if there is a general form error or local errors.

[CCR] Remote clusters settings sources #26067

[CCR] Remote clusters settings sources #26067

Conversation

jen-huang commented Nov 22, 2018 • edited Loading

Summary

List view

Add/Edit

Delete (disconnect)

Known bugs

elasticmachine commented Nov 22, 2018

💚 Build Succeeded

sebelga left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Nov 28, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Nov 27, 2018 • edited Loading

Ignoring transient settings

Ignoring the config file

What's left from this PR

bleskes commented Nov 27, 2018

martijnvg commented Nov 27, 2018

bleskes commented Nov 27, 2018

martijnvg commented Nov 27, 2018

Choose a reason for hiding this comment

cjcenizal commented Nov 27, 2018 • edited Loading

cjcenizal commented Nov 28, 2018

jen-huang commented Nov 22, 2018 •

edited

Loading

cjcenizal Nov 28, 2018 •

edited

Loading

cjcenizal commented Nov 27, 2018 •

edited

Loading

cjcenizal commented Nov 27, 2018 •

edited

Loading