From 3ae5ea796fd2d498a4d01e5dd328bc20c493d4a7 Mon Sep 17 00:00:00 2001
From: silvacarloss <ces.eduardo98@gmail.com>
Date: Wed, 4 May 2022 19:02:47 -0300
Subject: [PATCH] Add exception handling to share server deletion

Automatic share server deletion can happen in a scenario where two
threads try to delete the same share server at once. We weren't
preventing that exception. This change adds an exception handling
to that case.

Related-Bug: 1587835
Change-Id: Ie7b6fe33de7b3dc74e9ca48a8847911c3a006a47
---
 manila/share/manager.py            | 15 +++++-
 manila/tests/share/test_manager.py | 79 ++++++++++++++++++++++++++++++
 2 files changed, 93 insertions(+), 1 deletion(-)

diff --git a/manila/share/manager.py b/manila/share/manager.py
index 819342e6cd..da8dc639ae 100644
--- a/manila/share/manager.py
+++ b/manila/share/manager.py
@@ -3764,7 +3764,14 @@ class ShareManager(manager.SchedulerDependentManager):
                                                                 self.host,
                                                                 updated_before)
         for server in servers:
-            self.delete_share_server(ctxt, server)
+            try:
+                self.delete_share_server(ctxt, server)
+            except exception.ShareServerNotFound:
+                continue
+            except Exception:
+                LOG.exception(
+                    "Unable to delete share server %s, will retry in the next "
+                    "run.", server['id'])
 
     @periodic_task.periodic_task(
         spacing=CONF.check_for_expired_shares_in_recycle_bin_interval)
@@ -4647,6 +4654,12 @@ class ShareManager(manager.SchedulerDependentManager):
             # this method starts executing when amount of dependent shares
             # has been changed.
             server_id = share_server['id']
+            try:
+                self.db.share_server_get(
+                    context, server_id)
+            except exception.ShareServerNotFound:
+                raise
+
             shares = self.db.share_instance_get_all_by_share_server(
                 context, server_id)
 
diff --git a/manila/tests/share/test_manager.py b/manila/tests/share/test_manager.py
index 039cee1230..5e42afd15b 100644
--- a/manila/tests/share/test_manager.py
+++ b/manila/tests/share/test_manager.py
@@ -4702,6 +4702,85 @@ class ShareManagerTestCase(test.TestCase):
         with test_utils.create_temp_config_with_opts(data):
             manager.ShareManager()
 
+    def test_delete_share_server_server_not_found(self):
+        share_server = db_utils.create_share_server()
+        self.share_manager.driver.initialized = True
+
+        mock_server_get = self.mock_object(
+            self.share_manager.db, 'share_server_get',
+            mock.Mock(side_effect=exception.ShareServerNotFound(
+                share_server_id=share_server['id'])))
+
+        self.assertRaises(
+            exception.ShareServerNotFound,
+            self.share_manager.delete_share_server,
+            self.context,
+            share_server
+        )
+
+        mock_server_get.assert_called_once_with(
+            self.context, share_server['id'])
+
+    def test_delete_share_server_server_in_use(self):
+        share_server = db_utils.create_share_server()
+        share_server_shares = [db_utils.create_share()]
+        self.share_manager.driver.initialized = True
+
+        mock_server_get = self.mock_object(
+            self.share_manager.db, 'share_server_get',
+            mock.Mock(return_value=share_server))
+        mock_instances_get = self.mock_object(
+            self.share_manager.db, 'share_instance_get_all_by_share_server',
+            mock.Mock(return_value=share_server_shares))
+
+        self.assertRaises(
+            exception.ShareServerInUse,
+            self.share_manager.delete_share_server,
+            self.context,
+            share_server
+        )
+
+        mock_server_get.assert_called_once_with(
+            self.context, share_server['id'])
+        mock_instances_get.assert_called_once_with(
+            self.context, share_server['id'])
+
+    def test_delete_share_server_teardown_failure(self):
+        share_server = db_utils.create_share_server()
+        self.share_manager.driver.initialized = True
+
+        mock_server_get = self.mock_object(
+            self.share_manager.db, 'share_server_get',
+            mock.Mock(return_value=share_server))
+        mock_instances_get = self.mock_object(
+            self.share_manager.db, 'share_instance_get_all_by_share_server',
+            mock.Mock(return_value=[]))
+        mock_teardown_server = self.mock_object(
+            self.share_manager.driver, 'teardown_server',
+            mock.Mock(side_effect=exception.ShareBackendException(msg='fake')))
+        mock_server_update = self.mock_object(
+            self.share_manager.db, 'share_server_update')
+
+        self.assertRaises(
+            exception.ShareBackendException,
+            self.share_manager.delete_share_server,
+            self.context,
+            share_server
+        )
+
+        mock_server_get.assert_called_once_with(
+            self.context, share_server['id'])
+        mock_instances_get.assert_called_once_with(
+            self.context, share_server['id'])
+        mock_teardown_server.assert_called_once_with(
+            server_details=share_server['backend_details'],
+            security_services=[]
+        )
+        mock_server_update.assert_called_with(
+            self.context, share_server['id'],
+            {'status': constants.STATUS_ERROR}
+        )
+
     @mock.patch.object(db, 'share_server_get_all_unused_deletable',
                        mock.Mock())
     @mock.patch.object(manager.ShareManager, 'delete_share_server',