openstack
/
cinder
Code Issues Proposed changes

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

This commit is contained in:
Zuul 2019-08-12 22:26:12 +00:00 committed by Gerrit Code Review
parent 44a15be6a7 eadff0df7e
commit a71fa76545
5 changed files with 111 additions and 13 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
cinder/cmd/api.py
View File

@ -35,6 +35,7 @@ i18n.enable_lazy()
# Need to register global_opts
from cinder.common import config
from cinder import coordination
from cinder import rpc
from cinder import service
from cinder import utils
@ -56,6 +57,8 @@ def main():
gmr.TextGuruMeditation.setup_autorun(version, conf=CONF)
coordination.COORDINATOR.start()
rpc.init(CONF)
launcher = service.process_launcher()
server = service.WSGIService('osapi_volume')

41
cinder/tests/unit/api/v3/test_attachments.py
View File

@ -19,12 +19,16 @@ Tests for attachments Api.
import ddt
import mock
from oslo_policy import policy as oslo_policy
from cinder.api import microversions as mv
from cinder.api.v3 import attachments as v3_attachments
from cinder import context
from cinder import exception
from cinder import objects
from cinder.policies import attachments as attachments_policies
from cinder.policies import base as base_policy
from cinder import policy
from cinder import test
from cinder.tests.unit.api import fakes
from cinder.tests.unit import fake_constants as fake
@ -49,13 +53,17 @@ class AttachmentsAPITestCase(test.TestCase):
self.volume2 = self._create_volume(display_name='fake_volume_2',
project_id=fake.PROJECT2_ID)
self.attachment1 = self._create_attachment(
volume_uuid=self.volume1.id, instance_uuid=fake.UUID1)
volume_uuid=self.volume1.id, instance_uuid=fake.UUID1,
host='host-a')
self.attachment2 = self._create_attachment(
volume_uuid=self.volume1.id, instance_uuid=fake.UUID1)
volume_uuid=self.volume1.id, instance_uuid=fake.UUID1,
host='host-b')
self.attachment3 = self._create_attachment(
volume_uuid=self.volume1.id, instance_uuid=fake.UUID2)
volume_uuid=self.volume1.id, instance_uuid=fake.UUID2,
host='host-c')
self.attachment4 = self._create_attachment(
volume_uuid=self.volume2.id, instance_uuid=fake.UUID2)
volume_uuid=self.volume2.id, instance_uuid=fake.UUID2,
host='host-d')
self.addCleanup(self._cleanup)
def _cleanup(self):
@ -99,7 +107,8 @@ class AttachmentsAPITestCase(test.TestCase):
@mock.patch.object(volume_rpcapi.VolumeAPI, 'attachment_update')
def test_update_attachment(self, mock_update):
fake_connector = {'fake_key': 'fake_value'}
fake_connector = {'fake_key': 'fake_value',
'host': 'somehost'}
mock_update.return_value = fake_connector
req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
(fake.PROJECT_ID, self.attachment1.id),
@ -108,7 +117,9 @@ class AttachmentsAPITestCase(test.TestCase):
body = {
"attachment":
{
"connector": {'fake_key': 'fake_value'},
"connector": {'fake_key': 'fake_value',
'host': 'somehost',
'connection_info': 'a'},
},
}
@ -134,19 +145,26 @@ class AttachmentsAPITestCase(test.TestCase):
self.controller.update, req,
self.attachment1.id, body=body)
@mock.patch('cinder.coordination.synchronized')
@mock.patch.object(objects.VolumeAttachment, 'get_by_id')
def test_attachment_operations_not_authorized(self, mock_get):
mock_get.return_value = {'project_id': fake.PROJECT2_ID}
def test_attachment_operations_not_authorized(self, mock_get, mock_synch):
mock_get.return_value = self.attachment1
req = fakes.HTTPRequest.blank('/v3/%s/attachments/%s' %
(fake.PROJECT_ID, self.attachment1.id),
(fake.PROJECT2_ID, self.attachment1.id),
version=mv.NEW_ATTACH,
use_admin_context=False)
body = {
"attachment":
{
"connector": {'fake_key': 'fake_value'},
"connector": {'fake_key': 'fake_value',
'host': 'somehost'},
},
}
rules = {attachments_policies.UPDATE_POLICY:
base_policy.RULE_ADMIN_OR_OWNER}
policy.set_rules(oslo_policy.Rules.from_dict(rules))
self.addCleanup(policy.reset)
self.assertRaises(exception.NotAuthorized,
self.controller.update, req,
self.attachment1.id, body=body)
@ -199,7 +217,7 @@ class AttachmentsAPITestCase(test.TestCase):
def _create_attachment(self, ctxt=None, volume_uuid=None,
instance_uuid=None, mountpoint=None,
attach_time=None, detach_time=None,
attach_status=None, attach_mode=None):
attach_status=None, attach_mode=None, host=None):
"""Create an attachment object."""
ctxt = ctxt or self.ctxt
attachment = objects.VolumeAttachment(ctxt)
@ -210,6 +228,7 @@ class AttachmentsAPITestCase(test.TestCase):
attachment.detach_time = detach_time
attachment.attach_status = attach_status or 'reserved'
attachment.attach_mode = attach_mode
attachment.connector = {'host': host}
attachment.create()
return attachment

47
cinder/tests/unit/attachments/test_attachments_api.py
View File

@ -126,7 +126,8 @@ class AttachmentManagerTestCase(test.TestCase):
vref = objects.Volume.get_by_id(self.context,
vref.id)
connector = {'fake': 'connector'}
connector = {'fake': 'connector',
'host': 'somehost'}
self.volume_api.attachment_update(self.context,
aref,
connector)
@ -308,13 +309,55 @@ class AttachmentManagerTestCase(test.TestCase):
self.assertEqual({}, aref.connection_info)
vref.status = 'error'
vref.save()
connector = {'fake': 'connector'}
connector = {'fake': 'connector',
'host': 'somehost'}
self.assertRaises(exception.InvalidVolume,
self.volume_api.attachment_update,
self.context,
aref,
connector)
@mock.patch('cinder.db.sqlalchemy.api.volume_attachment_update',
return_value={})
@mock.patch('cinder.volume.rpcapi.VolumeAPI.attachment_update',
return_value={})
def test_attachment_update_duplicate(self, mock_va_update, mock_db_upd):
volume_params = {'status': 'available'}
vref = tests_utils.create_volume(self.context,
deleted=0,
**volume_params)
tests_utils.attach_volume(self.context,
vref.id,
fake.UUID1,
'somehost',
'somemountpoint')
# Update volume with another attachment
tests_utils.attach_volume(self.context,
vref.id,
fake.UUID2,
'somehost2',
'somemountpoint2')
vref.refresh()
# This attachment will collide with the first
connector = {'host': 'somehost'}
vref.volume_attachment[0]['connector'] = {'host': 'somehost'}
vref.volume_attachment[0]['connection_info'] = {'c': 'd'}
with mock.patch('cinder.objects.Volume.get_by_id', return_value=vref):
with mock.patch.object(self.volume_api.volume_rpcapi,
'attachment_update') as m_au:
self.assertRaises(exception.InvalidVolume,
self.volume_api.attachment_update,
self.context,
vref.volume_attachment[1],
connector)
m_au.assert_not_called()
mock_va_update.assert_not_called()
mock_db_upd.assert_not_called()
def test_attachment_create_creating_volume(self):
"""Test attachment_create on a creating volume."""
volume_params = {'status': 'creating'}

30
cinder/volume/api.py
View File

@ -32,6 +32,7 @@ import six
from cinder.api import common
from cinder.common import constants
from cinder import context
from cinder import coordination
from cinder import db
from cinder.db import base
from cinder import exception
@ -2156,6 +2157,8 @@ class API(base.Base):
attachment_ref.save()
return attachment_ref
@coordination.synchronized(
'{f_name}-{attachment_ref.volume_id}-{connector[host]}')
def attachment_update(self, ctxt, attachment_ref, connector):
"""Update an existing attachment record."""
# Valid items to update (connector includes mode and mountpoint):
@ -2164,6 +2167,10 @@ class API(base.Base):
# b. mountpoint (if None use value from attachment_ref)
# c. instance_uuid(if None use value from attachment_ref)
# This method has a synchronized() lock on the volume id
# because we have to prevent race conditions around checking
# for duplicate attachment requests to the same host.
# We fetch the volume object and pass it to the rpc call because we
# need to direct this to the correct host/backend
@ -2178,6 +2185,29 @@ class API(base.Base):
'volume_status': volume_ref.status}
LOG.error(msg)
raise exception.InvalidVolume(reason=msg)
if (len(volume_ref.volume_attachment) > 1 and
not (volume_ref.multiattach or
self._is_multiattach(volume_ref.volume_type))):
# Check whether all connection hosts are unique
# Multiple attachments to different hosts is permitted to
# support Nova instance migration.
# This particular check also does not prevent multiple attachments
# for a multiattach volume to the same instance.
connection_hosts = set(a.connector['host']
for a in volume_ref.volume_attachment
if a.connection_info)
if len(connection_hosts) > 0:
# We raced, and have more than one connection
msg = _('duplicate connectors detected on volume '
'%(vol)s') % {'vol': volume_ref.id}
raise exception.InvalidVolume(reason=msg)
connection_info = (
self.volume_rpcapi.attachment_update(ctxt,
volume_ref,

3
cinder/wsgi/wsgi.py
View File

@ -30,6 +30,7 @@ i18n.enable_lazy()
# Need to register global_opts
from cinder.common import config
from cinder.common import constants
from cinder import coordination
from cinder import rpc
from cinder import service
from cinder import version
@ -44,6 +45,8 @@ def initialize_application():
logging.setup(CONF, "cinder")
config.set_middleware_defaults()
coordination.COORDINATOR.start()
rpc.init(CONF)
service.setup_profiler(constants.API_BINARY, CONF.host)