NVMe-oF: Return right nqn when missing hostnqn
This patch fixes a problem where we don't return the right nqn value when the /etc/nvme/hostnqn file doesn't exist. Currently get_connector_properties returns a new nqn value every time it gets called if the hostnqn file is not present in the system. The reason for this is that we are calling "nvme gen-hostnqn" and piping it to tee, but for some reason tee is failing to write the file, so on each call we'll generate a new nqn. Even if we were successfully creating the hostnqn file, that could create problems if we have already attached some volumes, since those would have used the existing nqn. This patch replaces the series of command executions (mkdir, nvme, chmod) with a privsep method that then leverages the equivalent python methods when possible and also tries to use the show-hostnqn nvme subcommand first to get the existing nqn before defaulting to generating a new one with gen-hostnqn. This way we don't replace the current nqn value that may be already being used by other nvme connections. Subcommand show-hostnqn returns the host NQN configured for the system. First looks for /etc/nvme/hostnqn, if it's not present (our case when we call the new create_hostnqn method) tries to construct it from dmi (/sys/firmware/dmi/entries) or from systemd's application-specific machine IDs for the system [1]. It's important to differentiate between the ENOENT returned as OSError and the same value returned by nvme [2] when show-hostnqn cannot return anything. [1]:ed9538622a/fabrics.c (L858-L875)
[2]:5b8b065b1d/nvme.c (L5642-L5643)
Closes-Bug: #1928944 Change-Id: I252dd958767dcdd4f9a2767b362aaf675edb79c4
This commit is contained in:
parent
7d3af72cc2
commit
37d57c4306
|
@ -28,6 +28,7 @@ try:
|
|||
from os_brick.initiator.connectors import nvmeof_agent
|
||||
except ImportError:
|
||||
nvmeof_agent = None
|
||||
from os_brick.privileged import nvmeof as priv_nvme
|
||||
from os_brick.privileged import rootwrap as priv_rootwrap
|
||||
from os_brick import utils
|
||||
|
||||
|
@ -128,26 +129,13 @@ class NVMeOFConnector(base.BaseLinuxConnector):
|
|||
return None
|
||||
|
||||
def _get_host_nqn(self):
|
||||
host_nqn = None
|
||||
try:
|
||||
with open('/etc/nvme/hostnqn', 'r') as f:
|
||||
host_nqn = f.read().strip()
|
||||
f.close()
|
||||
except IOError:
|
||||
try:
|
||||
self._execute(
|
||||
'mkdir', '-m', '755', '-p', '/etc/nvme',
|
||||
root_helper=self._root_helper, run_as_root=True)
|
||||
out, err = self._execute(
|
||||
'nvme', 'gen-hostnqn', '|', 'tee', '/etc/nvme/hostnqn',
|
||||
root_helper=self._root_helper, run_as_root=True)
|
||||
if out.strip():
|
||||
host_nqn = out.strip()
|
||||
self._execute(
|
||||
'chmod', '644', '/etc/nvme/hostnqn',
|
||||
root_helper=self._root_helper, run_as_root=True)
|
||||
except Exception as e:
|
||||
LOG.warning("Could not generate host nqn: %s" % str(e))
|
||||
host_nqn = priv_nvme.create_hostnqn()
|
||||
except Exception:
|
||||
host_nqn = None
|
||||
return host_nqn
|
||||
|
||||
def _get_system_uuid(self):
|
||||
|
|
|
@ -0,0 +1,66 @@
|
|||
# Copyright (c) 2021, Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may
|
||||
# not use this file except in compliance with the License. You may obtain
|
||||
# a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
|
||||
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
|
||||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
|
||||
import errno
|
||||
import os
|
||||
|
||||
from oslo_concurrency import processutils as putils
|
||||
from oslo_log import log as logging
|
||||
|
||||
import os_brick.privileged
|
||||
from os_brick.privileged import rootwrap
|
||||
|
||||
|
||||
LOG = logging.getLogger(__name__)
|
||||
|
||||
|
||||
@os_brick.privileged.default.entrypoint
|
||||
def create_hostnqn():
|
||||
"""Create the hostnqn file to speed up finding out the nqn.
|
||||
|
||||
By having the /etc/nvme/hostnqn not only do we make sure that that value is
|
||||
always used on this system, but we are also able to just open the file to
|
||||
get the nqn on each get_connector_properties call instead of having to make
|
||||
a call to nvme show-hostnqn command.
|
||||
"""
|
||||
host_nqn = ''
|
||||
try:
|
||||
os.makedirs('/etc/nvme', mode=0o755, exist_ok=True)
|
||||
|
||||
# Try to get existing nqn generated from dmi or systemd
|
||||
try:
|
||||
host_nqn, err = rootwrap.custom_execute('nvme', 'show-hostnqn')
|
||||
host_nqn = host_nqn.strip()
|
||||
|
||||
# This is different from OSError's ENOENT, which is missing nvme
|
||||
# command. This ENOENT is when nvme says there isn't an nqn.
|
||||
except putils.ProcessExecutionError as e:
|
||||
if e.exit_code != errno.ENOENT:
|
||||
raise
|
||||
LOG.debug('No nqn could be formed from dmi or systemd.')
|
||||
|
||||
if not host_nqn:
|
||||
LOG.debug('Generating nqn')
|
||||
host_nqn, err = rootwrap.custom_execute('nvme', 'gen-hostnqn')
|
||||
host_nqn = host_nqn.strip()
|
||||
|
||||
with open('/etc/nvme/hostnqn', 'w') as f:
|
||||
LOG.debug('Writing hostnqn file')
|
||||
f.write(host_nqn)
|
||||
os.chmod('/etc/nvme/hostnqn', 0o644)
|
||||
except Exception as e:
|
||||
LOG.warning("Could not generate host nqn: %s" % str(e))
|
||||
|
||||
return host_nqn
|
|
@ -882,14 +882,26 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase):
|
|||
mock_open.return_value.__enter__.return_value.read = (
|
||||
lambda: HOST_NQN + "\n")
|
||||
host_nqn = self._get_host_nqn()
|
||||
self.assertEqual(host_nqn, HOST_NQN)
|
||||
mock_open.assert_called_once_with('/etc/nvme/hostnqn', 'r')
|
||||
self.assertEqual(HOST_NQN, host_nqn)
|
||||
|
||||
@mock.patch.object(executor.Executor, '_execute')
|
||||
@mock.patch.object(nvmeof.priv_nvme, 'create_hostnqn')
|
||||
@mock.patch.object(builtins, 'open')
|
||||
def test_get_host_nqn_err(self, mock_open, mock_execute):
|
||||
mock_execute.side_effect = Exception()
|
||||
def test_get_host_nqn_io_err(self, mock_open, mock_create):
|
||||
mock_create.return_value = mock.sentinel.nqn
|
||||
mock_open.side_effect = IOError()
|
||||
result = self.connector._get_host_nqn()
|
||||
mock_open.assert_called_once_with('/etc/nvme/hostnqn', 'r')
|
||||
mock_create.assert_called_once_with()
|
||||
self.assertEqual(mock.sentinel.nqn, result)
|
||||
|
||||
@mock.patch.object(nvmeof.priv_nvme, 'create_hostnqn')
|
||||
@mock.patch.object(builtins, 'open')
|
||||
def test_get_host_nqn_err(self, mock_open, mock_create):
|
||||
mock_open.side_effect = Exception()
|
||||
result = self.connector._get_host_nqn()
|
||||
mock_open.assert_called_once_with('/etc/nvme/hostnqn', 'r')
|
||||
mock_create.assert_not_called()
|
||||
self.assertIsNone(result)
|
||||
|
||||
@mock.patch.object(executor.Executor, '_execute')
|
||||
|
|
|
@ -0,0 +1,97 @@
|
|||
# Copyright (c) 2021, Red Hat, Inc.
|
||||
# All Rights Reserved.
|
||||
#
|
||||
# Licensed under the Apache License, Version 2.0 (the "License"); you may
|
||||
# not use this file except in compliance with the License. You may obtain
|
||||
# a copy of the License at
|
||||
#
|
||||
# http://www.apache.org/licenses/LICENSE-2.0
|
||||
#
|
||||
# Unless required by applicable law or agreed to in writing, software
|
||||
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
|
||||
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
|
||||
# License for the specific language governing permissions and limitations
|
||||
# under the License.
|
||||
import builtins
|
||||
import errno
|
||||
from unittest import mock
|
||||
|
||||
import ddt
|
||||
from oslo_concurrency import processutils as putils
|
||||
|
||||
import os_brick.privileged as privsep_brick
|
||||
import os_brick.privileged.nvmeof as privsep_nvme
|
||||
from os_brick.privileged import rootwrap
|
||||
from os_brick.tests import base
|
||||
|
||||
|
||||
@ddt.ddt
|
||||
class PrivNVMeTestCase(base.TestCase):
|
||||
def setUp(self):
|
||||
super(PrivNVMeTestCase, self).setUp()
|
||||
|
||||
# Disable privsep server/client mode
|
||||
privsep_brick.default.set_client_mode(False)
|
||||
self.addCleanup(privsep_brick.default.set_client_mode, True)
|
||||
|
||||
@mock.patch('os.chmod')
|
||||
@mock.patch.object(builtins, 'open', new_callable=mock.mock_open)
|
||||
@mock.patch('os.makedirs')
|
||||
@mock.patch.object(rootwrap, 'custom_execute')
|
||||
def test_create_hostnqn(self, mock_exec, mock_mkdirs, mock_open,
|
||||
mock_chmod):
|
||||
hostnqn = mock.Mock()
|
||||
mock_exec.return_value = (hostnqn, mock.sentinel.err)
|
||||
|
||||
res = privsep_nvme.create_hostnqn()
|
||||
|
||||
mock_mkdirs.assert_called_once_with('/etc/nvme',
|
||||
mode=0o755,
|
||||
exist_ok=True)
|
||||
mock_exec.assert_called_once_with('nvme', 'show-hostnqn')
|
||||
mock_open.assert_called_once_with('/etc/nvme/hostnqn', 'w')
|
||||
stripped_hostnqn = hostnqn.strip.return_value
|
||||
mock_open().write.assert_called_once_with(stripped_hostnqn)
|
||||
mock_chmod.assert_called_once_with('/etc/nvme/hostnqn', 0o644)
|
||||
self.assertEqual(stripped_hostnqn, res)
|
||||
|
||||
@mock.patch('os.chmod')
|
||||
@mock.patch.object(builtins, 'open', new_callable=mock.mock_open)
|
||||
@mock.patch('os.makedirs')
|
||||
@mock.patch.object(rootwrap, 'custom_execute')
|
||||
def test_create_hostnqn_generate(self, mock_exec, mock_mkdirs, mock_open,
|
||||
mock_chmod):
|
||||
hostnqn = mock.Mock()
|
||||
mock_exec.side_effect = [
|
||||
putils.ProcessExecutionError(exit_code=errno.ENOENT),
|
||||
(hostnqn, mock.sentinel.err)
|
||||
]
|
||||
|
||||
res = privsep_nvme.create_hostnqn()
|
||||
|
||||
mock_mkdirs.assert_called_once_with('/etc/nvme',
|
||||
mode=0o755,
|
||||
exist_ok=True)
|
||||
self.assertEqual(2, mock_exec.call_count)
|
||||
mock_exec.assert_has_calls([mock.call('nvme', 'show-hostnqn'),
|
||||
mock.call('nvme', 'gen-hostnqn')])
|
||||
|
||||
mock_open.assert_called_once_with('/etc/nvme/hostnqn', 'w')
|
||||
stripped_hostnqn = hostnqn.strip.return_value
|
||||
mock_open().write.assert_called_once_with(stripped_hostnqn)
|
||||
mock_chmod.assert_called_once_with('/etc/nvme/hostnqn', 0o644)
|
||||
self.assertEqual(stripped_hostnqn, res)
|
||||
|
||||
@ddt.data(OSError(errno.ENOENT), # nvme not present in system
|
||||
putils.ProcessExecutionError(exit_code=123)) # nvme error
|
||||
@mock.patch('os.makedirs')
|
||||
@mock.patch.object(rootwrap, 'custom_execute')
|
||||
def test_create_hostnqn_nvme_not_present(self, exception,
|
||||
mock_exec, mock_mkdirs):
|
||||
mock_exec.side_effect = exception
|
||||
res = privsep_nvme.create_hostnqn()
|
||||
mock_mkdirs.assert_called_once_with('/etc/nvme',
|
||||
mode=0o755,
|
||||
exist_ok=True)
|
||||
mock_exec.assert_called_once_with('nvme', 'show-hostnqn')
|
||||
self.assertEqual('', res)
|
|
@ -0,0 +1,7 @@
|
|||
---
|
||||
fixes:
|
||||
- |
|
||||
NVMe-oF connector `bug #1928944
|
||||
<https://bugs.launchpad.net/os-brick/+bug/1928944>`_: Fixed not returning
|
||||
the right nqn value on ``get_connector_properties`` when
|
||||
``/etc/nvme/hostnqn`` doesn't exist.
|
Loading…
Reference in New Issue