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

Coordinator nodes cannot be defined using node.roles #3718

Closed
charith-elastic opened this issue Sep 7, 2020 · 12 comments · Fixed by #5044
Closed

Coordinator nodes cannot be defined using node.roles #3718

charith-elastic opened this issue Sep 7, 2020 · 12 comments · Fixed by #5044
Assignees
Labels
>bug Something isn't working

Comments

@charith-elastic
Copy link
Contributor

This was mentioned in #3324 but is worth raising as a separate issue.

ucfg seems to treat empty arrays as null values so the suggested method of defining a coordinator node cannot be done on ECK by setting node.roles: [ ].

ElasticsearchParseException[null-valued setting found for key [node.roles] found at line number [16], column number [10]]
@charith-elastic charith-elastic added >bug Something isn't working v1.3.0 labels Sep 7, 2020
@charith-elastic
Copy link
Contributor Author

Workaround is to use YAML type tags:

node.roles: !!seq ""

This is probably not going to be obvious to the users and slightly deviates from the Elasticsearch documentation (even though they are semantically equivalent.)

Other options that I could think of are:

  • Make config a strongly-typed struct to help ucfg figure out how to deserialize each field.
    • Con: we'll have to keep on top of any changes to the Elasticsearch configuration schema.
  • Ask the Elasticsearch team to consider adding coordinator as a valid role name.
    • Con: as the empty array is already an accepted value in a released version, they will have to keep supporting it as well but ECK won't be able to.
  • Ditch ucfg and use standard YAML parsing libraries
    • Con: Handling dot separated setting keys is going to be tricky.
  • Ask ucfg to support external schemas/type hints for parsing certain paths.

@charith-elastic
Copy link
Contributor Author

Added a note to docs/orchestrating-elastic-stack-applications/elasticsearch/node-configuration.asciidoc in #3719 about the YAML type tag workaround. If a different solution is found, that note should be removed.

@david-kow david-kow removed the v1.3.0 label Oct 7, 2020
@pebrc
Copy link
Collaborator

pebrc commented Dec 13, 2020

I don't think the workaround works as expected. On 7.10.0 with node.roles: !!seq "" all node roles seem to be configured. The rendered configuration on the node has node.roles: ""

10.73.50.49 41 65  4 1.21 0.41 0.27 cdhilmrstw - elasticsearch-es-coordinating-0

@pebrc
Copy link
Collaborator

pebrc commented Dec 14, 2020

The only workaround <8.0 that I could find is to use the legacy node role format as in:

  - config:
      node.master: false
      node.data: false
      node.ingest: false
      node.ml: false
      node.remote_cluster_client: false

which results in

10.131.0.126 11 63  6 0.38 0.63 0.53 -          - elasticsearch-es-coordinating-0

pebrc added a commit to pebrc/cloud-on-k8s that referenced this issue Dec 14, 2020
Stopping short of replacing our use of ucfg alltogether this tries to provide a short term fix by introducing a simple (hacky) replacement mechanism into the configuration rendering process to handle the cases where we know that null is not equivalent to [] (empty seq) in the rendered Yaml configuration.

If we think this is viable as short term fix for elastic#3718 I would add a few more tests and see to get this merged for 1.4.0
@pebrc
Copy link
Collaborator

pebrc commented Dec 14, 2020

The underlying problem is that once we create ucfg Config object out of the the YAML we lose the type information that node.roles was pointing to a empty slice of strings and instead we end up with a nil valued interface{}. Standard go.yaml renders the former as [] and the latter as null.

@pebrc pebrc self-assigned this Dec 16, 2020
@pebrc pebrc added the v1.4.0 label Dec 16, 2020
@pebrc
Copy link
Collaborator

pebrc commented Dec 21, 2020

#4041 implements a workaround. I am leaving this issue open for a proper solution.

@pebrc pebrc removed the v1.4.0 label Dec 21, 2020
@bvierra
Copy link

bvierra commented Apr 12, 2021

Using eck-operator 1.5 with ES 7.12.0 with the following:

node.roles: !!seq ""

is accepted via the operator, however all of the roles were enabled on the node.

Using:

- config:
      node.master: false
      node.data: false
      node.ingest: false
      node.ml: false
      node.remote_cluster_client: false

worked as expected

@sebgl
Copy link
Contributor

sebgl commented Apr 20, 2021

In case it was not clear: starting ECK 1.4.0 this bug is solved. You can specify node.roles: [] to indicate you want a coordinating-only node. It should work fine.

This issue is left open because we are not satisfied with how we implemented that fix :)

@jen-huang
Copy link

Hi, we had a report of the same issue coming up for a Kibana YAML setting: #4853

I will leave it to this team to consolidate these two issues as needed :)

@thbkrkr
Copy link
Contributor

thbkrkr commented Nov 10, 2021

I will check that we can revert #4041 with elastic/go-ucfg#188.

@pebrc
Copy link
Collaborator

pebrc commented Nov 10, 2021

I will check that we can revert #4041 with elastic/go-ucfg#188.

Yes 💯But we should keep a unit test around that verifies that the configuration with empty array values works.

@piyushkp
Copy link

piyushkp commented Aug 30, 2022

i'm upgrading from ES 8.2.0 to 8.4.0 and coordinator nodes failing with: (it worked fine in 8.2.0)

Exception in thread "main" org.elasticsearch.ElasticsearchParseException: null-valued setting found for key [node.roles] found at line number [9], column number [12] at org.elasticsearch.common.settings.Settings.validateValue(Settings.java:768) at org.elasticsearch.common.settings.Settings.fromXContent(Settings.java:743) at org.elasticsearch.common.settings.Settings.fromXContent(Settings.java:687) at org.elasticsearch.common.settings.Settings$Builder.loadFromStream(Settings.java:1190) at org.elasticsearch.node.InternalSettingsPreparer.loadOverrides(InternalSettingsPreparer.java:143) at org.elasticsearch.node.InternalSettingsPreparer.prepareEnvironment(InternalSettingsPreparer.java:53) at org.elasticsearch.common.cli.EnvironmentAwareCommand.createEnv(EnvironmentAwareCommand.java:110) at org.elasticsearch.common.cli.EnvironmentAwareCommand.execute(EnvironmentAwareCommand.java:54) at org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:85) at org.elasticsearch.cli.MultiCommand.execute(MultiCommand.java:94) at org.elasticsearch.cli.Command.mainWithoutErrorHandling(Command.java:85) at org.elasticsearch.cli.Command.main(Command.java:50) at org.elasticsearch.launcher.CliToolLauncher.main(CliToolLauncher.java:64)

```
    roles: []
    # For client nodes, we also need to add an empty node.roles in elasticsearch.yml
    # This is due to https://github.com/elastic/helm-charts/pull/1186#discussion_r631225687
    esConfig:
      elasticsearch.yml: |
        node.roles: []
```

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug Something isn't working
Projects
None yet
8 participants