From d6061caa2cb1ab13ffd476a6d7f39b5b7ff75803 Mon Sep 17 00:00:00 2001 From: Billy Olsen Date: Thu, 8 Jun 2017 16:01:17 -0700 Subject: [PATCH] Only change owner/permissions of new devices Do not change owner and permissions of already existing devices in the setup_storage() function as this runs during every config-changed hook invocation. Change-Id: I21f23aee34d315ccb4df303527b4d791fc043f58 Closes-Bug: #1676728 --- lib/swift_storage_utils.py | 4 ++-- unit_tests/test_swift_storage_utils.py | 25 ++++++++++++++++++++----- 2 files changed, 22 insertions(+), 7 deletions(-) diff --git a/lib/swift_storage_utils.py b/lib/swift_storage_utils.py index 5aa0b1e..9647267 100644 --- a/lib/swift_storage_utils.py +++ b/lib/swift_storage_utils.py @@ -464,8 +464,8 @@ def setup_storage(): mount(dev, mountpoint, filesystem=filesystem) fstab_add(dev, mountpoint, filesystem, options=options) - check_call(['chown', '-R', 'swift:swift', '/srv/node/']) - check_call(['chmod', '-R', '0755', '/srv/node/']) + check_call(['chown', '-R', 'swift:swift', mountpoint]) + check_call(['chmod', '-R', '0755', mountpoint]) @retry_on_exception(3, base_delay=2, exc_type=CalledProcessError) diff --git a/unit_tests/test_swift_storage_utils.py b/unit_tests/test_swift_storage_utils.py index 712d731..c83aed4 100644 --- a/unit_tests/test_swift_storage_utils.py +++ b/unit_tests/test_swift_storage_utils.py @@ -169,7 +169,7 @@ class SwiftStorageUtilsTests(CharmTestCase): self.assertEqual(ex, result) @patch.object(swift_utils, 'ensure_block_device') - def test_determine_block_device_dublicate_dev(self, _ensure): + def test_determine_block_device_duplicate_dev(self, _ensure): _ensure.side_effect = self._fake_ensure bdevs = '/dev/vdb /dev/vdc /dev/vdc /dev/vdb /tmp/swift.img|1G' self.test_config.set('block-device', bdevs) @@ -246,8 +246,8 @@ class SwiftStorageUtilsTests(CharmTestCase): determine.return_value = ['/dev/vdb'] swift_utils.setup_storage() self.assertFalse(clean.called) - calls = [call(['chown', '-R', 'swift:swift', '/srv/node/']), - call(['chmod', '-R', '0755', '/srv/node/'])] + calls = [call(['chown', '-R', 'swift:swift', '/srv/node/vdb']), + call(['chmod', '-R', '0755', '/srv/node/vdb'])] self.check_call.assert_has_calls(calls) self.mkdir.assert_has_calls([ call('/srv/node', owner='swift', group='swift', @@ -274,8 +274,8 @@ class SwiftStorageUtilsTests(CharmTestCase): self.fstab_add.assert_called_with('/dev/vdb', '/srv/node/vdb', 'xfs', options=None) - calls = [call(['chown', '-R', 'swift:swift', '/srv/node/']), - call(['chmod', '-R', '0755', '/srv/node/'])] + calls = [call(['chown', '-R', 'swift:swift', '/srv/node/vdb']), + call(['chmod', '-R', '0755', '/srv/node/vdb'])] self.check_call.assert_has_calls(calls) self.mkdir.assert_has_calls([ call('/srv/node', owner='swift', group='swift', @@ -283,6 +283,21 @@ class SwiftStorageUtilsTests(CharmTestCase): call('/srv/node/vdb', group='swift', owner='swift') ]) + @patch.object(swift_utils, 'is_device_in_ring') + @patch.object(swift_utils, 'determine_block_devices') + def test_setup_storage_no_chmod_existing_devs(self, determine_block_devs, + mock_is_device_in_ring): + """ + Verifies that only newly added and formatted storage devices are + chmodded and chowned and not the entire /srv/node directory. Doing + this will cause unnecessary write updates and for a production cluster, + there could potentially be a lot of files to process. + """ + determine_block_devs.return_values = ['/dev/vdb', '/dev/vdc'] + mock_is_device_in_ring.return_value = True + swift_utils.setup_storage() + self.assertEquals(self.check_call.call_count, 0) + def _fake_is_device_mounted(self, device): if device in ["/dev/sda", "/dev/vda", "/dev/cciss/c0d0"]: return True