Browse Source

Removed custom synchronized in service_instance

Replaced using custom synchronized with
lockutils.synchronized
Removed redundant synchronized from
set_up_service_instance

Partially-implements bp setup-teardown-server-enhancements

Change-Id: I3fc6fe2eb9b7062223f4a0e49ee9bfd00ad24cd4
Yulia Portnova 4 years ago
parent
commit
33b5aacda3

+ 1
- 0
.gitignore View File

@@ -38,3 +38,4 @@ doc/build
38 38
 
39 39
 # Lock dirs and files
40 40
 service_instance_locks
41
+attach_detach_locks

+ 59
- 60
manila/share/drivers/generic.py View File

@@ -92,9 +92,6 @@ def ensure_server(f):
92 92
     return wrap
93 93
 
94 94
 
95
-synchronized = service_instance.synchronized
96
-
97
-
98 95
 class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
99 96
     """Executes commands relating to Shares."""
100 97
 
@@ -143,8 +140,6 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
143 140
         self.service_instance_manager = service_instance.\
144 141
             ServiceInstanceManager(self.db, self._helpers,
145 142
                                    driver_config=self.configuration)
146
-        self.share_networks_locks = self.service_instance_manager.\
147
-                                                          share_networks_locks
148 143
         self.share_networks_servers = self.service_instance_manager.\
149 144
                                                         share_networks_servers
150 145
         self._setup_helpers()
@@ -157,8 +152,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
157 152
             self._helpers[share_proto.upper()] = helper(
158 153
                 self._execute,
159 154
                 self._ssh_exec,
160
-                self.configuration,
161
-                self.share_networks_locks)
155
+                self.configuration)
162 156
 
163 157
     @ensure_server
164 158
     def create_share(self, context, share, share_server=None):
@@ -213,38 +207,40 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
213 207
         """
214 208
         return os.path.join(self.configuration.share_mount_path, share['name'])
215 209
 
216
-    @synchronized
217 210
     def _attach_volume(self, context, share, instance_id, volume):
218 211
         """Attaches cinder volume to service vm."""
219
-        if volume['status'] == 'in-use':
220
-            attached_volumes = [vol.id for vol in
221
-                self.compute_api.instance_volumes_list(self.admin_context,
222
-                                                       instance_id)]
223
-            if volume['id'] in attached_volumes:
224
-                return volume
225
-            else:
226
-                raise exception.ManilaException(_('Volume %s is already '
227
-                        'attached to another instance') % volume['id'])
228
-        self.compute_api.instance_volume_attach(self.admin_context,
229
-                                                instance_id,
230
-                                                volume['id'],
231
-                                                )
232
-
233
-        t = time.time()
234
-        while time.time() - t < self.configuration.max_time_to_attach:
235
-            volume = self.volume_api.get(context, volume['id'])
212
+        @lockutils.synchronized(instance_id, external=True,
213
+                                lock_path="attach_detach_locks")
214
+        def do_attach(volume):
236 215
             if volume['status'] == 'in-use':
237
-                break
238
-            elif volume['status'] != 'attaching':
239
-                raise exception.ManilaException(_('Failed to attach volume %s')
240
-                                                % volume['id'])
241
-            time.sleep(1)
242
-        else:
243
-            raise exception.ManilaException(_('Volume have not been attached '
244
-                                              'in %ss. Giving up') %
245
-                                      self.configuration.max_time_to_attach)
216
+                attached_volumes = [vol.id for vol in
217
+                                    self.compute_api.instance_volumes_list(
218
+                                        self.admin_context, instance_id)]
219
+                if volume['id'] in attached_volumes:
220
+                    return volume
221
+                else:
222
+                    raise exception.ManilaException(
223
+                        _('Volume %s is already attached to another instance')
224
+                        % volume['id'])
225
+            self.compute_api.instance_volume_attach(self.admin_context,
226
+                                                    instance_id,
227
+                                                    volume['id'],
228
+                                                    )
246 229
 
247
-        return volume
230
+            t = time.time()
231
+            while time.time() - t < self.configuration.max_time_to_attach:
232
+                volume = self.volume_api.get(context, volume['id'])
233
+                if volume['status'] == 'in-use':
234
+                    return volume
235
+                elif volume['status'] != 'attaching':
236
+                    raise exception.ManilaException(
237
+                        _('Failed to attach volume %s') % volume['id'])
238
+                time.sleep(1)
239
+            else:
240
+                raise exception.ManilaException(
241
+                    _('Volume have not been attached in %ss. Giving up') %
242
+                    self.configuration.max_time_to_attach)
243
+        return do_attach(volume)
248 244
 
249 245
     def _get_volume(self, context, share_id):
250 246
         """Finds volume, associated to the specific share."""
@@ -274,30 +270,34 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
274 270
                     _('Error. Ambiguous volume snaphots'))
275 271
         return volume_snapshot
276 272
 
277
-    @synchronized
278 273
     def _detach_volume(self, context, share, server_details):
279 274
         """Detaches cinder volume from service vm."""
280
-        attached_volumes = [vol.id for vol in
281
-                self.compute_api.instance_volumes_list(
275
+        instance_id = server_details['instance_id']
276
+
277
+        @lockutils.synchronized(instance_id, external=True,
278
+                                lock_path="attach_detach_locks")
279
+        def do_detach():
280
+            attached_volumes = [vol.id for vol in
281
+                                self.compute_api.instance_volumes_list(
282
+                                    self.admin_context, instance_id)]
283
+            volume = self._get_volume(context, share['id'])
284
+            if volume and volume['id'] in attached_volumes:
285
+                self.compute_api.instance_volume_detach(
282 286
                     self.admin_context,
283
-                    server_details['instance_id'])]
284
-        volume = self._get_volume(context, share['id'])
285
-        if volume and volume['id'] in attached_volumes:
286
-            self.compute_api.instance_volume_detach(
287
-                self.admin_context,
288
-                server_details['instance_id'],
289
-                volume['id']
290
-            )
291
-            t = time.time()
292
-            while time.time() - t < self.configuration.max_time_to_attach:
293
-                volume = self.volume_api.get(context, volume['id'])
294
-                if volume['status'] in ('available', 'error'):
295
-                    break
296
-                time.sleep(1)
297
-            else:
298
-                raise exception.ManilaException(_('Volume have not been '
299
-                                                  'detached in %ss. Giving up')
300
-                                       % self.configuration.max_time_to_attach)
287
+                    instance_id,
288
+                    volume['id']
289
+                )
290
+                t = time.time()
291
+                while time.time() - t < self.configuration.max_time_to_attach:
292
+                    volume = self.volume_api.get(context, volume['id'])
293
+                    if volume['status'] in ('available', 'error'):
294
+                        break
295
+                    time.sleep(1)
296
+                else:
297
+                    raise exception.ManilaException(
298
+                        _('Volume have not been detached in %ss. Giving up')
299
+                        % self.configuration.max_time_to_attach)
300
+        do_detach()
301 301
 
302 302
     def _allocate_container(self, context, share, snapshot=None):
303 303
         """Creates cinder volume, associated to share by name."""
@@ -498,8 +498,8 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
498 498
         server = self.service_instance_manager.set_up_service_instance(
499 499
             context=self.admin_context,
500 500
             share_server_id=srv_id,
501
-            share_network_id=sn_id,
502
-            create=True)
501
+            share_network_id=sn_id
502
+        )
503 503
         for helper in self._helpers.values():
504 504
                 helper.init_helper(server)
505 505
         return server
@@ -524,11 +524,10 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
524 524
 class NASHelperBase(object):
525 525
     """Interface to work with share."""
526 526
 
527
-    def __init__(self, execute, ssh_execute, config_object, locks):
527
+    def __init__(self, execute, ssh_execute, config_object):
528 528
         self.configuration = config_object
529 529
         self._execute = execute
530 530
         self._ssh_exec = ssh_execute
531
-        self.share_networks_locks = locks
532 531
 
533 532
     def init_helper(self, server):
534 533
         pass

+ 1
- 32
manila/share/drivers/service_instance.py View File

@@ -88,31 +88,6 @@ CONF.register_opts(server_opts)
88 88
 lock = threading.Lock()
89 89
 
90 90
 
91
-def synchronized(f):
92
-    """Decorates function with unique locks for each share network.
93
-
94
-    Share network id must be provided either as value/attribute
95
-    of one of args or as named argument.
96
-    """
97
-
98
-    def wrapped_func(self, *args, **kwargs):
99
-        share_network_id = kwargs.get('share_network_id', None)
100
-        if not share_network_id:
101
-            for arg in args:
102
-                share_network_id = getattr(arg, 'share_network_id', None)
103
-                if isinstance(arg, dict):
104
-                    share_network_id = arg.get('share_network_id', None)
105
-                if share_network_id:
106
-                    break
107
-            else:
108
-                msg = _("Could not get share network id.")
109
-                raise exception.ServiceInstanceException(msg)
110
-        with self.share_networks_locks.setdefault(share_network_id,
111
-                                                  threading.Lock()):
112
-            return f(self, *args, **kwargs)
113
-    return wrapped_func
114
-
115
-
116 91
 class ServiceInstanceManager(object):
117 92
     """Manages nova instances for various share drivers.
118 93
 
@@ -166,7 +141,6 @@ class ServiceInstanceManager(object):
166 141
         else:
167 142
             raise exception.ServiceInstanceException(_('Can not receive '
168 143
                                                        'service tenant id.'))
169
-        self.share_networks_locks = {}
170 144
         self.share_networks_servers = {}
171 145
         self.service_network_id = self._get_service_network()
172 146
         self.vif_driver = importutils.import_class(
@@ -291,18 +265,13 @@ class ServiceInstanceManager(object):
291 265
                 'been deleted in %ss. Giving up.') %
292 266
                 self.max_time_to_build_instance)
293 267
 
294
-    @synchronized
295 268
     def set_up_service_instance(self, context, share_server_id,
296
-                                share_network_id, create=False,
297
-                                return_inactive=False):
269
+                                share_network_id):
298 270
         """Finds or creates and sets up service vm.
299 271
 
300 272
         :param context: defines context, that should be used
301 273
         :param share_network_id: it provides network data for service VM
302 274
         :param share_server_id: provides server id for service VM
303
-        :param create: allow create service VM or not
304
-        :param return_inactive: allows to return not active VM, without
305
-                                raise of exception
306 275
         :returns: dict with data for service VM
307 276
         :raises: exception.ServiceInstanceException
308 277
         """

+ 6
- 5
manila/tests/test_service_instance.py View File

@@ -21,6 +21,7 @@ import os
21 21
 
22 22
 from manila import context
23 23
 from manila import exception
24
+from manila.openstack.common import lockutils
24 25
 from manila.share.drivers import service_instance
25 26
 from manila import test
26 27
 from manila.tests.db import fakes as db_fakes
@@ -67,8 +68,8 @@ class ServiceInstanceManagerTestCase(test.TestCase):
67 68
         self._manager.admin_context = self._context
68 69
         self._manager._execute = mock.Mock(return_value=('', ''))
69 70
         self._manager.vif_driver = mock.Mock()
70
-        self.stubs.Set(service_instance, 'synchronized', mock.Mock(side_effect=
71
-                                                                  lambda f: f))
71
+        self.stubs.Set(lockutils, 'synchronized',
72
+                       mock.Mock(return_value=lambda f: f))
72 73
         self.stubs.Set(service_instance.os.path, 'exists',
73 74
                        mock.Mock(return_value=True))
74 75
         self._manager._helpers = {
@@ -221,12 +222,12 @@ class ServiceInstanceManagerTestCase(test.TestCase):
221 222
                        mock.Mock(return_value='fake_ip'))
222 223
         self.stubs.Set(self._manager.compute_api, 'server_list',
223 224
                        mock.Mock(return_value=[]))
225
+        create = mock.Mock(return_value=fake_server)
224 226
         self.stubs.Set(self._manager, '_create_service_instance',
225
-                       mock.Mock(return_value=fake_server))
226
-
227
+                       create)
227 228
         result = self._manager.set_up_service_instance(
228 229
             self._context, share_server_id='fake_share_srv_id',
229
-            share_network_id='fake_share_network_id', create=True)
230
+            share_network_id='fake_share_network_id')
230 231
 
231 232
         self._manager.compute_api.server_list.assert_called_once()
232 233
         self._manager._get_server_ip.assert_called_once()

+ 7
- 6
manila/tests/test_share_generic.py View File

@@ -23,6 +23,7 @@ from oslo.config import cfg
23 23
 from manila import compute
24 24
 from manila import context
25 25
 from manila import exception
26
+from manila.openstack.common import lockutils
26 27
 from manila.share.configuration import Configuration
27 28
 from manila.share.drivers import generic
28 29
 from manila import test
@@ -114,8 +115,8 @@ class GenericShareDriverTestCase(test.TestCase):
114 115
             share_network_id=self.fake_sn["id"], old_server_ip="fake")
115 116
 
116 117
         self._driver._ssh_exec = mock.Mock(return_value=('', ''))
117
-        self.stubs.Set(generic, 'synchronized', mock.Mock(side_effect=
118
-                                                          lambda f: f))
118
+        self.stubs.Set(lockutils, 'synchronized',
119
+                       mock.Mock(return_value=lambda f: f))
119 120
         self.stubs.Set(generic.os.path, 'exists', mock.Mock(return_value=True))
120 121
         self._driver._helpers = {
121 122
             'CIFS': self._helper_cifs,
@@ -153,8 +154,8 @@ class GenericShareDriverTestCase(test.TestCase):
153 154
         self._helper_nfs.assert_called_once_with(
154 155
             self._execute,
155 156
             self._driver._ssh_exec,
156
-            self.fake_conf,
157
-            self._driver.share_networks_locks)
157
+            self.fake_conf
158
+        )
158 159
         self.assertEqual(len(self._driver._helpers), 1)
159 160
 
160 161
     def test_create_share(self):
@@ -588,7 +589,7 @@ class NFSHelperTestCase(test.TestCase):
588 589
         self._ssh_exec = mock.Mock(return_value=('', ''))
589 590
         self._execute = mock.Mock(return_value=('', ''))
590 591
         self._helper = generic.NFSHelper(self._execute, self._ssh_exec,
591
-                                         self.fake_conf, {})
592
+                                         self.fake_conf)
592 593
 
593 594
     def test_create_export(self):
594 595
         fake_server = fake_compute.FakeServer(ip='10.254.0.3')
@@ -632,7 +633,7 @@ class CIFSHelperTestCase(test.TestCase):
632 633
         self._ssh_exec = mock.Mock(return_value=('', ''))
633 634
         self._execute = mock.Mock(return_value=('', ''))
634 635
         self._helper = generic.CIFSHelper(self._execute, self._ssh_exec,
635
-                                          self.fake_conf, {})
636
+                                          self.fake_conf)
636 637
 
637 638
     def test_create_export(self):
638 639
         #fake_server = fake_compute.FakeServer(ip='10.254.0.3',

Loading…
Cancel
Save