Browse Source

Merge "Prevent double-attachment race in attachment_reserve" into stable/rocky

changes/74/677274/1
Zuul 1 month ago
parent
commit
3fd8656239

+ 3
- 0
cinder/cmd/api.py View File

@@ -35,6 +35,7 @@ i18n.enable_lazy()
35 35
 
36 36
 # Need to register global_opts
37 37
 from cinder.common import config
38
+from cinder import coordination
38 39
 from cinder import rpc
39 40
 from cinder import service
40 41
 from cinder import utils
@@ -56,6 +57,8 @@ def main():
56 57
 
57 58
     gmr.TextGuruMeditation.setup_autorun(version, conf=CONF)
58 59
 
60
+    coordination.COORDINATOR.start()
61
+
59 62
     rpc.init(CONF)
60 63
     launcher = service.process_launcher()
61 64
     server = service.WSGIService('osapi_volume')

+ 30
- 11
cinder/tests/unit/api/v3/test_attachments.py View File

@@ -19,12 +19,16 @@ Tests for attachments Api.
19 19
 
20 20
 import ddt
21 21
 import mock
22
+from oslo_policy import policy as oslo_policy
22 23
 
23 24
 from cinder.api import microversions as mv
24 25
 from cinder.api.v3 import attachments as v3_attachments
25 26
 from cinder import context
26 27
 from cinder import exception
27 28
 from cinder import objects
29
+from cinder.policies import attachments as attachments_policies
30
+from cinder.policies import base as base_policy
31
+from cinder import policy
28 32
 from cinder import test
29 33
 from cinder.tests.unit.api import fakes
30 34
 from cinder.tests.unit import fake_constants as fake
@@ -48,13 +52,17 @@ class AttachmentsAPITestCase(test.TestCase):
48 52
         self.volume2 = self._create_volume(display_name='fake_volume_2',
49 53
                                            project_id=fake.PROJECT2_ID)
50 54
         self.attachment1 = self._create_attachment(
51
-            volume_uuid=self.volume1.id, instance_uuid=fake.UUID1)
55
+            volume_uuid=self.volume1.id, instance_uuid=fake.UUID1,
56
+            host='host-a')
52 57
         self.attachment2 = self._create_attachment(
53
-            volume_uuid=self.volume1.id, instance_uuid=fake.UUID1)
58
+            volume_uuid=self.volume1.id, instance_uuid=fake.UUID1,
59
+            host='host-b')
54 60
         self.attachment3 = self._create_attachment(
55
-            volume_uuid=self.volume1.id, instance_uuid=fake.UUID2)
61
+            volume_uuid=self.volume1.id, instance_uuid=fake.UUID2,
62
+            host='host-c')
56 63
         self.attachment4 = self._create_attachment(
57
-            volume_uuid=self.volume2.id, instance_uuid=fake.UUID2)
64
+            volume_uuid=self.volume2.id, instance_uuid=fake.UUID2,
65
+            host='host-d')
58 66
         self.addCleanup(self._cleanup)
59 67
 
60 68
     def _cleanup(self):
@@ -98,7 +106,8 @@ class AttachmentsAPITestCase(test.TestCase):
98 106
 
99 107
     @mock.patch.object(volume_rpcapi.VolumeAPI, 'attachment_update')
100 108
     def test_update_attachment(self, mock_update):
101
-        fake_connector = {'fake_key': 'fake_value'}
109
+        fake_connector = {'fake_key': 'fake_value',
110
+                          'host': 'somehost'}
102 111
         mock_update.return_value = fake_connector
103 112
         req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
104 113
                                       (fake.PROJECT_ID, self.attachment1.id),
@@ -107,7 +116,9 @@ class AttachmentsAPITestCase(test.TestCase):
107 116
         body = {
108 117
             "attachment":
109 118
                 {
110
-                    "connector": {'fake_key': 'fake_value'},
119
+                    "connector": {'fake_key': 'fake_value',
120
+                                  'host': 'somehost',
121
+                                  'connection_info': 'a'},
111 122
                 },
112 123
         }
113 124
 
@@ -133,19 +144,26 @@ class AttachmentsAPITestCase(test.TestCase):
133 144
                           self.controller.update, req,
134 145
                           self.attachment1.id, body=body)
135 146
 
147
+    @mock.patch('cinder.coordination.synchronized')
136 148
     @mock.patch.object(objects.VolumeAttachment, 'get_by_id')
137
-    def test_attachment_operations_not_authorized(self, mock_get):
138
-        mock_get.return_value = {'project_id': fake.PROJECT2_ID}
149
+    def test_attachment_operations_not_authorized(self, mock_get, mock_synch):
150
+        mock_get.return_value = self.attachment1
139 151
         req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
140
-                                      (fake.PROJECT_ID, self.attachment1.id),
152
+                                      (fake.PROJECT2_ID, self.attachment1.id),
141 153
                                       version=mv.NEW_ATTACH,
142 154
                                       use_admin_context=False)
143 155
         body = {
144 156
             "attachment":
145 157
                 {
146
-                    "connector": {'fake_key': 'fake_value'},
158
+                    "connector": {'fake_key': 'fake_value',
159
+                                  'host': 'somehost'},
147 160
                 },
148 161
         }
162
+        rules = {attachments_policies.UPDATE_POLICY:
163
+                 base_policy.RULE_ADMIN_OR_OWNER}
164
+        policy.set_rules(oslo_policy.Rules.from_dict(rules))
165
+        self.addCleanup(policy.reset)
166
+
149 167
         self.assertRaises(exception.NotAuthorized,
150 168
                           self.controller.update, req,
151 169
                           self.attachment1.id, body=body)
@@ -198,7 +216,7 @@ class AttachmentsAPITestCase(test.TestCase):
198 216
     def _create_attachment(self, ctxt=None, volume_uuid=None,
199 217
                            instance_uuid=None, mountpoint=None,
200 218
                            attach_time=None, detach_time=None,
201
-                           attach_status=None, attach_mode=None):
219
+                           attach_status=None, attach_mode=None, host=None):
202 220
         """Create an attachment object."""
203 221
         ctxt = ctxt or self.ctxt
204 222
         attachment = objects.VolumeAttachment(ctxt)
@@ -209,6 +227,7 @@ class AttachmentsAPITestCase(test.TestCase):
209 227
         attachment.detach_time = detach_time
210 228
         attachment.attach_status = attach_status or 'reserved'
211 229
         attachment.attach_mode = attach_mode
230
+        attachment.connector = {'host': host}
212 231
         attachment.create()
213 232
         return attachment
214 233
 

+ 45
- 2
cinder/tests/unit/attachments/test_attachments_api.py View File

@@ -126,7 +126,8 @@ class AttachmentManagerTestCase(test.TestCase):
126 126
         vref = objects.Volume.get_by_id(self.context,
127 127
                                         vref.id)
128 128
 
129
-        connector = {'fake': 'connector'}
129
+        connector = {'fake': 'connector',
130
+                     'host': 'somehost'}
130 131
         self.volume_api.attachment_update(self.context,
131 132
                                           aref,
132 133
                                           connector)
@@ -308,9 +309,51 @@ class AttachmentManagerTestCase(test.TestCase):
308 309
         self.assertEqual({}, aref.connection_info)
309 310
         vref.status = 'error'
310 311
         vref.save()
311
-        connector = {'fake': 'connector'}
312
+        connector = {'fake': 'connector',
313
+                     'host': 'somehost'}
312 314
         self.assertRaises(exception.InvalidVolume,
313 315
                           self.volume_api.attachment_update,
314 316
                           self.context,
315 317
                           aref,
316 318
                           connector)
319
+
320
+    @mock.patch('cinder.db.sqlalchemy.api.volume_attachment_update',
321
+                return_value={})
322
+    @mock.patch('cinder.volume.rpcapi.VolumeAPI.attachment_update',
323
+                return_value={})
324
+    def test_attachment_update_duplicate(self, mock_va_update, mock_db_upd):
325
+        volume_params = {'status': 'available'}
326
+
327
+        vref = tests_utils.create_volume(self.context,
328
+                                         deleted=0,
329
+                                         **volume_params)
330
+
331
+        tests_utils.attach_volume(self.context,
332
+                                  vref.id,
333
+                                  fake.UUID1,
334
+                                  'somehost',
335
+                                  'somemountpoint')
336
+
337
+        # Update volume with another attachment
338
+        tests_utils.attach_volume(self.context,
339
+                                  vref.id,
340
+                                  fake.UUID2,
341
+                                  'somehost2',
342
+                                  'somemountpoint2')
343
+        vref.refresh()
344
+
345
+        # This attachment will collide with the first
346
+        connector = {'host': 'somehost'}
347
+        vref.volume_attachment[0]['connector'] = {'host': 'somehost'}
348
+        vref.volume_attachment[0]['connection_info'] = {'c': 'd'}
349
+        with mock.patch('cinder.objects.Volume.get_by_id', return_value=vref):
350
+            with mock.patch.object(self.volume_api.volume_rpcapi,
351
+                                   'attachment_update') as m_au:
352
+                self.assertRaises(exception.InvalidVolume,
353
+                                  self.volume_api.attachment_update,
354
+                                  self.context,
355
+                                  vref.volume_attachment[1],
356
+                                  connector)
357
+                m_au.assert_not_called()
358
+        mock_va_update.assert_not_called()
359
+        mock_db_upd.assert_not_called()

+ 30
- 0
cinder/volume/api.py View File

@@ -32,6 +32,7 @@ import six
32 32
 from cinder.api import common
33 33
 from cinder.common import constants
34 34
 from cinder import context
35
+from cinder import coordination
35 36
 from cinder import db
36 37
 from cinder.db import base
37 38
 from cinder import exception
@@ -2164,6 +2165,8 @@ class API(base.Base):
2164 2165
         attachment_ref.save()
2165 2166
         return attachment_ref
2166 2167
 
2168
+    @coordination.synchronized(
2169
+        '{f_name}-{attachment_ref.volume_id}-{connector[host]}')
2167 2170
     def attachment_update(self, ctxt, attachment_ref, connector):
2168 2171
         """Update an existing attachment record."""
2169 2172
         # Valid items to update (connector includes mode and mountpoint):
@@ -2172,6 +2175,10 @@ class API(base.Base):
2172 2175
         #     b. mountpoint (if None use value from attachment_ref)
2173 2176
         #     c. instance_uuid(if None use value from attachment_ref)
2174 2177
 
2178
+        # This method has a synchronized() lock on the volume id
2179
+        # because we have to prevent race conditions around checking
2180
+        # for duplicate attachment requests to the same host.
2181
+
2175 2182
         # We fetch the volume object and pass it to the rpc call because we
2176 2183
         # need to direct this to the correct host/backend
2177 2184
 
@@ -2186,6 +2193,29 @@ class API(base.Base):
2186 2193
                        'volume_status': volume_ref.status}
2187 2194
             LOG.error(msg)
2188 2195
             raise exception.InvalidVolume(reason=msg)
2196
+
2197
+        if (len(volume_ref.volume_attachment) > 1 and
2198
+            not (volume_ref.multiattach or
2199
+                 self._is_multiattach(volume_ref.volume_type))):
2200
+            # Check whether all connection hosts are unique
2201
+            # Multiple attachments to different hosts is permitted to
2202
+            # support Nova instance migration.
2203
+
2204
+            # This particular check also does not prevent multiple attachments
2205
+            # for a multiattach volume to the same instance.
2206
+
2207
+            connection_hosts = set(a.connector['host']
2208
+                                   for a in volume_ref.volume_attachment
2209
+                                   if a.connection_info)
2210
+
2211
+            if len(connection_hosts) > 0:
2212
+                # We raced, and have more than one connection
2213
+
2214
+                msg = _('duplicate connectors detected on volume '
2215
+                        '%(vol)s') % {'vol': volume_ref.id}
2216
+
2217
+                raise exception.InvalidVolume(reason=msg)
2218
+
2189 2219
         connection_info = (
2190 2220
             self.volume_rpcapi.attachment_update(ctxt,
2191 2221
                                                  volume_ref,

+ 3
- 0
cinder/wsgi/wsgi.py View File

@@ -30,6 +30,7 @@ i18n.enable_lazy()
30 30
 # Need to register global_opts
31 31
 from cinder.common import config
32 32
 from cinder.common import constants
33
+from cinder import coordination
33 34
 from cinder import rpc
34 35
 from cinder import service
35 36
 from cinder import version
@@ -44,6 +45,8 @@ def initialize_application():
44 45
     logging.setup(CONF, "cinder")
45 46
     config.set_middleware_defaults()
46 47
 
48
+    coordination.COORDINATOR.start()
49
+
47 50
     rpc.init(CONF)
48 51
     service.setup_profiler(constants.API_BINARY, CONF.host)
49 52
 

Loading…
Cancel
Save