NVMe-oF: Create /etc/nvme/hostid

The NVMe-oF connector currently create the `/etc/nvme/hostnqn` file if
it doesn't exist, but it may still be missing the `/etc/nvme/hostid`
value.

Some distribution packages create the file on installation but others
may not.

It is recommended for the file to be present so that nvme doesn't
randomly generate it.

Randomly generating it means that the value will be different for the
same storage array and the same volume if we connect, disconnect, and
connect the volume again.

This patch ensures that the file will exist and will try to use the
system's UUID as reported by DMI or a randomly generated one.

BACKPORT NOTE: This patch also includes another patch fixing unittests
               Otherwise tests would fail

  Fix unit tests when hostid file exists

  After merging change I0b60f9078f23f8464d8234841645ed520e8ba655, we
  noticed an issue with existing unit tests which started failing.
  The reason is 'nvme_hostid' was an additional parameter returned
  in the response while fetching connector properties from nvme
  connector.
  This is environment specific and won't occur in environments where
  '/etc/nvme/hostid' file doesn't exist due to which these tests
  passed in gate but failed in the local run when hostid file
  was present.
  This patch mocks the get_nvme_host_id method for tests so the
  hostid is never returned irrespective of the environment.

  Closes-Bug: #2032941
  Change-Id: I8b1aaedfdb9bef6e34813e39dede9afe98371d2b
  (cherry picked from commit 71627c56ac)

Closes-Bug: #2016029
Change-Id: I0b60f9078f23f8464d8234841645ed520e8ba655
(cherry picked from commit 16c90d5fe9)

os_brick/initiator/connectors/nvmeof.py

@@ -24,7 +24,6 @@ import time
 from typing import (Callable, Optional, Sequence, Type, Union)  # noqa: H301
 import uuid as uuid_lib
 
-
 from oslo_concurrency import processutils as putils
 from oslo_log import log as logging
 
@@ -768,10 +767,16 @@ class NVMeOFConnector(base.BaseLinuxConnector):
         ret = {}
 
         nqn = None
+        hostid = None
         uuid = nvmf._get_host_uuid()
         suuid = priv_nvmeof.get_system_uuid()
         if cls.nvme_present():
             nqn = utils.get_host_nqn()
+            # Ensure /etc/nvme/hostid exists and defaults to the system uuid,
+            # or a random value.
+            hostid = utils.get_nvme_host_id(suuid)
+        if hostid:
+            ret['nvme_hostid'] = hostid
         if uuid:
             ret['uuid'] = uuid
         if suuid:

os_brick/privileged/nvmeof.py

@@ -13,8 +13,11 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+from __future__ import annotations
+
 import errno
 import os
+from typing import Optional  # noqa: H301
 
 from oslo_concurrency import processutils as putils
 from oslo_log import log as logging
@@ -93,3 +96,21 @@ def get_system_uuid() -> str:
         out = ""
 
     return out.strip()
+
+
+@os_brick.privileged.default.entrypoint
+def create_hostid(uuid: str) -> Optional[str]:
+    """Create the hostid to ensure it's always the same."""
+    try:
+        os.makedirs('/etc/nvme', mode=0o755, exist_ok=True)
+
+        with open('/etc/nvme/hostid', 'w') as f:
+            LOG.debug('Writing nvme hostid %s', uuid)
+            f.write(f'{uuid}\n')
+        os.chmod('/etc/nvme/hostid', 0o644)
+
+    except Exception as e:
+        LOG.warning("Could not generate nvme host id: %s", e)
+        return None
+
+    return uuid

os_brick/tests/initiator/connectors/test_nvmeof.py

@@ -921,6 +921,8 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase):
         uuid = self.connector._get_host_uuid()
         self.assertIsNone(uuid)
 
+    @mock.patch.object(utils, 'get_nvme_host_id',
+                       return_value=SYS_UUID)
     @mock.patch.object(nvmeof.NVMeOFConnector,
                        '_is_native_multipath_supported',
                        return_value=True)
@@ -935,11 +937,17 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase):
     def test_get_connector_properties_without_sysuuid(self, mock_host_uuid,
                                                       mock_sysuuid, mock_nqn,
                                                       mock_nvme_present,
-                                                      mock_nat_mpath_support):
+                                                      mock_nat_mpath_support,
+                                                      mock_get_host_id):
         props = self.connector.get_connector_properties('sudo')
-        expected_props = {'nqn': 'fakenqn', 'nvme_native_multipath': False}
+        expected_props = {'nqn': 'fakenqn',
+                          'nvme_native_multipath': False,
+                          'nvme_hostid': SYS_UUID}
         self.assertEqual(expected_props, props)
+        mock_get_host_id.assert_called_once_with(None)
 
+    @mock.patch.object(utils, 'get_nvme_host_id',
+                       return_value=SYS_UUID)
     @mock.patch.object(nvmeof.NVMeOFConnector,
                        '_is_native_multipath_supported',
                        return_value=True)
@@ -951,15 +959,18 @@ class NVMeOFConnectorTestCase(test_connector.ConnectorTestCase):
     def test_get_connector_properties_with_sysuuid(self, mock_host_uuid,
                                                    mock_sysuuid, mock_nqn,
                                                    mock_nvme_present,
-                                                   mock_native_mpath_support):
+                                                   mock_native_mpath_support,
+                                                   mock_get_host_id):
         mock_host_uuid.return_value = HOST_UUID
         mock_sysuuid.return_value = SYS_UUID
         mock_nqn.return_value = HOST_NQN
         mock_nvme_present.return_value = True
         props = self.connector.get_connector_properties('sudo')
         expected_props = {"system uuid": SYS_UUID, "nqn": HOST_NQN,
-                          "uuid": HOST_UUID, 'nvme_native_multipath': False}
+                          "uuid": HOST_UUID, 'nvme_native_multipath': False,
+                          'nvme_hostid': SYS_UUID}
         self.assertEqual(expected_props, props)
+        mock_get_host_id.assert_called_once_with(SYS_UUID)
 
     def test_get_volume_paths_device_info(self):
         """Device info path has highest priority."""

os_brick/tests/initiator/test_connector.py

@@ -60,6 +60,7 @@ class ConnectorUtilsTestCase(test_base.TestCase):
                        return_value=None)
     @mock.patch.object(platform, 'machine', mock.Mock(return_value='s390x'))
     @mock.patch('sys.platform', 'linux2')
+    @mock.patch.object(utils, 'get_nvme_host_id', mock.Mock(return_value=None))
     def _test_brick_get_connector_properties(self, multipath,
                                              enforce_multipath,
                                              multipath_result,

os_brick/tests/privileged/test_nvmeof.py

@@ -126,6 +126,35 @@ class PrivNVMeTestCase(base.TestCase):
         mock_exec.assert_called_once_with('nvme', 'show-hostnqn')
         self.assertEqual('', res)
 
+    @mock.patch('os.chmod')
+    @mock.patch.object(builtins, 'open', new_callable=mock.mock_open)
+    @mock.patch('os.makedirs')
+    def test_create_hostid(self, mock_mkdirs, mock_open, mock_chmod):
+        res = privsep_nvme.create_hostid('uuid')
+
+        mock_mkdirs.assert_called_once_with('/etc/nvme',
+                                            mode=0o755,
+                                            exist_ok=True)
+        mock_open.assert_called_once_with('/etc/nvme/hostid', 'w')
+        mock_open().write.assert_called_once_with('uuid\n')
+        mock_chmod.assert_called_once_with('/etc/nvme/hostid', 0o644)
+        self.assertEqual('uuid', res)
+
+    @mock.patch('os.chmod')
+    @mock.patch.object(builtins, 'open', new_callable=mock.mock_open)
+    @mock.patch('os.makedirs')
+    def test_create_hostid_fails(self, mock_mkdirs, mock_open, mock_chmod):
+        mock_mkdirs.side_effect = OSError
+
+        res = privsep_nvme.create_hostid(None)
+
+        mock_mkdirs.assert_called_once_with('/etc/nvme',
+                                            mode=0o755,
+                                            exist_ok=True)
+        mock_open.assert_not_called()
+        mock_chmod.assert_not_called()
+        self.assertIsNone(res)
+
     @mock.patch.object(builtins, 'open', new_callable=mock.mock_open)
     def test_get_system_uuid_product_uuid(self, mock_open):
         uuid = 'dbc6ba60-36ae-4b96-9310-628832bdfc3d'

os_brick/tests/test_utils.py

@@ -12,6 +12,7 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import builtins
 import functools
 import io
 import time
@@ -43,6 +44,45 @@ class TestUtils(base.TestCase):
             'blockdev', '--getsize64', device,
             run_as_root=True, root_helper=mock_execute._root_helper)
 
+    @mock.patch.object(builtins, 'open')
+    def test_get_nvme_host_id_file_available(self, mock_open):
+        mock_open.return_value.__enter__.return_value.read.return_value = (
+            'uuid\n')
+        result = utils.get_nvme_host_id(mock.sentinel.uuid)
+        mock_open.assert_called_once_with('/etc/nvme/hostid', 'r')
+        self.assertEqual('uuid', result)
+
+    @mock.patch.object(utils.priv_nvme, 'create_hostid')
+    @mock.patch.object(builtins, 'open')
+    def test_get_nvme_host_id_io_err(self, mock_open, mock_create):
+        mock_create.return_value = mock.sentinel.uuid_return
+        mock_open.side_effect = IOError()
+        result = utils.get_nvme_host_id(mock.sentinel.uuid)
+        mock_open.assert_called_once_with('/etc/nvme/hostid', 'r')
+        mock_create.assert_called_once_with(mock.sentinel.uuid)
+        self.assertEqual(mock.sentinel.uuid_return, result)
+
+    @mock.patch('uuid.uuid4')
+    @mock.patch.object(utils.priv_nvme, 'create_hostid')
+    @mock.patch.object(builtins, 'open')
+    def test_get_nvme_host_id_io_err_no_uuid(self, mock_open, mock_create,
+                                             mock_uuid):
+        mock_create.return_value = mock.sentinel.uuid_return
+        mock_open.side_effect = IOError()
+        result = utils.get_nvme_host_id(None)
+        mock_open.assert_called_once_with('/etc/nvme/hostid', 'r')
+        mock_create.assert_called_once_with(str(mock_uuid.return_value))
+        self.assertEqual(mock.sentinel.uuid_return, result)
+
+    @mock.patch.object(utils.priv_nvme, 'create_hostid')
+    @mock.patch.object(builtins, 'open')
+    def test_get_nvme_host_id_err(self, mock_open, mock_create):
+        mock_open.side_effect = Exception()
+        result = utils.get_nvme_host_id(None)
+        mock_open.assert_called_once_with('/etc/nvme/hostid', 'r')
+        mock_create.assert_not_called()
+        self.assertIsNone(result)
+
 
 class TestRetryDecorator(base.TestCase):
 

os_brick/utils.py

@@ -20,6 +20,7 @@ import logging as py_logging
 import os
 import time
 from typing import Any, Callable, Optional, Type, Union   # noqa: H301
+import uuid as uuid_lib
 
 from oslo_concurrency import processutils
 from oslo_log import log as logging
@@ -231,6 +232,23 @@ def get_host_nqn() -> Optional[str]:
     return host_nqn
 
 
+def get_nvme_host_id(uuid: Optional[str]) -> Optional[str]:
+    """Get the nvme host id
+
+    If the hostid file doesn't exist create it either with the passed uuid or
+    a random one.
+    """
+    try:
+        with open('/etc/nvme/hostid', 'r') as f:
+            host_id = f.read().strip()
+    except IOError:
+        uuid = uuid or str(uuid_lib.uuid4())
+        host_id = priv_nvme.create_hostid(uuid)
+    except Exception:
+        host_id = None
+    return host_id
+
+
 def _symlink_name_from_device_path(device_path):
     """Generate symlink absolute path for encrypted devices.
 

releasenotes/notes/nvmeof-create-hostid-15bf84ec00726fad.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - |
+    NVMe-oF `bug #2016029 <https://bugs.launchpad.net/os-brick/+bug/2016029>`_:
+    The NVMe-oF connector now creates `/etc/nvme/hostid` when it's missing from
+    the system.  That way the Host ID will persist and always be the same,
+    instead of being randomly generated.