Remove multi instance support #1085

fatmcgav · 2020-04-20T13:01:58Z

As indicated in #1068, as precursor to adding support for Elasticsearch 7.x is removing the multi-instance support from this module.

Unfortunately there was no clean way of splitting the changes into smaller PR's, and the history could probably do with some cleanup...

TODO:

Some more test cleanup
Update documentation
Remove any additional unused code.

For some reason, using the shared `:with_cleanup` hook doesn't always stop the service cleanly, causing subsequent tests to fail.

tylerjl

This is looking good! I added a few comments here and there, overall it looks right though. I understand that my comment about jvm.options may just be premature since this is still a WIP, but I wanted to ensure I mentioned it since removing those module-defined files in favor of what the package ships is one motivating factor for taking out instances (jvm.options is one, the init/systemd scripts are another, etc.).

There are some other parts of the module that are very old and crufty that probably don't need the baggage with a modern release (like any sort of support for Shield), but whether that's easier to cut out as part of this or as a separate PR is totally whichever is easiest from your perspective.

tylerjl · 2020-04-20T21:56:22Z

manifests/init.pp

@@ -85,6 +92,13 @@
 # @param defaults_location
 #   Absolute path to directory containing init defaults file.
 #
+# @param deprecation_logging
+#   Wheter to enable deprecation logging. If enabled, deprecation logs will be


Suggested change

# Wheter to enable deprecation logging. If enabled, deprecation logs will be

# Whether to enable deprecation logging. If enabled, deprecation logs will be

manifests/config.pp

manifests/init.pp

ghost

This is a massive effort and I'm so pleased it's in such good hands. As you can see, my comments are largely about trivial things.

My only serious concern is that I can't currently run the acceptance tests, but that looks like it's probably just a result of bit rot and not caused by this patch.

$ bundle exec rake beaker
[...]
Failure/Error: run_puppet_install_helper('agent') unless ENV['BEAKER_provision'] == 'no'
Beaker::Host::CommandFailure:
  Host 'ubuntu-16-04' exited with 8 running:
   wget -O /tmp/puppet.deb http://apt.puppet.com/pc1-release-xenial.deb
  Last 10 lines of output were:
        --2020-04-22 06:09:22--  http://apt.puppet.com/pc1-release-xenial.deb
        Resolving apt.puppet.com (apt.puppet.com)... 99.86.212.17, 99.86.212.23, 99.86.212.32, ...
        Connecting to apt.puppet.com (apt.puppet.com)|99.86.212.17|:80... connected.
        HTTP request sent, awaiting response... 404 Not Found
        2020-04-22 06:09:22 ERROR 404: Not Found.

ghost · 2020-04-22T04:58:54Z

manifests/config.pp

+      $elasticsearch::homedir:
+        ensure => 'directory',
+        group  => $elasticsearch::elasticsearch_group,
+        owner  => $elasticsearch::elasticsearch_user;


Not today, but I'd love to see this multi-resource declaration become an iterator one day.

Alternatively, a default: section could DRY this up a lot.

https://puppet.com/docs/puppet/latest/style_guide.html#multiple-resources

ghost · 2020-04-22T05:03:11Z

manifests/config.pp

+      }
+    }
+  } else { # absent
+    file { "${elasticsearch::defaults_location}/elasticsearch":


There's a bunch of variable references that have explicit paths from the top-level scope with $::, but some don't.

It's been a while, but I had a feeling that modern Puppet versions don't need leading colons. Is that right? Either way, it's probably worth being consistent.

Good question... I'll find out what the current best practise is :)

manifests/init.pp

ghost · 2020-04-22T05:11:59Z

manifests/init.pp

-
-  # Manage users/roles before instances (req'd to keep dir in sync)
-  Elasticsearch::Role <| |>
-  -> Elasticsearch::Instance <| |>
-  Elasticsearch::User <| |>
-  -> Elasticsearch::Instance <| |>
-
-  # Ensure instances are started before managing REST resources
-  Elasticsearch::Instance <| ensure == 'present' |>
-  -> Elasticsearch::Template <| |>
-  Elasticsearch::Instance <| ensure == 'present' |>
-  -> Elasticsearch::Pipeline <| |>
-  Elasticsearch::Instance <| ensure == 'present' |>
-  -> Elasticsearch::Index <| |>
-  Elasticsearch::Instance <| ensure == 'present' |>
-  -> Elasticsearch::Snapshot_repository <| |>
-  # Ensure instances are stopped after managing REST resources
-  Elasticsearch::Template <| |>
-  -> Elasticsearch::Instance <| ensure == 'absent' |>
-  Elasticsearch::Pipeline <| |>
-  -> Elasticsearch::Instance <| ensure == 'absent' |>
-  Elasticsearch::Index <| |>
-  -> Elasticsearch::Instance <| ensure == 'absent' |>
-  Elasticsearch::Snapshot_repository <| |>
-  -> Elasticsearch::Instance <| ensure == 'absent' |>
-
-  # Ensure scripts are installed before copying them to configuration directory
-  Elasticsearch::Script <| |>
-  -> File["${configdir}/scripts"]


ghost · 2020-04-22T05:14:50Z

spec/classes/001_hiera_spec.rb

+          %w[elasticsearch.yml jvm.options log4j2.properties].each do |file|
+            it { should contain_file("/etc/elasticsearch/#{file}") }


manifests/config.pp

spec/acceptance/tests/acceptance_spec.rb

Remove the `elasticsearch_service_file` type/provider and associated tests.

This removes the templated 'jvm.options' file, instead we append to the shipped 'jvm.options' file by using the `file_line` resource. Removed associated tests and template file. Add additional tests to cover specifying 'jvm_options'.

Also update unit tests to reflect changes.

This can be used to override the default sleep interval, for example on a slower system. Also updated default value from `2` seconds to `10` seconds. Finally, added a unit test for the type.

fatmcgav · 2020-05-06T08:37:21Z

Right, this PR is big enough now and I think it covers most of the important stuff, so can you take another look @tylerjl and @Jarpy?

ghost

Sorry to keep you waiting. Please ship this magnificent monster!

marvi · 2020-05-13T13:03:16Z

I'm trying out your branch on CentOS 7 and get an error when running puppet apply:

  class { 'elasticsearch':
    manage_repo => false,
    version     => '7.6.2',
  }

systemd[1]: elasticsearch.service: main process exited, code=exited, status=233/RUNTIME_DIRECTORY

If I manually run systemctl start elasticsearch it works.

In the unit file RuntimeDirectory is specified. The directory is also created in config.pp and a tmpfiles.d config is added. I think there is a conflict among these three.

fatmcgav · 2020-06-09T14:42:59Z

@marvi Thank you for taking the time to test this PR and report your findings
I've not been able to replicate the behaviour using our Docker tests.

However I'll run some more in-depth tests using Vagrant images once I've merged this PR.

marvi · 2020-06-09T19:45:01Z

I solved it for CentOS 7 by setting elasticsearch::pid_dir: ~.

Gavin Williams added 28 commits March 27, 2020 16:34

Initial round of unpicking

c4def54

Rubocop fixes

ca527b7

Add missing param doc string

0123957

First set of spec test fixes

4636106

Rubocop fix

d240072

Lots more test fixes

f0fefe5

Fix whitespace rubocop failure

209329a

Convert defined types to classes

aff0628

Add Amazon Linux 2 to travis matrix

2af26b9

More service tweaks

f32b789

Lint fixes

5377044

Fix pid_file for Ubuntu 14.04

37ee30d

Tweak removal acceptance tests

9b7ee82

Expand acceptance tests

fe9f946

Test elasticsearch plugins

741189d

Test snapshot repository resources

f2695d9

Test datadir param

3e7644f

Run the security acceptance tests

c71478b

Fix rubocop issues

34553f5

Add 'package_url' tests

1f35d1f

Fix rubocop warning

82d9083

Remove 'instance.pp.ignore' file

d039afa

Tidy up 'superuser_role' var

1ccbeb2

Run hieradata tests

0222bd6

Fix rubocop emptylines warning

567009d

Fix missing 'superuser_role'

76c3f0e

Trigger a new CI run

5b0ee54

Remove elasticsearch within the hiera shared examples.

959a09d

For some reason, using the shared `:with_cleanup` hook doesn't always stop the service cleanly, causing subsequent tests to fail.

fatmcgav added feature es-7.x labels Apr 20, 2020

fatmcgav requested a review from tylerjl April 20, 2020 13:01

fatmcgav self-assigned this Apr 20, 2020

tylerjl reviewed Apr 21, 2020

View reviewed changes

ghost reviewed Apr 22, 2020

View reviewed changes

Gavin Williams added 3 commits April 22, 2020 16:35

Remove references to shield plugin.

4aa4d95

Remove the `elasticsearch_service_file` type/provider and associated tests.

Fix rubocop warnings

d28bc38

Fix role and user manifests

c5072d8

fatmcgav force-pushed the remove_multi-instance branch from eca2cf3 to 05f557f Compare April 24, 2020 10:30

fatmcgav force-pushed the remove_multi-instance branch from 05f557f to bec95ae Compare April 24, 2020 10:45

Gavin Williams added 3 commits April 28, 2020 12:18

Missed Shield removal

4e5b19a

One final reference to 'security_plugin'

3bd4642

Fix failing 'security_logging' tests

67a0720

fatmcgav force-pushed the remove_multi-instance branch 2 times, most recently from a5487bb to 6dca520 Compare April 29, 2020 21:52

Fix private variable usage for '_plugindir'

d2d83e5

fatmcgav force-pushed the remove_multi-instance branch from 6dca520 to d2d83e5 Compare April 29, 2020 21:53

Gavin Williams added 5 commits April 30, 2020 14:38

Remove duplicate providers for x-pack/oss-xpack.

fbacf44

Also update unit tests to reflect changes.

Add a sleep_interval param to the es_instance_conn_validator type.

8a7208b

This can be used to override the default sleep interval, for example on a slower system. Also updated default value from `2` seconds to `10` seconds. Finally, added a unit test for the type.

Add some ordering to the elasticsearch::user defined type.

196a0a6

Fix rubocop warnings on new unit test file

a5ac737

Missed a file save :(

030bb2e

ghost approved these changes May 11, 2020

View reviewed changes

fatmcgav merged commit 82b5380 into master Jun 9, 2020

fatmcgav deleted the remove_multi-instance branch June 9, 2020 14:43

olifre mentioned this pull request Nov 17, 2020

Beginner example from documentation fails with "Unknown resource type: 'elasticsearch::instance'" #1101

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove multi instance support #1085

Remove multi instance support #1085

fatmcgav commented Apr 20, 2020

tylerjl left a comment

tylerjl Apr 20, 2020

ghost left a comment

ghost Apr 22, 2020

fatmcgav May 6, 2020

ghost Apr 22, 2020

fatmcgav Apr 27, 2020

ghost Apr 22, 2020

ghost Apr 22, 2020

fatmcgav commented May 6, 2020

ghost left a comment

marvi commented May 13, 2020

fatmcgav commented Jun 9, 2020

marvi commented Jun 9, 2020

	# Wheter to enable deprecation logging. If enabled, deprecation logs will be
	# Whether to enable deprecation logging. If enabled, deprecation logs will be

		%w[elasticsearch.yml jvm.options log4j2.properties].each do \|file\|
		it { should contain_file("/etc/elasticsearch/#{file}") }

Remove multi instance support #1085

Remove multi instance support #1085

Conversation

fatmcgav commented Apr 20, 2020

tylerjl left a comment

Choose a reason for hiding this comment

tylerjl Apr 20, 2020

Choose a reason for hiding this comment

ghost left a comment

Choose a reason for hiding this comment

ghost Apr 22, 2020

Choose a reason for hiding this comment

fatmcgav May 6, 2020

Choose a reason for hiding this comment

ghost Apr 22, 2020

Choose a reason for hiding this comment

fatmcgav Apr 27, 2020

Choose a reason for hiding this comment

ghost Apr 22, 2020

Choose a reason for hiding this comment

ghost Apr 22, 2020

Choose a reason for hiding this comment

fatmcgav commented May 6, 2020

ghost left a comment

Choose a reason for hiding this comment

marvi commented May 13, 2020

fatmcgav commented Jun 9, 2020

marvi commented Jun 9, 2020