RBD: Default rbd_secret_uuid to the cluster FSID

Currently the `rbd_secret_uuid` configuration option has no default
value, but at the same time the RBD driver doesn't complain if it's
missing, and will only fail on the Nova side if there is no
`rbd_secret_uuid` configured in the `[LIBVIRT]` section of `nova.conf`.

Using Cinder's `rbd_secret_uuid` has been the preferred way since the
Ocata release.

Most deployments set the `rbd_secret_uuid` value in libvirt's secret to
the Ceph cluster FSID value, and that's what this patch does when it is
not defined.

It doesn't change how replication locations are configured, and those
will still require the secret uuid to be defined.

Change-Id: I739ae6ae5b4d9b074d610f6a70371b294a3e70f8

cinder/tests/unit/volume/drivers/test_rbd.py

@@ -19,6 +19,7 @@ import math
 import os
 import tempfile
 import time
+import types
 from unittest import mock
 from unittest.mock import call
 import uuid
@@ -176,14 +177,14 @@ class MockDriverConfig(object):
 @ddt.ddt
 class RBDTestCase(test.TestCase):
 
-    @staticmethod
-    def _make_configuration(conf_in=None):
+    @classmethod
+    def _make_configuration(cls, conf_in=None):
         cfg = mock.Mock(spec=conf.Configuration)
         cfg.image_conversion_dir = None
         cfg.rbd_cluster_name = 'nondefault'
         cfg.rbd_pool = 'rbd'
         cfg.rbd_ceph_conf = '/etc/ceph/my_ceph.conf'
-        cfg.rbd_secret_uuid = None
+        cfg.rbd_secret_uuid = '5fe62cc7-0392-4a32-8466-081ce0ea970f'
         cfg.rbd_user = 'cinder'
         cfg.volume_backend_name = None
         cfg.volume_dd_blocksize = '1M'
@@ -193,12 +194,22 @@ class RBDTestCase(test.TestCase):
         cfg.backup_use_temp_snapshot = False
         cfg.enable_deferred_deletion = False
 
+        # Because the mocked conf doesn't actually have an underlying oslo conf
+        # it doesn't have the set_default method, so we use a fake one.
+        cfg.set_default = types.MethodType(cls._set_default, cfg)
+
         if conf_in is not None:
             for k in conf_in:
                 setattr(cfg, k, conf_in[k])
 
         return cfg
 
+    @staticmethod
+    def _set_default(cfg, name, value, group=None):
+        # Ignore the group for now
+        if not getattr(cfg, name):
+            setattr(cfg, name, value)
+
     @staticmethod
     def _make_drv(conf_in):
         cfg = RBDTestCase._make_configuration(conf_in)
@@ -337,8 +348,11 @@ class RBDTestCase(test.TestCase):
 
     def test_do_setup_replication_disabled(self):
         with mock.patch.object(self.driver.configuration, 'safe_get',
-                               return_value=None):
+                               return_value=None), \
+                mock.patch.object(self.driver,
+                                  '_set_default_secret_uuid') as mock_secret:
             self.driver.do_setup(self.context)
+            mock_secret.assert_called_once_with()
             self.assertFalse(self.driver._is_replication_enabled)
             self.assertEqual([], self.driver._replication_targets)
             self.assertEqual([], self.driver._target_names)
@@ -348,6 +362,36 @@ class RBDTestCase(test.TestCase):
                               'secret_uuid': self.cfg.rbd_secret_uuid},
                              self.driver._active_config)
 
+    @ddt.data('', None)
+    @mock.patch.object(driver.RBDDriver, '_get_fsid')
+    def test__set_default_secret_uuid_missing(self, secret_uuid, mock_fsid):
+        # Clear the current values
+        self.cfg.rbd_secret_uuid = secret_uuid
+        self.driver._active_config['secret_uuid'] = secret_uuid
+        # Fake fsid value returned by the cluster
+        fsid = str(uuid.uuid4())
+        mock_fsid.return_value = fsid
+
+        self.driver._set_default_secret_uuid()
+
+        mock_fsid.assert_called_once_with()
+        self.assertEqual(fsid, self.driver._active_config['secret_uuid'])
+        self.assertEqual(fsid, self.cfg.rbd_secret_uuid)
+
+    @mock.patch.object(driver.RBDDriver, '_get_fsid')
+    def test__set_default_secret_uuid_present(self, mock_fsid):
+        # Set secret_uuid like _get_target_config does on do_setup
+        secret_uuid = self.cfg.rbd_secret_uuid
+        self.driver._active_config['secret_uuid'] = secret_uuid
+        # Fake fsid value returned by the cluster (should not be callled)
+        mock_fsid.return_value = str(uuid.uuid4())
+        self.driver._set_default_secret_uuid()
+        mock_fsid.assert_not_called()
+        # Values must not have changed
+        self.assertEqual(secret_uuid,
+                         self.driver._active_config['secret_uuid'])
+        self.assertEqual(secret_uuid, self.cfg.rbd_secret_uuid)
+
     def test_do_setup_replication(self):
         cfg = [{'backend_id': 'secondary-backend',
                 'conf': 'foo',
@@ -1983,7 +2027,8 @@ class RBDTestCase(test.TestCase):
 
         self.driver._active_config = {'name': 'secondary_id',
                                       'user': 'foo',
-                                      'conf': 'bar'}
+                                      'conf': 'bar',
+                                      'secret_uuid': self.cfg.rbd_secret_uuid}
         expected = {
             'driver_volume_type': 'rbd',
             'data': {

cinder/volume/drivers/rbd.py

@@ -81,7 +81,7 @@ RBD_OPTS = [
                      'dependency from volume to snapshot'),
     cfg.StrOpt('rbd_secret_uuid',
                help='The libvirt uuid of the secret for the rbd_user '
-                    'volumes'),
+                    'volumes. Defaults to the cluster FSID.'),
     cfg.IntOpt('rbd_max_clone_depth',
                default=5,
                help='Maximum number of nested volume clones that are '
@@ -424,6 +424,17 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD,
         """Performs initialization steps that could raise exceptions."""
         self._do_setup_replication()
         self._active_config = self._get_target_config(self._active_backend_id)
+        self._set_default_secret_uuid()
+
+    def _set_default_secret_uuid(self):
+        # Set secret_uuid to the cluster FSID if missing, should only happen
+        # with the primary/default configuration
+        if not self._active_config['secret_uuid']:
+            # self._active_config must be set before this call
+            fsid = self._get_fsid()
+            self._active_config['secret_uuid'] = fsid
+            LOG.info('Secret UUID defaulting to cluster FSID: %s', fsid)
+            self.configuration.set_default('rbd_secret_uuid', fsid)
 
     def _do_setup_replication(self) -> None:
         replication_devices = self.configuration.safe_get(
@@ -1719,6 +1730,7 @@ class RBDDriver(driver.CloneableImageVD, driver.MigrateVD,
 
         self._active_backend_id = secondary_id
         self._active_config = remote
+        self._set_default_secret_uuid()
         LOG.info('RBD driver failover completion completed.')
 
     def failover_host(self,

releasenotes/notes/rbd-rbd_secret_uuid-fsid-95daee128f59c8e4.yaml

@@ -0,0 +1,5 @@
+---
+features:
+  - |
+    RBD driver: Sets the Ceph cluster FSID as the default value for the
+    ``rbd_secret_uuid`` configuration option.