Merge "Add ability to specify mode to attachment-create"

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.

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

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.

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

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)
 

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'),\

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
 

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,