Test sysctl vm.max_map_count before failing init script

[biggreenogre]

• Module version: 5.4.3
• Puppet version: ANY
• OS and version: Ubuntu 16.04 Xenial

Bug description

elasticsearch daemon always fails to start when running inside a container.

The elasticsearch daemon requires that the value of /proc/sys/vm/max_map_count be equal to or greater than MAX_MAP_COUNT. The daemon init script attempts to set this value with sysctl. Unfortunately, since this value cannot be set from inside a container, the init script always fails, even when the current value is perfectly good. Issue was encountered in LXC containers, it also applies to LXD, Docker and other container environments.

Given that the smarter DevOps practitioner will set this value on the host PRIOR to starting the container, would it be reasonable to TEST the value before refusing to start the daemon?

I proposes a minor change to /files/etc/init.d/elasticsearch.Debian.erb (and other OS script templates as applicable) as follows:

--- a/files/etc/init.d/elasticsearch.Debian.erb
+++ b/files/etc/init.d/elasticsearch.Debian.erb
@@ -144,7 +144,9 @@ case "$1" in
        fi

        if [ -n "$MAX_MAP_COUNT" -a -f /proc/sys/vm/max_map_count ]; then
-               sysctl -q -w vm.max_map_count=$MAX_MAP_COUNT
+               if [ `sysctl -n vm.max_map_count` -lt $MAX_MAP_COUNT ];then
+                       sysctl -q -w vm.max_map_count=$MAX_MAP_COUNT
+               fi
        fi

        # Start Daemon

This adds a test of the current value of vm.max_map_count against MAX_MAP_COUNT. If the current value equals or exceeds the required value, no action is taken and the daemon will start. Only if the current value is insufficient, will the script the attempt to make the change and fail.

This potentially fixes an irritating issue, suffered by many, in a more elegant fashion than other methods proposed in the past. My only concern is for the portability of the test construct, in case it is too "BASH-centric".

I will submit a pull request including patches for all the supported OS init scripts if this would be acceptable.

regards,

Drew

Some references: https://github.com/elastic/elasticsearch/pull/21899 https://github.com/elastic/puppet-elasticsearch/issues/806 https://github.com/elastic/puppet-elasticsearch/issues/745

[tylerjl]

The relationship between this module and the upstream init scripts has been to track upstream changes to keep the committed rpm/deb/system service scripts as similar as possible to what the package installs by default. In the future, the plan is to rely on mechanisms like templated systemd units in order to alleviate the need to keep syncing up upstream changes to the init scripts entirely and just rely on what the packages ship, so some changes like this eventually just won't end up working.

Could you possibly go into further detail about what your deployment scenario looks like? Typically this module is used for people who are provisioning ES on normal hosts, and the Docker images are used when running in a containerized environment (and those images account for the specifics of running in containers like this.)

[biggreenogre]

Our application is deployed as a group of containers that exist within an "appliance". The "appliance" could be a physical or virtual machine, local or remote, VmWare, KVM or any cloud vendor and is managed by Puppet from inception to demise. The application containers are long-lived and created by the LXC tools from within a Puppet module, there is no "image" as in Docker.

Whilst troubleshooting the original issue (ES won't run), I found a number of different solutions offered, some involved increasing privilege levels on the container.

nut{ 'vm.max_map_count':
  ensure   => cracked,
  provider => sledgehammer,
}

As a sysadmin, it "feels wrong" to change a kernel parameter, potentially affecting other applications on the host, without first checking if the change is required.

The upstream scripts are already being converted to templates, is the addition of two lines of code to the template too much more?.

The patch would be best implemented upstream but it does add value to the module in the meantime.

[tylerjl]

Thank you for the detailed explanation @biggreenogre, the reasoning here is pretty clear now. I suspect that the related issue that you linked to here in the elasticsearch repo pertaining to skipping sysctl in containers doesn't have a counterpart for init.d scripts is just because the systemd services get more attention - the upstream init.d scripts probably need the same treatment for these sysctl parameters that the systemd services have gotten.

Making the necessary changes in a PR for the rpm SysV init script and the deb SysV init script therefore sounds reasonable to me, so I think if we get those changes in upstream, we can reflect the change here as well. I'd still like to vet the changes upstream to ensure we aren't missing anything that might cause issues on the Elasticsearch side.

If you'd like to submit the upstream PR to explain the reasoning (which you've done excellently here) then feel free, otherwise I can do so. 👍

[biggreenogre]

Thanks tylerjl, I have submitted an upstream bug report at https://github.com/elastic/elasticsearch/issues/27236

I will be out of town next week with limited access to email, apologies in advance if I'm slow to respond. :-)

[elfranne]

Issue has been fixed in https://github.com/elastic/elasticsearch/pull/31285 and https://github.com/elastic/elasticsearch/pull/31512 Please close