Refactor Access Rules APIs

- Pull up policy check to beginning of the APIs.
- Avoid making access rules changes when one or
  more instances of the share are in an invalid state.
- Add back the per rule share instance access status.
  This restoration provides better visibility for which
  rules were applied successfully.
- Remove 'updating' and 'updating_multiple' as valid
  states for the share instance access rules status.
- Deprecate the access rule state 'new' in favor of
  'queued_to_apply' and the share instance access rules
  status 'out_of_sync' in favor of 'syncing'.

In a new API micro-version:
- Allow access rule changes irrespective of the share's
   access_rules_status.
- Expose new access rule states and share's
  access_rules_status values.

Access rules for each share instance now transition
from 'queued_to_apply' to 'applying' to 'active' or 'error';
and from 'active', 'queued_to_apply', 'applying' or 'error'
to 'queued_to_deny' to 'denying' to 'deleted'.

APIImpact
DocImpact

Partially-implements: bp fix-and-improve-access-rules
Co-Authored-By: Mike Rooney <rooneym@netapp.com>
Change-Id: Ic25e63215b5ba723cbc8cab7c51789c698e76f28

api-ref/source/parameters.yaml

@@ -506,19 +506,13 @@ access_rule_updated_at:
   type: string
 access_rules_status:
   description: |
-    The share instance access rules status. Valid value are ``active``,
-    ``error``, and ``out_of_sync``.
+    The share instance access rules status. A valid value is ``active``,
+    ``error``, or ``syncing``. In versions prior to 2.28, ``syncing`` was
+    represented with status ``out_of_sync``.
   in: body
   required: true
   type: string
   min_version: 2.10
-access_rules_status_1:
-  description: |
-    (Since API v2.10) The share access rules status.
-    Valid values are ``active``, ``error``, and ``out_of_sync``.
-  in: body
-  required: true
-  type: string
 access_share_id:
   description: |
     The UUID of the share to which you are granted
@@ -3088,8 +3082,13 @@ source_cgsnapshot_member_id:
   type: string
 state:
   description: |
-    The access rule state. A valid value is
-    ``active`` or ``error``.
+    Prior to versions 2.28, the state of all access rules of a given share
+    is the same at all times. This could be ``new``, ``active`` or
+    ``error``. Since 2.28, the state of each access rule of a share is
+    independent of the others and can be ``queued_to_apply``,
+    ``applying``, ``active``, ``error``, ``queued_to_deny`` or ``denying``.
+    A new rule starts out in ``queued_to_apply`` state and is successfully
+    applied if it transitions to ``active`` state.
   in: body
   required: true
   type: string

api-ref/source/share-actions.inc

@@ -4,35 +4,9 @@
 Share actions
 =============
 
-Grants or revokes share access, lists the permissions for a share,
-and explicitly updates the state of a share.
-
-To grant or revoke share access, specify one of these supported
-share access levels:
-
-- ``rw``. Read and write (RW) access.
-
-- ``ro``. Read-only (RO) access.
-
-You must also specify one of these supported authentication
-methods:
-
-- ``ip``. Authenticates an instance through its IP address. A valid
-  format is ``XX.XX.XX.XX`` or ``XX.XX.XX.XX/XX``. For example
-  ``0.0.0.0/0``.
-
-- ``cert``. Authenticates an instance through a TLS certificate.
-  Specify the TLS identity as the IDENTKEY. A valid value is any
-  string up to 64 characters long in the common name (CN) of the
-  certificate. The meaning of a string depends on its
-  interpretation.
-
-- ``user``. Authenticates by a user or group name. A valid value is
-  an alphanumeric string that can contain some special characters
-  and is from 4 to 32 characters long.
-
-To verify that the access rules (ACL) were configured correctly for
-a share, you list permissions for a share.
+Share actions include granting or revoking share access, listing the
+available access rules for a share, explicitly updating the state of a
+share, resizing a share and un-managing a share.
 
 As administrator, you can reset the state of a share and force-
 delete a share in any state. Use the ``policy.json`` file to grant
@@ -61,11 +35,38 @@ access_list": null} is valid for v1.0-2.6
 Grant access
 ============
 
+All manila shares begin with no access. Clients must be provided with
+explicit access via this API.
+
+To grant access, specify one of these supported share access levels:
+
+- ``rw``. Read and write (RW) access.
+
+- ``ro``. Read-only (RO) access.
+
+You must also specify one of these supported authentication
+methods:
+
+- ``ip``. Authenticates an instance through its IP address. A valid
+  format is ``XX.XX.XX.XX`` or ``XX.XX.XX.XX/XX``. For example
+  ``0.0.0.0/0``.
+
+- ``cert``. Authenticates an instance through a TLS certificate.
+  Specify the TLS identity as the IDENTKEY. A valid value is any
+  string up to 64 characters long in the common name (CN) of the
+  certificate. The meaning of a string depends on its
+  interpretation.
+
+- ``user``. Authenticates by a user or group name. A valid value is
+  an alphanumeric string that can contain some special characters
+  and is from 4 to 32 characters long.
+
 .. rest_method::  POST /v2/{tenant_id}/shares/{share_id}/action
 
 Grants access to a share.
 
 Normal response codes: 202
+
 Error response codes: badRequest(400), unauthorized(401), forbidden(403),
 itemNotFound(404)
 
@@ -114,9 +115,12 @@ Revoke access
 
 .. rest_method::  POST /v2/{tenant_id}/shares/{share_id}/action
 
-Revokes access from a share.
+The shared file systems service stores each access rule in its database and
+assigns it a unique ID. This ID can be used to revoke access after access
+has been requested.
 
 Normal response codes: 202
+
 Error response codes: badRequest(400), unauthorized(401), forbidden(403),
 itemNotFound(404)
 
@@ -142,9 +146,11 @@ List access rules
 
 .. rest_method::  POST /v2/{tenant_id}/shares/{share_id}/action
 
-Lists access rules for a share.
+Lists access rules for a share. The Access ID returned is necessary to deny
+access.
 
 Normal response codes: 200
+
 Error response codes: badRequest(400), unauthorized(401), forbidden(403),
 itemNotFound(404)
 

manila/api/openstack/api_version_request.py

@@ -84,15 +84,19 @@ REST_API_VERSION_HISTORY = """
              spec. Also made the 'snapshot_support' extra spec optional.
     * 2.25 - Added quota-show detail API.
     * 2.26 - Removed 'nova_net_id' parameter from share_network API.
-
     * 2.27 - Added share revert to snapshot API.
+    * 2.28 - Added transitional states to access rules and replaced all
+             transitional access_rules_status values of
+             shares (share_instances) with 'syncing'. Share action API
+             'access_allow' now accepts rules even when a share or any of
+             its instances may have an access_rules_status set to 'error'.
 """
 
 # The minimum and maximum versions of the API supported
 # The default api version request is defined to be the
 # minimum version of the API supported.
 _MIN_API_VERSION = "2.0"
-_MAX_API_VERSION = "2.27"
+_MAX_API_VERSION = "2.28"
 DEFAULT_API_VERSION = _MIN_API_VERSION
 
 

manila/api/openstack/rest_api_version_history.rst

@@ -163,3 +163,12 @@ user documentation.
   snapshot. The share is reverted in place, and the snapshot must be the most
   recent one known to manila. The feature is controlled by a new standard
   optional extra spec, revert_to_snapshot_support.
+
+2.28
+----
+  Added transitional states ('queued_to_apply' - was previously 'new',
+  'queued_to_deny', 'applying' and 'denying') to access rules.
+  'updating', 'updating_multiple' and 'out_of_sync' are no longer valid
+  values for the 'access_rules_status' field of shares, they have
+  been collapsed into the transitional state 'syncing'. Access rule changes
+  can be made independent of a share's 'access_rules_status'.

manila/api/v1/shares.py

@@ -30,6 +30,7 @@ from manila.api import common
 from manila.api.openstack import wsgi
 from manila.api.views import share_accesses as share_access_views
 from manila.api.views import shares as share_views
+from manila.common import constants
 from manila import db
 from manila import exception
 from manila.i18n import _, _LI
@@ -401,12 +402,34 @@ class ShareMixin(object):
             raise webob.exc.HTTPBadRequest(explanation=_(
                 'Ceph IDs may not contain periods'))
 
-    def _allow_access(self, req, id, body, enable_ceph=False):
+    @staticmethod
+    def _any_instance_has_errored_rules(share):
+        for instance in share['instances']:
+            access_rules_status = instance['access_rules_status']
+            if access_rules_status == constants.SHARE_INSTANCE_RULES_ERROR:
+                return True
+        return False
+
+    @wsgi.Controller.authorize('allow_access')
+    def _allow_access(self, req, id, body, enable_ceph=False,
+                      allow_on_error_status=False):
         """Add share access rule."""
         context = req.environ['manila.context']
         access_data = body.get('allow_access', body.get('os-allow_access'))
         share = self.share_api.get(context, id)
 
+        if (not allow_on_error_status and
+                self._any_instance_has_errored_rules(share)):
+            msg = _("Access rules cannot be added while the share or any of "
+                    "its replicas or migration copies has its "
+                    "access_rules_status set to %(instance_rules_status)s. "
+                    "Deny any rules in %(rule_state) state and try "
+                    "again.") % {
+                'instance_rules_status': constants.SHARE_INSTANCE_RULES_ERROR,
+                'rule_state': constants.ACCESS_STATE_ERROR,
+            }
+            raise webob.exc.HTTPBadRequest(explanation=msg)
+
         access_type = access_data['access_type']
         access_to = access_data['access_to']
         if access_type == 'ip':
@@ -435,6 +458,7 @@ class ShareMixin(object):
 
         return self._access_view_builder.view(req, access)
 
+    @wsgi.Controller.authorize('deny_access')
     def _deny_access(self, req, id, body):
         """Remove share access rule."""
         context = req.environ['manila.context']

manila/api/v2/shares.py

@@ -323,10 +323,14 @@ class ShareController(shares.ShareMixin,
     @wsgi.action('allow_access')
     def allow_access(self, req, id, body):
         """Add share access rule."""
-        if req.api_version_request < api_version.APIVersionRequest("2.13"):
-            return self._allow_access(req, id, body)
-        else:
-            return self._allow_access(req, id, body, enable_ceph=True)
+        args = (req, id, body)
+        kwargs = {}
+        if req.api_version_request >= api_version.APIVersionRequest("2.13"):
+            kwargs['enable_ceph'] = True
+        if req.api_version_request >= api_version.APIVersionRequest("2.28"):
+            kwargs['allow_on_error_status'] = True
+
+        return self._allow_access(*args, **kwargs)
 
     @wsgi.Controller.api_version('2.0', '2.6')
     @wsgi.action('os-deny_access')

manila/api/views/share_accesses.py

@@ -14,6 +14,8 @@
 #    under the License.
 
 from manila.api import common
+from manila.common import constants
+from manila.share import api as share_api
 
 
 class ViewBuilder(common.ViewBuilder):
@@ -22,6 +24,7 @@ class ViewBuilder(common.ViewBuilder):
     _collection_name = 'share_accesses'
     _detail_version_modifiers = [
         "add_access_key",
+        "translate_transitional_statuses",
     ]
 
     def list_view(self, request, accesses):
@@ -59,3 +62,15 @@ class ViewBuilder(common.ViewBuilder):
     @common.ViewBuilder.versioned_method("2.21")
     def add_access_key(self, context, access_dict, access):
         access_dict['access_key'] = access.get('access_key')
+
+    @common.ViewBuilder.versioned_method("1.0", "2.27")
+    def translate_transitional_statuses(self, context, access_dict, access):
+        """In 2.28, the per access rule status was (re)introduced."""
+        api = share_api.API()
+        share = api.get(context, access['share_id'])
+
+        if (share['access_rules_status'] ==
+                constants.SHARE_INSTANCE_RULES_SYNCING):
+            access_dict['state'] = constants.STATUS_NEW
+        else:
+            access_dict['state'] = share['access_rules_status']

manila/api/views/shares.py

@@ -14,6 +14,7 @@
 #    under the License.
 
 from manila.api import common
+from manila.common import constants
 
 
 class ViewBuilder(common.ViewBuilder):
@@ -31,6 +32,7 @@ class ViewBuilder(common.ViewBuilder):
         "add_user_id",
         "add_create_share_from_snapshot_support_field",
         "add_revert_to_snapshot_support_field",
+        "translate_access_rules_status",
     ]
 
     def summary_list(self, request, shares):
@@ -154,6 +156,12 @@ class ViewBuilder(common.ViewBuilder):
         share_dict['revert_to_snapshot_support'] = share.get(
             'revert_to_snapshot_support')
 
+    @common.ViewBuilder.versioned_method("2.10", "2.27")
+    def translate_access_rules_status(self, context, share_dict, share):
+        if (share['access_rules_status'] ==
+                constants.SHARE_INSTANCE_RULES_SYNCING):
+            share_dict['access_rules_status'] = constants.STATUS_OUT_OF_SYNC
+
     def _list_view(self, func, request, shares):
         """Provide a view for a list of shares."""
         shares_list = [func(request, share)['share'] for share in shares]

manila/common/constants.py

@@ -13,18 +13,14 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
-STATUS_NEW = 'new'
+# SHARE AND GENERAL STATUSES
 STATUS_CREATING = 'creating'
 STATUS_DELETING = 'deleting'
 STATUS_DELETED = 'deleted'
 STATUS_ERROR = 'error'
 STATUS_ERROR_DELETING = 'error_deleting'
 STATUS_AVAILABLE = 'available'
-STATUS_ACTIVE = 'active'
 STATUS_INACTIVE = 'inactive'
-STATUS_OUT_OF_SYNC = 'out_of_sync'
-STATUS_UPDATING = 'updating'
-STATUS_UPDATING_MULTIPLE = 'updating_multiple'
 STATUS_MANAGING = 'manage_starting'
 STATUS_MANAGE_ERROR = 'manage_error'
 STATUS_UNMANAGING = 'unmanage_starting'
@@ -44,6 +40,23 @@ STATUS_RESTORING = 'restoring'
 STATUS_REVERTING = 'reverting'
 STATUS_REVERTING_ERROR = 'reverting_error'
 
+# Access rule states
+ACCESS_STATE_QUEUED_TO_APPLY = 'queued_to_apply'
+ACCESS_STATE_QUEUED_TO_DENY = 'queued_to_deny'
+ACCESS_STATE_APPLYING = 'applying'
+ACCESS_STATE_DENYING = 'denying'
+ACCESS_STATE_ACTIVE = 'active'
+ACCESS_STATE_ERROR = 'error'
+
+# Share instance "access_rules_status" field values
+SHARE_INSTANCE_RULES_SYNCING = 'syncing'
+SHARE_INSTANCE_RULES_ERROR = 'error'
+
+# States/statuses for multiple resources
+STATUS_NEW = 'new'
+STATUS_OUT_OF_SYNC = 'out_of_sync'
+STATUS_ACTIVE = 'active'
+
 TASK_STATE_MIGRATION_STARTING = 'migration_starting'
 TASK_STATE_MIGRATION_IN_PROGRESS = 'migration_in_progress'
 TASK_STATE_MIGRATION_COMPLETING = 'migration_completing'
@@ -87,9 +100,8 @@ TRANSITIONAL_STATUSES = (
     STATUS_RESTORING, STATUS_REVERTING,
 )
 
-UPDATING_RULES_STATUSES = (
-    STATUS_UPDATING,
-    STATUS_UPDATING_MULTIPLE,
+INVALID_SHARE_INSTANCE_STATUSES_FOR_ACCESS_RULE_UPDATES = (
+    TRANSITIONAL_STATUSES + (STATUS_ERROR,)
 )
 
 SUPPORTED_SHARE_PROTOCOLS = (

manila/data/helper.py

@@ -22,6 +22,7 @@ from oslo_log import log
 from manila.common import constants
 from manila import exception
 from manila.i18n import _, _LW
+from manila.share import access as access_manager
 from manila.share import rpcapi as share_rpc
 from manila import utils
 
@@ -66,14 +67,13 @@ class DataServiceHelper(object):
         self.share = share
         self.context = context
         self.share_rpc = share_rpc.ShareAPI()
+        self.access_helper = access_manager.ShareInstanceAccess(self.db, None)
         self.wait_access_rules_timeout = (
             CONF.data_access_wait_access_rules_timeout)
 
     def deny_access_to_data_service(self, access_ref_list, share_instance):
-
-        for access_ref in access_ref_list:
         self._change_data_access_to_instance(
-                share_instance, access_ref, allow=False)
+            share_instance, access_ref_list, deny=True)
 
     # NOTE(ganso): Cleanup methods do not throw exceptions, since the
     # exceptions that should be thrown are the ones that call the cleanup
@@ -114,16 +114,22 @@ class DataServiceHelper(object):
                                 'instance_id': share_instance_id,
                                 'share_id': self.share['id']})
 
-    def _change_data_access_to_instance(
-            self, instance, access_ref, allow=False):
+    def _change_data_access_to_instance(self, instance, accesses, deny=False):
+        if not isinstance(accesses, list):
+            accesses = [accesses]
 
-        self.db.share_instance_update_access_status(
-            self.context, instance['id'], constants.STATUS_OUT_OF_SYNC)
+        self.access_helper.get_and_update_share_instance_access_rules_status(
+            self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING,
+            share_instance_id=instance['id'])
 
-        if allow:
-            self.share_rpc.allow_access(self.context, instance, access_ref)
-        else:
-            self.share_rpc.deny_access(self.context, instance, access_ref)
+        if deny:
+            access_filters = {'access_id': [a['id'] for a in accesses]}
+            updates = {'state': constants.ACCESS_STATE_QUEUED_TO_DENY}
+            self.access_helper.get_and_update_share_instance_access_rules(
+                self.context, filters=access_filters, updates=updates,
+                share_instance_id=instance['id'])
+
+        self.share_rpc.update_access(self.context, instance)
 
         utils.wait_for_access_update(
             self.context, self.db, instance, self.wait_access_rules_timeout)
@@ -166,20 +172,19 @@ class DataServiceHelper(object):
                 self.context, self.share['id'], access['access_type'],
                 access['access_to'])
 
-            for old_access in old_access_list:
-                self._change_data_access_to_instance(
-                    share_instance, old_access, allow=False)
-
+            # Create new access rule and deny all old ones corresponding to
+            # the mapping. Since this is a bulk update, all access changes
+            # are made in one go.
             access_ref = self.db.share_instance_access_create(
                 self.context, values, share_instance['id'])
             self._change_data_access_to_instance(
-                share_instance, access_ref, allow=True)
+                share_instance, old_access_list, deny=True)
 
             if allow_access_to_destination_instance:
                 access_ref = self.db.share_instance_access_create(
                     self.context, values, dest_share_instance['id'])
                 self._change_data_access_to_instance(
-                    dest_share_instance, access_ref, allow=True)
+                    dest_share_instance, access_ref)
 
             access_ref_list.append(access_ref)
 

manila/db/api.py

@@ -392,6 +392,31 @@ def share_access_create(context, values):
     return IMPL.share_access_create(context, values)
 
 
+def share_access_get(context, access_id):
+    """Get share access rule."""
+    return IMPL.share_access_get(context, access_id)
+
+
+def share_access_get_all_for_share(context, share_id):
+    """Get all access rules for given share."""
+    return IMPL.share_access_get_all_for_share(context, share_id)
+
+
+def share_access_get_all_for_instance(context, instance_id, filters=None,
+                                      with_share_access_data=True):
+    """Get all access rules related to a certain share instance."""
+    return IMPL.share_access_get_all_for_instance(
+        context, instance_id, filters=filters,
+        with_share_access_data=with_share_access_data)
+
+
+def share_access_get_all_by_type_and_access(context, share_id, access_type,
+                                            access):
+    """Returns share access by given type and access."""
+    return IMPL.share_access_get_all_by_type_and_access(
+        context, share_id, access_type, access)
+
+
 def share_instance_access_create(context, values, share_instance_id):
     """Allow access to share instance."""
     return IMPL.share_instance_access_create(
@@ -407,55 +432,24 @@ def share_instance_access_copy(context, share_id, instance_id):
     return IMPL.share_instance_access_copy(context, share_id, instance_id)
 
 
-def share_access_get(context, access_id):
-    """Get share access rule."""
-    return IMPL.share_access_get(context, access_id)
-
-
-def share_instance_access_get(context, access_id, instance_id):
+def share_instance_access_get(context, access_id, instance_id,
+                              with_share_access_data=True):
     """Get access rule mapping for share instance."""
-    return IMPL.share_instance_access_get(context, access_id, instance_id)
+    return IMPL.share_instance_access_get(
+        context, access_id, instance_id,
+        with_share_access_data=with_share_access_data)
 
 
-def share_access_get_all_for_share(context, share_id):
-    """Get all access rules for given share."""
-    return IMPL.share_access_get_all_for_share(context, share_id)
-
-
-def share_access_update_access_key(context, access_id, access_key):
-    """Update the access_key field of a share access mapping."""
-    return IMPL.share_access_update_access_key(context, access_id, access_key)
-
-
-def share_access_get_all_for_instance(context, instance_id, session=None):
-    """Get all access rules related to a certain share instance."""
-    return IMPL.share_access_get_all_for_instance(
-        context, instance_id, session=None)
-
-
-def share_access_get_all_by_type_and_access(context, share_id, access_type,
-                                            access):
-    """Returns share access by given type and access."""
-    return IMPL.share_access_get_all_by_type_and_access(
-        context, share_id, access_type, access)
-
-
-def share_access_delete(context, access_id):
-    """Deny access to share."""
-    return IMPL.share_access_delete(context, access_id)
+def share_instance_access_update(context, access_id, instance_id, updates):
+    """Update the access mapping row for a given share instance and access."""
+    return IMPL.share_instance_access_update(
+        context, access_id, instance_id, updates)
 
 
 def share_instance_access_delete(context, mapping_id):
     """Deny access to share instance."""
     return IMPL.share_instance_access_delete(context, mapping_id)
 
-
-def share_instance_update_access_status(context, share_instance_id, status):
-    """Update access rules status of share instance."""
-    return IMPL.share_instance_update_access_status(context, share_instance_id,
-                                                    status)
-
-
 ####################
 
 

manila/db/migrations/alembic/versions/344c1ac4747f_add_share_instance_access_rules_status.py

@@ -42,14 +42,6 @@ upgrade_data_mapping = {
     'error': 'error',
 }
 
-downgrade_data_mapping = {
-    'active': 'active',
-    # NOTE(u_glide): We cannot determine is it applied rule or not in Manila,
-    # so administrator should manually handle such access rules.
-    'out_of_sync': 'error',
-    'error': 'error',
-}
-
 
 def upgrade():
     """Transform individual access rules states to 'access_rules_status'.
@@ -120,7 +112,12 @@ def downgrade():
 
     for instance in connection.execute(instances_query):
 
-        state = downgrade_data_mapping[instance['access_rules_status']]
+        # NOTE(u_glide): We cannot determine if a rule is applied or not in
+        # Manila, so administrator should manually handle such access rules.
+        if instance['access_rules_status'] == 'active':
+            state = 'active'
+        else:
+            state = 'error'
 
         op.execute(
             instance_access_table.update().where(

manila/db/migrations/alembic/versions/54667b9cade7_restore_share_instance_access_map_state.py

@@ -0,0 +1,99 @@
+# Licensed under the Apache License, Version 2.0 (the "License"); you may
+# not use this file except in compliance with the License. You may obtain
+# a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+# License for the specific language governing permissions and limitations
+# under the License.
+
+"""add_share_instance_access_map_state
+
+Revision ID: 54667b9cade7
+Revises: 87ce15c59bbe
+Create Date: 2016-09-02 10:18:07.290461
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '54667b9cade7'
+down_revision = '87ce15c59bbe'
+
+from alembic import op
+from sqlalchemy import Column, String
+
+from manila.common import constants
+from manila.db.migrations import utils
+
+
+# Mapping for new value to be assigned as ShareInstanceAccessMapping's state
+access_rules_status_to_state_mapping = {
+    constants.STATUS_ACTIVE: constants.ACCESS_STATE_ACTIVE,
+    constants.STATUS_OUT_OF_SYNC: constants.ACCESS_STATE_QUEUED_TO_APPLY,
+    'updating': constants.ACCESS_STATE_QUEUED_TO_APPLY,
+    'updating_multiple': constants.ACCESS_STATE_QUEUED_TO_APPLY,
+    constants.STATUS_ERROR: constants.ACCESS_STATE_ERROR,
+}
+
+# Mapping for changes to Share Instance's access_rules_status
+access_rules_status_upgrade_mapping = {
+    constants.STATUS_ACTIVE: constants.STATUS_ACTIVE,
+    constants.STATUS_OUT_OF_SYNC: constants.SHARE_INSTANCE_RULES_SYNCING,
+    'updating': constants.SHARE_INSTANCE_RULES_SYNCING,
+    'updating_multiple': constants.SHARE_INSTANCE_RULES_SYNCING,
+    constants.STATUS_ERROR: constants.STATUS_ERROR,
+}
+
+
+def upgrade():
+    op.add_column('share_instance_access_map',
+                  Column('state', String(length=255),
+                         default=constants.ACCESS_STATE_QUEUED_TO_APPLY))
+
+    connection = op.get_bind()
+    share_instances_table = utils.load_table('share_instances', connection)
+    instance_access_map_table = utils.load_table('share_instance_access_map',
+                                                 connection)
+
+    instances_query = (
+        share_instances_table.select().where(
+            share_instances_table.c.status ==
+            constants.STATUS_AVAILABLE).where(
+            share_instances_table.c.deleted == 'False')
+    )
+
+    for instance in connection.execute(instances_query):
+        access_rule_status = instance['access_rules_status']
+        op.execute(
+            instance_access_map_table.update().where(
+                instance_access_map_table.c.share_instance_id == instance['id']
+            ).values({
+                'state': access_rules_status_to_state_mapping[
+                    access_rule_status],
+            })
+        )
+        op.execute(
+            share_instances_table.update().where(
+                share_instances_table.c.id == instance['id']
+            ).values({
+                'access_rules_status': access_rules_status_upgrade_mapping[
+                    access_rule_status],
+            })
+        )
+
+
+def downgrade():
+    op.drop_column('share_instance_access_map', 'state')
+
+    connection = op.get_bind()
+    share_instances_table = utils.load_table('share_instances', connection)
+
+    op.execute(
+        share_instances_table.update().where(
+            share_instances_table.c.access_rules_status ==
+            constants.SHARE_INSTANCE_RULES_SYNCING).values({
+                'access_rules_status': constants.STATUS_OUT_OF_SYNC})
+    )

manila/db/sqlalchemy/api.py

@@ -38,7 +38,6 @@ from oslo_log import log
 from oslo_utils import timeutils
 from oslo_utils import uuidutils
 import six
-from sqlalchemy import and_
 from sqlalchemy import MetaData
 from sqlalchemy import or_
 from sqlalchemy.orm import joinedload
@@ -1084,25 +1083,25 @@ def reservation_expire(context):
 
 ################
 
-def _extract_instance_values(values_to_extract, fields):
-    values = copy.deepcopy(values_to_extract)
-    instance_values = {}
+def _extract_subdict_by_fields(source_dict, fields):
+    dict_to_extract_from = copy.deepcopy(source_dict)
+    sub_dict = {}
     for field in fields:
-        field_value = values.pop(field, None)
+        field_value = dict_to_extract_from.pop(field, None)
         if field_value:
-            instance_values.update({field: field_value})
+            sub_dict.update({field: field_value})
 
-    return instance_values, values
+    return sub_dict, dict_to_extract_from
 
 
 def _extract_share_instance_values(values):
     share_instance_model_fields = [
         'status', 'host', 'scheduled_at', 'launched_at', 'terminated_at',
         'share_server_id', 'share_network_id', 'availability_zone',
-        'replica_state', 'share_type_id', 'share_type',
+        'replica_state', 'share_type_id', 'share_type', 'access_rules_status',
     ]
     share_instance_values, share_values = (
-        _extract_instance_values(values, share_instance_model_fields)
+        _extract_subdict_by_fields(values, share_instance_model_fields)
     )
     return share_instance_values, share_values
 
@@ -1110,7 +1109,7 @@ def _extract_share_instance_values(values):
 def _extract_snapshot_instance_values(values):
     fields = ['status', 'progress', 'provider_location']
     snapshot_instance_values, snapshot_values = (
-        _extract_instance_values(values, fields)
+        _extract_subdict_by_fields(values, fields)
     )
     return snapshot_instance_values, snapshot_values
 
@@ -1756,8 +1755,9 @@ def share_instance_access_copy(context, share_id, instance_id, session=None):
     """Copy access rules from share to share instance."""
     session = session or get_session()
 
-    share_access_rules = share_access_get_all_for_share(
-        context, share_id, session=session)
+    share_access_rules = _share_access_get_query(
+        context, session, {'share_id': share_id}).all()
+
     for access_rule in share_access_rules:
         values = {
             'share_instance_id': instance_id,
@@ -1777,9 +1777,9 @@ def _share_instance_access_create(values, session):
 
 
 @require_context
-def share_access_get(context, access_id):
+def share_access_get(context, access_id, session=None):
     """Get access record."""
-    session = get_session()
+    session = session or get_session()
 
     access = _share_access_get_query(
         context, session, {'id': access_id}).first()
@@ -1790,43 +1790,63 @@ def share_access_get(context, access_id):
 
 
 @require_context
-def share_instance_access_get(context, access_id, instance_id):
+def share_instance_access_get(context, access_id, instance_id,
+                              with_share_access_data=True):
     """Get access record."""
     session = get_session()
 
     access = _share_instance_access_query(context, session, access_id,
                                           instance_id).first()
-    if access:
-        return access
-    else:
+    if access is None:
         raise exception.NotFound()
 
+    if with_share_access_data:
+        access = _set_instances_share_access_data(context, access, session)[0]
+
+    return access
+
 
 @require_context
 def share_access_get_all_for_share(context, share_id, session=None):
     session = session or get_session()
-    return _share_access_get_query(context, session,
-                                   {'share_id': share_id}).all()
+    return _share_access_get_query(
+        context, session, {'share_id': share_id}).filter(
+        models.ShareAccessMapping.instance_mappings.any()).all()
 
 
 @require_context
-def share_access_get_all_for_instance(context, instance_id, session=None):
+def share_access_get_all_for_instance(context, instance_id, filters=None,
+                                      with_share_access_data=True,
+                                      session=None):
     """Get all access rules related to a certain share instance."""
-    session = get_session()
-    return _share_access_get_query(context, session, {}).join(
-        models.ShareInstanceAccessMapping,
-        models.ShareInstanceAccessMapping.access_id ==
-        models.ShareAccessMapping.id).filter(and_(
-            models.ShareInstanceAccessMapping.share_instance_id ==
-            instance_id, models.ShareInstanceAccessMapping.deleted ==
-            "False")).all()
+    session = session or get_session()
+    filters = copy.deepcopy(filters) if filters else {}
+    filters.update({'share_instance_id': instance_id})
+    legal_filter_keys = ('id', 'share_instance_id', 'access_id', 'state')
+    query = _share_instance_access_query(context, session)
+
+    query = exact_filter(
+        query, models.ShareInstanceAccessMapping, filters, legal_filter_keys)
+
+    instance_accesses = query.all()
+
+    if with_share_access_data:
+        instance_accesses = _set_instances_share_access_data(
+            context, instance_accesses, session)
+
+    return instance_accesses
 
 
-@require_context
-def share_instance_access_get_all(context, access_id, session=None):
-    if not session:
-        session = get_session()
-    return _share_instance_access_query(context, session, access_id).all()
+def _set_instances_share_access_data(context, instance_accesses, session):
+    if instance_accesses and not isinstance(instance_accesses, list):
+        instance_accesses = [instance_accesses]
+
+    for instance_access in instance_accesses:
+        share_access = share_access_get(
+            context, instance_access['access_id'], session=session)
+        instance_access.set_share_access_data(share_access)
+
+    return instance_accesses
 
 
 @require_context
@@ -1839,22 +1859,6 @@ def share_access_get_all_by_type_and_access(context, share_id, access_type,
                                     'access_to': access}).all()
 
 
-@require_context
-def share_access_delete(context, access_id):
-    session = get_session()
-
-    with session.begin():
-        mappings = share_instance_access_get_all(context, access_id, session)
-
-        if len(mappings) > 0:
-            msg = (_("Access rule %s has mappings"
-                     " to share instances.") % access_id)
-            raise exception.InvalidShareAccess(msg)
-
-        session.query(models.ShareAccessMapping).\
-            filter_by(id=access_id).soft_delete()
-
-
 @require_context
 def share_access_delete_all_by_share(context, share_id):
     session = get_session()
@@ -1863,17 +1867,6 @@ def share_access_delete_all_by_share(context, share_id):
             filter_by(share_id=share_id).soft_delete()
 
 
-@require_context
-def share_access_update_access_key(context, access_id, access_key):
-    session = get_session()
-    with session.begin():
-        mapping = (session.query(models.ShareAccessMapping).
-                   filter_by(id=access_id).first())
-        mapping.update({'access_key': access_key})
-        mapping.save(session=session)
-        return mapping
-
-
 @require_context
 def share_instance_access_delete(context, mapping_id):
     session = get_session()
@@ -1885,10 +1878,11 @@ def share_instance_access_delete(context, mapping_id):
         if not mapping:
             exception.NotFound()
 
-        mapping.soft_delete(session)
+        mapping.soft_delete(session, update_status=True,
+                            status_field_name='state')
 
-        other_mappings = share_instance_access_get_all(
-            context, mapping['access_id'], session)
+        other_mappings = _share_instance_access_query(
+            context, session, mapping['access_id']).all()
 
         # NOTE(u_glide): Remove access rule if all mappings were removed.
         if len(other_mappings) == 0:
@@ -1901,15 +1895,27 @@ def share_instance_access_delete(context, mapping_id):
 
 @require_context
 @oslo_db_api.wrap_db_retry(max_retries=5, retry_on_deadlock=True)
-def share_instance_update_access_status(context, share_instance_id, status):
+def share_instance_access_update(context, access_id, instance_id, updates):
     session = get_session()
-    with session.begin():
-        mapping = session.query(models.ShareInstance).\
-            filter_by(id=share_instance_id).first()
-        mapping.update({'access_rules_status': status})
-        mapping.save(session=session)
-        return mapping
+    share_access_fields = ('access_type', 'access_to', 'access_key',
+                           'access_level')
 
+    share_access_map_updates, share_instance_access_map_updates = (
+        _extract_subdict_by_fields(updates, share_access_fields)
+    )
+
+    with session.begin():
+        share_access = _share_access_get_query(
+            context, session, {'id': access_id}).first()
+        share_access.update(share_access_map_updates)
+        share_access.save(session=session)
+
+        access = _share_instance_access_query(
+            context, session, access_id, instance_id).first()
+        access.update(share_instance_access_map_updates)
+        access.save(session=session)
+
+        return access
 
 ###################
 

manila/db/sqlalchemy/models.py

@@ -364,17 +364,13 @@ class ShareInstance(BASE, ManilaBase):
 
     ACCESS_STATUS_PRIORITIES = {
         constants.STATUS_ACTIVE: 0,
-        constants.STATUS_OUT_OF_SYNC: 1,
-        constants.STATUS_UPDATING: 2,
-        constants.STATUS_UPDATING_MULTIPLE: 3,
-        constants.STATUS_ERROR: 4,
+        constants.SHARE_INSTANCE_RULES_SYNCING: 1,
+        constants.SHARE_INSTANCE_RULES_ERROR: 2,
     }
 
     access_rules_status = Column(Enum(constants.STATUS_ACTIVE,
-                                      constants.STATUS_OUT_OF_SYNC,
-                                      constants.STATUS_UPDATING,
-                                      constants.STATUS_UPDATING_MULTIPLE,
-                                      constants.STATUS_ERROR),
+                                      constants.SHARE_INSTANCE_RULES_SYNCING,
+                                      constants.SHARE_INSTANCE_RULES_ERROR),
                                  default=constants.STATUS_ACTIVE)
 
     scheduled_at = Column(DateTime)
@@ -546,6 +542,28 @@ class ShareAccessMapping(BASE, ManilaBase):
     access_level = Column(Enum(*constants.ACCESS_LEVELS),
                           default=constants.ACCESS_LEVEL_RW)
 
+    @property
+    def state(self):
+        """Get the aggregated 'state' from all the instance mapping states.
+
+        An access rule is supposed to be truly 'active' when it has been
+        applied across all of the share instances of the parent share object.
+        """
+        state = None
+        if len(self.instance_mappings) > 0:
+            order = (constants.ACCESS_STATE_ERROR,
+                     constants.ACCESS_STATE_DENYING,
+                     constants.ACCESS_STATE_QUEUED_TO_DENY,
+                     constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                     constants.ACCESS_STATE_APPLYING,
+                     constants.ACCESS_STATE_ACTIVE)
+
+            sorted_instance_mappings = sorted(
+                self.instance_mappings, key=lambda x: order.index(x['state']))
+
+            state = sorted_instance_mappings[0].state
+        return state
+
     instance_mappings = orm.relationship(
         "ShareInstanceAccessMapping",
         lazy='immediate',
@@ -557,28 +575,23 @@ class ShareAccessMapping(BASE, ManilaBase):
         )
     )
 
-    @property
-    def state(self):
-        instances = [im.instance for im in self.instance_mappings]
-        access_rules_status = get_access_rules_status(instances)
-
-        if access_rules_status in (
-                constants.STATUS_OUT_OF_SYNC,
-                constants.STATUS_UPDATING,
-                constants.STATUS_UPDATING_MULTIPLE):
-            return constants.STATUS_NEW
-        else:
-            return access_rules_status
-
 
 class ShareInstanceAccessMapping(BASE, ManilaBase):
     """Represents access to individual share instances."""
 
     __tablename__ = 'share_instance_access_map'
+    _proxified_properties = ('share_id', 'access_type', 'access_key',
+                             'access_to', 'access_level')
+
+    def set_share_access_data(self, share_access):
+        for share_access_attr in self._proxified_properties:
+            setattr(self, share_access_attr, share_access[share_access_attr])
+
     id = Column(String(36), primary_key=True)
     deleted = Column(String(36), default='False')
     share_instance_id = Column(String(36), ForeignKey('share_instances.id'))
     access_id = Column(String(36), ForeignKey('share_access_map.id'))
+    state = Column(String(255), default=constants.ACCESS_STATE_QUEUED_TO_APPLY)
 
     instance = orm.relationship(
         "ShareInstance",
@@ -1019,7 +1032,7 @@ def get_access_rules_status(instances):
                 share_access_status):
             share_access_status = instance_access_status
 
-        if share_access_status == constants.STATUS_ERROR:
+        if share_access_status == constants.SHARE_INSTANCE_RULES_ERROR:
             break
 
     return share_access_status

manila/share/access.py

@@ -13,109 +13,436 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import copy
+
 from oslo_log import log
-import six
 
 from manila.common import constants
-from manila import exception
 from manila.i18n import _, _LI
 from manila import utils
 
 LOG = log.getLogger(__name__)
 
 
-class ShareInstanceAccess(object):
+def locked_access_rules_operation(operation):
+    """Lock decorator for access rules operations.
+
+    Takes a named lock prior to executing the operation. The lock is
+    named with the ID of the share instance to which the access rule belongs.
+
+    Intended use:
+    If an database operation to retrieve or update access rules uses this
+    decorator, it will block actions on all access rules of the share
+    instance until the named lock is free. This is used to avoid race
+    conditions while performing access rules updates on a given share instance.
+    """
+
+    def wrapped(*args, **kwargs):
+        instance_id = kwargs.get('share_instance_id')
+
+        @utils.synchronized(
+            "locked_access_rules_operation_by_share_instance_%s" % instance_id,
+            external=True)
+        def locked_operation(*_args, **_kwargs):
+            return operation(*_args, **_kwargs)
+
+        return locked_operation(*args, **kwargs)
+
+    return wrapped
+
+
+class ShareInstanceAccessDatabaseMixin(object):
+
+    @locked_access_rules_operation
+    def get_and_update_share_instance_access_rules_status(
+            self, context, status=None, conditionally_change={},
+            share_instance_id=None):
+        """Get and update the access_rules_status of a share instance.
+
+        :param status: Set this parameter only if you want to
+            omit the conditionally_change parameter; i.e, if you want to
+            force a state change on the share instance regardless of the prior
+            state.
+        :param conditionally_change: Set this parameter to a dictionary of rule
+            state transitions to be made. The key is the expected
+            access_rules_status and the value is the state to transition the
+            access_rules_status to. If the state is not as expected,
+            no transition is performed. Default is {}, which means no state
+            transitions will be made.
+        :returns share_instance: if an update was made.
+        """
+        if status is not None:
+            updates = {'access_rules_status': status}
+        else:
+            share_instance = self.db.share_instance_get(
+                context, share_instance_id)
+            access_rules_status = share_instance['access_rules_status']
+            try:
+                updates = {
+                    'access_rules_status':
+                        conditionally_change[access_rules_status],
+                }
+            except KeyError:
+                updates = {}
+        if updates:
+            share_instance = self.db.share_instance_update(
+                context, share_instance_id, updates, with_share_data=True)
+            return share_instance
+
+    @locked_access_rules_operation
+    def get_and_update_share_instance_access_rules(self, context,
+                                                   filters={}, updates={},
+                                                   conditionally_change={},
+                                                   share_instance_id=None):
+        """Get and conditionally update all access rules of a share instance.
+
+        :param updates: Set this parameter to a dictionary of key:value
+            pairs corresponding to the keys in the ShareInstanceAccessMapping
+            model. Include 'state' in this dictionary only if you want to
+            omit the conditionally_change parameter; i.e, if you want to
+            force a state change on all filtered rules regardless of the prior
+            state. This parameter is always honored, regardless of whether
+            conditionally_change allows for a state transition as desired.
+
+            Example::
+
+            {
+                'access_key': 'bob007680048318f4239dfc1c192d5',
+                'access_level': 'ro',
+            }
+
+        :param conditionally_change: Set this parameter to a dictionary of rule
+            state transitions to be made. The key is the expected state of
+            the access rule the value is the state to transition the
+            access rule to. If the state is not as expected, no transition is
+            performed. Default is {}, which means no state transitions
+            will be made.
+
+            Example::
+
+            {
+                'queued_to_apply': 'applying',
+                'queued_to_deny': 'denying',
+            }
+
+        """
+        instance_rules = self.db.share_access_get_all_for_instance(
+            context, share_instance_id, filters=filters)
+
+        if instance_rules and (updates or conditionally_change):
+            for rule in instance_rules:
+                mapping_state = rule['state']
+                rule_updates = copy.deepcopy(updates)
+                try:
+                    rule_updates['state'] = conditionally_change[mapping_state]
+                except KeyError:
+                    pass
+                if rule_updates:
+                    self.db.share_instance_access_update(
+                        context, rule['access_id'], share_instance_id,
+                        rule_updates)
+
+            # Refresh the rules after the updates
+            rules_to_get = {
+                'access_id': tuple([i['access_id'] for i in instance_rules]),
+            }
+            instance_rules = self.db.share_access_get_all_for_instance(
+                context, share_instance_id, filters=rules_to_get)
+
+        return instance_rules
+
+    @locked_access_rules_operation
+    def get_and_update_share_instance_access_rule(self, context, rule_id,
+                                                  updates={},
+                                                  share_instance_id=None,
+                                                  conditionally_change={}):
+        """Get and conditionally update a given share instance access rule.
+
+        :param updates: Set this parameter to a dictionary of key:value
+            pairs corresponding to the keys in the ShareInstanceAccessMapping
+            model. Include 'state' in this dictionary only if you want to
+            omit the conditionally_change parameter; i.e, if you want to
+            force a state change regardless of the prior state.
+        :param conditionally_change: Set this parameter to a dictionary of rule
+            state transitions to be made. The key is the expected state of
+            the access rule the value is the state to transition the
+            access rule to. If the state is not as expected, no transition is
+            performed. Default is {}, which means no state transitions
+            will be made.
+
+            Example::
+
+            {
+                'queued_to_apply': 'applying',
+                'queued_to_deny': 'denying',
+            }
+        """
+        instance_rule_mapping = self.db.share_instance_access_get(
+            context, rule_id, share_instance_id)
+
+        if conditionally_change:
+            mapping_state = instance_rule_mapping['state']
+            try:
+                updated_state = conditionally_change[mapping_state]
+                updates.update({'state': updated_state})
+            except KeyError:
+                msg = ("The state of the access rule %(rule_id)s (allowing "
+                       "access to share instance %(si)s) was not updated "
+                       "because its state was modified by another operation.")
+                msg_payload = {
+                    'si': share_instance_id,
+                    'rule_id': rule_id,
+                }
+                LOG.debug(msg, msg_payload)
+        if updates:
+            self.db.share_instance_access_update(
+                context, rule_id, share_instance_id, updates)
+
+            # Refresh the rule after update
+            instance_rule_mapping = self.db.share_instance_access_get(
+                context, rule_id, share_instance_id)
+
+        return instance_rule_mapping
+
+    @locked_access_rules_operation
+    def delete_share_instance_access_rules(self, context, access_rules,
+                                           share_instance_id=None):
+        for rule in access_rules:
+            self.db.share_instance_access_delete(context, rule['id'])
+
+
+class ShareInstanceAccess(ShareInstanceAccessDatabaseMixin):
 
     def __init__(self, db, driver):
         self.db = db
         self.driver = driver
 
-    def update_access_rules(self, context, share_instance_id, add_rules=None,
-                            delete_rules=None, share_server=None):
-        """Update access rules in driver and database for given share instance.
+    def update_access_rules(self, context, share_instance_id,
+                            delete_all_rules=False, share_server=None):
+        """Update access rules for a given share instance.
 
-        :param context: current context
-        :param share_instance_id: Id of the share instance model
-        :param add_rules: list with ShareAccessMapping models or None - rules
-        which should be added
-        :param delete_rules: list with ShareAccessMapping models, "all", None
-        - rules which should be deleted. If "all" is provided - all rules will
-        be deleted.
+        :param context: request context
+        :param share_instance_id: ID of the share instance
+        :param delete_all_rules: set this parameter to True if all
+        existing access rules must be denied for a given share instance
         :param share_server: Share server model or None
         """
         share_instance = self.db.share_instance_get(
             context, share_instance_id, with_share_data=True)
-        share_id = share_instance["share_id"]
+        msg_payload = {
+            'si': share_instance_id,
+            'shr': share_instance['share_id'],
+        }
 
-        @utils.synchronized(
-            "update_access_rules_for_share_%s" % share_id, external=True)
-        def _update_access_rules_locked(*args, **kwargs):
-            return self._update_access_rules(*args, **kwargs)
+        if delete_all_rules:
+            updates = {
+                'state': constants.ACCESS_STATE_QUEUED_TO_DENY,
+            }
+            self.get_and_update_share_instance_access_rules(
+                context, updates=updates, share_instance_id=share_instance_id)
 
-        _update_access_rules_locked(
-            context=context,
-            share_instance_id=share_instance_id,
-            add_rules=add_rules,
-            delete_rules=delete_rules,
-            share_server=share_server,
+        # Is there a sync in progress? If yes, ignore the incoming request.
+        rule_filter = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        syncing_rules = self.get_and_update_share_instance_access_rules(
+            context, filters=rule_filter, share_instance_id=share_instance_id)
+
+        if syncing_rules:
+            msg = ("Access rules are being synced for share instance "
+                   "%(si)s belonging to share %(shr)s, any rule changes will "
+                   "be applied shortly.")
+            LOG.debug(msg, msg_payload)
+        else:
+            rules_to_apply_or_deny = (
+                self._update_and_get_unsynced_access_rules_from_db(
+                    context, share_instance_id)
             )
+            if rules_to_apply_or_deny:
+                msg = ("Updating access rules for share instance %(si)s "
+                       "belonging to share %(shr)s.")
+                LOG.debug(msg, msg_payload)
+                self._update_access_rules(context, share_instance_id,
+                                          share_server=share_server)
+            else:
+                msg = ("All access rules have been synced for share instance "
+                       "%(si)s belonging to share %(shr)s.")
+                LOG.debug(msg, msg_payload)
 
-    def _update_access_rules(self, context, share_instance_id, add_rules=None,
-                             delete_rules=None, share_server=None):
-        # Reget share instance
+    def _update_access_rules(self, context, share_instance_id,
+                             share_server=None):
+        # Refresh the share instance model
         share_instance = self.db.share_instance_get(
             context, share_instance_id, with_share_data=True)
 
-        # NOTE (rraja): preserve error state to trigger maintenance mode
-        if share_instance['access_rules_status'] != constants.STATUS_ERROR:
-            self.db.share_instance_update_access_status(
-                context,
-                share_instance_id,
-                constants.STATUS_UPDATING)
+        conditionally_change = {
+            constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING,
+        }
+        share_instance = (
+            self.get_and_update_share_instance_access_rules_status(
+                context, conditionally_change=conditionally_change,
+                share_instance_id=share_instance_id) or share_instance
+        )
 
-        add_rules = add_rules or []
-        delete_rules = delete_rules or []
-        remove_rules = None
-
-        if six.text_type(delete_rules).lower() == "all":
-            # NOTE(ganso): if we are deleting an instance or clearing all
-            # the rules, we want to remove only the ones related
-            # to this instance.
-            delete_rules = self.db.share_access_get_all_for_instance(
-                context, share_instance['id'])
-            rules = []
-        else:
-            _rules = self.db.share_access_get_all_for_instance(
-                context, share_instance['id'])
-            rules = _rules
-            if delete_rules:
-                delete_ids = [rule['id'] for rule in delete_rules]
-                rules = list(filter(lambda r: r['id'] not in delete_ids,
-                                    rules))
-                # NOTE(ganso): trigger maintenance mode
-                if share_instance['access_rules_status'] == (
-                        constants.STATUS_ERROR):
-                    remove_rules = [
-                        rule for rule in _rules
-                        if rule["id"] in delete_ids]
-                    delete_rules = []
-
-        # NOTE(ganso): up to here we are certain of the rules that we are
-        # supposed to pass to drivers. 'rules' variable is used for validating
-        # the refresh mechanism later, according to the 'supposed' rules.
-        driver_rules = rules
+        rules_to_be_removed_from_db = []
+        # Populate rules to send to the driver
+        (access_rules_to_be_on_share, add_rules, delete_rules) = (
+            self._get_rules_to_send_to_driver(context, share_instance)
+        )
 
         if share_instance['status'] == constants.STATUS_MIGRATING:
+            # Ensure read/only semantics for a migrating instances
+            access_rules_to_be_on_share = self._set_rules_to_readonly(
+                add_rules, access_rules_to_be_on_share, share_instance)
+            add_rules = []
+            rules_to_be_removed_from_db = delete_rules
+            delete_rules = []
+
+        try:
+            driver_rule_updates = self._update_rules_through_share_driver(
+                context, share_instance, access_rules_to_be_on_share,
+                add_rules, delete_rules, rules_to_be_removed_from_db,
+                share_server)
+
+            self._process_driver_rule_updates(
+                context, driver_rule_updates, share_instance_id)
+
+            # Update access rules that are still in 'applying' state
+            conditionally_change = {
+                constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE,
+            }
+            self.get_and_update_share_instance_access_rules(
+                context, share_instance_id=share_instance_id,
+                conditionally_change=conditionally_change)
+
+        except Exception:
+            conditionally_change_rule_state = {
+                constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ERROR,
+                constants.ACCESS_STATE_DENYING: constants.ACCESS_STATE_ERROR,
+            }
+            self.get_and_update_share_instance_access_rules(
+                context, share_instance_id=share_instance_id,
+                conditionally_change=conditionally_change_rule_state)
+
+            conditionally_change_access_rules_status = {
+                constants.ACCESS_STATE_ACTIVE: constants.STATUS_ERROR,
+                constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_ERROR,
+            }
+            self.get_and_update_share_instance_access_rules_status(
+                context, share_instance_id=share_instance_id,
+                conditionally_change=conditionally_change_access_rules_status)
+            raise
+
+        if rules_to_be_removed_from_db:
+            delete_rules = rules_to_be_removed_from_db
+
+        self.delete_share_instance_access_rules(
+            context, delete_rules, share_instance_id=share_instance['id'])
+
+        self._loop_for_refresh_else_update_access_rules_status(
+            context, share_instance_id, share_server)
+
+        msg = _("Access rules were successfully modified for share instance "
+                "%(si)s belonging to share %(shr)s.")
+        msg_payload = {
+            'si': share_instance['id'],
+            'shr': share_instance['share_id'],
+        }
+        LOG.info(msg, msg_payload)
+
+    def _update_rules_through_share_driver(self, context, share_instance,
+                                           access_rules_to_be_on_share,
+                                           add_rules, delete_rules,
+                                           rules_to_be_removed_from_db,
+                                           share_server):
+        driver_rule_updates = {}
+        try:
+            driver_rule_updates = self.driver.update_access(
+                context,
+                share_instance,
+                access_rules_to_be_on_share,
+                add_rules=add_rules,
+                delete_rules=delete_rules,
+                share_server=share_server
+            ) or {}
+        except NotImplementedError:
+            # NOTE(u_glide): Fallback to legacy allow_access/deny_access
+            # for drivers without update_access() method support
+            self._update_access_fallback(context, add_rules, delete_rules,
+                                         rules_to_be_removed_from_db,
+                                         share_instance,
+                                         share_server)
+        return driver_rule_updates
+
+    def _loop_for_refresh_else_update_access_rules_status(self, context,
+                                                          share_instance_id,
+                                                          share_server):
+        # Do we need to re-sync or apply any new changes?
+        if self._check_needs_refresh(context, share_instance_id):
+            self._update_access_rules(context, share_instance_id,
+                                      share_server=share_server)
+        else:
+            # Switch the share instance's access_rules_status to 'active'
+            # if there are no more rules in 'error' state, else, ensure
+            # 'error' state.
+            rule_filter = {'state': constants.STATUS_ERROR}
+            rules_in_error_state = (
+                self.get_and_update_share_instance_access_rules(
+                    context, filters=rule_filter,
+                    share_instance_id=share_instance_id)
+            )
+            if not rules_in_error_state:
+                conditionally_change = {
+                    constants.SHARE_INSTANCE_RULES_SYNCING:
+                        constants.STATUS_ACTIVE,
+                    constants.SHARE_INSTANCE_RULES_ERROR:
+                        constants.STATUS_ACTIVE,
+                }
+                self.get_and_update_share_instance_access_rules_status(
+                    context, conditionally_change=conditionally_change,
+                    share_instance_id=share_instance_id)
+            else:
+                conditionally_change = {
+                    constants.SHARE_INSTANCE_RULES_SYNCING:
+                        constants.SHARE_INSTANCE_RULES_ERROR,
+                }
+                self.get_and_update_share_instance_access_rules_status(
+                    context, conditionally_change=conditionally_change,
+                    share_instance_id=share_instance_id)
+
+    def _process_driver_rule_updates(self, context, driver_rule_updates,
+                                     share_instance_id):
+        for rule_id, rule_updates in driver_rule_updates.items():
+            if 'state' in rule_updates:
+                # We allow updates *only* if the state is unchanged from
+                # the time this update was initiated. It is possible
+                # that the access rule was denied at the API prior to
+                # the driver reporting that the access rule was added
+                # successfully.
+                state = rule_updates.pop('state')
+                conditional_state_updates = {
+                    constants.ACCESS_STATE_APPLYING: state,
+                    constants.ACCESS_STATE_DENYING: state,
+                    constants.ACCESS_STATE_ACTIVE: state,
+                }
+            else:
+                conditional_state_updates = {}
+            self.get_and_update_share_instance_access_rule(
+                context, rule_id, updates=rule_updates,
+                share_instance_id=share_instance_id,
+                conditionally_change=conditional_state_updates)
+
+    def _set_rules_to_readonly(self, add_rules, access_rules_to_be_on_share,
+                               share_instance):
         # NOTE(ganso): If the share instance is the source in a migration,
         # it should have all its rules cast to read-only.
-
         readonly_support = self.driver.configuration.safe_get(
             'migration_readonly_rules_support')
-
         # NOTE(ganso): If the backend supports read-only rules, then all
         # rules are cast to read-only here and passed to drivers.
         if readonly_support:
-                for rule in driver_rules + add_rules:
+            for rule in access_rules_to_be_on_share:
                 rule['access_level'] = constants.ACCESS_LEVEL_RO
             LOG.debug("All access rules of share instance %s are being "
                       "cast to read-only for migration.",
@@ -128,103 +455,40 @@ class ShareInstanceAccess(object):
                       "instance %s are being removed for migration, as "
                       "driver does not support read-only mode rules.",
                       share_instance['id'])
-                driver_rules = add_rules
+            access_rules_to_be_on_share = add_rules
+        return access_rules_to_be_on_share
 
-        try:
-            access_keys = None
-            try:
-                access_keys = self.driver.update_access(
-                    context,
-                    share_instance,
-                    driver_rules,
-                    add_rules=add_rules,
-                    delete_rules=delete_rules,
-                    share_server=share_server
+    def _get_rules_to_send_to_driver(self, context, share_instance):
+        add_rules = []
+        delete_rules = []
+        access_filters = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_ACTIVE,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        existing_rules_in_db = self.get_and_update_share_instance_access_rules(
+            context, filters=access_filters,
+            share_instance_id=share_instance['id'])
+        # Update queued rules to transitional states
+        for rule in existing_rules_in_db:
+            if rule['state'] == constants.ACCESS_STATE_APPLYING:
+                add_rules.append(rule)
+            elif rule['state'] == constants.ACCESS_STATE_DENYING:
+                delete_rules.append(rule)
+        delete_rule_ids = [r['id'] for r in delete_rules]
+        access_rules_to_be_on_share = [
+            r for r in existing_rules_in_db if r['id'] not in delete_rule_ids
+        ]
+        return access_rules_to_be_on_share, add_rules, delete_rules
+
+    def _check_needs_refresh(self, context, share_instance_id):
+        rules_to_apply_or_deny = (
+            self._update_and_get_unsynced_access_rules_from_db(
+                context, share_instance_id)
         )
-            except NotImplementedError:
-                # NOTE(u_glide): Fallback to legacy allow_access/deny_access
-                # for drivers without update_access() method support
+        return any(rules_to_apply_or_deny)
 
-                self._update_access_fallback(add_rules, context, delete_rules,
-                                             remove_rules, share_instance,
-                                             share_server)
-
-            if access_keys:
-                self._validate_access_keys(rules, add_rules, delete_rules,
-                                           access_keys)
-
-                for access_id, access_key in access_keys.items():
-                    self.db.share_access_update_access_key(
-                        context, access_id, access_key)
-
-        except Exception:
-            self.db.share_instance_update_access_status(
-                context,
-                share_instance['id'],
-                constants.STATUS_ERROR)
-            raise
-
-        # NOTE(ganso): remove rules after maintenance is complete
-        if remove_rules:
-            delete_rules = remove_rules
-
-        self._remove_access_rules(context, delete_rules, share_instance['id'])
-
-        share_instance = self.db.share_instance_get(context, share_instance_id,
-                                                    with_share_data=True)
-
-        if self._check_needs_refresh(context, rules, share_instance):
-            self._update_access_rules(context, share_instance_id,
-                                      share_server=share_server)
-        else:
-            self.db.share_instance_update_access_status(
-                context,
-                share_instance['id'],
-                constants.STATUS_ACTIVE
-            )
-
-            LOG.info(_LI("Access rules were successfully applied for "
-                         "share instance: %s"),
-                     share_instance['id'])
-
-    @staticmethod
-    def _validate_access_keys(access_rules, add_rules, delete_rules,
-                              access_keys):
-        if not isinstance(access_keys, dict):
-            msg = _("The access keys must be supplied as a dictionary that "
-                    "maps rule IDs to access keys.")
-            raise exception.Invalid(message=msg)
-
-        actual_rule_ids = sorted(access_keys)
-        expected_rule_ids = []
-        if not (add_rules or delete_rules):
-            expected_rule_ids = [rule['id'] for rule in access_rules]
-        else:
-            expected_rule_ids = [rule['id'] for rule in add_rules]
-        if actual_rule_ids != sorted(expected_rule_ids):
-            msg = (_("The rule IDs supplied: %(actual)s do not match the "
-                     "rule IDs that are expected: %(expected)s.")
-                   % {'actual': actual_rule_ids,
-                      'expected': expected_rule_ids})
-            raise exception.Invalid(message=msg)
-
-        for access_key in access_keys.values():
-            if not isinstance(access_key, six.string_types):
-                msg = (_("Access key %s is not string type.") % access_key)
-                raise exception.Invalid(message=msg)
-
-    def _check_needs_refresh(self, context, rules, share_instance):
-        rule_ids = set([rule['id'] for rule in rules])
-        queried_rules = self.db.share_access_get_all_for_instance(
-            context, share_instance['id'])
-        queried_ids = set([rule['id'] for rule in queried_rules])
-
-        access_rules_status = share_instance['access_rules_status']
-
-        return (access_rules_status == constants.STATUS_UPDATING_MULTIPLE or
-                rule_ids != queried_ids)
-
-    def _update_access_fallback(self, add_rules, context, delete_rules,
+    def _update_access_fallback(self, context, add_rules, delete_rules,
                                 remove_rules, share_instance, share_server):
         for rule in add_rules:
             LOG.info(
@@ -242,7 +506,8 @@ class ShareInstanceAccess(object):
 
         # NOTE(ganso): Fallback mode temporary compatibility workaround
         if remove_rules:
-            delete_rules = remove_rules
+            delete_rules.extend(remove_rules)
+
         for rule in delete_rules:
             LOG.info(
                 _LI("Denying access rule '%(rule)s' from share "
@@ -257,12 +522,33 @@ class ShareInstanceAccess(object):
                 share_server=share_server
             )
 
-    def _remove_access_rules(self, context, access_rules, share_instance_id):
-        if not access_rules:
-            return
+    def _update_and_get_unsynced_access_rules_from_db(self, context,
+                                                      share_instance_id):
+        rule_filter = {
+            'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                      constants.ACCESS_STATE_QUEUED_TO_DENY),
+        }
+        conditionally_change = {
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_APPLYING,
+            constants.ACCESS_STATE_QUEUED_TO_DENY:
+                constants.ACCESS_STATE_DENYING,
+        }
+        rules_to_apply_or_deny = (
+            self.get_and_update_share_instance_access_rules(
+                context, filters=rule_filter,
+                share_instance_id=share_instance_id,
+                conditionally_change=conditionally_change)
+        )
+        return rules_to_apply_or_deny
 
-        for rule in access_rules:
-            access_mapping = self.db.share_instance_access_get(
-                context, rule['id'], share_instance_id)
-
-            self.db.share_instance_access_delete(context, access_mapping['id'])
+    def reset_applying_rules(self, context, share_instance_id):
+        conditional_updates = {
+            constants.ACCESS_STATE_APPLYING:
+                constants.ACCESS_STATE_QUEUED_TO_APPLY,
+            constants.ACCESS_STATE_DENYING:
+                constants.ACCESS_STATE_QUEUED_TO_DENY,
+        }
+        self.get_and_update_share_instance_access_rules(
+            context, share_instance_id=share_instance_id,
+            conditionally_change=conditional_updates)

manila/share/api.py

@@ -36,6 +36,7 @@ from manila.i18n import _, _LE, _LI, _LW
 from manila import policy
 from manila import quota
 from manila.scheduler import rpcapi as scheduler_rpcapi
+from manila.share import access
 from manila.share import rpcapi as share_rpcapi
 from manila.share import share_types
 from manila.share import utils as share_utils
@@ -61,9 +62,10 @@ class API(base.Base):
     """API for interacting with the share manager."""
 
     def __init__(self, db_driver=None):
+        super(API, self).__init__(db_driver)
         self.scheduler_rpcapi = scheduler_rpcapi.SchedulerAPI()
         self.share_rpcapi = share_rpcapi.ShareAPI()
-        super(API, self).__init__(db_driver)
+        self.access_helper = access.ShareInstanceAccess(self.db, None)
 
     def create(self, context, share_proto, size, name, description,
                snapshot_id=None, availability_zone=None, metadata=None,
@@ -1513,21 +1515,20 @@ class API(base.Base):
         """Get the newest snapshot of a share."""
         return self.db.share_snapshot_get_latest_for_share(context, share_id)
 
+    @staticmethod
+    def _is_invalid_share_instance(instance):
+        return (instance['host'] is None
+                or instance['status'] in constants.
+                INVALID_SHARE_INSTANCE_STATUSES_FOR_ACCESS_RULE_UPDATES)
+
     def allow_access(self, ctx, share, access_type, access_to,
                      access_level=None):
         """Allow access to share."""
-        policy.check_policy(ctx, 'share', 'allow_access')
-        share = self.db.share_get(ctx, share['id'])
-        if share['status'] != constants.STATUS_AVAILABLE:
-            msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
-            raise exception.InvalidShare(reason=msg)
-        values = {
-            'share_id': share['id'],
-            'access_type': access_type,
-            'access_to': access_to,
-            'access_level': access_level,
-        }
 
+        # Access rule validation:
+        if access_level not in constants.ACCESS_LEVELS + (None, ):
+            msg = _("Invalid share access level: %s.") % access_level
+            raise exception.InvalidShareAccess(reason=msg)
         share_access_list = self.db.share_access_get_all_by_type_and_access(
             ctx, share['id'], access_type, access_to)
 
@@ -1535,94 +1536,64 @@ class API(base.Base):
             raise exception.ShareAccessExists(access_type=access_type,
                                               access=access_to)
 
-        if access_level not in constants.ACCESS_LEVELS + (None, ):
-            msg = _("Invalid share access level: %s.") % access_level
-            raise exception.InvalidShareAccess(reason=msg)
+        # Share instance validation
+        if any(instance for instance in share.instances
+               if self._is_invalid_share_instance(instance)):
+            msg = _("New access rules cannot be applied while the share or "
+                    "any of its replicas or migration copies lacks a valid "
+                    "host or is in an invalid state.")
+            raise exception.InvalidShare(message=msg)
+
+        values = {
+            'share_id': share['id'],
+            'access_type': access_type,
+            'access_to': access_to,
+            'access_level': access_level,
+        }
+
         access = self.db.share_access_create(ctx, values)
 
         for share_instance in share.instances:
-            self.allow_access_to_instance(ctx, share_instance, access)
-
-        # NOTE(tpsilva): refreshing share_access model
-        access = self.db.share_access_get(ctx, access['id'])
+            self.allow_access_to_instance(ctx, share_instance)
 
         return access
 
-    def allow_access_to_instance(self, context, share_instance, access):
-        policy.check_policy(context, 'share', 'allow_access')
+    def allow_access_to_instance(self, context, share_instance):
+        self._conditionally_transition_share_instance_access_rules_status(
+            context, share_instance)
+        self.share_rpcapi.update_access(context, share_instance)
 
-        if not share_instance['host']:
-            msg = _("Invalid share instance host: %s") % share_instance['host']
-            raise exception.InvalidShareInstance(reason=msg)
-
-        status = share_instance['access_rules_status']
-
-        if status == constants.STATUS_ERROR:
-            values = {
-                'instance_id': share_instance['id'],
-                'status': status,
-                'valid_status': constants.STATUS_ACTIVE
+    def _conditionally_transition_share_instance_access_rules_status(
+            self, context, share_instance):
+        conditionally_change = {
+            constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING,
         }
-            msg = _("Share instance %(instance_id)s access rules status is: "
-                    "%(status)s. Please remove any incorrect rules to get it "
-                    "back to %(valid_status)s.") % values
-
-            raise exception.InvalidShareInstance(reason=msg)
-        else:
-            if status == constants.STATUS_ACTIVE:
-                self.db.share_instance_update_access_status(
-                    context, share_instance['id'],
-                    constants.STATUS_OUT_OF_SYNC
-                )
-            elif status == constants.STATUS_UPDATING:
-                self.db.share_instance_update_access_status(
-                    context, share_instance['id'],
-                    constants.STATUS_UPDATING_MULTIPLE
-                )
-
-            self.share_rpcapi.allow_access(context, share_instance, access)
+        self.access_helper.get_and_update_share_instance_access_rules_status(
+            context, conditionally_change=conditionally_change,
+            share_instance_id=share_instance['id'])
 
     def deny_access(self, ctx, share, access):
         """Deny access to share."""
-        policy.check_policy(ctx, 'share', 'deny_access')
-        # First check state of the target share
-        share = self.db.share_get(ctx, share['id'])
-        if not (share.instances and share.instance['host']):
-            msg = _("Share doesn't have any instances")
-            raise exception.InvalidShare(reason=msg)
-        if share['status'] != constants.STATUS_AVAILABLE:
-            msg = _("Share status must be %s") % constants.STATUS_AVAILABLE
-            raise exception.InvalidShare(reason=msg)
+
+        if any(instance for instance in share.instances if
+               self._is_invalid_share_instance(instance)):
+            msg = _("Access rules cannot be denied while the share, "
+                    "any of its replicas or migration copies lacks a valid "
+                    "host or is in an invalid state.")
+            raise exception.InvalidShare(message=msg)
 
         for share_instance in share.instances:
-            try:
                 self.deny_access_to_instance(ctx, share_instance, access)
-            except exception.NotFound:
-                LOG.warning(_LW("Access rule %(access_id)s not found "
-                                "for instance %(instance_id)s.") % {
-                    'access_id': access['id'],
-                    'instance_id': share_instance['id']})
 
     def deny_access_to_instance(self, context, share_instance, access):
-        policy.check_policy(context, 'share', 'deny_access')
+        self._conditionally_transition_share_instance_access_rules_status(
+            context, share_instance)
+        updates = {'state': constants.ACCESS_STATE_QUEUED_TO_DENY}
+        self.access_helper.get_and_update_share_instance_access_rule(
+            context, access['id'], updates=updates,
+            share_instance_id=share_instance['id'])
 
-        if not share_instance['host']:
-            msg = _("Invalid share instance host: %s") % share_instance['host']
-            raise exception.InvalidShareInstance(reason=msg)
-
-        status = share_instance['access_rules_status']
-
-        if status != constants.STATUS_ERROR:
-            new_status = constants.STATUS_OUT_OF_SYNC
-
-            if status in constants.UPDATING_RULES_STATUSES:
-                new_status = constants.STATUS_UPDATING_MULTIPLE
-
-            self.db.share_instance_update_access_status(
-                context, share_instance['id'],
-                new_status)
-
-        self.share_rpcapi.deny_access(context, share_instance, access)
+        self.share_rpcapi.update_access(context, share_instance)
 
     def access_get_all(self, context, share):
         """Returns all access rules for share."""

manila/share/driver.py

@@ -607,16 +607,22 @@ class ShareDriver(object):
                       delete_rules, share_server=None):
         """Update access rules for given share.
 
-        Drivers should support 2 different cases in this method:
-        1. Recovery after error - 'access_rules' contains all access_rules,
-        'add_rules' and 'delete_rules' shall be empty. Driver should clear any
-        existent access rules and apply all access rules for given share.
-        This recovery is made at driver start up.
+        ``access_rules`` contains all access_rules that need to be on the
+        share. If the driver can make bulk access rule updates, it can
+        safely ignore the ``add_rules`` and ``delete_rules`` parameters.
 
-        2. Adding/Deleting of several access rules - 'access_rules' contains
-        all access_rules, 'add_rules' and 'delete_rules' contain rules which
-        should be added/deleted. Driver can ignore rules in 'access_rules' and
-        apply only rules from 'add_rules' and 'delete_rules'.
+        If the driver cannot make bulk access rule changes, it can rely on
+        new rules to be present in ``add_rules`` and rules that need to be
+        removed to be present in ``delete_rules``.
+
+        When a rule in ``delete_rules`` was never applied, drivers must not
+        raise an exception, or attempt to set the rule to ``error`` state.
+
+        ``add_rules`` and ``delete_rules`` can be empty lists, in this
+        situation, drivers should ensure that the rules present in
+        ``access_rules`` are the same as those on the back end. One scenario
+        where this situation is forced is when the access_level is changed for
+        all existing rules (share migration and for readable replicas).
 
         Drivers must be mindful of this call for share replicas. When
         'update_access' is called on one of the replicas, the call is likely
@@ -634,20 +640,51 @@ class ShareDriver(object):
 
         :param context: Current context
         :param share: Share model with share data.
-        :param access_rules: All access rules for given share
+        :param access_rules: A list of access rules for given share
         :param add_rules: Empty List or List of access rules which should be
                added. access_rules already contains these rules.
         :param delete_rules: Empty List or List of access rules which should be
                removed. access_rules doesn't contain these rules.
         :param share_server: None or Share server model
-        :returns: None, or a dictionary of ``access_id``, ``access_key`` as
-                  key: value pairs for the rules added, where, ``access_id``
-                  is the UUID (string) of the access rule, and ``access_key``
-                  is the credential (string) of the entity granted access.
-                  During recovery after error, the returned dictionary must
-                  contain ``access_id``, ``access_key`` for all the rules that
-                  the driver is ordered to resync, i.e. rules in the
-                  ``access_rules`` parameter.
+        :returns: None, or a dictionary of updates in the format:
+
+            {
+                '09960614-8574-4e03-89cf-7cf267b0bd08': {
+                        'access_key': 'alice31493e5441b8171d2310d80e37e',
+                        'state': 'error',
+                    },
+                '28f6eabb-4342-486a-a7f4-45688f0c0295': {
+                        'access_key': 'bob0078aa042d5a7325480fd13228b',
+                        'state': 'active',
+                    },
+            }
+
+        The top level keys are 'access_id' fields of the access rules that
+        need to be updated. ``access_key``s are credentials (str) of the
+        entities granted access. Any rule in the ``access_rules`` parameter
+        can be updated.
+
+        .. important::
+
+            Raising an exception in this method will force *all* rules in
+            'applying' and 'denying' states to 'error'.
+
+            An access rule can be set to 'error' state, either explicitly
+            via this return parameter or because of an exception raised in
+            this method. Such an access rule will no longer be sent to the
+            driver on subsequent access rule updates. When users deny that
+            rule however, the driver will be asked to deny access to the
+            client/s represented by the rule. We expect that a
+            rule that was error-ed at the driver should never exist on the
+            back end. So, do not fail the deletion request.
+
+            Also, it is possible that the driver may receive a request to
+            add a rule that is already present on the back end.
+            This can happen if the share manager service goes down
+            while the driver is committing access rule changes. Since we
+            cannot determine if the rule was applied successfully by the driver
+            before the disruption, we will treat all 'applying' transitional
+            rules as new rules and repeat the request.
         """
         raise NotImplementedError()
 

manila/share/drivers/cephfs/cephfs_native.py

@@ -289,7 +289,7 @@ class CephFSNativeDriver(driver.ShareDriver,):
         # backend are in sync.
         for rule in add_rules:
             access_key = self._allow_access(context, share, rule)
-            access_keys.update({rule['id']: access_key})
+            access_keys.update({rule['access_id']: {'access_key': access_key}})
 
         for rule in delete_rules:
             self._deny_access(context, share, rule)

manila/share/drivers/huawei/v3/connection.py

@@ -1810,8 +1810,9 @@ class V3StorageConnection(driver.HuaweiBase):
             'replica_state': common_constants.REPLICA_STATE_ACTIVE,
         }
         new_active_update['access_rules_status'] = (
-            common_constants.STATUS_ACTIVE if updated_new_active_access
-            else common_constants.STATUS_OUT_OF_SYNC)
+            common_constants.STATUS_ACTIVE
+            if updated_new_active_access
+            else common_constants.SHARE_INSTANCE_RULES_SYNCING)
 
         # get replica state for new secondary after switch over
         replica_state = self.replica_mgr.get_replica_state(replica_pair_id)
@@ -1821,7 +1822,8 @@ class V3StorageConnection(driver.HuaweiBase):
             'replica_state': replica_state,
         }
         old_active_update['access_rules_status'] = (
-            common_constants.STATUS_OUT_OF_SYNC if cleared_old_active_access
+            common_constants.SHARE_INSTANCE_RULES_SYNCING
+            if cleared_old_active_access
             else common_constants.STATUS_ACTIVE)
 
         return [new_active_update, old_active_update]

manila/share/drivers/netapp/dataontap/cluster_mode/lib_base.py

@@ -1469,7 +1469,7 @@ class NetAppCmodeFileStorageLibrary(object):
             helper.update_access(replica, share_name, access_rules)
         except Exception:
             new_active_replica['access_rules_status'] = (
-                constants.STATUS_OUT_OF_SYNC)
+                constants.SHARE_INSTANCE_RULES_SYNCING)
         else:
             new_active_replica['access_rules_status'] = constants.STATUS_ACTIVE
 

manila/share/drivers/zfsonlinux/driver.py

@@ -1086,7 +1086,7 @@ class ZFSonLinuxShareDriver(zfs_utils.ExecuteMixin, driver.ShareDriver):
                 'id': r['id'],
                 # NOTE(vponomaryov): access rules will be updated in next
                 # 'sync' operation.
-                'access_rules_status': constants.STATUS_OUT_OF_SYNC,
+                'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING,
             }
             for r in replica_list
         }

manila/share/manager.py

@@ -188,7 +188,7 @@ def add_hooks(f):
 class ShareManager(manager.SchedulerDependentManager):
     """Manages NAS storages."""
 
-    RPC_API_VERSION = '1.13'
+    RPC_API_VERSION = '1.14'
 
     def __init__(self, share_driver=None, service_name=None, *args, **kwargs):
         """Load the driver from args, or from flags."""
@@ -218,6 +218,8 @@ class ShareManager(manager.SchedulerDependentManager):
         )
 
         self.access_helper = access.ShareInstanceAccess(self.db, self.driver)
+        self.migration_wait_access_rules_timeout = (
+            CONF.migration_wait_access_rules_timeout)
 
         self.hooks = []
         self._init_hook_drivers()
@@ -330,10 +332,12 @@ class ShareManager(manager.SchedulerDependentManager):
                 self.db.share_export_locations_update(
                     ctxt, share_instance['id'], export_locations)
 
-            if share_instance['access_rules_status'] == (
-                    constants.STATUS_OUT_OF_SYNC):
-
+            if share_instance['access_rules_status'] != (
+                    constants.STATUS_ACTIVE):
                 try:
+                    # Cast any existing 'applying' rules to 'new'
+                    self.access_helper.reset_applying_rules(
+                        ctxt, share_instance['id'])
                     self.access_helper.update_access_rules(
                         ctxt, share_instance['id'], share_server=share_server)
                 except Exception:
@@ -688,7 +692,8 @@ class ShareManager(manager.SchedulerDependentManager):
         dest_share_instance = self.db.share_instance_get(
             context, dest_share_instance['id'], with_share_data=True)
 
-        helper = migration.ShareMigrationHelper(context, self.db, share_ref)
+        helper = migration.ShareMigrationHelper(
+            context, self.db, share_ref, self.access_helper)
 
         try:
             if dest_share_instance['share_network_id']:
@@ -739,14 +744,8 @@ class ShareManager(manager.SchedulerDependentManager):
                 raise exception.ShareMigrationFailed(reason=msg)
 
             if not compatibility.get('writable'):
-
-                self.db.share_instance_update_access_status(
-                    context, src_share_instance['id'],
-                    constants.STATUS_OUT_OF_SYNC)
-
-                self.access_helper.update_access_rules(
-                    context, src_share_instance['id'],
-                    share_server=share_server)
+                self._cast_access_rules_to_readonly(
+                    context, src_share_instance, share_server)
 
             LOG.debug("Initiating driver migration for share %s.",
                       share_ref['id'])
@@ -780,6 +779,27 @@ class ShareManager(manager.SchedulerDependentManager):
 
         return True
 
+    def _cast_access_rules_to_readonly(self, context, src_share_instance,
+                                       share_server):
+        # Set all 'applying' or 'active' rules to 'queued_to_apply'. Since the
+        # share has a status set to 'migrating', this will render rules
+        # read/only.
+        acceptable_past_states = (constants.ACCESS_STATE_APPLYING,
+                                  constants.ACCESS_STATE_ACTIVE)
+        new_state = constants.ACCESS_STATE_QUEUED_TO_APPLY
+        conditionally_change = {k: new_state for k in acceptable_past_states}
+        self.access_helper.get_and_update_share_instance_access_rules(
+            context, share_instance_id=src_share_instance['id'],
+            conditionally_change=conditionally_change)
+
+        self.access_helper.update_access_rules(
+            context, src_share_instance['id'],
+            share_server=share_server)
+
+        utils.wait_for_access_update(
+            context, self.db, src_share_instance,
+            self.migration_wait_access_rules_timeout)
+
     @periodic_task.periodic_task(
         spacing=CONF.migration_driver_continue_update_interval)
     @utils.require_driver_initialized
@@ -932,17 +952,14 @@ class ShareManager(manager.SchedulerDependentManager):
 
         rpcapi = share_rpcapi.ShareAPI()
 
-        helper = migration.ShareMigrationHelper(context, self.db, share)
+        helper = migration.ShareMigrationHelper(
+            context, self.db, share, self.access_helper)
 
         share_server = self._get_share_server(context.elevated(),
                                               src_share_instance)
 
-        self.db.share_instance_update_access_status(
-            context, src_share_instance['id'],
-            constants.STATUS_OUT_OF_SYNC)
-
-        self.access_helper.update_access_rules(
-            context, src_share_instance['id'], share_server=share_server)
+        self._cast_access_rules_to_readonly(
+            context, src_share_instance, share_server)
 
         try:
             dest_share_instance = helper.create_instance_and_wait(
@@ -957,8 +974,7 @@ class ShareManager(manager.SchedulerDependentManager):
             msg = _("Failed to create instance on destination "
                     "backend during migration of share %s.") % share['id']
             LOG.exception(msg)
-            helper.cleanup_access_rules(src_share_instance, share_server,
-                                        self.driver)
+            helper.cleanup_access_rules(src_share_instance, share_server)
             raise exception.ShareMigrationFailed(reason=msg)
 
         ignore_list = self.driver.configuration.safe_get(
@@ -987,8 +1003,7 @@ class ShareManager(manager.SchedulerDependentManager):
                     "share %s.") % share['id']
             LOG.exception(msg)
             helper.cleanup_new_instance(dest_share_instance)
-            helper.cleanup_access_rules(src_share_instance, share_server,
-                                        self.driver)
+            helper.cleanup_access_rules(src_share_instance, share_server)
             raise exception.ShareMigrationFailed(reason=msg)
 
     def _migration_complete_driver(
@@ -1010,7 +1025,8 @@ class ShareManager(manager.SchedulerDependentManager):
             self.db.share_export_locations_update(
                 context, dest_share_instance['id'], export_locations)
 
-        helper = migration.ShareMigrationHelper(context, self.db, share_ref)
+        helper = migration.ShareMigrationHelper(
+            context, self.db, share_ref, self.access_helper)
 
         helper.apply_new_access_rules(dest_share_instance)
 
@@ -1032,15 +1048,11 @@ class ShareManager(manager.SchedulerDependentManager):
         self.db.share_instance_update(
             context, instance_id, {'status': constants.STATUS_INACTIVE})
 
-        rules = self.db.share_access_get_all_for_instance(
-            context, instance_id)
+        rules = self.access_helper.get_and_update_share_instance_access_rules(
+            context, share_instance_id=instance_id)
 
-        for rule in rules:
-            access_mapping = self.db.share_instance_access_get(
-                context, rule['id'], instance_id)
-
-            self.db.share_instance_access_delete(
-                context, access_mapping['id'])
+        self.access_helper.delete_share_instance_access_rules(
+            context, rules, instance_id)
 
         self.db.share_instance_delete(context, instance_id)
         LOG.info(_LI("Share instance %s: deleted successfully."),
@@ -1113,7 +1125,8 @@ class ShareManager(manager.SchedulerDependentManager):
 
         share_server = self._get_share_server(context, src_share_instance)
 
-        helper = migration.ShareMigrationHelper(context, self.db, share_ref)
+        helper = migration.ShareMigrationHelper(
+            context, self.db, share_ref, self.access_helper)
 
         task_state = share_ref['task_state']
         if task_state in (constants.TASK_STATE_DATA_COPYING_ERROR,
@@ -1123,8 +1136,7 @@ class ShareManager(manager.SchedulerDependentManager):
             LOG.warning(msg)
             helper.cleanup_new_instance(dest_share_instance)
 
-            helper.cleanup_access_rules(src_share_instance, share_server,
-                                        self.driver)
+            helper.cleanup_access_rules(src_share_instance, share_server)
             if task_state == constants.TASK_STATE_DATA_COPYING_CANCELLED:
                 self.db.share_instance_update(
                     context, src_instance_id,
@@ -1152,8 +1164,7 @@ class ShareManager(manager.SchedulerDependentManager):
                     "of share %s.") % share_ref['id']
             LOG.exception(msg)
             helper.cleanup_new_instance(dest_share_instance)
-            helper.cleanup_access_rules(src_share_instance, share_server,
-                                        self.driver)
+            helper.cleanup_access_rules(src_share_instance, share_server)
             raise exception.ShareMigrationFailed(reason=msg)
 
         self.db.share_update(
@@ -1201,7 +1212,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 constants.TASK_STATE_DATA_COPYING_COMPLETED):
 
             helper = migration.ShareMigrationHelper(
-                context, self.db, share_ref)
+                context, self.db, share_ref, self.access_helper)
 
             self.db.share_instance_update(
                 context, dest_share_instance['id'],
@@ -1209,8 +1220,7 @@ class ShareManager(manager.SchedulerDependentManager):
 
             helper.cleanup_new_instance(dest_share_instance)
 
-            helper.cleanup_access_rules(src_share_instance, share_server,
-                                        self.driver)
+            helper.cleanup_access_rules(src_share_instance, share_server)
         else:
             self.driver.migration_cancel(
                 context, src_share_instance, dest_share_instance,
@@ -1382,9 +1392,8 @@ class ShareManager(manager.SchedulerDependentManager):
     def _update_share_replica_access_rules_state(self, context,
                                                  share_replica_id, state):
         """Update the access_rules_status for the share replica."""
-
-        self.db.share_instance_update_access_status(
-            context, share_replica_id, state)
+        self.access_helper.get_and_update_share_instance_access_rules_status(
+            context, status=state, share_instance_id=share_replica_id)
 
     def _get_replica_snapshots_for_snapshot(self, context, snapshot_id,
                                             active_replica_id,
@@ -1549,7 +1558,8 @@ class ShareManager(manager.SchedulerDependentManager):
                 replica_ref.get('access_rules_status'))
         else:
             self._update_share_replica_access_rules_state(
-                context, share_replica['id'], constants.STATUS_ACTIVE)
+                context, share_replica['id'],
+                constants.STATUS_ACTIVE)
 
         LOG.info(_LI("Share replica %s created successfully."),
                  share_replica['id'])
@@ -1589,7 +1599,7 @@ class ShareManager(manager.SchedulerDependentManager):
             self.access_helper.update_access_rules(
                 context,
                 share_replica_id,
-                delete_rules="all",
+                delete_all_rules=True,
                 share_server=share_server
             )
         except Exception:
@@ -2074,7 +2084,7 @@ class ShareManager(manager.SchedulerDependentManager):
                 self.access_helper.update_access_rules(
                     context,
                     share_instance['id'],
-                    delete_rules="all",
+                    delete_all_rules=True,
                     share_server=share_server
                 )
             except Exception as e:
@@ -2222,7 +2232,7 @@ class ShareManager(manager.SchedulerDependentManager):
             self.access_helper.update_access_rules(
                 context,
                 share_instance_id,
-                delete_rules="all",
+                delete_all_rules=True,
                 share_server=share_server
             )
         except exception.ShareResourceNotFound:
@@ -2711,42 +2721,18 @@ class ShareManager(manager.SchedulerDependentManager):
 
     @add_hooks
     @utils.require_driver_initialized
-    def allow_access(self, context, share_instance_id, access_rules):
-        """Allow access to some share instance."""
-        share_instance = self._get_share_instance(context, share_instance_id)
-        status = share_instance['access_rules_status']
-
-        if status not in (constants.STATUS_UPDATING,
-                          constants.STATUS_UPDATING_MULTIPLE,
-                          constants.STATUS_ACTIVE):
-            add_rules = [self.db.share_access_get(context, rule_id)
-                         for rule_id in access_rules]
-
-            share_server = self._get_share_server(context, share_instance)
-
-            return self.access_helper.update_access_rules(
-                context,
-                share_instance_id,
-                add_rules=add_rules,
-                share_server=share_server
-            )
-
-    @add_hooks
-    @utils.require_driver_initialized
-    def deny_access(self, context, share_instance_id, access_rules):
-        """Deny access to some share."""
-        delete_rules = [self.db.share_access_get(context, rule_id)
-                        for rule_id in access_rules]
-
+    def update_access(self, context, share_instance_id):
+        """Allow/Deny access to some share."""
         share_instance = self._get_share_instance(context, share_instance_id)
         share_server = self._get_share_server(context, share_instance)
 
-        return self.access_helper.update_access_rules(
+        LOG.debug("Received request to update access for share instance"
+                  " %s." % share_instance_id)
+
+        self.access_helper.update_access_rules(
             context,
             share_instance_id,
-            delete_rules=delete_rules,
-            share_server=share_server
-        )
+            share_server=share_server)
 
     @periodic_task.periodic_task(spacing=CONF.periodic_interval)
     @utils.require_driver_initialized

manila/share/migration.py

@@ -47,11 +47,12 @@ CONF.register_opts(migration_opts)
 
 class ShareMigrationHelper(object):
 
-    def __init__(self, context, db, share):
+    def __init__(self, context, db, share, access_helper):
 
         self.db = db
         self.share = share
         self.context = context
+        self.access_helper = access_helper
         self.api = share_api.API()
 
         self.migration_create_delete_share_timeout = (
@@ -131,7 +132,7 @@ class ShareMigrationHelper(object):
             LOG.warning(_LW("Failed to cleanup new instance during generic"
                         " migration for share %s."), self.share['id'])
 
-    def cleanup_access_rules(self, share_instance, share_server, driver):
+    def cleanup_access_rules(self, share_instance, share_server):
 
         # NOTE(ganso): For the purpose of restoring the access rules, the share
         # instance status must not be "MIGRATING", else they would be cast to
@@ -142,42 +143,33 @@ class ShareMigrationHelper(object):
                                       {'status': constants.STATUS_INACTIVE})
 
         try:
-            self.revert_access_rules(share_instance, share_server, driver)
+            self.revert_access_rules(share_instance, share_server)
         except Exception:
             LOG.warning(_LW("Failed to cleanup access rules during generic"
                         " migration for share %s."), self.share['id'])
 
-    def revert_access_rules(self, share_instance, share_server, driver):
+    def revert_access_rules(self, share_instance, share_server):
 
-        rules = self.db.share_access_get_all_for_instance(
-            self.context, share_instance['id'])
+        # Cast all rules to 'queued_to_apply' so that they can be re-applied.
+        updates = {'state': constants.ACCESS_STATE_QUEUED_TO_APPLY}
+        self.access_helper.get_and_update_share_instance_access_rules(
+            self.context, updates=updates,
+            share_instance_id=share_instance['id'])
 
-        if len(rules) > 0:
-            LOG.debug("Restoring all of share %s access rules according to "
-                      "DB.", self.share['id'])
+        self.access_helper.update_access_rules(
+            self.context, share_instance['id'], share_server=share_server)
 
-            driver.update_access(self.context, share_instance, rules,
-                                 add_rules=[], delete_rules=[],
-                                 share_server=share_server)
+        utils.wait_for_access_update(
+            self.context, self.db, share_instance,
+            self.migration_wait_access_rules_timeout)
 
     def apply_new_access_rules(self, new_share_instance):
 
         self.db.share_instance_access_copy(self.context, self.share['id'],
                                            new_share_instance['id'])
 
-        rules = self.db.share_access_get_all_for_instance(
-            self.context, new_share_instance['id'])
+        self.api.allow_access_to_instance(self.context, new_share_instance)
 
-        if len(rules) > 0:
-            LOG.debug("Restoring all of share %s access rules according to "
-                      "DB.", self.share['id'])
-
-            # refresh share instance
-            new_share_instance = self.db.share_instance_get(
-                self.context, new_share_instance['id'], with_share_data=True)
-
-            self.api.allow_access_to_instance(self.context, new_share_instance,
-                                              rules)
         utils.wait_for_access_update(
             self.context, self.db, new_share_instance,
             self.migration_wait_access_rules_timeout)

manila/share/rpcapi.py

@@ -45,7 +45,8 @@ class ShareAPI(object):
             migrate_share()
             get_migration_info()
             get_driver_migration_info()
-        1.7  - Update target call API in allow/deny access methods
+        1.7  - Update target call API in allow/deny access methods (Removed
+            in 1.14)
         1.8  - Introduce Share Replication:
             create_share_replica()
             delete_share_replica()
@@ -65,6 +66,7 @@ class ShareAPI(object):
             migration_get_progress method signature, rename
             migration_get_info() to connection_get_info()
         1.13 - Introduce share revert to snapshot: revert_to_snapshot()
+        1.14 - Add update_access() and remove allow_access() and deny_access().
     """
 
     BASE_RPC_API_VERSION = '1.0'
@@ -73,7 +75,7 @@ class ShareAPI(object):
         super(ShareAPI, self).__init__()
         target = messaging.Target(topic=CONF.share_topic,
                                   version=self.BASE_RPC_API_VERSION)
-        self.client = rpc.get_client(target, version_cap='1.13')
+        self.client = rpc.get_client(target, version_cap='1.14')
 
     def create_share_instance(self, context, share_instance, host,
                               request_spec, filter_properties,
@@ -200,28 +202,11 @@ class ShareAPI(object):
                           share_id=share_id,
                           force=force)
 
-    @staticmethod
-    def _get_access_rules(access):
-        if isinstance(access, list):
-            return [rule['id'] for rule in access]
-        else:
-            return [access['id']]
-
-    def allow_access(self, context, share_instance, access):
+    def update_access(self, context, share_instance):
         host = utils.extract_host(share_instance['host'])
-        call_context = self.client.prepare(server=host, version='1.7')
-        call_context.cast(context,
-                          'allow_access',
-                          share_instance_id=share_instance['id'],
-                          access_rules=self._get_access_rules(access))
-
-    def deny_access(self, context, share_instance, access):
-        host = utils.extract_host(share_instance['host'])
-        call_context = self.client.prepare(server=host, version='1.7')
-        call_context.cast(context,
-                          'deny_access',
-                          share_instance_id=share_instance['id'],
-                          access_rules=self._get_access_rules(access))
+        call_context = self.client.prepare(server=host, version='1.14')
+        call_context.cast(context, 'update_access',
+                          share_instance_id=share_instance['id'])
 
     def publish_service_capabilities(self, context):
         call_context = self.client.prepare(fanout=True, version='1.0')

manila/tests/api/contrib/stubs.py

@@ -52,6 +52,7 @@ def stub_share(id, **kwargs):
         'availability_zone': 'fakeaz',
         'share_network_id': None,
         'share_server_id': 'fake_share_server_id',
+        'access_rules_status': 'active',
     }
     if 'instance' in kwargs:
         share_instance.update(kwargs.pop('instance'))
@@ -92,6 +93,8 @@ def stub_share(id, **kwargs):
     else:
         fake_share.is_busy = False
 
+    fake_share.instances = [fake_share.instance]
+
     return fake_share
 
 

manila/tests/api/v1/test_shares.py

@@ -30,6 +30,7 @@ from manila.common import constants
 from manila import context
 from manila import db
 from manila import exception
+from manila import policy
 from manila.share import api as share_api
 from manila.share import share_types
 from manila import test
@@ -862,6 +863,7 @@ class ShareActionsTest(test.TestCase):
         super(self.__class__, self).setUp()
         self.controller = shares.ShareController()
         self.mock_object(share_api.API, 'get', stubs.stub_share_get)
+        self.mock_policy_check = self.mock_object(policy, 'check_policy')
 
     @ddt.data(
         {'access_type': 'ip', 'access_to': '127.0.0.1'},
@@ -885,8 +887,12 @@ class ShareActionsTest(test.TestCase):
         body = {'os-allow_access': access}
         expected = {'access': {'fake': 'fake'}}
         req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id)
+
         res = self.controller._allow_access(req, id, body)
+
         self.assertEqual(expected, res)
+        self.mock_policy_check.assert_called_once_with(
+            req.environ['manila.context'], 'share', 'allow_access')
 
     @ddt.data(
         {'access_type': 'error_type', 'access_to': '127.0.0.1'},
@@ -907,8 +913,11 @@ class ShareActionsTest(test.TestCase):
         id = 'fake_share_id'
         body = {'os-allow_access': access}
         req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id)
+
         self.assertRaises(webob.exc.HTTPBadRequest,
                           self.controller._allow_access, req, id, body)
+        self.mock_policy_check.assert_called_once_with(
+            req.environ['manila.context'], 'share', 'allow_access')
 
     def test_deny_access(self):
         def _stub_deny_access(*args, **kwargs):
@@ -920,8 +929,12 @@ class ShareActionsTest(test.TestCase):
         id = 'fake_share_id'
         body = {"os-deny_access": {"access_id": 'fake_acces_id'}}
         req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id)
+
         res = self.controller._deny_access(req, id, body)
+
         self.assertEqual(202, res.status_int)
+        self.mock_policy_check.assert_called_once_with(
+            req.environ['manila.context'], 'share', 'deny_access')
 
     def test_deny_access_not_found(self):
         def _stub_deny_access(*args, **kwargs):
@@ -933,11 +946,29 @@ class ShareActionsTest(test.TestCase):
         id = 'super_fake_share_id'
         body = {"os-deny_access": {"access_id": 'fake_acces_id'}}
         req = fakes.HTTPRequest.blank('/v1/tenant1/shares/%s/action' % id)
+
         self.assertRaises(webob.exc.HTTPNotFound,
                           self.controller._deny_access,
                           req,
                           id,
                           body)
+        self.mock_policy_check.assert_called_once_with(
+            req.environ['manila.context'], 'share', 'deny_access')
+
+    @ddt.data('_allow_access', '_deny_access')
+    def test_allow_access_deny_access_policy_not_authorized(self, method):
+        req = fakes.HTTPRequest.blank('/v1/tenant1/shares/someuuid/action')
+        action = method[1:]
+        body = {action: None}
+        noauthexc = exception.PolicyNotAuthorized(action=action)
+        with mock.patch.object(
+                policy, 'check_policy', mock.Mock(side_effect=noauthexc)):
+            method = getattr(self.controller, method)
+
+            self.assertRaises(
+                webob.exc.HTTPForbidden, method, req, body, 'someuuid')
+            policy.check_policy.assert_called_once_with(
+                req.environ['manila.context'], 'share', action)
 
     def test_access_list(self):
         fake_access_list = [

manila/tests/api/v2/test_shares.py

@@ -15,6 +15,7 @@
 
 import copy
 import datetime
+import itertools
 
 import ddt
 import mock
@@ -1174,6 +1175,24 @@ class ShareAPITest(test.TestCase):
 
         self.assertEqual(expected, res_dict)
 
+    @ddt.data(('2.10', True), ('2.27', True), ('2.28', False))
+    @ddt.unpack
+    def test_share_show_access_rules_status_translated(self, version,
+                                                       translated):
+        share = db_utils.create_share(
+            access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING,
+            status=constants.STATUS_AVAILABLE)
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
+        req = fakes.HTTPRequest.blank(
+            '/shares/%s' % share['id'], version=version)
+
+        res_dict = self.controller.show(req, share['id'])
+
+        expected = (constants.STATUS_OUT_OF_SYNC if translated else
+                    constants.SHARE_INSTANCE_RULES_SYNCING)
+
+        self.assertEqual(expected, res_dict['share']['access_rules_status'])
+
     def test_share_delete(self):
         req = fakes.HTTPRequest.blank('/shares/1')
         resp = self.controller.delete(req, 1)
@@ -1626,13 +1645,14 @@ class ShareActionsTest(test.TestCase):
         self.mock_object(self.controller._access_view_builder, 'view',
                          mock.Mock(return_value={'access':
                                                  {'fake': 'fake'}}))
-
         id = 'fake_share_id'
         body = {'allow_access': access}
         expected = {'access': {'fake': 'fake'}}
         req = fakes.HTTPRequest.blank(
             '/v2/tenant1/shares/%s/action' % id, version="2.7")
+
         res = self.controller.allow_access(req, id, body)
+
         self.assertEqual(expected, res)
 
     @ddt.data(
@@ -1690,6 +1710,124 @@ class ShareActionsTest(test.TestCase):
             res = self.controller.allow_access(req, id, body)
             self.assertEqual(expected, res)
 
+    @ddt.data('2.1', '2.27')
+    def test_allow_access_access_rules_status_is_in_error(self, version):
+        share = db_utils.create_share(
+            access_rules_status=constants.SHARE_INSTANCE_RULES_ERROR)
+
+        req = fakes.HTTPRequest.blank(
+            '/v2/shares/%s/action' % share['id'], version=version)
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
+        self.mock_object(share_api.API, 'allow_access')
+        if (api_version.APIVersionRequest(version) >=
+                api_version.APIVersionRequest('2.7')):
+            key = 'allow_access'
+            method = self.controller.allow_access
+        else:
+            key = 'os-allow_access'
+            method = self.controller.allow_access_legacy
+
+        body = {
+            key: {
+                'access_type': 'user',
+                'access_to': 'crimsontide',
+                'access_level': 'rw',
+            }
+        }
+
+        self.assertRaises(webob.exc.HTTPBadRequest,
+                          method, req, share['id'], body)
+        self.assertFalse(share_api.API.allow_access.called)
+
+    @ddt.data(*itertools.product(
+        ('2.1', '2.27'), (constants.SHARE_INSTANCE_RULES_SYNCING,
+                          constants.STATUS_ACTIVE)))
+    @ddt.unpack
+    def test_allow_access_no_transitional_states(self, version, status):
+        share = db_utils.create_share(access_rules_status=status,
+                                      status=constants.STATUS_AVAILABLE)
+        req = fakes.HTTPRequest.blank(
+            '/v2/shares/%s/action' % share['id'], version=version)
+        ctxt = req.environ['manila.context']
+        access = {
+            'access_type': 'user',
+            'access_to': 'clemsontigers',
+            'access_level': 'rw',
+        }
+        expected_mapping = {
+            constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_NEW,
+            constants.SHARE_INSTANCE_RULES_ERROR:
+                constants.ACCESS_STATE_ERROR,
+            constants.STATUS_ACTIVE: constants.ACCESS_STATE_ACTIVE,
+        }
+        share = db.share_get(ctxt, share['id'])
+        updated_access = db_utils.create_access(share_id=share['id'], **access)
+        expected_access = access
+        expected_access.update(
+            {
+                'id': updated_access['id'],
+                'state': expected_mapping[share['access_rules_status']],
+                'share_id': updated_access['share_id'],
+            })
+
+        if (api_version.APIVersionRequest(version) >=
+                api_version.APIVersionRequest('2.7')):
+            key = 'allow_access'
+            method = self.controller.allow_access
+        else:
+            key = 'os-allow_access'
+            method = self.controller.allow_access_legacy
+        if (api_version.APIVersionRequest(version) >=
+                api_version.APIVersionRequest('2.13')):
+            expected_access['access_key'] = None
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
+        self.mock_object(share_api.API, 'allow_access',
+                         mock.Mock(return_value=updated_access))
+        body = {key: access}
+
+        access = method(req, share['id'], body)
+
+        self.assertEqual(expected_access, access['access'])
+        share_api.API.allow_access.assert_called_once_with(
+            req.environ['manila.context'], share, 'user',
+            'clemsontigers', 'rw')
+
+    @ddt.data(*itertools.product(
+        set(['2.28', api_version._MAX_API_VERSION]),
+        (constants.SHARE_INSTANCE_RULES_ERROR,
+         constants.SHARE_INSTANCE_RULES_SYNCING, constants.STATUS_ACTIVE)))
+    @ddt.unpack
+    def test_allow_access_access_rules_status_dont_care(self, version, status):
+        access = {
+            'access_type': 'user',
+            'access_to': 'clemsontigers',
+            'access_level': 'rw',
+        }
+        updated_access = db_utils.create_access(**access)
+        expected_access = access
+        expected_access.update(
+            {
+                'id': updated_access['id'],
+                'state': updated_access['state'],
+                'share_id': updated_access['share_id'],
+                'access_key': None,
+            })
+
+        share = db_utils.create_share(access_rules_status=status)
+        req = fakes.HTTPRequest.blank(
+            '/v2/shares/%s/action' % share['id'], version=version)
+        self.mock_object(share_api.API, 'get', mock.Mock(return_value=share))
+        self.mock_object(share_api.API, 'allow_access',
+                         mock.Mock(return_value=updated_access))
+        body = {'allow_access': access}
+
+        access = self.controller.allow_access(req, share['id'], body)
+
+        self.assertEqual(expected_access, access['access'])
+        share_api.API.allow_access.assert_called_once_with(
+            req.environ['manila.context'], share, 'user',
+            'clemsontigers', 'rw')
+
     def test_deny_access(self):
         def _stub_deny_access(*args, **kwargs):
             pass

manila/tests/api/views/test_share_accesses.py

@@ -14,9 +14,11 @@
 #    under the License.
 
 import ddt
+import mock
 
 from manila.api.openstack import api_version_request as api_version
 from manila.api.views import share_accesses
+from manila.share import api
 from manila import test
 from manila.tests.api import fakes
 
@@ -36,6 +38,9 @@ class ViewBuilderTestCase(test.TestCase):
             'state': 'fakeaccessstate',
             'access_key': 'fakeaccesskey',
         }
+        self.fake_share = {
+            'access_rules_status': self.fake_access['state'],
+        }
 
     def test_collection_name(self):
         self.assertEqual('share_accesses', self.builder._collection_name)
@@ -43,6 +48,8 @@ class ViewBuilderTestCase(test.TestCase):
     @ddt.data("2.20", "2.21")
     def test_view(self, version):
         req = fakes.HTTPRequest.blank('/shares', version=version)
+        self.mock_object(api.API, 'get',
+                         mock.Mock(return_value=self.fake_share))
 
         result = self.builder.view(req, self.fake_access)
 
@@ -55,6 +62,8 @@ class ViewBuilderTestCase(test.TestCase):
     @ddt.data("2.20", "2.21")
     def test_summary_view(self, version):
         req = fakes.HTTPRequest.blank('/shares', version=version)
+        self.mock_object(api.API, 'get',
+                         mock.Mock(return_value=self.fake_share))
 
         result = self.builder.summary_view(req, self.fake_access)
 
@@ -68,6 +77,8 @@ class ViewBuilderTestCase(test.TestCase):
     @ddt.data("2.20", "2.21")
     def test_list_view(self, version):
         req = fakes.HTTPRequest.blank('/shares', version=version)
+        self.mock_object(api.API, 'get',
+                         mock.Mock(return_value=self.fake_share))
         accesses = [self.fake_access, ]
 
         result = self.builder.list_view(req, accesses)

manila/tests/data/test_helper.py

@@ -82,7 +82,8 @@ class DataServiceHelperTestCase(test.TestCase):
         self.mock_object(
             self.helper.db, 'share_access_get_all_by_type_and_access',
             mock.Mock(return_value=[access]))
-        self.mock_object(self.helper, '_change_data_access_to_instance')
+        change_data_access_call = self.mock_object(
+            self.helper, '_change_data_access_to_instance')
         self.mock_object(self.helper.db, 'share_instance_access_create',
                          mock.Mock(return_value=access))
 
@@ -110,14 +111,14 @@ class DataServiceHelperTestCase(test.TestCase):
         self.helper.db.share_instance_access_create.assert_has_calls(
             access_create_calls)
         change_access_calls = [
-            mock.call(self.share_instance, access, allow=False),
-            mock.call(self.share_instance, access, allow=True),
+            mock.call(self.share_instance, [access], deny=True),
         ]
         if allow_dest_instance:
             change_access_calls.append(
-                mock.call(self.share_instance, access, allow=True))
-        self.helper._change_data_access_to_instance.assert_has_calls(
-            change_access_calls)
+                mock.call(self.share_instance, access))
+        self.assertEqual(len(change_access_calls),
+                         change_data_access_call.call_count)
+        change_data_access_call.assert_has_calls(change_access_calls)
 
     @ddt.data({'ip': []}, {'cert': []}, {'user': []}, {'cephx': []}, {'x': []})
     def test__get_access_entries_according_to_mapping(self, mapping):
@@ -154,9 +155,8 @@ class DataServiceHelperTestCase(test.TestCase):
             [self.access], self.share_instance['id'])
 
         # asserts
-        self.helper._change_data_access_to_instance.\
-            assert_called_once_with(
-                self.share_instance['id'], self.access, allow=False)
+        self.helper._change_data_access_to_instance.assert_called_once_with(
+            self.share_instance['id'], [self.access], deny=True)
 
     @ddt.data(None, Exception('fake'))
     def test_cleanup_data_access(self, exc):
@@ -225,34 +225,39 @@ class DataServiceHelperTestCase(test.TestCase):
             self.assertTrue(data_copy_helper.LOG.warning.called)
 
     @ddt.data(True, False)
-    def test__change_data_access_to_instance(self, allow):
+    def test__change_data_access_to_instance(self, deny):
+        access_rule = db_utils.create_access(share_id=self.share['id'])
+        access_rule = db.share_instance_access_get(
+            self.context, access_rule['id'], self.share_instance['id'])
 
         # mocks
-        self.mock_object(self.helper.db, 'share_instance_update_access_status')
-
-        if allow:
-            self.mock_object(share_rpc.ShareAPI, 'allow_access')
-        else:
-            self.mock_object(share_rpc.ShareAPI, 'deny_access')
-
+        self.mock_object(share_rpc.ShareAPI, 'update_access')
         self.mock_object(utils, 'wait_for_access_update')
+        mock_access_rules_status_update = self.mock_object(
+            self.helper.access_helper,
+            'get_and_update_share_instance_access_rules_status')
+        mock_rules_update = self.mock_object(
+            self.helper.access_helper,
+            'get_and_update_share_instance_access_rules')
 
         # run
         self.helper._change_data_access_to_instance(
-            self.share_instance, self.access, allow=allow)
+            self.share_instance, access_rule, deny=deny)
 
         # asserts
-        self.helper.db.share_instance_update_access_status.\
-            assert_called_once_with(self.context, self.share_instance['id'],
-                                    constants.STATUS_OUT_OF_SYNC)
+        if deny:
+            mock_rules_update.assert_called_once_with(
+                self.context, share_instance_id=self.share_instance['id'],
+                filters={'access_id': [access_rule['id']]},
+                updates={'state': constants.ACCESS_STATE_QUEUED_TO_DENY})
 
-        if allow:
-            share_rpc.ShareAPI.allow_access.assert_called_once_with(
-                self.context, self.share_instance, self.access)
         else:
-            share_rpc.ShareAPI.deny_access.assert_called_once_with(
-                self.context, self.share_instance, self.access)
-
+            self.assertFalse(mock_rules_update.called)
+        share_rpc.ShareAPI.update_access.assert_called_once_with(
+            self.context, self.share_instance)
+        mock_access_rules_status_update.assert_called_once_with(
+            self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING,
+            share_instance_id=self.share_instance['id'])
         utils.wait_for_access_update.assert_called_once_with(
             self.context, self.helper.db, self.share_instance,
             data_copy_helper.CONF.data_access_wait_access_rules_timeout)

manila/tests/db/migrations/alembic/migrations_data_checks.py

@@ -1580,3 +1580,139 @@ class RemoveNovaNetIdColumnFromShareNetworks(BaseMigrationChecks):
         for row in rows:
             self.test_case.assertTrue(hasattr(row, self.nova_net_column_name))
             self.test_case.assertIsNone(row[self.nova_net_column_name])
+
+
+@map_to_migration('54667b9cade7')
+class RestoreStateToShareInstanceAccessMap(BaseMigrationChecks):
+    new_instance_mapping_state = {
+        constants.STATUS_ACTIVE: constants.STATUS_ACTIVE,
+        constants.SHARE_INSTANCE_RULES_SYNCING:
+            constants.ACCESS_STATE_QUEUED_TO_APPLY,
+        constants.STATUS_OUT_OF_SYNC: constants.ACCESS_STATE_QUEUED_TO_APPLY,
+        'updating': constants.ACCESS_STATE_QUEUED_TO_APPLY,
+        'updating_multiple': constants.ACCESS_STATE_QUEUED_TO_APPLY,
+        constants.SHARE_INSTANCE_RULES_ERROR: constants.ACCESS_STATE_ERROR,
+    }
+
+    new_access_rules_status = {
+        constants.STATUS_ACTIVE: constants.STATUS_ACTIVE,
+        constants.STATUS_OUT_OF_SYNC: constants.SHARE_INSTANCE_RULES_SYNCING,
+        'updating': constants.SHARE_INSTANCE_RULES_SYNCING,
+        'updating_multiple': constants.SHARE_INSTANCE_RULES_SYNCING,
+        constants.SHARE_INSTANCE_RULES_ERROR:
+            constants.SHARE_INSTANCE_RULES_ERROR,
+    }
+
+    @staticmethod
+    def generate_share_instance(sid, access_rules_status):
+        share_instance_data = {
+            'id': uuidutils.generate_uuid(),
+            'deleted': 'False',
+            'host': 'fake',
+            'share_id': sid,
+            'status': constants.STATUS_AVAILABLE,
+            'access_rules_status': access_rules_status
+        }
+        return share_instance_data
+
+    @staticmethod
+    def generate_share_instance_access_map(share_access_data_id,
+                                           share_instance_id):
+        share_instance_access_data = {
+            'id': uuidutils.generate_uuid(),
+            'share_instance_id': share_instance_id,
+            'access_id': share_access_data_id,
+            'deleted': 'False'
+        }
+        return share_instance_access_data
+
+    def setup_upgrade_data(self, engine):
+        share_data = {
+            'id': uuidutils.generate_uuid(),
+            'share_proto': 'fake',
+            'size': 1,
+            'snapshot_id': None,
+            'user_id': 'fake',
+            'project_id': 'fake'
+        }
+        share_table = utils.load_table('shares', engine)
+        engine.execute(share_table.insert(share_data))
+
+        share_instances = [
+            self.generate_share_instance(
+                share_data['id'], constants.STATUS_ACTIVE),
+            self.generate_share_instance(
+                share_data['id'], constants.STATUS_OUT_OF_SYNC),
+            self.generate_share_instance(
+                share_data['id'], constants.STATUS_ERROR),
+            self.generate_share_instance(
+                share_data['id'], 'updating'),
+            self.generate_share_instance(
+                share_data['id'], 'updating_multiple'),
+        ]
+        self.updating_share_instance = share_instances[3]
+        self.updating_multiple_share_instance = share_instances[4]
+
+        share_instance_table = utils.load_table('share_instances', engine)
+        for share_instance_data in share_instances:
+            engine.execute(share_instance_table.insert(share_instance_data))
+
+        share_access_data = {
+            'id': uuidutils.generate_uuid(),
+            'share_id': share_data['id'],
+            'access_type': 'fake',
+            'access_to': 'alice',
+            'deleted': 'False'
+        }
+        share_access_table = utils.load_table('share_access_map', engine)
+        engine.execute(share_access_table.insert(share_access_data))
+
+        share_instance_access_data = []
+        for share_instance in share_instances:
+            sia_map = self.generate_share_instance_access_map(
+                share_access_data['id'], share_instance['id'])
+            share_instance_access_data.append(sia_map)
+
+        share_instance_access_table = utils.load_table(
+            'share_instance_access_map', engine)
+        for sia_map in share_instance_access_data:
+            engine.execute(share_instance_access_table.insert(sia_map))
+
+    def check_upgrade(self, engine, data):
+        share_instance_table = utils.load_table('share_instances', engine)
+        sia_table = utils.load_table('share_instance_access_map', engine)
+
+        for rule in engine.execute(sia_table.select()):
+            self.test_case.assertTrue(hasattr(rule, 'state'))
+            correlated_share_instances = engine.execute(
+                share_instance_table.select().where(
+                    share_instance_table.c.id == rule['share_instance_id']))
+            access_rules_status = getattr(correlated_share_instances.first(),
+                                          'access_rules_status')
+            self.test_case.assertEqual(
+                self.new_instance_mapping_state[access_rules_status],
+                rule['state'])
+
+        for instance in engine.execute(share_instance_table.select()):
+            self.test_case.assertTrue(instance['access_rules_status']
+                                      not in ('updating',
+                                              'updating_multiple',
+                                              constants.STATUS_OUT_OF_SYNC))
+            if instance['id'] in (self.updating_share_instance['id'],
+                                  self.updating_multiple_share_instance['id']):
+                self.test_case.assertEqual(
+                    constants.SHARE_INSTANCE_RULES_SYNCING,
+                    instance['access_rules_status'])
+
+    def check_downgrade(self, engine):
+        share_instance_table = utils.load_table('share_instances', engine)
+        sia_table = utils.load_table('share_instance_access_map', engine)
+        for rule in engine.execute(sia_table.select()):
+            self.test_case.assertFalse(hasattr(rule, 'state'))
+
+        for instance in engine.execute(share_instance_table.select()):
+            if instance['id'] in (self.updating_share_instance['id'],
+                                  self.updating_multiple_share_instance['id']):
+                self.test_case.assertEqual(
+                    constants.STATUS_OUT_OF_SYNC,
+                    instance['access_rules_status'])

manila/tests/db/sqlalchemy/test_api.py

@@ -85,7 +85,6 @@ class GenericDatabaseAPITestCase(test.TestCase):
 
         db_api.share_instance_access_delete(
             self.ctxt, share_access.instance_mappings[0].id)
-        db_api.share_access_delete(self.ctxt, share_access.id)
         self.assertRaises(exception.NotFound, db_api.share_access_get,
                           self.ctxt, share_access.id)
 
@@ -98,44 +97,126 @@ class ShareAccessDatabaseAPITestCase(test.TestCase):
         super(ShareAccessDatabaseAPITestCase, self).setUp()
         self.ctxt = context.get_admin_context()
 
-    def test_share_instance_update_access_status(self):
+    @ddt.data(0, 3)
+    def test_share_access_get_all_for_share(self, len_rules):
         share = db_utils.create_share()
-        share_instance = db_utils.create_share_instance(share_id=share['id'])
-        db_utils.create_access(share_id=share_instance['share_id'])
+        rules = [db_utils.create_access(share_id=share['id'])
+                 for i in range(0, len_rules)]
+        rule_ids = [r['id'] for r in rules]
 
-        db_api.share_instance_update_access_status(
-            self.ctxt,
-            share_instance['id'],
-            constants.STATUS_ACTIVE
-        )
+        result = db_api.share_access_get_all_for_share(self.ctxt, share['id'])
 
-        result = db_api.share_instance_get(self.ctxt, share_instance['id'])
+        self.assertEqual(len_rules, len(result))
+        result_ids = [r['id'] for r in result]
+        self.assertEqual(rule_ids, result_ids)
 
-        self.assertEqual(constants.STATUS_ACTIVE,
-                         result['access_rules_status'])
-
-    def test_share_instance_update_access_status_invalid(self):
+    def test_share_access_get_all_for_share_no_instance_mappings(self):
         share = db_utils.create_share()
-        share_instance = db_utils.create_share_instance(share_id=share['id'])
-        db_utils.create_access(share_id=share_instance['share_id'])
+        share_instance = share['instance']
+        rule = db_utils.create_access(share_id=share['id'])
+        # Mark instance mapping soft deleted
+        db_api.share_instance_access_update(
+            self.ctxt, rule['id'], share_instance['id'], {'deleted': "True"})
 
-        self.assertRaises(
-            db_exception.DBError,
-            db_api.share_instance_update_access_status,
-            self.ctxt, share_instance['id'],
-            "fake_status"
-        )
+        result = db_api.share_access_get_all_for_share(self.ctxt, share['id'])
 
-    @ddt.data(None, 'rhubarb')
-    def test_share_access_update_access_key(self, key_value):
+        self.assertEqual([], result)
+
+    def test_share_instance_access_update(self):
         share = db_utils.create_share()
         access = db_utils.create_access(share_id=share['id'])
 
-        db_api.share_access_update_access_key(self.ctxt, access['id'],
-                                              key_value)
+        instance_access_mapping = db_api.share_instance_access_get(
+            self.ctxt, access['id'], share.instance['id'])
+        self.assertEqual(constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                         access['state'])
+        self.assertIsNone(access['access_key'])
 
+        db_api.share_instance_access_update(
+            self.ctxt, access['id'], share.instance['id'],
+            {'state': constants.STATUS_ERROR, 'access_key': 'watson4heisman'})
+
+        instance_access_mapping = db_api.share_instance_access_get(
+            self.ctxt, access['id'], share.instance['id'])
         access = db_api.share_access_get(self.ctxt, access['id'])
-        self.assertEqual(key_value, access['access_key'])
+        self.assertEqual(constants.STATUS_ERROR,
+                         instance_access_mapping['state'])
+        self.assertEqual('watson4heisman', access['access_key'])
+
+    @ddt.data(True, False)
+    def test_share_access_get_all_for_instance_with_share_access_data(
+            self, with_share_access_data):
+        share = db_utils.create_share()
+        access_1 = db_utils.create_access(share_id=share['id'])
+        access_2 = db_utils.create_access(share_id=share['id'])
+        share_access_keys = ('access_to', 'access_type', 'access_level',
+                             'share_id')
+
+        rules = db_api.share_access_get_all_for_instance(
+            self.ctxt, share.instance['id'],
+            with_share_access_data=with_share_access_data)
+
+        share_access_keys_present = True if with_share_access_data else False
+        actual_access_ids = [r['access_id'] for r in rules]
+        self.assertEqual(sorted([access_1['id'], access_2['id']]),
+                         sorted(actual_access_ids))
+        for rule in rules:
+            for key in share_access_keys:
+                self.assertEqual(share_access_keys_present, key in rule)
+            self.assertTrue('state' in rule)
+
+    def test_share_access_get_all_for_instance_with_filters(self):
+        share = db_utils.create_share()
+        new_share_instance = db_utils.create_share_instance(
+            share_id=share['id'])
+        access_1 = db_utils.create_access(share_id=share['id'])
+        access_2 = db_utils.create_access(share_id=share['id'])
+        share_access_keys = ('access_to', 'access_type', 'access_level',
+                             'share_id')
+        db_api.share_instance_access_update(
+            self.ctxt, access_1['id'], new_share_instance['id'],
+            {'state': constants.STATUS_ACTIVE})
+
+        rules = db_api.share_access_get_all_for_instance(
+            self.ctxt, new_share_instance['id'],
+            filters={'state': constants.ACCESS_STATE_QUEUED_TO_APPLY})
+
+        self.assertEqual(1, len(rules))
+        self.assertEqual(access_2['id'], rules[0]['access_id'])
+
+        for rule in rules:
+            for key in share_access_keys:
+                self.assertTrue(key in rule)
+
+    def test_share_instance_access_delete(self):
+        share = db_utils.create_share()
+        access = db_utils.create_access(share_id=share['id'])
+        instance_access_mapping = db_api.share_instance_access_get(
+            self.ctxt, access['id'], share.instance['id'])
+
+        db_api.share_instance_access_delete(
+            self.ctxt, instance_access_mapping['id'])
+
+        rules = db_api.share_access_get_all_for_instance(
+            self.ctxt, share.instance['id'])
+        self.assertEqual([], rules)
+
+        self.assertRaises(exception.NotFound, db_api.share_instance_access_get,
+                          self.ctxt, access['id'], share['instance']['id'])
+
+    @ddt.data(True, False)
+    def test_share_instance_access_get_with_share_access_data(
+            self, with_share_access_data):
+        share = db_utils.create_share()
+        access = db_utils.create_access(share_id=share['id'])
+
+        instance_access = db_api.share_instance_access_get(
+            self.ctxt, access['id'], share['instance']['id'],
+            with_share_access_data=with_share_access_data)
+
+        for key in ('share_id', 'access_type', 'access_to', 'access_level',
+                    'access_key'):
+            self.assertEqual(with_share_access_data, key in instance_access)
 
 
 @ddt.ddt

manila/tests/db/sqlalchemy/test_models.py

@@ -17,6 +17,8 @@
 import ddt
 
 from manila.common import constants
+from manila import context
+from manila.db.sqlalchemy import api as db_api
 from manila import test
 from manila.tests import db_utils
 
@@ -152,8 +154,8 @@ class ShareTestCase(test.TestCase):
 
         self.assertEqual(constants.STATUS_ACTIVE, share.access_rules_status)
 
-    @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_OUT_OF_SYNC,
-              constants.STATUS_ERROR)
+    @ddt.data(constants.STATUS_ACTIVE, constants.SHARE_INSTANCE_RULES_SYNCING,
+              constants.SHARE_INSTANCE_RULES_ERROR)
     def test_access_rules_status(self, access_status):
         instances = [
             db_utils.create_share_instance(
@@ -173,6 +175,43 @@ class ShareTestCase(test.TestCase):
 
 
 @ddt.ddt
+class ShareAccessTestCase(test.TestCase):
+    """Testing of SQLAlchemy Share Access related model classes."""
+
+    @ddt.data(constants.ACCESS_STATE_QUEUED_TO_APPLY,
+              constants.ACCESS_STATE_ACTIVE, constants.ACCESS_STATE_ERROR,
+              constants.ACCESS_STATE_APPLYING)
+    def test_share_access_mapping_state(self, expected_status):
+        ctxt = context.get_admin_context()
+
+        share = db_utils.create_share()
+        share_instances = [
+            share.instance,
+            db_utils.create_share_instance(share_id=share['id']),
+            db_utils.create_share_instance(share_id=share['id']),
+            db_utils.create_share_instance(share_id=share['id']),
+        ]
+        access_rule = db_utils.create_access(share_id=share['id'])
+
+        # Update the access mapping states
+        db_api.share_instance_access_update(
+            ctxt, access_rule['id'], share_instances[0]['id'],
+            {'state': constants.ACCESS_STATE_ACTIVE})
+        db_api.share_instance_access_update(
+            ctxt, access_rule['id'], share_instances[1]['id'],
+            {'state': expected_status})
+        db_api.share_instance_access_update(
+            ctxt, access_rule['id'], share_instances[2]['id'],
+            {'state': constants.ACCESS_STATE_ACTIVE})
+        db_api.share_instance_access_update(
+            ctxt, access_rule['id'], share_instances[3]['id'],
+            {'deleted': 'True', 'state': constants.STATUS_DELETED})
+
+        access_rule = db_api.share_access_get(ctxt, access_rule['id'])
+
+        self.assertEqual(expected_status, access_rule['state'])
+
+
 class ShareSnapshotTestCase(test.TestCase):
     """Testing of SQLAlchemy ShareSnapshot model class."""
 

manila/tests/db_utils.py

@@ -168,12 +168,21 @@ def create_snapshot_instance(snapshot_id, **kwargs):
 
 def create_access(**kwargs):
     """Create a access rule object."""
+    state = kwargs.pop('state', constants.ACCESS_STATE_QUEUED_TO_APPLY)
     access = {
         'access_type': 'fake_type',
         'access_to': 'fake_IP',
-        'share_id': None,
+        'share_id': kwargs.pop('share_id', None) or create_share()['id'],
     }
-    return _create_db_row(db.share_access_create, access, kwargs)
+    access.update(kwargs)
+    share_access_rule = _create_db_row(db.share_access_create, access, kwargs)
+
+    for mapping in share_access_rule.instance_mappings:
+        db.share_instance_access_update(
+            context.get_admin_context(), share_access_rule['id'],
+            mapping.share_instance_id, {'state': state})
+
+    return share_access_rule
 
 
 def create_share_server(**kwargs):

manila/tests/fake_share.py

@@ -242,7 +242,7 @@ def fake_replica(id=None, as_primitive=True, for_manager=False, **kwargs):
         'export_locations': [{'path': 'path1'}, {'path': 'path2'}],
         'share_network_id': '4ccd5318-65f1-11e5-9d70-feff819cdc9f',
         'share_server_id': '53099868-65f1-11e5-9d70-feff819cdc9f',
-        'access_rules_status': 'out_of_sync',
+        'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING,
     }
     if for_manager:
         replica.update({

manila/tests/share/drivers/cephfs/test_cephfs_native.py

@@ -249,24 +249,26 @@ class CephFSNativeDriverTestCase(test.TestCase):
 
     def test_update_access_add_rm(self):
         alice = {
-            'id': 'accessid1',
+            'id': 'instance_mapping_id1',
+            'access_id': 'accessid1',
             'access_level': 'rw',
             'access_type': 'cephx',
             'access_to': 'alice'
         }
         bob = {
-            'id': 'accessid2',
+            'id': 'instance_mapping_id2',
+            'access_id': 'accessid2',
             'access_level': 'rw',
             'access_type': 'cephx',
             'access_to': 'bob'
         }
 
-        access_keys = self._driver.update_access(self._context, self._share,
-                                                 access_rules=[alice],
-                                                 add_rules=[alice],
-                                                 delete_rules=[bob])
+        access_updates = self._driver.update_access(
+            self._context, self._share, access_rules=[alice],
+            add_rules=[alice], delete_rules=[bob])
 
-        self.assertEqual({'accessid1': 'abc123'}, access_keys)
+        self.assertEqual(
+            {'accessid1': {'access_key': 'abc123'}}, access_updates)
         self._driver._volume_client.authorize.assert_called_once_with(
             self._driver._share_path(self._share),
             "alice",
@@ -279,19 +281,21 @@ class CephFSNativeDriverTestCase(test.TestCase):
     @ddt.data(None, 1)
     def test_update_access_all(self, volume_client_version):
         alice = {
-            'id': 'accessid1',
+            'id': 'instance_mapping_id1',
+            'access_id': 'accessid1',
             'access_level': 'rw',
             'access_type': 'cephx',
             'access_to': 'alice'
         }
         self._driver.volume_client.version = volume_client_version
 
-        access_keys = self._driver.update_access(self._context, self._share,
+        access_updates = self._driver.update_access(self._context, self._share,
                                                     access_rules=[alice],
                                                     add_rules=[],
                                                     delete_rules=[])
 
-        self.assertEqual({'accessid1': 'abc123'}, access_keys)
+        self.assertEqual(
+            {'accessid1': {'access_key': 'abc123'}}, access_updates)
         if volume_client_version:
             (self._driver._volume_client.get_authorized_ids.
              assert_called_once_with(self._driver._share_path(self._share)))

manila/tests/share/drivers/huawei/test_huawei_nas.py

@@ -4284,16 +4284,20 @@ class HuaweiShareDriverTestCase(test.TestCase):
              'access_rules_status': common_constants.STATUS_ACTIVE},
             {'id': self.active_replica['id'],
              'replica_state': common_constants.REPLICA_STATE_IN_SYNC,
-             'access_rules_status': common_constants.STATUS_OUT_OF_SYNC},
+             'access_rules_status':
+                 common_constants.SHARE_INSTANCE_RULES_SYNCING},
         ]
 
         self.assertEqual(expected, result)
 
     @ddt.data({'mock_method': 'update_access',
-               'new_access_status': common_constants.STATUS_OUT_OF_SYNC,
-               'old_access_status': common_constants.STATUS_OUT_OF_SYNC},
+               'new_access_status':
+                   common_constants.SHARE_INSTANCE_RULES_SYNCING,
+               'old_access_status':
+                   common_constants.SHARE_INSTANCE_RULES_SYNCING},
               {'mock_method': 'clear_access',
-               'new_access_status': common_constants.STATUS_OUT_OF_SYNC,
+               'new_access_status':
+                   common_constants.SHARE_INSTANCE_RULES_SYNCING,
                'old_access_status': common_constants.STATUS_ACTIVE},)
     @ddt.unpack
     def test_promote_replica_with_access_update_error(
@@ -4372,7 +4376,8 @@ class HuaweiShareDriverTestCase(test.TestCase):
              'access_rules_status': common_constants.STATUS_ACTIVE},
             {'id': self.active_replica['id'],
              'replica_state': common_constants.STATUS_ERROR,
-             'access_rules_status': common_constants.STATUS_OUT_OF_SYNC},
+             'access_rules_status':
+                 common_constants.SHARE_INSTANCE_RULES_SYNCING},
         ]
 
         self.assertEqual(expected, result)

manila/tests/share/drivers/netapp/dataontap/cluster_mode/test_lib_base.py

@@ -2946,7 +2946,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
                          actual_replica_2['replica_state'])
         self.assertEqual('fake_export_location',
                          actual_replica_2['export_locations'])
-        self.assertEqual(constants.STATUS_OUT_OF_SYNC,
+        self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING,
                          actual_replica_2['access_rules_status'])
 
     def test_convert_destination_replica_to_independent_with_access_rules(
@@ -2982,7 +2982,7 @@ class NetAppFileStorageLibraryTestCase(test.TestCase):
                          replica['export_locations'])
         self.assertEqual(constants.REPLICA_STATE_ACTIVE,
                          replica['replica_state'])
-        self.assertEqual(constants.STATUS_OUT_OF_SYNC,
+        self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING,
                          replica['access_rules_status'])
 
     def test_convert_destination_replica_to_independent_failed_access_rules(

manila/tests/share/drivers/zfsonlinux/test_driver.py

@@ -1738,13 +1738,15 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
             'fake_context', replica_list, replica, access_rules)
 
         expected = [
-            {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC,
+            {'access_rules_status':
+                zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING,
              'id': 'fake_active_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_IN_SYNC},
             {'access_rules_status': zfs_driver.constants.STATUS_ACTIVE,
              'id': 'fake_first_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_ACTIVE},
-            {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC,
+            {'access_rules_status':
+                zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING,
              'id': 'fake_second_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC},
         ]
@@ -1838,15 +1840,18 @@ class ZFSonLinuxShareDriverTestCase(test.TestCase):
             'fake_context', replica_list, replica, access_rules)
 
         expected = [
-            {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC,
+            {'access_rules_status':
+                zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING,
              'id': 'fake_active_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC},
             {'access_rules_status': zfs_driver.constants.STATUS_ACTIVE,
              'id': 'fake_first_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_ACTIVE},
-            {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC,
+            {'access_rules_status':
+                zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING,
              'id': 'fake_second_replica_id'},
-            {'access_rules_status': zfs_driver.constants.STATUS_OUT_OF_SYNC,
+            {'access_rules_status':
+                zfs_driver.constants.SHARE_INSTANCE_RULES_SYNCING,
              'id': 'fake_third_replica_id',
              'replica_state': zfs_driver.constants.REPLICA_STATE_OUT_OF_SYNC},
         ]

manila/tests/share/test_access.py

@@ -13,6 +13,8 @@
 #    License for the specific language governing permissions and limitations
 #    under the License.
 
+import itertools
+
 import ddt
 import mock
 
@@ -23,6 +25,199 @@ from manila import exception
 from manila.share import access
 from manila import test
 from manila.tests import db_utils
+from manila import utils
+
+
+class LockedOperationsTestCase(test.TestCase):
+
+    class FakeAccessHelper(object):
+
+        @access.locked_access_rules_operation
+        def some_access_rules_operation(self, context, share_instance_id=None):
+            pass
+
+    def setUp(self):
+        super(self.__class__, self).setUp()
+        self.access_helper = self.FakeAccessHelper()
+        self.context = context.RequestContext('fake_user', 'fake_project')
+        self.lock_call = self.mock_object(
+            utils, 'synchronized', mock.Mock(return_value=lambda f: f))
+
+    def test_locked_access_rules_operation(self, **replica):
+
+        self.access_helper.some_access_rules_operation(
+            self.context, share_instance_id='FAKE_INSTANCE_ID')
+
+        self.lock_call.assert_called_once_with(
+            "locked_access_rules_operation_by_share_instance_FAKE_INSTANCE_ID",
+            external=True)
+
+
+@ddt.ddt
+class ShareInstanceAccessDatabaseMixinTestCase(test.TestCase):
+
+    def setUp(self):
+        super(self.__class__, self).setUp()
+        self.driver = mock.Mock()
+        self.access_helper = access.ShareInstanceAccess(db, self.driver)
+        self.context = context.RequestContext('fake_user', 'fake_project')
+        self.mock_object(
+            utils, 'synchronized', mock.Mock(return_value=lambda f: f))
+
+    def test_get_and_update_access_rules_status_force_status(self):
+        share = db_utils.create_share(
+            access_rule_status=constants.STATUS_ACTIVE,
+            status=constants.STATUS_AVAILABLE)
+        share = db.share_get(self.context, share['id'])
+        self.assertEqual(constants.STATUS_ACTIVE, share['access_rules_status'])
+
+        self.access_helper.get_and_update_share_instance_access_rules_status(
+            self.context, status=constants.SHARE_INSTANCE_RULES_SYNCING,
+            share_instance_id=share['instance']['id'])
+
+        share = db.share_get(self.context, share['id'])
+        self.assertEqual(constants.SHARE_INSTANCE_RULES_SYNCING,
+                         share['access_rules_status'])
+
+    @ddt.data((constants.SHARE_INSTANCE_RULES_SYNCING, True),
+              (constants.STATUS_ERROR, False))
+    @ddt.unpack
+    def test_get_and_update_access_rules_status_conditionally_change(
+            self, initial_status, change_allowed):
+        share = db_utils.create_share(access_rules_status=initial_status,
+                                      status=constants.STATUS_AVAILABLE)
+        share = db.share_get(self.context, share['id'])
+        self.assertEqual(initial_status, share['access_rules_status'])
+
+        conditionally_change = {
+            constants.SHARE_INSTANCE_RULES_SYNCING: constants.STATUS_ACTIVE,
+        }
+
+        updated_instance = (
+            self.access_helper.
+            get_and_update_share_instance_access_rules_status(
+                self.context, conditionally_change=conditionally_change,
+                share_instance_id=share['instance']['id'])
+        )
+
+        share = db.share_get(self.context, share['id'])
+        if change_allowed:
+            self.assertEqual(constants.STATUS_ACTIVE,
+                             share['access_rules_status'])
+            self.assertIsNotNone(updated_instance)
+        else:
+            self.assertEqual(initial_status, share['access_rules_status'])
+            self.assertIsNone(updated_instance)
+
+    def test_get_and_update_all_access_rules_just_get(self):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        rule_1 = db_utils.create_access(share_id=share['id'])
+        rule_2 = db_utils.create_access(share_id=share['id'])
+        self.mock_object(db, 'share_instance_access_update')
+
+        rules = self.access_helper.get_and_update_share_instance_access_rules(
+            self.context, share_instance_id=share['instance']['id'])
+
+        self.assertEqual(2, len(rules))
+        rule_ids = [r['access_id'] for r in rules]
+        self.assertTrue(rule_1['id'] in rule_ids)
+        self.assertTrue(rule_2['id'] in rule_ids)
+        self.assertFalse(db.share_instance_access_update.called)
+
+    @ddt.data(
+        ([constants.ACCESS_STATE_QUEUED_TO_APPLY], 2),
+        ([constants.ACCESS_STATE_QUEUED_TO_APPLY,
+          constants.STATUS_ACTIVE], 1),
+        ([constants.ACCESS_STATE_APPLYING], 2),
+        ([constants.ACCESS_STATE_APPLYING, constants.ACCESS_STATE_ERROR], 1),
+        ([constants.ACCESS_STATE_ACTIVE, constants.ACCESS_STATE_DENYING], 0))
+    @ddt.unpack
+    def test_get_and_update_all_access_rules_updates_conditionally_changed(
+            self, statuses, changes_allowed):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        db_utils.create_access(share_id=share['id'], state=statuses[0])
+        db_utils.create_access(share_id=share['id'], state=statuses[-1])
+        self.mock_object(db, 'share_instance_access_update', mock.Mock(
+            side_effect=db.share_instance_access_update))
+        updates = {
+            'access_key': 'renfrow2stars'
+        }
+        expected_updates = {
+            'access_key': 'renfrow2stars',
+            'state': constants.ACCESS_STATE_QUEUED_TO_DENY,
+        }
+        conditionally_change = {
+            constants.ACCESS_STATE_APPLYING:
+                constants.ACCESS_STATE_QUEUED_TO_DENY,
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_QUEUED_TO_DENY,
+        }
+
+        rules = self.access_helper.get_and_update_share_instance_access_rules(
+            self.context, share_instance_id=share['instance']['id'],
+            updates=updates, conditionally_change=conditionally_change)
+
+        state_changed_rules = [
+            r for r in rules if
+            r['state'] == constants.ACCESS_STATE_QUEUED_TO_DENY
+        ]
+        self.assertEqual(changes_allowed, len(state_changed_rules))
+        self.assertEqual(2, db.share_instance_access_update.call_count)
+        db.share_instance_access_update.assert_has_calls([
+            mock.call(self.context, mock.ANY, share['instance']['id'],
+                      expected_updates),
+        ] * changes_allowed)
+
+    def test_get_and_update_access_rule_just_get(self):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        expected_rule = db_utils.create_access(share_id=share['id'])
+        self.mock_object(db, 'share_instance_access_update')
+
+        actual_rule = (
+            self.access_helper.get_and_update_share_instance_access_rule(
+                self.context, expected_rule['id'],
+                share_instance_id=share['instance']['id'])
+        )
+
+        self.assertEqual(expected_rule['id'], actual_rule['access_id'])
+        self.assertFalse(db.share_instance_access_update.called)
+
+    @ddt.data(constants.ACCESS_STATE_APPLYING,
+              constants.ACCESS_STATE_DENYING,
+              constants.ACCESS_STATE_ACTIVE,
+              constants.ACCESS_STATE_QUEUED_TO_APPLY)
+    def test_get_and_update_access_rule_updates_conditionally_changed(
+            self, initial_state):
+        mock_debug_log = self.mock_object(access.LOG, 'debug')
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        rule = db_utils.create_access(share_id=share['id'],
+                                      state=initial_state)
+        self.mock_object(db, 'share_instance_access_update', mock.Mock(
+            side_effect=db.share_instance_access_update))
+        updates = {
+            'access_key': 'renfrow2stars'
+        }
+        conditionally_change = {
+            constants.ACCESS_STATE_APPLYING:
+                constants.ACCESS_STATE_QUEUED_TO_DENY,
+            constants.ACCESS_STATE_DENYING:
+                constants.ACCESS_STATE_QUEUED_TO_DENY,
+        }
+
+        actual_rule = (
+            self.access_helper.get_and_update_share_instance_access_rule(
+                self.context, rule['id'], updates=updates,
+                share_instance_id=share['instance']['id'],
+                conditionally_change=conditionally_change)
+        )
+        self.assertEqual(rule['id'], actual_rule['access_id'])
+        if 'ing' in initial_state:
+            self.assertEqual(constants.ACCESS_STATE_QUEUED_TO_DENY,
+                             actual_rule['state'])
+            self.assertFalse(mock_debug_log.called)
+        else:
+            self.assertEqual(initial_state, actual_rule['state'])
+            mock_debug_log.assert_called_once()
 
 
 @ddt.ddt
@@ -31,282 +226,506 @@ class ShareInstanceAccessTestCase(test.TestCase):
         super(ShareInstanceAccessTestCase, self).setUp()
         self.driver = self.mock_class("manila.share.driver.ShareDriver",
                                       mock.Mock())
-        self.share_access_helper = access.ShareInstanceAccess(db, self.driver)
-        self.context = context.get_admin_context()
-        self.share = db_utils.create_share()
-        self.share_instance = db_utils.create_share_instance(
-            share_id=self.share['id'],
-            access_rules_status=constants.STATUS_ERROR)
-        self.rule = db_utils.create_access(
-            id='fakeaccessid',
-            share_id=self.share['id'],
-            access_to='fakeaccessto')
+        self.access_helper = access.ShareInstanceAccess(db, self.driver)
+        self.context = context.RequestContext('fake_user', 'fake_project')
 
-    @ddt.data(True, False)
-    def test_update_access_rules_maintenance_mode(self, maintenance_mode):
-        existing_rules = []
-        for i in range(2):
-            existing_rules.append(
-                db_utils.create_access(
-                    id='fakeid%s' % i,
-                    share_id=self.share['id'],
-                    access_to='fakeip%s' % i,
-                ))
-        delete_rules = [existing_rules[0], ]
-        rules = [existing_rules[1], ]
-        access_rules_status = (
-            constants.STATUS_ERROR if maintenance_mode
-            else constants.STATUS_ACTIVE)
-        share_instance = db_utils.create_share_instance(
-            id='fakeid',
-            share_id=self.share['id'],
-            access_rules_status=access_rules_status)
+    @ddt.data(constants.ACCESS_STATE_APPLYING, constants.ACCESS_STATE_DENYING)
+    def test_update_access_rules_an_update_is_in_progress(self, initial_state):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        share_instance = share['instance']
+        db_utils.create_access(share_id=share['id'], state=initial_state)
+        mock_debug_log = self.mock_object(access.LOG, 'debug')
+        self.mock_object(self.access_helper, '_update_access_rules')
+        get_and_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules))
 
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=existing_rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(return_value=None))
-        self.mock_object(self.share_access_helper,
-                         "_remove_access_rules", mock.Mock())
-        self.mock_object(self.share_access_helper, "_check_needs_refresh",
-                         mock.Mock(return_value=False))
-
-        self.share_access_helper.update_access_rules(
-            self.context, share_instance['id'],
-            delete_rules=delete_rules)
-
-        self.driver.update_access.assert_called_once_with(
-            self.context, share_instance, rules, add_rules=[],
-            delete_rules=([] if maintenance_mode else delete_rules),
-            share_server=None)
-        self.share_access_helper._remove_access_rules.assert_called_once_with(
-            self.context, delete_rules, share_instance['id'])
-        self.share_access_helper._check_needs_refresh.assert_called_once_with(
-            self.context, rules, share_instance)
-        db.share_instance_update_access_status.assert_called_with(
-            self.context, share_instance['id'], constants.STATUS_ACTIVE)
-
-    @ddt.data(None, {'fakeaccessid': 'fakeaccesskey'})
-    def test_update_access_rules_returns_access_keys(self, access_keys):
-        share_instance = db_utils.create_share_instance(
-            id='fakeshareinstanceid',
-            share_id=self.share['id'],
-            access_rules_status=constants.STATUS_ACTIVE)
-        rules = [self.rule]
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(db, "share_access_update_access_key",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(return_value=access_keys))
-        self.mock_object(self.share_access_helper,
-                         "_remove_access_rules", mock.Mock())
-        self.mock_object(self.share_access_helper, "_check_needs_refresh",
-                         mock.Mock(return_value=False))
-
-        self.share_access_helper.update_access_rules(
-            self.context, share_instance['id'], add_rules=rules)
-
-        self.driver.update_access.assert_called_once_with(
-            self.context, share_instance, rules, add_rules=rules,
-            delete_rules=[], share_server=None)
-        self.share_access_helper._remove_access_rules.assert_called_once_with(
-            self.context, [], share_instance['id'])
-        self.share_access_helper._check_needs_refresh.assert_called_once_with(
-            self.context, rules, share_instance)
-        if access_keys:
-            db.share_access_update_access_key.assert_called_with(
-                self.context, 'fakeaccessid', 'fakeaccesskey')
-        else:
-            self.assertFalse(db.share_access_update_access_key.called)
-        db.share_instance_update_access_status.assert_called_with(
-            self.context, share_instance['id'], constants.STATUS_ACTIVE)
-
-    @ddt.data({'maintenance_mode': True,
-               'access_keys': ['invalidaccesskey']},
-              {'maintenance_mode': True,
-               'access_keys': {'invalidaccessid': 'accesskey'}},
-              {'maintenance_mode': True,
-               'access_keys': {'fakeaccessid': 9}},
-              {'maintenance_mode': False,
-               'access_keys': {'fakeaccessid': 9}})
-    @ddt.unpack
-    def test_update_access_rules_invalid_access_keys(self, maintenance_mode,
-                                                     access_keys):
-        access_rules_status = (
-            constants.STATUS_ERROR if maintenance_mode
-            else constants.STATUS_ACTIVE)
-        share_instance = db_utils.create_share_instance(
-            id='fakeid',
-            share_id=self.share['id'],
-            access_rules_status=access_rules_status)
-
-        rules = [self.rule]
-        add_rules = [] if maintenance_mode else rules
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(return_value=access_keys))
-
-        self.assertRaises(exception.Invalid,
-                          self.share_access_helper.update_access_rules,
-                          self.context, share_instance['id'],
-                          add_rules=add_rules)
-
-        self.driver.update_access.assert_called_once_with(
-            self.context, share_instance, rules, add_rules=add_rules,
-            delete_rules=[], share_server=None)
-
-    def test_update_access_rules_fallback(self):
-        add_rules = [db_utils.create_access(share_id=self.share['id'])]
-        delete_rules = [db_utils.create_access(share_id=self.share['id'])]
-        original_rules = [db_utils.create_access(share_id=self.share['id'])]
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=self.share_instance))
-        self.mock_object(db, "share_access_get_all_for_share",
-                         mock.Mock(return_value=original_rules))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=original_rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(side_effect=NotImplementedError))
-        self.mock_object(self.driver, "allow_access",
-                         mock.Mock())
-        self.mock_object(self.driver, "deny_access",
-                         mock.Mock())
-
-        self.share_access_helper.update_access_rules(self.context,
-                                                     self.share_instance['id'],
-                                                     add_rules, delete_rules)
-
-        self.driver.update_access.assert_called_with(
-            self.context, self.share_instance, original_rules,
-            add_rules=add_rules, delete_rules=[], share_server=None)
-        self.driver.allow_access.assert_called_with(self.context,
-                                                    self.share_instance,
-                                                    add_rules[0],
-                                                    share_server=None)
-        self.assertFalse(self.driver.deny_access.called)
-        db.share_instance_update_access_status.assert_called_with(
-            self.context, self.share_instance['id'], constants.STATUS_ACTIVE)
-
-    def test_update_access_rules_exception(self):
-        original_rules = []
-        add_rules = [db_utils.create_access(share_id=self.share['id'])]
-        delete_rules = 'all'
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=self.share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=original_rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(side_effect=exception.ManilaException))
-
-        self.assertRaises(exception.ManilaException,
-                          self.share_access_helper.update_access_rules,
-                          self.context, self.share_instance['id'], add_rules,
-                          delete_rules)
-
-        self.driver.update_access.assert_called_with(
-            self.context, self.share_instance, [], add_rules=add_rules,
-            delete_rules=original_rules, share_server=None)
-
-        db.share_instance_update_access_status.assert_called_with(
-            self.context, self.share_instance['id'], constants.STATUS_ERROR)
-
-    def test_update_access_rules_recursive_call(self):
-        share_instance = db_utils.create_share_instance(
-            access_rules_status=constants.STATUS_ACTIVE,
-            share_id=self.share['id'])
-        add_rules = [db_utils.create_access(
-            share_id=self.share['id'])]
-        original_rules = []
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=original_rules))
-        mock_update_access = self.mock_object(self.driver, "update_access",
-                                              mock.Mock(return_value=None))
-        self.mock_object(self.share_access_helper, '_check_needs_refresh',
-                         mock.Mock(side_effect=[True, False]))
-
-        self.share_access_helper.update_access_rules(self.context,
-                                                     share_instance['id'],
-                                                     add_rules=add_rules)
-
-        mock_update_access.assert_has_calls([
-            mock.call(self.context, share_instance, original_rules,
-                      add_rules=add_rules, delete_rules=[], share_server=None),
-            mock.call(self.context, share_instance, original_rules,
-                      add_rules=[], delete_rules=[], share_server=None)
-        ])
-
-    @ddt.data(True, False)
-    def test_update_access_rules_migrating(self, read_only_support):
-
-        def override_conf(conf_name):
-            if conf_name == 'migration_readonly_rules_support':
-                return read_only_support
-
-        rules = []
-        for i in range(2):
-            rules.append(
-                db_utils.create_access(
-                    id='fakeid%s' % i,
-                    share_id=self.share['id'],
-                    access_to='fakeip%s' % i,
-                ))
-        driver_rules = [] if not read_only_support else rules
-        access_rules_status = constants.STATUS_OUT_OF_SYNC
-        share_instance = db_utils.create_share_instance(
-            id='fakeid',
-            status=constants.STATUS_MIGRATING,
-            share_id=self.share['id'],
-            access_rules_status=access_rules_status)
-
-        self.mock_object(db, "share_instance_get", mock.Mock(
-            return_value=share_instance))
-        self.mock_object(db, "share_access_get_all_for_instance",
-                         mock.Mock(return_value=rules))
-        self.mock_object(db, "share_instance_update_access_status",
-                         mock.Mock())
-        self.mock_object(self.driver, "update_access",
-                         mock.Mock(return_value=None))
-        self.mock_object(self.share_access_helper,
-                         "_remove_access_rules", mock.Mock())
-        self.mock_object(self.share_access_helper, "_check_needs_refresh",
-                         mock.Mock(return_value=False))
-        self.mock_object(self.driver.configuration, 'safe_get',
-                         mock.Mock(side_effect=override_conf))
-
-        self.share_access_helper.update_access_rules(
+        retval = self.access_helper.update_access_rules(
             self.context, share_instance['id'])
 
-        self.driver.update_access.assert_called_once_with(
-            self.context, share_instance, driver_rules, add_rules=[],
-            delete_rules=[], share_server=None)
-        self.share_access_helper._remove_access_rules.assert_called_once_with(
-            self.context, [], share_instance['id'])
-        self.share_access_helper._check_needs_refresh.assert_called_once_with(
-            self.context, rules, share_instance)
-        db.share_instance_update_access_status.assert_called_with(
-            self.context, share_instance['id'], constants.STATUS_ACTIVE)
+        expected_filters = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        self.assertIsNone(retval)
+        mock_debug_log.assert_called_once()
+        get_and_update_call.assert_called_once_with(
+            self.context, filters=expected_filters,
+            share_instance_id=share_instance['id'])
+        self.assertFalse(self.access_helper._update_access_rules.called)
+
+    def test_update_access_rules_nothing_to_update(self):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        share_instance = share['instance']
+        db_utils.create_access(share_id=share['id'],
+                               state=constants.STATUS_ACTIVE)
+        mock_debug_log = self.mock_object(access.LOG, 'debug')
+        self.mock_object(self.access_helper, '_update_access_rules')
+        get_and_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules))
+
+        retval = self.access_helper.update_access_rules(
+            self.context, share_instance['id'])
+
+        expected_rule_filter_1 = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        expected_rule_filter_2 = {
+            'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                      constants.ACCESS_STATE_QUEUED_TO_DENY),
+        }
+        expected_conditionally_change = {
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_APPLYING,
+            constants.ACCESS_STATE_QUEUED_TO_DENY:
+                constants.ACCESS_STATE_DENYING,
+        }
+        self.assertIsNone(retval)
+        mock_debug_log.assert_called_once()
+        get_and_update_call.assert_has_calls(
+            [
+                mock.call(self.context, filters=expected_rule_filter_1,
+                          share_instance_id=share_instance['id']),
+                mock.call(self.context, filters=expected_rule_filter_2,
+                          share_instance_id=share_instance['id'],
+                          conditionally_change=expected_conditionally_change),
+            ])
+        self.assertFalse(self.access_helper._update_access_rules.called)
+
+    @ddt.data(True, False)
+    def test_update_access_rules_delete_all_rules(self, delete_all_rules):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        share_instance = share['instance']
+        db_utils.create_access(
+            share_id=share['id'], state=constants.STATUS_ACTIVE)
+        db_utils.create_access(
+            share_id=share['id'], state=constants.ACCESS_STATE_QUEUED_TO_APPLY)
+        db_utils.create_access(
+            share_id=share['id'], state=constants.ACCESS_STATE_QUEUED_TO_DENY)
+        mock_debug_log = self.mock_object(access.LOG, 'debug')
+        self.mock_object(self.access_helper, '_update_access_rules')
+        get_and_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules))
+
+        retval = self.access_helper.update_access_rules(
+            self.context, share_instance['id'],
+            delete_all_rules=delete_all_rules)
+
+        expected_rule_filter_1 = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        expected_rule_filter_2 = {
+            'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                      constants.ACCESS_STATE_QUEUED_TO_DENY),
+        }
+        expected_conditionally_change = {
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_APPLYING,
+            constants.ACCESS_STATE_QUEUED_TO_DENY:
+                constants.ACCESS_STATE_DENYING,
+        }
+        expected_get_and_update_calls = []
+        if delete_all_rules:
+            deny_all_updates = {
+                'state': constants.ACCESS_STATE_QUEUED_TO_DENY,
+            }
+            expected_get_and_update_calls = [
+                mock.call(self.context, updates=deny_all_updates,
+                          share_instance_id=share_instance['id']),
+            ]
+        expected_get_and_update_calls.extend([
+            mock.call(self.context, filters=expected_rule_filter_1,
+                      share_instance_id=share_instance['id']),
+            mock.call(self.context, filters=expected_rule_filter_2,
+                      share_instance_id=share_instance['id'],
+                      conditionally_change=expected_conditionally_change),
+        ])
+
+        self.assertIsNone(retval)
+        mock_debug_log.assert_called_once()
+        get_and_update_call.assert_has_calls(expected_get_and_update_calls)
+        self.access_helper._update_access_rules.assert_called_once_with(
+            self.context, share_instance['id'], share_server=None)
+
+    @ddt.data(*itertools.product(
+        (True, False), (constants.ACCESS_STATE_ERROR,
+                        constants.ACCESS_STATE_ACTIVE)))
+    @ddt.unpack
+    def test__update_access_rules_with_driver_updates(
+            self, driver_returns_updates, access_state):
+        expected_access_rules_status = (
+            constants.STATUS_ACTIVE
+            if access_state == constants.ACCESS_STATE_ACTIVE
+            else constants.SHARE_INSTANCE_RULES_ERROR
+        )
+        share = db_utils.create_share(
+            status=constants.STATUS_AVAILABLE,
+            access_rules_status=expected_access_rules_status)
+        share_instance_id = share['instance']['id']
+        rule_1 = db_utils.create_access(
+            share_id=share['id'], state=access_state)
+        rule_1 = db.share_instance_access_get(
+            self.context, rule_1['id'], share_instance_id)
+        rule_2 = db_utils.create_access(
+            share_id=share['id'], state=constants.ACCESS_STATE_APPLYING)
+        rule_2 = db.share_instance_access_get(
+            self.context, rule_2['id'], share_instance_id)
+        rule_3 = db_utils.create_access(
+            share_id=share['id'], state=constants.ACCESS_STATE_DENYING)
+        rule_3 = db.share_instance_access_get(
+            self.context, rule_3['id'], share_instance_id)
+        if driver_returns_updates:
+            driver_rule_updates = {
+                rule_3['access_id']: {'access_key': 'alic3h4sAcc355'},
+                rule_2['access_id']: {'state': access_state}
+            }
+        else:
+            driver_rule_updates = None
+
+        shr_instance_access_rules_status_update_call = self.mock_object(
+            self.access_helper,
+            'get_and_update_share_instance_access_rules_status',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules_status))
+        all_access_rules_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules))
+        one_access_rule_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rule',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rule))
+
+        driver_call = self.mock_object(
+            self.access_helper.driver, 'update_access',
+            mock.Mock(return_value=driver_rule_updates))
+        self.mock_object(self.access_helper, '_check_needs_refresh',
+                         mock.Mock(return_value=False))
+
+        retval = self.access_helper._update_access_rules(
+            self.context, share_instance_id, share_server='fake_server')
+
+        # Expected Values:
+        if access_state != constants.ACCESS_STATE_ERROR:
+            expected_rules_to_be_on_share = [r['id'] for r in (rule_1, rule_2)]
+        else:
+            expected_rules_to_be_on_share = [rule_2['id']]
+
+        expected_filters_1 = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_ACTIVE,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        expected_filters_2 = {'state': constants.STATUS_ERROR}
+        expected_get_and_update_calls = [
+            mock.call(self.context, filters=expected_filters_1,
+                      share_instance_id=share_instance_id),
+            mock.call(self.context, filters=expected_filters_2,
+                      share_instance_id=share_instance_id),
+        ]
+        expected_access_rules_status_change_cond1 = {
+            constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING,
+        }
+        if access_state == constants.SHARE_INSTANCE_RULES_ERROR:
+            expected_access_rules_status_change_cond2 = {
+                constants.SHARE_INSTANCE_RULES_SYNCING:
+                    constants.SHARE_INSTANCE_RULES_ERROR,
+            }
+        else:
+            expected_access_rules_status_change_cond2 = {
+                constants.SHARE_INSTANCE_RULES_SYNCING:
+                    constants.STATUS_ACTIVE,
+                constants.SHARE_INSTANCE_RULES_ERROR:
+                    constants.STATUS_ACTIVE,
+            }
+        call_args = driver_call.call_args_list[0][0]
+        call_kwargs = driver_call.call_args_list[0][1]
+        access_rules_to_be_on_share = [r['id'] for r in call_args[2]]
+
+        # Asserts
+        self.assertIsNone(retval)
+        self.assertEqual(share_instance_id, call_args[1]['id'])
+        self.assertEqual(sorted(expected_rules_to_be_on_share),
+                         sorted(access_rules_to_be_on_share))
+        self.assertEqual(1, len(call_kwargs['add_rules']))
+        self.assertEqual(rule_2['id'], call_kwargs['add_rules'][0]['id'])
+        self.assertEqual(1, len(call_kwargs['delete_rules']))
+        self.assertEqual(rule_3['id'], call_kwargs['delete_rules'][0]['id'])
+        self.assertEqual('fake_server', call_kwargs['share_server'])
+        shr_instance_access_rules_status_update_call.assert_has_calls([
+            mock.call(
+                self.context, share_instance_id=share_instance_id,
+                conditionally_change=expected_access_rules_status_change_cond1
+            ),
+            mock.call(
+                self.context, share_instance_id=share_instance_id,
+                conditionally_change=expected_access_rules_status_change_cond2
+            ),
+        ])
+
+        if driver_returns_updates:
+            expected_conditional_state_updates = {
+                constants.ACCESS_STATE_APPLYING: access_state,
+                constants.ACCESS_STATE_DENYING: access_state,
+                constants.ACCESS_STATE_ACTIVE: access_state,
+            }
+            expected_access_rule_update_calls = [
+                mock.call(
+                    self.context, rule_3['access_id'],
+                    updates={'access_key': 'alic3h4sAcc355'},
+                    share_instance_id=share_instance_id,
+                    conditionally_change={}),
+                mock.call(
+                    self.context, rule_2['access_id'],
+                    updates=mock.ANY, share_instance_id=share_instance_id,
+                    conditionally_change=expected_conditional_state_updates)
+            ]
+            one_access_rule_update_call.assert_has_calls(
+                expected_access_rule_update_calls, any_order=True)
+        else:
+            self.assertFalse(one_access_rule_update_call.called)
+            expected_conditionally_change = {
+                constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE,
+            }
+            expected_get_and_update_calls.append(
+                mock.call(self.context, share_instance_id=share_instance_id,
+                          conditionally_change=expected_conditionally_change))
+
+        all_access_rules_update_call.assert_has_calls(
+            expected_get_and_update_calls, any_order=True)
+
+        share_instance = db.share_instance_get(
+            self.context, share_instance_id)
+        self.assertEqual(expected_access_rules_status,
+                         share_instance['access_rules_status'])
+
+    @ddt.data(True, False)
+    def test__update_access_rules_recursive_driver_exception(self, drv_exc):
+        other = access.ShareInstanceAccess(db, None)
+        share = db_utils.create_share(
+            status=constants.STATUS_AVAILABLE,
+            access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING)
+        share_instance_id = share['instance']['id']
+        rule_4 = []
+        get_and_update_count = [1]
+        drv_count = [1]
+
+        def _get_and_update_side_effect(*args, **kwargs):
+            # The third call to this method needs to create a new access rule
+            mtd = other.get_and_update_share_instance_access_rules
+            if get_and_update_count[0] == 3:
+                rule_4.append(
+                    db_utils.create_access(
+                        state=constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                        share_id=share['id']))
+            get_and_update_count[0] += 1
+            return mtd(*args, **kwargs)
+
+        def _driver_side_effect(*args, **kwargs):
+            if drv_exc and drv_count[0] == 2:
+                raise exception.ManilaException('fake')
+            drv_count[0] += 1
+
+        rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'}
+        rule_1 = db_utils.create_access(state=constants.ACCESS_STATE_APPLYING,
+                                        **rule_kwargs)
+        rule_2 = db_utils.create_access(state=constants.ACCESS_STATE_ACTIVE,
+                                        **rule_kwargs)
+        rule_3 = db_utils.create_access(state=constants.ACCESS_STATE_DENYING,
+                                        **rule_kwargs)
+
+        self.mock_object(self.access_helper,
+                         'get_and_update_share_instance_access_rules',
+                         mock.Mock(side_effect=_get_and_update_side_effect))
+        self.mock_object(self.access_helper.driver, 'update_access',
+                         mock.Mock(side_effect=_driver_side_effect))
+
+        if drv_exc:
+            self.assertRaises(exception.ManilaException,
+                              self.access_helper._update_access_rules,
+                              self.context, share_instance_id)
+        else:
+            retval = self.access_helper._update_access_rules(self.context,
+                                                             share_instance_id)
+            self.assertIsNone(retval)
+
+        expected_filters_1 = {
+            'state': (constants.ACCESS_STATE_APPLYING,
+                      constants.ACCESS_STATE_ACTIVE,
+                      constants.ACCESS_STATE_DENYING),
+        }
+        conditionally_change_2 = {
+            constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ACTIVE,
+        }
+        expected_filters_3 = {
+            'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                      constants.ACCESS_STATE_QUEUED_TO_DENY),
+        }
+        expected_conditionally_change_3 = {
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_APPLYING,
+            constants.ACCESS_STATE_QUEUED_TO_DENY:
+                constants.ACCESS_STATE_DENYING,
+        }
+        expected_conditionally_change_4 = {
+            constants.ACCESS_STATE_APPLYING: constants.ACCESS_STATE_ERROR,
+            constants.ACCESS_STATE_DENYING: constants.ACCESS_STATE_ERROR,
+        }
+        expected_get_and_update_calls = [
+            mock.call(self.context, filters=expected_filters_1,
+                      share_instance_id=share_instance_id),
+            mock.call(self.context, share_instance_id=share_instance_id,
+                      conditionally_change=conditionally_change_2),
+            mock.call(self.context, filters=expected_filters_3,
+                      share_instance_id=share_instance_id,
+                      conditionally_change=expected_conditionally_change_3),
+            mock.call(self.context, filters=expected_filters_1,
+                      share_instance_id=share_instance_id),
+        ]
+
+        if drv_exc:
+            expected_get_and_update_calls.append(
+                mock.call(
+                    self.context, share_instance_id=share_instance_id,
+                    conditionally_change=expected_conditionally_change_4))
+        else:
+            expected_get_and_update_calls.append(
+                mock.call(self.context, share_instance_id=share_instance_id,
+                          conditionally_change=conditionally_change_2))
+
+        # Verify rule changes:
+        # 'denying' rule must not exist
+        self.assertRaises(exception.NotFound,
+                          db.share_access_get,
+                          self.context, rule_3['id'])
+        # 'applying' rule must be set to 'active'
+        rules_that_must_be_active = (rule_1, rule_2)
+        if not drv_exc:
+            rules_that_must_be_active += (rule_4[0], )
+        for rule in rules_that_must_be_active:
+            rule = db.share_access_get(self.context, rule['id'])
+        self.assertEqual(constants.ACCESS_STATE_ACTIVE,
+                         rule['state'])
+        # access_rules_status must be as expected
+        expected_access_rules_status = (
+            constants.SHARE_INSTANCE_RULES_ERROR if drv_exc
+            else constants.STATUS_ACTIVE)
+        share_instance = db.share_instance_get(self.context, share_instance_id)
+        self.assertEqual(
+            expected_access_rules_status,
+            share_instance['access_rules_status'])
+
+    @ddt.data(True, False)
+    def test__update_access_rules_for_migration_driver_supports_ro_rules(
+            self, driver_supports_ro_rules):
+        self.mock_object(self.driver.configuration, 'safe_get', mock.Mock(
+            return_value=driver_supports_ro_rules))
+        share = db_utils.create_share(
+            status=constants.STATUS_MIGRATING,
+            access_rules_status=constants.STATUS_ACTIVE)
+        share_instance_id = share['instance']['id']
+        rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'}
+        rule_1 = db_utils.create_access(
+            state=constants.ACCESS_STATE_ACTIVE, **rule_kwargs)
+        rule_1 = db.share_instance_access_get(
+            self.context, rule_1['id'], share_instance_id)
+        rule_2 = db_utils.create_access(
+            state=constants.ACCESS_STATE_APPLYING, share_id=share['id'],
+            access_level='ro')
+        rule_2 = db.share_instance_access_get(
+            self.context, rule_2['id'], share_instance_id)
+        rule_3 = db_utils.create_access(
+            state=constants.ACCESS_STATE_DENYING, **rule_kwargs)
+        rule_3 = db.share_instance_access_get(
+            self.context, rule_3['id'], share_instance_id)
+
+        driver_call = self.mock_object(
+            self.access_helper.driver, 'update_access',
+            mock.Mock(return_value=None))
+        self.mock_object(self.access_helper, '_check_needs_refresh',
+                         mock.Mock(return_value=False))
+
+        retval = self.access_helper._update_access_rules(
+            self.context, share_instance_id, share_server='fake_server')
+
+        call_args = driver_call.call_args_list[0][0]
+        call_kwargs = driver_call.call_args_list[0][1]
+        access_rules_to_be_on_share = [r['id'] for r in call_args[2]]
+        access_levels = [r['access_level'] for r in call_args[2]]
+        expected_rules_to_be_on_share = (
+            [rule_1['id'], rule_2['id']]
+            if driver_supports_ro_rules else [rule_2['id']]
+        )
+
+        self.assertIsNone(retval)
+        self.assertEqual(share_instance_id, call_args[1]['id'])
+        self.assertEqual(sorted(expected_rules_to_be_on_share),
+                         sorted(access_rules_to_be_on_share))
+        self.assertEqual(['ro'] * len(expected_rules_to_be_on_share),
+                         access_levels)
+        self.assertEqual(0, len(call_kwargs['add_rules']))
+        self.assertEqual(0, len(call_kwargs['delete_rules']))
+        self.assertEqual('fake_server', call_kwargs['share_server'])
+
+    @ddt.data(True, False)
+    def test__check_needs_refresh(self, expected_needs_refresh):
+        states = (
+            [constants.ACCESS_STATE_QUEUED_TO_DENY,
+             constants.ACCESS_STATE_QUEUED_TO_APPLY] if expected_needs_refresh
+            else [constants.ACCESS_STATE_ACTIVE]
+        )
+        share = db_utils.create_share(
+            status=constants.STATUS_AVAILABLE,
+            access_rules_status=constants.SHARE_INSTANCE_RULES_SYNCING)
+        share_instance_id = share['instance']['id']
+        rule_kwargs = {'share_id': share['id'], 'access_level': 'rw'}
+        rule_1 = db_utils.create_access(state=states[0], **rule_kwargs)
+        db_utils.create_access(state=constants.ACCESS_STATE_ACTIVE,
+                               **rule_kwargs)
+        db_utils.create_access(state=constants.ACCESS_STATE_DENYING,
+                               **rule_kwargs)
+        rule_4 = db_utils.create_access(state=states[-1], **rule_kwargs)
+
+        get_and_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
+            mock.Mock(side_effect=self.access_helper.
+                      get_and_update_share_instance_access_rules))
+
+        needs_refresh = self.access_helper._check_needs_refresh(
+            self.context, share_instance_id)
+
+        expected_filter = {
+            'state': (constants.ACCESS_STATE_QUEUED_TO_APPLY,
+                      constants.ACCESS_STATE_QUEUED_TO_DENY),
+        }
+        expected_conditionally_change = {
+            constants.ACCESS_STATE_QUEUED_TO_APPLY:
+                constants.ACCESS_STATE_APPLYING,
+            constants.ACCESS_STATE_QUEUED_TO_DENY:
+                constants.ACCESS_STATE_DENYING,
+        }
+
+        self.assertEqual(expected_needs_refresh, needs_refresh)
+        get_and_update_call.assert_called_once_with(
+            self.context, filters=expected_filter,
+            share_instance_id=share_instance_id,
+            conditionally_change=expected_conditionally_change)
+
+        rule_1 = db.share_instance_access_get(
+            self.context, rule_1['id'], share_instance_id)
+        rule_4 = db.share_instance_access_get(
+            self.context, rule_4['id'], share_instance_id)
+
+        if expected_needs_refresh:
+            self.assertEqual(constants.ACCESS_STATE_DENYING, rule_1['state'])
+            self.assertEqual(constants.ACCESS_STATE_APPLYING, rule_4['state'])
+        else:
+            self.assertEqual(states[0], rule_1['state'])
+            self.assertEqual(states[-1], rule_4['state'])

manila/tests/share/test_api.py

@@ -1903,7 +1903,42 @@ class ShareAPITestCase(test.TestCase):
         share_api.policy.check_policy.assert_called_once_with(
             ctx, 'share_snapshot', 'get_all_snapshots')
 
-    @ddt.data(None, 'rw', 'ro')
+    def test_allow_access_rule_already_exists(self):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        fake_access = db_utils.create_access(share_id=share['id'])
+        self.mock_object(self.api.db, 'share_access_create')
+
+        self.assertRaises(
+            exception.ShareAccessExists, self.api.allow_access,
+            self.context, share, fake_access['access_type'],
+            fake_access['access_to'], fake_access['access_level'])
+        self.assertFalse(self.api.db.share_access_create.called)
+
+    def test_allow_access_invalid_access_level(self):
+        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
+        self.mock_object(self.api.db, 'share_access_create')
+
+        self.assertRaises(
+            exception.InvalidShareAccess, self.api.allow_access,
+            self.context, share, 'user', 'alice', access_level='execute')
+        self.assertFalse(self.api.db.share_access_create.called)
+
+    @ddt.data({'host': None},
+              {'status': constants.STATUS_ERROR_DELETING,
+               'access_rules_status': constants.STATUS_ACTIVE},
+              {'host': None, 'access_rules_status': constants.STATUS_ERROR},
+              {'access_rules_status': constants.STATUS_ERROR})
+    def test_allow_access_invalid_instance(self, params):
+        share = db_utils.create_share(host='fake')
+        db_utils.create_share_instance(share_id=share['id'])
+        db_utils.create_share_instance(share_id=share['id'], **params)
+        self.mock_object(self.api.db, 'share_access_create')
+
+        self.assertRaises(exception.InvalidShare, self.api.allow_access,
+                          self.context, share, 'ip', '10.0.0.1')
+        self.assertFalse(self.api.db.share_access_create.called)
+
+    @ddt.data(*(constants.ACCESS_LEVELS + (None,)))
     def test_allow_access(self, level):
         share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
         values = {
@@ -1920,196 +1955,97 @@ class ShareAPITestCase(test.TestCase):
             'deleted_at': 'fake_deleted_at',
             'instance_mappings': ['foo', 'bar'],
         })
+        self.mock_object(db_api, 'share_get',
+                         mock.Mock(return_value=share))
         self.mock_object(db_api, 'share_access_create',
                          mock.Mock(return_value=fake_access))
         self.mock_object(db_api, 'share_access_get',
                          mock.Mock(return_value=fake_access))
+        self.mock_object(db_api, 'share_access_get_all_by_type_and_access',
+                         mock.Mock(return_value=[]))
+        self.mock_object(self.api, 'allow_access_to_instance')
 
         access = self.api.allow_access(
             self.context, share, fake_access['access_type'],
             fake_access['access_to'], level)
 
         self.assertEqual(fake_access, access)
-        self.share_rpcapi.allow_access.assert_called_once_with(
-            self.context, utils.IsAMatcher(models.ShareInstance),
-            fake_access)
         db_api.share_access_create.assert_called_once_with(
             self.context, values)
-        share_api.policy.check_policy.assert_called_with(
-            self.context, 'share', 'allow_access')
+        self.api.allow_access_to_instance.assert_called_once_with(
+            self.context, share.instance)
 
-    def test_allow_access_existent_access(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        fake_access = db_utils.create_access(share_id=share['id'])
+    def test_allow_access_to_instance(self):
+        share = db_utils.create_share(host='fake')
+        rpc_method = self.mock_object(self.api.share_rpcapi, 'update_access')
 
-        self.assertRaises(exception.ShareAccessExists, self.api.allow_access,
-                          self.context, share, fake_access['access_type'],
-                          fake_access['access_to'], fake_access['access_level']
-                          )
+        self.api.allow_access_to_instance(self.context, share.instance)
 
-    def test_allow_access_invalid_access_level(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        self.assertRaises(exception.InvalidShareAccess, self.api.allow_access,
-                          self.context, share, 'fakeacctype', 'fakeaccto',
-                          'ab')
+        rpc_method.assert_called_once_with(self.context, share.instance)
 
-    def test_allow_access_status_not_available(self):
-        share = db_utils.create_share(status=constants.STATUS_ERROR)
-        self.assertRaises(exception.InvalidShare, self.api.allow_access,
-                          self.context, share, 'fakeacctype', 'fakeaccto')
+    @ddt.data({'host': None},
+              {'status': constants.STATUS_ERROR_DELETING,
+               'access_rules_status': constants.STATUS_ACTIVE},
+              {'host': None, 'access_rules_status': constants.STATUS_ERROR},
+              {'access_rules_status': constants.STATUS_ERROR})
+    def test_deny_access_invalid_instance(self, params):
+        share = db_utils.create_share(host='fake')
+        db_utils.create_share_instance(share_id=share['id'])
+        db_utils.create_share_instance(share_id=share['id'], **params)
+        access_rule = db_utils.create_access(share_id=share['id'])
+        self.mock_object(self.api, 'deny_access_to_instance')
 
-    def test_allow_access_no_host(self):
-        share = db_utils.create_share(host=None)
-        self.assertRaises(exception.InvalidShare, self.api.allow_access,
-                          self.context, share, 'fakeacctype', 'fakeaccto')
+        self.assertRaises(exception.InvalidShare, self.api.deny_access,
+                          self.context, share, access_rule)
+        self.assertFalse(self.api.deny_access_to_instance.called)
 
-    @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_UPDATING)
-    def test_allow_access_to_instance(self, status):
+    def test_deny_access(self):
+        share = db_utils.create_share(
+            host='fake', status=constants.STATUS_AVAILABLE,
+            access_rules_status=constants.STATUS_ACTIVE)
+        access_rule = db_utils.create_access(share_id=share['id'])
+        self.mock_object(self.api, 'deny_access_to_instance')
+
+        retval = self.api.deny_access(self.context, share, access_rule)
+
+        self.assertIsNone(retval)
+        self.api.deny_access_to_instance.assert_called_once_with(
+            self.context, share.instance, access_rule)
+
+    def test_deny_access_to_instance(self):
         share = db_utils.create_share(host='fake')
         share_instance = db_utils.create_share_instance(
-            share_id=share['id'], access_rules_status=status, host='fake')
+            share_id=share['id'], host='fake')
         access = db_utils.create_access(share_id=share['id'])
-        rpc_method = self.mock_object(self.api.share_rpcapi, 'allow_access')
-
-        self.api.allow_access_to_instance(self.context, share_instance, access)
-
-        rpc_method.assert_called_once_with(
-            self.context, share_instance, access)
-
-    def test_allow_access_to_instance_exception(self):
-        share = db_utils.create_share(host='fake')
-        access = db_utils.create_access(share_id=share['id'])
-
-        share.instance['access_rules_status'] = constants.STATUS_ERROR
-
-        self.assertRaises(exception.InvalidShareInstance,
-                          self.api.allow_access_to_instance, self.context,
-                          share.instance, access)
-
-    def test_allow_access_to_instance_out_of_sync(self):
-        share = db_utils.create_share(host='fake')
-        access = db_utils.create_access(share_id=share['id'])
-        rpc_method = self.mock_object(self.api.share_rpcapi, 'allow_access')
-
-        share.instance['access_rules_status'] = constants.STATUS_OUT_OF_SYNC
-
-        self.api.allow_access_to_instance(self.context, share.instance, access)
-        rpc_method.assert_called_once_with(
-            self.context, share.instance, access)
-
-    @ddt.data(constants.STATUS_ACTIVE, constants.STATUS_UPDATING,
-              constants.STATUS_UPDATING_MULTIPLE)
-    def test_deny_access_to_instance(self, status):
-        share = db_utils.create_share(host='fake')
-        share_instance = db_utils.create_share_instance(
-            share_id=share['id'], access_rules_status=status, host='fake')
-        access = db_utils.create_access(share_id=share['id'])
-        rpc_method = self.mock_object(self.api.share_rpcapi, 'deny_access')
+        rpc_method = self.mock_object(self.api.share_rpcapi, 'update_access')
         self.mock_object(db_api, 'share_instance_access_get',
                          mock.Mock(return_value=access.instance_mappings[0]))
-        self.mock_object(db_api, 'share_instance_update_access_status')
+        mock_share_instance_rules_status_update = self.mock_object(
+            self.api.access_helper,
+            'get_and_update_share_instance_access_rules_status')
+        mock_access_rule_state_update = self.mock_object(
+            self.api.access_helper,
+            'get_and_update_share_instance_access_rule')
 
         self.api.deny_access_to_instance(self.context, share_instance, access)
 
-        if status == constants.STATUS_ACTIVE:
-            expected_new_status = constants.STATUS_OUT_OF_SYNC
-        else:
-            expected_new_status = constants.STATUS_UPDATING_MULTIPLE
-
-        rpc_method.assert_called_once_with(
-            self.context, share_instance, access)
-        db_api.share_instance_update_access_status.assert_called_once_with(
-            self.context,
-            share_instance['id'],
-            expected_new_status
-        )
-
-    @ddt.data('allow_access_to_instance', 'deny_access_to_instance')
-    def test_allow_and_deny_access_to_instance_invalid_instance(self, method):
-        share = db_utils.create_share(host=None)
-
-        self.assertRaises(
-            exception.InvalidShareInstance,
-            getattr(self.api, method),
-            self.context, share.instance, 'fake'
-        )
-
-    @mock.patch.object(db_api, 'share_get', mock.Mock())
-    @mock.patch.object(share_api.API, 'deny_access_to_instance', mock.Mock())
-    @mock.patch.object(db_api, 'share_instance_update_access_status',
-                       mock.Mock())
-    def test_deny_access_error(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        db_api.share_get.return_value = share
-        access = db_utils.create_access(share_id=share['id'])
-        share_instance = share.instances[0]
-
-        self.api.deny_access(self.context, share, access)
-        db_api.share_get.assert_called_once_with(self.context, share['id'])
-        share_api.policy.check_policy.assert_called_once_with(
-            self.context, 'share', 'deny_access')
-        share_api.API.deny_access_to_instance.assert_called_once_with(
-            self.context, share_instance, access)
-
-    @mock.patch.object(db_api, 'share_get', mock.Mock())
-    @mock.patch.object(db_api, 'share_access_delete', mock.Mock())
-    def test_deny_access_error_no_share_instance_mapping(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        db_api.share_get.return_value = share
-        access = db_utils.create_access(share_id=share['id'])
-
-        self.api.deny_access(self.context, share, access)
-
-        db_api.share_get.assert_called_once_with(self.context, share['id'])
-        self.assertTrue(share_api.policy.check_policy.called)
-
-    @mock.patch.object(db_api, 'share_instance_update_access_status',
-                       mock.Mock())
-    def test_deny_access_active(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        access = db_utils.create_access(share_id=share['id'])
-        self.api.deny_access(self.context, share, access)
-        db_api.share_instance_update_access_status.assert_called_once_with(
-            self.context,
-            share.instance['id'],
-            constants.STATUS_OUT_OF_SYNC
-        )
-        share_api.policy.check_policy.assert_called_with(
-            self.context, 'share', 'deny_access')
-        self.share_rpcapi.deny_access.assert_called_once_with(
-            self.context, utils.IsAMatcher(models.ShareInstance), access)
-
-    def test_deny_access_not_found(self):
-        share = db_utils.create_share(status=constants.STATUS_AVAILABLE)
-        access = db_utils.create_access(share_id=share['id'])
-        self.mock_object(db_api, 'share_instance_access_get',
-                         mock.Mock(side_effect=[exception.NotFound('fake')]))
-        self.api.deny_access(self.context, share, access)
-        share_api.policy.check_policy.assert_called_with(
-            self.context, 'share', 'deny_access')
-
-    def test_deny_access_status_not_available(self):
-        share = db_utils.create_share(status=constants.STATUS_ERROR)
-        self.assertRaises(exception.InvalidShare, self.api.deny_access,
-                          self.context, share, 'fakeacc')
-        share_api.policy.check_policy.assert_called_once_with(
-            self.context, 'share', 'deny_access')
-
-    def test_deny_access_no_host(self):
-        share = db_utils.create_share(
-            status=constants.STATUS_AVAILABLE, host=None)
-        self.assertRaises(exception.InvalidShare, self.api.deny_access,
-                          self.context, share, 'fakeacc')
-        share_api.policy.check_policy.assert_called_once_with(
-            self.context, 'share', 'deny_access')
+        rpc_method.assert_called_once_with(self.context, share_instance)
+        mock_access_rule_state_update.assert_called_once_with(
+            self.context, access['id'],
+            updates={'state': constants.ACCESS_STATE_QUEUED_TO_DENY},
+            share_instance_id=share_instance['id'])
+        expected_conditional_change = {
+            constants.STATUS_ACTIVE: constants.SHARE_INSTANCE_RULES_SYNCING,
+        }
+        mock_share_instance_rules_status_update.assert_called_once_with(
+            self.context, share_instance_id=share_instance['id'],
+            conditionally_change=expected_conditional_change)
 
     def test_access_get(self):
         with mock.patch.object(db_api, 'share_access_get',
                                mock.Mock(return_value='fake')):
             rule = self.api.access_get(self.context, 'fakeid')
             self.assertEqual('fake', rule)
-            share_api.policy.check_policy.assert_called_once_with(
-                self.context, 'share', 'access_get')
             db_api.share_access_get.assert_called_once_with(
                 self.context, 'fakeid')
 

manila/tests/share/test_manager.py

@@ -32,7 +32,6 @@ from manila import db
 from manila.db.sqlalchemy import models
 from manila import exception
 from manila import quota
-from manila.share import access as share_access
 from manila.share import api
 from manila.share import drivers_private_data
 from manila.share import manager
@@ -193,8 +192,7 @@ class ShareManagerTestCase(test.TestCase):
         "delete_free_share_servers",
         "create_snapshot",
         "delete_snapshot",
-        "allow_access",
-        "deny_access",
+        "update_access",
         "_report_driver_status",
         "_execute_periodic_hook",
         "publish_service_capabilities",
@@ -265,7 +263,8 @@ class ShareManagerTestCase(test.TestCase):
                 display_name='fake_name_6').instance,
         ]
 
-        instances[4]['access_rules_status'] = constants.STATUS_OUT_OF_SYNC
+        instances[4]['access_rules_status'] = (
+            constants.SHARE_INSTANCE_RULES_SYNCING)
 
         if not setup_access_rules:
             return instances
@@ -639,7 +638,8 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(db, 'share_instance_access_get',
                          mock.Mock(return_value=fake_access_rules[0]))
         mock_share_replica_access_update = self.mock_object(
-            db, 'share_instance_update_access_status')
+            self.share_manager.access_helper,
+            'get_and_update_share_instance_access_rules_status')
         self.mock_object(self.share_manager, '_get_share_server')
 
         driver_call = self.mock_object(
@@ -650,9 +650,13 @@ class ShareManagerTestCase(test.TestCase):
                           self.share_manager.create_share_replica,
                           self.context, replica)
         mock_replica_update_call.assert_called_once_with(
-            mock.ANY, replica['id'], {'status': constants.STATUS_ERROR,
+            utils.IsAMatcher(context.RequestContext), replica['id'],
+            {'status': constants.STATUS_ERROR,
              'replica_state': constants.STATUS_ERROR})
-        self.assertEqual(1, mock_share_replica_access_update.call_count)
+        mock_share_replica_access_update.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext),
+            share_instance_id=replica['id'],
+            status=constants.SHARE_INSTANCE_RULES_ERROR)
         self.assertFalse(mock_export_locs_update_call.called)
         self.assertTrue(mock_log_error.called)
         self.assertFalse(mock_log_info.called)
@@ -662,7 +666,8 @@ class ShareManagerTestCase(test.TestCase):
         driver_retval = {
             'export_locations': 'FAKE_EXPORT_LOC',
         }
-        replica = fake_replica(share_network='')
+        replica = fake_replica(share_network='',
+                               access_rules_status=constants.STATUS_ACTIVE)
         replica_2 = fake_replica(id='fake2')
         fake_access_rules = [{'id': '1'}, {'id': '2'}]
         self.mock_object(db, 'share_replicas_get_available_active_replica',
@@ -693,12 +698,15 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(db, 'share_instance_access_get',
                          mock.Mock(return_value=fake_access_rules[0]))
         mock_share_replica_access_update = self.mock_object(
-            db, 'share_instance_update_access_status')
+            self.share_manager.access_helper,
+            'get_and_update_share_instance_access_rules_status')
 
         self.share_manager.create_share_replica(self.context, replica)
 
         self.assertFalse(mock_replica_update_call.called)
-        self.assertEqual(1, mock_share_replica_access_update.call_count)
+        mock_share_replica_access_update.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext),
+            share_instance_id=replica['id'], status=constants.STATUS_ACTIVE)
         self.assertFalse(mock_export_locs_update_call.called)
         self.assertTrue(mock_log_info.called)
         self.assertTrue(mock_log_warning.called)
@@ -804,7 +812,8 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(db, 'share_instance_access_get',
                          mock.Mock(return_value=fake_access_rules[0]))
         mock_share_replica_access_update = self.mock_object(
-            db, 'share_instance_update_access_status')
+            self.share_manager.access_helper,
+            'get_and_update_share_instance_access_rules_status')
         driver_call = self.mock_object(
             self.share_manager.driver, 'create_replica',
             mock.Mock(return_value=replica))
@@ -813,10 +822,13 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.create_share_replica(self.context, replica)
 
         mock_replica_update_call.assert_called_once_with(
-            mock.ANY, replica['id'],
+            utils.IsAMatcher(context.RequestContext), replica['id'],
             {'status': constants.STATUS_AVAILABLE,
              'replica_state': constants.REPLICA_STATE_IN_SYNC})
-        self.assertEqual(1, mock_share_replica_access_update.call_count)
+        mock_share_replica_access_update.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext),
+            share_instance_id=replica['id'],
+            status=replica['access_rules_status'])
         self.assertTrue(mock_export_locs_update_call.called)
         self.assertTrue(mock_log_info.called)
         self.assertFalse(mock_log_warning.called)
@@ -2292,7 +2304,7 @@ class ShareManagerTestCase(test.TestCase):
 
         smanager.driver.unmanage.assert_called_once_with(mock.ANY)
         smanager.access_helper.update_access_rules.assert_called_once_with(
-            mock.ANY, mock.ANY, delete_rules='all', share_server=None
+            mock.ANY, mock.ANY, delete_all_rules=True, share_server=None
         )
         smanager.db.share_instance_delete.assert_called_once_with(
             mock.ANY, share_instance_id)
@@ -2330,12 +2342,17 @@ class ShareManagerTestCase(test.TestCase):
                 context.get_admin_context(), share_srv['id'], backend_details)
         share = db_utils.create_share(share_network_id=share_net['id'],
                                       share_server_id=share_srv['id'])
+        mock_access_helper_call = self.mock_object(
+            self.share_manager.access_helper, 'update_access_rules')
         self.share_manager.driver = mock.Mock()
         manager.CONF.delete_share_server_with_last_share = True
 
         self.share_manager.delete_share_instance(self.context,
                                                  share.instance['id'])
 
+        mock_access_helper_call.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext), share.instance['id'],
+            delete_all_rules=True, share_server=mock.ANY)
         self.share_manager.driver.teardown_server.assert_called_once_with(
             server_details=backend_details,
             security_services=[jsonutils.loads(
@@ -2353,19 +2370,24 @@ class ShareManagerTestCase(test.TestCase):
         )
         share = db_utils.create_share(share_network_id=share_net['id'],
                                       share_server_id=share_srv['id'])
-
+        share_srv = db.share_server_get(self.context, share_srv['id'])
+        mock_access_helper_call = self.mock_object(
+            self.share_manager.access_helper, 'update_access_rules')
         self.share_manager.driver = mock.Mock()
         if side_effect == 'update_access':
-            self.mock_object(
-                self.share_manager.access_helper, 'update_access_rules',
-                mock.Mock(side_effect=Exception('fake')))
+            mock_access_helper_call.side_effect = exception.ManilaException
         if side_effect == 'delete_share':
             self.mock_object(self.share_manager.driver, 'delete_share',
                              mock.Mock(side_effect=Exception('fake')))
         self.mock_object(manager.LOG, 'error')
         manager.CONF.delete_share_server_with_last_share = True
+
         self.share_manager.delete_share_instance(
             self.context, share.instance['id'], force=force)
+
+        mock_access_helper_call.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext), share.instance['id'],
+            delete_all_rules=True, share_server=mock.ANY)
         self.share_manager.driver.teardown_server.assert_called_once_with(
             server_details=share_srv.get('backend_details'),
             security_services=[])
@@ -2379,11 +2401,21 @@ class ShareManagerTestCase(test.TestCase):
         )
         share = db_utils.create_share(share_network_id=share_net['id'],
                                       share_server_id=share_srv['id'])
+        share_srv = db.share_server_get(self.context, share_srv['id'])
 
         manager.CONF.delete_share_server_with_last_share = False
         self.share_manager.driver = mock.Mock()
+        mock_access_helper_call = self.mock_object(
+            self.share_manager.access_helper, 'update_access_rules')
+        self.mock_object(db, 'share_server_get',
+                         mock.Mock(return_value=share_srv))
+
         self.share_manager.delete_share_instance(self.context,
                                                  share.instance['id'])
+
+        mock_access_helper_call.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext), share.instance['id'],
+            delete_all_rules=True, share_server=share_srv)
         self.assertFalse(self.share_manager.driver.teardown_network.called)
 
     def test_delete_share_instance_not_last_on_server(self):
@@ -2396,11 +2428,21 @@ class ShareManagerTestCase(test.TestCase):
                                       share_server_id=share_srv['id'])
         db_utils.create_share(share_network_id=share_net['id'],
                               share_server_id=share_srv['id'])
+        share_srv = db.share_server_get(self.context, share_srv['id'])
 
         manager.CONF.delete_share_server_with_last_share = True
         self.share_manager.driver = mock.Mock()
+        self.mock_object(db, 'share_server_get',
+                         mock.Mock(return_value=share_srv))
+        mock_access_helper_call = self.mock_object(
+            self.share_manager.access_helper, 'update_access_rules')
+
         self.share_manager.delete_share_instance(self.context,
                                                  share.instance['id'])
+
+        mock_access_helper_call.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext), share.instance['id'],
+            delete_all_rules=True, share_server=share_srv)
         self.assertFalse(self.share_manager.driver.teardown_network.called)
 
     @ddt.data('update_access', 'delete_share')
@@ -2414,6 +2456,7 @@ class ShareManagerTestCase(test.TestCase):
         access = db_utils.create_access(share_id=share['id'])
         db_utils.create_share(share_network_id=share_net['id'],
                               share_server_id=share_srv['id'])
+        share_srv = db.share_server_get(self.context, share_srv['id'])
 
         manager.CONF.delete_share_server_with_last_share = False
 
@@ -2426,23 +2469,19 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.driver = mock.Mock()
         self.share_manager.access_helper.driver = mock.Mock()
         if side_effect == 'update_access':
-            self.mock_object(
-                self.share_manager.access_helper.driver, 'update_access',
+            mock_access_helper_call = self.mock_object(
+                self.share_manager.access_helper, 'update_access_rules',
                 mock.Mock(side_effect=exception.ShareResourceNotFound(
                     share_id=share['id'])))
         if side_effect == 'delete_share':
-            self.mock_object(
-                self.share_manager.access_helper.driver, 'update_access',
+            mock_access_helper_call = self.mock_object(
+                self.share_manager.access_helper, 'update_access_rules',
                 mock.Mock(return_value=None)
             )
             self.mock_object(
                 self.share_manager.driver, 'delete_share',
                 mock.Mock(side_effect=exception.ShareResourceNotFound(
                     share_id=share['id'])))
-            self.mock_object(
-                self.share_manager.access_helper, '_check_needs_refresh',
-                mock.Mock(return_value=False)
-            )
 
         self.mock_object(manager.LOG, 'warning')
 
@@ -2450,78 +2489,11 @@ class ShareManagerTestCase(test.TestCase):
                                                  share.instance['id'])
         self.assertFalse(self.share_manager.driver.teardown_network.called)
 
-        (self.share_manager.access_helper.driver.update_access.
-            assert_called_once_with(utils.IsAMatcher(
-                context.RequestContext), share.instance, [], add_rules=[],
-                delete_rules=[access], share_server=share_srv))
-
+        mock_access_helper_call.assert_called_once_with(
+            utils.IsAMatcher(context.RequestContext), share.instance['id'],
+            delete_all_rules=True, share_server=share_srv)
         self.assertTrue(manager.LOG.warning.called)
 
-    def test_allow_deny_access(self):
-        """Test access rules to share can be created and deleted."""
-        self.mock_object(share_access.LOG, 'info')
-
-        share = db_utils.create_share()
-        share_id = share['id']
-        share_instance = db_utils.create_share_instance(
-            share_id=share_id,
-            access_rules_status=constants.STATUS_OUT_OF_SYNC)
-        share_instance_id = share_instance['id']
-        access = db_utils.create_access(share_id=share_id,
-                                        share_instance_id=share_instance_id)
-        access_id = access['id']
-
-        self.share_manager.allow_access(self.context, share_instance_id,
-                                        [access_id])
-        self.assertEqual('active', db.share_instance_get(
-            self.context, share_instance_id).access_rules_status)
-
-        share_access.LOG.info.assert_called_with(mock.ANY,
-                                                 share_instance_id)
-        share_access.LOG.info.reset_mock()
-
-        self.share_manager.deny_access(self.context, share_instance_id,
-                                       [access_id])
-
-        share_access.LOG.info.assert_called_with(mock.ANY,
-                                                 share_instance_id)
-        share_access.LOG.info.reset_mock()
-
-    def test_allow_deny_access_error(self):
-        """Test access rules to share can be created and deleted with error."""
-
-        def _fake_allow_access(self, *args, **kwargs):
-            raise exception.NotFound()
-
-        def _fake_deny_access(self, *args, **kwargs):
-            raise exception.NotFound()
-
-        self.mock_object(self.share_manager.access_helper.driver,
-                         "allow_access", _fake_allow_access)
-        self.mock_object(self.share_manager.access_helper.driver,
-                         "deny_access", _fake_deny_access)
-
-        share = db_utils.create_share()
-        share_id = share['id']
-        share_instance = db_utils.create_share_instance(
-            share_id=share_id,
-            access_rules_status=constants.STATUS_OUT_OF_SYNC)
-        share_instance_id = share_instance['id']
-        access = db_utils.create_access(share_id=share_id,
-                                        share_instance_id=share_instance_id)
-        access_id = access['id']
-
-        def validate(method):
-            self.assertRaises(exception.ManilaException, method, self.context,
-                              share_instance_id, [access_id])
-
-            inst = db.share_instance_get(self.context, share_instance_id)
-            self.assertEqual(constants.STATUS_ERROR,
-                             inst['access_rules_status'])
-
-        validate(self.share_manager.allow_access)
-        validate(self.share_manager.deny_access)
-
     def test_setup_server(self):
         # Setup required test data
         share_server = {
@@ -3794,8 +3766,8 @@ class ShareManagerTestCase(test.TestCase):
                          mock.Mock(return_value=server))
         self.mock_object(self.share_manager.db, 'share_instance_update',
                          mock.Mock(return_value=server))
-        self.mock_object(self.share_manager.db,
-                         'share_instance_update_access_status')
+        self.mock_object(self.share_manager.access_helper,
+                         'get_and_update_share_instance_access_rules')
         self.mock_object(self.share_manager.access_helper,
                          'update_access_rules')
         if exc is None:
@@ -3828,18 +3800,15 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.db.share_server_get.assert_called_once_with(
             utils.IsAMatcher(context.RequestContext),
             instance['share_server_id'])
-        (self.share_manager.db.share_instance_update_access_status.
-         assert_called_once_with(self.context, instance['id'],
-                                 constants.STATUS_OUT_OF_SYNC))
         (self.share_manager.access_helper.update_access_rules.
          assert_called_once_with(
              self.context, instance['id'], share_server=server))
         migration_api.ShareMigrationHelper.create_instance_and_wait.\
             assert_called_once_with(share, 'fake_host', 'fake_net_id',
                                     'fake_az_id', 'fake_type_id')
-        migration_api.ShareMigrationHelper.\
+        (migration_api.ShareMigrationHelper.
             cleanup_access_rules.assert_called_once_with(
-                instance, server, self.share_manager.driver)
+                instance, server))
         if exc is None:
             self.share_manager.db.share_instance_update.\
                 assert_called_once_with(
@@ -3904,10 +3873,9 @@ class ShareManagerTestCase(test.TestCase):
             mock.Mock(return_value=dest_server))
         self.mock_object(self.share_manager.driver, 'migration_start')
         self.mock_object(self.share_manager, '_migration_delete_instance')
-        self.mock_object(self.share_manager.db,
-                         'share_instance_update_access_status')
         self.mock_object(self.share_manager.access_helper,
                          'update_access_rules')
+        self.mock_object(utils, 'wait_for_access_update')
 
         # run
         if exc:
@@ -3934,9 +3902,6 @@ class ShareManagerTestCase(test.TestCase):
                     {'task_state':
                      constants.TASK_STATE_MIGRATION_DRIVER_IN_PROGRESS})
             ])
-            (self.share_manager.db.share_instance_update_access_status.
-             assert_called_once_with(self.context, src_instance['id'],
-                                     constants.STATUS_OUT_OF_SYNC))
             (self.share_manager.access_helper.update_access_rules.
              assert_called_once_with(
                  self.context, src_instance['id'], share_server=src_server))
@@ -4020,6 +3985,7 @@ class ShareManagerTestCase(test.TestCase):
         self.mock_object(self.share_manager.driver,
                          'migration_check_compatibility',
                          mock.Mock(return_value=compatibility))
+        self.mock_object(utils, 'wait_for_access_update')
 
         # run
         self.assertRaises(
@@ -4257,9 +4223,8 @@ class ShareManagerTestCase(test.TestCase):
         if status != 'other':
             migration_api.ShareMigrationHelper.cleanup_new_instance.\
                 assert_called_once_with(new_instance)
-            migration_api.ShareMigrationHelper.cleanup_access_rules.\
-                assert_called_once_with(instance, server,
-                                        self.share_manager.driver)
+            (migration_api.ShareMigrationHelper.cleanup_access_rules.
+                assert_called_once_with(instance, server))
         if status == constants.TASK_STATE_MIGRATION_CANCELLED:
             self.share_manager.db.share_instance_update.\
                 assert_called_once_with(self.context, instance['id'],
@@ -4432,8 +4397,7 @@ class ShareManagerTestCase(test.TestCase):
             (migration_api.ShareMigrationHelper.cleanup_new_instance.
              assert_called_once_with(instance_2))
             (migration_api.ShareMigrationHelper.cleanup_access_rules.
-             assert_called_once_with(instance_1, server_1,
-                                     self.share_manager.driver))
+             assert_called_once_with(instance_1, server_1))
 
         else:
             self.share_manager.driver.migration_cancel.assert_called_once_with(
@@ -4466,19 +4430,19 @@ class ShareManagerTestCase(test.TestCase):
     def test__migration_delete_instance(self):
 
         instance = db_utils.create_share_instance(share_id='fake_id')
-        mapping_list = [{'id': 'mapping_id_1'}, {'id': 'mapping_id_2'}]
         rules = [{'id': 'rule_id_1'}, {'id': 'rule_id_2'}]
 
         # mocks
         self.mock_object(self.share_manager.db, 'share_instance_get',
                          mock.Mock(return_value=instance))
         self.mock_object(self.share_manager.db, 'share_instance_update')
-        self.mock_object(
-            self.share_manager.db, 'share_access_get_all_for_instance',
+        mock_get_access_rules_call = self.mock_object(
+            self.share_manager.access_helper,
+            'get_and_update_share_instance_access_rules',
             mock.Mock(return_value=rules))
-        self.mock_object(
-            self.share_manager.db, 'share_instance_access_get', mock.Mock(
-                side_effect=[mapping_list[0], mapping_list[1]]))
+        mock_delete_access_rules_call = self.mock_object(
+            self.share_manager.access_helper,
+            'delete_share_instance_access_rules')
         self.mock_object(self.share_manager.db, 'share_instance_delete')
         self.mock_object(self.share_manager.db, 'share_instance_access_delete')
         self.mock_object(self.share_manager, '_check_delete_share_server')
@@ -4493,16 +4457,10 @@ class ShareManagerTestCase(test.TestCase):
         self.share_manager.db.share_instance_update.assert_called_once_with(
             self.context, instance['id'],
             {'status': constants.STATUS_INACTIVE})
-        (self.share_manager.db.share_access_get_all_for_instance.
-            assert_called_once_with(self.context, instance['id']))
-        self.share_manager.db.share_instance_access_get.assert_has_calls([
-            mock.call(self.context, 'rule_id_1', instance['id']),
-            mock.call(self.context, 'rule_id_2', instance['id'])
-        ])
-        self.share_manager.db.share_instance_access_delete.assert_has_calls([
-            mock.call(self.context, 'mapping_id_1'),
-            mock.call(self.context, 'mapping_id_2')
-        ])
+        mock_get_access_rules_call.assert_called_once_with(
+            self.context, share_instance_id=instance['id'])
+        mock_delete_access_rules_call.assert_called_once_with(
+            self.context, rules, instance['id'])
         self.share_manager.db.share_instance_delete.assert_called_once_with(
             self.context, instance['id'])
         self.share_manager._check_delete_share_server.assert_called_once_with(
@@ -5599,6 +5557,23 @@ class ShareManagerTestCase(test.TestCase):
             self.assertFalse(mock_db_update_call.called)
         self.assertFalse(mock_db_delete_call.called)
 
+    def test_update_access(self):
+        share_instance = fakes.fake_share_instance()
+        self.mock_object(self.share_manager, '_get_share_server',
+                         mock.Mock(return_value='fake_share_server'))
+        self.mock_object(self.share_manager, '_get_share_instance',
+                         mock.Mock(return_value=share_instance))
+        access_rules_update_method = self.mock_object(
+            self.share_manager.access_helper, 'update_access_rules')
+
+        retval = self.share_manager.update_access(
+            self.context, share_instance['id'])
+
+        self.assertIsNone(retval)
+        access_rules_update_method.assert_called_once_with(
+            self.context, share_instance['id'],
+            share_server='fake_share_server')
+
 
 @ddt.ddt
 class HookWrapperTestCase(test.TestCase):

manila/tests/share/test_migration.py

@@ -22,6 +22,7 @@ from manila.common import constants
 from manila import context
 from manila import db
 from manila import exception
+from manila.share import access as access_helper
 from manila.share import api as share_api
 from manila.share import migration
 from manila import test
@@ -39,9 +40,10 @@ class ShareMigrationHelperTestCase(test.TestCase):
         self.share_instance = db_utils.create_share_instance(
             share_id=self.share['id'],
             share_network_id='fake_network_id')
+        self.access_helper = access_helper.ShareInstanceAccess(db, None)
         self.context = context.get_admin_context()
         self.helper = migration.ShareMigrationHelper(
-            self.context, db, self.share)
+            self.context, db, self.share, self.access_helper)
 
     def test_delete_instance_and_wait(self):
 
@@ -260,38 +262,31 @@ class ShareMigrationHelperTestCase(test.TestCase):
         server = db_utils.create_share_server(share_id=self.share['id'])
 
         # mocks
-        share_driver = mock.Mock()
-        self.mock_object(share_driver, 'update_access')
-
-        self.mock_object(db, 'share_access_get_all_for_instance',
+        self.mock_object(self.access_helper, 'update_access_rules')
+        get_and_update_call = self.mock_object(
+            self.access_helper, 'get_and_update_share_instance_access_rules',
             mock.Mock(return_value=[access]))
 
         # run
-        self.helper.revert_access_rules(share_instance, server, share_driver)
+        self.helper.revert_access_rules(share_instance, server)
 
         # asserts
-        db.share_access_get_all_for_instance.assert_called_once_with(
-            self.context, share_instance['id'])
-        share_driver.update_access.assert_called_once_with(
-            self.context, share_instance, [access], add_rules=[],
-            delete_rules=[], share_server=server)
+        get_and_update_call.assert_called_once_with(
+            self.context, share_instance_id=share_instance['id'],
+            updates={'state': constants.ACCESS_STATE_QUEUED_TO_APPLY})
+        self.access_helper.update_access_rules.assert_called_once_with(
+            self.context, share_instance['id'], share_server=server)
 
     def test_apply_new_access_rules(self):
 
         new_share_instance = db_utils.create_share_instance(
             share_id=self.share['id'], status=constants.STATUS_AVAILABLE,
             access_rules_status='active')
-
-        access = db_utils.create_access(share_id=self.share['id'],
-                                        access_to='fake_ip',
-                                        access_level='rw')
+        db_utils.create_access(
+            share_id=self.share['id'], access_to='fake_ip', access_level='rw')
 
         # mocks
-        self.mock_object(db, 'share_instance_get',
-                         mock.Mock(return_value=new_share_instance))
         self.mock_object(db, 'share_instance_access_copy')
-        self.mock_object(db, 'share_access_get_all_for_instance',
-                         mock.Mock(return_value=[access]))
         self.mock_object(share_api.API, 'allow_access_to_instance')
         self.mock_object(utils, 'wait_for_access_update')
 
@@ -299,14 +294,10 @@ class ShareMigrationHelperTestCase(test.TestCase):
         self.helper.apply_new_access_rules(new_share_instance)
 
         # asserts
-        db.share_instance_get.assert_called_once_with(
-            self.context, new_share_instance['id'], with_share_data=True)
         db.share_instance_access_copy(self.context, self.share['id'],
                                       new_share_instance['id'])
-        db.share_access_get_all_for_instance.assert_called_once_with(
-            self.context, new_share_instance['id'])
         share_api.API.allow_access_to_instance.assert_called_with(
-            self.context, new_share_instance, [access])
+            self.context, new_share_instance)
         utils.wait_for_access_update.assert_called_with(
             self.context, db, new_share_instance,
             self.helper.migration_wait_access_rules_timeout)
@@ -335,7 +326,6 @@ class ShareMigrationHelperTestCase(test.TestCase):
 
         # mocks
         server = db_utils.create_share_server()
-        share_driver = mock.Mock()
         self.mock_object(self.helper, 'revert_access_rules',
                          mock.Mock(side_effect=exc))
         self.mock_object(self.helper.db, 'share_instance_update')
@@ -343,12 +333,11 @@ class ShareMigrationHelperTestCase(test.TestCase):
         self.mock_object(migration.LOG, 'warning')
 
         # run
-        self.helper.cleanup_access_rules(self.share_instance, server,
-                                         share_driver)
+        self.helper.cleanup_access_rules(self.share_instance, server)
 
         # asserts
         self.helper.revert_access_rules.assert_called_once_with(
-            self.share_instance, server, share_driver)
+            self.share_instance, server)
         self.helper.db.share_instance_update.assert_called_once_with(
             self.context, self.share_instance['id'],
             {'status': constants.STATUS_INACTIVE})

manila/tests/share/test_rpcapi.py

@@ -39,7 +39,6 @@ class ShareRpcAPITestCase(test.TestCase):
             availability_zone=CONF.storage_availability_zone,
             status=constants.STATUS_AVAILABLE
         )
-        access = db_utils.create_access(share_id=share['id'])
         snapshot = db_utils.create_snapshot(share_id=share['id'])
         share_replica = db_utils.create_share_replica(
             id='fake_replica',
@@ -55,7 +54,6 @@ class ShareRpcAPITestCase(test.TestCase):
         # doesn't know about those extra attributes to pull in
         self.fake_share['instance'] = jsonutils.to_primitive(share.instance)
         self.fake_share_replica = jsonutils.to_primitive(share_replica)
-        self.fake_access = jsonutils.to_primitive(access)
         self.fake_snapshot = jsonutils.to_primitive(snapshot)
         self.fake_share_server = jsonutils.to_primitive(share_server)
         self.fake_cg = jsonutils.to_primitive(cg)
@@ -89,10 +87,6 @@ class ShareRpcAPITestCase(test.TestCase):
             snap = expected_msg['cgsnapshot']
             del expected_msg['cgsnapshot']
             expected_msg['cgsnapshot_id'] = snap['id']
-        if 'access' in expected_msg:
-            access = expected_msg['access']
-            del expected_msg['access']
-            expected_msg['access_rules'] = [access['id']]
         if 'host' in expected_msg:
             del expected_msg['host']
         if 'snapshot' in expected_msg:
@@ -113,6 +107,9 @@ class ShareRpcAPITestCase(test.TestCase):
         if 'src_share_instance' in expected_msg:
             share_instance = expected_msg.pop('src_share_instance', None)
             expected_msg['src_instance_id'] = share_instance['id']
+        if 'update_access' in expected_msg:
+            share_instance = expected_msg.pop('share_instance', None)
+            expected_msg['share_instance_id'] = share_instance['id']
 
         if 'host' in kwargs:
             host = kwargs['host']
@@ -177,19 +174,11 @@ class ShareRpcAPITestCase(test.TestCase):
                              share_instance=self.fake_share,
                              force=False)
 
-    def test_allow_access(self):
-        self._test_share_api('allow_access',
+    def test_update_access(self):
+        self._test_share_api('update_access',
                              rpc_method='cast',
-                             version='1.7',
-                             share_instance=self.fake_share,
-                             access=self.fake_access)
-
-    def test_deny_access(self):
-        self._test_share_api('deny_access',
-                             rpc_method='cast',
-                             version='1.7',
-                             share_instance=self.fake_share,
-                             access=self.fake_access)
+                             version='1.14',
+                             share_instance=self.fake_share)
 
     def test_create_snapshot(self):
         self._test_share_api('create_snapshot',

manila/tests/test_utils.py

@@ -691,8 +691,14 @@ class ShareMigrationHelperTestCase(test.TestCase):
     def test_wait_for_access_update(self):
         sid = 1
         fake_share_instances = [
-            {'id': sid, 'access_rules_status': constants.STATUS_OUT_OF_SYNC},
-            {'id': sid, 'access_rules_status': constants.STATUS_ACTIVE},
+            {
+                'id': sid,
+                'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING,
+            },
+            {
+                'id': sid,
+                'access_rules_status': constants.STATUS_ACTIVE,
+            },
         ]
 
         self.mock_object(time, 'sleep')
@@ -709,11 +715,17 @@ class ShareMigrationHelperTestCase(test.TestCase):
 
     @ddt.data(
         (
-            {'id': '1', 'access_rules_status': constants.STATUS_ERROR},
+            {
+                'id': '1',
+                'access_rules_status': constants.SHARE_INSTANCE_RULES_ERROR,
+            },
             exception.ShareMigrationFailed
         ),
         (
-            {'id': '1', 'access_rules_status': constants.STATUS_OUT_OF_SYNC},
+            {
+                'id': '1',
+                'access_rules_status': constants.SHARE_INSTANCE_RULES_SYNCING,
+            },
             exception.ShareMigrationFailed
         ),
     )

manila/utils.py

@@ -550,7 +550,8 @@ def wait_for_access_update(context, db, share_instance,
 
         tries += 1
         now = time.time()
-        if instance['access_rules_status'] == constants.STATUS_ERROR:
+        if (instance['access_rules_status'] ==
+                constants.SHARE_INSTANCE_RULES_ERROR):
             msg = _("Failed to update access rules"
                     " on share instance %s") % share_instance['id']
             raise exception.ShareMigrationFailed(reason=msg)

manila_tempest_tests/config.py

@@ -30,7 +30,7 @@ ShareGroup = [
                help="The minimum api microversion is configured to be the "
                     "value of the minimum microversion supported by Manila."),
     cfg.StrOpt("max_api_microversion",
-               default="2.27",
+               default="2.28",
                help="The maximum api microversion is configured to be the "
                     "value of the latest microversion supported by Manila."),
     cfg.StrOpt("region",

releasenotes/notes/bug-1626249-reintroduce-per-share-instance-access-rule-state-7c08a91373b21557.yaml

@@ -0,0 +1,24 @@
+---
+features:
+  - New micro-states ('applying', 'denying'), appear in the 'state' field
+    of access rules list API. These transitional states signify the state of
+    an access rule while its application or denial is being processed
+    asynchronously.
+  - Access rules can be added regardless of the 'access_rules_status'
+    of the share or any of its replicas.
+fixes:
+  - Fixed a bug with the share manager losing access rule updates when
+    multiple access rules are added to a given share simultaneously.
+  - Instead of all existing access rules transitioning to 'error' state
+    when some error occurs while applying or denying access rules to a given
+    share, only the rules that were in transitional statuses ('applying',
+    'denying') during an update will transition to 'error' state. This
+    change is expected to aid in identifying any 'bad' rules that require a
+    resolution by the user.
+  - Share action APIs dealing with allowing and denying access to shares now
+    perform the policy check for authorization to invoke those APIs as a
+    preliminary step.
+  - As before, when a share is replicated (or being migrated), all replicas
+    (or migration instances) of the share must be in a valid state in order
+    to allow or deny access to the share (where such actions are otherwise
+    allowed). The check enforcing this in the API is fixed.