From 91074c6a335e22871b74cb7210a1e421a1b808a4 Mon Sep 17 00:00:00 2001 From: Matt Van Dijk Date: Tue, 4 Oct 2016 11:44:41 -0400 Subject: [PATCH] Fix mountpoint detection During guestagent volume operations the calls to unmount check if the volume is mounted first. The python function os.path.ismount returns False if the directory if not readable by the current user, breaking this functionality for some datastores. The symptom of this failure is that the device ends up mounted twice during prepare and then fails to unmount fully during resize. The fix is to create a custom is_mount function that runs as the root user. Change-Id: I151402717386230371bafcedc170d70b3588e912 Closes-Bug: #1645773 --- ...mountpoint-detection-096734f0097eb75a.yaml | 4 ++++ trove/guestagent/common/operating_system.py | 20 +++++++++++++++++++ trove/guestagent/volume.py | 16 +++++---------- .../tests/unittests/guestagent/test_volume.py | 17 +++++++--------- 4 files changed, 36 insertions(+), 21 deletions(-) create mode 100644 releasenotes/notes/mountpoint-detection-096734f0097eb75a.yaml diff --git a/releasenotes/notes/mountpoint-detection-096734f0097eb75a.yaml b/releasenotes/notes/mountpoint-detection-096734f0097eb75a.yaml new file mode 100644 index 0000000000..0971bbfbe8 --- /dev/null +++ b/releasenotes/notes/mountpoint-detection-096734f0097eb75a.yaml @@ -0,0 +1,4 @@ +--- +fixes: + - Improved mountpoint detection by running it as root. This prevents guests + that have undiscoverable mount points from failing to unmount. diff --git a/trove/guestagent/common/operating_system.py b/trove/guestagent/common/operating_system.py index aacb1391a2..282d8857b9 100644 --- a/trove/guestagent/common/operating_system.py +++ b/trove/guestagent/common/operating_system.py @@ -817,3 +817,23 @@ def _build_command_options(options): """ return ['-' + item[0] for item in options if item[1]] + + +def get_device(path, as_root=False): + """Get the device that a given path exists on.""" + stdout = _execute_shell_cmd('df', [], path, as_root=as_root) + return stdout.splitlines()[1].split()[0] + + +def is_mount(path): + """Check if the given directory path is a mountpoint. Try the standard + ismount first. This fails if the path is not accessible though, so resort + to checking as the root user (which is slower). + """ + if os.access(path, os.R_OK): + return os.path.ismount(path) + if not exists(path, is_directory=True, as_root=True): + return False + directory_dev = get_device(path, as_root=True) + parent_dev = get_device(os.path.join(path, '..'), as_root=True) + return directory_dev != parent_dev diff --git a/trove/guestagent/volume.py b/trove/guestagent/volume.py index 5427a2fe28..2558b0bf60 100644 --- a/trove/guestagent/volume.py +++ b/trove/guestagent/volume.py @@ -135,7 +135,7 @@ class VolumeDevice(object): "Error resizing the filesystem: %s") % self.device_path) def unmount(self, mount_point): - if os.path.exists(mount_point): + if operating_system.is_mount(mount_point): cmd = "sudo umount %s" % mount_point child = pexpect.spawn(cmd) child.expect(pexpect.EOF) @@ -151,16 +151,10 @@ class VolumeDevice(object): def mount_points(self, device_path): """Returns a list of mount points on the specified device.""" - try: - cmd = "grep %s /etc/mtab | awk '{print $2}'" % device_path - stdout, stderr = utils.execute(cmd, shell=True) - return stdout.strip().split('\n') - - except ProcessExecutionError: - LOG.exception(_("Error retrieving mount points")) - raise GuestError(original_message=_( - "Could not obtain a list of mount points for device: %s") % - device_path) + stdout, stderr = utils.execute( + "grep %s /etc/mtab" % device_path, + shell=True, check_exit_code=[0, 1]) + return [entry.strip().split()[1] for entry in stdout.splitlines()] def set_readahead_size(self, readahead_size, execute_function=utils.execute): diff --git a/trove/tests/unittests/guestagent/test_volume.py b/trove/tests/unittests/guestagent/test_volume.py index efbb96bb7c..6ef160659d 100644 --- a/trove/tests/unittests/guestagent/test_volume.py +++ b/trove/tests/unittests/guestagent/test_volume.py @@ -19,6 +19,7 @@ import pexpect from trove.common.exception import GuestError, ProcessExecutionError from trove.common import utils +from trove.guestagent.common import operating_system from trove.guestagent import volume from trove.tests.unittests import trove_testtools @@ -160,8 +161,8 @@ class VolumeDeviceTest(trove_testtools.TestCase): @patch.object(pexpect, 'spawn', Mock()) def _test_unmount(self, positive=True): - origin_ = os.path.exists - os.path.exists = MagicMock(return_value=positive) + origin_is_mount = operating_system.is_mount + operating_system.is_mount = MagicMock(return_value=positive) fake_spawn = _setUp_fake_spawn() self.volumeDevice.unmount('/mnt/volume') @@ -169,19 +170,15 @@ class VolumeDeviceTest(trove_testtools.TestCase): if not positive: COUNT = 0 self.assertEqual(COUNT, fake_spawn.expect.call_count) - os.path.exists = origin_ + operating_system.is_mount = origin_is_mount - @patch.object(utils, 'execute', return_value=('/var/lib/mysql', '')) + @patch.object(utils, 'execute') def test_mount_points(self, mock_execute): + mock_execute.return_value = ( + ("/dev/vdb /var/lib/mysql xfs rw 0 0", "")) mount_point = self.volumeDevice.mount_points('/dev/vdb') self.assertEqual(['/var/lib/mysql'], mount_point) - @patch.object(utils, 'execute', side_effect=ProcessExecutionError) - @patch('trove.guestagent.volume.LOG') - def test_fail_mount_points(self, mock_logging, mock_execute): - self.assertRaises(GuestError, self.volumeDevice.mount_points, - '/mnt/volume') - def test_set_readahead_size(self): origin_check_device_exists = self.volumeDevice._check_device_exists self.volumeDevice._check_device_exists = MagicMock()