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

Proposed by Francesco Banconi
Status: Merged
Merged at revision: 9
Proposed branch: lp:~frankban/charms/trusty/redis/memory-management
Merge into: lp:~juju-gui/charms/trusty/redis/trunk
Diff against target: 555 lines (+302/-27)
8 files modified
config.yaml (+45/-3)
hooks/services.py (+3/-1)
hooks/serviceutils.py (+27/-0)
hooks/settings.py (+3/-0)
hooks/system.py (+33/-0)
tests/test_10_deploy.py (+26/-5)
unit_tests/test_serviceutils.py (+106/-18)
unit_tests/test_system.py (+59/-0)
To merge this branch: bzr merge lp:~frankban/charms/trusty/redis/memory-management
Reviewer Review Type Date Requested Status
Richard Harding Approve
Jay R. Wren (community) Approve
Review via email: mp+265136@code.launchpad.net

Description of the change

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!

To post a comment you must log in.
Revision history for this message
Jay R. Wren (evarlast) wrote :

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
Revision history for this message
Richard Harding (rharding) wrote :

LGTM with a few questions/comments.

review: Approve
18. By Francesco Banconi

Improve memory overcommit option description.

Revision history for this message
Francesco Banconi (frankban) wrote :

Thanks for the reviews!

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'config.yaml'
2--- config.yaml 2015-06-19 09:47:04 +0000
3+++ config.yaml 2015-07-20 11:52:30 +0000
4@@ -44,6 +44,48 @@
5 Set the number of databases. The default database is DB 0. You can select
6 a different one on a per-connection basis using SELECT <dbid> where dbid
7 is a number between 0 and 'databases'-1.
8-
9-
10-
11+ maxmemory:
12+ type: string
13+ default: ""
14+ description: |
15+ Don't use more than the specified amount of memory.
16+ When the memory limit is reached Redis will try to remove keys according
17+ to the eviction policy selected (see "maxmemory-policy" below).
18+ If Redis can't remove keys according to the policy, or if the policy is
19+ set to "noeviction", Redis will start to reply with errors to commands
20+ that would use more memory, like SET, LPUSH, and so on, and will continue
21+ to reply to read-only commands like GET.
22+ This option is usually useful when using Redis as an LRU cache, or to set
23+ a hard memory limit for an instance (using the "noeviction" policy).
24+ It is possible to specify the memory size in the usual form of 1k, 5GB,
25+ 4M and so forth:
26+ - 1k: 1000 bytes;
27+ - 1kb: 1024 bytes;
28+ - 1m: 1000000 bytes;
29+ - 1mb: 1024*1024 bytes;
30+ - 1g: 1000000000 bytes;
31+ - 1gb: 1024*1024*1024 bytes.
32+ maxmemory-policy:
33+ type: string
34+ default: volatile-lru
35+ description: |
36+ how Redis will select what to remove when maxmemory is reached.
37+ You can select among five behaviors:
38+ - volatile-lru: remove the key with an expire set using an LRU algorithm;
39+ - allkeys-lru: remove any key according to the LRU algorithm;
40+ - volatile-random: remove a random key with an expire set;
41+ - allkeys-random: remove a random key, any key;
42+ - volatile-ttl: remove the key with the nearest expire time (minor TTL);
43+ - noeviction: don't expire at all, just return an error on write
44+ operations.
45+ Note: with any of the above policies, Redis will return an error on write
46+ operations, when there are no suitable keys for eviction.
47+ memory-overcommit:
48+ type: boolean
49+ default: true
50+ description: |
51+ Set the Linux kernel overcommit memory setting to 1 so that no memory
52+ overcommit handling is performed. When this is enabled, the performance
53+ for memory-intensive tasks is increased, but there is more risk of memory
54+ overload. This option is ignored when the charm is deployed to a local
55+ environment.
56
57=== modified file 'hooks/services.py'
58--- hooks/services.py 2015-05-25 13:34:16 +0000
59+++ hooks/services.py 2015-07-20 11:52:30 +0000
60@@ -26,8 +26,8 @@
61 from charmhelpers.core.services import base
62
63 import hookutils
64+import relations
65 import serviceutils
66-import relations
67
68
69 @hookutils.hook_name_logged
70@@ -60,6 +60,7 @@
71
72 # Callables called when required data is ready.
73 'data_ready': [
74+ serviceutils.write_sysctl(config),
75 serviceutils.write_config_file(
76 config,
77 db_relation=db_relation,
78@@ -87,6 +88,7 @@
79
80 # Callables called when required data is ready.
81 'data_ready': [
82+ serviceutils.write_sysctl(config),
83 serviceutils.write_config_file(
84 config,
85 db_relation=db_relation,
86
87=== modified file 'hooks/serviceutils.py'
88--- hooks/serviceutils.py 2015-05-26 09:41:30 +0000
89+++ hooks/serviceutils.py 2015-07-20 11:52:30 +0000
90@@ -16,6 +16,7 @@
91 import configfile
92 import hookutils
93 import settings
94+import system
95
96
97 def service_start(port, previous_port, service_name):
98@@ -51,6 +52,9 @@
99 # Remove redis package and clean up files.
100 hookutils.log('Removing system packages.')
101 fetch.apt_purge(settings.PACKAGES)
102+ # Restore the default sysctl values and remove the sysctl file.
103+ hookutils.log('Cleaning up sysctl state.')
104+ system.remove_sysctl()
105
106
107 def write_config_file(
108@@ -87,6 +91,20 @@
109 return callback
110
111
112+def write_sysctl(config):
113+ """Handle sysctl configuration file creation.
114+
115+ Return a function that can be used as a callback in the services framework,
116+ and that generates the redis syctl configuration file.
117+ """
118+ def callback(service_name):
119+ hookutils.log('Setting up sysctl state.')
120+ overcommit_memory = config.get('memory-overcommit', True)
121+ system.write_sysctl(overcommit_memory=overcommit_memory)
122+
123+ return callback
124+
125+
126 def _get_service_options(config, slave_relation=None):
127 """Return a dict containing the redis service configuration options.
128
129@@ -108,9 +126,18 @@
130 'tcp-keepalive': config['tcp-keepalive'],
131 'timeout': config['timeout'],
132 }
133+ # Add memory management options.
134+ maxmemory = config.get('maxmemory', '').strip()
135+ if maxmemory:
136+ options.update({
137+ 'maxmemory': maxmemory,
138+ 'maxmemory-policy': config['maxmemory-policy'],
139+ })
140+ # Add authentication options.
141 password = config['password'].strip()
142 if password:
143 options['requirepass'] = password
144+ # Add master/slave relationship options.
145 if slave_relation is not None:
146 hookutils.log('Setting up slave relation.')
147 # If slave_relation is defined, it is assumed that the relation is
148
149=== modified file 'hooks/settings.py'
150--- hooks/settings.py 2015-05-25 14:09:49 +0000
151+++ hooks/settings.py 2015-07-20 11:52:30 +0000
152@@ -8,6 +8,9 @@
153 DEFAULT_REDIS_CONF = '/etc/redis/redis.conf'
154 REDIS_CONF = '/etc/redis/redis-charm.conf'
155
156+# Define the path to the redis sysctl configuration file.
157+SYSCTL_CONF = '/etc/sysctl.d/60-redis.conf'
158+
159 # Define Debian packages to be installed.
160 PACKAGES = ['redis-server']
161
162
163=== added file 'hooks/system.py'
164--- hooks/system.py 1970-01-01 00:00:00 +0000
165+++ hooks/system.py 2015-07-20 11:52:30 +0000
166@@ -0,0 +1,33 @@
167+# Copyright 2015 Canonical Ltd.
168+# Licensed under the GPLv3, see copyright file for details.
169+
170+"""Helpers for interacting with the operating system."""
171+
172+import os
173+
174+from charmhelpers.core import host
175+
176+import settings
177+
178+
179+def write_sysctl(overcommit_memory=False):
180+ """Set the sysctl "vm.overcommit_memory" based on the given boolean value.
181+
182+ The default value indicates the system default if values were not changed.
183+ Also restart procps when done, so that new values take effect.
184+
185+ Note that this will not work when using the local provider, since /sys
186+ is a read-only file system and sysctl options really belong to the host.
187+ Anyway, this function still executes without errors.
188+ """
189+ value = 1 if overcommit_memory else 0
190+ with open(settings.SYSCTL_CONF, 'w') as sysctl_file:
191+ # See http://redis.io/topics/admin.
192+ sysctl_file.write('vm.overcommit_memory = {}\n'.format(value))
193+ host.service_restart('procps')
194+
195+
196+def remove_sysctl():
197+ """Remove the sysctl configuration file for redis."""
198+ write_sysctl()
199+ os.remove(settings.SYSCTL_CONF)
200
201=== modified file 'tests/test_10_deploy.py'
202--- tests/test_10_deploy.py 2015-05-25 15:59:59 +0000
203+++ tests/test_10_deploy.py 2015-07-20 11:52:30 +0000
204@@ -25,11 +25,13 @@
205 import settings
206
207
208-# Define a test decorator for running the test only if the current environment
209-# type is local.
210+# Define a test decorator for running/skipping tests based on whether the suite
211+# is using a local Juju environment.
212+_env_type = helpers.get_environment_type()
213 only_on_local_environments = unittest.skipIf(
214- helpers.get_environment_type() != 'local',
215- 'only available whe using a local environment')
216+ _env_type != 'local', 'only available when using a local environment')
217+not_on_local_environments = unittest.skipIf(
218+ _env_type == 'local', 'not available when using a local environment')
219
220
221 class TestDeployment(unittest.TestCase):
222@@ -68,6 +70,12 @@
223 client.set('my-key', 'my-value')
224 self.assertEqual('my-value', client.get('my-key'))
225
226+ @not_on_local_environments
227+ def test_sysctl(self):
228+ output, retcode = self.unit.run('sysctl vm.overcommit_memory')
229+ self.assertEqual(0, retcode, output)
230+ self.assertEqual('vm.overcommit_memory = 1', output)
231+
232
233 class TestDeploymentOptions(unittest.TestCase):
234
235@@ -78,6 +86,9 @@
236 'loglevel': 'verbose',
237 'logfile': '/tmp/redis.log',
238 'timeout': 60,
239+ 'maxmemory': '1g',
240+ 'maxmemory-policy': 'noeviction',
241+ 'memory-overcommit': False,
242 }
243
244 @classmethod
245@@ -97,6 +108,8 @@
246 'databases 3\n'
247 'logfile /tmp/redis.log\n'
248 'loglevel verbose\n'
249+ 'maxmemory 1g\n'
250+ 'maxmemory-policy noeviction\n'
251 'port 4242\n'
252 'requirepass secret\n'
253 'tcp-keepalive 0\n'
254@@ -116,6 +129,12 @@
255 client.set('my-key', 'my-value')
256 self.assertEqual('my-value', client.get('my-key'))
257
258+ @not_on_local_environments
259+ def test_sysctl(self):
260+ output, retcode = self.unit.run('sysctl vm.overcommit_memory')
261+ self.assertEqual(0, retcode, output)
262+ self.assertEqual('vm.overcommit_memory = 0', output)
263+
264
265 class TestMasterSlaveRelation(unittest.TestCase):
266
267@@ -178,7 +197,7 @@
268
269 class TestMasterSlaveRelationOptions(unittest.TestCase):
270
271- master_options = {'databases': 5, 'password': 'secret'}
272+ master_options = {'databases': 5, 'password': 'secret', 'maxmemory': '2g'}
273 slave_options = {'port': 4747, 'loglevel': 'warning', 'timeout': 42}
274
275 @classmethod
276@@ -202,6 +221,8 @@
277 'databases 5\n'
278 'logfile /var/log/redis/redis-server.log\n'
279 'loglevel notice\n'
280+ 'maxmemory 2g\n'
281+ 'maxmemory-policy volatile-lru\n'
282 'port 6379\n'
283 'requirepass secret\n'
284 'tcp-keepalive 0\n'
285
286=== modified file 'unit_tests/test_serviceutils.py'
287--- unit_tests/test_serviceutils.py 2015-05-25 15:34:53 +0000
288+++ unit_tests/test_serviceutils.py 2015-07-20 11:52:30 +0000
289@@ -79,6 +79,7 @@
290 @mock.patch('charmhelpers.fetch.apt_purge')
291 @mock.patch('charmhelpers.core.host.service_stop')
292 @mock.patch('charmhelpers.core.hookenv.close_port')
293+@mock.patch('system.remove_sysctl')
294 @mock.patch('hookutils.log')
295 class TestServiceStop(unittest.TestCase):
296
297@@ -86,22 +87,23 @@
298 port = 6379
299
300 def test_service_running_stop_hook(
301- self, mock_log, mock_close_port, mock_service_stop,
302- mock_apt_purge):
303+ self, mock_log, mock_remove_sysctl, mock_close_port,
304+ mock_service_stop, mock_apt_purge):
305 with patch_service_running(True):
306 with patch_hook_name('stop'):
307 serviceutils.service_stop(self.port, self.service_name)
308 mock_service_stop.assert_called_once_with(settings.SERVICE_NAME)
309- self.assertEqual(2, mock_log.call_count)
310+ self.assertEqual(3, mock_log.call_count)
311 mock_log.assert_has_calls([
312 mock.call('Stopping service {}.'.format(self.service_name)),
313- mock.call('Removing system packages.')
314+ mock.call('Removing system packages.'),
315+ mock.call('Cleaning up sysctl state.'),
316 ])
317 mock_close_port.assert_called_once_with(self.port)
318
319 def test_service_not_running_stop_hook(
320- self, mock_log, mock_close_port, mock_service_stop,
321- mock_apt_purge):
322+ self, mock_log, mock_remove_sysctl, mock_close_port,
323+ mock_service_stop, mock_apt_purge):
324 with patch_service_running(False):
325 with patch_hook_name('stop'):
326 serviceutils.service_stop(self.port, self.service_name)
327@@ -109,8 +111,8 @@
328 mock_close_port.assert_called_once_with(self.port)
329
330 def test_customized_port(
331- self, mock_log, mock_close_port, mock_service_stop,
332- mock_apt_purge):
333+ self, mock_log, mock_remove_sysctl, mock_close_port,
334+ mock_service_stop, mock_apt_purge):
335 port = 4747
336 with patch_service_running(False):
337 with patch_hook_name('stop'):
338@@ -118,18 +120,23 @@
339 mock_close_port.assert_called_once_with(port)
340
341 def test_cleaning_up(
342- self, mock_log, mock_close_port, mock_service_stop,
343- mock_apt_purge):
344+ self, mock_log, mock_remove_sysctl, mock_close_port,
345+ mock_service_stop, mock_apt_purge):
346 port = 4747
347 with patch_service_running(False):
348 with patch_hook_name('stop'):
349 serviceutils.service_stop(port, self.service_name)
350- mock_log.assert_called_once_with('Removing system packages.')
351+ self.assertEqual(2, mock_log.call_count)
352+ mock_log.assert_has_calls([
353+ mock.call('Removing system packages.'),
354+ mock.call('Cleaning up sysctl state.'),
355+ ])
356 mock_apt_purge.assert_called_once_with(settings.PACKAGES)
357+ mock_remove_sysctl.assert_called_once_with()
358
359 def test_service_running_other_hook(
360- self, mock_log, mock_close_port, mock_service_stop,
361- mock_apt_purge):
362+ self, mock_log, mock_remove_sysctl, mock_close_port,
363+ mock_service_stop, mock_apt_purge):
364 with patch_service_running(True):
365 with patch_hook_name('config-changed'):
366 serviceutils.service_stop(self.port, self.service_name)
367@@ -139,8 +146,8 @@
368 self.assertFalse(mock_apt_purge.called)
369
370 def test_service_not_running_other_hook(
371- self, mock_log, mock_close_port, mock_service_stop,
372- mock_apt_purge):
373+ self, mock_log, mock_remove_sysctl, mock_close_port,
374+ mock_service_stop, mock_apt_purge):
375 with patch_service_running(False):
376 with patch_hook_name('config-changed'):
377 serviceutils.service_stop(self.port, self.service_name)
378@@ -216,9 +223,38 @@
379 mocks.log.assert_has_calls([
380 mock.call('Retrieving service options.'),
381 mock.call('Writing configuration file for foo.'),
382- mock.call('Restarting service due to configuration change.')
383+ mock.call('Restarting service due to configuration change.'),
384 ])
385
386+ def test_configuration_changed_maxmemory(self):
387+ config = {
388+ 'databases': 3,
389+ 'logfile': '/path/to/logfile',
390+ 'loglevel': 'debug',
391+ 'maxmemory': '5gb',
392+ 'maxmemory-policy': 'volatile-lru',
393+ 'password': '',
394+ 'port': 4242,
395+ 'tcp-keepalive': 10,
396+ 'timeout': 42,
397+ }
398+ callback = serviceutils.write_config_file(config)
399+ with self.patch_all(configuration_changed=True) as mocks:
400+ callback('foo')
401+ mocks.write.assert_called_once_with({
402+ 'bind': '1.2.3.4',
403+ 'databases': 3,
404+ 'maxmemory': '5gb',
405+ 'maxmemory-policy': 'volatile-lru',
406+ 'logfile': '/path/to/logfile',
407+ 'loglevel': 'debug',
408+ 'port': 4242,
409+ 'tcp-keepalive': 10,
410+ 'timeout': 42,
411+ }, settings.REDIS_CONF)
412+ mocks.unit_get.assert_called_once_with('private-address')
413+ mocks.service_restart.assert_called_once_with(settings.SERVICE_NAME)
414+
415 def test_configuration_changed_password(self):
416 config = {
417 'databases': 3,
418@@ -307,7 +343,7 @@
419 mocks.log.assert_has_calls([
420 mock.call('Retrieving service options.'),
421 mock.call('Writing configuration file for foo.'),
422- mock.call('No changes detected in the configuration file.')
423+ mock.call('No changes detected in the configuration file.'),
424 ])
425
426 def test_configuration_unchanged_slave(self):
427@@ -345,7 +381,42 @@
428 mock.call('Retrieving service options.'),
429 mock.call('Setting up slave relation.'),
430 mock.call('Writing configuration file for foo.'),
431- mock.call('No changes detected in the configuration file.')
432+ mock.call('No changes detected in the configuration file.'),
433+ ])
434+
435+ def test_configuration_unchanged_master_maxmemory(self):
436+ config = {
437+ 'databases': 3,
438+ 'logfile': '/path/to/logfile',
439+ 'loglevel': 'debug',
440+ 'maxmemory': '42m',
441+ 'maxmemory-policy': 'noeviction',
442+ 'password': '',
443+ 'port': 4242,
444+ 'tcp-keepalive': 60,
445+ 'timeout': 10,
446+ }
447+ callback = serviceutils.write_config_file(config)
448+ with self.patch_all() as mocks:
449+ callback('foo')
450+ mocks.write.assert_called_once_with({
451+ 'bind': '1.2.3.4',
452+ 'databases': 3,
453+ 'logfile': '/path/to/logfile',
454+ 'loglevel': 'debug',
455+ 'maxmemory': '42m',
456+ 'maxmemory-policy': 'noeviction',
457+ 'port': 4242,
458+ 'tcp-keepalive': 60,
459+ 'timeout': 10,
460+ }, settings.REDIS_CONF)
461+ mocks.unit_get.assert_called_once_with('private-address')
462+ self.assertFalse(mocks.service_restart.called)
463+ self.assertEqual(3, mocks.log.call_count)
464+ mocks.log.assert_has_calls([
465+ mock.call('Retrieving service options.'),
466+ mock.call('Writing configuration file for foo.'),
467+ mock.call('No changes detected in the configuration file.'),
468 ])
469
470 def test_configuration_unchanged_master_password(self):
471@@ -406,3 +477,20 @@
472 }, settings.REDIS_CONF)
473 mocks.unit_get.assert_called_once_with('private-address')
474 self.assertFalse(mocks.service_restart.called)
475+
476+
477+@mock.patch('system.write_sysctl')
478+@mock.patch('hookutils.log')
479+class TestWriteSysctl(unittest.TestCase):
480+
481+ def test_memory_overcommit_enabled(self, mock_log, mock_write_sysctl):
482+ callback = serviceutils.write_sysctl({'memory-overcommit': True})
483+ callback('service')
484+ mock_log.assert_called_once_with('Setting up sysctl state.')
485+ mock_write_sysctl.assert_called_once_with(overcommit_memory=True)
486+
487+ def test_memory_overcommit_disabled(self, mock_log, mock_write_sysctl):
488+ callback = serviceutils.write_sysctl({'memory-overcommit': False})
489+ callback('service')
490+ mock_log.assert_called_once_with('Setting up sysctl state.')
491+ mock_write_sysctl.assert_called_once_with(overcommit_memory=False)
492
493=== added file 'unit_tests/test_system.py'
494--- unit_tests/test_system.py 1970-01-01 00:00:00 +0000
495+++ unit_tests/test_system.py 2015-07-20 11:52:30 +0000
496@@ -0,0 +1,59 @@
497+# Copyright 2015 Canonical Ltd.
498+# Licensed under the GPLv3, see copyright file for details.
499+
500+import contextlib
501+from pkg_resources import resource_filename
502+import sys
503+import tempfile
504+import unittest
505+
506+import mock
507+
508+# Allow importing modules and packages from the hooks directory.
509+sys.path.append(resource_filename(__name__, '../hooks'))
510+
511+import system
512+
513+
514+@contextlib.contextmanager
515+def patch_sysctl_conf():
516+ """Replace the sysctl configuration path.
517+
518+ Return a temporary file that can be used for tests when entering the
519+ context block. The file is automatically closed and deleted when exiting
520+ the context block.
521+ """
522+ conf = tempfile.NamedTemporaryFile()
523+ with mock.patch('charmhelpers.core.host.service_restart') as mock_restart:
524+ with mock.patch('settings.SYSCTL_CONF', conf.name):
525+ try:
526+ yield conf
527+ finally:
528+ conf.close()
529+ mock_restart.assert_called_once_with('procps')
530+
531+
532+class TestWriteSysctl(unittest.TestCase):
533+
534+ def test_memory_overcommit_enabled(self):
535+ with patch_sysctl_conf() as conf:
536+ system.write_sysctl(overcommit_memory=True)
537+ content = conf.read()
538+ self.assertEqual('vm.overcommit_memory = 1\n', content)
539+
540+ def test_memory_overcommit_disabled(self):
541+ with patch_sysctl_conf() as conf:
542+ system.write_sysctl(overcommit_memory=False)
543+ content = conf.read()
544+ self.assertEqual('vm.overcommit_memory = 0\n', content)
545+
546+
547+@mock.patch('os.remove')
548+class TestRemoveSysctl(unittest.TestCase):
549+
550+ def test_file_removed(self, mock_remove):
551+ with patch_sysctl_conf() as conf:
552+ system.remove_sysctl()
553+ content = conf.read()
554+ self.assertEqual('vm.overcommit_memory = 0\n', content)
555+ mock_remove.assert_called_once_with(conf.name)

Subscribers

People subscribed via source and target branches