diff --git a/cinder/api/microversions.py b/cinder/api/microversions.py index 1b835fc5135..18866472fa7 100644 --- a/cinder/api/microversions.py +++ b/cinder/api/microversions.py @@ -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. diff --git a/cinder/api/openstack/api_version_request.py b/cinder/api/openstack/api_version_request.py index 7acb6a0138a..027b554632a 100644 --- a/cinder/api/openstack/api_version_request.py +++ b/cinder/api/openstack/api_version_request.py @@ -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 diff --git a/cinder/api/openstack/rest_api_version_history.rst b/cinder/api/openstack/rest_api_version_history.rst index 58b09a5a408..74681854b4c 100644 --- a/cinder/api/openstack/rest_api_version_history.rst +++ b/cinder/api/openstack/rest_api_version_history.rst @@ -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. diff --git a/cinder/api/v3/attachments.py b/cinder/api/v3/attachments.py index 171d2f84586..0a209176f4e 100644 --- a/cinder/api/v3/attachments.py +++ b/cinder/api/v3/attachments.py @@ -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 diff --git a/cinder/tests/unit/attachments/test_attachments_api.py b/cinder/tests/unit/attachments/test_attachments_api.py index b3cdf1b220f..f0d2202950c 100644 --- a/cinder/tests/unit/attachments/test_attachments_api.py +++ b/cinder/tests/unit/attachments/test_attachments_api.py @@ -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) diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index beed36a4300..a50f9d4edd5 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -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'),\ diff --git a/cinder/volume/api.py b/cinder/volume/api.py index faea17216f6..3d01ea8b055 100644 --- a/cinder/volume/api.py +++ b/cinder/volume/api.py @@ -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 diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index 33cd396160a..d1105935f45 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -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,