openstack
/
cinder
Code Issues Proposed changes

Add ability to specify mode to attachment-create 

This patch is the next step in simplifying how we create and
manage Read Only Volume Attachments.

Currently, we have multiple ways of doing this including
volume_admin_metadata and the connector that's supplied during
attachment processes.  The problem is that this method requires
you to set and match both settings; and there are a lot of places
things can go wrong.

If you use volume_admin_metadata, the volume must ALWAYS receive
a connector that has read only specified in it, otherwise if the
values don't match, the attach calls fail.

If you don't use the metadata setting (ie you don't want the volume
to be exclusively Read Only) you can just specify the mode in the
connector.  Rather than have these two different settings and juggle
the various attachment parameters that have been introduced
(attach_mode, access_mode, volume_admin_metadata:read_only...) it
would be much more straight forward to just allow the ability to
specify an attach mode during attachment-create and use that setting.

This change adds that option to attachment-create, but it also
keeps backward compatibility with the volume_admin_metadata for those
that are using it.  Yes, we could just do a version bump and eliminate
admin_metadata; but maintaining the two independently is more disruptive
than just keeping compatibility.

One thing that this change does that might be unexpected, is it
eliminates the requirement for volume_admin_metadata and the connectors
mode parameters to match.  It assumes that if an admin went through the
trouble to set a volumes admin_metadata to read only, then it should in
fact be read only regardless of the connector.  So we always revert to
the admin_metadata setting, and modify the connector if they don't
match.

The next step in getting completely away from admin_metadata is to
introduce the ability to create a Read Only volume via types; that will
by the global setting for a volume in terms of its attachment setting.

None of this enforces anything on the consumer side (the old methods
didn't either) but it does set flags on the volume and attachment record
indicating what was requested and what we expect the consumer to be
doing with the volume.

Change-Id: I091f4eb8f1b464bb2dddb0a29245690a0430142e
This commit is contained in:
John Griffith 2018-01-10 17:27:35 -07:00 committed by Sean McGinnis
parent d7908be9ac
commit 9337f396ab
8 changed files with 49 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
cinder/api/microversions.py
View File

@ -145,6 +145,8 @@ SUPPORT_VOLUME_TYPE_FILTER = '3.52'
SUPPORT_VOLUME_SCHEMA_CHANGES = '3.53'
ATTACHMENT_CREATE_MODE_ARG = '3.54'
def get_mv_header(version):
"""Gets a formatted HTTP microversion header.

5
cinder/api/openstack/api_version_request.py
View File

@ -125,6 +125,7 @@ REST_API_VERSION_HISTORY = """
2. Update volume API expects user to pass at least one valid
parameter in the request body in order to update the volume.
Also, additional parameters will not be allowed.
* 3.54 - Add ``mode`` argument to attachment-create.
"""
# The minimum and maximum versions of the API supported
@ -132,9 +133,9 @@ REST_API_VERSION_HISTORY = """
# minimum version of the API supported.
# Explicitly using /v2 endpoints will still work
_MIN_API_VERSION = "3.0"
_MAX_API_VERSION = "3.53"
_MAX_API_VERSION = "3.54"
_LEGACY_API_VERSION2 = "2.0"
UPDATED = "2018-06-29T05:34:49Z"
UPDATED = "2018-07-17T00:00:00Z"
# NOTE(cyeoh): min and max versions declared as functions so we can

4
cinder/api/openstack/rest_api_version_history.rst
View File

@ -433,3 +433,7 @@ volume APIs.
the volume was updated.
But in 3.53, user will need to pass at least one valid parameter in the request
body otherwise it will return 400 error.
3.54
----
Add ``mode`` argument to attachment-create.

13
cinder/api/v3/attachments.py
View File

@ -164,14 +164,23 @@ class AttachmentsController(wsgi.Controller):
volume_ref = objects.Volume.get_by_id(
context,
volume_uuid)
connector = body['attachment'].get('connector', None)
args = {'connector': body['attachment'].get('connector', None)}
if req.api_version_request.matches(mv.ATTACHMENT_CREATE_MODE_ARG):
# We check for attach_mode here and default to `null`
# if nothing's provided. This seems odd to not just
# set `rw`, BUT we want to keep compatability with
# setting the mode via the connector for now, so we
# use `null` as an identifier to distinguish that case
args['attach_mode'] = body['attachment'].get('mode', 'null')
err_msg = None
try:
attachment_ref = (
self.volume_api.attachment_create(context,
volume_ref,
instance_uuid,
connector=connector))
**args))
except (exception.NotAuthorized,
exception.InvalidVolume):
raise

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

@ -57,7 +57,7 @@ class AttachmentManagerTestCase(test.TestCase):
self.assertEqual(fake.UUID2, aref.instance_uuid)
self.assertIsNone(aref.attach_time)
self.assertEqual('reserved', aref.attach_status)
self.assertIsNone(aref.attach_mode)
self.assertEqual('null', aref.attach_mode)
self.assertEqual(vref.id, aref.volume_id)
self.assertEqual({}, aref.connection_info)
@ -162,7 +162,7 @@ class AttachmentManagerTestCase(test.TestCase):
self.assertEqual(fake.UUID2, aref.instance_uuid)
self.assertIsNone(aref.attach_time)
self.assertEqual('reserved', aref.attach_status)
self.assertIsNone(aref.attach_mode)
self.assertEqual('null', aref.attach_mode)
self.assertEqual(vref.id, aref.volume_id)
self.assertEqual({}, aref.connection_info)

28
cinder/tests/unit/attachments/test_attachments_manager.py
View File

@ -43,31 +43,6 @@ class AttachmentManagerTestCase(test.TestCase):
self.manager.stats = {'allocated_capacity_gb': 100,
'pools': {}}
@mock.patch.object(db, 'volume_admin_metadata_update')
@mock.patch('cinder.message.api.API.create', mock.Mock())
def test_attachment_update_with_readonly_volume(self, mock_update):
mock_update.return_value = {'readonly': 'True'}
vref = tests_utils.create_volume(self.context, **{'status':
'available'})
self.manager.create_volume(self.context, vref)
attachment_ref = db.volume_attach(self.context,
{'volume_id': vref.id,
'volume_host': vref.host,
'attach_status': 'reserved',
'instance_uuid': fake.UUID1})
with mock.patch.object(self.manager,
'_notify_about_volume_usage',
return_value=None), mock.patch.object(
self.manager, '_connection_create'):
self.assertRaises(exception.InvalidVolumeAttachMode,
self.manager.attachment_update,
self.context, vref, {}, attachment_ref.id)
attachment = db.volume_attachment_get(self.context,
attachment_ref.id)
self.assertEqual(fields.VolumeAttachStatus.ERROR_ATTACHING,
attachment['attach_status'])
def test_attachment_update(self):
"""Test attachment_update."""
volume_params = {'status': 'available'}
@ -84,7 +59,8 @@ class AttachmentManagerTestCase(test.TestCase):
values = {'volume_id': vref.id,
'attached_host': vref.host,
'attach_status': 'reserved',
'instance_uuid': fake.UUID1}
'instance_uuid': fake.UUID1,
'attach_mode': 'rw'}
attachment_ref = db.volume_attach(self.context, values)
with mock.patch.object(
self.manager, '_notify_about_volume_usage'),\

16
cinder/volume/api.py
View File

@ -2115,7 +2115,8 @@ class API(base.Base):
ctxt,
volume_ref,
instance_uuid,
connector=None):
connector=None,
attach_mode='null'):
"""Create an attachment record for the specified volume."""
ctxt.authorize(attachment_policy.CREATE_POLICY, target_obj=volume_ref)
connection_info = {}
@ -2129,10 +2130,23 @@ class API(base.Base):
connector,
attachment_ref.id))
attachment_ref.connection_info = connection_info
# Use of admin_metadata for RO settings is deprecated
# switch to using mode argument to attachment-create
if self.db.volume_admin_metadata_get(
ctxt.elevated(),
volume_ref['id']).get('readonly', False):
LOG.warning("Using volume_admin_metadata to set "
"Read Only mode is deprecated! Please "
"use the mode argument in attachment-create.")
attachment_ref.attach_mode = 'ro'
# for now we have to let the admin_metadata override
# so we're using an else in the next step here, in
# other words, using volume_admin_metadata and mode params
# are NOT compatible
else:
attachment_ref.attach_mode = attach_mode
attachment_ref.save()
return attachment_ref

27
cinder/volume/manager.py
View File

@ -4387,29 +4387,22 @@ class VolumeManager(manager.CleanableManager,
self._notify_about_volume_usage(context, vref, 'attach.start')
attachment_ref = objects.VolumeAttachment.get_by_id(context,
attachment_id)
# Check to see if a mode parameter was set during attachment-create;
# this seems kinda wonky, but it's how we're keeping back compatability
# with the use of connector.mode for now. In other words, we're
# making sure we still honor ro settings from the connector but
# we override that if a value was specified in attachment-create
if attachment_ref.attach_mode != 'null':
mode = attachment_ref.attach_mode
connector['mode'] = mode
connection_info = self._connection_create(context,
vref,
attachment_ref,
connector)
# FIXME(jdg): get rid of this admin_meta option here, the only thing
# it does is enforce that a volume is R/O, that should be done via a
# type and not *more* metadata
volume_metadata = self.db.volume_admin_metadata_update(
context.elevated(),
attachment_ref.volume_id,
{'attached_mode': mode}, False)
# The prior seting of mode in the attachment_ref overrides any
# settings within the connector when dealing with read only
# attachment options
if mode != 'ro' and attachment_ref.attach_mode == 'ro':
connector['mode'] = 'ro'
mode = 'ro'
try:
if volume_metadata.get('readonly') == 'True' and mode != 'ro':
raise exception.InvalidVolumeAttachMode(mode=mode,
volume_id=vref.id)
utils.require_driver_initialized(self.driver)
self.driver.attach_volume(context,
vref,