Merge lp:~frankban/charms/trusty/redis/memory-management into lp:~juju-gui/charms/trusty/redis/trunk

[Francesco Banconi]

Implement memory management.

Add new maxmemory and maxmemory-policy charm
options. By setting them we should prevent
the whole system to become unresponsive when
running out of memory.

Also enable memory overcommit by default
(but it can be disabled) as suggested
at http://redis.io/topics/admin

Tests (including functional tests): `make check`.

QA:
- `juju bootstrap -e ec2`;
- `make deploy JUJU_ENV=ec2`;
- wait for the unit to be ready;
- play with redis, e.g.
  `juju ssh -e ec2 redis/0 telnet `juju run -e ec2 --unit redis/0 "unit-get private-address"` 6379`,
  then `set foo 42` in the telnet terminal, then `get foo` and then `quit` to
  exit;
- check that overcommit is enabled:
  `juju ssh -e ec2 redis/0 sysctl vm.overcommit_memory`;
- `juju set -e ec2 redis maxmemory=50m`;
- wait for the new configuration to apply;
- play with redis/telnet as usual to check it works:
  the telnet "config get maxmemory*" command should return
  sane values, including 50000000 and volatile-lru.
Done, thank you!

[Jay R. Wren (evarlast)]

LGTM. QA worked exactly as stated. I also tested in LXC local env. I was concerned with sysctl working there, but it works (or fails) perfectly.

review: Approve

[Richard Harding (rharding)]

LGTM with a few questions/comments.

review: Approve

[lp:~frankban/charms/trusty/redis/memory-management]

18. By Francesco Banconi on 2015-07-20

Improve memory overcommit option description.

[Francesco Banconi (frankban)]

Thanks for the reviews!

=== modified file 'config.yaml'
--- config.yaml	2015-06-19 09:47:04 +0000
+++ config.yaml	2015-07-20 11:52:30 +0000
@@ -44,6 +44,48 @@
       Set the number of databases. The default database is DB 0. You can select
       a different one on a per-connection basis using SELECT <dbid> where dbid
       is a number between 0 and 'databases'-1.
-
-
-
+  maxmemory:
+    type: string
+    default: ""
+    description: |
+      Don't use more than the specified amount of memory.
+      When the memory limit is reached Redis will try to remove keys according
+      to the eviction policy selected (see "maxmemory-policy" below).
+      If Redis can't remove keys according to the policy, or if the policy is
+      set to "noeviction", Redis will start to reply with errors to commands
+      that would use more memory, like SET, LPUSH, and so on, and will continue
+      to reply to read-only commands like GET.
+      This option is usually useful when using Redis as an LRU cache, or to set
+      a hard memory limit for an instance (using the "noeviction" policy).
+      It is possible to specify the memory size in the usual form of 1k, 5GB,
+      4M and so forth:
+      - 1k: 1000 bytes;
+      - 1kb: 1024 bytes;
+      - 1m: 1000000 bytes;
+      - 1mb: 1024*1024 bytes;
+      - 1g: 1000000000 bytes;
+      - 1gb: 1024*1024*1024 bytes.
+  maxmemory-policy:
+    type: string
+    default: volatile-lru
+    description: |
+      how Redis will select what to remove when maxmemory is reached.
+      You can select among five behaviors:
+      - volatile-lru: remove the key with an expire set using an LRU algorithm;
+      - allkeys-lru: remove any key according to the LRU algorithm;
+      - volatile-random: remove a random key with an expire set;
+      - allkeys-random: remove a random key, any key;
+      - volatile-ttl: remove the key with the nearest expire time (minor TTL);
+      - noeviction: don't expire at all, just return an error on write
+        operations.
+      Note: with any of the above policies, Redis will return an error on write
+      operations, when there are no suitable keys for eviction.
+  memory-overcommit:
+    type: boolean
+    default: true
+    description: |
+      Set the Linux kernel overcommit memory setting to 1 so that no memory
+      overcommit handling is performed. When this is enabled, the performance
+      for memory-intensive tasks is increased, but there is more risk of memory
+      overload. This option is ignored when the charm is deployed to a local
+      environment.
 
=== modified file 'hooks/services.py'
--- hooks/services.py	2015-05-25 13:34:16 +0000
+++ hooks/services.py	2015-07-20 11:52:30 +0000
@@ -26,8 +26,8 @@
 from charmhelpers.core.services import base
 
 import hookutils
+import relations
 import serviceutils
-import relations
 
 
 @hookutils.hook_name_logged
@@ -60,6 +60,7 @@
 
             # Callables called when required data is ready.
             'data_ready': [
+                serviceutils.write_sysctl(config),
                 serviceutils.write_config_file(
                     config,
                     db_relation=db_relation,
@@ -87,6 +88,7 @@
 
             # Callables called when required data is ready.
             'data_ready': [
+                serviceutils.write_sysctl(config),
                 serviceutils.write_config_file(
                     config,
                     db_relation=db_relation,
 
=== modified file 'hooks/serviceutils.py'
--- hooks/serviceutils.py	2015-05-26 09:41:30 +0000
+++ hooks/serviceutils.py	2015-07-20 11:52:30 +0000
@@ -16,6 +16,7 @@
 import configfile
 import hookutils
 import settings
+import system
 
 
 def service_start(port, previous_port, service_name):
@@ -51,6 +52,9 @@
     # Remove redis package and clean up files.
     hookutils.log('Removing system packages.')
     fetch.apt_purge(settings.PACKAGES)
+    # Restore the default sysctl values and remove the sysctl file.
+    hookutils.log('Cleaning up sysctl state.')
+    system.remove_sysctl()
 
 
 def write_config_file(
@@ -87,6 +91,20 @@
     return callback
 
 
+def write_sysctl(config):
+    """Handle sysctl configuration file creation.
+
+    Return a function that can be used as a callback in the services framework,
+    and that generates the redis syctl configuration file.
+    """
+    def callback(service_name):
+        hookutils.log('Setting up sysctl state.')
+        overcommit_memory = config.get('memory-overcommit', True)
+        system.write_sysctl(overcommit_memory=overcommit_memory)
+
+    return callback
+
+
 def _get_service_options(config, slave_relation=None):
     """Return a dict containing the redis service configuration options.
 
@@ -108,9 +126,18 @@
         'tcp-keepalive': config['tcp-keepalive'],
         'timeout': config['timeout'],
     }
+    # Add memory management options.
+    maxmemory = config.get('maxmemory', '').strip()
+    if maxmemory:
+        options.update({
+            'maxmemory': maxmemory,
+            'maxmemory-policy': config['maxmemory-policy'],
+        })
+    # Add authentication options.
     password = config['password'].strip()
     if password:
         options['requirepass'] = password
+    # Add master/slave relationship options.
     if slave_relation is not None:
         hookutils.log('Setting up slave relation.')
         # If slave_relation is defined, it is assumed that the relation is
 
=== modified file 'hooks/settings.py'
--- hooks/settings.py	2015-05-25 14:09:49 +0000
+++ hooks/settings.py	2015-07-20 11:52:30 +0000
@@ -8,6 +8,9 @@
 DEFAULT_REDIS_CONF = '/etc/redis/redis.conf'
 REDIS_CONF = '/etc/redis/redis-charm.conf'
 
+# Define the path to the redis sysctl configuration file.
+SYSCTL_CONF = '/etc/sysctl.d/60-redis.conf'
+
 # Define Debian packages to be installed.
 PACKAGES = ['redis-server']
 
 
=== added file 'hooks/system.py'
--- hooks/system.py	1970-01-01 00:00:00 +0000
+++ hooks/system.py	2015-07-20 11:52:30 +0000
@@ -0,0 +1,33 @@
+# Copyright 2015 Canonical Ltd.
+# Licensed under the GPLv3, see copyright file for details.
+
+"""Helpers for interacting with the operating system."""
+
+import os
+
+from charmhelpers.core import host
+
+import settings
+
+
+def write_sysctl(overcommit_memory=False):
+    """Set the sysctl "vm.overcommit_memory" based on the given boolean value.
+
+    The default value indicates the system default if values were not changed.
+    Also restart procps when done, so that new values take effect.
+
+    Note that this will not work when using the local provider, since /sys
+    is a read-only file system and sysctl options really belong to the host.
+    Anyway, this function still executes without errors.
+    """
+    value = 1 if overcommit_memory else 0
+    with open(settings.SYSCTL_CONF, 'w') as sysctl_file:
+        # See http://redis.io/topics/admin.
+        sysctl_file.write('vm.overcommit_memory = {}\n'.format(value))
+    host.service_restart('procps')
+
+
+def remove_sysctl():
+    """Remove the sysctl configuration file for redis."""
+    write_sysctl()
+    os.remove(settings.SYSCTL_CONF)
 
=== modified file 'tests/test_10_deploy.py'
--- tests/test_10_deploy.py	2015-05-25 15:59:59 +0000
+++ tests/test_10_deploy.py	2015-07-20 11:52:30 +0000
@@ -25,11 +25,13 @@
 import settings
 
 
-# Define a test decorator for running the test only if the current environment
-# type is local.
+# Define a test decorator for running/skipping tests based on whether the suite
+# is using a local Juju environment.
+_env_type = helpers.get_environment_type()
 only_on_local_environments = unittest.skipIf(
-    helpers.get_environment_type() != 'local',
-    'only available whe using a local environment')
+    _env_type != 'local', 'only available when using a local environment')
+not_on_local_environments = unittest.skipIf(
+    _env_type == 'local', 'not available when using a local environment')
 
 
 class TestDeployment(unittest.TestCase):
@@ -68,6 +70,12 @@
         client.set('my-key', 'my-value')
         self.assertEqual('my-value', client.get('my-key'))
 
+    @not_on_local_environments
+    def test_sysctl(self):
+        output, retcode = self.unit.run('sysctl vm.overcommit_memory')
+        self.assertEqual(0, retcode, output)
+        self.assertEqual('vm.overcommit_memory = 1', output)
+
 
 class TestDeploymentOptions(unittest.TestCase):
 
@@ -78,6 +86,9 @@
         'loglevel': 'verbose',
         'logfile': '/tmp/redis.log',
         'timeout': 60,
+        'maxmemory': '1g',
+        'maxmemory-policy': 'noeviction',
+        'memory-overcommit': False,
     }
 
     @classmethod
@@ -97,6 +108,8 @@
             'databases 3\n'
             'logfile /tmp/redis.log\n'
             'loglevel verbose\n'
+            'maxmemory 1g\n'
+            'maxmemory-policy noeviction\n'
             'port 4242\n'
             'requirepass secret\n'
             'tcp-keepalive 0\n'
@@ -116,6 +129,12 @@
         client.set('my-key', 'my-value')
         self.assertEqual('my-value', client.get('my-key'))
 
+    @not_on_local_environments
+    def test_sysctl(self):
+        output, retcode = self.unit.run('sysctl vm.overcommit_memory')
+        self.assertEqual(0, retcode, output)
+        self.assertEqual('vm.overcommit_memory = 0', output)
+
 
 class TestMasterSlaveRelation(unittest.TestCase):
 
@@ -178,7 +197,7 @@
 
 class TestMasterSlaveRelationOptions(unittest.TestCase):
 
-    master_options = {'databases': 5, 'password': 'secret'}
+    master_options = {'databases': 5, 'password': 'secret', 'maxmemory': '2g'}
     slave_options = {'port': 4747, 'loglevel': 'warning', 'timeout': 42}
 
     @classmethod
@@ -202,6 +221,8 @@
             'databases 5\n'
             'logfile /var/log/redis/redis-server.log\n'
             'loglevel notice\n'
+            'maxmemory 2g\n'
+            'maxmemory-policy volatile-lru\n'
             'port 6379\n'
             'requirepass secret\n'
             'tcp-keepalive 0\n'
 
=== modified file 'unit_tests/test_serviceutils.py'
--- unit_tests/test_serviceutils.py	2015-05-25 15:34:53 +0000
+++ unit_tests/test_serviceutils.py	2015-07-20 11:52:30 +0000
@@ -79,6 +79,7 @@
 @mock.patch('charmhelpers.fetch.apt_purge')
 @mock.patch('charmhelpers.core.host.service_stop')
 @mock.patch('charmhelpers.core.hookenv.close_port')
[e-mail]('system.remove_sysctl')
 @mock.patch('hookutils.log')
 class TestServiceStop(unittest.TestCase):
 
@@ -86,22 +87,23 @@
     port = 6379
 
     def test_service_running_stop_hook(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         with patch_service_running(True):
             with patch_hook_name('stop'):
                 serviceutils.service_stop(self.port, self.service_name)
         mock_service_stop.assert_called_once_with(settings.SERVICE_NAME)
-        self.assertEqual(2, mock_log.call_count)
+        self.assertEqual(3, mock_log.call_count)
         mock_log.assert_has_calls([
             mock.call('Stopping service {}.'.format(self.service_name)),
-            mock.call('Removing system packages.')
+            mock.call('Removing system packages.'),
+            mock.call('Cleaning up sysctl state.'),
         ])
         mock_close_port.assert_called_once_with(self.port)
 
     def test_service_not_running_stop_hook(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         with patch_service_running(False):
             with patch_hook_name('stop'):
                 serviceutils.service_stop(self.port, self.service_name)
@@ -109,8 +111,8 @@
         mock_close_port.assert_called_once_with(self.port)
 
     def test_customized_port(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         port = 4747
         with patch_service_running(False):
             with patch_hook_name('stop'):
@@ -118,18 +120,23 @@
         mock_close_port.assert_called_once_with(port)
 
     def test_cleaning_up(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         port = 4747
         with patch_service_running(False):
             with patch_hook_name('stop'):
                 serviceutils.service_stop(port, self.service_name)
-        mock_log.assert_called_once_with('Removing system packages.')
+        self.assertEqual(2, mock_log.call_count)
+        mock_log.assert_has_calls([
+            mock.call('Removing system packages.'),
+            mock.call('Cleaning up sysctl state.'),
+        ])
         mock_apt_purge.assert_called_once_with(settings.PACKAGES)
+        mock_remove_sysctl.assert_called_once_with()
 
     def test_service_running_other_hook(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         with patch_service_running(True):
             with patch_hook_name('config-changed'):
                 serviceutils.service_stop(self.port, self.service_name)
@@ -139,8 +146,8 @@
         self.assertFalse(mock_apt_purge.called)
 
     def test_service_not_running_other_hook(
-            self, mock_log, mock_close_port, mock_service_stop,
-            mock_apt_purge):
+            self, mock_log, mock_remove_sysctl, mock_close_port,
+            mock_service_stop, mock_apt_purge):
         with patch_service_running(False):
             with patch_hook_name('config-changed'):
                 serviceutils.service_stop(self.port, self.service_name)
@@ -216,9 +223,38 @@
         mocks.log.assert_has_calls([
             mock.call('Retrieving service options.'),
             mock.call('Writing configuration file for foo.'),
-            mock.call('Restarting service due to configuration change.')
+            mock.call('Restarting service due to configuration change.'),
         ])
 
+    def test_configuration_changed_maxmemory(self):
+        config = {
+            'databases': 3,
+            'logfile': '/path/to/logfile',
+            'loglevel': 'debug',
+            'maxmemory': '5gb',
+            'maxmemory-policy': 'volatile-lru',
+            'password': '',
+            'port': 4242,
+            'tcp-keepalive': 10,
+            'timeout': 42,
+        }
+        callback = serviceutils.write_config_file(config)
+        with self.patch_all(configuration_changed=True) as mocks:
+            callback('foo')
+        mocks.write.assert_called_once_with({
+            'bind': '1.2.3.4',
+            'databases': 3,
+            'maxmemory': '5gb',
+            'maxmemory-policy': 'volatile-lru',
+            'logfile': '/path/to/logfile',
+            'loglevel': 'debug',
+            'port': 4242,
+            'tcp-keepalive': 10,
+            'timeout': 42,
+        }, settings.REDIS_CONF)
+        mocks.unit_get.assert_called_once_with('private-address')
+        mocks.service_restart.assert_called_once_with(settings.SERVICE_NAME)
+
     def test_configuration_changed_password(self):
         config = {
             'databases': 3,
@@ -307,7 +343,7 @@
         mocks.log.assert_has_calls([
             mock.call('Retrieving service options.'),
             mock.call('Writing configuration file for foo.'),
-            mock.call('No changes detected in the configuration file.')
+            mock.call('No changes detected in the configuration file.'),
         ])
 
     def test_configuration_unchanged_slave(self):
@@ -345,7 +381,42 @@
             mock.call('Retrieving service options.'),
             mock.call('Setting up slave relation.'),
             mock.call('Writing configuration file for foo.'),
-            mock.call('No changes detected in the configuration file.')
+            mock.call('No changes detected in the configuration file.'),
+        ])
+
+    def test_configuration_unchanged_master_maxmemory(self):
+        config = {
+            'databases': 3,
+            'logfile': '/path/to/logfile',
+            'loglevel': 'debug',
+            'maxmemory': '42m',
+            'maxmemory-policy': 'noeviction',
+            'password': '',
+            'port': 4242,
+            'tcp-keepalive': 60,
+            'timeout': 10,
+        }
+        callback = serviceutils.write_config_file(config)
+        with self.patch_all() as mocks:
+            callback('foo')
+        mocks.write.assert_called_once_with({
+            'bind': '1.2.3.4',
+            'databases': 3,
+            'logfile': '/path/to/logfile',
+            'loglevel': 'debug',
+            'maxmemory': '42m',
+            'maxmemory-policy': 'noeviction',
+            'port': 4242,
+            'tcp-keepalive': 60,
+            'timeout': 10,
+        }, settings.REDIS_CONF)
+        mocks.unit_get.assert_called_once_with('private-address')
+        self.assertFalse(mocks.service_restart.called)
+        self.assertEqual(3, mocks.log.call_count)
+        mocks.log.assert_has_calls([
+            mock.call('Retrieving service options.'),
+            mock.call('Writing configuration file for foo.'),
+            mock.call('No changes detected in the configuration file.'),
         ])
 
     def test_configuration_unchanged_master_password(self):
@@ -406,3 +477,20 @@
         }, settings.REDIS_CONF)
         mocks.unit_get.assert_called_once_with('private-address')
         self.assertFalse(mocks.service_restart.called)
+
+
[e-mail]('system.write_sysctl')
[e-mail]('hookutils.log')
+class TestWriteSysctl(unittest.TestCase):
+
+    def test_memory_overcommit_enabled(self, mock_log, mock_write_sysctl):
+        callback = serviceutils.write_sysctl({'memory-overcommit': True})
+        callback('service')
+        mock_log.assert_called_once_with('Setting up sysctl state.')
+        mock_write_sysctl.assert_called_once_with(overcommit_memory=True)
+
+    def test_memory_overcommit_disabled(self, mock_log, mock_write_sysctl):
+        callback = serviceutils.write_sysctl({'memory-overcommit': False})
+        callback('service')
+        mock_log.assert_called_once_with('Setting up sysctl state.')
+        mock_write_sysctl.assert_called_once_with(overcommit_memory=False)
 
=== added file 'unit_tests/test_system.py'
--- unit_tests/test_system.py	1970-01-01 00:00:00 +0000
+++ unit_tests/test_system.py	2015-07-20 11:52:30 +0000
@@ -0,0 +1,59 @@
+# Copyright 2015 Canonical Ltd.
+# Licensed under the GPLv3, see copyright file for details.
+
+import contextlib
+from pkg_resources import resource_filename
+import sys
+import tempfile
+import unittest
+
+import mock
+
+# Allow importing modules and packages from the hooks directory.
+sys.path.append(resource_filename(__name__, '../hooks'))
+
+import system
+
+
[e-mail]
+def patch_sysctl_conf():
+    """Replace the sysctl configuration path.
+
+    Return a temporary file that can be used for tests when entering the
+    context block. The file is automatically closed and deleted when exiting
+    the context block.
+    """
+    conf = tempfile.NamedTemporaryFile()
+    with mock.patch('charmhelpers.core.host.service_restart') as mock_restart:
+        with mock.patch('settings.SYSCTL_CONF', conf.name):
+            try:
+                yield conf
+            finally:
+                conf.close()
+    mock_restart.assert_called_once_with('procps')
+
+
+class TestWriteSysctl(unittest.TestCase):
+
+    def test_memory_overcommit_enabled(self):
+        with patch_sysctl_conf() as conf:
+            system.write_sysctl(overcommit_memory=True)
+            content = conf.read()
+        self.assertEqual('vm.overcommit_memory = 1\n', content)
+
+    def test_memory_overcommit_disabled(self):
+        with patch_sysctl_conf() as conf:
+            system.write_sysctl(overcommit_memory=False)
+            content = conf.read()
+        self.assertEqual('vm.overcommit_memory = 0\n', content)
+
+
[e-mail]('os.remove')
+class TestRemoveSysctl(unittest.TestCase):
+
+    def test_file_removed(self, mock_remove):
+        with patch_sysctl_conf() as conf:
+            system.remove_sysctl()
+            content = conf.read()
+        self.assertEqual('vm.overcommit_memory = 0\n', content)
+        mock_remove.assert_called_once_with(conf.name)