Merge pull request #1187 from rackerlabs/fix-cinder

fix(cinder-understack): correct cleanup and error handling to not be the problem

Author: cardoe

python/cinder-understack/cinder_understack/dynamic_netapp_driver.py

@@ -233,18 +233,12 @@ def do_setup(self, ctxt):
             svm_lib.do_setup(ctxt)
             self._libraries[svm_name] = svm_lib
 
-    def _remove_svm_lib(self, svm_name: str, svm_lib: NetAppMinimalLibrary):
+    def _remove_svm_lib(self, svm_lib: NetAppMinimalLibrary):
         """Remove resources for a given SVM library."""
         # TODO: Need to free up resources here.
-        LOG.info("Removing resources for SVM library %s", svm_name)
-        # Stop any looping calls if they exist
-        if svm_lib._looping_call and hasattr(svm_lib, "loopingcalls"):
-            LOG.info("Stopping looping call for SVM library %s", svm_name)
-            svm_lib.loopingcalls.stop_tasks()
-            # Adding None coz it has a reference to the looping call
-            svm_lib.looping_call = None
-            # There are other attributes which are in svm_lib even after
-            # Stopping the looping.
+        for task in svm_lib.loopingcalls.tasks:
+            task.looping_call.stop()
+        svm_lib.loopingcalls.tasks = []
 
     def _refresh_svm_libraries(self):
         return self._actual_refresh_svm_libraries(context.get_admin_context())
@@ -262,17 +256,16 @@ def _actual_refresh_svm_libraries(self, ctxt):
         stale_svms = existing_libs - current_svms
         for svm_name in stale_svms:
             LOG.info("Removing stale NVMe library for SVM: %s", svm_name)
-            # TODO : stop looping calls, free resources.
-            svm_lib = self._libraries.get(svm_name)
-            self._remove_svm_lib(svm_name, svm_lib)
+            svm_lib = self._libraries[svm_name]
+            self._remove_svm_lib(svm_lib)
             del self._libraries[svm_name]
 
         # Add new SVM libraries
         new_svms = current_svms - existing_libs
         for svm_name in new_svms:
             LOG.info("Creating NVMe library for new SVM: %s", svm_name)
+            lib = self._create_svm_lib(svm_name)
             try:
-                lib = self._create_svm_lib(svm_name)
                 # Call do_setup to initialize the library
                 lib.do_setup(ctxt)
                 lib.check_for_setup_error()
@@ -283,13 +276,21 @@ def _actual_refresh_svm_libraries(self, ctxt):
                     "Failed to create library for SVM %s",
                     svm_name,
                 )
+                self._remove_svm_lib(lib)
         LOG.info("Final libraries loaded: %s", list(self._libraries.keys()))
 
     def check_for_setup_error(self):
         """Check for setup errors."""
-        for svm_name, svm_lib in self._libraries.items():
+        svm_to_init = set(self._libraries.keys())
+        for svm_name in svm_to_init:
             LOG.info("Checking NVMe library for errors for SVM %s", svm_name)
-            svm_lib.check_for_setup_error()
+            svm_lib = self._libraries[svm_name]
+            try:
+                svm_lib.check_for_setup_error()
+            except Exception:
+                LOG.exception("Failed to initialize SVM %s, skipping", svm_name)
+                self._remove_svm_lib(svm_lib)
+                del self._libraries[svm_name]
 
         # looping call to refresh SVM libraries
         if not self._looping_call:

python/cinder-understack/cinder_understack/tests/test_dynamic_netapp_driver.py

@@ -10,10 +10,20 @@
 from cinder.tests.unit import utils as test_utils
 from cinder.tests.unit.volume.drivers.netapp import fakes as na_fakes
 from cinder.volume.drivers.netapp.dataontap.nvme_library import NetAppNVMeStorageLibrary
+from cinder.volume.drivers.netapp.dataontap.utils import loopingcalls
 
 from cinder_understack import dynamic_netapp_driver
 
 
+def _create_mock_svm_lib(svm_name: str):
+    mock_lib = mock.create_autospec(
+        dynamic_netapp_driver.NetAppMinimalLibrary, instance=True
+    )
+    mock_lib.vserver = svm_name
+    mock_lib.loopingcalls = loopingcalls.LoopingCalls()
+    return mock_lib
+
+
 class NetappDynamicDriverTestCase(test.TestCase):
     """Test case for NetappCinderDynamicDriver."""
 
@@ -152,21 +162,16 @@ def test_refresh_svm_libraries_adds_and_removes_svms(
         expected_svm = f"os-{self.project_id}"
 
         self.driver._libraries = {
-            "os-old-svm": mock.Mock(vserver="os-old-svm"),
-            expected_svm: mock.Mock(vserver=expected_svm),
+            "os-old-svm": _create_mock_svm_lib("os-old-svm"),
+            expected_svm: _create_mock_svm_lib(expected_svm),
         }
 
         self.driver._get_svms = mock_get_svms
         # Returned by _get_svms (after refresh)
         mock_get_svms.return_value = [expected_svm, "os-new-svm"]
 
         # make the created lib look like the real thing
-        mock_lib_instance = mock.create_autospec(
-            dynamic_netapp_driver.NetAppMinimalLibrary, instance=True
-        )
-
-        # Mock the new lib instance created
-        mock_lib_instance.vserver = "os-new-svm"
+        mock_lib_instance = _create_mock_svm_lib("os-new-svm")
         mock_create_svm_lib.return_value = mock_lib_instance
 
         # Trigger refresh
@@ -196,8 +201,11 @@ def test_refresh_svm_libraries_handles_lib_creation_failure(
         self, mock_get_svms, mock_create_svm_lib
     ):
         """Ensure that failure in lib creation is caught and logged, not raised."""
-        mock_get_svms.return_value = ["os-new-failing-svm"]
-        mock_create_svm_lib.side_effect = Exception("Simulated failure")
+        test_svm_name = "os-new-failing_svm"
+        mock_get_svms.return_value = [test_svm_name]
+        mock_svm_lib = _create_mock_svm_lib(test_svm_name)
+        mock_svm_lib.check_for_setup_error.side_effect = Exception("Simulated failure")
+        mock_create_svm_lib.return_value = mock_svm_lib
 
         self.driver._libraries = {}