From 7d995b6cf6552c1119dcde9f7cdf2c5a05b3b92b Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Thu, 18 Jun 2020 21:05:05 +0200 Subject: [PATCH] RBD: Fix check_valid_device The check_valid_device method of the RBD connector doesn't work for locally attached volumes (using krbd) because it only has code for handles. This patch adds code to handle the check of locally attached RBD volumes running the code as root and as the current user. In order to preserve current behavior we don't change the code for checking valid handles, which means we are ignoring the run_as_root parameter. Closes-Bug: #1884552 Related-Bug: #1883720 Change-Id: Iedf1d4678eeef438ac4f03a61f5f91f3a225c3fd --- os_brick/initiator/connectors/rbd.py | 25 ++++++--- os_brick/privileged/rbd.py | 7 +++ .../tests/initiator/connectors/test_rbd.py | 55 +++++++++++++++++++ os_brick/tests/privileged/test_rbd.py | 12 ++++ ...d_check_valid_device-2f50c0639adb8e7c.yaml | 5 ++ 5 files changed, 97 insertions(+), 7 deletions(-) create mode 100644 releasenotes/notes/rbd_check_valid_device-2f50c0639adb8e7c.yaml diff --git a/os_brick/initiator/connectors/rbd.py b/os_brick/initiator/connectors/rbd.py index 1c1162932..8d0122eac 100644 --- a/os_brick/initiator/connectors/rbd.py +++ b/os_brick/initiator/connectors/rbd.py @@ -352,13 +352,8 @@ class RBDConnector(base.BaseLinuxConnector): fileutils.delete_if_exists(rbd_handle.rbd_conf) rbd_handle.close() - def check_valid_device(self, path, run_as_root=True): - """Verify an existing RBD handle is connected and valid.""" - rbd_handle = path - - if rbd_handle is None: - return False - + @staticmethod + def _check_valid_device(rbd_handle): original_offset = rbd_handle.tell() try: @@ -372,6 +367,22 @@ class RBDConnector(base.BaseLinuxConnector): return True + def check_valid_device(self, path, run_as_root=True): + """Verify an existing RBD handle is connected and valid.""" + if not path: + return False + + # We can receive a file handle or a path to a device + if isinstance(path, str): + if run_as_root: + return rbd_privsep.check_valid_path(path) + else: + with open(path, 'rb') as rbd_handle: + return self._check_valid_device(rbd_handle) + + # For backward compatibility ignore run_as_root param with handles + return self._check_valid_device(path) + def extend_volume(self, connection_properties): # TODO(walter-boring): is this possible? raise NotImplementedError diff --git a/os_brick/privileged/rbd.py b/os_brick/privileged/rbd.py index a861169d1..89f4a225e 100644 --- a/os_brick/privileged/rbd.py +++ b/os_brick/privileged/rbd.py @@ -50,3 +50,10 @@ def root_create_ceph_conf(monitor_ips, monitor_ports, cluster_name, user, get_rbd_class() return RBDConnector._create_ceph_conf(monitor_ips, monitor_ports, cluster_name, user, keyring) + + +@os_brick.privileged.default.entrypoint +def check_valid_path(path): + get_rbd_class() + with open(path, 'rb') as rbd_handle: + return RBDConnector._check_valid_device(rbd_handle) diff --git a/os_brick/tests/initiator/connectors/test_rbd.py b/os_brick/tests/initiator/connectors/test_rbd.py index 64ebfb710..ed807801c 100644 --- a/os_brick/tests/initiator/connectors/test_rbd.py +++ b/os_brick/tests/initiator/connectors/test_rbd.py @@ -458,3 +458,58 @@ class RBDConnectorTestCase(test_connector.ConnectorTestCase): 'rbd', 'showmapped', '--format=json', mock.sentinel.rbd_args, root_helper=connector._root_helper, run_as_root=True) self.assertEqual('/dev/rbd1', res) + + @mock.patch.object(rbd.RBDConnector, '_check_valid_device') + @mock.patch('os_brick.privileged.rbd.check_valid_path') + @mock.patch.object(rbd, 'open') + def test_check_valid_device_handle_no_path(self, mock_open, check_path, + check_device): + connector = rbd.RBDConnector(None) + res = connector.check_valid_device(None) + + self.assertFalse(res) + mock_open.assert_not_called() + check_path.assert_not_called() + check_device.assert_not_called() + + @ddt.data(True, False) + @mock.patch.object(rbd.RBDConnector, '_check_valid_device') + @mock.patch('os_brick.privileged.rbd.check_valid_path') + @mock.patch.object(rbd, 'open') + def test_check_valid_device_handle(self, run_as_root, mock_open, + check_path, check_device): + connector = rbd.RBDConnector(None) + res = connector.check_valid_device(mock.sentinel.handle, + run_as_root=run_as_root) + check_device.assert_called_once_with(mock.sentinel.handle) + self.assertIs(check_device.return_value, res) + mock_open.assert_not_called() + check_path.assert_not_called() + + @mock.patch.object(rbd.RBDConnector, '_check_valid_device') + @mock.patch('os_brick.privileged.rbd.check_valid_path') + @mock.patch.object(rbd, 'open') + def test_check_valid_device_block_root(self, mock_open, check_path, + check_device): + connector = rbd.RBDConnector(None) + path = '/dev/rbd0' + res = connector.check_valid_device(path, run_as_root=True) + + check_path.assert_called_once_with(path) + self.assertEqual(check_path.return_value, res) + mock_open.assert_not_called() + check_device.assert_not_called() + + @mock.patch.object(rbd.RBDConnector, '_check_valid_device') + @mock.patch('os_brick.privileged.rbd.check_valid_path') + @mock.patch.object(rbd, 'open') + def test_check_valid_device_block_non_root(self, mock_open, check_path, + check_device): + connector = rbd.RBDConnector(None) + path = '/dev/rbd0' + res = connector.check_valid_device(path, run_as_root=False) + + mock_open.assert_called_once_with(path, 'rb') + check_device.assert_called_once_with(mock_open().__enter__()) + self.assertIs(check_device.return_value, res) + check_path.assert_not_called() diff --git a/os_brick/tests/privileged/test_rbd.py b/os_brick/tests/privileged/test_rbd.py index dda4a58e4..c79133f7a 100644 --- a/os_brick/tests/privileged/test_rbd.py +++ b/os_brick/tests/privileged/test_rbd.py @@ -65,3 +65,15 @@ class PrivRBDTestCase(base.TestCase): mock_connector._create_ceph_conf.assert_called_once_with( s.monitor_ips, s.monitor_ports, s.cluster_name, s.user, s.keyring) self.assertIs(mock_connector._create_ceph_conf.return_value, res) + + @mock.patch.object(privsep_rbd, 'get_rbd_class') + @mock.patch.object(privsep_rbd, 'open') + @mock.patch.object(privsep_rbd, 'RBDConnector') + def test_check_valid_path(self, mock_connector, mock_open, mock_get_class): + res = privsep_rbd.check_valid_path(mock.sentinel.path) + + mock_get_class.assert_called_once_with() + mock_open.assert_called_once_with(mock.sentinel.path, 'rb') + mock_connector._check_valid_device.assert_called_once_with( + mock_open.return_value.__enter__.return_value) + self.assertEqual(mock_connector._check_valid_device.return_value, res) diff --git a/releasenotes/notes/rbd_check_valid_device-2f50c0639adb8e7c.yaml b/releasenotes/notes/rbd_check_valid_device-2f50c0639adb8e7c.yaml new file mode 100644 index 000000000..dc1434907 --- /dev/null +++ b/releasenotes/notes/rbd_check_valid_device-2f50c0639adb8e7c.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fix check_valid_path method in the RBD connector for locally attached + volumes. (Bug #1884552)