From 56495eecbac9652f1340b91c188d84ef18d1dac0 Mon Sep 17 00:00:00 2001 From: Andre Ruiz Date: Tue, 26 Feb 2019 17:13:40 +0100 Subject: [PATCH] Implement new option to enable discard on SSDs This change implements a new option called 'bdev-enable-discard' to control behaviour of issuing discards to SSDs on ceph bluestore. The new code tries to autodetect cases where it should be enabled by default but will allow forcing if desired. Change-Id: I7b83605c827eb4058bc4b46c92eb114c11108c93 Closes-Bug: #1788433 --- config.yaml | 9 ++++++ hooks/ceph_hooks.py | 8 ++++++ hooks/utils.py | 33 +++++++++++++++++++++ lib/ceph/utils.py | 2 +- templates/ceph.conf | 2 ++ unit_tests/test_ceph_hooks.py | 8 ++++++ unit_tests/test_ceph_utils.py | 54 +++++++++++++++++++++++++++++++++++ 7 files changed, 115 insertions(+), 1 deletion(-) diff --git a/config.yaml b/config.yaml index 1220334b..6fe04ffe 100644 --- a/config.yaml +++ b/config.yaml @@ -63,6 +63,15 @@ options: . For ceph >= 0.56.6 these can also be directories instead of devices - the charm assumes anything not starting with /dev is a directory instead. + bdev-enable-discard: + type: string + default: auto + description: | + Enables async discard on devices. This option will enable/disable both + bdev-enable-discard and bdev-async-discard options in ceph configuration + at the same time. The default value "auto" will try to autodetect and + should work in most cases. If you need to force a behaviour you can + set it to "enable" or "disable". Only applies for Ceph Mimic or later. osd-journal: type: string default: diff --git a/hooks/ceph_hooks.py b/hooks/ceph_hooks.py index c6b925fd..187c6c56 100755 --- a/hooks/ceph_hooks.py +++ b/hooks/ceph_hooks.py @@ -80,6 +80,7 @@ from utils import ( get_cluster_addr, get_blacklist, get_journal_devices, + should_enable_discard, ) from charmhelpers.contrib.openstack.alternatives import install_alternative from charmhelpers.contrib.network.ip import ( @@ -388,6 +389,13 @@ def get_ceph_context(upgrading=False): 'bluestore_block_db_size': config('bluestore-block-db-size'), } + if config('bdev-enable-discard').lower() == 'enabled': + cephcontext['bdev_discard'] = True + elif config('bdev-enable-discard').lower() == 'auto': + cephcontext['bdev_discard'] = should_enable_discard(get_devices()) + else: + cephcontext['bdev_discard'] = False + if config('prefer-ipv6'): dynamic_ipv6_address = get_ipv6_addr()[0] if not public_network: diff --git a/hooks/utils.py b/hooks/utils.py index b773e2d1..3064f7a5 100644 --- a/hooks/utils.py +++ b/hooks/utils.py @@ -15,6 +15,7 @@ import re import os import socket +import subprocess from charmhelpers.core.hookenv import ( unit_get, @@ -23,6 +24,7 @@ from charmhelpers.core.hookenv import ( network_get_primary_address, log, DEBUG, + WARNING, status_set, storage_get, storage_list, @@ -194,3 +196,34 @@ def get_journal_devices(): _blacklist = get_blacklist() return set(device for device in devices if device not in _blacklist and os.path.exists(device)) + + +def should_enable_discard(devices): + """ + Tries to autodetect if we can enable discard on devices and if that + discard can be asynchronous. We want to enable both options if there's + any SSDs unless any of them are using SATA <= 3.0, in which case + discard is supported but is a blocking operation. + """ + discard_enable = True + for device in devices: + # whitelist some devices that do not need checking + if (device.startswith("/dev/nvme") or + device.startswith("/dev/vd")): + continue + if (device.startswith("/dev/") and + os.path.exists(device) and + is_sata30orless(device)): + discard_enable = False + log("SSD Discard autodetection: {} is forcing discard off" + "(sata <= 3.0)".format(device), level=WARNING) + return discard_enable + + +def is_sata30orless(device): + result = subprocess.check_output(["/usr/sbin/smartctl", "-i", device]) + print(result) + for line in str(result).split("\\n"): + if re.match("SATA Version is: *SATA (1\.|2\.|3\.0)", str(line)): + return True + return False diff --git a/lib/ceph/utils.py b/lib/ceph/utils.py index 83388c9b..dac98c99 100644 --- a/lib/ceph/utils.py +++ b/lib/ceph/utils.py @@ -82,7 +82,7 @@ QUORUM = [LEADER, PEON] PACKAGES = ['ceph', 'gdisk', 'btrfs-tools', 'radosgw', 'xfsprogs', - 'lvm2', 'parted'] + 'lvm2', 'parted', 'smartmontools'] CEPH_KEY_MANAGER = 'ceph' VAULT_KEY_MANAGER = 'vault' diff --git a/templates/ceph.conf b/templates/ceph.conf index 77fce613..2682be01 100644 --- a/templates/ceph.conf +++ b/templates/ceph.conf @@ -75,6 +75,8 @@ osd journal size = {{ osd_journal_size }} filestore xattr use omap = true journal dio = {{ dio }} {%- endif %} +bdev enable discard = {{ bdev_discard }} +bdev async discard = {{ bdev_discard }} {%- if short_object_len %} osd max object name len = 256 diff --git a/unit_tests/test_ceph_hooks.py b/unit_tests/test_ceph_hooks.py index 23e9d851..b55b87ae 100644 --- a/unit_tests/test_ceph_hooks.py +++ b/unit_tests/test_ceph_hooks.py @@ -37,6 +37,8 @@ CHARM_CONFIG = {'config-flags': '', 'customize-failure-domain': False, 'bluestore': False, 'crush-initial-weight': '0', + 'bdev-enable-discard': 'enabled', + 'osd-devices': '/dev/vdb', 'bluestore': False, 'bluestore-block-wal-size': 0, 'bluestore-block-db-size': 0, @@ -84,6 +86,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': False, 'bluestore_experimental': False, 'bluestore_block_wal_size': 0, @@ -123,6 +126,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': False, 'bluestore_experimental': True, 'bluestore_block_wal_size': 0, @@ -168,6 +172,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': True, 'bluestore_experimental': False, 'bluestore_block_wal_size': BLUESTORE_WAL_TEST_SIZE, @@ -210,6 +215,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': True, 'bluestore_experimental': True, 'bluestore_block_wal_size': BLUESTORE_WAL_TEST_SIZE, @@ -250,6 +256,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': False, 'bluestore_experimental': False, 'bluestore_block_wal_size': 0, @@ -292,6 +299,7 @@ class CephHooksTestCase(unittest.TestCase): 'short_object_len': True, 'upgrade_in_progress': False, 'use_syslog': 'true', + 'bdev_discard': True, 'bluestore': False, 'bluestore_experimental': False, 'bluestore_block_wal_size': 0, diff --git a/unit_tests/test_ceph_utils.py b/unit_tests/test_ceph_utils.py index f58ae070..88dfefe4 100644 --- a/unit_tests/test_ceph_utils.py +++ b/unit_tests/test_ceph_utils.py @@ -61,3 +61,57 @@ class CephUtilsTestCase(unittest.TestCase): mock_os_path_exists.assert_called() mock_get_blacklist.assert_called() self.assertEqual(devices, set(['/dev/vdb'])) + + @patch('os.path.exists') + @patch.object(utils, 'is_sata30orless') + def test_should_enable_discard_yes(self, mock_is_sata30orless, + mock_os_path_exists): + devices = ['/dev/sda', '/dev/vda', '/dev/nvme0n1'] + mock_os_path_exists.return_value = True + mock_is_sata30orless.return_value = False + ret = utils.should_enable_discard(devices) + mock_os_path_exists.assert_called() + mock_is_sata30orless.assert_called() + self.assertEqual(ret, True) + + @patch('os.path.exists') + @patch.object(utils, 'is_sata30orless') + def test_should_enable_discard_no(self, mock_is_sata30orless, + mock_os_path_exists): + devices = ['/dev/sda', '/dev/vda', '/dev/nvme0n1'] + mock_os_path_exists.return_value = True + mock_is_sata30orless.return_value = True + ret = utils.should_enable_discard(devices) + mock_os_path_exists.assert_called() + mock_is_sata30orless.assert_called() + self.assertEqual(ret, False) + + @patch('subprocess.check_output') + def test_is_sata30orless_sata31(self, mock_subprocess_check_output): + extcmd_output = (b'supressed text\nSATA Version is: ' + b'SATA 3.1, 6.0 Gb/s (current: 6.0 Gb/s)\n' + b'supressed text\n\n') + mock_subprocess_check_output.return_value = extcmd_output + ret = utils.is_sata30orless('/dev/sda') + mock_subprocess_check_output.assert_called() + self.assertEqual(ret, False) + + @patch('subprocess.check_output') + def test_is_sata30orless_sata30(self, mock_subprocess_check_output): + extcmd_output = (b'supressed text\nSATA Version is: ' + b'SATA 3.0, 6.0 Gb/s (current: 6.0 Gb/s)\n' + b'supressed text\n\n') + mock_subprocess_check_output.return_value = extcmd_output + ret = utils.is_sata30orless('/dev/sda') + mock_subprocess_check_output.assert_called() + self.assertEqual(ret, True) + + @patch('subprocess.check_output') + def test_is_sata30orless_sata26(self, mock_subprocess_check_output): + extcmd_output = (b'supressed text\nSATA Version is: ' + b'SATA 2.6, 3.0 Gb/s (current: 3.0 Gb/s)\n' + b'supressed text\n\n') + mock_subprocess_check_output.return_value = extcmd_output + ret = utils.is_sata30orless('/dev/sda') + mock_subprocess_check_output.assert_called() + self.assertEqual(ret, True)