Cleanup installation and make shared file system prefixes optional #301

bndabbs · 2017-05-09T22:02:38Z

This is a lot of small changes and I wanted to submit them as separate pull requests for easier review, but a lot of them ended up being dependent on each other. I believe ee73f18 can be pulled out and moved to a separate PR if necessary, but I left it in there because I couldn't get my playbook to run without it.

The primary goal of this PR is to make the shared filesystem stuff optional, because I got tired of having to prefix the instance name to the service name when performing maintenance. I think the prefix is useful and necessary when operating in specific environments, but it adds unnecessary complexity for a lot of use cases.

elasticmachine · 2017-05-09T22:02:40Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

elasticmachine · 2017-05-09T22:02:40Z

Jenkins standing by to test this. If you aren't a maintainer, you can ignore this comment. Someone with commit access, please review this and clear it for Jenkins to run.

Slach · 2017-07-25T17:08:35Z

es_use_shared_fs: false
it's a really usefull feature
pls someone, merge this PR

greenhua · 2017-07-25T17:16:30Z

good! +1

gingerwizard · 2017-08-18T15:50:54Z

Other than the plugin handling (which i think has subsequently improved) this makes considerable improvements to the current role. I'd like to get this merged but i think we first need to fix some critical issues this doesn't resolve. @barryib any thoughts on this? The ability to have only one instance per machine, and thus not be impacted by the instance_name seems like a popular request.

AutomationD · 2017-09-28T15:29:07Z

Any chance to take care of this? Thanks!

@bndabbs Also, I'm wondering if we could ensure that since we don't have a concept of instance any more, should we not append es_instance_name to everything? I did it here #351

Let me know what you think!

bndabbs · 2017-10-07T05:17:13Z

This PR had gotten pretty far behind the current master, so I did a rebase this evening to bring everything up-to-date.

AutomationD · 2017-10-16T14:44:17Z

@elasticmachine test (?)
@gingerwizard any additional changes before this can be merged?

Thank you

AutomationD · 2017-10-16T15:01:44Z

@bndabbs actually, I just realized that es_instance_name is still appended to everything. Is there a reason why we still keep it in the single-node mode? if it is not specified ES breaks:
Starting Elasticsearch-...

Also, default es_use_shared_fs: falsewill probably break backwards-compatibility.

AutomationD · 2017-10-16T23:57:51Z

I submitted changes back to your fork @bndabbs bndabbs#1

ajrpayne · 2017-11-14T19:07:11Z

I submitted changes that I believe clean up the params section a bit. @bndabbs

karmi · 2017-11-14T21:33:17Z

Hi @bndabbs, we have found your signature in our records, but it seems like you have signed with a different e-mail than the one used in yout Git commit. Can you please add both of these e-mails into your Github profile (they can be hidden), so we can match your e-mails to your Github profile?

bndabbs · 2017-11-14T22:04:18Z

@karmi I believe the emails are sorted out now.

janhoy · 2017-11-15T13:12:09Z

tasks/elasticsearch-scripts.yml

@@ -1,6 +1,6 @@
 ---

- set_fact: es_script_dir={{ es_conf_dir }}/{{es_instance_name}}
+- set_fact: es_script_dir={{ es_conf_dir }}{{ es_conf_dir }}{{'/' + es_instance_name if es_use_shared_fs else ''}}


The double es_conf_dir looks like a bug?

It does to me. I made a pull request with @bndabbs to remove it.

This has been merged now.

karmi · 2017-11-15T14:30:44Z

@bndabbs, yes, thanks, the check is now green!

ajrpayne · 2017-11-15T15:46:23Z

I think es_use_shared_fs: true should be the default to maintain backwards-compatibility.

bndabbs · 2017-11-15T16:27:49Z

@ajrpayne good call. I flipped the default on that.

janhoy · 2017-11-18T11:18:18Z

I’ve tested your PR with my install and it looks good. Is there perhaps need for README changes as well, before merging?

janhoy · 2017-11-27T21:01:05Z

@bndabbs, @karmi What's left on this now?

janhoy · 2017-11-27T21:48:59Z

@bndabbs We think we might have found a bug with this PR. If you have plugins defined, upgrading to a newer ES version using this role gives this error:

/usr/share/elasticsearch/bin/elasticsearch -Epath.conf=/etc/elasticsearch
[2017-11-27T14:52:22,712][WARN ][o.e.b.ElasticsearchUncaughtExceptionHandler] [control] uncaught exception in thread [main]
org.elasticsearch.bootstrap.StartupException: java.lang.IllegalArgumentException: plugin [analysis-phonetic] is incompatible with version [5.6.4]; was designed for version [5.6.3]
at org.elasticsearch.bootstrap.Elasticsearch.init(Elasticsearch.java:136)
    ...

I believe the problem is that the variable es_install.changed is used to determine whether to reinstall plugins but the var is never defined anywhere...?

janhoy · 2017-11-28T08:05:50Z

Well, I now understand that es_install.changed gets automatically set if the install task causes a change in state. But it does not seem to work, the var es_plugins_reinstall gets set to False in line 6 of elasticsearch-plugins.yml:

set_fact: es_plugins_reinstall={{ es_install.changed }}

I can workaround the bug by setting that line to True. It does not help to set the es_plugins_reinstall variable to True in my inventory.

janhoy · 2017-12-04T08:47:25Z

Hello @bndabbs ? :)

bndabbs · 2017-12-04T15:35:28Z

@janhoy I'll take a look at the plugin logic and see if I can figure that out.

janhoy · 2017-12-17T22:46:13Z

@bndabbs figured it out yet? I'd really like for this PR to be merged soon as we base our install on it.

janhoy · 2018-01-03T08:36:38Z

Hi, any luck with this? I'm considering reverting back to latest release since this PR seems to be going nowhere anytime soon?

ajrpayne · 2018-01-22T01:38:48Z

@janhoy
What OS are you using? Was 5.6.4 installed by some other method that would cause es_install to return false?

janhoy · 2018-01-22T08:57:06Z

I've rolled back to default version of the role since this PR is going nowhere. I was using CentOS, and installed with this role+PR from a clean system. It is easily reproducible by doing an install with plugins, then do an upgrade to a newer minor version.

This fixes elastic#384, fixes elastic#354

Cleaned up the facts. conf_dir is set in defaults and reset in elasticsearch-sharedfs.yml if es_use_shared_fs is true. instance_default_file was being reset at the bottom of the params file. instance_init_script was being reset at the bottom of the params file as well.

Crazybus

This is some really great work! Left a couple of small comments (one which is causing a test to fail). Really love what you have done to simplify a lot of the role!

Crazybus · 2018-02-05T10:38:10Z

templates/elasticsearch.yml.j2

@@ -8,7 +8,7 @@ cluster.name: elasticsearch
 {% endif %}

 {% if es_config['node.name'] is not defined %}
-node.name: {{inventory_hostname}}-{{es_instance_name}}
+node.name: {{inventory_hostname}}{{' - ' + es_instance_name if es_use_shared_fs else ''}}


Are the extra spaces around - intentional? This is causing the tests to fail because the node changed localhost-node1 to localhost - node1

Crazybus · 2018-02-05T10:48:46Z

templates/init/redhat/elasticsearch.j2

@@ -67,7 +67,7 @@ if [ ! -z "$CONF_FILE" ]; then
 fi

 exec="$ES_HOME/bin/elasticsearch"
-prog="{{es_instance_name}}_{{default_file | basename}}"
+prog="{{es_instance_name + '_' if es_use_shared_fs else ''}}{{default_file | basename}}"


Would it be possible to move all of '_' if es_use_shared_fs else '' logic into defaults/main.yml? It makes it a lot more readable and means that any future changes won't need to keep copy pasting this if else statement everywhere.

Rebase with patches

This was failing on my Red Hat 7 system without the full path

ajrpayne · 2018-02-14T09:19:25Z

I like the changes @bndabbs. Made a pull request that I thought brought it more in line with the sharedfs structure you were going for.

Crazybus · 2018-02-19T08:49:33Z

Sorry for the late reply just got back from holidays! 🌞

Almost there! Looks like only a couple small fixes are needed before this can be merged!

The Ubuntu 14.04 tests seems to be failing because of the service detection stuff. You can run this locally with kitchen converge standard-ubuntu-1404 and kitchen verify standard-ubuntu-1404 to run the tests.

17:36:32        TASK [elasticsearch : Include tasks specific to installed service manager] *****
17:36:32        fatal: [localhost]: FAILED! => {"reason": "Unable to retrieve file contents\nCould not find or access '/tmp/kitchen/elasticsearch-sshd.yml'"}

Apart from that only the multi suites are failing with elasticsearch failing to start up. You can easily run this locally with kitchen converge multi-ubuntu-1604 and kitchen verify multi-ubuntu-1604 to run the tests. The variables for this suite can be found in https://github.com/elastic/ansible-elasticsearch/blob/master/test/integration/multi.yml

18:04:18        TASK [elasticsearch : Wait for elasticsearch to startup] ***********************
18:09:18        fatal: [localhost]: FAILED! => {"changed": false, "elapsed": 300, "msg": "Timeout when waiting for localhost:9201"}

AutomationD · 2018-03-16T18:32:19Z

@bndabbs Any chance to make a last push to get it merged? Thanks!

rozling · 2018-08-02T07:10:24Z

Would love to see this merged. Is there anything I / others can do to help?

botelastic · 2019-09-18T07:10:53Z

This PR has been automatically marked as stale because it has not had
recent activity. It will be closed if no further activity occurs. Thank you
for your contributions.
To track this PR (even if closed), please open a corresponding issue if one does not already exist.

botelastic · 2019-10-18T08:11:09Z

This PR has been automatically closed because it has not had recent activity since being marked as stale.
Please reopen when work resumes.

Slach · 2019-10-18T08:15:56Z

so sad ;-(( this PR was really usefull i hope currently ansible-elasticsearch role implements some analogs

jmlrt · 2019-10-18T08:20:49Z

Hi @Slach,
What exact part of this PR did you need?
Don't hesitate to create a new PR based on current master branch if you think there is still a need for it.

Slach · 2019-10-18T08:32:52Z

i use this PR for install once elasticsearch instance to once host directly to /var/lib/elasticsearch/data instead of /var/lib/something-with-instance-name/

jmlrt · 2019-10-18T08:49:30Z

Are you still using this 2 years old branch PR for your Elasticsearch deployments?
I think what you need has already been implemented in #566. You can refer to multi-instance.md and #554 (comment) for more details.

I also encourage you to use 7.4.0 release Ansible Galaxy or master branch of the role now that we are trying to release our roles more often (please check the [CHANGELOG.md] (https://github.com/elastic/ansible-elasticsearch/blob/master/CHANGELOG.md) and test it on a dev environment before as they will be breaking changes).

Slach · 2019-10-18T11:03:32Z

@jmlrt oh, thanks a lot for explanation ;-)
no, currently i use clickhouse with ngram indexes and Grafana Loke instead of Elasticsearch
so i happy for this ticket close

This was referenced Aug 18, 2017

Start a node-per-server cluster, ability to notify on log4j and init change #351

Closed

Make "one node per server" the Ansible-Elasticsearch default #354

Closed

bndabbs force-pushed the master branch from d000fb4 to 9ec774c Compare October 7, 2017 04:07

janhoy reviewed Nov 15, 2017

View reviewed changes

Bradford Dabbs and others added 7 commits February 2, 2018 13:56

Cleanup outlying merge conflict

53437c1

Don't append hostname to es_conf_dir if es_use_shared_fs is false

ceb1d07

This fixes elastic#384, fixes elastic#354

Don't use _ or - when no es_use_shared_fs is not enabled

c6597dd

removed duplicated {{ es_conf_dir }}

2d3f5dc

added /scripts subfolder

78b6440

Flip es_use_shared_fs boolean for backward campat

ef74d41

bndabbs force-pushed the master branch from c057f29 to ef74d41 Compare February 2, 2018 20:02

ajrpayne added 3 commits February 3, 2018 17:39

es_install fixes

fcbb00c

removed extra whitespace

af88ea5

fixes for filebased scripts

e5c49d5

Crazybus suggested changes Feb 5, 2018

View reviewed changes

bndabbs added 4 commits February 5, 2018 08:50

Merge pull request #8 from ajrpayne/rebase-with-patches

00546e1

Rebase with patches

Simplify use of es_use_shared_fs checks

578d780

Provide full path to update-alternatives

6615305

This was failing on my Red Hat 7 system without the full path

Update default node name to include inventory_hostname

ada2e36

jmlrt added the enhancement label Sep 17, 2019

botelastic bot added the triage/stale label Sep 18, 2019

botelastic bot closed this Oct 18, 2019

Cleanup installation and make shared file system prefixes optional #301

Cleanup installation and make shared file system prefixes optional #301

Conversation

bndabbs commented May 9, 2017 • edited Loading

elasticmachine commented May 9, 2017

elasticmachine commented May 9, 2017

Slach commented Jul 25, 2017

greenhua commented Jul 25, 2017

gingerwizard commented Aug 18, 2017

AutomationD commented Sep 28, 2017 • edited Loading

bndabbs commented Oct 7, 2017

AutomationD commented Oct 16, 2017

AutomationD commented Oct 16, 2017 • edited Loading

AutomationD commented Oct 16, 2017

ajrpayne commented Nov 14, 2017

karmi commented Nov 14, 2017

bndabbs commented Nov 14, 2017

janhoy Nov 15, 2017

Choose a reason for hiding this comment

ajrpayne Nov 15, 2017

Choose a reason for hiding this comment

bndabbs Nov 15, 2017

Choose a reason for hiding this comment

karmi commented Nov 15, 2017

ajrpayne commented Nov 15, 2017

bndabbs commented Nov 15, 2017

janhoy commented Nov 18, 2017

janhoy commented Nov 27, 2017

janhoy commented Nov 27, 2017

janhoy commented Nov 28, 2017

janhoy commented Dec 4, 2017

bndabbs commented Dec 4, 2017

janhoy commented Dec 17, 2017

janhoy commented Jan 3, 2018

ajrpayne commented Jan 22, 2018

janhoy commented Jan 22, 2018

Crazybus left a comment

Choose a reason for hiding this comment

Crazybus Feb 5, 2018

Choose a reason for hiding this comment

Crazybus Feb 5, 2018

Choose a reason for hiding this comment

ajrpayne commented Feb 14, 2018

Crazybus commented Feb 19, 2018

AutomationD commented Mar 16, 2018

rozling commented Aug 2, 2018

botelastic bot commented Sep 18, 2019

botelastic bot commented Oct 18, 2019

Slach commented Oct 18, 2019

jmlrt commented Oct 18, 2019

Slach commented Oct 18, 2019

jmlrt commented Oct 18, 2019

Slach commented Oct 18, 2019

bndabbs commented May 9, 2017 •

edited

Loading

AutomationD commented Sep 28, 2017 •

edited

Loading

AutomationD commented Oct 16, 2017 •

edited

Loading